Implement global shortcuts #393

benebsiny · 2023-06-18T10:51:38Z

Solved #392

This PR implements globalShortcut like electron.

4 methods are implemented: register, isRegistered, unregister, unregisterAll.

I was going to implement these methods as static methods of GlobalShortcut struct. Unfortunately, Go doesn't support static method. so an alternative way is implemented. See follow examples:

// Register
astilectron.GlobalShortcutRegister("shift+1", func() {
    fmt.Println("shift+1")
})

// Is registered
astilectron.GlobalShortcutIsRegistered("shift+1")  // true

// Unregister
astilectron.GlobalShortcutUnregister("shift+1")

// Unregister all
astilectron.GlobalShortcutUnregisterAll()

TODO

The relevant event will no longer be triggered after the shortcut is unregistered. If the same key is registered again, the original callback will be replaced with new one. Therefore, we don't need to find the registered key from the map. Besides, we don't need to parse the accelerator, too. An error will be returned if the method failed.

1. Add test case 2. Add doc to README.md 3. Handle `ctx.Err` in functions

asticode

Thanks for the PR.

Before diving into the details, let's reorganize your changes a bit 👍

asticode · 2023-08-17T07:58:27Z

event.go

@@ -29,6 +29,7 @@ type Event struct {
 	Displays            *EventDisplays       `json:"displays,omitempty"`
 	Enable              *bool                `json:"enable,omitempty"`
 	FilePath            string               `json:"filePath,omitempty"`
+	GlobalShortcut      *GlobalShortcut      `json:"globalShortcut,omitempty"`


Instead of using GlobalShortcut directly here, could you:

create a specific EventGlobalShortcuts struct containing json fields only?

remove json fields from GlobalShortcuts

make sure you add an s at the end of all global shortcut occurences

asticode · 2023-08-17T08:07:41Z

astilectron.go

@@ -187,6 +187,10 @@ func (a *Astilectron) Start() (err error) {
 	} else {
 		synchronousFunc(a.worker.Context(), a, nil, "app.event.ready")
 	}
+
+	// Initialize global Shortcuts
+	InitGlobalShortcuts(a.worker.Context(), a.dispatcher, a.identifier, a.writer)


Instead of making the Global Shortcut methods/variables global to the package, could you:

create a GlobalShortcuts struct that will contain the *object and registered callbacks

add a globalShortcuts *GlobalShortcuts variable to the Astilectron struct

add a newGlobalShortcuts() method that creates a *GlobalShortcuts (and does the other instantiating stuff that your InitGlobalShortcuts() does) and call this method after creating the dock

add a GlobalShortcuts() method to the Astilectron struct that will return a.globalShortcuts

add Register(), IsRegistered(), Unregister() and UnregisterAll() methods to the GlobalShortcuts struct

benebsiny · 2023-09-02T04:19:51Z

Thanks for your suggestion! I've followed your instructions to make some changes, have a look at it.

asticode

I've not finished reviewing all the PR but I'll continue once the requested changes will be made 👍

asticode · 2023-09-03T10:30:25Z

global_shortcut.go

@@ -0,0 +1,123 @@
+package astilectron


Could you rename the file to global_shortcuts.go?

asticode · 2023-09-03T10:31:00Z

global_shortcut.go

+import "context"
+
+const (
+	EventNameGlobalShortcutCmdRegister          = "global.shortcuts.event.registered"


Could you make sure the events here are the same as in Javascript?

asticode · 2023-09-03T10:31:18Z

global_shortcut.go

+	EventNameGlobalShortcutEventTriggered       = "global.shortcuts.event.triggered"
+)
+
+type callback func()


Could you rename this type to globalShortcutsCallback?

asticode · 2023-09-03T10:31:52Z

global_shortcut.go

+// GlobalShortcuts represents a global shortcut
+type GlobalShortcuts struct {
+	*object
+	callbacks map[string]*callback // Store all registered Global Shortcuts


You will need to add a *sync.Mutex as well and use it anywhere you read/write in the map

asticode · 2023-09-03T10:33:29Z

global_shortcut.go

+
+func newGlobalShortcuts(ctx context.Context, d *dispatcher, i *identifier, w *writer) (gs *GlobalShortcuts) {
+	var obj = newObject(ctx, d, i, w, i.new())
+	gs = &GlobalShortcuts{object: obj, callbacks: make(map[string]*callback)}


Could you avoid creating the var obj and instead gs = &GlobalShortcuts{object: newObject(ctx, d, i, w, i.new()), ...} and below gs.On(...)?

asticode · 2023-09-03T10:35:27Z

global_shortcut.go

+}
+
+// globalShortcutHandler Handle the GlobalShortcuts event triggered from astilectron
+func globalShortcutHandler(gs *GlobalShortcuts, accelerator string) {


Could you make this function part of the GlobalShortcuts struct instead and rename it to execCallback:

func (gs *GlobalShortcuts) execCallback(accelerator string) { // TODO Lock mutex // Exec callback }

benebsiny · 2023-09-04T09:04:11Z

Changes have been made, have a look at it.

roromix · 2023-12-12T09:57:54Z

This is an interesting feature. What is missing to merge and tag it?

…E-392

README.md

asticode · 2023-12-14T08:17:36Z

astilectron.go

@@ -187,6 +192,7 @@ func (a *Astilectron) Start() (err error) {
 	} else {
 		synchronousFunc(a.worker.Context(), a, nil, "app.event.ready")
 	}
+


Could your remove this empty line ?

asticode · 2023-12-14T08:19:44Z

global_shortcuts.go

+
+const (
+	EventNameGlobalShortcutsCmdRegister          = "global.shortcuts.cmd.register"
+	EventNameGlobalShortcutsCmdIsRegistered      = "global.shortcuts.cmd.is.register"


Could you add the missing ed at the end of global.shortcuts.cmd.is.register?

asticode · 2023-12-14T08:20:24Z

global_shortcuts.go

+
+type globalShortcutsCallback func()
+
+// GlobalShortcuts represents a global shortcut


Could you replace the comment with // GlobalShortcuts represents global shortcuts?

asticode · 2023-12-14T08:20:52Z

global_shortcuts.go

+type GlobalShortcuts struct {
+	*object
+	m         *sync.Mutex
+	callbacks map[string]*globalShortcutsCallback // Store all registered Global Shortcuts


Could you remove the comment here?

global_shortcuts.go

asticode · 2023-12-14T08:32:50Z

global_shortcuts.go

+	return
+}
+
+// IsRegistered Check if a global shortcut is registered


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?

asticode · 2023-12-14T08:33:30Z

global_shortcuts.go

+	return
+}
+
+// Unregister Unregister a global shortcut


Could you update the comment to Unregister unregisters a global shortcut

Also could you make the same modifications I proposed for the previous function?

asticode · 2023-12-14T08:33:58Z

global_shortcuts.go

+		return
+	}
+
+	// No need to find the callback from the map and delete it


I do think cleaning the map is still a good idea

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 with S+Shift+Ctrl, expected that shortcut should be unregistred. Therefore Ctrl+Shift+S should be removed from the map.
A more complex case: registered with CtrlOrCmd+X then unregistered with Cmd+X, expected that shortcut is unregistered. Therefore CtrlOrCmd+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.

Mmm last time I used shortcuts in JS, Ctrl+Shift+S was different than S+Shift+Ctrl 🤔 Therefore wanting to register a shortcut Ctrl+Shift+S and unregister it with S+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 👍

Mmm last time I used shortcuts in JS, Ctrl+Shift+S was different than S+Shift+Ctrl 🤔 Therefore wanting to register a shortcut Ctrl+Shift+S and unregister it with S+Shift+Ctrl should not work in Electron 🤔

Howerver, the combination of ctrl+shift+S was unregistered successfully.

Therefore, I'd rather see the map cleaned 👍

Sure thing

global_shortcuts.go

asticode · 2023-12-14T08:36:01Z

That PR got lost in my notifications, my apologies. I've reviewed it and made comments 👍

benebsiny · 2023-12-14T15:49:24Z

@asticode Hi, I've followed suggestions to make changes, have a look at it. astilectron is also updated!

asticode

Thanks for the last changes, there are 2 remaining points 👍

I'll also check the JS PR 👍

asticode · 2023-12-15T08:18:37Z

global_shortcuts.go

+	m *sync.Mutex
+}
+
+var callbacks map[string]*globalShortcutsCallback


Could you not use a pointer to globalShortcutsCallback in the map?

asticode · 2023-12-15T08:22:44Z

global_shortcuts.go

+		return
+	}
+
+	// No need to find the callback from the map and delete it


Mmm last time I used shortcuts in JS, Ctrl+Shift+S was different than S+Shift+Ctrl 🤔 Therefore wanting to register a shortcut Ctrl+Shift+S and unregister it with S+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 👍

benebsiny · 2023-12-16T04:21:01Z

@asticode Hi, I've updated, have a look at it 👍

asticode · 2023-12-19T08:07:18Z

global_shortcuts.go

+	m *sync.Mutex
+}
+
+var callbacks map[string]globalShortcutsCallback


I missed it in my previous reviews, but could you move callbacks to the GlobalShortcuts struct and make sure you initialize it with make(map[string]globalShortcutsCallback) in newGlobalShortcuts?

asticode · 2023-12-19T08:09:02Z

global_shortcuts.go

+
+	// Send an event to astilectron to unregister the global shortcut
+	_, err = synchronousEvent(gs.ctx, gs, gs.w, Event{Name: EventNameGlobalShortcutsCmdUnregister, TargetID: gs.id, GlobalShortcuts: &EventGlobalShortcuts{Accelerator: accelerator}}, EventNameGlobalShortcutsEventUnregistered)
+	if err != nil {


Shouldn't it be instead:

if err != nil { return } gs.m.Lock() delete(callbacks, accelerator) gs.m.Unlock()

Ah.. yes, my fault

benebsiny · 2023-12-21T07:12:24Z

@asticode I've updated, have a look at the latest changes

asticode

I've merged the JS PR but we need to update the astilectron here as well.

Also I've found a minor issue to fix 👍

asticode · 2023-12-21T08:06:19Z

global_shortcuts.go

+		}
+		return
+	})
+	gs.callbacks = make(map[string]globalShortcutsCallback)


Could you remove this line?

asticode · 2023-12-21T08:06:45Z

astilectron.go

 	"net"
 	"os/exec"
 	"runtime"
 	"time"
-
-	"github.com/asticode/go-astikit"
 )

 // Versions


Could you update DefaultVersionAstilectron to v0.58.0 below?

benebsiny · 2023-12-21T08:51:17Z

@asticode Hi, minor issues are fixed, have a look at it

asticode · 2023-12-21T13:16:26Z

Thanks for sticking with this PR! ❤️

Implement global shortcuts

1afa8f5

benebsiny mentioned this pull request Jun 18, 2023

Implement global shortcuts asticode/astilectron#66

Merged

benebsiny added 2 commits June 19, 2023 22:06

Add test case and doc

2d21475

1. Add test case 2. Add doc to README.md 3. Handle `ctx.Err` in functions

benebsiny marked this pull request as ready for review June 20, 2023 05:30

asticode force-pushed the master branch from b331d89 to ba5c286 Compare July 27, 2023 09:39

asticode force-pushed the master branch from ba5c286 to e849f62 Compare August 8, 2023 09:00

asticode suggested changes Aug 17, 2023

View reviewed changes

Updated

7773575

asticode suggested changes Sep 3, 2023

View reviewed changes

Updated

75762cc

Merge branch 'master' of github.com:asticode/go-astilectron into ISSU…

bff74b8

…E-392

asticode suggested changes Dec 14, 2023

View reviewed changes

Updated

a47d53a

asticode suggested changes Dec 15, 2023

View reviewed changes

Updated

35cffd4

asticode suggested changes Dec 19, 2023

View reviewed changes

Updated

6b0bc39

asticode suggested changes Dec 21, 2023

View reviewed changes

Updated

985de79

asticode merged commit e91cd32 into asticode:master Dec 21, 2023


		type globalShortcutsCallback func()

		// GlobalShortcuts represents a global shortcut

Implement global shortcuts #393

Implement global shortcuts #393

Conversation

benebsiny commented Jun 18, 2023 • edited

TODO

asticode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benebsiny commented Sep 2, 2023

asticode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benebsiny commented Sep 4, 2023

roromix commented Dec 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benebsiny Dec 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asticode commented Dec 14, 2023

benebsiny commented Dec 14, 2023

asticode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benebsiny commented Dec 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benebsiny commented Dec 21, 2023

asticode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benebsiny commented Dec 21, 2023

asticode commented Dec 21, 2023

benebsiny commented Jun 18, 2023 •

edited

benebsiny Dec 14, 2023 •

edited