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 ResolvedKeybindingItem to support multiple chords #68863

Merged
merged 2 commits into from
Feb 18, 2019

Conversation

toumorokoshi
Copy link
Contributor

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:

{
    "ctrl-x": {
        "ctrl-z": [ResolvedKeybindingItem]
    }
}

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?

@toumorokoshi
Copy link
Contributor Author

quick shout out to #6966 to note there is progress being made!

@alexdima
Copy link
Member

I've left TODO@chords inside keybindingsResolver.ts where assumptions are made. I suggest we merge more often since reviewing is a lot of work... I am not 100% sure about the data-structure. Please give me some more time to think about it.

@alexdima alexdima merged commit eda0071 into microsoft:master Feb 18, 2019
@toumorokoshi
Copy link
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?

@toumorokoshi
Copy link
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.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants