Skip to content
This repository has been archived by the owner on Apr 18, 2024. It is now read-only.

fix: LEAP-248: Fix app crashing by hotkeys #1653

Merged
merged 6 commits into from Jan 5, 2024

Conversation

Gondragos
Copy link
Contributor

@Gondragos Gondragos commented Dec 29, 2023

This PR

  1. wraps unbinding functional to separate keys corectly outside of the keymaster
  2. adds alias for , to avoid problems with it
  3. improves tokenization of hotkeys

PR fulfills these requirements

  • Tests for the changes have been added/updated
  • Docs have been added/updated
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance

Describe the reason for change

Keymaster can't handle unbinding of multiple keys. It also misunderstands , and thinks that it is two hotkeys in one (',' and ''). So that , breaks application in two ways related to each other.

What alternative approaches were there?

  1. we can fix keymaster in a fork (but we did it before and we didn't like it)
  2. we can use another library to handle hotkeys. But it's hard to test and integrate.

This change affects (describe how if yes)

  • Performance
  • Security
  • UX

Does this PR introduce a breaking change?

  • Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • Not sure (briefly explain the situation below)

What level of testing was included in the change?

  • e2e (codecept)
  • integration (cypress)
  • unit (jest)

Which logical domain(s) does this change affect?

Hotkeys, Labels

The 'unbind' function in Hotkey.ts has been enhanced to handle multiple keys provided as a string. A new helper function 'getKeys' was added to split the key string and return an array of keys. Each key in the array is then processed individually in the 'unbind' function. This helps keymaster to handle more that one hotkey.
Added a new mapping for the comma key in the ALIASES object. Also, refactored the applyAliases function to use the getKeys function for better tokenization and handling of keys. This change helps in fixing the keymaster inconsistencies with certain keys.
@codecov-commenter
Copy link

codecov-commenter commented Dec 29, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (d1e4416) 64.56% compared to head (415e87d) 64.57%.

Files Patch % Lines
src/core/Hotkey.ts 85.71% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1653   +/-   ##
=======================================
  Coverage   64.56%   64.57%           
=======================================
  Files         443      443           
  Lines       28693    28699    +6     
  Branches     7519     7519           
=======================================
+ Hits        18527    18532    +5     
- Misses      10166    10167    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -82,6 +82,7 @@ keymaster.filter = function(event) {
const ALIASES = {
'plus': '=', // "ctrl plus" is actually a "ctrl =" because shift is not used
'minus': '-',
',': '¼', // This should be a comma but it's not working with keymaster
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is it ¼? pretty confusing

@hlomzik
Copy link
Collaborator

hlomzik commented Jan 4, 2024

/git merge master

Workflow run
Successfully pushed new changes:
Merge remote-tracking branch 'origin/master' into fb-leap-248/multiple-hotkeys (415e87d)

@Gondragos Gondragos enabled auto-merge (squash) January 5, 2024 15:15
@Gondragos Gondragos merged commit 28c0e12 into master Jan 5, 2024
11 of 14 checks passed
@Gondragos Gondragos deleted the fb-leap-248/multiple-hotkeys branch January 5, 2024 15:30
MasherJames pushed a commit to HelloPareto/label-studio-frontend that referenced this pull request Feb 29, 2024
* fix: LEAP-248: Fix app crashing by multiple hotkeys

The 'unbind' function in Hotkey.ts has been enhanced to handle multiple keys provided as a string. A new helper function 'getKeys' was added to split the key string and return an array of keys. Each key in the array is then processed individually in the 'unbind' function. This helps keymaster to handle more that one hotkey.

* Add alias for comma to prevent keymaster fails

Added a new mapping for the comma key in the ALIASES object. Also, refactored the applyAliases function to use the getKeys function for better tokenization and handling of keys. This change helps in fixing the keymaster inconsistencies with certain keys.

* Add regression tests for hotkeys problem

* Fix import paths

* Improve the comment

---------

Co-authored-by: hlomzik <hlomzik@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants