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

Add advanced settings checkbox and hide advanced settings by default #13861

Merged
merged 3 commits into from Oct 7, 2023

Conversation

grorp
Copy link
Member

@grorp grorp commented Oct 1, 2023

Based on #13730, this PR adds a "Show advanced settings" checkbox to the new settings GUI. As discussed, the checkbox determines whether the "Advanced" and "Mapgen" toplevel categories are shown and is unchecked by default.

screenshot

This is part of #13476 "Before 5.8.0". The creation of this PR was discussed on IRC: https://irc.minetest.net/minetest-dev/2023-10-01#i_6118939.

To do

This PR is a Ready for Review.

How to test

Open the settings and toggle the "Show advanced settings" checkbox. Verify that it changes the visibility of the "Advanced" and "Mapgen" toplevel categories, and nothing else.

Verify that you don't stay on an advanced settings page if you uncheck the checkbox while you are on an advanced settings page.

Verify that toggling the checkbox doesn't reset the current page and scroll position in other cases.

Co-authored-by: rubenwardy <rw@rubenwardy.com>
@grorp grorp added @ Startup / Config / Util Feature ✨ PRs that add or enhance a feature @ Mainmenu labels Oct 1, 2023
@grorp grorp added this to the 5.8.0 milestone Oct 1, 2023
@grorp grorp requested a review from rubenwardy October 1, 2023 16:55
@SmallJoker SmallJoker self-requested a review October 1, 2023 17:03
@grorp grorp mentioned this pull request Sep 17, 2023
17 tasks
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Works. The scrollbar is also reasonably positioned when toggling the state.
Unfortunately there is no visual indicator to mark which pages are affected by this checkbox (an idea for later).

@rubenwardy
Copy link
Member

Looks good and works great. My only suggestion would be to make "Client and Server" advanced as well

@Zughy Zughy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Oct 7, 2023
@grorp
Copy link
Member Author

grorp commented Oct 7, 2023

My only suggestion would be to make "Client and Server" advanced as well

Seems reasonable.

There are now some advanced settings in the non-advanced sections (e.g. 3d_mode, enable_bloom_debug, water_wave_height, etc.) and some non-advanced settings in the advanced sections (e.g. client_mesh_chunk), but that can be cleaned up by a later PR.

@grorp grorp removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Oct 7, 2023
Copy link
Member

@rubenwardy rubenwardy left a comment

Choose a reason for hiding this comment

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

Works well

@grorp grorp merged commit 26bb397 into minetest:master Oct 7, 2023
2 checks passed
@grorp grorp deleted the advanced-settings-2 branch December 18, 2023 16:39
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants