From 37a73aaf7acf55ab70860cfb068d52adabbc12be Mon Sep 17 00:00:00 2001 From: "Michael Mattiello (mcm)" Date: Mon, 7 Jun 2021 11:39:15 +0000 Subject: [PATCH] [REF] web: change hotkey service API Before this commit, service hotkey exported 2 functions: registerHotkey and unregisterHotkey. Now, it exports only one function "add" which is calls registerHotkey and returns a callback which calls unregisterHotkey --- .../web/static/src/core/dropdown/dropdown.js | 14 +++---- addons/web/static/src/core/hotkey_hook.js | 8 ++-- addons/web/static/src/legacy/utils.js | 18 ++++---- .../src/webclient/commands/command_service.js | 8 ++-- .../src/webclient/hotkeys/hotkey_service.js | 17 +++++++- .../webclient/hotkeys/hotkey_service_tests.js | 42 +++++++++---------- 6 files changed, 59 insertions(+), 48 deletions(-) diff --git a/addons/web/static/src/core/dropdown/dropdown.js b/addons/web/static/src/core/dropdown/dropdown.js index 8590f4baa5d..906f0efc783 100644 --- a/addons/web/static/src/core/dropdown/dropdown.js +++ b/addons/web/static/src/core/dropdown/dropdown.js @@ -61,7 +61,7 @@ export class Dropdown extends Component { // Set up key navigation ----------------------------------------------- this.hotkeyService = useService("hotkey"); - this.hotkeyTokens = []; + this.hotkeyRemoves = []; const nextActiveIndexFns = { "FIRST": () => 0, @@ -104,13 +104,13 @@ export class Dropdown extends Component { function autoSubscribeKeynav() { if (this.state.open) { // Subscribe keynav - if (this.hotkeyTokens.length) { + if (this.hotkeyRemoves.length) { // Keynav already subscribed return; } for (const [hotkey, callback] of Object.entries(hotkeyCallbacks)) { - this.hotkeyTokens.push( - this.hotkeyService.registerHotkey(hotkey, callback, { + this.hotkeyRemoves.push( + this.hotkeyService.add(hotkey, callback, { altIsOptional: true, allowRepeat: true, }) @@ -118,10 +118,10 @@ export class Dropdown extends Component { } } else { // Unsubscribe keynav - for (const token of this.hotkeyTokens) { - this.hotkeyService.unregisterHotkey(token); + for (const removeHotkey of this.hotkeyRemoves) { + removeHotkey(); } - this.hotkeyTokens = []; + this.hotkeyRemoves = []; } } diff --git a/addons/web/static/src/core/hotkey_hook.js b/addons/web/static/src/core/hotkey_hook.js index cc7fbac3cb3..f9535dbc9dc 100644 --- a/addons/web/static/src/core/hotkey_hook.js +++ b/addons/web/static/src/core/hotkey_hook.js @@ -19,8 +19,8 @@ import { useEffect } from "./effect_hook"; */ export function useHotkey(hotkey, callback, options = {}) { const hotkeyService = useService("hotkey"); - useEffect(() => { - const token = hotkeyService.registerHotkey(hotkey, callback, options); - return () => hotkeyService.unregisterHotkey(token); - }, () => []); + useEffect( + () => hotkeyService.add(hotkey, callback, options), + () => [] + ); } diff --git a/addons/web/static/src/legacy/utils.js b/addons/web/static/src/legacy/utils.js index 9164e584a33..d752124e050 100644 --- a/addons/web/static/src/legacy/utils.js +++ b/addons/web/static/src/legacy/utils.js @@ -78,24 +78,22 @@ export function makeLegacyDialogMappingService(legacyEnv) { return dialog.modalRef ? () => dialog._close() : () => dialog.$modal.modal("hide"); } - const tokensMap = new Map(); + const dialogHotkeyRemoveMap = new Map(); function onOpenDialog(dialog) { ui.activateElement(getModalEl(dialog)); - const token = hotkey.registerHotkey("escape", getCloseCallback(dialog), { + const remove = hotkey.add("escape", getCloseCallback(dialog), { altIsOptional: true, }); - tokensMap.set(token, dialog); + dialogHotkeyRemoveMap.set(dialog, remove); } function onCloseDialog(dialog) { - for (const [token, d] of tokensMap) { - if (d === dialog) { - ui.deactivateElement(getModalEl(dialog)); - hotkey.unregisterHotkey(token); - tokensMap.delete(token); - break; - } + ui.deactivateElement(getModalEl(dialog)); + if (dialogHotkeyRemoveMap.has(dialog)) { + const removeHotkey = dialogHotkeyRemoveMap.get(dialog); + removeHotkey(); + dialogHotkeyRemoveMap.delete(dialog); } } diff --git a/addons/web/static/src/webclient/commands/command_service.js b/addons/web/static/src/webclient/commands/command_service.js index a09783edfab..95d391b450b 100644 --- a/addons/web/static/src/webclient/commands/command_service.js +++ b/addons/web/static/src/webclient/commands/command_service.js @@ -21,7 +21,7 @@ export const commandService = { let nextToken = 0; let isPaletteOpened = false; - hotkeyService.registerHotkey("control+k", openPalette, { + hotkeyService.add("control+k", openPalette, { altIsOptional: true, global: true, }); @@ -82,7 +82,7 @@ export const commandService = { const registration = Object.assign({}, command, { activeElement: null }); if (command.hotkey) { - registration.hotkeyToken = hotkeyService.registerHotkey( + registration.removeHotkey = hotkeyService.add( command.hotkey, command.action, command.hotkeyOptions @@ -111,8 +111,8 @@ export const commandService = { */ function unregisterCommand(token) { const cmd = registeredCommands.get(token); - if (cmd && cmd.hotkeyToken >= 0) { - env.services.hotkey.unregisterHotkey(cmd.hotkeyToken); + if (cmd && cmd.removeHotkey) { + cmd.removeHotkey(); } registeredCommands.delete(token); } diff --git a/addons/web/static/src/webclient/hotkeys/hotkey_service.js b/addons/web/static/src/webclient/hotkeys/hotkey_service.js index 59ef533a996..3e36338ae83 100644 --- a/addons/web/static/src/webclient/hotkeys/hotkey_service.js +++ b/addons/web/static/src/webclient/hotkeys/hotkey_service.js @@ -308,8 +308,21 @@ export const hotkeyService = { } return { - registerHotkey, - unregisterHotkey, + /** + * @param {string} hotkey + * @param {() => void} callback + * @param {Object} options + * @param {boolean} [options.altIsOptional=false] + * @param {boolean} [options.allowRepeat=false] + * @param {boolean} [options.global=false] + * @returns {() => void} + */ + add(hotkey, callback, options = {}) { + const token = registerHotkey(hotkey, callback, options); + return () => { + unregisterHotkey(token); + }; + }, }; }, }; diff --git a/addons/web/static/tests/webclient/hotkeys/hotkey_service_tests.js b/addons/web/static/tests/webclient/hotkeys/hotkey_service_tests.js index f5c08e50908..9fdb94b51fc 100644 --- a/addons/web/static/tests/webclient/hotkeys/hotkey_service_tests.js +++ b/addons/web/static/tests/webclient/hotkeys/hotkey_service_tests.js @@ -33,13 +33,13 @@ QUnit.test("register / unregister", async (assert) => { triggerHotkey(key); await nextTick(); - let token = hotkey.registerHotkey(key, () => assert.step(key)); + let removeHotkey = hotkey.add(key, () => assert.step(key)); await nextTick(); triggerHotkey(key); await nextTick(); - hotkey.unregisterHotkey(token); + removeHotkey(); triggerHotkey(key); await nextTick(); @@ -117,7 +117,7 @@ QUnit.test("non-MacOS usability", async (assert) => { const key = "q"; // On non-MacOS, ALT is NOT replaced by CONTROL key - let token = hotkey.registerHotkey(key, () => assert.step(key), { altIsOptional: false }); + let removeHotkey = hotkey.add(key, () => assert.step(key), { altIsOptional: false }); await nextTick(); let keydown = new KeyboardEvent("keydown", { key, altKey: true }); @@ -130,10 +130,10 @@ QUnit.test("non-MacOS usability", async (assert) => { await nextTick(); assert.verifySteps([]); - hotkey.unregisterHotkey(token); + removeHotkey(); // On non-MacOS, CONTROL is NOT replaced by COMMAND key (= metaKey) - token = hotkey.registerHotkey(`control+${key}`, () => assert.step(`control+${key}`), { + removeHotkey = hotkey.add(`control+${key}`, () => assert.step(`control+${key}`), { altIsOptional: true, }); await nextTick(); @@ -148,7 +148,7 @@ QUnit.test("non-MacOS usability", async (assert) => { await nextTick(); assert.verifySteps([]); - hotkey.unregisterHotkey(token); + removeHotkey(); }); QUnit.test("MacOS usability", async (assert) => { @@ -164,7 +164,7 @@ QUnit.test("MacOS usability", async (assert) => { const key = "q"; // On MacOS, ALT is replaced by CONTROL key - let token = hotkey.registerHotkey(key, () => assert.step(key), { altIsOptional: false }); + let removeHotkey = hotkey.add(key, () => assert.step(key), { altIsOptional: false }); await nextTick(); let keydown = new KeyboardEvent("keydown", { key, altKey: true }); @@ -177,10 +177,10 @@ QUnit.test("MacOS usability", async (assert) => { await nextTick(); assert.verifySteps([key]); - hotkey.unregisterHotkey(token); + removeHotkey(); // On MacOS, CONTROL is replaced by COMMAND key (= metaKey) - token = hotkey.registerHotkey(`control+${key}`, () => assert.step(`control+${key}`), { + removeHotkey = hotkey.add(`control+${key}`, () => assert.step(`control+${key}`), { altIsOptional: true, }); await nextTick(); @@ -195,7 +195,7 @@ QUnit.test("MacOS usability", async (assert) => { await nextTick(); assert.verifySteps([`control+${key}`]); - hotkey.unregisterHotkey(token); + removeHotkey(); }); QUnit.test("alt is optional parameter", async (assert) => { @@ -263,13 +263,13 @@ QUnit.test("registration allows repeat if specified", async (assert) => { const disallowRepeatKey = "b"; const defaultBehaviourKey = "c"; - env.services.hotkey.registerHotkey(allowRepeatKey, () => assert.step(allowRepeatKey), { + env.services.hotkey.add(allowRepeatKey, () => assert.step(allowRepeatKey), { allowRepeat: true, }); - env.services.hotkey.registerHotkey(disallowRepeatKey, () => assert.step(disallowRepeatKey), { + env.services.hotkey.add(disallowRepeatKey, () => assert.step(disallowRepeatKey), { allowRepeat: false, }); - env.services.hotkey.registerHotkey(defaultBehaviourKey, () => assert.step(defaultBehaviourKey)); + env.services.hotkey.add(defaultBehaviourKey, () => assert.step(defaultBehaviourKey)); await nextTick(); // Dispatch the three keys without repeat: @@ -316,30 +316,30 @@ QUnit.test("hotkeys evil 👹", async (assert) => { const hotkey = env.services.hotkey; assert.throws(function () { - hotkey.registerHotkey(); + hotkey.add(); }, /must specify an hotkey/); assert.throws(function () { - hotkey.registerHotkey(null); + hotkey.add(null); }, /must specify an hotkey/); function callback() {} assert.throws(function () { - hotkey.registerHotkey(null, callback); + hotkey.add(null, callback); }, /must specify an hotkey/); assert.throws(function () { - hotkey.registerHotkey(""); + hotkey.add(""); }, /must specify an hotkey/); assert.throws(function () { - hotkey.registerHotkey("crap", callback); + hotkey.add("crap", callback); }, /not whitelisted/); assert.throws(function () { - hotkey.registerHotkey("ctrl+o", callback); + hotkey.add("ctrl+o", callback); }, /not whitelisted/); assert.throws(function () { - hotkey.registerHotkey("Control+o"); + hotkey.add("Control+o"); }, /specify a callback/); assert.throws(function () { - hotkey.registerHotkey("Control+o+d", callback); + hotkey.add("Control+o+d", callback); }, /more than one single key part/); });