New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GUI: Restore previous settings if GUI cannot be rendered #921

Merged
merged 1 commit into from Apr 9, 2017

Conversation

Projects
None yet
2 participants
@Joefish
Contributor

Joefish commented Mar 12, 2017

This PR focuses on GlobalOptionsDialog::apply() to determine incompatible font/charset and fallback to previous settings on error.
'Greek' always triggers an error out of unknown reasons to me. It is the only language with charset iso-8859-7, so looking at ThemeEngine::init(), how fonts/charsets are loaded and why it fails and falls back to ASCII is the next step.

@criezy you mentioned in #919 that you have mapped out combinations that broke previously.
Would be great if you could reply and paste your list here :)

@criezy

This comment has been minimized.

Show comment
Hide comment
@criezy

criezy Mar 12, 2017

Member

I have not looked at this code yet. The mapping I wrote assumes that:

  • If the new language charset is not compatible with the current theme, it fallback to English
  • If the new theme is not compatible with the current language charset, it reverts to the previous theme.

Based on that I mapped what happens when changing both the language and theme, if the language is changed first (as was done before PR #919) and when the theme is changed first (as was done in PR #919). The 7 cases I considered are a combination of 3 incompatibilities:

  1. The new language is not compatible with the old theme
  2. The new theme is not compatible with the old language
  3. The new language is not compatible with the new theme.

This gave me the following table:

Incompatibilities  |  Change language first         |  Change Theme first
--------------------------------------------------------------------------
        1          |  English + new theme           |  OK (new language + new theme)
        2          |  OK (new language + new theme) |  English + new theme
        3          |  new language + old theme      |  English + new theme
       1+2         |  English + new theme           |  English + old theme
       1+3         |  English + new theme           |  English + new theme
       2+3         |  new language + old theme      |  new language + old theme
      1+2+3        |  English + new theme           |  English + old theme

Note: The issue you see with greek is likely related to missing Helvetica font for charset iso-8859-7 in our code base.

Member

criezy commented Mar 12, 2017

I have not looked at this code yet. The mapping I wrote assumes that:

  • If the new language charset is not compatible with the current theme, it fallback to English
  • If the new theme is not compatible with the current language charset, it reverts to the previous theme.

Based on that I mapped what happens when changing both the language and theme, if the language is changed first (as was done before PR #919) and when the theme is changed first (as was done in PR #919). The 7 cases I considered are a combination of 3 incompatibilities:

  1. The new language is not compatible with the old theme
  2. The new theme is not compatible with the old language
  3. The new language is not compatible with the new theme.

This gave me the following table:

Incompatibilities  |  Change language first         |  Change Theme first
--------------------------------------------------------------------------
        1          |  English + new theme           |  OK (new language + new theme)
        2          |  OK (new language + new theme) |  English + new theme
        3          |  new language + old theme      |  English + new theme
       1+2         |  English + new theme           |  English + old theme
       1+3         |  English + new theme           |  English + new theme
       2+3         |  new language + old theme      |  new language + old theme
      1+2+3        |  English + new theme           |  English + old theme

Note: The issue you see with greek is likely related to missing Helvetica font for charset iso-8859-7 in our code base.

@Joefish

This comment has been minimized.

Show comment
Hide comment
@Joefish

Joefish Mar 20, 2017

Contributor

The behavior described in the table of criezy's post has changed drastically with this PR.
Currently, the settings are reset to the previous state if the GUI fails to be rendered.
This means that when changing [classic theme, English] to [modern theme, Russian] and Russian
is not supported it falls back to [classic theme, English] although English may be supported by modern theme.
If this is not desired, please reply here and define the behavior.

What's currently left on my list:

  • Clearing buffered fonts on theme change
  • Track down segfault in BdfFont::~BdfFont on shutdown
Contributor

Joefish commented Mar 20, 2017

The behavior described in the table of criezy's post has changed drastically with this PR.
Currently, the settings are reset to the previous state if the GUI fails to be rendered.
This means that when changing [classic theme, English] to [modern theme, Russian] and Russian
is not supported it falls back to [classic theme, English] although English may be supported by modern theme.
If this is not desired, please reply here and define the behavior.

What's currently left on my list:

  • Clearing buffered fonts on theme change
  • Track down segfault in BdfFont::~BdfFont on shutdown
Show outdated Hide outdated gui/options.cpp
@criezy

criezy requested changes Mar 21, 2017 edited

There is an issue with this change.
Have you noticed what is done at the end of OptionsDialog::apply() and thus before your change was done at the end of GlobalOptionsDialog::apply() but is not done there anymore?

Answer: the changes to ConfMan done by GlobalOptionsDialog::apply() after calling OptionsDialog::apply() are not flushed to disk anymore. This needs to be addressed.

Edit: This comment refers to commit 8959dee. Despite adding this comment in the commit view, it seems it is not associated to the commit apparently, which makes it confusing.

@criezy

criezy requested changes Mar 21, 2017 edited

Edit: this comment refers to commit d39beb7.

I don't think this is the correct change to fix this issue. With your change, when selecting the default language, you save to the config file the language actually used. This means that if the user then closes ScummVM, changes the system language and then reopen ScummVM, he will not get the default language (the system language) but the old system language instead.

Currently, when selecting the "gui_language" setting is removed from the config file, which is as it should be. Next time ScummVM is started it will use the system language, whatever it is.

The issue is with the way the ConfigManager works: after calling ConfMan::set("gui_language", Common::String()), ConfMan::hasKey("gui_language") will return true, despite the value being empty. And TransMan.parseLanguage("") returns the kTranslationBuiltinId and not the kTranslationAutodetectId. So if you set the language to and apply the change (or click on OK and then reopen the options dialog), the language selector is initialized to English. This is what needs to be fixed.

Note: when the config is saved to disk it skips config that have an empty value. So setting the language to default, clicking on OK and then closing ScummVM will behave as expected. After restarting ScummVM since the gui_language setting is not defined in the config file, ConfMan::hasKey("gui_language") will be false and opening the option dialog will correctly initialize the language selector.

@criezy

This comment has been minimized.

Show comment
Hide comment
@criezy

criezy Mar 21, 2017

Member

The behaviour you propose in case of language/theme incompatibility seems fine to me. At least it is simple. I have not checked but I suspect we might get some confusing warning messages though (for example the one from ThemeEngine that says that it fails to load the localized font and use stye default language instead, when in the end we revert to the previous language, which might not be the default language). So I think it might be nice to expand a bit the error MessageDialog you have in GlobalOptionsDialog::apply() when reverting changes.

Another remark is that with your change it seems that it is rebuilding the dialog whenever redraw is true, which happens when changing the language or the theme or the renderer. But rebuild is only needed when changing the language. When changing the theme or renderer without changing the language calls to loadNewTheme() and draw() are sufficient. So it would be nice to avoid rebuilding the dialog in such a case.

Member

criezy commented Mar 21, 2017

The behaviour you propose in case of language/theme incompatibility seems fine to me. At least it is simple. I have not checked but I suspect we might get some confusing warning messages though (for example the one from ThemeEngine that says that it fails to load the localized font and use stye default language instead, when in the end we revert to the previous language, which might not be the default language). So I think it might be nice to expand a bit the error MessageDialog you have in GlobalOptionsDialog::apply() when reverting changes.

Another remark is that with your change it seems that it is rebuilding the dialog whenever redraw is true, which happens when changing the language or the theme or the renderer. But rebuild is only needed when changing the language. When changing the theme or renderer without changing the language calls to loadNewTheme() and draw() are sufficient. So it would be nice to avoid rebuilding the dialog in such a case.

Show outdated Hide outdated gui/options.cpp
Show outdated Hide outdated gui/options.cpp
@criezy

This comment has been minimized.

Show comment
Hide comment
@criezy

criezy Apr 4, 2017

Member

Thank you for the update. I added two comments and it would be nice to do the two minor changes I pointed out.

Then, please squash all those commits together and I think it is ready to be merged (I will have one last look once everything is squashed).

Member

criezy commented Apr 4, 2017

Thank you for the update. I added two comments and it would be nice to do the two minor changes I pointed out.

Then, please squash all those commits together and I think it is ready to be merged (I will have one last look once everything is squashed).

@criezy

criezy approved these changes Apr 5, 2017

Show outdated Hide outdated gui/options.cpp
// Rebuild the Launcher and Options dialogs
g_gui.loadNewTheme(g_gui.theme()->getThemeId(), ThemeEngine::kGfxDisabled, true);
if (isRebuildNeeded) {
rebuild();
if (_launcher != 0)
_launcher->rebuild();
}

This comment has been minimized.

@criezy

criezy Apr 5, 2017

Member

There is something missing in there. If you change the theme, but without changing the language, while there is no need to rebuild the dialog, there is still a need to redraw it. And unless I missed it, your change removed the call to draw(). So you might want to keep two booleans: isRebuildNeeded and isRedrawNeeded, and add a else if (isRedrawNeeded) draw(); here.

@criezy

criezy Apr 5, 2017

Member

There is something missing in there. If you change the theme, but without changing the language, while there is no need to rebuild the dialog, there is still a need to redraw it. And unless I missed it, your change removed the call to draw(). So you might want to keep two booleans: isRebuildNeeded and isRedrawNeeded, and add a else if (isRedrawNeeded) draw(); here.

This comment has been minimized.

@Joefish

Joefish Apr 5, 2017

Contributor

draw() just sets the redraw status for redrawing the top dialog. In loadNewTheme() it is set to full redraw what does exactly the same (GuiManager::redraw() +222).

@Joefish

Joefish Apr 5, 2017

Contributor

draw() just sets the redraw status for redrawing the top dialog. In loadNewTheme() it is set to full redraw what does exactly the same (GuiManager::redraw() +222).

This comment has been minimized.

@criezy

criezy Apr 5, 2017

Member

Right sorry. I mixed the draw() implementation of Dialog and Widget in my head.
Also the old code (from 11 years ago) was doing a draw() just after the loadNewTheme() and I didn't take the time to check it was (still) needed.

Here this is indeed not needed as loadNewTheme() does a full redraw.

@criezy

criezy Apr 5, 2017

Member

Right sorry. I mixed the draw() implementation of Dialog and Widget in my head.
Also the old code (from 11 years ago) was doing a draw() just after the loadNewTheme() and I didn't take the time to check it was (still) needed.

Here this is indeed not needed as loadNewTheme() does a full redraw.

GUI: Restore settings if GUI cannot be rendered
PR#921 changes the behavior of the client that if the GUI fails to be
rendered the previously applied settings in the misc category are
restored. Error messages were altered according to the changes.

Bug: #9717 GUI: Indirectly changing 'GUI Language' can produce
                inconsistent behaviour when changing some options.
@criezy

This comment has been minimized.

Show comment
Hide comment
@criezy

criezy Apr 9, 2017

Member

Thank you. This looks all good to me now.
I am merging this.

Member

criezy commented Apr 9, 2017

Thank you. This looks all good to me now.
I am merging this.

@criezy criezy merged commit 05bd770 into scummvm:master Apr 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Joefish Joefish deleted the Joefish:PR_9711 branch Jul 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment