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

fixes eventToHotkeyString returns doubled "modifier" when "key" is a modifier #64

Merged
merged 3 commits into from
Dec 17, 2021

Conversation

theinterned
Copy link
Contributor

@theinterned theinterned commented Dec 16, 2021

resolves #60

This PR fixes a bug where eventToHotkeyString was returning a "double modifier" for a key-combination where a modifier key (Control, Alt, Meta, or Shift) is also the event.key.

This PR updates eventToHotkeyString so that the resulting string still presents modifiers in a consistent order.

For more details see #60

Before After
Screen Shot 2021-12-15 at 9 50 36 AM Screen Shot 2021-12-16 at 5 58 21 PM

Screencast of the after

Screen.Recording.2021-12-16.at.5.59.41.PM.mov

Breaking change

Although the previous behaviour was likely a bug, this is a breaking change in that it changes the "hotkey string" format when a modifier key is used as the hotkey (the final element in a hotkey string sequence) for example Meta+Shift+Shift becomes Meta+Shift after this change.

Comment on lines +222 to +223
['Control+Shift', {ctrlKey: true, shiftKey: true, key: 'Shift'}],
['Control+Shift', {ctrlKey: true, shiftKey: true, key: 'Control'}]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were initially failing with:

FAILED TESTS:
  hotkey
    eventToHotkeyString
      ✖ {"ctrlKey":true,"shiftKey":true,"key":"Shift"} => Control+Shift
        Chrome Headless 96.0.4664.110 (Mac OS 10.15.7)
      Error: Uncaught AssertionError: expected 'Control+Shift+Shift' to equal 'Control+Shift' (node_modules/chai/chai.js:250)
          at Context.<anonymous> (test/test.js:232:23)

      ✖ {"ctrlKey":true,"shiftKey":true,"key":"Control"} => Control+Shift
        Chrome Headless 96.0.4664.110 (Mac OS 10.15.7)
      Error: Uncaught AssertionError: expected 'Control+Shift+Control' to equal 'Control+Shift' (node_modules/chai/chai.js:250)
          at Context.<anonymous> (test/test.js:232:23)

@theinterned theinterned added the bug Something isn't working label Dec 16, 2021
@theinterned theinterned self-assigned this Dec 16, 2021
@theinterned
Copy link
Contributor Author

I wonder if you reviewers would consider this a breaking change? It is arguably as the format of the string does change.

Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

This looks good 👍

I do think it's a breaking change. Maybe we should rip the band-aid off with a breaking change that reworks the format?

@theinterned theinterned mentioned this pull request Dec 17, 2021
3 tasks
@theinterned theinterned merged commit 70208b4 into main Dec 17, 2021
@theinterned theinterned deleted the theinterned/double-modifier branch December 17, 2021 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eventToHotkeyString returns doubled "modifier" when "key" is a modifier
2 participants