soffes / HotKey

Simple global shortcuts in macOS
MIT License
921 stars 82 forks source link

Latest version need to re-register handler in every event #16

Open aVolpe opened 4 years ago

aVolpe commented 4 years ago

With the version 0.1.3 I need to re-register the key handler every time the handler is fired, for example, in the version 0.1.2 this code works:

    setupGlobalKeybindings() {
        let hotKey = HotKey(key: .r, modifiers: [.command, .option])
        hotKey.keyDownHandler = {
            self.handleKeyDown(hotkey: hotKey)
        }
        print("Keybindings initialized")
    }

    func handleKeyDown(hotkey: HotKey) {
        print("Pressed at \(Date())")
    }

But in the version 0.1.3 I need to re-register the handler, like this:

    func handleKeyDown(hotkey: HotKey) {

        print("Pressed at \(Date())")
        // I really don't know why I must have to do this, but it's the only
        // way that it works
        hotkey.keyDownHandler = {
            self.handleKeyDown(hotkey: hotkey)
        }
    }
jdisho commented 4 years ago

@aVolpe I ended up creating something like this

private extension HotKey {
    func handleKeyDown(_ handler: @escaping (() -> Void)) {
        keyDownHandler = {
            handler()
            self.handleKeyDown(handler)
        }
    }
}
tlk commented 4 years ago

Hello @aVolpe, have you considered storing the hotkey object on an instance variable of an object that is guaranteed to exist during the entire app lifecycle? I believe that would solve the issue.

My thoughts:

aVolpe commented 4 years ago

@tlk thats sound right, but it doesn't explain why it works in the version 0.1.2, all I can see in the log is that various methods became public and a new extension to KeyCombo (Sources/HotKey/KeyCombo+System.swift) was added.

tlk commented 4 years ago

@aVolpe Good point :-)

In order to reproduce the issue I created a new project with Xcode 11.3 (macOS, App, Swift, Storyboard, target 10.15), added HotKey as a SPM dependency and added the setupGlobalKeybindings & handleKeyDown code from above. The hotkey works repeatedly and I cannot seem to reproduce the error as reported.

Could the issue be caused by other factors than the library?

harryworld commented 4 years ago

Able to reproduce in Catalyst-based Plugin, but it works on a simple Cocoa-based demo app. Really weird.

tlk commented 4 years ago

@harryworld would you be able to share a small code sample - as small and simple as possible - that reproduces the issue?

harryworld commented 4 years ago

I have created a repo for this https://github.com/harryworld/TestCatalyst

tlk commented 4 years ago

For some reason Catalyst.appKit is null when I run the TestCatalyst project and setup() is never called. Would you be able to simplify the example even further?

Did the Expected code block work with HotKey v0.1.2?

aVolpe commented 4 years ago

@tlk sorry for the late response.

I can't reproduce the issue with the latest version of xcode with Hotkey 0.1.3.

harryworld commented 4 years ago

@tlk this the simplest setup to run Cocoa code in Catalyst app... Also, are you running the target destination on Mac? Anyway, no big deal. I can still use @aVolpe hack in the beginning. Now, I'm just curious if @aVolpe has resolved the issue and how?

Side note In my sample project, it is working because I'm using the suggested hack. Please uncomment and replace with Expected Code to reproduce the issue.

aVolpe commented 4 years ago

@harryworld I try to reproduce the issue like the first time, creating a new project, adding the deps using swift packages, and then registering the key, but this time it's working.

tlk commented 4 years ago

@aVolpe No worries, thanks! Interesting.

@harryworld Ok. Yes I ran the TestCatalyst project on macOS Catalina.

Reading the code in MacApp.swift I see the hotKey instance variable on the MacApp class. Looking at Catalyst.swift and FirstViewController.swift the MacApp instance is only requested once (in FirstViewController.swift) and not stored anywhere.

Based on that I would expect the MacApp instance to be deinitialized when FirstViewController.viewDidLoad() is completed unless there are any references to the MacApp instance or the hotKey instance. Consequently, the hotKey instance would also be deinitialized and therefore disabled.

In your "Actual" code block the hotKey instance is captured by the keyDownHandler closure because it is referenced from self.handleKeyDown(hotkey: hotKey). This means that it is somewhat self-referencing, preventing it from being deinitialized, and I believe this is the reason why it works.

Out of curiosity, here is a slightly modified version of your "Actual" code block where the hotKey reference is not captured by the keyDownHandler closure:

// PLEASE DO NOT USE THIS
// Should not be necessary to re-register the keyDownHandler

class MacApp: NSObject, AppKit {
    var hotKey: HotKey?

    func setup() {
        let hotKey = HotKey(key: .a, modifiers: [.control, .shift])
        hotKey.keyDownHandler = {
            self.handleKeyDown()
        }
        self.hotKey = hotKey
    }

    func handleKeyDown() {
        print("Pressed at \(Date())")
        key.keyDownHandler = {
            self.handleKeyDown()
        }
    }
}

My guess is that this slightly modified version does not work unless you keep a reference to the MacApp instance stored somewhere during the entire app lifecycle.

If this observation is correct another (better?) solution for you would be to store a reference to the MacApp instance, fx as an instance variable on the FirstViewController. Then your "Expected" code block as well as the code above should work.