diff --git a/packages/lib/services/KeymapService.test.js b/packages/lib/services/KeymapService.test.js index b67e8a9f16b..005649b1c35 100644 --- a/packages/lib/services/KeymapService.test.js +++ b/packages/lib/services/KeymapService.test.js @@ -345,4 +345,24 @@ describe('services_KeymapService', () => { keymapService.registerCommandAccelerator('some-command', null); expect(keymapService.getAriaKeyShortcuts('some-command')).toBe(undefined); }); + + describe('validateKeymap', () => { + beforeEach(() => keymapService.initialize([], 'linux')); + + it('should throw when proposed accelerator conflicts with an existing command', () => { + // Ctrl+N is the default for newNote + expect(() => keymapService.validateKeymap({ accelerator: 'Ctrl+N', command: 'synchronize' })).toThrow(); + }); + + it('should not throw when proposed accelerator is unique', () => { + expect(() => keymapService.validateKeymap({ accelerator: 'Ctrl+Shift+Alt+F12', command: 'synchronize' })).not.toThrow(); + }); + + it('should not throw for pre-existing conflicts when checking an unrelated command', () => { + // Force a duplicate in the keymap directly, simulating a pre-existing conflict + keymapService.setAccelerator('newTodo', 'Ctrl+N'); // newNote also has Ctrl+N + // The pre-existing newNote/newTodo conflict should not block saving an unrelated shortcut + expect(() => keymapService.validateKeymap({ accelerator: 'Ctrl+Shift+Alt+F12', command: 'synchronize' })).not.toThrow(); + }); + }); }); diff --git a/packages/lib/services/KeymapService.ts b/packages/lib/services/KeymapService.ts index 7b2ae4f8762..c449b043979 100644 --- a/packages/lib/services/KeymapService.ts +++ b/packages/lib/services/KeymapService.ts @@ -350,25 +350,38 @@ export default class KeymapService extends BaseService { } public validateKeymap(proposedKeymapItem: KeymapItem = null) { - const usedAccelerators = new Set(); + if (proposedKeymapItem) { + // When checking a proposed change, only throw if the proposed accelerator + // conflicts with another command. Pre-existing conflicts between other commands + // should not block the user from saving an unrelated shortcut change. + for (const item of Object.values(this.keymap)) { + if (item.command === proposedKeymapItem.command) continue; + if (item.accelerator === proposedKeymapItem.accelerator) { + throw new Error(_( + 'Accelerator "%s" is used for "%s" and "%s" commands. This may lead to unexpected behaviour.', + proposedKeymapItem.accelerator, + item.command, + proposedKeymapItem.command, + )); + } + } + return; + } - // Validate as if the proposed change is already present in the current keymap - // Helpful for detecting any errors that'll occur, when the proposed change is performed on the keymap - if (proposedKeymapItem) usedAccelerators.add(proposedKeymapItem.accelerator); + // Validate the entire keymap for duplicates + const usedAccelerators = new Set(); for (const item of Object.values(this.keymap)) { const [itemAccelerator, itemCommand] = [item.accelerator, item.command]; - if (proposedKeymapItem && itemCommand === proposedKeymapItem.command) continue; // Ignore the original accelerator if (usedAccelerators.has(itemAccelerator)) { - const originalItem = (proposedKeymapItem && proposedKeymapItem.accelerator === itemAccelerator) - ? proposedKeymapItem - : Object.values(this.keymap).find(_item => _item.accelerator === itemAccelerator); + const originalItem = Object.values(this.keymap).find(_item => _item.accelerator === itemAccelerator); throw new Error(_( 'Accelerator "%s" is used for "%s" and "%s" commands. This may lead to unexpected behaviour.', itemAccelerator, - originalItem.command, + // originalItem must exist here — its accelerator was added to usedAccelerators in a prior iteration + originalItem!.command, itemCommand, )); } else if (itemAccelerator) {