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: Fix for #9711 #919

Merged
merged 2 commits into from Mar 9, 2017

Conversation

Projects
None yet
2 participants
@Joefish
Contributor

Joefish commented Mar 9, 2017

Fix for GUI: Changing 'GUI Language' in Global Options removes theme overlay.
Also, cbcf819 fixes the theme label to display the correct theme.
For more details see the commit messages.

@Joefish Joefish closed this Mar 9, 2017

Joefish added some commits Mar 8, 2017

GUI: Fix resolution of theme filename to id
getThemeId() returned "builtin" for valid filenames because FSNode only
searches for the theme filename, like "scummmodern.zip" in the current
directory. listUsableThemes() searches SearchMan default directories
for theme files.
GUI: Fix Theme Label in Options->Misc
The theme label in the Misc tab will not change to the correct theme
when current language and theme is changed and 'apply' pressed.
loadNewTheme() does not do a rebuild of all widgets, including the
theme label, like it is explicitly done in the 'language section'.
The problem is that rebuild() uses the currently applied settings to
rebuild all widgets. Although a new theme was selected by the user the
label will be overwritten with the name of the still active theme.

By rearranging the logic a complete rebuild of the GUI is done and
updates the widgets correctly.

@Joefish Joefish reopened this Mar 9, 2017

@criezy

This comment has been minimized.

Show comment
Hide comment
@criezy

criezy Mar 9, 2017

Member

Good catch!
And good work on following our conventions for the code style and commit messages.

Currently getThemeId(file) is indeed not working in all cases and as a result ThemeEngine can set _themeId to "builtin" instead of the correct ID. Somehow we did not notice this before, probably because this _themeId is not used in many place. Your first commit is OK to fix this issue.

The second issue is more minor (I didn't even notice it at first and had to read again your description to understand what was wrong - by the way good work on your commit messages!). But it's a nice one to get fixed nonetheless.

Your second commit will introduce a change of behaviour when changing both the language and the theme and some combination of old or new theme not supporting the charset for the old or new language. I don't see that as an issue though as I don't see one better than the other and neither is optimal. I have mapped all 7 combination of incompatibilities I can think of, and compared the old behaviour with the new ones. The old code behaves a bit better generally, but that is marginal, and both fail in some cases where the new language is compatible with the new theme (so they could succeed), but the new theme is not compatible with the old language and/or the new language is not compatible with the old theme. That might be the subject of another PR for a GSoC candidate :P

Thank you. I am merging this as it is.

Member

criezy commented Mar 9, 2017

Good catch!
And good work on following our conventions for the code style and commit messages.

Currently getThemeId(file) is indeed not working in all cases and as a result ThemeEngine can set _themeId to "builtin" instead of the correct ID. Somehow we did not notice this before, probably because this _themeId is not used in many place. Your first commit is OK to fix this issue.

The second issue is more minor (I didn't even notice it at first and had to read again your description to understand what was wrong - by the way good work on your commit messages!). But it's a nice one to get fixed nonetheless.

Your second commit will introduce a change of behaviour when changing both the language and the theme and some combination of old or new theme not supporting the charset for the old or new language. I don't see that as an issue though as I don't see one better than the other and neither is optimal. I have mapped all 7 combination of incompatibilities I can think of, and compared the old behaviour with the new ones. The old code behaves a bit better generally, but that is marginal, and both fail in some cases where the new language is compatible with the new theme (so they could succeed), but the new theme is not compatible with the old language and/or the new language is not compatible with the old theme. That might be the subject of another PR for a GSoC candidate :P

Thank you. I am merging this as it is.

@criezy criezy merged commit 8726792 into scummvm:master Mar 9, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment