GUI: Changing 'GUI Language' in Global Options removes theme overlay.

[macca8]

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

This bug has been around since the 'Apply' button was added to the Global Options tabs.

First detected: DB 1.10.0git681-gc32be8e (4 Nov 2016) post-'Apply' button
Last known without bug: DB 1.10.0git509-g7e5842f (24 Oct 2016) pre-'Apply' button
Tested with current Daily Build: 1.10.0git2635-g1a67dee (4 Mch 2017)

Bug details:
Change the 'GUI Language' in the Launcher's 'Options' menu, then click either 'Apply' or 'OK'.

The screen is redrawn without the theme overlay (a black background, with green text, white-outlined rectangular buttons, and a cross cursor… the Launcher menu buttons appear across the bottom of the screen).
On a positive note, the text is changed to the selected language and the relevant buttons remain interactive (though any further changes may cause a crash).
Unfortunately, the only way to restore the Launcher to its normal appearance is to quit ScummVM, then relaunch (making the 'Apply' button redundant in this case).

The expected behaviour of this action, before the 'Apply' button was added, was to display a message to the user advising the need to quit ScummVM to effect the change.
This message no longer appears, so it seems that this bug either (intentionally or otherwise) replaces or overrides the previous behaviour.

Limited testing has revealed that changes to other global options (for example, OpenGL in combination with Fullscreen & Filtering) can also trigger this bug, though not as consistently. Seems that somewhere along the way, one of these actions indirectly changes the 'GUI Language' setting from default to English (in my case), thus priming the bug as previously described.

Just a thought (and I apologise if this is considered off topic), but perhaps it would be prudent to disable the 'Apply' button by default, and only enable it if the selected option doesn't require the user to restart, or exit the tab, to complete the change.

[macca8]

I've found a closed PR (859, 4 Nov 2016) by criezy which confirms that the intention here was to change the GUI language without restarting ScummVM.
The restart message has been removed, but clearly the need to do so still exists.

[criezy]

The goal of the change was indeed to allow changing the language without restarting ScummVM.

I just made some tests on MacOS X 10.6 and 10.9, and in both cases changing the language and clicking on OK or Apply works correctly for me with the versions I compiled.

However I could reproduce the issue with the nightly builds available on our web site. Do you use those, or are you compiling ScummVM yourself? I will need to have a deeper look to find out if the issue is with those builds or if there is a bug in the source code that randomly appears.

Note: ScummVM is provided with two themes, the modern one (orange), and the classic one (green). The behaviour you see is that it switches to the classic theme when applying the change of the language. You can still switch back to the modern theme in the Misc tab in the ScummVM options (click on the Theme button at the top, choose the modern theme and then click on Apply or OK). There is no need to close ScummVM to switch back to the modern theme. Also I could not reproduce any crash (although I believe there was a separate issue introduced recently and fixed today with the graphics mode that could cause a crash).

[criezy]

Note for other developers (and my future self): I just made some test with valgrind and it does appear that there is a memory issue. When applying a change in the options dialog I get the following error:

==36687== Invalid write of size 1
==36687== at 0x102140E7E: GUI::ButtonWidget::handleMouseUp(int, int, int, int) (widget.cpp:341)
==36687== by 0x1020DB57E: GUI::Dialog::handleMouseUp(int, int, int, int) (dialog.cpp:212)
==36687== by 0x1020EA7B0: GUI::GuiManager::processEvent(Common::Event const&, GUI::Dialog*) (gui-manager.cpp:553)
==36687== by 0x1020E9FD9: GUI::GuiManager::runLoop() (gui-manager.cpp:334)
==36687== by 0x1020DAF39: GUI::Dialog::runModal() (dialog.cpp:80)
==36687== by 0x1020EFB59: GUI::LauncherDialog::handleCommand(GUI::CommandSender*, unsigned int, unsigned int) (launcher.cpp:642)
==36687== by 0x1021C00DA: GUI::CommandSender::sendCommand(unsigned int, unsigned int) (object.h:55)
==36687== by 0x102140E79: GUI::ButtonWidget::handleMouseUp(int, int, int, int) (widget.cpp:339)
==36687== by 0x1020DB57E: GUI::Dialog::handleMouseUp(int, int, int, int) (dialog.cpp:212)
==36687== by 0x1020EA7B0: GUI::GuiManager::processEvent(Common::Event const&, GUI::Dialog*) (gui-manager.cpp:553)
==36687== by 0x1020E9FD9: GUI::GuiManager::runLoop() (gui-manager.cpp:334)
==36687== by 0x1020DAF39: GUI::Dialog::runModal() (dialog.cpp:80)
==36687== Address 0x10520af4d is 221 bytes inside a block of size 232 free'd
==36687== at 0x4D8D: free (vg_replace_malloc.c:477)
==36687== by 0x102144E71: GUI::ButtonWidget::~ButtonWidget() (widget.h:197)
==36687== by 0x1020F7A7F: GUI::OptionsDialog::clean() (options.cpp:426)
==36687== by 0x102105C3E: GUI::GlobalOptionsDialog::clean() (options.cpp:1793)
==36687== by 0x1020F7AD2: GUI::OptionsDialog::rebuild() (options.cpp:433)
==36687== by 0x102106471: GUI::GlobalOptionsDialog::apply() (options.cpp:1854)
==36687== by 0x1020FAED8: GUI::OptionsDialog::handleCommand(GUI::CommandSender*, unsigned int, unsigned int) (options.cpp:812)
==36687== by 0x102107D93: GUI::GlobalOptionsDialog::handleCommand(GUI::CommandSender*, unsigned int, unsigned int) (options.cpp:2149)
==36687== by 0x1021C00DA: GUI::CommandSender::sendCommand(unsigned int, unsigned int) (object.h:55)
==36687== by 0x102140E79: GUI::ButtonWidget::handleMouseUp(int, int, int, int) (widget.cpp:339)
==36687== by 0x1020DB57E: GUI::Dialog::handleMouseUp(int, int, int, int) (dialog.cpp:212)
==36687== by 0x1020EA7B0: GUI::GuiManager::processEvent(Common::Event const&, GUI::Dialog*) (gui-manager.cpp:553)

[macca8]

Thanks for your reply criezy.

I do use the daily build from the website, and I've just downloaded and tested the DB for the 6 Mch, and can confirm your findings and the workaround with the Theme setting to avoid restarting.

However, I did notice a subtle difference between the result of your workaround and simply restarting ScummVM, but I don't know if it has any relevance.

For the purpose of the test, I first changed the language from 'default' (my normal default setting) to French, then I tried changing it back to 'default'.
If I chose to restart, then the 'default' setting would stick.
If I used your theme setting workaround, then the setting would change to 'English' (my default OS language).

For the record, I don't need to change my GUI language... it was purely a test for the 'Apply' button.
However, the need for your workaround suggests that an underlying problem does exist.

[macca8]

Sorry, one other difference between restarting & your workaround.
When returning to the Launcher, the last selected game is no longer highlighted after using the workaround, but the entry is still present in the config file.

[criezy]

Thanks to joefish (PR 919) this issue is now fixed.

The memory issue still exists but was not the cause of this bug.

[criezy]

And for the record, the memory issue is also fixed now.