Connect: Prepare keyboard shortcuts configuration #22193

gzdunek · 2023-02-23T15:48:12Z

1/2
Part of #21914

Initially I wanted to create a single PR for all the changes, but I realized that it will be hard to review. The PR that actually enables shortcuts configuration is here.

In this PR I focused on renaming a few things:

For keyboard shortcuts, from now we will be using only "accelerator" (e.g. Command+1) and "action" (e.g. tab1) words. Previously, we used words like: "key", "type", "shortcut" which were really confusing (even for me who wrote this code 🙃). I borrowed "accelerator" from Electron.
I renamed some config keys, like keymap.tabNew, to keymap.newTab to make them all more consistent.
I renamed some config keys, like keymap.toggleClusters to keymap.openClusters. "Toggle" sounds like a more serious and permanent action, while it only opens (and closes) a popup.
I renamed keyboard shortcuts actions, like tab-1 to tab1 to match config keys (keymap.tab1).

I also changed the separator for the accelerators. With "-" we were not able to use this symbol as a valid key code.

Best to review commit by commit.

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"

With "-" we are not able to use this symbol as a valid key code.

ravicious

Good refactor, I like the distinction between an accelerator and an action a lot.

At first I thought that the name "accelerator" is just a pun (because electron accelerators exist) but it looks like it's used outside of Electron as well.

ravicious · 2023-02-23T15:59:44Z

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

+  /** Inverts shortcuts-keys pairs to allow accessing shortcut by an accelerator. */
+  private mapAcceleratorsToActions(): void {
+    this.acceleratorsToActions.clear();


Why does it need to call clear in the first place? 🤔

If you feel like it, seeing how this is used only in the constructor I'd be for rewriting it to be free of side effects. It could be a function which takes a shortcut config and returns a map. In the constructor, we could assign its return value to this.acceleratorsToActions.

Why does it need to call clear in the first place? 🤔

I think it was prepared to recalculate the shortcuts multiple times (to update them while the app is working), but we do it only once :)

I'd be for rewriting it to be free of side effects.

Done.

ravicious · 2023-02-23T16:02:19Z

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

@@ -41,18 +41,18 @@ export function useKeyboardShortcutFormatters(): KeyboardShortcutFormatters {
  const keyboardShortcuts = keyboardShortcutsService.getShortcutsConfig();

  return {
-    getLabelWithShortcut(label, shortcutKey, options) {
+    getLabelWithShortcut(label, shortcutAction, options) {


Should those functions be renamed to something like getLabelWithAccelerator, getAccelerator? I like the action & accelerator distinction, when we say just "shortcut" I feel like we loose the clarity that this distinction brings us.

Done! I'm wondering if we should rename the top level useKeyboardShortcutFormatters. It feels to me like it is a good, more general name 🤔

I'm wondering if we should rename the top level useKeyboardShortcutFormatters. It feels to me like it is a good, more general name 🤔

I feel the same way.

ravicious · 2023-02-23T16:02:53Z

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

@@ -61,19 +61,19 @@ export function useKeyboardShortcutFormatters(): KeyboardShortcutFormatters {

 function formatKeyboardShortcut(options: {


Same question as above.

public-teleport-github-review-bot · 2023-03-03T12:09:51Z

@gzdunek See the table below for backport results.

Branch	Result
branch/v12	Failed

* 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)

@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 <[email protected]> (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 <[email protected]> (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 <[email protected]> --------- Co-authored-by: Rafał Cieślak <[email protected]> (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 <[email protected]> Co-authored-by: Isaiah Becker-Mayer <[email protected]> * 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 <[email protected]> * Schema should not be modified * Config file is created on first launch --------- Co-authored-by: Rafał Cieślak <[email protected]> Co-authored-by: Isaiah Becker-Mayer <[email protected]> Co-authored-by: Paul Gottschling <[email protected]> (cherry picked from commit 2d387e1)

gzdunek added 4 commits February 23, 2023 10:56

Use "+" instead of "-" as a separator

86f2071

With "-" we are not able to use this symbol as a valid key code.

Rename some config keys to follow the same naming pattern

b68cbb5

Rename keyboard shortcuts to match config keys

857805e

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

github-actions bot requested review from hatched and ryanclark February 23, 2023 15:48

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

gzdunek removed request for hatched and ryanclark February 23, 2023 15:49

gzdunek mentioned this pull request Feb 23, 2023

Connect: Enable keyboard shortcuts configuration #22194

Merged

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

ravicious approved these changes Feb 23, 2023

View reviewed changes

gzdunek added 2 commits February 24, 2023 11:42

Extract mapAcceleratorsToActions to a function

790087e

Rename other "keyboard shortcuts" to "accelerators" and "actions"

38aabfe

avatus approved these changes Feb 24, 2023

View reviewed changes

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

Merged via the queue into master with commit 717c4fa Mar 3, 2023

gzdunek mentioned this pull request Mar 16, 2023

[v12] Connect: Allow config customization #23197

Merged

gzdunek deleted the gzdunek/prepare-keyboard-shortcuts-configuration branch March 20, 2023 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connect: Prepare keyboard shortcuts configuration #22193

Connect: Prepare keyboard shortcuts configuration #22193

gzdunek commented Feb 23, 2023 •

edited

Loading

ravicious left a comment

ravicious Feb 23, 2023

gzdunek Feb 24, 2023

ravicious Feb 23, 2023

gzdunek Feb 24, 2023

ravicious Feb 24, 2023

ravicious Feb 23, 2023

gzdunek Feb 24, 2023

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

		@@ -61,19 +61,19 @@ export function useKeyboardShortcutFormatters(): KeyboardShortcutFormatters {

		function formatKeyboardShortcut(options: {

Connect: Prepare keyboard shortcuts configuration #22193

Connect: Prepare keyboard shortcuts configuration #22193

Conversation

gzdunek commented Feb 23, 2023 • edited Loading

ravicious left a comment

Choose a reason for hiding this comment

ravicious Feb 23, 2023

Choose a reason for hiding this comment

gzdunek Feb 24, 2023

Choose a reason for hiding this comment

ravicious Feb 23, 2023

Choose a reason for hiding this comment

gzdunek Feb 24, 2023

Choose a reason for hiding this comment

ravicious Feb 24, 2023

Choose a reason for hiding this comment

ravicious Feb 23, 2023

Choose a reason for hiding this comment

gzdunek Feb 24, 2023

Choose a reason for hiding this comment

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

gzdunek commented Feb 23, 2023 •

edited

Loading