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

Open
wants to merge 2 commits into
base: master
Choose a base branch
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.

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.

@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
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) 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

4 participants