From 886c40a69acff5a3d6ef848a515765c8c51ecfbc Mon Sep 17 00:00:00 2001
From: Chenlei Hu <huchenlei@proton.me>
Date: Thu, 21 Nov 2024 11:49:57 -0500
Subject: [PATCH] Fix keybinding conflict (#1630)

---
 src/stores/coreKeybindings.ts                 | 11 ---
 src/stores/keybindingStore.ts                 | 27 ++++---
 .../tests/slow/store/keybindingStore.test.ts  | 72 +++++++++++++++++++
 3 files changed, 84 insertions(+), 26 deletions(-)

diff --git a/src/stores/coreKeybindings.ts b/src/stores/coreKeybindings.ts
index 8e3066bb8..3d0d60ec3 100644
--- a/src/stores/coreKeybindings.ts
+++ b/src/stores/coreKeybindings.ts
@@ -182,14 +182,3 @@ export const CORE_KEYBINDINGS: Keybinding[] = [
     commandId: 'Workspace.ToggleFocusMode'
   }
 ]
-
-export const DEPRECATED_KEYBINDINGS: Keybinding[] = [
-  // Ctrl+S is now used for saving the workflow after v1.4.6.
-  {
-    combo: {
-      key: 's',
-      ctrl: true
-    },
-    commandId: 'Comfy.ExportWorkflow'
-  }
-]
diff --git a/src/stores/keybindingStore.ts b/src/stores/keybindingStore.ts
index 103fba7e6..057e8c6ac 100644
--- a/src/stores/keybindingStore.ts
+++ b/src/stores/keybindingStore.ts
@@ -2,7 +2,7 @@ import { defineStore } from 'pinia'
 import { computed, Ref, ref, toRaw } from 'vue'
 import { Keybinding, KeyCombo } from '@/types/keyBindingTypes'
 import { useSettingStore } from './settingStore'
-import { CORE_KEYBINDINGS, DEPRECATED_KEYBINDINGS } from './coreKeybindings'
+import { CORE_KEYBINDINGS } from './coreKeybindings'
 import type { ComfyExtension } from '@/types/comfy'
 
 export class KeybindingImpl implements Keybinding {
@@ -121,8 +121,7 @@ export const useKeybindingStore = defineStore('keybinding', () => {
 
   const keybindingByKeyCombo = computed<Record<string, KeybindingImpl>>(() => {
     const result: Record<string, KeybindingImpl> = {
-      ...defaultKeybindings.value,
-      ...userKeybindings.value
+      ...defaultKeybindings.value
     }
 
     for (const keybinding of Object.values(userUnsetKeybindings.value)) {
@@ -131,7 +130,11 @@ export const useKeybindingStore = defineStore('keybinding', () => {
         delete result[serializedCombo]
       }
     }
-    return result
+
+    return {
+      ...result,
+      ...userKeybindings.value
+    }
   })
 
   const keybindings = computed<KeybindingImpl[]>(() =>
@@ -215,13 +218,13 @@ export const useKeybindingStore = defineStore('keybinding', () => {
     addKeybinding(userKeybindings, keybinding, { existOk: true })
   }
 
-  const deprecatedKeybindings = DEPRECATED_KEYBINDINGS.map(
-    (k) => new KeybindingImpl(k)
-  )
   function unsetKeybinding(keybinding: KeybindingImpl) {
     const serializedCombo = keybinding.combo.serialize()
     if (!(serializedCombo in keybindingByKeyCombo.value)) {
-      throw new Error(`Keybinding on ${keybinding.combo} does not exist`)
+      console.warn(
+        `Trying to unset non-exist keybinding: ${JSON.stringify(keybinding)}`
+      )
+      return
     }
 
     if (userKeybindings.value[serializedCombo]?.equals(keybinding)) {
@@ -234,13 +237,7 @@ export const useKeybindingStore = defineStore('keybinding', () => {
       return
     }
 
-    if (deprecatedKeybindings.some((k) => k.equals(keybinding))) {
-      return
-    }
-
-    console.warn(
-      `Trying to unset non-exist keybinding: ${JSON.stringify(keybinding)}`
-    )
+    throw new Error(`Unknown keybinding: ${JSON.stringify(keybinding)}`)
   }
 
   /**
diff --git a/tests-ui/tests/slow/store/keybindingStore.test.ts b/tests-ui/tests/slow/store/keybindingStore.test.ts
index 245c25ef1..35aee13fe 100644
--- a/tests-ui/tests/slow/store/keybindingStore.test.ts
+++ b/tests-ui/tests/slow/store/keybindingStore.test.ts
@@ -160,4 +160,76 @@ describe('useKeybindingStore', () => {
       defaultKeybinding
     )
   })
+
+  it('Should accept same keybinding from default and user', () => {
+    const store = useKeybindingStore()
+    const keybinding = new KeybindingImpl({
+      commandId: 'test.command',
+      combo: { key: 'J', ctrl: true }
+    })
+    // Add default keybinding.
+    // This can happen when we change default keybindings.
+    store.addDefaultKeybinding(keybinding)
+    // Add user keybinding.
+    store.addUserKeybinding(keybinding)
+
+    expect(store.keybindings).toHaveLength(1)
+    expect(store.getKeybinding(keybinding.combo)).toEqual(keybinding)
+  })
+
+  it('Should keep previously customized keybindings after default keybindings change', () => {
+    // Initially command 'foo' was bound to 'K, Ctrl'. User unset it and bound the
+    // command to 'A, Ctrl'.
+    // Now we change the default keybindings of 'foo' to 'A, Ctrl'.
+    // The user customized keybinding should be kept.
+    const store = useKeybindingStore()
+
+    const userUnsetKeybindings = [
+      new KeybindingImpl({
+        commandId: 'foo',
+        combo: { key: 'K', ctrl: true }
+      })
+    ]
+
+    const userNewKeybindings = [
+      new KeybindingImpl({
+        commandId: 'foo',
+        combo: { key: 'A', ctrl: true }
+      })
+    ]
+
+    const newCoreKeybindings = [
+      new KeybindingImpl({
+        commandId: 'foo',
+        combo: { key: 'A', ctrl: true }
+      })
+    ]
+
+    for (const keybinding of newCoreKeybindings) {
+      store.addDefaultKeybinding(keybinding)
+    }
+
+    expect(store.keybindings).toHaveLength(1)
+    expect(store.getKeybinding(userNewKeybindings[0].combo)).toEqual(
+      userNewKeybindings[0]
+    )
+
+    for (const keybinding of userUnsetKeybindings) {
+      store.unsetKeybinding(keybinding)
+    }
+
+    expect(store.keybindings).toHaveLength(1)
+    expect(store.getKeybinding(userNewKeybindings[0].combo)).toEqual(
+      userNewKeybindings[0]
+    )
+
+    for (const keybinding of userNewKeybindings) {
+      store.addUserKeybinding(keybinding)
+    }
+
+    expect(store.keybindings).toHaveLength(1)
+    expect(store.getKeybinding(userNewKeybindings[0].combo)).toEqual(
+      userNewKeybindings[0]
+    )
+  })
 })