From 4b12d28f3ee830b91f0eb087209820453eeb9af2 Mon Sep 17 00:00:00 2001 From: Christopher Allen Date: Fri, 27 Sep 2024 14:59:28 +0100 Subject: [PATCH 1/3] refactor(shortcuts): Improve shortcut registry documentation & style Make some minor refactorings of shortcut_registry.ts to improve readability and style, and add documentation for the KeyboardShortcut interface type. --- core/shortcut_registry.ts | 121 ++++++++++++++++++++++++++++---------- 1 file changed, 89 insertions(+), 32 deletions(-) diff --git a/core/shortcut_registry.ts b/core/shortcut_registry.ts index 161a2ed1495..d4c95214d2e 100644 --- a/core/shortcut_registry.ts +++ b/core/shortcut_registry.ts @@ -45,31 +45,27 @@ export class ShortcutRegistry { * Registers a keyboard shortcut. * * @param shortcut The shortcut for this key code. - * @param opt_allowOverrides True to prevent a warning when overriding an + * @param allowOverrides True to prevent a warning when overriding an * already registered item. * @throws {Error} if a shortcut with the same name already exists. */ - register(shortcut: KeyboardShortcut, opt_allowOverrides?: boolean) { + register(shortcut: KeyboardShortcut, allowOverrides?: boolean) { const registeredShortcut = this.shortcuts.get(shortcut.name); - if (registeredShortcut && !opt_allowOverrides) { + if (registeredShortcut && !allowOverrides) { throw new Error(`Shortcut named "${shortcut.name}" already exists.`); } this.shortcuts.set(shortcut.name, shortcut); const keyCodes = shortcut.keyCodes; - if (keyCodes && keyCodes.length > 0) { - for (let i = 0; i < keyCodes.length; i++) { - this.addKeyMapping( - keyCodes[i], - shortcut.name, - !!shortcut.allowCollision, - ); + if (keyCodes?.length) { + for (const keyCode of keyCodes) { + this.addKeyMapping(keyCode, shortcut.name, !!shortcut.allowCollision); } } } /** - * Unregisters a keyboard shortcut registered with the given key code. This + * Unregisters a keyboard shortcut registered with the given name. This * will also remove any key mappings that reference this shortcut. * * @param shortcutName The name of the shortcut to unregister. @@ -92,27 +88,31 @@ export class ShortcutRegistry { /** * Adds a mapping between a keycode and a keyboard shortcut. * + * If allowCollisions is used to mapped a single keycode to multiple + * shortcuts, they will be processed in last-registered, + * first-activated order by onKeyDown. + * * @param keyCode The key code for the keyboard shortcut. If registering a key * code with a modifier (ex: ctrl+c) use * ShortcutRegistry.registry.createSerializedKey; * @param shortcutName The name of the shortcut to execute when the given * keycode is pressed. - * @param opt_allowCollision True to prevent an error when adding a shortcut + * @param allowCollision True to prevent an error when adding a shortcut * to a key that is already mapped to a shortcut. * @throws {Error} if the given key code is already mapped to a shortcut. */ addKeyMapping( keyCode: string | number | KeyCodes, shortcutName: string, - opt_allowCollision?: boolean, + allowCollision?: boolean, ) { keyCode = `${keyCode}`; const shortcutNames = this.keyMap.get(keyCode); - if (shortcutNames && !opt_allowCollision) { + if (shortcutNames && !allowCollision) { throw new Error( `Shortcut named "${shortcutName}" collides with shortcuts "${shortcutNames}"`, ); - } else if (shortcutNames && opt_allowCollision) { + } else if (shortcutNames && allowCollision) { shortcutNames.unshift(shortcutName); } else { this.keyMap.set(keyCode, [shortcutName]); @@ -127,19 +127,19 @@ export class ShortcutRegistry { * ShortcutRegistry.registry.createSerializedKey; * @param shortcutName The name of the shortcut to execute when the given * keycode is pressed. - * @param opt_quiet True to not console warn when there is no shortcut to + * @param quiet True to not console warn when there is no shortcut to * remove. * @returns True if a key mapping was removed, false otherwise. */ removeKeyMapping( keyCode: string, shortcutName: string, - opt_quiet?: boolean, + quiet?: boolean, ): boolean { const shortcutNames = this.keyMap.get(keyCode); if (!shortcutNames) { - if (!opt_quiet) { + if (!quiet) { console.warn( `No keyboard shortcut named "${shortcutName}" registered with key code "${keyCode}"`, ); @@ -155,7 +155,7 @@ export class ShortcutRegistry { } return true; } - if (!opt_quiet) { + if (!quiet) { console.warn( `No keyboard shortcut named "${shortcutName}" registered with key code "${keyCode}"`, ); @@ -172,7 +172,7 @@ export class ShortcutRegistry { */ removeAllKeyMappings(shortcutName: string) { for (const keyCode of this.keyMap.keys()) { - this.removeKeyMapping(keyCode, shortcutName, true); + this.removeKeyMapping(keyCode, shortcutName, /* quiet= */ true); } } @@ -219,6 +219,20 @@ export class ShortcutRegistry { /** * Handles key down events. * + * - Any `KeyboardShortcut`(s) mapped to the keyscode that cause + * event `e` to be fired will be processed, in order from least- + * to most-recently registered. + * - If the shortcut's `preconditionFn` exists it will be called, + * and if it returns false the shortcut will otherwise be ignored + * and processing will continue with the next shortcut, if any. + * - The shortcut's `callback` function will the be called. If it + * returns true, processing will terminate and onKeyDown will + * return true. If it returns false, processing will continue + * with with the next shortcut, if any. + * - If all registered shortcuts for the given keycode have been + * processed without any having returnd true, onKeyDown will + * return false. + * * @param workspace The main workspace where the event was captured. * @param e The key down event. * @returns True if the event was handled, false otherwise. @@ -226,17 +240,15 @@ export class ShortcutRegistry { onKeyDown(workspace: WorkspaceSvg, e: KeyboardEvent): boolean { const key = this.serializeKeyEvent_(e); const shortcutNames = this.getShortcutNamesByKeyCode(key); - if (!shortcutNames) { - return false; - } - for (let i = 0, shortcutName; (shortcutName = shortcutNames[i]); i++) { + if (!shortcutNames) return false; + for (const shortcutName of shortcutNames) { const shortcut = this.shortcuts.get(shortcutName); - if (!shortcut?.preconditionFn || shortcut?.preconditionFn(workspace)) { - // If the key has been handled, stop processing shortcuts. - if (shortcut?.callback && shortcut?.callback(workspace, e, shortcut)) { - return true; - } + if (!shortcut || + (shortcut.preconditionFn && !shortcut.preconditionFn(workspace))) { + continue; } + // If the key has been handled, stop processing shortcuts. + if (shortcut.callback?.(workspace, e, shortcut)) return true; } return false; } @@ -301,7 +313,7 @@ export class ShortcutRegistry { * @throws {Error} if the modifier is not in the valid modifiers list. */ private checkModifiers_(modifiers: KeyCodes[]) { - for (let i = 0, modifier; (modifier = modifiers[i]); i++) { + for (const modifier of modifiers) { if (!(modifier in ShortcutRegistry.modifierKeys)) { throw new Error(modifier + ' is not a valid modifier key.'); } @@ -344,12 +356,57 @@ export class ShortcutRegistry { } export namespace ShortcutRegistry { + /** Interface defining a keyboard shortcut. */ export interface KeyboardShortcut { - callback?: (p1: WorkspaceSvg, p2: Event, p3: KeyboardShortcut) => boolean; + /** + * The function to be called when the shorctut is invoked. + * + * @param workspace The WorkspaceSvg when the shortcut was invoked. + * @param e The event that caused the shortcut to be activated. + * @param shortcut the KeyboardShortcut that was activated (i.e., + * the one this callback is attached to). + * @returns Returning true ends processing of the invoked keycode. + * Returning false causes processing to coninue with the + * next-most-recently registered shortcut for the invoked + * keycode. + */ + callback?: ( + workspace: WorkspaceSvg, + e: Event, + shortcut: KeyboardShortcut, + ) => boolean; + + /** The name of the shortcut. Should be unique. */ name: string; - preconditionFn?: (p1: WorkspaceSvg) => boolean; + + /** + * A function to be called when the shortcut is invoked, before + * calling callback, to decide if this shortcut is applicable in + * the current situation. + * + * @param workspace The WorkspaceSvg where the shortcut was invoked. + * @returns True iff callback function should be called. + */ + preconditionFn?: (workspace: WorkspaceSvg) => boolean; + + /** Optional arbitray extra data attached to the shortcut. */ metadata?: object; + + /** + * Optional list of key codes to be bound (via + * ShortcutRegistry.prototype.addKeyMapping) to this shortcut. + */ keyCodes?: (number | string)[]; + + /** + * Value of allowClollision to pass to addKeyMapping when binding + * this shortcut's .keyCodes (if any). + * + * N.B.: this is only used for binding keycodes at the time this + * shortcut is initially registered, not for any subsequent + * addKeyMapping calls that happen to reference this shortcut's + * name. + */ allowCollision?: boolean; } From e0b7daadd9b1de3349207f755b46828f52ba0bf9 Mon Sep 17 00:00:00 2001 From: Christopher Allen Date: Tue, 1 Oct 2024 11:35:35 +0100 Subject: [PATCH 2/3] docs(shortcuts): Fix JSDoc typos etc. for PR #8598 --- core/shortcut_registry.ts | 48 ++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/core/shortcut_registry.ts b/core/shortcut_registry.ts index d4c95214d2e..b1352c2a930 100644 --- a/core/shortcut_registry.ts +++ b/core/shortcut_registry.ts @@ -88,9 +88,12 @@ export class ShortcutRegistry { /** * Adds a mapping between a keycode and a keyboard shortcut. * - * If allowCollisions is used to mapped a single keycode to multiple - * shortcuts, they will be processed in last-registered, - * first-activated order by onKeyDown. + * Normally only one shortcut can be mapped to any given keycode, + * but setting allowCollisions to true allows a keyboard to be + * mapped to multiple shortcuts. In that case, when onKeyDown is + * called with the given keystroke, it will process the mapped + * shortcuts in reverse order, from the most- to least-recently + * mapped). * * @param keyCode The key code for the keyboard shortcut. If registering a key * code with a modifier (ex: ctrl+c) use @@ -219,18 +222,19 @@ export class ShortcutRegistry { /** * Handles key down events. * - * - Any `KeyboardShortcut`(s) mapped to the keyscode that cause + * - Any `KeyboardShortcut`(s) mapped to the keycodes that cause * event `e` to be fired will be processed, in order from least- * to most-recently registered. - * - If the shortcut's `preconditionFn` exists it will be called, - * and if it returns false the shortcut will otherwise be ignored - * and processing will continue with the next shortcut, if any. - * - The shortcut's `callback` function will the be called. If it - * returns true, processing will terminate and onKeyDown will + * - If the shortcut's `preconditionFn` exists it will be called. + * If `preconditionFn` returns false the shortcut's `callback` + * function will be skipped. Processing will continue with the + * next shortcut, if any. + * - The shortcut's `callback` function will then be called. If it + * returns true, processing will terminate and `onKeyDown` will * return true. If it returns false, processing will continue * with with the next shortcut, if any. * - If all registered shortcuts for the given keycode have been - * processed without any having returnd true, onKeyDown will + * processed without any having returned true, `onKeyDown` will * return false. * * @param workspace The main workspace where the event was captured. @@ -325,7 +329,7 @@ export class ShortcutRegistry { * * @param keyCode Number code representing the key. * @param modifiers List of modifier key codes to be used with the key. All - * valid modifiers can be found in the ShortcutRegistry.modifierKeys. + * valid modifiers can be found in the `ShortcutRegistry.modifierKeys`. * @returns The serialized key code for the given modifiers and key. */ createSerializedKey(keyCode: number, modifiers: KeyCodes[] | null): string { @@ -361,12 +365,13 @@ export namespace ShortcutRegistry { /** * The function to be called when the shorctut is invoked. * - * @param workspace The WorkspaceSvg when the shortcut was invoked. + * @param workspace The `WorkspaceSvg` when the shortcut was + * invoked. * @param e The event that caused the shortcut to be activated. - * @param shortcut the KeyboardShortcut that was activated (i.e., - * the one this callback is attached to). + * @param shortcut The `KeyboardShortcut` that was activated + * (i.e., the one this callback is attached to). * @returns Returning true ends processing of the invoked keycode. - * Returning false causes processing to coninue with the + * Returning false causes processing to continue with the * next-most-recently registered shortcut for the invoked * keycode. */ @@ -381,11 +386,12 @@ export namespace ShortcutRegistry { /** * A function to be called when the shortcut is invoked, before - * calling callback, to decide if this shortcut is applicable in + * calling `callback`, to decide if this shortcut is applicable in * the current situation. * - * @param workspace The WorkspaceSvg where the shortcut was invoked. - * @returns True iff callback function should be called. + * @param workspace The `WorkspaceSvg` where the shortcut was + * invoked. + * @returns True iff `callback` function should be called. */ preconditionFn?: (workspace: WorkspaceSvg) => boolean; @@ -399,12 +405,12 @@ export namespace ShortcutRegistry { keyCodes?: (number | string)[]; /** - * Value of allowClollision to pass to addKeyMapping when binding - * this shortcut's .keyCodes (if any). + * Value of `allowCollision` to pass to `addKeyMapping` when + * binding this shortcut's `.keyCodes` (if any). * * N.B.: this is only used for binding keycodes at the time this * shortcut is initially registered, not for any subsequent - * addKeyMapping calls that happen to reference this shortcut's + * `addKeyMapping` calls that happen to reference this shortcut's * name. */ allowCollision?: boolean; From bdd2bf45a9457c8d933153e265bed7ee88d62478 Mon Sep 17 00:00:00 2001 From: Christopher Allen Date: Wed, 2 Oct 2024 12:25:08 +0100 Subject: [PATCH 3/3] chore: Format --- core/shortcut_registry.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/core/shortcut_registry.ts b/core/shortcut_registry.ts index b1352c2a930..32615c86686 100644 --- a/core/shortcut_registry.ts +++ b/core/shortcut_registry.ts @@ -247,8 +247,10 @@ export class ShortcutRegistry { if (!shortcutNames) return false; for (const shortcutName of shortcutNames) { const shortcut = this.shortcuts.get(shortcutName); - if (!shortcut || - (shortcut.preconditionFn && !shortcut.preconditionFn(workspace))) { + if ( + !shortcut || + (shortcut.preconditionFn && !shortcut.preconditionFn(workspace)) + ) { continue; } // If the key has been handled, stop processing shortcuts. @@ -404,7 +406,7 @@ export namespace ShortcutRegistry { */ keyCodes?: (number | string)[]; - /** + /** * Value of `allowCollision` to pass to `addKeyMapping` when * binding this shortcut's `.keyCodes` (if any). *