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

Clean up and improve mainmenu theme / game theme code #13885

Merged
merged 5 commits into from Oct 18, 2023

Conversation

grorp
Copy link
Member

@grorp grorp commented Oct 11, 2023

This PR reworks the code that sets the mainmenu theme aka game theme. It resolves the "Hide 'Minetest' header in dialog" to-do from #13476, the last remaining "Before 5.8.0" to-do in that issue.

Improvements:

  • In addition to the engine theme ("MINETEST" header) and to game-specific themes, a new "neutral" theme is introduced. This theme is used by the settings dialog to avoid the "MINETEST" header appearing behind the dialog.

  • The ContentDB dialog now always uses the engine theme. Previously, it would use the theme of the currently selected game if it was opened from the "Start Game" tab.

  • On touchscreen builds, the ContentDB dialog uses the neutral theme instead of the engine theme to avoid the "MINETEST" header appearing behind the dialog.

  • The update notification dialog now always uses the engine theme.

  • The game-specific "topleft text" on the "Start Game" tab (core.set_topleft_text) no longer disappears when a dialog (e.g. "Select Mods") is opened. This is archieved by moving the responsibility for that text to the mainmenu theme as well.

  • Cleaner code: Before this PR, the "Start Game" tab was responsible for setting the mainmenu theme for all other tabs. After this PR, all tabs and dialogs themselves are responsible for setting the mainmenu theme.

To do

This PR is a Ready for Review.

How to test

Click your way through the mainmenu and verify that all tabs and dialogs use the correct background, header, footer, music, etc.

This allows the game-specific topleft text to be shown e.g. in the "Select Mods" dialog.
@grorp grorp added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Bugfix 🐛 PRs that fix a bug @ Mainmenu @ Builtin labels Oct 11, 2023
@grorp grorp mentioned this pull request Oct 11, 2023
17 tasks
@grorp grorp added this to the 5.8.0 milestone Oct 11, 2023
Copy link
Member

@srifqi srifqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good to me. I like the idea of a "neutral" theme.

@grorp grorp marked this pull request as draft October 15, 2023 06:57
- Less code
- menu_background.png provided by texture packs is applied again for the settings dialog
@grorp grorp marked this pull request as ready for review October 16, 2023 13:00
@grorp
Copy link
Member Author

grorp commented Oct 16, 2023

This PR is ready again. I replaced the "neutral" theme with a hide_decorations flag that hides the header and the footer. That requires less code and has the effect that the menu_background.png file provided by texture packs is applied again in the settings dialog.

Sorry for not noticing this earlier.

@grorp
Copy link
Member Author

grorp commented Oct 18, 2023

@srifqi Does your approval still apply?

@srifqi
Copy link
Member

srifqi commented Oct 18, 2023

Does your approval still apply?

Yes, even though I liked the idea of a "neutral" theme, but it turns out that it is simpler to use the engine theme with a flag/variable/parameter for the decorations.

@grorp grorp merged commit b1dec37 into minetest:master Oct 18, 2023
2 checks passed
@grorp grorp deleted the rework-game-theme branch December 18, 2023 16:40
kawogi pushed a commit to kawogi/minetest that referenced this pull request Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix 🐛 PRs that fix a bug @ Builtin @ Mainmenu Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements One approval ✅ ◻️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants