This repository has been archived by the owner on Feb 16, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implement global shortcuts #393
Implement global shortcuts #393
Changes from 6 commits
1afa8f5
1f1b5ca
2d21475
7773575
75762cc
bff74b8
a47d53a
35cffd4
6b0bc39
985de79
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update
DefaultVersionAstilectron
tov0.58.0
below?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could your remove this empty line ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add the missing
ed
at the end ofglobal.shortcuts.cmd.is.register
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you replace the comment with
// GlobalShortcuts represents global shortcuts
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove the comment here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also could you not store pointers to the callback here?
Unless I'm missing something 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update the comment to
Register registers a global shortcut
and remove the empty line under thefunc
declaration?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you not create a variable here and add the content of the event directly in
synchronousEvent
?Also could you remove the empty line below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update the comment to
IsRegistered checks whether a global shortcut is registered
Also could you make the same modifications I proposed for the previous function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update the comment to
Unregister unregisters a global shortcut
Also could you make the same modifications I proposed for the previous function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be instead:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah.. yes, my fault
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think cleaning the map is still a good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why I didn't clean the map is that there are extra costs to determine the corresponded shortcut to be removed from the map. For example, a user registered an event with
Ctrl+Shift+S
then unregistered withS+Shift+Ctrl
, expected that shortcut should be unregistred. ThereforeCtrl+Shift+S
should be removed from the map.A more complex case: registered with
CtrlOrCmd+X
then unregistered withCmd+X
, expected that shortcut is unregistered. ThereforeCtrlOrCmd+X
should be removed from the map.It's too complex to determine the equivalent shortcuts, the best way is to let electron decide whether the shortcuts should be registered or unregistered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm last time I used shortcuts in JS,
Ctrl+Shift+S
was different thanS+Shift+Ctrl
🤔 Therefore wanting to register a shortcutCtrl+Shift+S
and unregister it withS+Shift+Ctrl
should not work in Electron 🤔Nevertheless, I think it's not too much to ask the developer to be consistent in the accelerator he/she uses when registering/unregistering a shortcut. Also I'd rather keep the map the closest to reality.
Therefore, I'd rather see the map cleaned 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Howerver, the combination of
ctrl
+shift
+S
was unregistered successfully.Sure thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a need for this function since it's called only once and it's not that big 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you lock the mutex only to retrieve the callback but not when executing it?
Something like this: