GUI: Indirectly changing 'GUI Language' can produce inconsistent behaviour when changing some options.

[macca8]

This bug was revealed when the 'restart' message was replaced with the 'Apply' button implementation.

Platform: Intel Mac (OS X 10.6.8)
Language (OS default): English

First detected: DB 1.10.0git681-gc32be8e (4 Nov 2016)
Tested with current Daily Build from website: 1.10.0git2760-g6e3ffd3 (13 Mch 2017)

Bug details:
This bug is created whenever changing a global option also indirectly changes the 'GUI Language' setting from 'default' to English.
It appears to be self healing, in that when it occurs, the bug removes itself from future changes (though there may be side effects of which I am unaware).
I guess the real issue is that it doesn't always do this gracefully.

Note: It also triggers a separate issue with rebuilding the Launcher after a GUI Language change, but that's not exclusive to this case.

What happens is that (unknown to the user) a change is made to the 'GUI Language' setting, but isn't immediately written to the Config file.
Instead it waits for the next time the user clicks 'Apply' or 'OK', then writes the change to the Config file, neglecting the change the user intended to make.

To reproduce an example of the effect of this bug, do the following:

1. Start ScummVM with all Global Options set to their normal defaults.
2. Check that 'GUI Language' is set to 'default'.
3. Change 'Graphics Mode' to OpenGL, then click 'Apply'.
4. Click 'Cancel' to return to the Launcher.
5. Return to the Graphics tab… the 'Graphics Filter' option is added (this activates the bug).
6. Select 'Graphics Filter', then click 'Apply'… the setting will be deselected (the language setting is added to the Config file).
7. Repeat the selection, then click 'Apply' to complete the change.

• Clicking 'OK' in step 3 skips step 4, but doesn't allow you to accurately track the changes in the Config file if you wish to do so.

Before the 'Apply' button was added, this bug would trigger the 'restart' message, and be deleted (along with the invalid theme switch fixed in PR 919) upon restart… which suggests the language change is not meant to be retained.
Restarting still works as before, but the user is no longer advised to do so.

As a workaround, this bug can be avoided by simply changing the 'GUI Language' setting from 'default' to English before making any other changes.

[macca8]

Don't know if it helps, but I've found what I suspect is an incomplete closure to an "#ifdef USE_TRANSLATION… #endif" statement at:

GUI/options.cpp/GlobalOptionsDialog::apply().line 1905

It may be nothing (I'm not into writing C++), but the statement includes code that can set the GUI language & load a theme, so I thought it worthy of inspection by someone more experienced than me.

[wjp]

What do you mean by an incomplete closure?

[macca8]

The closing '#endif' component of the statement is displayed in a black font, as opposed to all the similar entries which are in red (I understand that this is just builtin formatting).
This suggested to me that it may be incorrectly placed and therefore not working as expected.

The effect may be purely cosmetic, as the only obvious effect is that the names of every new method written after this point appear in black as well, instead of the usual purple.
However, since I don't understand the intricacies of C++, I reported it just in case.

[criezy]

You mean on GitHub I suppose (​https://github.com/scummvm/scummvm/blob/master/gui/options.cpp#L1905)? It does indeed look like it has some issues parsing this code. But I think that this issue is with the display on GitHub, not with the code.

Regarding your reported issue, I can indeed reproduce it (and thank you for reporting it!). I think there are actually two issues here:

1. The first one is similar to bug #9711. Changes of the GUI language triggers a rebuild of the GUI. As such any setting changed but not not yet applied when applying the language change may be lost. Bug #9711 was fixed by moving the language change apply to after applying all other changes in GlobalOptionsDialog, but before applying the changes of the OptionsDialog. Thus settings common to the global options and game options, such as graphics settings, may be lost when we also change the language at the same time.

To reproduce 1, change the language and toggle on/off the "Filter graphics" button. When you apply the change stye language is change but the Filter graphics options is reverted back to its original value. The same happen with other options (such as changing the Fullscreen mode and language at the same time).

2. The second issue is that the <default>language setting appear to not be handled correctly resulting in a change to the language setting when we would not expect it. And this in turn triggers the first issue.

[Joefish]

First of all, your reproduction steps are top-notch as always. Love it :)
I fixed the issue in a ​branch of mine.

I should have read criezy's post more carefully..
Not only is the cause of the bug already described but I also missed the case of <default> != "English".
Back to the drawing board it is!

[macca8]

Criezy, regarding the incorrect handling of the <default> language setting that you identified above, I've noted your comments in PR 921, in particular the handling of an empty string (the <default> equivalent).

Your comprehensive analysis suggests a simple solution to resolve this.

Add the following code as the opening statement to the definition of TranslationManager::parseLanguage(string &lang).

if (lang.empty())
return kTranslationAutodetectId;

I may be over simplifying things, but if you consider it viable, please feel free to commit this change.

[criezy]

Yes, that is one of a possible three simple changes that would fix that issue (each one would fix the issue on its own, but I am thinking we might want to do two of them, the one you propose and another one, which both fix an inconsistency in different part of the code). I am remaining deliberately a bit vague here, and not doing the change myself, as a candidate for the Google Summer of Code is looking at fixing the issue and I don't want to do all the work for him ;) Please bear with us. It will take a bit longer to get fixed, but the issue will get fixed eventually.

Note: we would also need to check the existing calls to TranslationManager::parseLanguage to make sure none of them abuse the current inconsistency by passing "" when they should pass "C", as those would need to be changed if we fix the inconsistency.

[criezy]

This bug has now been fixed by commit 7ff4641 (PR #921 from Joefish).