[keybindings] Aligned keybindings with VSCode

packages/core/src/browser/keybinding.spec.ts

@@ -253,8 +253,9 @@ describe('keybindings', () => {
         expect(bindings.partial.length > 0);
     });
 
-    it('should not register a shadowing keybinding', () => {
+    it('should register a shadowing keybinding', () => {
         const validKeyBinding = 'ctrlcmd+b a';
+        const otherValidKeyBinding = 'ctrlcmd+b';
         const command = TEST_COMMAND_SHADOW.id;
         const keybindingShadowing: Keybinding[] = [
             {
@@ -263,18 +264,19 @@ describe('keybindings', () => {
             },
             {
                 command,
-                keybinding: 'ctrlcmd+b'
+                keybinding: otherValidKeyBinding
             }
         ];
 
         keybindingRegistry.registerKeybindings(...keybindingShadowing);
 
         const bindings = keybindingRegistry.getKeybindingsForCommand(command);
-        expect(bindings.length).to.be.equal(1);
+        expect(bindings.length).to.be.equal(2);
         expect(bindings[0].keybinding).to.be.equal(validKeyBinding);
+        expect(bindings[1].keybinding).to.be.equal(otherValidKeyBinding);
     });
 
-    it('shadowed bindings should not be returned', () => {
+    it('shadowed bindings should be returned', () => {
         const keyCode = KeyCode.createKeyCode({ first: Key.KEY_A, modifiers: [KeyModifier.Shift] });
         let bindings: Keybinding[];
 
@@ -318,7 +320,7 @@ describe('keybindings', () => {
         // and finally it should fallback to DEFAULT 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(defaultBinding.command);
 
         keybindingRegistry.resetKeybindingsForScope(KeybindingScope.DEFAULT);
@@ -328,6 +330,29 @@ describe('keybindings', () => {
         expect(bindings).to.be.empty;
 
     });
+
+    it('should register conflicting keybindings', () => {
+        const keybindings: Keybinding[] = [{
+            command: TEST_COMMAND.id,
+            keybinding: 'ctrl+c'
+        },
+        {
+            command: TEST_COMMAND2.id,
+            keybinding: 'ctrl+c'
+        }];
+
+        keybindingRegistry.setKeymap(KeybindingScope.USER, keybindings);
+
+        const bindings = keybindingRegistry.getKeybindingsForCommand(TEST_COMMAND.id);
+
+        const keyCode = KeyCode.parse(bindings[0].keybinding);
+        const bindingsForKey = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full;
+        if (bindingsForKey) {
+            expect(bindingsForKey).to.have.lengthOf(2);
+            expect(bindingsForKey[0]).to.be.equal(keybindings[0]);
+            expect(bindingsForKey[1]).to.be.equal(keybindings[1]);
+        }
+    });
 });
 
 const TEST_COMMAND: Command = {

packages/core/src/browser/keybinding.ts

@@ -242,10 +242,8 @@ export class KeybindingRegistry {
     protected doRegisterKeybinding(binding: Keybinding, scope: KeybindingScope = KeybindingScope.DEFAULT) {
         try {
             this.resolveKeybinding(binding);
-            if (this.containsKeybinding(this.keymaps[scope], binding)) {
-                throw new Error(`"${binding.keybinding}" is in collision with something else [scope:${scope}]`);
-            }
             this.keymaps[scope].push(binding);
+            this.keybindingsChanged.fire(undefined);
         } catch (error) {
             this.logger.warn(`Could not register keybinding:\n  ${Keybinding.stringify(binding)}\n${error}`);
         }
@@ -556,7 +554,7 @@ export class KeybindingRegistry {
             return false;
         }
 
-        for (const binding of bindings) {
+        for (const binding of [...bindings].reverse()) {
             if (this.isEnabled(binding, event)) {
                 if (this.isPseudoCommand(binding.command)) {
                     /* Don't do anything, let the event propagate.  */
@@ -594,6 +592,9 @@ export class KeybindingRegistry {
         if (binding.when && !this.whenContextService.match(binding.when, <HTMLElement>event.target)) {
             return false;
         }
+        if (binding.command.startsWith('-')) {
+            return false;
+        }
         return true;
     }
 
@@ -653,7 +654,6 @@ export class KeybindingRegistry {
     setKeymap(scope: KeybindingScope, bindings: Keybinding[]): void {
         this.resetKeybindingsForScope(scope);
         this.doRegisterKeybindings(bindings, scope);
-        this.keybindingsChanged.fire(undefined);
     }
 
     /**

packages/keymaps/src/browser/keybindings-widget.tsx

@@ -440,19 +440,18 @@ export class KeybindingWidget extends ReactWidget {
         for (let i = 0; i < commands.length; i++) {
             // Obtain the keybinding for the given command.
             const keybindings = this.keybindingRegistry.getKeybindingsForCommand(commands[i].id);
-            const item: KeybindingItem = {
-                id: commands[i].id,
-                // Get the command label if available, else use the keybinding id.
-                command: commands[i].label || commands[i].id,
-                keybinding: (keybindings && keybindings[0]) ? keybindings[0].keybinding : '',
-                context: (keybindings && keybindings[0])
-                    ? keybindings[0].context
-                        ? keybindings[0].context : keybindings[0].when
-                    : '',
-                source: (keybindings && keybindings[0] && typeof keybindings[0].scope !== 'undefined')
-                    ? KeybindingScope[keybindings[0].scope!].toLocaleLowerCase() : '',
-            };
-            items.push(item);
+            for (const keybindingCmd of keybindings) {
+                const item: KeybindingItem = {
+                    id: commands[i].id,
+                    // Get the command label if available, else use the keybinding id.
+                    command: commands[i].label || commands[i].id,
+                    keybinding: keybindingCmd.keybinding,
+                    context: keybindingCmd.context ? keybindingCmd.context : (keybindingCmd.when ? keybindingCmd.when : ''),
+                    source: typeof keybindingCmd.scope !== 'undefined'
+                        ? KeybindingScope[keybindingCmd.scope!].toLocaleLowerCase() : '',
+                };
+                items.push(item);
+            }
         }
         // Sort the keybinding item by label.
         const sorted: KeybindingItem[] = items.sort((a: KeybindingItem, b: KeybindingItem) => this.compareItem(a.command, b.command));
@@ -500,6 +499,7 @@ export class KeybindingWidget extends ReactWidget {
         const id = this.getRawValue(item.id);
         const keybinding = (item.keybinding) ? this.getRawValue(item.keybinding) : '';
         const context = (item.context) ? this.getRawValue(item.context) : '';
+        const scope = item.source;
         const dialog = new EditKeybindingDialog({
             title: `Edit Keybinding For ${command}`,
             initialValue: keybinding,
@@ -508,6 +508,10 @@ export class KeybindingWidget extends ReactWidget {
         dialog.open().then(async newKeybinding => {
             if (newKeybinding) {
                 await this.keymapsService.setKeybinding({ 'command': id, 'keybinding': newKeybinding, 'context': context });
+                if (scope === 'default') {
+                    const removalCommand = `-${id}`;
+                    await this.keymapsService.setKeybinding({ 'command': removalCommand, 'keybinding': keybinding, 'context': context });
+                }
             }
         });
     }
@@ -535,7 +539,8 @@ export class KeybindingWidget extends ReactWidget {
         const rawCommand = this.getRawValue(item.command);
         const confirmed = await this.confirmResetKeybinding(rawCommand);
         if (confirmed) {
-            this.keymapsService.removeKeybinding(rawCommandId);
+            await this.keymapsService.removeKeybinding(rawCommandId);
+            await this.keymapsService.removeKeybinding(`-${rawCommandId}`);
         }
     }
 
@@ -552,14 +557,10 @@ export class KeybindingWidget extends ReactWidget {
             return 'keybinding value is required';
         }
         try {
-            const binding = { 'command': command, 'keybinding': keybinding };
             KeySequence.parse(keybinding);
             if (oldKeybinding === keybinding) {
                 return ' '; // if old and new keybindings match, quietly reject update
             }
-            if (this.keybindingRegistry.containsKeybindingInScope(binding)) {
-                return 'keybinding currently collides';
-            }
             return '';
         } catch (error) {
             return error;

packages/plugin-ext/src/main/browser/keybindings/keybindings-contribution-handler.ts

@@ -16,7 +16,7 @@
 
 import { injectable, inject } from 'inversify';
 import { PluginContribution, Keybinding as PluginKeybinding } from '../../../common';
-import { Keybinding, KeybindingRegistry, KeybindingScope } from '@theia/core/lib/browser/keybinding';
+import { Keybinding, KeybindingRegistry } from '@theia/core/lib/browser/keybinding';
 import { ILogger } from '@theia/core/lib/common/logger';
 import { OS } from '@theia/core/lib/common/os';
 
@@ -48,7 +48,7 @@ export class KeybindingsContributionPointHandler {
                 }
             }
         }
-        this.keybindingRegistry.setKeymap(KeybindingScope.USER, keybindings);
+        this.keybindingRegistry.registerKeybindings(...keybindings);
     }
 
     protected toKeybinding(pluginKeybinding: PluginKeybinding): Keybinding | undefined {
@@ -75,7 +75,7 @@ export class KeybindingsContributionPointHandler {
 
     private handlePartialKeybindings(keybinding: Keybinding, partialKeybindings: Keybinding[]) {
         partialKeybindings.forEach(partial => {
-            if (keybinding.context === undefined || keybinding.context === partial.context) {
+            if (keybinding.when === undefined || keybinding.when === partial.context) {
                 this.logger.warn(`Partial keybinding is ignored; ${Keybinding.stringify(keybinding)} shadows ${Keybinding.stringify(partial)}`);
             }
         });
@@ -84,8 +84,6 @@ export class KeybindingsContributionPointHandler {
     private handleShadingKeybindings(keybinding: Keybinding, shadingKeybindings: Keybinding[]) {
         shadingKeybindings.forEach(shadow => {
             if (shadow.context === undefined || shadow.context === keybinding.context) {
-                this.keybindingRegistry.unregisterKeybinding(shadow);
-
                 this.logger.warn(`Shadowing keybinding is ignored; ${Keybinding.stringify(shadow)}, shadows ${Keybinding.stringify(keybinding)}`);
             }
         });