Feature: Configurable Keybindings for RapidRaw#1083
Merged
Conversation
6 tasks
Owner
|
This looks absolutely amazing!! Really clean refactor and a huge usability win. The configurable keybindings and the move away from nested conditionals make the system much easier to extend and reason about. Looks good to me, will merge now! 💯 |
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Configurable Keybindings for RapidRaw
This PR introduces configurable keybindings/controls for RapidRaw. As a requirement for that, the
handleKeyDownwas refactored going from many nested if/else statements to a simpler to edit matching strategy.This should give users the control they want, and we developers don't break their workflows as easily.
Motivation:
This should maybe get priority over #1078, as the numpad can be bound to tasks now.
For Users:
UI-wise, not a lot has changed (with full intent). To the user, they can now simply click on a keyboard on the shortcuts (now controls) page and record a keybinding.
Users can also "unassign" a function as described above. If keybindings already exist, they are marked as conflicting but not auto-removed. If conflicting keys are kept, the first one in the
handleKeyDownof the conflicting ones fires, should be pretty safe.For Developers:
I tried my best to not bloat the code unnecessarily while allowing for easy extension later.
Key Recording records physical key position through
event.code, more robust on different language keyboards than the actual key.There are two types of keybindings, builtin and user configurable. Builtins are always checked first, meaning the user can't override them.
New keybinds just get added to
KEYBIND_DEFINITIONS. Each one has a condition (shouldFire) and a handler (execute). On key press, builtin guards run first; if none match, the pressed combo is looked up in the user-configured map and its handler runs.Needs Feedback/Discussion/Testing:
Since I don't have access, I'd appreciate some testing there, especially the ctrl+delete shortcuts.
Numlock is currently not respected, as we record the physical key positions.
Right now, keybinds are displayed on a QWERTY keyboard. This is a compromise many packages do, even vscode, but maybe we can do better in the future. Sadly this is not fully supported yet: https://developer.mozilla.org/en-US/docs/Web/API/Keyboard_API
For now, there is just a single keybind per action, but giving a secondary should be easy, do you prefer that?
For keybind that only fire in certain situations, the conflict detection should not fire if they never could collide with another specific keybind.
I was not able to get it running, the regression might actually be not in the keybind code
Right now it's in src/utils/keyboardUtils.ts, which is a bit underwhelming given its importance
Let me know your feedback and ideas, I'd be fine we keep this open until all points are discussed.
Type of Change
Screenshots/Videos
Testing
Test Configuration:
Checklist
AI Disclaimer:
Please state the involvement of AI in this PR: