Modifying ResolvedKeybindingItem to support multiple chords#68863
Merged
alexdima merged 2 commits intomicrosoft:masterfrom Feb 18, 2019
Merged
Modifying ResolvedKeybindingItem to support multiple chords#68863alexdima merged 2 commits intomicrosoft:masterfrom
alexdima merged 2 commits intomicrosoft:masterfrom
Conversation
Contributor
Author
|
quick shout out to #6966 to note there is progress being made! |
alexdima
force-pushed
the
feature/keychords
branch
from
9d96c68 to
e0ba314
Compare
Member
|
I've left |
Contributor
Author
|
@alexandrudima sounds good. Do you want to let me know when you have an idea about what to do next? Or should I take a stab at a PR to start the discussion? |
Contributor
Author
|
@alexandrudima revisitting this. Have you thought more about the data structure? Would a PR to start the conversation be helpful? Feels like we're close, would love to get this moving forward. |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
A continuation of #65826, this handles one of the TODO items thus far (ResolvedKeybindingItem).
Before I move forward I wanted to do a quick preliminary discussion. @alexandrudima as you mentioned, keybindingResolver.ts looks to need a significant refactor to make this work and keep the code clean.
My thoughts from a high-level:
Instead of having a map of <key, [ResolvedKeybindingItem]> pairs, instead have a nested map of Keys to a multiple [ResolvedKeybindingItem] (multiple because there could multiple matches that satisfy different contexts). E.g. ctrl-x ctrl-z would look like:
I think this would be faster than a list as you wouldn't have to iterate through all options and check if the keychord matches. Otherwise this will probably be noticeable with really deep keychords.
After that, it's a matter of ensuring that the existing rules around overrides and context matches works as expected.
Does that sound ok?