Fix Keybinding disablement and remapping

Make sure disablements for keymappings are only stored once in keymaps.json.
Only check conflicting keybindings against usable keybindings.

Fixes #9094

Contributed by STMicroelectronics
Signed-off-by: Samuel HULTGREN <samuel.hultgren@st.com>

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

@@ -50,6 +50,25 @@ export interface Keybinding {
 }
 export namespace Keybinding {
 
+    /**
+     * 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.

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