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 sliders to settings #14469

Closed
wants to merge 2 commits into from

Conversation

kotek900
Copy link
Contributor

A compact, short information about PR for easier understanding:

  • Sliders for settings
  • More user friendly that typing in numbers

To do

This PR is a Work in Progress / Ready for Review.

  • add button to turn slider into a regular text box
  • add indicators to sliders

How to test

image

@Zughy Zughy added Roadmap The change matches an item on the current roadmap. Feature ✨ PRs that add or enhance a feature @ Mainmenu UI/UX labels Mar 17, 2024
local value = core.settings:get(setting.name) or setting.default
local scrollbar_value = value/(setting.max - setting.min)*steps - setting.min

self.resettable = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.resettable = true
self.resettable = core.settings:has(setting.name)

Comment on lines +154 to +157
local raw_value = core.explode_scrollbar_event(fields[setting.name]).value
local value = (raw_value/steps) * (setting.max - setting.min) + setting.min
core.settings:set(setting.name, tostring(value))
return false
Copy link
Contributor

@y5nw y5nw Mar 17, 2024

Choose a reason for hiding this comment

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

The value should only be changed if the scrollbar is modified.

Suggested change
local raw_value = core.explode_scrollbar_event(fields[setting.name]).value
local value = (raw_value/steps) * (setting.max - setting.min) + setting.min
core.settings:set(setting.name, tostring(value))
return false
local event = core.explode_scrollbar_event(fields[setting.name])
if event.type ~= "CHG" then
return false
end
local raw_value = event.value
local value = (raw_value/steps) * (setting.max - setting.min) + setting.min
core.settings:set(setting.name, tostring(value))
return true

Edit: kotek pointed out in Discord that this somehow breaks mouse dragging.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reason why mouse dragging stops working is that after returning true GUI gets regenerated and this probably causes scrollbar to lose focus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

normally regenerating the GUI doesn't make scrollbars loose focus, but for some minetest reason it does in the main menu

steps)
local scrollbar_formspec = ("scrollbar[0,0.5;%f,0.4;horizontal;%s;%f]"):format(
avail_w - 1, setting.name, scrollbar_value)
local formspec = setting_label..scrollbar_options..scrollbar_formspec
Copy link
Contributor

@y5nw y5nw Mar 17, 2024

Choose a reason for hiding this comment

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

Nitpicking: Why not use table.concat? That would make sense if you want to add more (e.g. value display/input box) to this field.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rest of code in this file also does this so I guess it's better for consistency.

@Athozus
Copy link

Athozus commented Apr 1, 2024

Your feature (which is great, imho) uses scrollbar formspec element : wouldn't it be better to create a global slider formspec element, which would also be usable in mods / other main menu tabs (or be used as a soft-coded solution for existing volume change slider) ?

@Zughy Zughy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Apr 1, 2024
@Zughy Zughy added the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label Jun 2, 2024
@Zughy
Copy link
Member

Zughy commented Jun 2, 2024

Feel free to reopen if you start working on this again

@Zughy Zughy closed this Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action / change needed Code still needs changes (PR) / more information requested (Issues) Adoption needed The pull request needs someone to adopt it. Adoption welcomed! Feature ✨ PRs that add or enhance a feature @ Mainmenu Roadmap The change matches an item on the current roadmap. UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants