Add a KeyChordListener to the Settings UI #10652

carlos-zamora · 2021-07-13T23:31:43Z

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

Actions page:
- Replace Keys with CurrentKeys for consistency with Action/CurrentAction
- ProposedKeys is now a Control::KeyChord
- removes key chord validation (now we don't need it)
- removes accept/cancel shortcuts (nowhere we could use it now)
KeyChordListener:
- Keys: dependency property that hooks us up to a system to the committed setting value
  - this is the key binding view model, which propagates the change to the settings model clone on "accept changes"
- We bind to PreviewKeyDown to intercept the key event before some special key bindings are handled (i.e. "select all" in the text box)
- CoreWindow is 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 detected
- LosingFocus means that we have completed our operation and want to commit our changes to the key binding view model
- KeyDown does most of the magic of updating Keys. We filter out any key chords that could be problematic (i.e. Shift+Tab and Tab for keyboard navigation)

Validation Steps Performed

Tested a few key chords:
- ✅single key: X
- ✅key with modifier(s): Ctrl+Alt+X
- ❌plain modifier: Ctrl
- ✅key that is used by text box: Ctrl+A
- ✅key that is used by Windows Terminal: Alt+F4
- ❌key that is taken by Windows OS: Windows+X
- ✅key that is not taken by Windows OS: Windows+Shift+X
Known issue:
- global key taken by Windows Terminal: (i.e. quake mode keybinding)
  - Behavior: global key binding executed
  - Expected: key chord recorded

Demo

skyline75489 · 2021-07-13T23:36:52Z

I’ve been expecting this feature for a long time. Thanks for this!

DHowett

Despite my comments, I'm totally cool with this. Well done!

src/cascadia/TerminalSettingsEditor/KeyChordListener.cpp

src/cascadia/TerminalSettingsEditor/Actions.h

src/cascadia/TerminalSettingsEditor/KeyChordListener.xaml

src/cascadia/TerminalSettingsEditor/KeyChordListener.cpp

DHowett · 2021-07-14T20:08:48Z

src/cascadia/TerminalSettingsEditor/KeyChordListener.cpp

+        const auto key{ e.OriginalKey() };
+        for (const auto mod : ModifierKeys)
+        {
+            if (key == mod)


some implementations show "partial" keys as they go. like, if you press shift it'll say "shift+". Is that desirable here, or too complicated?

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.

lhecker

Just one question and some unimportant nits.

src/cascadia/TerminalSettingsEditor/KeyChordListener.cpp

lhecker · 2021-07-16T18:14:18Z

src/cascadia/TerminalSettingsEditor/KeyChordListener.cpp

+            }
+        }
+
+        const auto modifiers{ _GetModifiers() };


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.)

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 😄

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).

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.

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.

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.

src/cascadia/TerminalSettingsEditor/KeyChordListener.cpp

src/cascadia/TerminalSettingsEditor/Actions.cpp

lhecker

I'll approve the PR as I realize that the current code is already written and functional.

There's a couple nits I commented before which you can tackle if you agree about them.

ghost · 2021-07-16T21:49:41Z

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2021-08-31T17:11:27Z

🎉Windows Terminal Preview v1.11.2421.0 has been released which incorporates this pull request.:tada:

Handy links:

Add a KeyChordListener to the Settings UI

5c81c07

carlos-zamora mentioned this pull request Jul 14, 2021

Epic: Introduce Actions page to the Settings UI #6900

Open

12 tasks

DHowett approved these changes Jul 14, 2021

View reviewed changes

apply Dustin's feedback

bbaf737

carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Jul 16, 2021

ghost requested review from zadjii-msft, miniksa, leonMSFT, PankajBhojwani and lhecker July 16, 2021 17:32

lhecker reviewed Jul 16, 2021

View reviewed changes

lhecker approved these changes Jul 16, 2021

View reviewed changes

address suggestions from Leonard

dc38f9f

carlos-zamora added AutoMerge Marked for automatic merge by the bot when requirements are met and removed Needs-Second It's a PR that needs another sign-off labels Jul 16, 2021

ghost merged commit 8947909 into main Jul 16, 2021

ghost deleted the dev/cazamor/actions-page/key-listener branch July 16, 2021 22:12

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a KeyChordListener to the Settings UI #10652

Add a KeyChordListener to the Settings UI #10652

carlos-zamora commented Jul 13, 2021 •

edited

Loading

skyline75489 commented Jul 13, 2021

DHowett left a comment

DHowett Jul 14, 2021

carlos-zamora Jul 14, 2021

lhecker left a comment

lhecker Jul 16, 2021

DHowett Jul 16, 2021

lhecker Jul 16, 2021 •

edited

Loading

lhecker Jul 16, 2021

lhecker Jul 16, 2021

carlos-zamora Jul 16, 2021

lhecker left a comment

ghost commented Jul 16, 2021

ghost commented Aug 31, 2021

Add a KeyChordListener to the Settings UI #10652

Add a KeyChordListener to the Settings UI #10652

Conversation

carlos-zamora commented Jul 13, 2021 • edited Loading

Summary of the Pull Request

References

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Demo

skyline75489 commented Jul 13, 2021

DHowett left a comment

Choose a reason for hiding this comment

DHowett Jul 14, 2021

Choose a reason for hiding this comment

carlos-zamora Jul 14, 2021

Choose a reason for hiding this comment

lhecker left a comment

Choose a reason for hiding this comment

lhecker Jul 16, 2021

Choose a reason for hiding this comment

DHowett Jul 16, 2021

Choose a reason for hiding this comment

lhecker Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

lhecker Jul 16, 2021

Choose a reason for hiding this comment

lhecker Jul 16, 2021

Choose a reason for hiding this comment

carlos-zamora Jul 16, 2021

Choose a reason for hiding this comment

lhecker left a comment

Choose a reason for hiding this comment

ghost commented Jul 16, 2021

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 (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Aug 31, 2021

carlos-zamora commented Jul 13, 2021 •

edited

Loading

lhecker Jul 16, 2021 •

edited

Loading

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 (`@msftbot`) and give me an instruction to get started! Learn more here.