refactor(shortcuts): Improve shortcut registry documentation & style

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,24 +219,36 @@ 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.
    */
   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;
   }