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

Key should fire continuously when key is held down #119

Closed
kasper opened this issue Jul 13, 2016 · 21 comments
Closed

Key should fire continuously when key is held down #119

kasper opened this issue Jul 13, 2016 · 21 comments
Assignees
Milestone

Comments

@kasper
Copy link
Owner

kasper commented Jul 13, 2016

Key should fire continuously when key is held down. Use [NSEvent keyRepeatInterval] as the interval.

  • Version: 2.2
  • macOS: 10.11.5
@kasper kasper added this to the 2.2.1 milestone Jul 13, 2016
@kasper kasper self-assigned this Jul 13, 2016
@mafredri
Copy link
Contributor

Should Phoenix consider the initial key repeat interval as well to better mimic keyboard behaviour?

defaults read -g InitialKeyRepeat
defaults read -g KeyRepeat

@kasper
Copy link
Owner Author

kasper commented Jul 13, 2016

@mafredri Yes, I think that is what [NSEvent keyRepeatDelay] is for.

@mafredri
Copy link
Contributor

Oh, my bad, should've checked 👍.

@kasper
Copy link
Owner Author

kasper commented Jul 13, 2016

The strategy is now:

  1. Key down, call callback
  2. Start delay timer
  3. After delay, call callback
  4. Start interval timer
  5. After interval, call callback
  6. Key up, invalidate timer

@kasper
Copy link
Owner Author

kasper commented Jul 13, 2016

You can now test this from the feature-key-repeat-branch. There’s still something essentially wrong with the timers if you try to do something weird like this:

var a = new Key('e', [ 'ctrl', 'shift' ], function () {
  Phoenix.log('A');
  b.enable();
});

var b = new Key('e', [ 'ctrl', 'shift' ], function () {
  Phoenix.log('B');
  a.enable();
});

@kasper
Copy link
Owner Author

kasper commented Jul 13, 2016

Oh and I delayed this feature so I could release 2.2.1 with the critical bug fixes. Better not to botch this up. 😏

@khalidchawtany
Copy link

It's 1:00 am and posting from bed :)
I will test the feature branch tomorrow.
Thank you vey much 👍

@khalidchawtany
Copy link

I just tried it and it is awesome :)

@kasper
Copy link
Owner Author

kasper commented Jul 14, 2016

@khalidchawtany Brilliant! I still need to figure out what is going on in the example I posted. If anyone has an idea, I’m all ears.

As key repeating is the default behaviour with system keyboard events, I think this can be implemented as the default behaviour. Some other window managers have this an optional feature, but I can’t see whether that’s needed. Everything should work as is, with the additional benefits.

If it’s critical for a key handler to only be invoked once, a user could for example throttle or debounce the event with Underscore. What do you guys think?

@mafredri
Copy link
Contributor

mafredri commented Jul 14, 2016

Seems to work as I'd expect as well!

As for your example above, what I believe is happening is:

  1. a is pressed, b enabled
  2. b starts listening for key event
  3. key event is never propagated to b since it's still in the down mode

But I have not taken a look at the code so this is just a hunch of what might be happening.

I think this can be implemented as the default behaviour.

I tend to agree, if someone is pressing down on the key for a longer period of time, I'd think they were expecting something to happen.

EDIT: I just noticed my screen brightness adjustment code started working perfectly with this branch, that's so awesome!

@khalidchawtany
Copy link

@kasper @mafredri I use Karabiner to set my key repeat rate beyond what OS X allows. This karabiner tweak is transparent to all apps in my OS. However, I am noticing a rather slower repeat rate when using Phoenix. Could this be because of window movement functions blocking? Or Phoenix asks the system for a key repeat rate and the default (Not tweaked) is passed back.

@mafredri
Copy link
Contributor

I believe kasper might be better equipped to answer this, but have you tried wrapping your code in a setTimeout? E.g:

var a = new Key('e', ['ctrl', 'shift'], function() {
    var win = Window.active();
    setTimeout(function() {
        // do stuff to win...
    }, 0);
});

@kasper
Copy link
Owner Author

kasper commented Jul 14, 2016

@khalidchawtany @mafredri It should use the system setting (which comes from the macOS preferences). However, I think it’s safe to assume there’s some latency related to this. All the Accessibility API calls are executed on the main thread.

@khalidchawtany
Copy link

@mafredri I tried your suggestion and the results were identical. Thanks.
@kasper Thanks for implementing this so quickly 😍

@mafredri
Copy link
Contributor

mafredri commented Jul 14, 2016

It it possible to get information in the callback whether or not this is the initial key press or a repeat key press? This would make it quite simple to implement any kind of logic where the initial keypress could e.g. perform a special action, or maybe zero a counter and you could keep track of how many key presses have been done, etc. Just a thought I had.

@kasper
Copy link
Owner Author

kasper commented Jul 14, 2016

@mafredri An interesting idea and definitely possible. I’ll have to think about it.

@kasper
Copy link
Owner Author

kasper commented Jul 30, 2016

Ok, now the callback will get an additional boolean argument that indicates whether the key is repeated. @mafredri @khalidchawtany Would you be able to test this change? The latest commit should also fix the scenario I described.

Regarding the latency, it’s probably best to keep the callbacks synchronous to each other. I can only see minimal changes with the setTimeout-trick. Obviously it’s rather dependant on what you actually want to do.

This makes me wonder, because a timer is a rather nice way to execute code asynchronously — maybe we should have syntactic sugar for this: async(function () {}) -> setTimeout(function () {}, 0).

@mafredri
Copy link
Contributor

Thanks, the repeated pram is working well on my end at least!

Regarding the async() sugar, I think there could be a tiny risk mixing it with async/await that might possibly be included in a future ES-standard. Also, it does not save a lot of keystrokes, but I agree that it's intention is a lot clearer. It's easy enough to implement yourself, but could also be part of core. I don't have any strong opinions on it either way, just presenting my thoughts on the matter :).

PS. Possible bug I've noticed, running the key-repeat branch for a longer time, is that sometimes, very rarely, the key gets stuck in repeat-mode. In this case it will keep repeating until I press the key combination again. Another detail is that I've mostly just noticed it with my keybinding of § (without any modifiers). Not sure if that's the reason or if it's simply that § performs a toggle action which is very noticeable, also I use it a lot!

@kasper
Copy link
Owner Author

kasper commented Jul 30, 2016

@mafredri Yes, I think we’ll leave asynchronous execution up to the user.

There was an issue with the previous implementation that might have contributed to what you described. Let me know if it still happens.

@kasper
Copy link
Owner Author

kasper commented Jul 31, 2016

Ok, last iteration and merged to master. Everything should be working as expected. (Fingers crossed.) Please test once more. This feature will be released in 2.3.

@kasper kasper closed this as completed Jul 31, 2016
@mafredri
Copy link
Contributor

Started using it now, so far so good! I'll report back if I notice anything out of the ordinary.

Cheers 🍻!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants