Skip to content

Move keybinding settings to (Lua-based) setting menu#15791

Merged
y5nw merged 34 commits into
luanti-org:masterfrom
y5nw:lua-key-change-menu
Apr 20, 2025
Merged

Move keybinding settings to (Lua-based) setting menu#15791
y5nw merged 34 commits into
luanti-org:masterfrom
y5nw:lua-key-change-menu

Conversation

@y5nw

@y5nw y5nw commented Feb 15, 2025

Copy link
Copy Markdown
Contributor

#15614 (comment)

Alternative to #15152.

Due to merge conflicts, please merge this after #14964.

To do

This PR is ready for review.

  • Implement button_key element.
    • Send event on release instead of on press.
  • Add button to delete keybinding.
  • Implement setting callback.
  • Add sections to the keybinding list.
  • Implement checks for keybinding collisions.

How to test

See #15152.

@y5nw y5nw added Feature ✨ PRs that add or enhance a feature @ Mainmenu / Settingsmenu @ Client / Controls / Input UI/UX Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) labels Feb 15, 2025
@y5nw y5nw mentioned this pull request Feb 23, 2025
8 tasks
@y5nw
y5nw force-pushed the lua-key-change-menu branch 3 times, most recently from f34b127 to b80e86f Compare March 16, 2025 20:20
@y5nw y5nw removed the Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) label Mar 16, 2025
@y5nw
y5nw marked this pull request as ready for review March 16, 2025 20:25
@y5nw
y5nw force-pushed the lua-key-change-menu branch from b80e86f to b6fd155 Compare March 21, 2025 11:45

@SmallJoker SmallJoker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unfortunately this layout crops a few texts on smaller screens or window sizes. Although I don't think this can be fixed easily with the current formspec design. Just mentioning.
small window PR
Meanwhile in master:
small window master

Comment thread builtin/common/settings/dlg_settings.lua Outdated
Comment thread src/gui/guiButtonKey.cpp
Comment thread src/gui/guiButtonKey.h
Comment thread builtin/common/settings/components.lua Outdated
Comment thread builtin/settingtypes.txt
Comment thread builtin/common/settings/components.lua
Comment thread builtin/common/settings/components.lua
@y5nw

This comment was marked as outdated.

@y5nw

y5nw commented Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

Unfortunately this layout crops a few texts on smaller screens or window sizes.

I tried #15791 (comment). It does not crop the text, but it looks worse IMO, so I reverted it.

IMO it might be better to change the setting menu to use up the full screen/window space, which will (in general) create less problems for setting entries with two columns; although that would be out of scope for this PR.

@y5nw
y5nw force-pushed the lua-key-change-menu branch from ce229cc to a992320 Compare March 30, 2025 16:28

@grorp grorp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for working on this.

Suggestions:

  • The menu feels like it's not using the available vertical space well enough. I have to scroll a lot to see all the keybindings. Current layout:
    screenshot
    I think something like this would be an improvement (button height reduced to 0.6, spacing reduced heavily as well):
    screenshot
    (The "info" buttons would need to be put in again of course)
    EDIT: in hindsight, this is too cramped, see newer proposal below instead

  • It's impossible to set a keybinding to mouse buttons, even though dig/place are listed and show them correctly

  • It might make sense to put the keybinding section below a new sub-heading

Comment thread src/script/lua_api/l_mainmenu.cpp Outdated
Comment thread src/script/lua_api/l_pause_menu.cpp
Comment thread src/gui/mainmenumanager.h Outdated
Comment thread src/client/inputhandler.h Outdated
Comment thread src/client/inputhandler.cpp
Comment thread builtin/common/settings/components.lua Outdated
@y5nw
y5nw force-pushed the lua-key-change-menu branch 2 times, most recently from 5243f2c to 6704a8f Compare April 1, 2025 20:37
@y5nw

y5nw commented Apr 1, 2025

Copy link
Copy Markdown
Contributor Author

(button height reduced to 0.6, spacing reduced heavily as well):

The height of 0.8 is taken from other elements, so I would like to keep it this way for consistency.

I would personally prefer having a dedicated tab with a table of keybindings, but this does not seem to fit well into the way the setting form is currently designed.

It's impossible to set a keybinding to mouse buttons, even though dig/place are listed and show them correctly

Fixed in 33adc71:

Bildschirmfoto 2025-04-01 um 22 56 13

(Actually I just noticed that this crashes the client if one tries to change the keybinding in-game; I need to look into this.) (Edit: should be solved now)

It might make sense to put the keybinding section below a new sub-heading

Done in cfaf84d.

@grorp
grorp self-requested a review April 4, 2025 15:33
@y5nw
y5nw force-pushed the lua-key-change-menu branch from 2dcf68b to d719328 Compare April 6, 2025 18:03
@SmallJoker
SmallJoker self-requested a review April 6, 2025 18:05
@grorp

grorp commented Apr 12, 2025

Copy link
Copy Markdown
Member

Fixed in 33adc71: [screenshot]

Thank you, that looks good.

The height of 0.8 is taken from other elements, so I would like to keep it this way for consistency.

Reducing the height would indeed make things look more inconsistent. It could be a significant improvement in this case though. The amount of hidden pixels (the amount of scrolling necessary to see all the keybindings) was reduced by about 40% in my test (from 3711 to 2185). Maybe it would be possible to argue that buttons that usually only show a single letter don't need as much space :)

Since #13282, the keybinding menu also serves the purpose of informing the user about the controls, so being able to get a quick overview is useful.

Some of the keys listed at the very bottom are actually quite useful, like the viewing range and camera mode keys, so you'd want users to scroll all the way. On a side note, it might make sense to move these three further up.

I would personally prefer having a dedicated tab with a table of keybindings, but this does not seem to fit well into the way the setting form is currently designed.

It indeed doesn't. However, what you could do (not saying you should) is moving the keybinding settings to a separate dialog again, just in Lua instead of C++ this time. This would also allow the keybindings to use the full available screen space.

@SmallJoker

SmallJoker commented Apr 12, 2025

Copy link
Copy Markdown
Member

Fixed in 33adc71:

Why exactly do we need this additional dialogue? Could we instead listen to mouse/keypad events as well to shorten the following steps ...

  1. Click the settings button to opening the "single key change menu"
  2. Selecting the right item in the dropdown
  3. Pressing the button to capture a key || or select the mouse button in the dropdown
  4. Close the menu

... to this:

  1. Click the settings button to listen to any input.

@y5nw
y5nw force-pushed the lua-key-change-menu branch from d719328 to 7446d7d Compare April 13, 2025 17:50
@y5nw

y5nw commented Apr 13, 2025

Copy link
Copy Markdown
Contributor Author

Why exactly do we need this additional dialogue? Could we instead listen to mouse/keypad events as well to shorten the following steps ...

The main issue was that I could not figure out how to cleanly handle mouse buttons with the keybinding button. I have a solution now (and committed) so I reverted the dialog.

Side note: We will likely still want a dialog for #15934, but we can leave it to that PR instead of here.

@y5nw
y5nw force-pushed the lua-key-change-menu branch from d2cb7b5 to b74ee3f Compare April 17, 2025 09:36
@y5nw
y5nw force-pushed the lua-key-change-menu branch from b74ee3f to 0869380 Compare April 17, 2025 10:10
Comment thread builtin/settingtypes.txt Outdated
Comment on lines +2057 to +2067
# Key for switching to the previous entry in Quicktune.
keymap_quicktune_prev (Quicktune: select previous entry) key

# Key for switching to the next entry in Quicktune.
keymap_quicktune_next (Quicktune: select next entry) key

# Key for decrementing the selected value in Quicktune.
keymap_quicktune_dec (Quicktune: decrement value) key

# Key for incrementing the selected value in Quicktune.
keymap_quicktune_inc (Quicktune: increment value) key

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I hope the description for Quicktune keys is correct? (I have never used quicktune before)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(I don't know either)

@grorp grorp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The new changes look good 👍

Comment thread builtin/settingtypes.txt Outdated

@grorp grorp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good 👍

Thank you for working on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment