Skip to content
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

Modifying ChordKeybinding to support multiple chords #65826

Merged
merged 12 commits into from Feb 15, 2019

Conversation

@toumorokoshi
Copy link
Contributor

toumorokoshi commented Dec 29, 2018

Hi! I'm very interested in getting support for keychords of more than two keys (see #6966), so here's a PR to kick off my interest.

I think a see a general strategy to get there, but before I get too deep, I wanted to get a quick review on my approach.

In general, it looks like many parts of the code base are built on the concept of a "firstPart" and a "chordPart" for key chords. I think the first step is modifying all of these to support an arbitrary number of parts. Once that is done, the logic needs to be modified slightly to iteratively run through each part rather than stop at the first two.

As a start to support key chords of longer than two parts,
modifying the internal structure of ChordKeybinding to not specifically
call out two parts.

@alexandrudima you are the assignee of the ticket mentioned. Would you mind giving me some feedback on this approach?

Thank you!

As a start to support key chords of longer than two parts,
modifying the internal structure of ChordKeybinding to not specifically
call out two parts.
@msftclas

This comment has been minimized.

Copy link

msftclas commented Dec 29, 2018

CLA assistant check
All CLA requirements met.

@alexandrudima

This comment has been minimized.

Copy link
Member

alexandrudima commented Jan 9, 2019

👍 This makes sense.

The next step should be to adopt multiple chords in the ResolvedKeybinding subclasses. I've left TODO@chords where this should happen.

@alexandrudima alexandrudima changed the title Modifying ChordKeybinding to support multiple chords [WIP] Modifying ChordKeybinding to support multiple chords Jan 9, 2019
It was difficult to refactor the ResolvedKeybindings without
also refactoring label creators, as the null checks where handled in the
label generation.
The major change with the unit tests was no longer requiring
a secondary null field for dispatchparts. As chords can be any length,
it's more forward-thinking to modify the unit tests to accept new behavior.
@toumorokoshi toumorokoshi force-pushed the toumorokoshi:feature/keychords branch to 50670b9 Jan 17, 2019
@toumorokoshi

This comment has been minimized.

Copy link
Contributor Author

toumorokoshi commented Jan 19, 2019

@alexandrudima I think I have everything for the first major change. I've made the choice to modify some interfaces slightly in a more future direction (return an array of parts vs a chord part). So you'll see a lot of changes there.

Also I'm switching all bindings to ChordKeybinding rather than SingleKeybinding because it removes the conditional code significantly. Curious about your thoughts.

@toumorokoshi

This comment has been minimized.

Copy link
Contributor Author

toumorokoshi commented Feb 4, 2019

@alexandrudima if you have a chance can you take a look at this PR? I've gone as far as I feel comfortable at this point, as I've seen giant prs become harder and harder to merge.

@alexandrudima

This comment has been minimized.

Copy link
Member

alexandrudima commented Feb 15, 2019

@toumorokoshi, I agree this PR has grown large enough.

I've only found a couple of bugs where some map invocations were not properly bound to this in WindowsNativeResolvedKeybinding.getParts and WindowsNativeResolvedKeybinding.getDispatchParts, but other than that this was very high quality, and I thank you for that!

I've also tweaked a bit the label calls and having done so, I plan to further refactor those classes and raise some methods to their abstract parent class.

I've left TODO@chords in two places:

  • where user bindings are parsed (i.e. allow users to write keybindings with N chords in their keybindings.json)
  • where bindings are converted for dispatching (i.e. allow dispatching of N chords)

The first one is rather easy to tackle, the second one is a bit more work, as the KeybindingResolver needs to change a bit.

@alexandrudima alexandrudima added this to the February 2019 milestone Feb 15, 2019
@alexandrudima alexandrudima changed the title [WIP] Modifying ChordKeybinding to support multiple chords Modifying ChordKeybinding to support multiple chords Feb 15, 2019
@alexandrudima alexandrudima merged commit 871a6fe into microsoft:master Feb 15, 2019
3 of 5 checks passed
3 of 5 checks passed
VS Code Build #20190215.7 has test failures
Details
VS Code (macOS) macOS failed
Details
VS Code (Linux) Linux succeeded
Details
VS Code (Windows) Windows succeeded
Details
license/cla All CLA requirements met.
Details
@toumorokoshi

This comment has been minimized.

Copy link
Contributor Author

toumorokoshi commented Feb 16, 2019

Awesome, thanks so much! Darn, I thought I caught all the bound errors... I'll be more careful.

Thanks for the TODOs! I'll take a look at those next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.