-
Notifications
You must be signed in to change notification settings - Fork 286
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
NDHotKeyEvent fixes #1147
NDHotKeyEvent fixes #1147
Conversation
Looks good and works fine for me. Btw nice work on getting it all to work with 32/64 bit QS. I scratched my head for ages over it :( |
One small quirk I have found with this is that the ␣ character no longer displays. |
@pjrobertson: Better ? I won't be able to get more key recognized without access to a more complete keyboard (or someone who can get Key Codes) because I miss the kVK_ codes for things like Insert, Home, End, ... |
Haven't looked at the code (time for bed!) On 14 November 2012 22:49, Etienne Samson [email protected] wrote:
|
I didn't even think about it ;-). I spent some time yesterday scouring the Unicode Character palette to find symbols for those missing keys. But I just checked, the Keyboard Viewer has the same layout than my MacBook Pro, hence I'm still stuck... |
:( OK, I'll post the key chars for all the extra keys now :) On 15 November 2012 16:18, Etienne Samson [email protected] wrote:
|
Here ya go:
|
Btw just incase you still want to get key codes, and don't know about this: On 15 November 2012 16:21, Patrick Robertson [email protected]:
|
@tiennou, any progress on this? Having a clearing out day :) |
Looks good to me, it fixes both issues above (#1386 because @pjrobertson A 7 month-old pull request coming back to haunt us, I'd say ;-). I'm not sure about the "display" keycode things. I fixed those I could, but either I still miss those keys on my keyboard (though I can now borrow my work one to test), or I can't find their |
It seems to break command encapsulation... Investigating ;-). |
No, it's probably easier for everyone that way.
OK, then I won't pull it yet. |
How's this looking @tiennou - did the investigating lead anywhere? ;-) |
It's hard. Using the newer NDHotKeyEvent stuff breaks shortcuts without 'characters' (a-zA-Z0-9) like Tab and Esc, but it fixes keyboard layout changes. Difference between our (old ?) version and the current one is that the latter NDHotKeyEvent class delegates keyboard layouts to (you've guessed it ;-)) NDKeyboardLayout and only keeps track of key "characters" (versus "keycodes"). There are two things I've tried :
I don't really have time to take a closer look, sorry... If I find some time I will (it's pretty high in my priority list), but at the moment I'm pretty busy on realer life ;-). |
Do you not use QS in the most real of real lives? ;-) hehe Bonsoir On 24 May 2013 00:44, Etienne Samson [email protected] wrote:
|
OK so turns out that my ramblings about fixing the keyboard stuff on the gGroups was unwarranted, since it's related to what @tiennou is already doing/done with 1f8b7d0be811ab33a5a84fb120db42d58fa38dcc So... can we either just merge / cherry pick that one commit, or get this wrapped up (9 months already! :o) |
Sorry, I'm still time-constrained. And there's still the issue I explained above : either you get nice symbols for "silent" keys (Cmd, Ctrl, ...) but you loose most key bindings, or you get keybindings but triggers like Cmd-Esc display and work as Cmd-Q. The easiest way would be to fix the keybinding problem, but I couldn't understand why it didn't work in the first place... |
Gnnnhhhnn, I got it... This requires quicksilver/ndhotkeyevent#1 to be merged first so it's equivalent in functionality to what we have now. Now I'm gonna wash myself in shame for not taking the time for such an easy fix ;-). |
This closes #1482 btw 🔥. |
No! Go wash yourself to get rid of that shame. No need! :) P.S. I think fixing #1482 is going to annoy me ;-) |
Yeah, I see what you mean, this keyboard layout stuff is hard to get right... I still have a radar filed for clarification of the difference between Anyways I don't think it will kill your triggers, just they won't respond anymore (unless something I don't expect happens — maybe a crash in |
On Mon, Aug 19, 2013 at 7:24 PM, Patrick Robertson <[email protected]
Another thing for me that would make it less annoying is if the key |
I guess partly. But because the keyboard layouts other than QWERTY that I'm using don't have any of the same characters (this and this) there's no way for me to think in terms of keys ;-) @tiennou, I'll take a look in the next day or so and report back :) On 20 Awst 2013, at 23:08, Tim Visher [email protected] wrote:
|
On Tue, Aug 20, 2013 at 10:44 AM, Patrick Robertson <
For english keyboards it does seem to move the mappings. I haven't tried |
Alright, so maybe we should do the same? On 21 Awst 2013, at 00:08, Tim Visher [email protected] wrote:
|
I would generally agree that for non-latin layouts the position of the No idea how difficult that would be to implement though. On Tue, Aug 20, 2013 at 11:21 AM, Patrick Robertson <
|
Ready to merge ! Unless @pjrobertson lost all its triggers ;-). |
As there are no place in the API where you can be sure to be called on trigger deserialization, let's just pretend one of those will always be called.
The new NDHotKeyEvent binds its registration to its lifetime. Since we're just creating instance and throwing them away (see -enableTrigger: and -descriptionForTrigger:), it can happen that the retain count gets to 0, and those autoreleased `NDHotKeyEvent` deallocs themselves, taking their registration with them. This fixes a bug (introduced in this branch) where opening the Trigger preferences would deactivate all triggers.
74849ff
to
b5992d4
Compare
This imports NDHotKeyEvent as a submodule and fixes #1134 by listening for the correct notification.
See nathanday/ndhotkeyevent#2 for the NDHotKeyEvent diff, since it's now separate.