Skip to content
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: Increase allowed number of ExtraGuiOptions #2783

Merged
merged 4 commits into from Feb 20, 2021

Conversation

@OMGPizzaGuy
Copy link
Member

@OMGPizzaGuy OMGPizzaGuy commented Feb 18, 2021

Warn and limit options when adding more ExtraGuiOptions than supported. OptionsContainerWidget passes layout to the scroll container if used.

OMGPizzaGuy added 2 commits Feb 18, 2021
Warn and limit options when adding more ExtraGuiOptions than supported. OptionsContainerWidget passes layout to the scroll container if used.
@criezy
Copy link
Member

@criezy criezy commented Feb 18, 2021

With the changes to the themes you might need to increment the theme version (in gui/ThemeEngine.h and the THEMERC for each theme) so that it does not try to use an old theme if you happen to have one in your extra path.

@SupSuper
Copy link
Contributor

@SupSuper SupSuper commented Feb 19, 2021

I wonder if it would make more sense to take these out of the theme so they could just be procedurally added like other GameOptionsDialogs, eg. https://github.com/scummvm/scummvm/blob/master/engines/sci/detection.cpp#L384

@OMGPizzaGuy
Copy link
Member Author

@OMGPizzaGuy OMGPizzaGuy commented Feb 19, 2021

@SupSuper - I'll check that out. I originally attempted something like the RemapWidget but had trouble making good layout. That example looks better.

@OMGPizzaGuy
Copy link
Member Author

@OMGPizzaGuy OMGPizzaGuy commented Feb 20, 2021

Procedurally generated layout works great!
I could see adding theme variables for the layout padding if desired - there's also the existing global padding variables, but they did not consistently match the dialog's padding

@bluegr
Copy link
Member

@bluegr bluegr commented Feb 20, 2021

Very nice! Thanks for following @SupSuper's advice. The end result looks superb, and ticks all the right boxes:

  • There is now no limit of engine-specific options
  • The options are now in a scrollable container
  • The hardcoded engine-specific options are now rightly out of the theme files

I had added the original comment in dialogs.cpp, and the advice given then about adding all the options in the theme files is now obsolete, thanks to procedurally generated options.

The end result is greatly simplified code, and a much more extendable custom options interface. I only see positive changes here, without any loss of functionality, so this is good to be merged now.

Many thanks for your work! Merging

@bluegr
Copy link
Member

@bluegr bluegr commented Feb 20, 2021

I'll squash the commits, to keep the latest version of the code, cause the initial commits followed a different type of implementation.

@bluegr bluegr merged commit f9f7d0e into scummvm:master Feb 20, 2021
3 checks passed
3 checks passed
Codacy Static Code Analysis Codacy Static Code Analysis
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deepcode-ci-bot Well done, no issues found!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants