[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 <[email protected]>

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);

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) {

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;
     /**