Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Key Command Handling #644

Open
wants to merge 2 commits into
base: port-to-js
Choose a base branch
from

Conversation

ibretsam
Copy link

This pull request updates the key command handling to replace the deprecated event.keyCode. The changes include:

  • Refactoring the Key class to use KeyMap objects for better key event handling.
  • Introducing a new getKeyFromEvent function to map keyboard events to KeyMap objects.
  • Updating the KeyCommandController and related classes to use the new key mapping.
  • Modifying the KeyCommands to use KeyMap instead of key codes directly.

These improvements enhance the maintainability and extendability of the key command handling mechanism while replacing the deprecated event.keyCode.

}

export interface KeyMap {
key: string[]; // Using array to support multiple key variations (e.g. 'a' and 'A')
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using keyCode, both a and A share the same code (65), but since we're switching to using the key property, a and A are treat as distinct values. Therefore, we should use an array to store all variations of the same key.


export interface KeyMap {
key: string[]; // Using array to support multiple key variations (e.g. 'a' and 'A')
keyCode: number;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of propagate the change to outside of this file, we should keep KeyMap inside this file and keep the previous code that works with keyCode (number) unchanged.

/**
 * An enum for key codes.
 */
export enum KeyCodes {
    ESC = 27,
    SHIFT = 16,
    ENTER = 13,
    BACKSPACE = 8,
    DELETE = 46,
    ARROW_LEFT = 37,
    ARROW_UP = 38,
    ARROW_RIGHT = 39,
    ARROW_DOWN = 40,
    A = 65,
    C = 67,
    D = 68,
    L = 76,
    R = 82,
    T = 84,
    V = 86,
    X = 88,
    Z = 90,
}

export type KeyCode =
    KeyCodes.ESC
    | KeyCodes.SHIFT
    | KeyCodes.ENTER
    | KeyCodes.BACKSPACE
    | KeyCodes.DELETE
    | KeyCodes.ARROW_LEFT
    | KeyCodes.ARROW_UP
    | KeyCodes.ARROW_RIGHT
    | KeyCodes.ARROW_DOWN
    | KeyCodes.A
    | KeyCodes.C
    | KeyCodes.D
    | KeyCodes.L
    | KeyCodes.R
    | KeyCodes.T
    | KeyCodes.V
    | KeyCodes.X
    | KeyCodes.Z;

const KeyToKeyCode: { [key: string]: KeyCode } = {
    Escape: KeyCodes.ESC,
    Shift: KeyCodes.SHIFT,
    Enter: KeyCodes.ENTER,
    Backspace: KeyCodes.BACKSPACE,
    Delete: KeyCodes.DELETE,
    ArrowLeft: KeyCodes.ARROW_LEFT,
    ArrowUp: KeyCodes.ARROW_UP,
    ArrowRight: KeyCodes.ARROW_RIGHT,
    ArrowDown: KeyCodes.ARROW_DOWN,
    a: KeyCodes.A,
    A: KeyCodes.A,
    c: KeyCodes.C,
    C: KeyCodes.C,
    d: KeyCodes.D,
    D: KeyCodes.D,
    l: KeyCodes.L,
    L: KeyCodes.L,
    r: KeyCodes.R,
    R: KeyCodes.R,
    t: KeyCodes.T,
    T: KeyCodes.T,
    v: KeyCodes.V,
    V: KeyCodes.V,
    x: KeyCodes.X,
    X: KeyCodes.X,
    z: KeyCodes.Z,
    Z: KeyCodes.Z,
};

export function getKeyCodeFromEvent(e: KeyboardEvent): KeyCode | undefined {
    return KeyToKeyCode[e.key];
}

Besides, KeyMap should not be used as a key for a map in KeyCodeToKeyCommandMap (the previous type of the key of KeyCodeToKeyCommandMap (Key) is wrong).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry to change export class Key to something else that is more appropriate like enum. I didn't know much about TypeScript when I started converting Kotlin into it (and Copilot at that time was not really good too)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, that's a nice suggestion, let me try to apply this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants