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

Add keybinding check in case when full and partial bindings are registered #6934

Merged
merged 1 commit into from
Jan 22, 2020

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented Jan 21, 2020

What it does

If full and partial keybindings are registered for the same key sequence, apply the keybinding which has higher priority.

fixes #6916

How to test

Perform some actions in terminal, then execute ctrl + k.
See: terminal content is cleared.

Review checklist

Reminder for reviewers

@akosyakov
Copy link
Member

@vinokurig does vs code has the same meaning of priority by scopes? Or it’s just rely on order and when closure to figure out the higher priority?

Do we need to test emacs extension? I.e. to make sure that the original issue for which check was introduced is still resolved

@akosyakov akosyakov added the keybindings issues related to keybindings label Jan 21, 2020
@vinokurig
Copy link
Contributor Author

@akosyakov

does vs code has the same meaning of priority by scopes? Or it’s just rely on order and when closure to figure out the higher priority?

Vscode has a single collection where full and partial keybindings are placed and the first item that matches context corresponds to key sequence

Do we need to test emacs extension? I.e. to make sure that the original issue for which check was introduced is still resolved

I've tested the emacs extension and didn't found any regression

@akosyakov
Copy link
Member

akosyakov commented Jan 21, 2020

Vscode has a single collection where full and partial keybindings are placed and the first item that matches context corresponds to key sequence

It would mean that they will have the same issue with the terminal? I just wonder whether we need to introduce scope, should not be filtering of keybindings which are not enabled be enough?

@vinokurig
Copy link
Contributor Author

vinokurig commented Jan 21, 2020

They don't have this issue because the list of keybindings is properly sorted. I suppose they use priorities in the step of collecting the list of keybindings: https://github.com/microsoft/vscode/blob/e7f559cd3b3127347203b1ef4e801689ca96f45e/src/vs/platform/keybinding/common/keybindingResolver.ts#L78-L102

@akosyakov
Copy link
Member

the list of keybindings is properly sorted

@vinokurig By that you mean that defaults comes before overrides, correct?

@vinokurig
Copy link
Contributor Author

yes

packages/core/src/browser/keybinding.ts Outdated Show resolved Hide resolved
packages/core/src/browser/keybinding.ts Outdated Show resolved Hide resolved
…tered

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works for me now, thank you @vinokurig

@vinokurig vinokurig merged commit 112f376 into master Jan 22, 2020
@vinokurig vinokurig deleted the theia-6916 branch January 22, 2020 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keybindings issues related to keybindings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cannot clean terminal with keybindings anymore
2 participants