fix: shortcuts with `Option` key by Ty3uK · Pull Request #31 · ParthJadhav/Verve

Ty3uK · 2023-01-05T23:04:45Z

Related to #11.

event.key returns transformed char when option key is pressed.

For example, when user is trying to record shortcut Option+Shift+G, event.key returns Option+Shift+˝.

event.code returns KeyG instead. So getting substring with offset 3 fixes this issue.

This commit fixes another bug: when user records shortcut that already registered, window will hide. Adding unregister before start recording fixes this issue.

Related to ParthJadhav#11. `event.key` returns transformed char when `option` key is pressed. For example, when user is trying to record shortcut `Option+Shift+G`, `event.key` returns `Option+Shift+˝`. `event.code` returns `KeyG` instead. So getting substring with offset 3 fixes this issue. This commit fixes another bug: when user records shortcut that already registered, window will hide. Adding `unregister` before start recording fixes this issue.

ParthJadhav · 2023-01-06T05:52:34Z

        shortcutArray = [];
        hotkeys.unbind();
+        // Prevent closing Preferences when recording already registered shortcut
+        unregister(preferences.get("shortcut"));


There would be an issue with this though...

For eg:

if I try executing setting CMD + Space which already is assigned to spotlight. When I click on the recorder, It will unregister the old shortcut but when I press CMD + Space spotlight would be opened. And hence there won't be a new registered shortcut & the previous shortcut would also be unregistered.

Leaving us with no shortcut to start the app 😄

@ParthJadhav, I have the same issue on current master 🥲

The problem is in hotkeys-js: it cannot capture last key of system hotkeys sequence

Screen.Recording.2023-01-06.at.21.14.36.mp4

So maybe we put this problem into another issue? Or you prefer resolve it in this PR? 🙂

ParthJadhav · 2023-01-06T15:21:14Z

Ohh, I see. That's alright. We will resolve it in different PR. This doesn't seem to be that important as of now

ParthJadhav · 2023-01-06T15:21:28Z

Do you think this PR is ready to merge?

Ty3uK · 2023-01-06T15:22:25Z

Do you think this PR is ready to merge?

Give me another five minutes to check production release 🫡

Ty3uK · 2023-01-06T15:28:43Z

Got another bug when trying to set Option+Shift+Space shortcut

Ty3uK · 2023-01-06T15:34:03Z

-                            } else {
-                                shortcutArray.push(event.code.substring(3));
-                            }
+                            const key = event.code.startsWith("Key")


@ParthJadhav, check this please

Ty3uK · 2023-01-06T15:37:32Z

Also, quick question: do I need to squash commits or you prefer to preserve full history of changes?

ParthJadhav · 2023-01-06T15:38:52Z

Also, quick question: do I need to squash commits or you prefer to preserve full history of changes?

Oh dw, I'll directly do Squash & Merge

ParthJadhav · 2023-01-06T15:39:14Z

TBH, The whole shortcut recorder function is messy & hard to understand.

Ty3uK · 2023-01-06T15:40:32Z

TBH, The whole shortcut recorder function is messy & hard to understand.

You can create issue and assign it to me, if you want. I can do some refactoring 🙂

ParthJadhav · 2023-01-06T15:41:52Z

I've tested it and everything works as expected... Except if anything else launches while setting the shortcut then we can only open the window by clicking the menu item

Ty3uK · 2023-01-06T15:43:13Z

I've tested it and everything works as expected... Except if anything else launches while setting the shortcut then we can only open the window by clicking the menu item

Maybe we can do some researching for global shortcuts from Rust itself? I think Tauri do this under the hood, but maybe some other library can do this better...

ParthJadhav · 2023-01-06T15:45:32Z

I've tested it and everything works as expected... Except if anything else launches while setting the shortcut then we can only open the window by clicking the menu item

Maybe we can do some researching for global shortcuts from Rust itself? I think Tauri do this under the hood, but maybe some other library can do this better...

Oh no, Everything works as expected. But the fact that we unregister the key before recording the shortcut is causing the problem.

What do you think?

ParthJadhav · 2023-01-06T15:48:08Z

You can create issue and assign it to me, if you want. I can do some refactoring 🙂

I think we can do it at later time when we find to be working on any issue related to the shortcut function... What are you opinion on it?

Ty3uK · 2023-01-06T15:48:39Z

I've tested it and everything works as expected... Except if anything else launches while setting the shortcut then we can only open the window by clicking the menu item

Maybe we can do some researching for global shortcuts from Rust itself? I think Tauri do this under the hood, but maybe some other library can do this better...

Oh no, Everything works as expected. But the fact that we unregister the key before recording the shortcut is causing the problem.

What do you think?

I've tried to comment unregister and it does nothing 🥲

Ty3uK · 2023-01-06T15:49:22Z

You can create issue and assign it to me, if you want. I can do some refactoring 🙂

I think we can do it at later time when we find to be working on any issue related to the shortcut function... What are you opinion on it?

Great catch! I think the same way 🙂

ParthJadhav · 2023-01-06T15:52:40Z

I've tested it and everything works as expected... Except if anything else launches while setting the shortcut then we can only open the window by clicking the menu item

Maybe we can do some researching for global shortcuts from Rust itself? I think Tauri do this under the hood, but maybe some other library can do this better...

Oh no, Everything works as expected. But the fact that we unregister the key before recording the shortcut is causing the problem.
What do you think?

I've tried to comment unregister and it does nothing 🥲

If you just unbind the unregister line you would notice that the pre-existing shortcut still works after you try to set a shortcut which already exist.

Eg: if you try option + command + space without unregister then it would now listen for the default shortcut which was CMD + SHIFT + G... But with Unregister it wouldn't listen for anything..

ParthJadhav · 2023-01-06T15:54:54Z

Do you use Discord by any chance ? It would be easier to communicate there IMO 😄

Ty3uK · 2023-01-06T16:26:39Z

Do you use Discord by any chance ? It would be easier to communicate there IMO 😄

Yes 🙂 Ty3uK#0057

Ty3uK · 2023-01-06T16:33:07Z

I've tested it and everything works as expected... Except if anything else launches while setting the shortcut then we can only open the window by clicking the menu item

Maybe we can do some researching for global shortcuts from Rust itself? I think Tauri do this under the hood, but maybe some other library can do this better...

Oh no, Everything works as expected. But the fact that we unregister the key before recording the shortcut is causing the problem.
What do you think?

I've tried to comment unregister and it does nothing 🥲

If you just unbind the unregister line you would notice that the pre-existing shortcut still works after you try to set a shortcut which already exist.

Eg: if you try option + command + space without unregister then it would now listen for the default shortcut which was CMD + SHIFT + G... But with Unregister it wouldn't listen for anything..

Got it. So maybe I just drop this line and we put this in another issue?

Also, we can do some refactoring just for resolving that kind of issues 🙂

ParthJadhav · 2023-01-06T16:41:27Z

I've tested it and everything works as expected... Except if anything else launches while setting the shortcut then we can only open the window by clicking the menu item

Maybe we can do some researching for global shortcuts from Rust itself? I think Tauri do this under the hood, but maybe some other library can do this better...

Oh no, Everything works as expected. But the fact that we unregister the key before recording the shortcut is causing the problem.
What do you think?

I've tried to comment unregister and it does nothing 🥲

If you just unbind the unregister line you would notice that the pre-existing shortcut still works after you try to set a shortcut which already exist.
Eg: if you try option + command + space without unregister then it would now listen for the default shortcut which was CMD + SHIFT + G... But with Unregister it wouldn't listen for anything..

Got it. So maybe I just drop this line and we put this in another issue?

Also, we can do some refactoring just for resolving that kind of issues 🙂

Yep, lets do that 🙌

ParthJadhav · 2023-01-06T17:04:41Z

Awesome, Let's merge it 🚀

ParthJadhav reviewed Jan 6, 2023

View reviewed changes

ParthJadhav linked an issue Jan 6, 2023 that may be closed by this pull request

Address Buggy shortcuts when using Option key as modifier #11

Closed

fix: properly resolve Space key

1fc13d4

Ty3uK commented Jan 6, 2023

View reviewed changes

fix: drop unregister code to fix it in another PR

e4daa95

ParthJadhav merged commit 8eb8ded into ParthJadhav:master Jan 6, 2023

Uh oh!

Conversation

Ty3uK commented Jan 5, 2023

Uh oh!

ParthJadhav Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

Ty3uK Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

Ty3uK Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

ParthJadhav commented Jan 6, 2023

Uh oh!

ParthJadhav commented Jan 6, 2023

Uh oh!

Ty3uK commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ty3uK commented Jan 6, 2023

Uh oh!

Ty3uK Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

ParthJadhav Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

Ty3uK commented Jan 6, 2023

Uh oh!

ParthJadhav commented Jan 6, 2023

Uh oh!

ParthJadhav commented Jan 6, 2023

Uh oh!

Ty3uK commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ParthJadhav commented Jan 6, 2023

Uh oh!

Ty3uK commented Jan 6, 2023

Uh oh!

ParthJadhav commented Jan 6, 2023

Uh oh!

ParthJadhav commented Jan 6, 2023

Uh oh!

Ty3uK commented Jan 6, 2023

Uh oh!

Ty3uK commented Jan 6, 2023

Uh oh!

ParthJadhav commented Jan 6, 2023

Uh oh!

ParthJadhav commented Jan 6, 2023

Uh oh!

Ty3uK commented Jan 6, 2023

Uh oh!

Ty3uK commented Jan 6, 2023 • edited by ParthJadhav Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ParthJadhav commented Jan 6, 2023

Uh oh!

ParthJadhav commented Jan 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Ty3uK commented Jan 6, 2023 •

edited

Loading

Ty3uK commented Jan 6, 2023 •

edited

Loading

Ty3uK commented Jan 6, 2023 •

edited by ParthJadhav

Loading