Skip to content

Commit

Permalink
[keybinding] don't exclude keybinding collided only by keys
Browse files Browse the repository at this point in the history
Keybindings are only collided when they belong to the same scope. It is not possible to detect without evaluating all keybindings which is expensive. Instead of we order keybindings according to scope semantics and evaluate in such order, the first one wins.

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
  • Loading branch information
akosyakov committed Jan 22, 2020
1 parent d622fc7 commit f80d334
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 9 deletions.
6 changes: 3 additions & 3 deletions packages/core/src/browser/keybinding.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ describe('keybindings', () => {
expect(bindings[0].keybinding).to.be.equal(validKeyBinding);
});

it('shadowed bindings should not be returned', () => {
it('shadowed bindings should be returned last', () => {
const keyCode = KeyCode.createKeyCode({ first: Key.KEY_A, modifiers: [KeyModifier.Shift] });
let bindings: Keybinding[];

Expand Down Expand Up @@ -304,14 +304,14 @@ describe('keybindings', () => {
// now WORKSPACE bindings are overriding the other scopes

bindings = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full;
expect(bindings).to.have.lengthOf(1);
expect(bindings).to.have.lengthOf(3);
expect(bindings[0].command).to.be.equal(workspaceBinding.command);

keybindingRegistry.resetKeybindingsForScope(KeybindingScope.WORKSPACE);
// now it should find USER bindings

bindings = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full;
expect(bindings).to.have.lengthOf(1);
expect(bindings).to.have.lengthOf(2);
expect(bindings[0].command).to.be.equal(userBinding.command);

keybindingRegistry.resetKeybindingsForScope(KeybindingScope.USER);
Expand Down
6 changes: 0 additions & 6 deletions packages/core/src/browser/keybinding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -424,12 +424,6 @@ export class KeybindingRegistry {

for (let scope = KeybindingScope.END; --scope >= KeybindingScope.DEFAULT;) {
const matches = this.getKeySequenceCollisions(this.keymaps[scope], keySequence);

matches.full = matches.full.filter(
binding => this.getKeybindingCollisions(result.full, binding).full.length === 0);
matches.partial = matches.partial.filter(
binding => this.getKeybindingCollisions(result.partial, binding).partial.length === 0);

if (scope === KeybindingScope.DEFAULT_OVERRIDING) {
matches.full.reverse();
matches.partial.reverse();
Expand Down

0 comments on commit f80d334

Please sign in to comment.