GH-1453 handle certain version loading corner cases better, clean up FTB

4 files changed, 38 insertions, 88 deletions

diff --git a/logic/ftb/FTBProfileStrategy.cpp b/logic/ftb/FTBProfileStrategy.cpp
index ac55bbe7..0876c707 100644
--- a/logic/ftb/FTBProfileStrategy.cpp
+++ b/logic/ftb/FTBProfileStrategy.cpp
@@ -15,6 +15,8 @@ FTBProfileStrategy::FTBProfileStrategy(OneSixFTBInstance* instance) : OneSixProf
 
 void FTBProfileStrategy::loadDefaultBuiltinPatches()
 {
+	// FIXME: this should be here, but it needs us to be able to deal with multiple libraries paths
+	// OneSixProfileStrategy::loadDefaultBuiltinPatches();
 	auto mcVersion = m_instance->intendedVersionId();
 
 	ProfilePatchPtr minecraftPatch;
@@ -78,86 +80,18 @@ void FTBProfileStrategy::loadDefaultBuiltinPatches()
 					}
 				}
 			}
-			minecraftPatch = std::dynamic_pointer_cast<ProfilePatch>(file);
+			packPatch = std::dynamic_pointer_cast<ProfilePatch>(file);
 		}
 		else
 		{
 			throw VersionIncomplete("org.multimc.ftb.pack");
 		}
-		minecraftPatch->setOrder(1);
+		packPatch->setOrder(1);
 	}
-	profile->appendPatch(minecraftPatch);
+	profile->appendPatch(packPatch);
 
 }
 
-void FTBProfileStrategy::loadUserPatches()
-{
-	// load all patches, put into map for ordering, apply in the right order
-	ProfileUtils::PatchOrder userOrder;
-	ProfileUtils::readOverrideOrders(FS::PathCombine(m_instance->instanceRoot(), "order.json"), userOrder);
-	QDir patches(FS::PathCombine(m_instance->instanceRoot(),"patches"));
-
-	// first, load things by sort order.
-	for (auto id : userOrder)
-	{
-		// ignore builtins
-		if (id == "net.minecraft")
-			continue;
-		if (id == "org.lwjgl")
-			continue;
-		// parse the file
-		QString filename = patches.absoluteFilePath(id + ".json");
-		QFileInfo finfo(filename);
-		if(!finfo.exists())
-		{
-			qDebug() << "Patch file " << filename << " was deleted by external means...";
-			continue;
-		}
-		qDebug() << "Reading" << filename << "by user order";
-		auto file = ProfileUtils::parseJsonFile(finfo, false);
-		// sanity check. prevent tampering with files.
-		if (file->fileId != id)
-		{
-			throw VersionBuildError(
-				QObject::tr("load id %1 does not match internal id %2").arg(id, file->fileId));
-		}
-		file->setRemovable(true);
-		file->setMovable(true);
-		profile->appendPatch(file);
-	}
-	// now load the rest by internal preference.
-	QMap<int, QPair<QString, VersionFilePtr>> files;
-	for (auto info : patches.entryInfoList(QStringList() << "*.json", QDir::Files))
-	{
-		// parse the file
-		qDebug() << "Reading" << info.fileName();
-		auto file = ProfileUtils::parseJsonFile(info, true);
-		// ignore builtins
-		if (file->fileId == "net.minecraft")
-			continue;
-		if (file->fileId == "org.lwjgl")
-			continue;
-		// do not load what we already loaded in the first pass
-		if (userOrder.contains(file->fileId))
-			continue;
-		if (files.contains(file->order))
-		{
-			// FIXME: do not throw?
-			throw VersionBuildError(QObject::tr("%1 has the same order as %2")
-										.arg(file->fileId, files[file->order].second->fileId));
-		}
-		file->setRemovable(true);
-		file->setMovable(true);
-		files.insert(file->order, qMakePair(info.fileName(), file));
-	}
-	for (auto order : files.keys())
-	{
-		auto &filePair = files[order];
-		profile->appendPatch(filePair.second);
-	}
-}
-
-
 void FTBProfileStrategy::load()
 {
 	profile->clearPatches();
diff --git a/logic/ftb/FTBProfileStrategy.h b/logic/ftb/FTBProfileStrategy.h
index 5e5c2e73..4810b9de 100644
--- a/logic/ftb/FTBProfileStrategy.h
+++ b/logic/ftb/FTBProfileStrategy.h
@@ -17,6 +17,5 @@ public:
 	virtual bool revertPatch (ProfilePatchPtr patch) override;
 
 protected:
-	void loadDefaultBuiltinPatches();
-	void loadUserPatches();
+	virtual void loadDefaultBuiltinPatches() override;
 };
diff --git a/logic/minecraft/OneSixProfileStrategy.cpp b/logic/minecraft/OneSixProfileStrategy.cpp
index 85e9491c..3e3534a0 100644
--- a/logic/minecraft/OneSixProfileStrategy.cpp
+++ b/logic/minecraft/OneSixProfileStrategy.cpp
@@ -140,6 +140,7 @@ void OneSixProfileStrategy::loadUserPatches()
 	ProfileUtils::PatchOrder userOrder;
 	ProfileUtils::readOverrideOrders(FS::PathCombine(m_instance->instanceRoot(), "order.json"), userOrder);
 	QDir patches(FS::PathCombine(m_instance->instanceRoot(),"patches"));
+	QSet<QString> seen_extra;
 
 	// first, load things by sort order.
 	for (auto id : userOrder)
@@ -158,19 +159,19 @@ void OneSixProfileStrategy::loadUserPatches()
 			continue;
 		}
 		qDebug() << "Reading" << filename << "by user order";
-		auto file = ProfileUtils::parseJsonFile(finfo, false);
+		VersionFilePtr file = ProfileUtils::parseJsonFile(finfo, false);
 		// sanity check. prevent tampering with files.
 		if (file->fileId != id)
 		{
-			throw VersionBuildError(
-				QObject::tr("load id %1 does not match internal id %2").arg(id, file->fileId));
+			file->addProblem(PROBLEM_WARNING, QObject::tr("load id %1 does not match internal id %2").arg(id, file->fileId));
+			seen_extra.insert(file->fileId);
 		}
 		file->setRemovable(true);
 		file->setMovable(true);
 		profile->appendPatch(file);
 	}
 	// now load the rest by internal preference.
-	QMap<int, QPair<QString, VersionFilePtr>> files;
+	QMultiMap<int, VersionFilePtr> files;
 	for (auto info : patches.entryInfoList(QStringList() << "*.json", QDir::Files))
 	{
 		// parse the file
@@ -181,23 +182,39 @@ void OneSixProfileStrategy::loadUserPatches()
 			continue;
 		if (file->fileId == "org.lwjgl")
 			continue;
+		// do not load versions with broken IDs twice
+		if(seen_extra.contains(file->fileId))
+			continue;
 		// do not load what we already loaded in the first pass
 		if (userOrder.contains(file->fileId))
 			continue;
-		if (files.contains(file->order))
-		{
-			// FIXME: do not throw?
-			throw VersionBuildError(QObject::tr("%1 has the same order as %2")
-										.arg(file->fileId, files[file->order].second->fileId));
-		}
 		file->setRemovable(true);
 		file->setMovable(true);
-		files.insert(file->order, qMakePair(info.fileName(), file));
+		files.insert(file->order, file);
 	}
+	QSet<int> seen;
 	for (auto order : files.keys())
 	{
-		auto &filePair = files[order];
-		profile->appendPatch(filePair.second);
+		if(seen.contains(order))
+			continue;
+		seen.insert(order);
+		const auto &values = files.values(order);
+		if(values.size() == 1)
+		{
+			profile->appendPatch(values[0]);
+			continue;
+		}
+		for(auto &file: values)
+		{
+			QStringList list;
+			for(auto &file2: values)
+			{
+				if(file != file2)
+					list.append(file2->name);
+			}
+			file->addProblem(PROBLEM_WARNING, QObject::tr("%1 has the same order as the following components:\n%2").arg(file->name, list.join(", ")));
+			profile->appendPatch(file);
+		}
 	}
 }
 
diff --git a/logic/minecraft/OneSixProfileStrategy.h b/logic/minecraft/OneSixProfileStrategy.h
index b140dee5..a6abe964 100644
--- a/logic/minecraft/OneSixProfileStrategy.h
+++ b/logic/minecraft/OneSixProfileStrategy.h
@@ -17,8 +17,8 @@ public:
 	virtual bool revertPatch(ProfilePatchPtr patch) override;
 
 protected:
-	void loadDefaultBuiltinPatches();
-	void loadUserPatches();
+	virtual void loadDefaultBuiltinPatches();
+	virtual void loadUserPatches();
 	void upgradeDeprecatedFiles();
 
 protected: