From 45024149343d93953b00ab75c368064486b29bf9 Mon Sep 17 00:00:00 2001 From: Laurent Cozic Date: Sat, 31 Oct 2020 12:25:12 +0000 Subject: [PATCH] Desktop: Make sure all commands appear in keymap editor --- CliClient/tests/services_KeymapService.js | 27 ++++++------- ElectronClient/app.ts | 17 ++++---- .../gui/KeymapConfig/utils/getLabel.ts | 2 +- .../gui/KeymapConfig/utils/useKeymap.ts | 15 ++++++- .../lib/services/CommandService.ts | 17 +++++++- .../lib/services/KeymapService.ts | 39 ++++++++++++------- 6 files changed, 78 insertions(+), 39 deletions(-) diff --git a/CliClient/tests/services_KeymapService.js b/CliClient/tests/services_KeymapService.js index 2a8020191..a2c7c1307 100644 --- a/CliClient/tests/services_KeymapService.js +++ b/CliClient/tests/services_KeymapService.js @@ -3,6 +3,7 @@ require('app-module-path').addPath(__dirname); const { tempFilePath } = require('test-utils.js'); const KeymapService = require('lib/services/KeymapService').default; const keymapService = KeymapService.instance(); +keymapService.initialize([]); describe('services_KeymapService', () => { describe('validateAccelerator', () => { @@ -31,7 +32,7 @@ describe('services_KeymapService', () => { }; Object.entries(testCases).forEach(([platform, accelerators]) => { - keymapService.initialize(platform); + keymapService.initialize([], platform); accelerators.forEach(accelerator => { expect(() => keymapService.validateAccelerator(accelerator)).not.toThrow(); }); @@ -69,7 +70,7 @@ describe('services_KeymapService', () => { }; Object.entries(testCases).forEach(([platform, accelerators]) => { - keymapService.initialize(platform); + keymapService.initialize([], platform); accelerators.forEach(accelerator => { expect(() => keymapService.validateAccelerator(accelerator)).toThrow(); }); @@ -81,12 +82,12 @@ describe('services_KeymapService', () => { beforeEach(() => keymapService.initialize()); it('should allow registering new commands', async () => { - keymapService.initialize('linux'); + keymapService.initialize([], 'linux'); keymapService.registerCommandAccelerator('myCustomCommand', 'Ctrl+Shift+Alt+B'); expect(keymapService.getAccelerator('myCustomCommand')).toEqual('Ctrl+Shift+Alt+B'); // Check that macOS key conversion is working - keymapService.initialize('darwin'); + keymapService.initialize([], 'darwin'); keymapService.registerCommandAccelerator('myCustomCommand', 'CmdOrCtrl+Shift+Alt+B'); expect(keymapService.getAccelerator('myCustomCommand')).toEqual('Cmd+Shift+Option+B'); keymapService.setAccelerator('myCustomCommand', 'Cmd+Shift+Option+X'); @@ -95,7 +96,7 @@ describe('services_KeymapService', () => { const keymapFilePath = tempFilePath('json'); await keymapService.saveCustomKeymap(keymapFilePath); - keymapService.initialize('darwin'); + keymapService.initialize([], 'darwin'); await keymapService.loadCustomKeymap(keymapFilePath); expect(keymapService.getAccelerator('myCustomCommand')).toEqual('Cmd+Shift+Option+X'); @@ -106,17 +107,17 @@ describe('services_KeymapService', () => { beforeEach(() => keymapService.initialize()); it('should return the platform-specific default Accelerator', () => { - keymapService.initialize('darwin'); + keymapService.initialize([], 'darwin'); expect(keymapService.getAccelerator('newNote')).toEqual('Cmd+N'); expect(keymapService.getAccelerator('synchronize')).toEqual('Cmd+S'); expect(keymapService.getAccelerator('textSelectAll')).toEqual('Cmd+A'); expect(keymapService.getAccelerator('textBold')).toEqual('Cmd+B'); - keymapService.initialize('linux'); + keymapService.initialize([], 'linux'); expect(keymapService.getAccelerator('newNote')).toEqual('Ctrl+N'); expect(keymapService.getAccelerator('synchronize')).toEqual('Ctrl+S'); - keymapService.initialize('win32'); + keymapService.initialize([], 'win32'); expect(keymapService.getAccelerator('textSelectAll')).toEqual('Ctrl+A'); expect(keymapService.getAccelerator('textBold')).toEqual('Ctrl+B'); }); @@ -130,7 +131,7 @@ describe('services_KeymapService', () => { beforeEach(() => keymapService.initialize()); it('should update the Accelerator', () => { - keymapService.initialize('darwin'); + keymapService.initialize(['print'], 'darwin'); const testCases_Darwin = [ { command: 'newNote', accelerator: 'Ctrl+Option+Shift+N' }, { command: 'synchronize', accelerator: 'F11' }, @@ -147,7 +148,7 @@ describe('services_KeymapService', () => { expect(keymapService.getAccelerator(command)).toEqual(accelerator); }); - keymapService.initialize('linux'); + keymapService.initialize(['print'], 'linux'); const testCases_Linux = [ { command: 'newNote', accelerator: 'Ctrl+Alt+Shift+N' }, { command: 'synchronize', accelerator: 'F15' }, @@ -167,7 +168,7 @@ describe('services_KeymapService', () => { }); describe('getDefaultAccelerator', () => { - beforeEach(() => keymapService.initialize()); + beforeEach(() => keymapService.initialize(['print', 'linux'])); it('should return the default accelerator', () => { const testCases = [ @@ -196,7 +197,7 @@ describe('services_KeymapService', () => { beforeEach(() => keymapService.initialize()); it('should update the keymap', () => { - keymapService.initialize('darwin'); + keymapService.initialize([], 'darwin'); const customKeymapItems_Darwin = [ { command: 'newNote', accelerator: 'Option+Shift+Cmd+N' }, { command: 'synchronize', accelerator: 'Ctrl+F11' }, @@ -217,7 +218,7 @@ describe('services_KeymapService', () => { expect(keymapService.getAccelerator(command)).toEqual(accelerator); }); - keymapService.initialize('win32'); + keymapService.initialize([], 'win32'); const customKeymapItems_Win32 = [ { command: 'newNote', accelerator: 'Ctrl+Alt+Shift+N' }, { command: 'synchronize', accelerator: 'Ctrl+F11' }, diff --git a/ElectronClient/app.ts b/ElectronClient/app.ts index 0b350300f..a1077806c 100644 --- a/ElectronClient/app.ts +++ b/ElectronClient/app.ts @@ -495,14 +495,6 @@ class Application extends BaseApplication { const filename = Setting.custom_css_files.JOPLIN_APP; await CssUtils.injectCustomStyles(`${dir}/${filename}`); - const keymapService = KeymapService.instance(); - - try { - await keymapService.loadCustomKeymap(`${dir}/keymap-desktop.json`); - } catch (err) { - reg.logger().error(err.message); - } - AlarmService.setDriver(new AlarmServiceDriverNode({ appName: packageInfo.build.appId })); AlarmService.setLogger(reg.logger()); @@ -533,6 +525,15 @@ class Application extends BaseApplication { CommandService.instance().registerDeclaration(declaration); } + const keymapService = KeymapService.instance(); + keymapService.initialize(CommandService.instance().commandNames(true)); + + try { + await keymapService.loadCustomKeymap(`${dir}/keymap-desktop.json`); + } catch (error) { + reg.logger().error(error); + } + // Since the settings need to be loaded before the store is created, it will never // receive the SETTING_UPDATE_ALL even, which mean state.settings will not be // initialised. So we manually call dispatchUpdateAll() to force an update. diff --git a/ElectronClient/gui/KeymapConfig/utils/getLabel.ts b/ElectronClient/gui/KeymapConfig/utils/getLabel.ts index 268ca0644..a0a20fe57 100644 --- a/ElectronClient/gui/KeymapConfig/utils/getLabel.ts +++ b/ElectronClient/gui/KeymapConfig/utils/getLabel.ts @@ -5,7 +5,7 @@ import { _ } from 'lib/locale'; const commandService = CommandService.instance(); -const getLabel = (commandName: string) => { +const getLabel = (commandName: string):string => { if (commandService.exists(commandName)) return commandService.label(commandName, true); // Some commands are not registered in CommandService at the moment diff --git a/ElectronClient/gui/KeymapConfig/utils/useKeymap.ts b/ElectronClient/gui/KeymapConfig/utils/useKeymap.ts index 7ce015b22..62e5f97cc 100644 --- a/ElectronClient/gui/KeymapConfig/utils/useKeymap.ts +++ b/ElectronClient/gui/KeymapConfig/utils/useKeymap.ts @@ -1,11 +1,22 @@ import { useState, useEffect } from 'react'; import KeymapService, { KeymapItem } from 'lib/services/KeymapService'; +import getLabel from './getLabel'; const keymapService = KeymapService.instance(); // This custom hook provides a synchronized snapshot of the keymap residing at KeymapService // All the logic regarding altering and interacting with the keymap is isolated from the components +function allKeymapItems() { + const output = keymapService.getKeymapItems().slice(); + + output.sort((a:KeymapItem, b:KeymapItem) => { + return getLabel(a.command).toLocaleLowerCase() < getLabel(b.command).toLocaleLowerCase() ? -1 : +1; + }); + + return output; +} + const useKeymap = (): [ KeymapItem[], Error, @@ -13,7 +24,7 @@ const useKeymap = (): [ (commandName: string, accelerator: string) => void, (commandName: string) => void ] => { - const [keymapItems, setKeymapItems] = useState(() => keymapService.getKeymapItems()); + const [keymapItems, setKeymapItems] = useState(() => allKeymapItems()); const [keymapError, setKeymapError] = useState(null); const [mustSave, setMustSave] = useState(false); @@ -42,7 +53,7 @@ const useKeymap = (): [ const overrideKeymapItems = (customKeymapItems: KeymapItem[]) => { const oldKeymapItems = [...customKeymapItems]; - keymapService.initialize(); // Start with a fresh keymap + keymapService.resetKeymap(); // Start with a fresh keymap try { // First, try to update the in-memory keymap of KeymapService diff --git a/ReactNativeClient/lib/services/CommandService.ts b/ReactNativeClient/lib/services/CommandService.ts index d220d611c..16e394435 100644 --- a/ReactNativeClient/lib/services/CommandService.ts +++ b/ReactNativeClient/lib/services/CommandService.ts @@ -144,8 +144,17 @@ export default class CommandService extends BaseService { return output; } - public commandNames() { - return Object.keys(this.commands_); + public commandNames(publicOnly:boolean = false) { + if (publicOnly) { + const output = []; + for (const name in this.commands_) { + if (!this.isPublic(name)) continue; + output.push(name); + } + return output; + } else { + return Object.keys(this.commands_); + } } public commandByName(name:string, options:CommandByNameOptions = null):Command { @@ -230,6 +239,10 @@ export default class CommandService extends BaseService { return stateToWhenClauseContext(this.store_.getState()); } + public isPublic(commandName:string) { + return !!this.label(commandName); + } + // When looping on commands and checking their enabled state, the whenClauseContext // should be specified (created using currentWhenClauseContext) to avoid having // to re-create it on each call. diff --git a/ReactNativeClient/lib/services/KeymapService.ts b/ReactNativeClient/lib/services/KeymapService.ts index e674b4fe6..4b65dacdf 100644 --- a/ReactNativeClient/lib/services/KeymapService.ts +++ b/ReactNativeClient/lib/services/KeymapService.ts @@ -16,7 +16,6 @@ const defaultKeymapItems = { { accelerator: 'Cmd+N', command: 'newNote' }, { accelerator: 'Cmd+T', command: 'newTodo' }, { accelerator: 'Cmd+S', command: 'synchronize' }, - { accelerator: '', command: 'print' }, { accelerator: 'Cmd+H', command: 'hideApp' }, { accelerator: 'Cmd+Q', command: 'quit' }, { accelerator: 'Cmd+,', command: 'config' }, @@ -51,7 +50,6 @@ const defaultKeymapItems = { { accelerator: 'Ctrl+N', command: 'newNote' }, { accelerator: 'Ctrl+T', command: 'newTodo' }, { accelerator: 'Ctrl+S', command: 'synchronize' }, - { accelerator: null, command: 'print' }, { accelerator: 'Ctrl+Q', command: 'quit' }, { accelerator: 'Ctrl+Alt+I', command: 'insertTemplate' }, { accelerator: 'Ctrl+C', command: 'textCopy' }, @@ -103,29 +101,42 @@ export default class KeymapService extends BaseService { super(); this.lastSaveTime_ = Date.now(); - - // By default, initialize for the current platform - // Manual initialization allows testing for other platforms - this.initialize(); } public get lastSaveTime():number { return this.lastSaveTime_; } - public initialize(platform: string = shim.platformName()) { + // `additionalDefaultCommandNames` will be added to the default keymap + // **except** if they are already in it. Basically this is a mechanism + // to add all the commands from the command service to the default + // keymap. + public initialize(additionalDefaultCommandNames:string[] = [], platform: string = shim.platformName()) { this.platform = platform; switch (platform) { case 'darwin': - this.defaultKeymapItems = defaultKeymapItems.darwin; + this.defaultKeymapItems = defaultKeymapItems.darwin.slice(); this.modifiersRegExp = modifiersRegExp.darwin; break; default: - this.defaultKeymapItems = defaultKeymapItems.default; + this.defaultKeymapItems = defaultKeymapItems.default.slice(); this.modifiersRegExp = modifiersRegExp.default; } + for (const name of additionalDefaultCommandNames) { + if (this.defaultKeymapItems.find((item:KeymapItem) => item.command === name)) continue; + this.defaultKeymapItems.push({ + command: name, + accelerator: null, + }); + } + + this.resetKeymap(); + } + + // Reset keymap back to its default values + public resetKeymap() { this.keymap = {}; for (let i = 0; i < this.defaultKeymapItems.length; i++) { // Keep the original defaultKeymapItems array untouched @@ -140,7 +151,9 @@ export default class KeymapService extends BaseService { if (await shim.fsDriver().exists(customKeymapPath)) { this.logger().info(`KeymapService: Loading keymap from file: ${customKeymapPath}`); - const customKeymapFile = await shim.fsDriver().readFile(customKeymapPath, 'utf-8'); + const customKeymapFile = (await shim.fsDriver().readFile(customKeymapPath, 'utf-8')).trim(); + if (!customKeymapFile) return; + // Custom keymaps are supposed to contain an array of keymap items this.overrideKeymap(JSON.parse(customKeymapFile)); } @@ -183,8 +196,8 @@ export default class KeymapService extends BaseService { if (!commandName) throw new Error('Cannot register an accelerator without a command name'); - const validatedAccelerator = this.convertToPlatform(accelerator); - this.validateAccelerator(validatedAccelerator); + const validatedAccelerator = accelerator ? this.convertToPlatform(accelerator) : null; + if (validatedAccelerator) this.validateAccelerator(validatedAccelerator); this.keymap[commandName] = { command: commandName, @@ -264,7 +277,7 @@ export default class KeymapService extends BaseService { // Throws whenever there are duplicate Accelerators used in the keymap this.validateKeymap(); } catch (err) { - this.initialize(); // Discard all the changes if there are any issues + this.resetKeymap(); // Discard all the changes if there are any issues throw err; } }