Connect: Enable keyboard shortcuts configuration #22194

gzdunek · 2023-02-23T15:50:08Z

2/2 (1/2 is here)
Part of #21914

This PR actually enables shortcuts configuration (with proper validation).

Note to Rafał:
I played a bit with with z.union and z.literal, but I encountered some problems. First, the value in z.literal can't be generated, so I would have to have much longer list of possible options. Second, I think the approach with superRefine is simpler and at the same time allows for more sophisticated validation.
Also in DM I wrote that validating shortcuts like Command-Control or Tab-Space is more complicated and maybe could be skipped.
Fortunately, I realized that if we sort the tokens first, it is really easy to validate the "key code" and "modifiers" separately.
(And that's another advantage, you can provide a shortcut as "1+Shift+Ctrl" and it will work!)

I think we have validation for almost all possible cases, please take a look at getKeyboardShortcutSchema.test.ts.
In case of an error the user will see a notification:

I also added a check for duplicated shortcuts - a user can assign an accelerator that is used for some other action. In that case they will see a warning:

I had some troubles with the naming. Keys like "Shift" or "Control" are modifiers. But what's the name of the key that goes after them (like "A" or "Tab")? A main key? I use "key code", hopefully it is clear enough.

Best to review commit by commit.

avatus

Will come back to this later today

web/packages/shared/components/Notification/Notification.tsx

ravicious

Looked at the first two commits today, I'll come back to this on Monday.

web/packages/teleterm/src/ui/services/keyboardShortcuts/getKeyCode.ts

web/packages/teleterm/src/ui/services/keyboardShortcuts/keyboardShortcutsService.ts

web/packages/teleterm/src/services/config/getKeyboardShortcutSchema.ts

web/packages/teleterm/src/services/config/getKeyboardShortcutSchema.test.ts

web/packages/teleterm/src/services/config/getKeyboardShortcutSchema.ts

avatus

Looks great to me. Approving with the caveat of taking care of @ravicious comments first of course. 🙌

web/packages/teleterm/src/ui/initUi.tsx

web/packages/teleterm/src/services/config/getKeyboardShortcutSchema.ts

web/packages/teleterm/src/ui/services/keyboardShortcuts/keyboardShortcutsService.ts

web/packages/teleterm/src/services/config/getKeyboardShortcutSchema.ts

web/packages/shared/components/Notification/types.ts

web/packages/teleterm/src/ui/initUi.tsx

web/packages/teleterm/src/ui/services/keyboardShortcuts/keyboardShortcutsService.ts

web/packages/teleterm/src/services/config/configStore.ts

web/packages/shared/components/Notification/Notification.story.tsx

web/packages/shared/components/Notification/Notification.tsx

web/packages/teleterm/src/services/config/getKeyboardShortcutSchema.ts

web/packages/teleterm/src/ui/initUi.ts

web/packages/shared/components/Notification/Notification.tsx

gzdunek · 2023-03-01T19:53:38Z

I renamed getKeyCode to getKeyName to avoid associations with event.code. This function sometimes relays event.code, sometimes on event.key.

web/packages/teleterm/src/ui/services/keyboardShortcuts/getKeyName.ts

web/packages/shared/components/Notification/Notification.tsx

ravicious

Some good comments, I submitted some small grammatical fixes.

With yesterday's Slack discussion in mind, I suppose this is all we had to take care of in terms of keyboard shortcut support, isn't it?

gzdunek · 2023-03-02T10:56:48Z

With yesterday's Slack discussion in mind, I suppose this is all we had to take care of in terms of keyboard shortcut support, isn't it?

Yes, that's all!

ravicious · 2023-03-02T11:11:05Z

I want to do a final test of this later today.

ravicious

I'm really sorry for surfacing these this late into the PR's lifecycle. For the most part though these are minor issues with only two bigger issues: the grave accent and listing valid modifiers.

To my defense, I was mostly focusing more on the technical aspects and immediate UI issues so far while only now I sat to this with the intent of "Alright, let's assume I know nothing about shortcuts in Connect and I want to set some custom ones".

web/packages/shared/components/Notification/Notification.tsx

web/packages/teleterm/src/services/config/getKeyboardShortcutSchema.ts

web/packages/teleterm/src/services/config/configService.ts

web/packages/teleterm/src/services/config/getKeyboardShortcutSchema.ts

web/packages/teleterm/src/ui/initUi.ts

web/packages/teleterm/src/services/config/configService.ts

web/packages/teleterm/src/services/config/getKeyboardShortcutSchema.ts

ravicious

🚀

Without this fix, shortcuts like "Shift+1" can't work, because the reported character is "!".

…ended by platform guidelines

gzdunek · 2023-03-03T13:24:26Z

Force-pushed because checks stuck on Waiting for status to be reported.

public-teleport-github-review-bot · 2023-03-03T14:22:52Z

@gzdunek See the table below for backport results.

Branch	Result
branch/v12	Failed

* Use event.code for non A-z keys Without this fix, shortcuts like "Shift+1" can't work, because the reported character is "!". * Add Zod schema to validate keyboard shortcuts * Render better formatted error notification, add a link to docs * Notify about duplicated accelerators * Improve the error message for re-parsing failure (it can happen when a default value does not pass validation) * A bunch of renames * Allow spaces in accelerators * Rename `isContentAnObject` * Allow `Notification` to render links and lists * Split by optional whitespace and "+" * Allow lowercase letters * Require a modifier for non-function keys * Move VALID_SHORTCUT_MESSAGE to `initUi` * Always return from `getDuplicateAccelerators` * Use 'Cmd' and 'Ctrl' for mac * Add comments * Fix notification not rendering content * Add more comments, rename `getKeyCode` to `getKeyName` * Fix incorrect size of <Link> text * Remove "expected" prefix * Revert `typeof content === 'object'` in `Notification` * Remove a comment about disabled keys in `ConfigService`, add a note about `keymap.` prefix * Improve `getKeyName` comment * Extract an inline object to a variable in `getKeyName` * Fix notification list padding * Change text for doc link & description for config error * Improve comment for `getKeyName` * Remove special formatting for list === 1 in `Notification` * Print valid modifiers * Call `getKeyboardShortcutSchema()` once * Collect issues from all validations, run `invalidModifiers` through a set * Change error message for `missingModifierIssue` * Convert duplicates warning to error * Define ALLOWED_KEY_CODES in a more concise way * Support both `IntlBackslash` and `Backquote` * Restore modifiers for mac to full spelling, sort them in order recommended by platform guidelines * Fix old comment about the shortcuts order --------- Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> (cherry picked from commit 4830ea1)

@ravicious

* Connect: Prepare keyboard shortcuts configuration (#22193) * Make naming around keyboard shortcuts more consistent Previously, we used words like: "key", "type", "shortcut" which were really confusing (and I wrote this code), because it was hard to tell what exactly they describe. From now, we will use only "accelerator" (borrowed from Electron) and "action" words. For example, in a shortcut "Command+1": "tab-1": - "Command+1" is "accelerator" - "tab-1" is "action" * Use "+" instead of "-" as a separator With "-" we are not able to use this symbol as a valid key code. * Rename some config keys to follow the same naming pattern * Rename keyboard shortcuts to match config keys * Extract `mapAcceleratorsToActions` to a function * Rename other "keyboard shortcuts" to "accelerators" and "actions" (cherry picked from commit 717c4fa) * Connect: Enable keyboard shortcuts configuration (#22194) * Use event.code for non A-z keys Without this fix, shortcuts like "Shift+1" can't work, because the reported character is "!". * Add Zod schema to validate keyboard shortcuts * Render better formatted error notification, add a link to docs * Notify about duplicated accelerators * Improve the error message for re-parsing failure (it can happen when a default value does not pass validation) * A bunch of renames * Allow spaces in accelerators * Rename `isContentAnObject` * Allow `Notification` to render links and lists * Split by optional whitespace and "+" * Allow lowercase letters * Require a modifier for non-function keys * Move VALID_SHORTCUT_MESSAGE to `initUi` * Always return from `getDuplicateAccelerators` * Use 'Cmd' and 'Ctrl' for mac * Add comments * Fix notification not rendering content * Add more comments, rename `getKeyCode` to `getKeyName` * Fix incorrect size of <Link> text * Remove "expected" prefix * Revert `typeof content === 'object'` in `Notification` * Remove a comment about disabled keys in `ConfigService`, add a note about `keymap.` prefix * Improve `getKeyName` comment * Extract an inline object to a variable in `getKeyName` * Fix notification list padding * Change text for doc link & description for config error * Improve comment for `getKeyName` * Remove special formatting for list === 1 in `Notification` * Print valid modifiers * Call `getKeyboardShortcutSchema()` once * Collect issues from all validations, run `invalidModifiers` through a set * Change error message for `missingModifierIssue` * Convert duplicates warning to error * Define ALLOWED_KEY_CODES in a more concise way * Support both `IntlBackslash` and `Backquote` * Restore modifiers for mac to full spelling, sort them in order recommended by platform guidelines * Fix old comment about the shortcuts order --------- Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> (cherry picked from commit 4830ea1) * Connect: Generate json schema for app config (#22538) * Rename `putAllSync` to `writeSync` * Extend `FileStorage` with functions to replace an entire state and return file path used to create it * Add `updateJsonSchema` function * Generate schema for the app config * Always return a string from `createMockFileStorage().getFilePath()` * Add missing license headers * Rename 'teleport_connect_config_schema.json' to 'schema_app_config.json' * Rename `getKeyboardShortcutDescription` to `getShortcutDesc` * Rename `keyboardShortcutSchema` to `shortcutSchema` * Update a valid shortcut message * Rename `configJsonSchemaFile` to `jsonSchema` * Simplify `updateJsonSchema` * Set `$refStrategy` to none * Add missing description for `usageReporting.enabled` * Bump zod to the latest (improves TS performance) * Move `configService` implementation to `configStore` * Rename `configStore` to `configService` * Move `updateJsonSchema` to `createConfigService` * Move `validateStoredConfig` outside `createConfigService` * Rename `createAppConfigSchema.ts` to `appConfigSchema.ts`, `getKeyboardShortcutSchema.ts` to `keyboardShortcutSchema.ts` * Export `createKeyboardShortcutSchema` * Add license header (cherry picked from commit 9967149) * Connect: Show config file errors (#22728) * Add a function that returns error that might happen during loading the file * Return validation and file loading errors from `ConfigService` * Discard file storage updates when the file was not loaded correctly * Do not show usage reporting dialog when the file was not loaded correctly * Make title of notifications a little bit smaller, so longer titles do not look weird * Get rid of sync fs functions wherever possible in file storage * Move error handling code to `createFileStorage` * Improve `getConfigError` comment * Rename `discardUpdatesWhenLoadingFileFailed` to `discardUpdatesOnLoadError` * Fix the error message in `notifyAboutConfigErrors` * Revert "Make title of notifications a little bit smaller, so longer titles do not look weird" * Make `writeSync` async too * Return `unknown` from `FileStorage.get` * Add a TODO comment about createFileStorage type * Add "the" to "Using default config instead" * Remove `toString()` --------- Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> (cherry picked from commit 17ff6d6) * Connect: Add "Open Config File" item to menu (#22730) * Expose `openConfigFile` IPC * Add "Open Config File" to `NavigationMenu` * Make `NavigationMenu` more concise * Remove unused imports * Call "three dots" menu "More Options" to make it easier to refer to in the docs * Show a notification with a config file path * Render the icon component only in `NavigationItem` * Render the separator only with access request items * Use just `separator` string instead of an object * Render `StyledListItem` as a button * Shorten the notification Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> --------- Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> (cherry picked from commit af9e0ed) * Add docs for Connect config (#22898) * Rename old "quick input" to the current "command bar" feature name * Remove 'Droid Sans Fallback' from Linux fonts * Add docs for config * Link to the proper documentation section * Apply suggestions from @ravicious and @ibeckermayer Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com> * Adjust property descriptions with the names used in User Interface * Drop `Courier New` * Simplify `$schema` description * Fix lint issues * Add a break line after each heading Co-authored-by: Paul Gottschling <paul.gottschling@goteleport.com> * Schema should not be modified * Config file is created on first launch --------- Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com> Co-authored-by: Paul Gottschling <paul.gottschling@goteleport.com> (cherry picked from commit 2d387e1)

gzdunek requested review from ravicious and avatus February 23, 2023 15:50

gzdunek changed the base branch from master to gzdunek/prepare-keyboard-shortcuts-configuration February 23, 2023 15:50

github-actions bot added size/md ui labels Feb 23, 2023

github-actions bot requested a review from kimlisa February 23, 2023 15:50

gzdunek mentioned this pull request Feb 23, 2023

Connect: Prepare keyboard shortcuts configuration #22193

Merged

gzdunek removed the request for review from kimlisa February 23, 2023 15:51

gzdunek added the backport/branch/v12 label Feb 23, 2023

avatus reviewed Feb 24, 2023

View reviewed changes

web/packages/shared/components/Notification/Notification.tsx Outdated Show resolved Hide resolved

ravicious reviewed Feb 24, 2023

View reviewed changes

ravicious self-requested a review February 24, 2023 14:20

avatus approved these changes Feb 24, 2023

View reviewed changes

web/packages/teleterm/src/ui/initUi.tsx Outdated Show resolved Hide resolved

web/packages/teleterm/src/services/config/getKeyboardShortcutSchema.ts Outdated Show resolved Hide resolved

web/packages/teleterm/src/services/config/getKeyboardShortcutSchema.ts Outdated Show resolved Hide resolved

ravicious reviewed Feb 24, 2023

View reviewed changes

web/packages/teleterm/src/ui/services/keyboardShortcuts/keyboardShortcutsService.ts Show resolved Hide resolved

ravicious self-requested a review February 24, 2023 15:48

ravicious reviewed Feb 28, 2023

View reviewed changes

web/packages/teleterm/src/services/config/getKeyboardShortcutSchema.ts Outdated Show resolved Hide resolved

ravicious reviewed Feb 28, 2023

View reviewed changes

ravicious reviewed Mar 1, 2023

View reviewed changes

web/packages/shared/components/Notification/Notification.tsx Outdated Show resolved Hide resolved

ravicious reviewed Mar 2, 2023

View reviewed changes

ravicious self-requested a review March 2, 2023 11:10

ravicious reviewed Mar 2, 2023

View reviewed changes

gzdunek requested a review from ravicious March 2, 2023 16:06

ravicious mentioned this pull request Mar 3, 2023

Setting § as a "Toggle terminal windows" Eugeny/tabby#5280

Open

ravicious approved these changes Mar 3, 2023

View reviewed changes

Use event.code for non A-z keys

7e2adee

Without this fix, shortcuts like "Shift+1" can't work, because the reported character is "!".

gzdunek added 2 commits March 3, 2023 14:03

Support both IntlBackslash and Backquote

85f9ba3

Restore modifiers for mac to full spelling, sort them in order recomm…

23a6621

…ended by platform guidelines

gzdunek force-pushed the gzdunek/enable-keyboard-shortcuts-configuration branch from 2f39d38 to 3227260 Compare March 3, 2023 13:09

gzdunek requested review from klizhentas, russjones, r0mant and zmb3 as code owners March 3, 2023 13:09

gzdunek changed the base branch from gzdunek/prepare-keyboard-shortcuts-configuration to master March 3, 2023 13:09

public-teleport-github-review-bot bot removed request for klizhentas, russjones, r0mant and zmb3 March 3, 2023 13:10

gzdunek enabled auto-merge March 3, 2023 13:11

Fix old comment about the shortcuts order

5fda1e7

gzdunek force-pushed the gzdunek/enable-keyboard-shortcuts-configuration branch from 3227260 to 5fda1e7 Compare March 3, 2023 13:22

gzdunek added this pull request to the merge queue Mar 3, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 3, 2023

gzdunek added this pull request to the merge queue Mar 3, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 3, 2023

gzdunek added this pull request to the merge queue Mar 3, 2023

Merged via the queue into master with commit 4830ea1 Mar 3, 2023

ravicious deleted the gzdunek/enable-keyboard-shortcuts-configuration branch March 15, 2023 12:00

gzdunek mentioned this pull request Mar 16, 2023

[v12] Connect: Allow config customization #23197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connect: Enable keyboard shortcuts configuration #22194

Connect: Enable keyboard shortcuts configuration #22194

gzdunek commented Feb 23, 2023 •

edited

Loading

avatus left a comment

ravicious left a comment

avatus left a comment

gzdunek commented Mar 1, 2023

ravicious left a comment

gzdunek commented Mar 2, 2023

ravicious commented Mar 2, 2023

ravicious left a comment

ravicious left a comment

gzdunek commented Mar 3, 2023

public-teleport-github-review-bot bot commented Mar 3, 2023

Connect: Enable keyboard shortcuts configuration #22194

Connect: Enable keyboard shortcuts configuration #22194

Conversation

gzdunek commented Feb 23, 2023 • edited Loading

avatus left a comment

Choose a reason for hiding this comment

ravicious left a comment

Choose a reason for hiding this comment

avatus left a comment

Choose a reason for hiding this comment

gzdunek commented Mar 1, 2023

ravicious left a comment

Choose a reason for hiding this comment

gzdunek commented Mar 2, 2023

ravicious commented Mar 2, 2023

ravicious left a comment

Choose a reason for hiding this comment

ravicious left a comment

Choose a reason for hiding this comment

gzdunek commented Mar 3, 2023

public-teleport-github-review-bot bot commented Mar 3, 2023

gzdunek commented Feb 23, 2023 •

edited

Loading