Choose Summon keybinding #1489

goosewobbler · 2023-03-15T00:47:06Z

Allowing users to choose the summon keybinding. Key presses are detected and mapped to an accelerator which is registered using the globalShortcut API. We are still using the experimental keyboard API getLayoutMap to better represent shortcut keys to users with different keyboard layouts (though this does not work in all cases)

https://www.w3.org/TR/uievents-code/#code-value-tables
https://www.electronjs.org/docs/latest/api/accelerator
https://www.electronjs.org/docs/latest/api/global-shortcut
https://developer.mozilla.org/en-US/docs/Web/API/Keyboard/getLayoutMap

Limitations

Only one shortcutKey per binding
- (e.g. CTRL + SHIFT + D works because multiple modifierKeys are allowed but CTRL + A + D is not possible due to two shortcutKeys) - this is an Electron limitation, accelerators are defined as such
Certain OS-bound keys won't be captured, depending on OS and settings
Some keys have issues - see [Bug]: globalShortcut won't register single quote or back quote correctly on different keyboard layouts (Windows 10) electron/electron#28900 (comment)

Testing

102 key UK keyboard (all platforms) & Macbook Pro UK keyboard (Mac)
Layouts - EN (US), EN (UK), ES (Spain), FR (France), DE (Germany)

Nonfunctional Bindings

Unless we disable these keys for every layout, there is currently no way to detect a layout (outside of specific cases where the event key property maps to a different code when it is likely to be nonfunctional).

Quote - nonfunctional on Windows (EN-GB)
Backquote - nonfunctional on Windows (EN-GB)

Disabled Keys

Keys are disabled when a matching accelerator does not exist, or if the accelerator is partly or completely non-functional. In these cases a user would be able to successfully bind a key which wouldn't summon or hide Frame. Disabling the keys means that they won't register when the user presses them when trying to configure the shortcut.

It's not feasible to test all of the layouts so we will aim for a decent base level of coverage / disablement of problematic keys and rely on users to report nonfunctional bindings which would benefit their setup with a given keyboard & layout.

Global - disabled on all platforms

IntlBackslash, IntlRo, IntlYen, Pause, NumpadEnter, AltRight

Mac

NumLock

Windows

F12

Disabled when the event key property doesn't match the expected one (most non EN layouts):
Backslash, Backquote, Minus, Equal, BracketLeft, BracketRight, Quote, Comma, Period, Slash, Semicolon

Linux

Backslash, Backquote, Minus, Equal, BracketLeft, BracketRight, Quote, Comma, Period, Slash

Disabled when the event key property doesn't match the expected one (most non EN layouts):
Semicolon

TODO

mholtzman

had a couple issues with running on Ubuntu, though not sure if they're about the code, OS specific, or specific to what I'm running:

some meta+ combinations such as Meta+B only hide but dont summon
can't assign slash as the key code, which is the default shortcut

in general I think this could be a pretty big rabbit hole if we try to carve out edge cases for every single keyboard layout and locale. if there's no comprehensive way to simply map key codes to strings, maybe we can simplify the problem by only allowing the key code to be a fixed, smaller set of mostly universally-defined keys? possibly something like only letters, numbers, and a small set of punctuation like slashes

mholtzman · 2023-03-21T12:50:14Z

app/dash/Settings/index.js

@@ -30,7 +32,37 @@ class Settings extends Component {
  }

  render() {
-    const { modifierKey, summonKey } = getSummonShortcut(this.store('platform'))
+    const summonShortcut = this.store('main.shortcuts.summon')


can this logic be extracted into its own functional component?

mholtzman · 2023-03-21T12:50:35Z

app/dash/Settings/index.js

+      platform,
+      summonShortcut
+    )
+    hotkeys.unbind()


this kind of setup can be done in a useEffect hook that runs when the value of configureShortcut changes instead of every time it renders

mholtzman · 2023-03-21T12:51:38Z

app/dash/Settings/index.js

-    const { modifierKey, summonKey } = getSummonShortcut(this.store('platform'))
+    const summonShortcut = this.store('main.shortcuts.summon')
+    const platform = this.store('platform')
+    const { modifierKeys: summonModifierKeys, shortcutKey: summonShortcutKey } = getDisplayShortcut(


it seems like a bunch of extra work to store the shortcut as a deconstructed object and have to put it together in multiple places. can we just create the accelerator string once when its pressed and pass that around?

The problem with this is that the accelerator string will never change according to the key layout of the user when it comes to displaying the shortcut. I thought it better to store the shortcut so that it can be mapped using the keyboard layoutMap API at runtime, rather than having to parse an accelerator each time

mholtzman · 2023-03-21T12:54:42Z

app/dash/Settings/index.js

+    hotkeys.unbind()
+    if (this.state.configureShortcut) {
+      // disable existing shortcut whilst configuring a new one
+      link.send('tray:action', 'setShortcut', 'summon', {


I don't think we should disable the shortcut as if the user cancels its now disabled if it was previously enabled

Problem here is that if you try to set the same shortcut again it just hides Frame. Maybe this is less of an issue now we have a cancel button

Setting the same shortcut again edge case definitely problematic so I made the cancel revert the state

mholtzman · 2023-03-21T12:56:38Z

app/dash/Settings/index.js

+        const isModifierKey = modifierKeys.includes(event.key)
+
+        // ignore modifier key solo keypresses and disabled keys
+        if (!isModifierKey && !isDisabledKey(event, platform)) {


this logic can be moved into something like the getShorcutFromKeyEvent function and that function can simply respond with the accelerator string or undefined if the shortcut is not valid

we may also be able to leverage something like this: https://github.com/brrd/electron-is-accelerator

this logic can be moved into something like the getShorcutFromKeyEvent function and that function can simply respond with the accelerator string or undefined if the shortcut is not valid

This works if we are storing the accelerator but I think it's better to keep the key code + modifiers as a single source of truth and create the accelerator from that

we may also be able to leverage something like this: brrd/electron-is-accelerator

Not been updated in a while, wouldn't want to rely on it

mholtzman · 2023-03-21T13:09:43Z

@goosewobbler also lets target canary as the base branch

Co-authored-by: goosewobbler <goosewobbler@pm.me>

goosewobbler · 2023-03-21T16:30:12Z

Closed in favour of #1494

goosewobbler added WIP PRs that are still in progress and not ready for review or merging enhancement New feature or request accessibility window management and removed WIP PRs that are still in progress and not ready for review or merging labels Mar 15, 2023

mholtzman requested changes Mar 21, 2023

View reviewed changes

mholtzman and others added 22 commits March 21, 2023 11:26

bump version to 0.6.3 (#1477)

f52286d

disable explorer btn on tx when no explorer (#1475)

23ab70c

Linting (#1337)

7ae3ed2

Co-authored-by: goosewobbler <goosewobbler@pm.me>

first pass

7f9dcec

remove debounce & key exemptions

8394917

update deps (#1480)

929cf73

Co-authored-by: goosewobbler <goosewobbler@pm.me>

fix broken images in balances & inventory (#1484)

93e7c65

Co-authored-by: goosewobbler <goosewobbler@pm.me>

Linting fixes (#1470)

2ad5299

Co-authored-by: goosewobbler <goosewobbler@pm.me>

explicitly add all keys requiring mapping

f109086

disable during config, enable after

51c17b6

fix onboard slide

584519b

update meta key display

d82a573

add event log

b6eeaa6

first pass at disabled keys

751a3fd

linux update

ee0b265

update lock

8e876df

windows update

9da9dd7

linux update

10fbdc3

more-linux-updates

a013f13

windows update

d70fe6e

remove logs

3a4c57e

address UX

cc7c63b

mholtzman force-pushed the choose-summon-keybinding branch from b8032e9 to cc7c63b Compare March 21, 2023 15:33

update migration

b2ea753

mholtzman changed the base branch from develop to canary March 21, 2023 15:37

goosewobbler closed this Mar 21, 2023

mholtzman deleted the choose-summon-keybinding branch April 14, 2023 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Choose Summon keybinding #1489

Choose Summon keybinding #1489

goosewobbler commented Mar 15, 2023 •

edited

Loading

mholtzman left a comment

mholtzman Mar 21, 2023

mholtzman Mar 21, 2023

mholtzman Mar 21, 2023

goosewobbler Mar 21, 2023 •

edited

Loading

mholtzman Mar 21, 2023

goosewobbler Mar 21, 2023 •

edited

Loading

goosewobbler Mar 21, 2023 •

edited

Loading

mholtzman Mar 21, 2023

mholtzman Mar 21, 2023

goosewobbler Mar 21, 2023

mholtzman commented Mar 21, 2023

goosewobbler commented Mar 21, 2023

Choose Summon keybinding #1489

Choose Summon keybinding #1489

Conversation

goosewobbler commented Mar 15, 2023 • edited Loading

Limitations

Testing

Nonfunctional Bindings

Disabled Keys

Global - disabled on all platforms

Mac

Windows

Linux

mholtzman left a comment

Choose a reason for hiding this comment

mholtzman Mar 21, 2023

Choose a reason for hiding this comment

mholtzman Mar 21, 2023

Choose a reason for hiding this comment

mholtzman Mar 21, 2023

Choose a reason for hiding this comment

goosewobbler Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

mholtzman Mar 21, 2023

Choose a reason for hiding this comment

goosewobbler Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

goosewobbler Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

mholtzman Mar 21, 2023

Choose a reason for hiding this comment

mholtzman Mar 21, 2023

Choose a reason for hiding this comment

goosewobbler Mar 21, 2023

Choose a reason for hiding this comment

mholtzman commented Mar 21, 2023

goosewobbler commented Mar 21, 2023

goosewobbler commented Mar 15, 2023 •

edited

Loading

goosewobbler Mar 21, 2023 •

edited

Loading

goosewobbler Mar 21, 2023 •

edited

Loading

goosewobbler Mar 21, 2023 •

edited

Loading