Rework guiKeyChangeMenu

[y5nw]

This PR reworks GUIKeyChangeMenu to use GUIFormSpecMenu. This partly changes the behavior of the menu:

• It is no longer possible to remove a keybinding by pressing the delete key. Use the "X" button instead.
• It is no longer possible to cancel keybinding by pressing the escape key. Press the "Press key" button to cancel binding to a key. (Please suggest a better better label to make this more obvious).
• The shift key is ignored. In particular, it is no longer possible to bind to e.g. Shift+7 even in non-SDL devices. However, this will be broken by SDL: Use scancodes for keybindings #14964 anyway and can be restored later by adjusting Allow keybindings with modifiers #14874.

---

• Goal of the PR
  Rework GUIKeyChangeMenu to use (more or less)
• How does the PR work?
  The GUIKeyChangeMenu is largely rewritten.
• Does it resolve any reported issue?
  Fixes Allow using the delete key as a keybinding #9602 (as the relevant logic has been reworked).
• Does this relate to a goal in the roadmap?
  2.2 Internal code refactoring.
• If not a bug fix, why is this PR needed? What usecases does it solve?
  This makes it easier to change this form in the future. In particular, this is required by Implement secondary keybindings #15577.

To do

This PR is a Work in Progress.

• Refactor GUIKeyChangeMenu to use GUIFormSpecMenu
• Rework control option checkboxes.
• Rework keybinding buttons.
• Add buttons to remove keybindings.
• Fix bug: icon for removing keybindings is not shown when the form is opened from the main menu (i.e. Settings -> Keyboard and Mouse -> Controls)
• Restore scrollbar position when appropriate.
• Disallow binding to the Escape key.
• Fix bug: Cannot bind to Return or Space keys.

How to test

• Check that the form for rebinding keys continue to work.
• Check that the to-dos above are actually done. In particular:
  • Press the escape key when binding a key and observe that the keybinding does not work.
  • Observe that you can then press another key to bind the action (i.e. pressing the Escape key does not cancel keybinding).
  • Press the return or space key when binding a key and observe that it works (i.e. it binds the key instead of closing the formspec).
  • Observe that there is an "X" (with the clear.png texture) button to clear the keybinding and that the button is present only for entries with a keybinding. Observe that it actually removes the keybinding.

Screenshot

[swagtoy]

Hey, ,do you know if this will interfere with my PR #14785 ? It's a month old PR but I was also planning on rewroking the gui key change menu. I wanted to bundle it into the settings menu itself, it that makes sense, so we'd need to expose some Lua client-side mod functions to do such a thing.

[y5nw]

Hey, ,do you know if this will interfere with my PR #14785 ?

From a quick skim: not much (yet). Or it would not be much work to rebase (since the focus of this PR is solely to update guiKeyChangeMenu and avoid touching others if possible).

It's a month old PR but I was also planning on rewroking the gui key change menu. I wanted to bundle it into the settings menu itself, it that makes sense, so we'd need to expose some Lua client-side mod functions to do such a thing.

First off: 👍

From a technical perspective, moving the keybinding menu to Lua would (from my limited understanding) be slightly more complicated than moving other menus, as you would also need a custom input event handler specifically (and only) for this formspec. My current approach is to make a subclass of GUIFormSpecMenu, which (aside from being consistent with other C++ forms, with the obvious drawbacks of inheriting Irrlicht/GUIFormSpecMenu's ... interesting way of doing things) allows overriding OnEvent to grab key inputs directly when this becomes relevant.

Moving this to Lua would likely involve moving key handling to Lua as well, but this would also mean that we would need to (at least partly) expose an internal API that allows replacing the event handler with a Lua function that is called before the formspec handler. Even if we limit this specifically to keypresses, I suppose the changes would probably be enough to become its own PR.

From a timing perspective, the goal here is mainly to have something in the (very) short term that allows other/future PRs to update the keybinding menu relatively easily. This becomes relevant as this is mainly a helper PR for me: my personal goal at the moment is to finish #14964, which (as I wrote in the comment linked in the top post) also involves (*) allowing players to use multiple keybindings, the latter of which would benefit from a PR like this one.

(*) This is not exactly a dependency. Using SDL scancodes and allowing players to use multiple keybindings are two different things (and also belong to different PRs), but it is best to have both PRs merged around the same time given the implications of using SDL scancodes.

[rubenwardy]

Moving this to Lua would likely involve moving key handling to Lua as well

So for moving the key change menu to Lua, I was thinking that there could be a binding[] field element type that would handle input and setting the setting (which isn't overwise possible on the CSM env)

That being said, moving it to Lua would also require setting up a secure Lua environment on the client for the pause menu controls or enabling CSM for all users. Which is certainly more work. So I'm fine with the approach of this PR

[swagtoy]

Moving this to Lua would likely involve moving key handling to Lua as well, but this would also mean that we would need to (at least partly) expose an internal API that allows replacing the event handler with a Lua function

That's kind of what I was hinting. As ruben suggested, a binding would work, but regardless, we could still show the keybinds set with Lua, and show a seperate C++ popup to change the keybind. That is also a client-side mod which is more work. Your approach is fine, but do consider it for later.

(Hell, that would be useful even for non-CSM cases; a mod might try to show the users keyboard key for Aux1 as e, when the user really mapped it to r)