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

Fix hotkeys matching #142

Merged

Conversation

TheBestTvarynka
Copy link
Contributor

Hi,
Thank you for such a great library! I'm using it in my project and am happy with the functionality. It has everything I need.

Unfortunately, I've faced a bug in the hotkeys matching algorithm: it doesn't check that the controls aren't pressed when they're not needed. Let me give you an example to explain the situation better.

Suppose I want to do some action on the ctrl+m hotkey. The action will be executed even on ctrl+shift+m or ctrl+alt+m. It doesn't seem correct, because I may want to set another action on the ctrl+shift+m hotkey. The most common case is when the user wants to add hotkeys for scrolling: ctrl+tab is next and ctrl+shift+tab is prev.

In this PR, I've fixed this bug by checking if the modifier key is not pressed when it doesn't need to be pressed.

Copy link
Member

@friendlymatthew friendlymatthew left a comment

Choose a reason for hiding this comment

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

Thank you for the kind words Pavlo :) and thank you for catching this bug! The changes look very sane to me.

Congrats on your first contribution here, and welcome aboard 🎉

@friendlymatthew friendlymatthew merged commit 669416d into gaucho-labs:main Oct 26, 2024
7 checks passed
@friendlymatthew
Copy link
Member

@all-contributors please add @TheBestTvarynka for code

Copy link
Contributor

@friendlymatthew

I've put up a pull request to add @TheBestTvarynka! 🎉

@TheBestTvarynka
Copy link
Contributor Author

Thank you 😊

@TheBestTvarynka TheBestTvarynka deleted the fix/hotkeys-matching branch October 26, 2024 14:30
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.

2 participants