Merge pull request #1187 from bgK/detection-refactor-unknown

ENGINES: Return unknown game variants with the list of detected games

3 files changed, 153 insertions, 103 deletions

diff --git a/base/commandLine.cpp b/base/commandLine.cpp
index 83c7b56171..96548b9129 100644
--- a/base/commandLine.cpp
+++ b/base/commandLine.cpp
@@ -687,9 +687,9 @@ static void listGames() {
 
 	const PluginList &plugins = EngineMan.getPlugins();
 	for (PluginList::const_iterator iter = plugins.begin(); iter != plugins.end(); ++iter) {
-		GameList list = (*iter)->get<MetaEngine>().getSupportedGames();
-		for (GameList::iterator v = list.begin(); v != list.end(); ++v) {
-			printf("%-20s %s\n", v->gameid().c_str(), v->description().c_str());
+		PlainGameList list = (*iter)->get<MetaEngine>().getSupportedGames();
+		for (PlainGameList::iterator v = list.begin(); v != list.end(); ++v) {
+			printf("%-20s %s\n", v->gameId, v->description);
 		}
 	}
 }
@@ -714,10 +714,10 @@ static void listTargets() {
 			// FIXME: At this point, we should check for a "gameid" override
 			// to find the proper desc. In fact, the platform probably should
 			// be taken into account, too.
-			Common::String gameid(name);
-			GameDescriptor g = EngineMan.findGame(gameid);
-			if (g.description().size() > 0)
-				description = g.description();
+			const Common::String &gameid = name;
+			PlainGameDescriptor g = EngineMan.findGame(gameid);
+			if (g.description)
+				description = g.description;
 		}
 
 		targets.push_back(Common::String::format("%-20s %s", name.c_str(), description.c_str()));
@@ -770,7 +770,7 @@ static Common::Error listSaves(const Common::String &target) {
 
 		// Find the plugin that will handle the specified gameid
 		const Plugin *plugin = nullptr;
-		GameDescriptor game = EngineMan.findGame(gameid, &plugin);
+		EngineMan.findGame(gameid, &plugin);
 
 		if (!plugin) {
 			// If the target was specified, treat this as an error, and otherwise skip it.
@@ -854,61 +854,36 @@ static void listAudioDevices() {
 }
 
 /** Display all games in the given directory, or current directory if empty */
-static GameList getGameList(const Common::FSNode &dir) {
+static DetectedGames getGameList(const Common::FSNode &dir) {
 	Common::FSList files;
 
 	// Collect all files from directory
 	if (!dir.getChildren(files, Common::FSNode::kListAll)) {
 		printf("Path %s does not exist or is not a directory.\n", dir.getPath().c_str());
-		return GameList();
+		return DetectedGames();
 	}
 
 	// detect Games
-	GameList candidates(EngineMan.detectGames(files));
-	Common::String dataPath = dir.getPath();
-	// add game data path
-	for (GameList::iterator v = candidates.begin(); v != candidates.end(); ++v) {
-		(*v)["path"] = dataPath;
-	}
-	return candidates;
-}
-
-static bool addGameToConf(const GameDescriptor &gd) {
-	const Common::String &domain = gd.preferredtarget();
+	DetectionResults detectionResults = EngineMan.detectGames(files);
 
-	// If game has already been added, don't add
-	if (ConfMan.hasGameDomain(domain))
-		return false;
-
-	// Add the name domain
-	ConfMan.addGameDomain(domain);
-
-	// Copy all non-empty key/value pairs into the new domain
-	for (GameDescriptor::const_iterator iter = gd.begin(); iter != gd.end(); ++iter) {
-		if (!iter->_value.empty() && iter->_key != "preferredtarget")
-			ConfMan.set(iter->_key, iter->_value, domain);
+	if (detectionResults.foundUnknownGames()) {
+		Common::String report = detectionResults.generateUnknownGameReport(false, 80);
+		g_system->logMessage(LogMessageType::kInfo, report.c_str());
 	}
 
-	// Display added game info
-	printf("Game Added: \n  GameID:   %s\n  Name:     %s\n  Language: %s\n  Platform: %s\n",
-			gd.gameid().c_str(),
-			gd.description().c_str(),
-			Common::getLanguageDescription(gd.language()),
-			Common::getPlatformDescription(gd.platform()));
-
-	return true;
+	return detectionResults.listRecognizedGames();
 }
 
-static GameList recListGames(const Common::FSNode &dir, const Common::String &gameId, bool recursive) {
-	GameList list = getGameList(dir);
+static DetectedGames recListGames(const Common::FSNode &dir, const Common::String &gameId, bool recursive) {
+	DetectedGames list = getGameList(dir);
 
 	if (recursive) {
 		Common::FSList files;
 		dir.getChildren(files, Common::FSNode::kListDirectoriesOnly);
 		for (Common::FSList::const_iterator file = files.begin(); file != files.end(); ++file) {
-			GameList rec = recListGames(*file, gameId, recursive);
-			for (GameList::const_iterator game = rec.begin(); game != rec.end(); ++game) {
-				if (gameId.empty() || game->gameid().c_str() == gameId)
+			DetectedGames rec = recListGames(*file, gameId, recursive);
+			for (DetectedGames::const_iterator game = rec.begin(); game != rec.end(); ++game) {
+				if (gameId.empty() || game->gameId == gameId)
 					list.push_back(*game);
 			}
 		}
@@ -922,7 +897,7 @@ static Common::String detectGames(const Common::String &path, const Common::Stri
 	bool noPath = path.empty();
 	//Current directory
 	Common::FSNode dir(path);
-	GameList candidates = recListGames(dir, gameId, recursive);
+	DetectedGames candidates = recListGames(dir, gameId, recursive);
 
 	if (candidates.empty()) {
 		printf("WARNING: ScummVM could not find any game in %s\n", dir.getPath().c_str());
@@ -937,25 +912,34 @@ static Common::String detectGames(const Common::String &path, const Common::Stri
 	// TODO this is not especially pretty
 	printf("ID             Description                                                Full Path\n");
 	printf("-------------- ---------------------------------------------------------- ---------------------------------------------------------\n");
-	for (GameList::iterator v = candidates.begin(); v != candidates.end(); ++v) {
-		printf("%-14s %-58s %s\n", v->gameid().c_str(), v->description().c_str(), (*v)["path"].c_str());
+	for (DetectedGames::const_iterator v = candidates.begin(); v != candidates.end(); ++v) {
+		printf("%-14s %-58s %s\n", v->gameId.c_str(), v->description.c_str(), v->path.c_str());
 	}
 
-	return candidates[0].gameid();
+	return candidates[0].gameId;
 }
 
 static int recAddGames(const Common::FSNode &dir, const Common::String &game, bool recursive) {
 	int count = 0;
-	GameList list = getGameList(dir);
-	for (GameList::iterator v = list.begin(); v != list.end(); ++v) {
-		if (v->gameid().c_str() != game && !game.empty()) {
-			printf("Found %s, only adding %s per --game option, ignoring...\n", v->gameid().c_str(), game.c_str());
-		} else if (!addGameToConf(*v)) {
-			// TODO Is it reall the case that !addGameToConf iff already added?
-			printf("Found %s, but has already been added, skipping\n", v->gameid().c_str());
+	DetectedGames list = getGameList(dir);
+	for (DetectedGames::const_iterator v = list.begin(); v != list.end(); ++v) {
+		if (v->gameId != game && !game.empty()) {
+			printf("Found %s, only adding %s per --game option, ignoring...\n", v->gameId.c_str(), game.c_str());
+		} else if (ConfMan.hasGameDomain(v->preferredTarget)) {
+			// TODO Better check for game already added?
+			printf("Found %s, but has already been added, skipping\n", v->gameId.c_str());
 		} else {
-			printf("Found %s, adding...\n", v->gameid().c_str());
+			Common::String target = EngineMan.createTargetForGame(*v);
 			count++;
+
+			// Display added game info
+			printf("Game Added: \n  Target:   %s\n  GameID:   %s\n  Name:     %s\n  Language: %s\n  Platform: %s\n",
+			       target.c_str(),
+			       v->gameId.c_str(),
+			       v->description.c_str(),
+			       Common::getLanguageDescription(v->language),
+			       Common::getPlatformDescription(v->platform)
+			);
 		}
 	}
 
@@ -1014,11 +998,13 @@ static void runDetectorTest() {
 			continue;
 		}
 
-		GameList candidates(EngineMan.detectGames(files));
+		DetectionResults detectionResults = EngineMan.detectGames(files);
+		DetectedGames candidates = detectionResults.listRecognizedGames();
+
 		bool gameidDiffers = false;
-		GameList::iterator x;
+		DetectedGames::const_iterator x;
 		for (x = candidates.begin(); x != candidates.end(); ++x) {
-			gameidDiffers |= (scumm_stricmp(gameid.c_str(), x->gameid().c_str()) != 0);
+			gameidDiffers |= (scumm_stricmp(gameid.c_str(), x->gameId.c_str()) != 0);
 		}
 
 		if (candidates.empty()) {
@@ -1041,10 +1027,10 @@ static void runDetectorTest() {
 
 		for (x = candidates.begin(); x != candidates.end(); ++x) {
 			printf("    gameid '%s', desc '%s', language '%s', platform '%s'\n",
-				   x->gameid().c_str(),
-				   x->description().c_str(),
-				   Common::getLanguageCode(x->language()),
-				   Common::getPlatformCode(x->platform()));
+				   x->gameId.c_str(),
+				   x->description.c_str(),
+				   Common::getLanguageDescription(x->language),
+				   Common::getPlatformDescription(x->platform));
 		}
 	}
 	int total = domains.size();
@@ -1092,24 +1078,26 @@ void upgradeTargets() {
 		Common::Platform plat = Common::parsePlatform(dom.getVal("platform"));
 		Common::String desc(dom.getVal("description"));
 
-		GameList candidates(EngineMan.detectGames(files));
-		GameDescriptor *g = 0;
+		DetectionResults detectionResults = EngineMan.detectGames(files);
+		DetectedGames candidates = detectionResults.listRecognizedGames();
+
+		DetectedGame *g = 0;
 
 		// We proceed as follows:
 		// * If detection failed to produce candidates, skip.
 		// * If there is a unique detector match, trust it.
 		// * If there are multiple match, run over them comparing gameid, language and platform.
 		//   If we end up with a unique match, use it. Otherwise, skip.
-		if (candidates.size() == 0) {
+		if (candidates.empty()) {
 			printf(" ... failed to detect game, skipping\n");
 			continue;
 		}
 		if (candidates.size() > 1) {
 			// Scan over all candidates, check if there is a unique match for gameid, language and platform
-			GameList::iterator x;
+			DetectedGames::iterator x;
 			int matchesFound = 0;
 			for (x = candidates.begin(); x != candidates.end(); ++x) {
-				if (x->gameid() == gameid && x->language() == lang && x->platform() == plat) {
+				if (x->gameId == gameid && x->language == lang && x->platform == plat) {
 					matchesFound++;
 					g = &(*x);
 				}
@@ -1127,27 +1115,27 @@ void upgradeTargets() {
 		// the target referred to by dom. We update several things
 
 		// Always set the gameid explicitly (in case of legacy targets)
-		dom["gameid"] = g->gameid();
+		dom["gameid"] = g->gameId;
 
 		// Always set the GUI options. The user should not modify them, and engines might
 		// gain more features over time, so we want to keep this list up-to-date.
-		if (g->contains("guioptions")) {
-			printf("  -> update guioptions to '%s'\n", (*g)["guioptions"].c_str());
-			dom["guioptions"] = (*g)["guioptions"];
+		if (!g->getGUIOptions().empty()) {
+			printf("  -> update guioptions to '%s'\n", g->getGUIOptions().c_str());
+			dom["guioptions"] = g->getGUIOptions();
 		} else if (dom.contains("guioptions")) {
 			dom.erase("guioptions");
 		}
 
 		// Update the language setting but only if none has been set yet.
-		if (lang == Common::UNK_LANG && g->language() != Common::UNK_LANG) {
-			printf("  -> set language to '%s'\n", Common::getLanguageCode(g->language()));
-			dom["language"] = (*g)["language"];
+		if (lang == Common::UNK_LANG && g->language != Common::UNK_LANG) {
+			printf("  -> set language to '%s'\n", Common::getLanguageCode(g->language));
+			dom["language"] = Common::getLanguageCode(g->language);
 		}
 
 		// Update the platform setting but only if none has been set yet.
-		if (plat == Common::kPlatformUnknown && g->platform() != Common::kPlatformUnknown) {
-			printf("  -> set platform to '%s'\n", Common::getPlatformCode(g->platform()));
-			dom["platform"] = (*g)["platform"];
+		if (plat == Common::kPlatformUnknown && g->platform != Common::kPlatformUnknown) {
+			printf("  -> set platform to '%s'\n", Common::getPlatformCode(g->platform));
+			dom["platform"] = Common::getPlatformCode(g->platform);
 		}
 
 		// TODO: We could also update the description. But not everybody will want that.
@@ -1156,8 +1144,8 @@ void upgradeTargets() {
 		// should only be updated if the user explicitly requests this.
 #if 0
 		if (desc != g->description()) {
-			printf("  -> update desc from '%s' to\n                      '%s' ?\n", desc.c_str(), g->description().c_str());
-			dom["description"] = (*g)["description"];
+			printf("  -> update desc from '%s' to\n                      '%s' ?\n", desc.c_str(), g->description.c_str());
+			dom["description"] = g->description;
 		}
 #endif
 	}
@@ -1254,8 +1242,8 @@ bool processSettings(Common::String &command, Common::StringMap &settings, Commo
 	// domain (i.e. a target) matching this argument, or alternatively
 	// whether there is a gameid matching that name.
 	if (!command.empty()) {
-		GameDescriptor gd = EngineMan.findGame(command);
-		if (ConfMan.hasGameDomain(command) || !gd.gameid().empty()) {
+		PlainGameDescriptor gd = EngineMan.findGame(command);
+		if (ConfMan.hasGameDomain(command) || gd.gameId) {
 			bool idCameFromCommandLine = false;
 
 			// WORKAROUND: Fix for bug #1719463: "DETECTOR: Launching
diff --git a/base/main.cpp b/base/main.cpp
index 8e783c9776..385b8f35a9 100644
--- a/base/main.cpp
+++ b/base/main.cpp
@@ -128,13 +128,13 @@ static const Plugin *detectPlugin() {
 	printf("User picked target '%s' (gameid '%s')...\n", ConfMan.getActiveDomainName().c_str(), gameid.c_str());
 	printf("  Looking for a plugin supporting this gameid... ");
 
- 	GameDescriptor game = EngineMan.findGame(gameid, &plugin);
+	PlainGameDescriptor game = EngineMan.findGame(gameid, &plugin);
 
 	if (plugin == 0) {
 		printf("failed\n");
 		warning("%s is an invalid gameid. Use the --list-games option to list supported gameid", gameid.c_str());
 	} else {
-		printf("%s\n  Starting '%s'\n", plugin->getName(), game.description().c_str());
+		printf("%s\n  Starting '%s'\n", plugin->getName(), game.description);
 	}
 
 	return plugin;
@@ -210,7 +210,10 @@ static Common::Error runGame(const Plugin *plugin, OSystem &system, const Common
 	Common::String caption(ConfMan.get("description"));
 
 	if (caption.empty()) {
-		caption = EngineMan.findGame(ConfMan.get("gameid")).description();
+		PlainGameDescriptor game = EngineMan.findGame(ConfMan.get("gameid"));
+		if (game.description) {
+			caption = game.description;
+		}
 	}
 	if (caption.empty())
 		caption = ConfMan.getActiveDomainName(); // Use the domain (=target) name
diff --git a/base/plugins.cpp b/base/plugins.cpp
index 852786919b..023f2f3bb3 100644
--- a/base/plugins.cpp
+++ b/base/plugins.cpp
@@ -22,6 +22,7 @@
 
 #include "base/plugins.h"
 
+#include "common/translation.h"
 #include "common/func.h"
 #include "common/debug.h"
 #include "common/config-manager.h"
@@ -457,13 +458,11 @@ DECLARE_SINGLETON(EngineManager);
  * For the uncached version, we first try to find the plugin using the gameId
  * and only if we can't find it there, we loop through the plugins.
  **/
-GameDescriptor EngineManager::findGame(const Common::String &gameName, const Plugin **plugin) const {
-	GameDescriptor result;
-
+PlainGameDescriptor EngineManager::findGame(const Common::String &gameName, const Plugin **plugin) const {
 	// First look for the game using the plugins in memory. This is critical
 	// for calls coming from inside games
-	result = findGameInLoadedPlugins(gameName, plugin);
-	if (!result.gameid().empty()) {
+	PlainGameDescriptor result = findGameInLoadedPlugins(gameName, plugin);
+	if (result.gameId) {
 		return result;
 	}
 
@@ -471,7 +470,7 @@ GameDescriptor EngineManager::findGame(const Common::String &gameName, const Plu
 	// by plugin
 	if (PluginMan.loadPluginFromGameId(gameName))  {
 		result = findGameInLoadedPlugins(gameName, plugin);
-		if (!result.gameid().empty()) {
+		if (result.gameId) {
 			return result;
 		}
 	}
@@ -480,7 +479,7 @@ GameDescriptor EngineManager::findGame(const Common::String &gameName, const Plu
 	PluginMan.loadFirstPlugin();
 	do {
 		result = findGameInLoadedPlugins(gameName, plugin);
-		if (!result.gameid().empty()) {
+		if (result.gameId) {
 			// Update with new plugin file name
 			PluginMan.updateConfigWithFileName(gameName);
 			break;
@@ -493,10 +492,9 @@ GameDescriptor EngineManager::findGame(const Common::String &gameName, const Plu
 /**
  * Find the game within the plugins loaded in memory
  **/
-GameDescriptor EngineManager::findGameInLoadedPlugins(const Common::String &gameName, const Plugin **plugin) const {
+PlainGameDescriptor EngineManager::findGameInLoadedPlugins(const Common::String &gameName, const Plugin **plugin) const {
 	// Find the GameDescriptor for this target
 	const PluginList &plugins = getPlugins();
-	GameDescriptor result;
 
 	if (plugin)
 		*plugin = 0;
@@ -504,18 +502,20 @@ GameDescriptor EngineManager::findGameInLoadedPlugins(const Common::String &game
 	PluginList::const_iterator iter;
 
 	for (iter = plugins.begin(); iter != plugins.end(); ++iter) {
-		result = (*iter)->get<MetaEngine>().findGame(gameName.c_str());
-		if (!result.gameid().empty()) {
+		PlainGameDescriptor pgd = (*iter)->get<MetaEngine>().findGame(gameName.c_str());
+		if (pgd.gameId) {
 			if (plugin)
 				*plugin = *iter;
-			return result;
+			return pgd;
 		}
 	}
-	return result;
+
+	return PlainGameDescriptor::empty();
 }
 
-GameList EngineManager::detectGames(const Common::FSList &fslist, bool useUnknownGameDialog) const {
-	GameList candidates;
+DetectionResults EngineManager::detectGames(const Common::FSList &fslist) const {
+	DetectedGames candidates;
+	Common::String path = fslist.begin()->getParent().getPath();
 	PluginList plugins;
 	PluginList::const_iterator iter;
 	PluginManager::instance().loadFirstPlugin();
@@ -524,16 +524,75 @@ GameList EngineManager::detectGames(const Common::FSList &fslist, bool useUnknow
 		// Iterate over all known games and for each check if it might be
 		// the game in the presented directory.
 		for (iter = plugins.begin(); iter != plugins.end(); ++iter) {
-			candidates.push_back((*iter)->get<MetaEngine>().detectGames(fslist, useUnknownGameDialog));
+			const MetaEngine &metaEngine = (*iter)->get<MetaEngine>();
+			DetectedGames engineCandidates = metaEngine.detectGames(fslist);
+
+			for (uint i = 0; i < engineCandidates.size(); i++) {
+				engineCandidates[i].engineName = metaEngine.getName();
+				engineCandidates[i].path = path;
+				candidates.push_back(engineCandidates[i]);
+			}
+
 		}
 	} while (PluginManager::instance().loadNextPlugin());
-	return candidates;
+
+	return DetectionResults(candidates);
 }
 
 const PluginList &EngineManager::getPlugins() const {
 	return PluginManager::instance().getPlugins(PLUGIN_TYPE_ENGINE);
 }
 
+namespace {
+
+void addStringToConf(const Common::String &key, const Common::String &value, const Common::String &domain) {
+	if (!value.empty())
+		ConfMan.set(key, value, domain);
+}
+
+} // End of anonymous namespace
+
+Common::String EngineManager::createTargetForGame(const DetectedGame &game) {
+	// The auto detector or the user made a choice.
+	// Pick a domain name which does not yet exist (after all, we
+	// are *adding* a game to the config, not replacing).
+	Common::String domain = game.preferredTarget;
+
+	assert(!domain.empty());
+	if (ConfMan.hasGameDomain(domain)) {
+		int suffixN = 1;
+		Common::String gameid(domain);
+
+		while (ConfMan.hasGameDomain(domain)) {
+			domain = gameid + Common::String::format("-%d", suffixN);
+			suffixN++;
+		}
+	}
+
+	// Add the name domain
+	ConfMan.addGameDomain(domain);
+
+	// Copy all non-empty relevant values into the new domain
+	addStringToConf("gameid", game.gameId, domain);
+	addStringToConf("description", game.description, domain);
+	addStringToConf("language", Common::getLanguageCode(game.language), domain);
+	addStringToConf("platform", Common::getPlatformCode(game.platform), domain);
+	addStringToConf("path", game.path, domain);
+	addStringToConf("extra", game.extra, domain);
+	addStringToConf("guioptions", game.getGUIOptions(), domain);
+
+	// TODO: Setting the description field here has the drawback
+	// that the user does never notice when we upgrade our descriptions.
+	// It might be nice to leave this field empty, and only set it to
+	// a value when the user edits the description string.
+	// However, at this point, that's impractical. Once we have a method
+	// to query all backends for the proper & full description of a given
+	// game target, we can change this (currently, you can only query
+	// for the generic gameid description; it's not possible to obtain
+	// a description which contains extended information like language, etc.).
+
+	return domain;
+}
 
 // Music plugins