Store keybindings properly in keymaps.json and improve conflict checking

packages/core/src/browser/keybinding.ts

@@ -278,7 +278,7 @@ export class KeybindingRegistry {
      */
     containsKeybindingInScope(binding: common.Keybinding, scope = KeybindingScope.USER): boolean {
         const bindingKeySequence = this.resolveKeybinding(binding);
-        const collisions = this.getKeySequenceCollisions(this.keymaps[scope], bindingKeySequence)
+        const collisions = this.getKeySequenceCollisions(this.getUsableBindings(this.keymaps[scope]), bindingKeySequence)
             .filter(b => b.context === binding.context && !b.when && !binding.when);
         if (collisions.full.length > 0) {
             return true;
@@ -564,7 +564,7 @@ export class KeybindingRegistry {
                 return false;
             }
             const { command, context, when, keybinding } = binding;
-            if (binding.command.charAt(0) === '-') {
+            if (!this.isUsable(binding)) {
                 disabled = disabled || new Set<string>();
                 disabled.add(JSON.stringify({ command: command.substr(1), context, when, keybinding }));
                 return false;
@@ -587,6 +587,22 @@ export class KeybindingRegistry {
         return undefined;
     }
 
+    /**
+     * Returns true if the binding is usable
+     * @param binding Binding to be checked
+     */
+    protected isUsable(binding: common.Keybinding): boolean {
+        return binding.command.charAt(0) !== '-';
+    }
+
+    /**
+     * Return a new filtered array containing only the usable bindings among the input bindings
+     * @param bindings Bindings to filter
+     */
+    protected getUsableBindings<T extends common.Keybinding>(bindings: T[]): T[] {
+        return bindings.filter(binding => this.isUsable(binding));
+    }
+
     /**
      * Return true of string a pseudo-command id, in other words a command id
      * that has a special meaning and that we won't find in the command

packages/core/src/common/keybinding.ts

@@ -51,21 +51,49 @@ export interface Keybinding {
 export namespace Keybinding {
 
     /**
-     * Returns with the string representation of the binding.
-     * Any additional properties which are not described on
-     * the `Keybinding` API will be ignored.
+     * Compares two keybindings for equality.
+     * Can optionally ignore the keybinding and/or args property in the comparison.
+     * @param a The first Keybinding in the comparison
+     * @param b The second Keybinding in the comparison
+     * @param ignoreKeybinding Ignore the 'keybinding' property in the comparison
+     * @param ignoreArgs Ignore the 'args' property in the comparison
+     */
+    export function equals(a: Keybinding, b: Keybinding, ignoreKeybinding: boolean = false, ignoreArgs: boolean = false): boolean {
+        if (a.command === b.command &&
+            (a.context || '') === (b.context || '') &&
+            (a.when || '') === (b.when || '') &&
+            (ignoreKeybinding || a.keybinding === b.keybinding) &&
+            (ignoreArgs || (a.args || '') === (b.args || ''))) {
+            return true;
+        }
+        return false;
+    }
+
+    /**
+     * Returns a new object only containing properties which
+     * are described on the `Keybinding` API.
      *
-     * @param binding the binding to stringify.
+     * @param binding the binding to create an API object for.
      */
-    export function stringify(binding: Keybinding): string {
-        const copy: Keybinding = {
+    export function apiObjectify(binding: Keybinding): Keybinding {
+        return {
             command: binding.command,
             keybinding: binding.keybinding,
             context: binding.context,
             when: binding.when,
             args: binding.args
         };
-        return JSON.stringify(copy);
+    }
+
+    /**
+     * Returns with the string representation of the binding.
+     * Any additional properties which are not described on
+     * the `Keybinding` API will be ignored.
+     *
+     * @param binding the binding to stringify.
+     */
+    export function stringify(binding: Keybinding): string {
+        return JSON.stringify(apiObjectify(binding));
     }
 
     /* Determine whether object is a KeyBinding */

packages/keymaps/src/browser/keymaps-service.ts

@@ -121,19 +121,14 @@ export class KeymapsService {
             let oldRemoved = false;
             const keybindings = [];
             for (let keybinding of this.keybindingRegistry.getKeybindingsByScope(KeybindingScope.USER)) {
-                if (keybinding.command === newKeybinding.command &&
-                    (keybinding.context || '') === (newKeybinding.context || '') &&
-                    (keybinding.when || '') === (newKeybinding.when || '')) {
+                if (Keybinding.equals(keybinding, newKeybinding, true, true)) {
                     newAdded = true;
                     keybinding = {
                         ...keybinding,
                         keybinding: newKeybinding.keybinding
                     };
                 }
-                if (oldKeybinding && keybinding.keybinding === oldKeybinding &&
-                    keybinding.command === '-' + newKeybinding.command &&
-                    (keybinding.context || '') === (newKeybinding.context || '') &&
-                    (keybinding.when || '') === (newKeybinding.when || '')) {
+                if (oldKeybinding && Keybinding.equals(keybinding, { ...newKeybinding, keybinding: oldKeybinding, command: '-' + newKeybinding.command }, false, true)) {
                     oldRemoved = true;
                 }
                 keybindings.push(keybinding);
@@ -149,14 +144,18 @@ export class KeymapsService {
                 newAdded = true;
             }
             if (!oldRemoved && oldKeybinding) {
-                keybindings.push({
+                const disabledBinding = {
                     command: '-' + newKeybinding.command,
                     // TODO key: oldKeybinding, see https://github.com/eclipse-theia/theia/issues/6879
                     keybinding: oldKeybinding,
                     context: newKeybinding.context,
                     when: newKeybinding.when,
                     args: newKeybinding.args
-                });
+                };
+                // Add disablement of the old keybinding if it isn't already disabled in the list to avoid duplicate disabled entries
+                if (!keybindings.some(binding => Keybinding.equals(binding, disabledBinding, true, true))) {
+                    keybindings.push(disabledBinding);
+                }
                 oldRemoved = true;
             }
             if (newAdded || oldRemoved) {
@@ -189,7 +188,7 @@ export class KeymapsService {
                 const textModel = model.textEditorModel;
                 const { insertSpaces, tabSize, defaultEOL } = textModel.getOptions();
                 const editOperations: monaco.editor.IIdentifiedSingleEditOperation[] = [];
-                for (const edit of jsoncparser.modify(content, [], keybindings, {
+                for (const edit of jsoncparser.modify(content, [], keybindings.map(binding => Keybinding.apiObjectify(binding)), {
                     formattingOptions: {
                         insertSpaces,
                         tabSize,