Skip to content

Allow keybindings with modifiers #14874

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

Closed
wants to merge 13 commits into from
Closed

Conversation

y5nw
Copy link
Contributor

@y5nw y5nw commented Jul 20, 2024

  • Goal of the PR
    This PR allows users to create keybindings with modifier keys, such as Control-F10 or Shift-5.
  • How does the PR work?
    This PR extends the KeyPress class to include information on the state of modifier keys.
  • Does it resolve any reported issue?
    No.
  • Does this relate to a goal in the roadmap?
    No
  • If not a bug fix, why is this PR needed? What usecases does it solve?
    This provides a partial workaround for SDL: Some keybinds broken due to missing character lookup #14545 by allowing users to use e.g. Shift-7 instead of / for keybindings. Some people may want to continue using e.g. Shift-7, which will no longer be possible with SDL: Use scancodes for keybindings #14964.

Notes

  • This PR now treats Left Shift and Right Shift as the same key.
  • This PR removes the incompatible feature where Shift+key binds to the "character" rather than the keycode. However, my testing shows that this removed feature did not fully work anyway. (Update: This feature is already dropped by SDL: Use scancodes for keybindings #14964).

To do

This PR is a Draft.

  • Adapt input handler to handle such keybindings.
  • "Resolve" keybindings to perform action(s) with the closest keybinding.
  • Update GUI (depends on Move keybinding settings to (Lua-based) setting menu #15791)
  • Fix bugs/regressions
    • Cannot bind an action to the Shift or Control key.
    • Cannot stop sneaking after pressing Shift once.
    • Cannot quit keybinding form with Esc after binding a key to Shift or Ctrl
    • Cannot perform action bound to a normal key when Shift-(key) or Ctrl-(key) is pressed.
    • Chat key does not work when bound to Modifier+Autoforward.
  • Add tests
    • KeyPress
    • KeyList
    • MyEventReceiver (src/client/inputhandler.h)

How to test

  • The testcases are complemented to reflect the changes made in this PR.
  • The above section includes a list of bugs/regressions that occurred during the development of the PR. These should not be reproducible.

@Zughy Zughy added the Bugfix 🐛 PRs that fix a bug label Jul 21, 2024
@y5nw y5nw marked this pull request as ready for review July 27, 2024 01:38
@Desour Desour added the Feature ✨ PRs that add or enhance a feature label Jul 27, 2024
@y5nw y5nw mentioned this pull request Sep 2, 2024
11 tasks
@y5nw y5nw mentioned this pull request Sep 12, 2024
8 tasks
@y5nw y5nw added the Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) label Feb 15, 2025
@y5nw y5nw marked this pull request as draft March 16, 2025 20:27
@y5nw y5nw added the Rebase needed The PR needs to be rebased by its author label Mar 16, 2025
@y5nw y5nw added Adoption needed The pull request needs someone to adopt it. Adoption welcomed! and removed Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) labels May 31, 2025
@y5nw
Copy link
Contributor Author

y5nw commented May 31, 2025

Closing for now. #15934 has higher priority IMO, and its (potential) followups are more impactful.

@y5nw y5nw closed this May 31, 2025
@y5nw y5nw removed the Bugfix 🐛 PRs that fix a bug label May 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Adoption needed The pull request needs someone to adopt it. Adoption welcomed! @ Client / Controls / Input Feature ✨ PRs that add or enhance a feature Rebase needed The PR needs to be rebased by its author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants