From 62c9a8361c772b4bfa33084fd62bef8eec158f08 Mon Sep 17 00:00:00 2001 From: Anton Kosyakov Date: Tue, 21 Jan 2020 10:46:21 +0000 Subject: [PATCH] [keybinding] don't exclude keybinding collided only by keys 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 --- packages/core/src/browser/keybinding.spec.ts | 6 +-- packages/core/src/browser/keybinding.ts | 46 +------------------- packages/core/src/common/keybinding.ts | 2 + 3 files changed, 7 insertions(+), 47 deletions(-) diff --git a/packages/core/src/browser/keybinding.spec.ts b/packages/core/src/browser/keybinding.spec.ts index f295240f3d349..0a48cddf483d5 100644 --- a/packages/core/src/browser/keybinding.spec.ts +++ b/packages/core/src/browser/keybinding.spec.ts @@ -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[]; @@ -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); diff --git a/packages/core/src/browser/keybinding.ts b/packages/core/src/browser/keybinding.ts index 9bab414ea2a4b..54c52e4811b6c 100644 --- a/packages/core/src/browser/keybinding.ts +++ b/packages/core/src/browser/keybinding.ts @@ -415,22 +415,15 @@ export class KeybindingRegistry { /** * Get the lists of keybindings matching fully or partially matching a KeySequence. - * The lists are sorted by priority (see #sortKeybindingsByPriority). * * @param keySequence The key sequence for which we are looking for keybindings. - * @param event The source keyboard event. */ - getKeybindingsForKeySequence(keySequence: KeySequence, event?: KeyboardEvent): KeybindingRegistry.KeybindingsResult { + getKeybindingsForKeySequence(keySequence: KeySequence): KeybindingRegistry.KeybindingsResult { const result = new KeybindingRegistry.KeybindingsResult(); for (let scope = KeybindingScope.END; --scope >= KeybindingScope.DEFAULT;) { const matches = this.getKeySequenceCollisions(this.keymaps[scope], keySequence); - matches.full = matches.full.filter( - binding => (!event || this.isEnabled(binding, event)) && this.getKeybindingCollisions(result.full, binding).full.length === 0); - matches.partial = matches.partial.filter( - binding => (!event || this.isEnabled(binding, event)) && this.getKeybindingCollisions(result.partial, binding).partial.length === 0); - if (scope === KeybindingScope.DEFAULT_OVERRIDING) { matches.full.reverse(); matches.partial.reverse(); @@ -439,8 +432,6 @@ export class KeybindingRegistry { matches.partial.forEach(binding => binding.scope = scope); result.merge(matches); } - this.sortKeybindingsByPriority(result.full); - this.sortKeybindingsByPriority(result.partial); return result; } @@ -490,39 +481,6 @@ export class KeybindingRegistry { return result; } - /** - * Sort keybindings in-place, in order of priority. - * - * The only criterion right now is that a keybinding with a context has - * more priority than a keybinding with no context. - * - * @param keybindings Array of keybindings to be sorted in-place. - */ - private sortKeybindingsByPriority(keybindings: Keybinding[]): void { - keybindings.sort((a: Keybinding, b: Keybinding): number => { - - let acontext: KeybindingContext | undefined; - if (a.context) { - acontext = this.contexts[a.context]; - } - - let bcontext: KeybindingContext | undefined; - if (b.context) { - bcontext = this.contexts[b.context]; - } - - if (acontext && !bcontext) { - return -1; - } - - if (!acontext && bcontext) { - return 1; - } - - return 0; - }); - } - protected isActive(binding: Keybinding): boolean { /* Pseudo commands like "passthrough" are always active (and not found in the command registry). */ @@ -620,7 +578,7 @@ export class KeybindingRegistry { this.keyboardLayoutService.validateKeyCode(keyCode); this.keySequence.push(keyCode); - const bindings = this.getKeybindingsForKeySequence(this.keySequence, event); + const bindings = this.getKeybindingsForKeySequence(this.keySequence); const full = bindings.full.find(binding => this.isEnabled(binding, event)); const partial = bindings.partial.find(binding => (!full || binding.scope! > full.scope!) && this.isEnabled(binding, event)); if (partial) { diff --git a/packages/core/src/common/keybinding.ts b/packages/core/src/common/keybinding.ts index fa5639498e3ee..497d388ad5c23 100644 --- a/packages/core/src/common/keybinding.ts +++ b/packages/core/src/common/keybinding.ts @@ -23,6 +23,8 @@ export interface Keybinding { * The optional keybinding context where this binding belongs to. * If not specified, then this keybinding context belongs to the NOOP * keybinding context. + * + * @deprecated use `when` closure instead */ context?: string; /**