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.

Sign up for GitHub

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

Jump to bottom

Mainmenu: Clean up and improve advanced settings dialogues #7802

Merged
merged 3 commits into from
Oct 20, 2018
Merged

Mainmenu: Clean up and improve advanced settings dialogues #7802

merged 3 commits into from
Oct 20, 2018

Conversation

SmallJoker
Copy link
Member

Improvements:

  1. Formspec size and description box are calculated last
  2. Width and height are now adjustable per setting type
  3. Error message (dialogdata.error_message) shortens the description field and is placed below
  4. Add more spacing for larger fonts
  5. More comments and extensible by setting different height and width values

Settings for screenshot below:

font_size = 17
gui_scaling = 0.95
screen_w = 900
screen_h = 600

2D-Noise, the free spot on the bottom is for the abs
2D-Noise

Boolean/enum input:
Bool/enum input

Basic field input with wrong format:
wrong format input

Designed to supersede #7791

@SmallJoker
Mainmenu: Clean up and improve setting dialogue
7db0844 
Improvements:
1. Formspec size and description box are calculated last
2. Width and height are now adjustable per setting type
3. Error message (dialogdata.error_message) shortens the description field and is placed below
4. Add more spacing for larger fonts
5. More comments and extensible by setting different height and width values
@SmallJoker
Noise params: fix covered absvalue checkbox, use Y = Z for 2d
4912fff
@paramat paramat mentioned this pull request Oct 20, 2018
Closed
@paramat paramat added this to the 5.0.0 milestone Oct 20, 2018
@paramat paramat added Blocker The issue needs to be addressed before the next release. Android High priority Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements labels Oct 20, 2018
@paramat
Copy link
Contributor

paramat commented Oct 20, 2018 •
edited
Loading

Looks good, i'll test and review.
Labelled as blocker because we can then (in a later PR) tune the size of the int/float formspec to make it more usable on Android, so that Android users are able to set suitable GUI/HUD/font sizes, to attend to #5054.
So replaces #7791

@paramat paramat removed the Android label Oct 20, 2018
@paramat paramat changed the title Mainmenu: Clean up and improve setting dialogue Mainmenu: Clean up and improve advanced settings dialogues Oct 20, 2018
paramat
paramat reviewed Oct 20, 2018
View reviewed changes
builtin/mainmenu/dlg_settings_advanced.lua Outdated
for index, value in ipairs(setting.values) do
-- translating value is not possible, since it's the value
-- that we set the setting to
formspec = formspec .. core.formspec_escape(value) .. ","
if get_current_value(setting) == value then
selected_index = index
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about breaking from the loop, surely all possible values must be added to the dropdown?

@paramat
Copy link
Contributor

paramat commented Oct 20, 2018 •
edited
Loading

Screenshots with game window 1024x600 (default) and all other relevant settings default.

screenshot from 2018-10-20 11-46-42

^ Flags

screenshot from 2018-10-20 11-47-56

^ v3f

screenshot from 2018-10-20 11-51-31

^ path/filepath

It's good that all the wasted space has been removed. The noise param formspecs are more compact.
The simpler setting types have formspecs that are too big for Android, and look too big, with too big description boxes, but all that can be tuned later.

paramat
paramat reviewed Oct 20, 2018
View reviewed changes
builtin/mainmenu/dlg_settings_advanced.lua Outdated
formspec ..
"button[" .. width / 2 - 2.5 .. "," .. height - 0.25 .. ";2.5,0.75;btn_done;" ..
fgettext("Save") .. "]" ..
"button[" .. width / 2 .. "," .. height - 0.25 .. ";2.5,0.75;btn_cancel;" ..
Copy link
Contributor

@paramat paramat Oct 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Button height can't be less than 1, height is always 1 for buttons and non-1 values cause bugs.

@paramat
Copy link
Contributor

paramat commented Oct 20, 2018

Looks good generally i'm close to approving.

@paramat
Copy link
Contributor

paramat commented Oct 20, 2018

👍

@SmallJoker SmallJoker merged commit ff35bff into minetest:master Oct 20, 2018
@paramat
Copy link
Contributor

paramat commented Oct 21, 2018 •
edited
Loading

@SmallJoker next perhaps we can detect Android and reduce the size of the int/float formspec (and perhaps bool and enum too) for that platform?

@SmallJoker
Copy link
Member Author

@paramat Rather check why the GUI is scaled so badly. The font usually displayed right on Android, but the formspec size does not seem to be adjusted well to the maximal height and width of the screen.

@paramat
Copy link
Contributor

paramat commented Oct 21, 2018

The main menu pages scale with game window size, but each setting value edit formspec doesn't, certainly this needs investigation.

@paramat paramat mentioned this pull request Nov 17, 2018
Closed
@SmallJoker SmallJoker deleted the adv_settings_cleanup branch November 28, 2018 19:12
@paramat
Copy link
Contributor

paramat commented Dec 3, 2018

@SmallJoker did this fix #5831 ?

@SmallJoker SmallJoker mentioned this pull request Dec 3, 2018
Closed
osjc pushed a commit to osjc/minetest that referenced this pull request Jan 23, 2019
@SmallJoker @osjc
Mainmenu: Clean up and improve advanced settings dialogues (minetest#…
2634b7d 
…7802)

Improvements:
1. Formspec size and description box are calculated last
2. Width and height are now adjustable per setting type
3. Error message (dialogdata.error_message) shortens the description field and is placed below
4. Add more spacing for larger fonts
5. More comments and extensible by setting different height and width values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paramat paramat paramat left review comments

Assignees
No one assigned
Labels
Blocker The issue needs to be addressed before the next release. High priority @ Mainmenu Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements One approval ✅ ◻️
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@SmallJoker @paramat