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

AF-155: Improved Hotkeys #106

Merged
merged 7 commits into from
Aug 29, 2023
Merged

AF-155: Improved Hotkeys #106

merged 7 commits into from
Aug 29, 2023

Conversation

mikecarenzo
Copy link
Contributor

Overview

Improves hotkey stability and separates settings for Windows/Linux and MacOS.

Improved Hotkey Stability

This PR introduces a set of stability enhancements to the HotkeyObserver. Originally, the HotkeyObserver tracked the current state of the keyboard by monitoring keydown and keyup events. In effect:

  • A keydown event adds a key to the key state.
  • A keyup event removes a key from the key state.
  • When the current key state reflects an assigned hotkey, its associated callback is invoked.

Unfortunately, there are cases where a keydown event DOES NOT trigger an associated keyup event. For instance:

  • If a hotkey opens up a link in another tab, there will be no keyup event.
  • If a hotkey opens a separate window (e.g. a file selection dialog), there will be no keyup event.
  • If the Command (⌘) key is held (on MacOS), all other keyup events are ignored.

As a result, the HotkeyObserver is occasionally led to believe that certain keys are still being held down even when the user's hands are off the keyboard. What's worse, if the user tries another hotkey combination, the pressed keys will be added to an already incorrect key state, the corrupted key state will fail to match any registered hotkeys, and nothing happens.

The initial implementation of HotkeyObserver attempted to account for this keyup quirk. However, the list of reasons keyup doesn't fire has grown to a point where it no longer makes sense to rely on it.

Now, the keyboard state is assessed using everything that can be gleaned from a single KeyboardEvent (raised by keydown). This includes the currently pressed key and any associated modifier keys (Control, Alt, Shift, and Meta). While this limits possible hotkey sequences, it does remove the need to track the current key state across keydown and keyup events.

Separate Hotkey Settings for Windows/Linux and MacOS

This PR replaces settings.json with settings_win.json and settings_macos.json. Both .json enumerate the same list of settings (with different values) and allow the application to be configured dynamically depending on the device's current operating system. This new feature has been used to add support for MacOS style hotkeys.

@mikecarenzo mikecarenzo self-assigned this Aug 26, 2023
Copy link
Contributor

@mehaase mehaase left a comment

Choose a reason for hiding this comment

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

Looks great and works great. Noticed one issue with key binding for Cmd+N and a couple minor comments.

@mehaase
Copy link
Contributor

mehaase commented Aug 28, 2023

@mikecarenzo As you merge the hotkeys from the search branch, I wanted to highlight this comment from PR #99:

I ended up going with F3 for "Find Next" and Shift+F3 for "Find Previous" to match how notepad.exe works. (On MacOS it would usually be Cmd+G and Cmd+Shift+G, I think, but that might conflict with the "Grid" hotkey.)

Screenshot 2023-08-28 at 3 30 59 PM

@sonarcloud
Copy link

sonarcloud bot commented Aug 29, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug B 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@mikecarenzo
Copy link
Contributor Author

@mehaase I'd like to stick to the Operating System's conventions as closely as possible. On MacOS I changed the "Grid" shortcut to Option+G. I also tweaked the way shortcut text appears so that it aligns with OS conventions.
shortcut_conventions

@mikecarenzo mikecarenzo merged commit b0be7f4 into main Aug 29, 2023
3 of 6 checks passed
@mikecarenzo mikecarenzo deleted the AF-155_macos_hotkeys branch August 29, 2023 15:28
@mehaase
Copy link
Contributor

mehaase commented Aug 29, 2023

The MacOS shortcuts look great! Thanks for picking up on that detail.

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.

None yet

2 participants