Add support for three or more chord keyboard shortcuts #175253

dyedgreen · 2023-02-23T16:00:12Z

Closes #6966.

Allow for three or more steps in keyboard shortcut cords.

This change only allows for such chords to be configured by the user in the key bindings JSON (by adding multiple steps with spaces; this was already parsed correctly, but the key binding machinery just ignored any keys past the first two).

The interactive keybinding recording widget is also updated to allow for unlimited length chord sequences.

Roughly, this is done by addressing the existing TODO@chords comments in the code.

alexdima · 2023-02-24T14:32:20Z

@dyedgreen Thank you for your PR. In order for me to be able to review it, please accept the Contributor License Agreement (see above comment)

dyedgreen · 2023-02-24T16:40:40Z

Thanks for getting back to me so quickly @alexdima!

I’m checking with my employer to see if I need to include them here / should accept purely myself and will accept the CLA here as soon as they get back to me 😅

dyedgreen · 2023-03-02T02:27:36Z

@microsoft-github-policy-service agree company="Jane Street"

ulugbekna

Thanks for the PR @dyedgreen (a fellow OCaml'er, I guess? 🐫)

The PR looks good, a couple of inline comments.

src/vs/platform/keybinding/test/common/keybindingsTestUtils.ts

src/vs/platform/keybinding/common/keybindingResolver.ts

dyedgreen · 2023-03-09T12:13:19Z

Thanks for the review @ulugbekna! I've made a few minor tweaks based on your comments, let me know if they make sense.

And yes, we famously write even our hardware in OCaml 😄

dyedgreen · 2023-03-14T11:53:16Z

Just to check, is there anything else required from my side for this to be merged?

ulugbekna · 2023-03-14T12:16:49Z

This is awaiting @alexdima 's review. He's very occupied right now, but we haven't forgotten about the PR. Thanks for patience with this

ulugbekna · 2023-03-15T08:51:22Z

@dyedgreen btw, would you be interested in implementing support for multi-chord keybindings in the keybindings editor? The feature would become complete and more likely to be shipped.

dyedgreen · 2023-03-15T10:08:25Z

I can take a look at how hard that would be 👍

dyedgreen · 2023-03-15T19:33:11Z

@ulugbekna I've updated the PR to also allow unlimited chords in the keybinding recording widgets.

This means if you miss-click while recording, you have to abort (escape) and retry; previously it would let you cycle and try again although I personally found that confusing.

ulugbekna

First, big thanks for being active with the PR and great work. The changes look good. I see two issues:

Search by keybinding prefix works only for 2 chords, ie:
- cmd+k finds keybindings that have their first chord cmd+k
- cmd+k cmd+k finds keybindings that have their 1st and 2nd chord cmd+k cmd+k
- but cmd+k cmd+k cmd+k doesn’t find the keybinding cmd+k cmd+k cmd+k cmd+i - user has to insert the exact keybinding to find itI think it would be great to fix before merging.

I think this needs to be fixed before merging.

In the define keybinding widget, we don't resize on overflow. I don't think people would be defining KBs with > 5 chords, but this remains an issue.

ulugbekna · 2023-03-16T11:10:35Z

@sandy081 it would be great to have a review from you for the keybinding widget changes

ulugbekna · 2023-03-16T11:40:11Z

Upd: I'll make a commit to this PR that restricts the UI to 2 chords, so that we don't burden you with more work and merge the PR sooner. We'll address the UI changes in another PR.

alexdima

This looks great! I've pushed a change to limit the chord count to 2 in the widget for now. We should improve this in the widget itself via separate PRs. However, there is one problem where modifier only keybindings no longer work e.g.:

    {
      "key": "shift shift",
      "command": "actions.find"
    }

alexdima · 2023-03-16T11:48:26Z

src/vs/platform/keybinding/common/abstractKeybindingService.ts


 		if (isSingleModiferChord) {
 			const [dispatchKeyname,] = keybinding.getSingleModifierDispatchChords();
 			firstChord = dispatchKeyname;
-			currentChord = dispatchKeyname;
+			currentChord = null;


Does a keybinding like shift shift still work after this change?

I think it might not. I’ll take a look at how we can make this work again 👀

I pushed some changes to restore the previous behavior. The way the single modifier chords are set-up, they can't be chorded to arbitrary length, but I feel like that's okay? (It fully restores the old behavior, while allowing arbitrary length chords for regular keys.)

This reverts commit 55c32ae.

alexdima

👍 Thank you!

ulugbekna

great work, thanks heaps! merging :-)

anddam · 2023-03-20T22:59:16Z

Thanks @dyedgreen and everybody involved for this.

When can end users expect this commit to land in a stable MS release?

dyedgreen added 3 commits February 23, 2023 23:37

Add support for three or more chords in shortcuts

41b0166

Add tests and fix bug that was uncovered

2cb48a3

Clean up TODO comments

9086a4f

vscodenpa assigned alexdima Feb 23, 2023

dyedgreen added 3 commits February 24, 2023 06:54

Merge branch 'main' into support-three-or-more-chords

c705bac

Make multi chords more explicit in quickInput.ts

645569a

Merge branch 'main' into support-three-or-more-chords

12b086c

dyedgreen marked this pull request as ready for review February 24, 2023 00:11

dyedgreen changed the title ~~Add support for three or more chords in shortcuts~~ Add support for three or more chord keyboard shortcuts Feb 24, 2023

alexdima self-requested a review February 24, 2023 14:31

dyedgreen added 4 commits February 25, 2023 21:49

Merge branch 'main' into support-three-or-more-chords

f4e6a83

Merge branch 'main' into support-three-or-more-chords

11b3cfd

Merge branch 'main' into support-three-or-more-chords

3c05b51

Merge branch 'main' into support-three-or-more-chords

0dfd715

dyedgreen added 3 commits March 4, 2023 10:14

Merge branch 'main' into support-three-or-more-chords

1df309c

Merge branch 'main' into support-three-or-more-chords

5605923

Merge branch 'main' into support-three-or-more-chords

8bd1a68

alexdima requested a review from ulugbekna March 7, 2023 14:29

Merge branch 'main' into support-three-or-more-chords

decf8c1

ulugbekna previously approved these changes Mar 8, 2023

View reviewed changes

src/vs/platform/keybinding/test/common/keybindingsTestUtils.ts Show resolved Hide resolved

src/vs/platform/keybinding/common/keybindingResolver.ts Outdated Show resolved Hide resolved

src/vs/platform/keybinding/common/keybindingResolver.ts Show resolved Hide resolved

ulugbekna self-assigned this Mar 9, 2023

Address review comments

eaeaeaf

dyedgreen dismissed ulugbekna’s stale review via eaeaeaf March 9, 2023 12:03

dyedgreen and others added 2 commits March 9, 2023 20:13

Merge branch 'main' into support-three-or-more-chords

a0d1293

keybindings: resolver: align terminology with rest of code

37612b5

ulugbekna previously approved these changes Mar 9, 2023

View reviewed changes

Allow input of >2 chords in keybinding settings

cffeedd

dyedgreen dismissed ulugbekna’s stale review via cffeedd March 15, 2023 19:30

Merge branch 'main' into support-three-or-more-chords

5303e9d

dyedgreen requested review from ulugbekna and removed request for alexdima March 16, 2023 06:52

ulugbekna requested changes Mar 16, 2023

View reviewed changes

Keep the limit at 2 chords in the widget for now

55c32ae

alexdima requested changes Mar 16, 2023

View reviewed changes

sandy081 self-requested a review March 16, 2023 13:57

ulugbekna and others added 3 commits March 16, 2023 15:16

Revert "Keep the limit at 2 chords in the widget for now"

1344a6d

This reverts commit 55c32ae.

keybindings: UI: search widget: limit chords to 2 for now

c87d70b

Fix single modifier chords

d7bb138

dyedgreen requested review from alexdima and removed request for sandy081 March 19, 2023 15:56

Remove unused import

3bf9003

alexdima approved these changes Mar 20, 2023

View reviewed changes

alexdima requested a review from ulugbekna March 20, 2023 07:16

ulugbekna approved these changes Mar 20, 2023

View reviewed changes

ulugbekna merged commit a0a30a5 into microsoft:main Mar 20, 2023

alexdima added this to the March 2023 milestone Mar 20, 2023

github-actions bot locked and limited conversation to collaborators May 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for three or more chord keyboard shortcuts #175253

Add support for three or more chord keyboard shortcuts #175253

dyedgreen commented Feb 23, 2023 •

edited

Loading

alexdima commented Feb 24, 2023

dyedgreen commented Feb 24, 2023 •

edited

Loading

dyedgreen commented Mar 2, 2023

ulugbekna left a comment

dyedgreen commented Mar 9, 2023 •

edited

Loading

dyedgreen commented Mar 14, 2023

ulugbekna commented Mar 14, 2023

ulugbekna commented Mar 15, 2023 •

edited

Loading

dyedgreen commented Mar 15, 2023

dyedgreen commented Mar 15, 2023

ulugbekna left a comment

ulugbekna commented Mar 16, 2023

ulugbekna commented Mar 16, 2023

alexdima left a comment

alexdima Mar 16, 2023

dyedgreen Mar 17, 2023

dyedgreen Mar 19, 2023

alexdima left a comment

ulugbekna left a comment

anddam commented Mar 20, 2023 •

edited

Loading

Add support for three or more chord keyboard shortcuts #175253

Add support for three or more chord keyboard shortcuts #175253

Conversation

dyedgreen commented Feb 23, 2023 • edited Loading

alexdima commented Feb 24, 2023

dyedgreen commented Feb 24, 2023 • edited Loading

dyedgreen commented Mar 2, 2023

ulugbekna left a comment

Choose a reason for hiding this comment

dyedgreen commented Mar 9, 2023 • edited Loading

dyedgreen commented Mar 14, 2023

ulugbekna commented Mar 14, 2023

ulugbekna commented Mar 15, 2023 • edited Loading

dyedgreen commented Mar 15, 2023

dyedgreen commented Mar 15, 2023

ulugbekna left a comment

Choose a reason for hiding this comment

ulugbekna commented Mar 16, 2023

ulugbekna commented Mar 16, 2023

alexdima left a comment

Choose a reason for hiding this comment

alexdima Mar 16, 2023

Choose a reason for hiding this comment

dyedgreen Mar 17, 2023

Choose a reason for hiding this comment

dyedgreen Mar 19, 2023

Choose a reason for hiding this comment

alexdima left a comment

Choose a reason for hiding this comment

ulugbekna left a comment

Choose a reason for hiding this comment

anddam commented Mar 20, 2023 • edited Loading

dyedgreen commented Feb 23, 2023 •

edited

Loading

dyedgreen commented Feb 24, 2023 •

edited

Loading

dyedgreen commented Mar 9, 2023 •

edited

Loading

ulugbekna commented Mar 15, 2023 •

edited

Loading

anddam commented Mar 20, 2023 •

edited

Loading