GUI: Implement an "Unknown game dialog" #1160

lotharsm · 2018-04-16T15:49:51Z

Thanks to the great help of @criezy, here's my implementation of an GUI dialog that appears when an unknown game is detected.

Features:

Allows copying the data collected by game detector to the clipboard
Allows opening the bug tracker in the default web browser and pre-fills the form fiels

Feedback is greatly appreciated! As I'm still learning how to C++ and have little experience, don't hesitate to tell me if I did something awfully stupid. :)

@wjp: The option to automatically report the collected data unfortunately will fail ungracefully if the user is not logged in to ScummVM's Trac instance. There's also something wrong with https://bugs.scummvm.org/login which to my understanding would handle this case by a redirect.

This closes https://bugs.scummvm.org/ticket/10435.


        OSYSTEM: Add API to copy text to clipboard

This has also been implemented for the SDL2 and macOS backends.


        SDL: Handle encoding conversion for clipboard text


        OSX: Handle encoding conversion for clipboard text


        GUI: Add copy to clipboard shortcut for EditableWidget

bonki

Because buttons are only added when the underlying feature is supported this means that depending on the platform and libraries used the "Report game" button might show while the "Copy to clipboard" button in the middle might be missing which is a bit ugly. One solution would be to always add the buttons and have them greyed-out when not available, but that's not much better (or maybe worse) from a user's PoV. Best would be if the coordinates of said buttons would be dynamic to avoid the potential gap.

bonki · 2018-04-16T17:33:08Z

backends/platform/sdl/macosx/macosx_wrapper.h

@@ -27,6 +27,7 @@

 bool hasTextInClipboardMacOSX();
 Common::String getTextFromClipboardMacOSX();
+bool setTextInClipboardMacOSX(const Common::String&);


While optional, I believe we list parameter names in all prototypes.

We indeed seem to do it, at least in the common code. In the backend code this is probably less important. And as you can see since I wrote this part of the code, personally I tend not to name parameters when their meaning is obvious, and unless needed (for example to document them), but this might be a bad habit. I can fix that code when merging this PR.

bonki · 2018-04-16T17:33:08Z

backends/platform/sdl/sdl.h

@@ -72,6 +72,7 @@ class OSystem_SDL : public ModularBackend {
 	// Clipboard
 	virtual bool hasTextInClipboard();
 	virtual Common::String getTextFromClipboard();
+	virtual bool setTextInClipboard(const Common::String&);


bonki · 2018-04-16T17:33:08Z

engines/advancedDetector.cpp

+			"its version, language, etc.:",
+			path.getPath().c_str(), getName(), "https://bugs.scummvm.org/");
+
+	Common::String reportTranslated = Common::String::format(


You can avoid duplication by having the format string in a separate variable.

I don't know if this could be avoided since the message contains strings that could not be translated if I simply tried to do something like reportTranslated = _(report)...

I am talking about the format string only, something along the lines of:

const char *reportFormat = "The game in '%s' ..."; Common::String report = Common::String::format( reportFormat, ...); Common::String reportTranslated = Common::String::format(_(reportFormat), ...);

Thanks! Fixed.

bonki · 2018-04-16T17:33:08Z

engines/unknown-game-dialog.cpp

+	}
+	case kOpenBugtrackerURL:
+	{
+		g_system->openUrl("https://bugs.scummvm.org/newticket?summary=[UNK] Unknown game for engine " + _bugtrackerAffectedEngine + ": &description=" + _bugtrackerGameData + "&component=Engine%3A+"+_bugtrackerAffectedEngine+"&type=enhancement"+"&keywords=unknown-game "+_bugtrackerAffectedEngine);


Using String::format() would make this a bit more readable.

bonki · 2018-04-16T17:33:08Z

engines/advancedDetector.cpp

-
+			  path.getPath().c_str(), getName(), "https://bugs.scummvm.org/");
+
+	Common::String bugtrackerAffectedEngine = Common::String::format(getName());


No need for format() here.

lotharsm · 2018-04-16T17:47:30Z

@bonki You are right about the button positions, this was an oversight be me. What do you think about the following?

	//Check if we have clipboard functionality
	if (g_system->hasFeature(OSystem::kFeatureClipboardSupport)) {
	   new ButtonWidget(this, copyToClipboardButtonPos, _h - buttonHeight - 8, copyToClipboardButtonWidth, buttonHeight, _("Copy to clipboard"), 0, kCopyToClipboard);
	} else {
		   //Set the bug tracker position to the one of the disabled copy-to-clipboard button
		   openBugtrackerURLButtonPos = copyToClipboardButtonPos;
	}

bonki · 2018-04-16T17:59:58Z

engines/unknown-game-dialog.cpp

+
+	//Check if we have clipboard functionality
+	if (g_system->hasFeature(OSystem::kFeatureClipboardSupport)) {
+	   new ButtonWidget(this, copyToClipboardButtonPos, _h - buttonHeight - 8, copyToClipboardButtonWidth, buttonHeight, _("Copy to clipboard"), 0, kCopyToClipboard);


Sneaky spaces; should be a tab :)

bonki · 2018-04-16T18:02:23Z

@RootFather Not particularly pretty but it gets the job done as long as no new buttons are added to the dialog :)

bgK · 2018-04-16T18:40:38Z

engines/advancedDetector.cpp

+
+	// Show the UnknownGameDialog and print the translated unknown game information
+	GUI::UnknownGameDialog dialog(report, reportTranslated, bugtrackerAffectedEngine);
+	dialog.runModal();


I wonder what happens when invoking the detector from the command line using say --detect.

Hm, apparently it quits without printing anything. Unless I'm doing something wrong that's not quite ideal.

Fixed in b711c1e (code by @criezy)

criezy

Thank you for your work! Overall this looks good, but there are some minor changes to do.

You should also swap the order of commit 3c21b81 and b711c1e (to have b711c1e first). As it is now commit 3c21b81 will fail to compile since it uses the not yet added Singleton::hasInstance() function.

criezy · 2018-04-16T22:48:15Z

backends/platform/sdl/macosx/macosx_wrapper.h

@@ -27,6 +27,7 @@

 bool hasTextInClipboardMacOSX();
 Common::String getTextFromClipboardMacOSX();
+bool setTextInClipboardMacOSX(const Common::String&);


We indeed seem to do it, at least in the common code. In the backend code this is probably less important. And as you can see since I wrote this part of the code, personally I tend not to name parameters when their meaning is obvious, and unless needed (for example to document them), but this might be a bad habit. I can fix that code when merging this PR.

criezy · 2018-04-16T22:48:15Z

engines/advancedDetector.cpp

-			  "team at %s along with the name of the game you tried to add and "
-			  "its version, language, etc.:"),
-			path.getPath().c_str(), getName(), "https://bugs.scummvm.org/");
+	const char *reportCommon = "The game in '%s' seems to be an unknown %s engine game " \


Why the backslashes at the end of this line (and the next two lines)? They are not necessary.

criezy · 2018-04-16T22:48:15Z

engines/advancedDetector.cpp

-	g_system->logMessage(LogMessageType::kInfo, report.c_str());
+	// Write the original message about the unknown game to the log file
+	Common::String reportLog = report;
+	reportLog.wordWrap(80);


I understand why you moved the wordWrap(80) here as you don't want to wrap the text that goes to the clipboard or the bug tracker. But this ends up wrapping the unknown game file descriptions {name, MD5, size} while they were not wrapped before, and I don't think they should be wrapped.

Also we probably don't want to add the \n to the report and reportTranslated here, and only add it to the reportLog.

So I would suggest something like this:

reportTranslated.wordWrap(65); Common::String reportLog = report; reportLog.wordWrap(80); Common::String unknownFiles; for (ADFilePropertiesMap::const_iterator file = filesProps.begin(); file != filesProps.end(); ++file) unknownFiles += Common::String::format(" {\"%s\", 0, \"%s\", %d},\n", file->_key.c_str(), file->_value.md5.c_str(), file->_value.size); report += unknownFiles; reportTranslated += unknownFiles; reportLog += unknownFiles + "\n";

criezy · 2018-04-16T22:48:15Z

engines/module.mk

@@ -1,6 +1,7 @@
 MODULE := engines

 MODULE_OBJS := \
+	unknown-game-dialog.o \


I think we tend to list files alphabetically in those module.mk, so it was not inserted in the correct place.

criezy · 2018-04-16T22:48:15Z

engines/unknown-game-dialog.cpp

+
+	//Check if we have clipboard functionality and expand the reportTranslated message if needed...
+	if (g_system->hasFeature(OSystem::kFeatureClipboardSupport)) {
+		_reportTranslated += _("Use the button below to copy the required game information into your clipboard. \n");


If the \n was not added at the end of the reportTranslated (see my related comment for advancedDetector.cpp), then here it should be added at the start and not the end. And also I would not include it in the translated string:

if (g_system->hasFeature(OSystem::kFeatureClipboardSupport)) { _reportTranslated += "\n"; _reportTranslated += _("Use the button below to copy the required game information into your clipboard."); }

And do the same for the Open URL text below.
That way the end of lines would actually be consistent in all cases, whatever feature is available. As it is currently if the kFeatureOpenUrl is not available the _reportTranslated ends with a new line, which means you probably have an empty line at the end of the list of lines after the call to wordWrapText.

criezy · 2018-04-16T22:48:15Z

engines/unknown-game-dialog.cpp

+	Common::Array<Common::String> lines;
+	int maxlineWidth = g_gui.getFont().wordWrapText(_reportTranslated, screenW - 2 * 20, lines);
+
+	_w = MAX(MAX(maxlineWidth, 0), (2 * buttonWidth) + 10) + 20;


You probably want to use 3 * buttonWidth since you can have up to three buttons here. Or you could adapt it depending on the available features to not force having space for a button that will not be created.

Also you probably want to move the button widths computation above so that you can use it here since it can be wider than buttonWidth. This would end up looking like this:

int closeButtonWidth = MAX(buttonWidth, g_gui.getFont().getStringWidth(_("Close")) + 10); int copyToClipboardButtonWidth = MAX(buttonWidth, g_gui.getFont().getStringWidth(_("Copy to clipboard")) + 10); int openBugtrackerURLButtonWidth = MAX(buttonWidth, g_gui.getFont().getStringWidth(_("Report game")) + 10); int totalButtonWidth = closeButtonWidth; if (g_system->hasFeature(OSystem::kFeatureClipboardSupport)) totalButtonWidth += 10 + copyToClipboardButtonWidth; if (g_system->hasFeature(OSystem::kFeatureOpenUrl)) totalButtonWidth += 10 + openBugtrackerURLButtonWidth; _w = MAX(MAX(maxlineWidth, 0), totalButtonWidth) + 20;

criezy · 2018-04-16T22:48:15Z

engines/unknown-game-dialog.cpp

+#include "engines/unknown-game-dialog.h"
+#include "backends/platform/sdl/sdl.h"
+
+namespace GUI {


I know you initially added the file to gui/, and it made sense to use the GUI namespace then, but since now the file is in engines/ I don't think you should use the GUI namespace. Looking at the other dialogs in engines/dialogs.h they are in the global namespace.

So I would suggest to do the same and remove this line, then remove the closing bracket at the end of this file, and do the same in unknown-game-dialog.h.

criezy · 2018-04-16T22:48:15Z

engines/unknown-game-dialog.cpp

+		//Formatting the reportData for bugtracker submission [replace line breaks]...
+		_bugtrackerGameData = _reportData;
+		while (_bugtrackerGameData.contains("\n")) {
+			replace(_bugtrackerGameData, "\n", "%0A");


I am confused as I would not expect this to compile (unless there are using namespace Common; directives scattered around). Shouldn't this line be:

Common::replace(_bugtrackerGameData, "\n", "%0A");

It compiled, but I changed this to your convenience ;)

criezy · 2018-04-16T22:54:04Z

Since I created the ticket on our bug tracker, you can guess that I think this is a good idea :-P

In addition to the comments I made in the review, I am also wondering if we should use a ScrollContainerWidget in the dialog in case the report message is too big to be displayed on the screen.

Kirben · 2018-04-16T23:42:28Z

There are a few problems with the feature:

ScummVM still generates way too many false unknown versions, have you actually tried a mass add with this feature enabled? I currently get around 25 false positives in console window.
Auto generated bug report are a bad idea, as we need contact details to confirm the game is actually working after details for that specific game version have been added.
If the bug report system formatting changes in the future, the report option gets broken.

bgK

I would very much like this feature to happen as not properly informing the user when unknown game versions are encountered is a great usability problem. However I have some concerns which are probably out of the scope of this PR:

ScummVM invokes the detector each time a user starts a game. I think this dialog should only be shown when adding games.
Multiple engines may report unknown files in a single folder. This means the user will be presented with the dialog multiple times.
The dialog interrupts the mass detector instead of presenting the results once all the directories have been recursively scanned.
Games not using the AdvancedDetector don't show the new dialog which may be confusing for our users.
In my opinion there is a separation of concerns issue with the implementation. The detector should return the detection results to the caller (maybe it found known games, maybe it found unknown files, maybe it found both, maybe it found nothing). The caller (the add button, mass add, launch game, the command line, ...) should deal with the result in the appropriate way. Show a dialog if the GUI is active; accumulate results for the mass add button, show a console report for the command line.

I am also wondering if we should use a ScrollContainerWidget in the dialog in case the report message is too big to be displayed on the screen.

That sound like a good idea.

bgK · 2018-04-17T05:45:47Z

backends/platform/sdl/sdl.cpp

@@ -502,17 +503,42 @@ Common::String OSystem_SDL::getTextFromClipboard() {

 #if SDL_VERSION_ATLEAST(2, 0, 0)
 	char *text = SDL_GetClipboardText();
+#ifdef USE_TRANSLATION


It is my understanding we use ISO-8859-1 when USE_TRANSLATION is not defined. It's probably better to convert to that rather than using raw UTF-8 data which is bound to fail to render properly.
(the same applies to setTextInClipboard)

Right. And that would be a very simple change to do (here, and in the macOS backend).

bgK · 2018-04-17T05:45:47Z

engines/unknown-game-dialog.cpp

+	}
+	case kOpenBugtrackerURL:
+	{
+		Common::String bugtrackerURL = Common::String::format((


This code will make its way into ResidualVM which does not use a Trac bugtracker. I would appreciate if the code used to build the bugtracker URL was moved to a separate function so it can be overridden easily.

That can be done. Simply moving the URL building part into a separate function and then calling this function in case of kOpenBugtrackerURL?

That would be fine.

Does https://gist.github.com/rootfather/222ff1079263ea8073d39b5ac4659a8a matches with what you proposed?

Yes, sounds good.

bgK · 2018-04-17T05:45:47Z

engines/unknown-game-dialog.cpp

+
+	// Center the dialog
+	_x = (screenW - _w) / 2;
+	_y = (screenH - _h) / 2;


Centering the dialog needs to happen in the reflowLayout method so the dialog stays centered when resizing the window.

bgK · 2018-04-17T05:45:47Z

engines/unknown-game-dialog.cpp

+	int closeButtonPos = _w - closeButtonWidth - 10;
+	int copyToClipboardButtonPos = _w - copyToClipboardButtonWidth - closeButtonWidth - 15;
+	int openBugtrackerURLButtonPos = _w - copyToClipboardButtonWidth - closeButtonWidth - openBugtrackerURLButtonWidth - 20;
+	_closeButton = new ButtonWidget(this, closeButtonPos, _h - buttonHeight - 8, buttonWidth, buttonHeight, _("Close"), 0, kClose);


There's a bunch of manual GUI widget placement in this method. Did you consider describing the dialog in a layout file to automate this?

I haven't looked at the layouting/theming yet...

bgK · 2018-04-17T06:20:52Z

engines/unknown-game-dialog.cpp

+	case kCopyToClipboard:
+	{
+		g_system->setTextInClipboard(_reportData);
+		MessageDialog dialog(_("All necessary information about your game has been copied into the clipboard"), _("OK"));


Maybe use an OSD message so the user does not have to click to confirm the operation was successful.

Indeed. Implemented.

lotharsm · 2018-04-17T07:29:06Z

@Kirben

Auto generated bug report are a bad idea, as we need contact details to confirm the game is actually working after details for that specific game version have been added.

I see your point, so we should keep the requirement for the user to be logged in into the Trac instance. This ensures that we have a proper way to contact the submitter. This also means that just the /login redirect needs to be fixed.

If the bug report system formatting changes in the future, the report option gets broken.

Yep, that's true - maintaining this feature requires to change the URL building method if required.

@bgK

ScummVM invokes the detector each time a user starts a game. I think this dialog should only be shown when adding games.

I don't know why, but at the moment, the dialog is only shown when adding the games, not when starting them. After starting a game from the launcher, the game information is shown in the log window, but the dialog is not invoked...

Multiple engines may report unknown files in a single folder. This means the user will be presented with the dialog multiple times.
The dialog interrupts the mass detector instead of presenting the results once all the directories have been recursively scanned.

That's true, this should be refined.

Games not using the AdvancedDetector don't show the new dialog which may be confusing for our users.

This means that some of the changes made to AdvancedDetector have to be applied to the engines that rely on own detectors instead of AdvancedDetector?

In my opinion there is a separation of concerns issue with the implementation. The detector should return the detection results to the caller (maybe it found known games, maybe it found unknown files, maybe it found both, maybe it found nothing). The caller (the add button, mass add, launch game, the command line, ...) should deal with the result in the appropriate way. Show a dialog if the GUI is active; accumulate results for the mass add button, show a console report for the command line.

This might actually be out of scope for me know...


        COMMON: Add hasInstance() bool to singleton class


        ENGINES, GUI: Add a dialog for reporting unknown games

@criezy

Thanks to the great help of @criezy, here's my implementation of an GUI dialog that appears when an unknown game is detected. Features: - Allows copying the data collected by game detector to the clipboard - Allows opening the bug tracker and pre-filling the form fiels This closes https://bugs.scummvm.org/ticket/10435.


        GUI: Use OSD instead of message box for setTextInClipboard(_reportDat…

…a) confirmation


        ENGINES: Remove unnecessary backslashes from advancedDetector


        ENGINES: Fix alphabetical order in module.mk


        GUI: Improve newline consistency in unknownGameDialog


        GUI: Proper use of Common::replace in unknownGameDialog


        GUI: Fix centering of the unknownGameDialog


        ENGINES: Make sure unknown file hashes are not wrapped in logs

bgK · 2018-04-17T11:08:47Z

engines/unknown-game-dialog.cpp

+	case kCopyToClipboard:
+	{
+		g_system->setTextInClipboard(_reportData);
+		g_system->displayMessageOnOSD(_("All necessary information about your game has been copied into the clipboard"));


setTextInClipboard returns a boolean to indicate success or failure. It would be better to only show the success message if putting the text in the clipboard actually worked (And probably add a failure message).


        GUI: Add proper check if _reportData has been properly copied into th…

…e clipboard


        GUI: Move bugtracker URL generation to seperate function

sev- · 2018-04-17T17:10:08Z

I recommend to limit this functionality only when 'Add game' button is pressed. Not even Mass Add. Then it will make sense even creating tickets. For creating tickets, however, I would add another field: username, or e-mail address, or both, so we could trace the report back.

criezy · 2018-04-17T20:02:53Z

@Kirben @sev- There are some good points there.

Too many false unknown version
Hopefully limiting this dialog to the case where we Add a game (not when starting a game, and not for Mass Add) as suggested by sev should help with that. And I agree this is a good idea.
User contact
Yes, anonymous report is a bad idea. But since the bug tracker requiers users to be logged in to create a new ticket, this PR will not allow that. I am wondering here if requiring a GitHub account is not stopping a lot of reports, and maby we could accept other login bridges (for example using a Facebook account as we have on the translations website) but that is a separate issue. But we should not allow reports without contact information.
Bur report system future changes
That is something I am also worried about. I know nothing about websites, but would it be feasible and easy to have a redirect page on the web site that takes a couple of query items (engine and description) and reformat and redirect to whatever is our current bug tracker system? So the dialog would open for example https://www.scummvm.org/unknowngame?engine=Foo&description=Bar. Then if we change the bug tracker in the future we would just need to update the website redirect as well?

lotharsm · 2018-04-18T18:41:02Z

@criezy:
regarding 1: I'll see what I can do about the Mass Add feature. Currently, the dialog is not shown when a game is started, even if I don't understand why.

regarding 3: I know nothing about websites neither, but this seems like a good approach. Then, we won't render the current version of ScummVM unable to send bugreports and only would have to change the website redirect.


        GUI: Improve button width calculations in the unknown game dialog

@criezy

as suggested by @criezy.


        GUI: Remove the unknown game dialog out of namespace GUI::


        BASE, ENGINES, GUI: Show the unknown Game dialog only if the detector…

… was *not* launched by the Mass Add feature

criezy

There are various indentation issues that crept in with your last commit and need to be fixed. Otherwise the changes in that commit look good.

criezy · 2018-04-25T19:50:39Z

base/plugins.cpp

@@ -524,7 +524,7 @@ GameList EngineManager::detectGames(const Common::FSList &fslist) const {
 		// 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));
+                       candidates.push_back((*iter)->get<MetaEngine>().detectGames(fslist, useUnknownGameDialog));


It looks like you mistakenly replaced tab indentation with spaces on this line.

criezy · 2018-04-25T19:50:39Z

engines/advancedDetector.cpp

@@ -373,7 +373,7 @@ void AdvancedMetaEngine::reportUnknown(const Common::FSNode &path, const ADFileP
 	g_system->logMessage(LogMessageType::kInfo, reportLog.c_str());

 	// Check if the GUI is running, show the UnknownGameDialog and print the translated unknown game information
-	if (GUI::GuiManager::hasInstance() && g_gui.isActive()) {
+       if (GUI::GuiManager::hasInstance() && g_gui.isActive() && useUnknownGameDialog == true) {
 		UnknownGameDialog dialog(report, reportTranslated, bugtrackerAffectedEngine);


Here as well the indentation seems to have been changed to spaces.

Also the test on GUI::GuiManager::hasInstance() && g_gui.isActive() is no longer necessary since I don't see a case where they would fail while useUnknownGameDialog is true. But I don't see any issue with keeping those to be extra-safe.

Note that the == true is not necessary here either, but since our code formatting conventions don't say anything about this, I guess this is fine.

criezy · 2018-04-25T19:50:39Z

engines/advancedDetector.cpp

@@ -574,7 +574,7 @@ ADGameDescList AdvancedMetaEngine::detectGame(const Common::FSNode &parent, cons
 	// We didn't find a match
 	if (matched.empty()) {
 		if (!filesProps.empty() && gotAnyMatchesWithAllFiles) {
-			reportUnknown(parent, filesProps, matchedGameIds);
+                       reportUnknown(parent, filesProps, matchedGameIds, useUnknownGameDialog);


More tabs that magically transformed into spaces...

criezy · 2018-04-25T19:50:39Z

engines/advancedDetector.h

@@ -278,7 +278,7 @@ class AdvancedMetaEngine : public MetaEngine {

 	virtual GameDescriptor findGame(const char *gameId) const;

-	virtual GameList detectGames(const Common::FSList &fslist) const;
+       virtual GameList detectGames(const Common::FSList &fslist, bool useUnknownGameDialog = false) const;


Another indentation issue.

criezy · 2018-04-25T19:50:39Z

engines/advancedDetector.h

@@ -313,7 +313,7 @@ class AdvancedMetaEngine : public MetaEngine {
 	 * @param extra		restrict results to specified extra string (only if kADFlagUseExtraAsHint is set)
 	 * @return	list of ADGameDescription pointers corresponding to matched games
 	 */
-	virtual ADGameDescList detectGame(const Common::FSNode &parent, const FileMap &allFiles, Common::Language language, Common::Platform platform, const Common::String &extra) const;
+       virtual ADGameDescList detectGame(const Common::FSNode &parent, const FileMap &allFiles, Common::Language language, Common::Platform platform, const Common::String &extra, bool useUnknownGameDialog = false) const;


Another indentation issue.

criezy · 2018-04-25T19:50:39Z

engines/scumm/detection.cpp

@@ -961,7 +961,7 @@ class ScummMetaEngine : public MetaEngine {
 	virtual bool hasFeature(MetaEngineFeature f) const;
 	virtual GameList getSupportedGames() const;
 	virtual GameDescriptor findGame(const char *gameid) const;
-	virtual GameList detectGames(const Common::FSList &fslist) const;
+       virtual GameList detectGames(const Common::FSList &fslist, bool useUnknownGameDialog = false) const;


Another indentation issue.

criezy · 2018-04-25T19:50:39Z

engines/sky/detection.cpp

@@ -79,7 +79,7 @@ class SkyMetaEngine : public MetaEngine {
 	virtual GameList getSupportedGames() const;
 	virtual const ExtraGuiOptions getExtraGuiOptions(const Common::String &target) const;
 	virtual GameDescriptor findGame(const char *gameid) const;
-	virtual GameList detectGames(const Common::FSList &fslist) const;
+       virtual GameList detectGames(const Common::FSList &fslist, bool useUnknownGameDialog = false) const;


Another indentation issue.

criezy · 2018-04-25T19:50:39Z

engines/sword1/detection.cpp

@@ -89,7 +89,7 @@ class SwordMetaEngine : public MetaEngine {
 	virtual bool hasFeature(MetaEngineFeature f) const;
 	virtual GameList getSupportedGames() const;
 	virtual GameDescriptor findGame(const char *gameid) const;
-	virtual GameList detectGames(const Common::FSList &fslist) const;
+       virtual GameList detectGames(const Common::FSList &fslist, bool useUnknownGameDialog = false) const;


Another indentation issue.

criezy · 2018-04-25T19:50:39Z

engines/sword2/sword2.cpp

@@ -95,7 +95,7 @@ class Sword2MetaEngine : public MetaEngine {
 	virtual GameList getSupportedGames() const;
 	virtual const ExtraGuiOptions getExtraGuiOptions(const Common::String &target) const;
 	virtual GameDescriptor findGame(const char *gameid) const;
-	virtual GameList detectGames(const Common::FSList &fslist) const;
+       virtual GameList detectGames(const Common::FSList &fslist, bool useUnknownGameDialog = false) const;


Another indentation issue.

criezy · 2018-04-25T19:50:39Z

gui/launcher.cpp

@@ -573,7 +573,7 @@ bool LauncherDialog::doGameDetection(const Common::String &path) {

 	// ...so let's determine a list of candidates, games that
 	// could be contained in the specified directory.
-	GameList candidates(EngineMan.detectGames(files));
+       GameList candidates(EngineMan.detectGames(files, true));


Another indentation issue.

criezy · 2018-04-25T20:34:12Z

I will try to summarise ideas and issues that were discussed above.

The short version of it is that it seems to me that once the incorrect indentation have been fixed, and maybe the creation of the Report game button has been disabled, this code is good to be merged. Is there any objections? Do you see any other pending issues not listed below?

Formatting of clipboard-related commits:

I have cleaned the clipboard-related commits in my fork (you can see them at https://github.com/criezy/scummvm/commits/unknown-game-dialog). I am planning to use those to replace the ones in this pull request.

ScummVM still generates way too many false unknown versions
ScummVM invokes the detector each time a user starts a game
The dialog interrupts the mass detector

With the latest changes from @RootFather the dialog only appears when using the Add Game button. This means it is not used with Mass Add, when detecting from the command line, or when starting a game. This I think fixes the main issue that was highlighted by several people.

We could consider using the dialog for Mass Add as well if we can get it to only popup once at the end with all the unknown version found. But I think this is out of scope of this PR.

we need contact details to confirm the game is actually working
If the bug report system formatting changes in the future, the report option gets broken

As previously noted, the bug tracker forces users to be logged in to create a ticket, so we do get contact details.

I am wondering however if we should #if 0 the code that creates the Report game button and add a comment for now until we can figure out how to handle the URL in a nice way to be more future proof.

Multiple engines may report unknown files in a single folder. This means the user will be presented with the dialog multiple times
The detector should return the detection results to the caller

Yes, I think it would be a good idea to return unknown version information to the caller and not just detected games, and let the caller handle this information as it sees fit instead of reporting those from the detection code. However I think this is out of scope of this PR.

Use a ScrollContainerWidget

I propose to work on that once this PR has been merged.

bonki · 2018-04-25T22:21:30Z

We could consider using the dialog for Mass Add as well if we can get it to only popup once at the end with all the unknown version found. But I think this is out of scope of this PR.

I'm leaning a bit towards being against that since this list could potentially be very long and I'm not convinced that a ScrollContainerWidget would be a proper workaround from a UX angle in this case (that being said, using a ScrollContainerWidget sounds like a very good idea nonetheless).

I am wondering however if we should #if 0 the code that creates the Report game button and add a comment for now until we can figure out how to handle the URL in a nice way to be more future proof.

+1


        BASE, ENGINES, GUI: Fix indentation issues


        GUI: Disable the button for reporting a unknown game directly to the …

…bugtracker for now

criezy · 2018-04-29T20:50:25Z

I have now merged manually those changes after cleaning some commits and squashing most of them.
I have also added a couple of commits to use a ScrollContainerWidget and to properly resize and relayout the dialog (and not just center it) when the overlay size changes.

Thank you for your work on this.

criezy added 4 commits Apr 10, 2018

OSYSTEM: Add API to copy text to clipboard

f159e5c

This has also been implemented for the SDL2 and macOS backends.

SDL: Handle encoding conversion for clipboard text

26bff5c

OSX: Handle encoding conversion for clipboard text

d323776

GUI: Add copy to clipboard shortcut for EditableWidget

484c6d5

lotharsm requested review from criezy, wjp and sev- Apr 16, 2018

bonki requested changes Apr 16, 2018

View changes

bonki reviewed Apr 16, 2018

View changes

bgK reviewed Apr 16, 2018

View changes

lotharsm force-pushed the lotharsm:unknown-game-dialog branch 2 times, most recently from f083d6d to b711c1e Apr 16, 2018

criezy requested changes Apr 16, 2018

View changes

bgK reviewed Apr 17, 2018

View changes

lotharsm added 3 commits Apr 16, 2018

COMMON: Add hasInstance() bool to singleton class

9765468

GUI: Use OSD instead of message box for setTextInClipboard(_reportDat…

Loading status checks…

0721b0e

…a) confirmation

lotharsm force-pushed the lotharsm:unknown-game-dialog branch from b711c1e to 0721b0e Apr 17, 2018

lotharsm added 6 commits Apr 17, 2018

ENGINES: Remove unnecessary backslashes from advancedDetector

Loading status checks…

30f331c

ENGINES: Fix alphabetical order in module.mk

ec8e5f7

GUI: Improve newline consistency in unknownGameDialog

7781444

GUI: Proper use of Common::replace in unknownGameDialog

Loading status checks…

4875d29

GUI: Fix centering of the unknownGameDialog

Loading status checks…

e82feb6

ENGINES: Make sure unknown file hashes are not wrapped in logs

Loading status checks…

0c3c9c9

bgK reviewed Apr 17, 2018

View changes

lotharsm added 2 commits Apr 17, 2018

GUI: Add proper check if _reportData has been properly copied into th…

Loading status checks…

38094d8

…e clipboard

GUI: Move bugtracker URL generation to seperate function

Loading status checks…

16bc15b

lotharsm added 2 commits Apr 22, 2018

GUI: Improve button width calculations in the unknown game dialog

33670e1

as suggested by @criezy.

GUI: Remove the unknown game dialog out of namespace GUI::

Loading status checks…

ab0cb8f

lotharsm force-pushed the lotharsm:unknown-game-dialog branch from 9581bdc to ab0cb8f Apr 22, 2018

BASE, ENGINES, GUI: Show the unknown Game dialog only if the detector…

Loading status checks…

2364db3

… was *not* launched by the Mass Add feature

criezy requested changes Apr 25, 2018

View changes

bonki approved these changes Apr 25, 2018

View changes

lotharsm added 2 commits Apr 26, 2018

BASE, ENGINES, GUI: Fix indentation issues

d51de6f

GUI: Disable the button for reporting a unknown game directly to the …

Loading status checks…

6f80ba1

…bugtracker for now

criezy closed this Apr 29, 2018

lotharsm deleted the lotharsm:unknown-game-dialog branch Mar 30, 2019

criezy mentioned this pull request Apr 25, 2019

WEB: Add redirect for unknown game reports #97

Closed


		path.getPath().c_str(), getName(), "https://bugs.scummvm.org/");

		Common::String bugtrackerAffectedEngine = Common::String::format(getName());

scummvm / scummvm

Join GitHub today

GUI: Implement an "Unknown game dialog" #1160

GUI: Implement an "Unknown game dialog" #1160

Conversation

lotharsm commented Apr 16, 2018 • edited

bonki left a comment • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bonki Apr 16, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lotharsm commented Apr 16, 2018

This comment has been minimized.

This comment has been minimized.

bonki commented Apr 16, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lotharsm Apr 16, 2018 • edited

criezy left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

criezy commented Apr 16, 2018

Kirben commented Apr 16, 2018

bgK left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lotharsm Apr 17, 2018 • edited

lotharsm commented Apr 17, 2018

This comment has been minimized.

bgK Apr 17, 2018 • edited

sev- commented Apr 17, 2018

criezy commented Apr 17, 2018

lotharsm commented Apr 18, 2018

criezy left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

criezy commented Apr 25, 2018

bonki commented Apr 25, 2018

criezy commented Apr 29, 2018

lotharsm commented Apr 16, 2018 •

edited

bonki left a comment •

edited

bonki Apr 16, 2018 •

edited

bonki commented Apr 16, 2018 •

edited

lotharsm Apr 16, 2018 •

edited

lotharsm Apr 17, 2018 •

edited

bgK Apr 17, 2018 •

edited