Add a KeyChordListener to the Settings UI #10652
Conversation
|
I’ve been expecting this feature for a long time. Thanks for this! |
| const auto key{ e.OriginalKey() }; | ||
| for (const auto mod : ModifierKeys) | ||
| { | ||
| if (key == mod) |
There was a problem hiding this comment.
some implementations show "partial" keys as they go. like, if you press shift it'll say "shift+". Is that desirable here, or too complicated?
There was a problem hiding this comment.
I played with that a little bit. I ended up not liking it because plain modifiers aren't a valid key chord (at least for a key binding, not the object itself).
We could change it at any time, but I vote we see how this feels in a selfhost/bug-bash, then iterate from there.
| } | ||
| } | ||
|
|
||
| const auto modifiers{ _GetModifiers() }; |
There was a problem hiding this comment.
I don't understand the need for _GetModifiers.
If you keep track over how many keys are simultaneously pressed you can use the key down events directly to assemble the modifiers bitfield. This removes the need for the majority of the _GetModifiers implementation.
(You'd also need to reset the modifiers when the control looses focus of course.)
There was a problem hiding this comment.
My gut feeling is that introducing another place where state must be preserved and maintained is going to be worse for code clarity (and has the risk of introducing torn state) where here in a non-perf-sensitive context we can just use the state the UI framework is already keeping track of for us 😄
There was a problem hiding this comment.
I haven't tried out this branch locally but I assumed that it works similar to key bindings in Discord: All key presses are additive, until you let go of any key, after which the result is added.
Meaning if you press:
- M Down
- Ctrl Down
- Shift Down
- M Up
...in that order it'll save Ctrl+Shift+M as a keybinding. Even though I pressed M first, which is important because the UI should work even if I attempt to press all 3 keys simultaneously! (...and then any of those may be pressed first.)
If that's how this is supposed to work as well I disagree: Using the events lends itself to a clearer association between what happens in the UI and how the code would work (namely all key downs being additive, until a key up occurs).
There was a problem hiding this comment.
I've tested it some more in Discord: If you let go of a modifier key for a while it'll count as a keybinding automatically. But I don't think we have to make it that complicated.
Also I just realized a problem with my approach: You still would have to detect when you have released all modifier keys. 😅 As such a good approach might be to count the concurrently pressed keys after all.
I'll approve the PR as I realize that the current code is already written and functional.
There was a problem hiding this comment.
I've been talking with Dustin about this: Making all key presses purely additive would probably be the most consistent way to enter key chords. Discord's keybinding editor is a pretty great template for us.
As I said before we'd need a "concurrently pressed keys counter" though, in order to detect when all keys have been released. Because every time this counter reaches zero we'll have to call KeyChordSerialization::ToString and see if all added up keys formed a valid sequence, as well as to reset the modifiers and vkey member fields.
There was a problem hiding this comment.
It's funny that you say that, because PowerToys' hotkey listener kinda works like that. It constantly updates the control on every key press, but it only sets the setting on a key-up event.
I figured the current approach would be a bit easier and cleaner because we only accept key chords that have an associated non-modifier vkey. This also forces you to actually press the key combination you want to enact, because in TermControl pressing M-Ctrl-Shift would execute the M key binding.
|
Hello @carlos-zamora! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
🎉 Handy links: |
Summary of the Pull Request
Replaces the key chord editor in the actions page with a listener instead of a plain text box.
References
#6900 - Settings UI Epic
Detailed Description of the Pull Request / Additional comments
Actionspage:KeyswithCurrentKeysfor consistency withAction/CurrentActionProposedKeysis now aControl::KeyChordKeyChordListener:Keys: dependency property that hooks us up to a system to the committed setting valuePreviewKeyDownto intercept the key event before some special key bindings are handled (i.e. "select all" in the text box)CoreWindowis used to get the modifier keys because (1) it's easier than updating on each key press and (2) that approach resulted in a strange bug where the Alt key-up event was not detectedLosingFocusmeans that we have completed our operation and want to commit our changes to the key binding view modelKeyDowndoes most of the magic of updatingKeys. We filter out any key chords that could be problematic (i.e. Shift+Tab and Tab for keyboard navigation)Validation Steps Performed
Demo