Closed pjrobertson closed 12 years ago
pjrobertson
commented
12 years ago Nathan has replied to the issue with something we can do to test.
https://github.com/nathanday/ndhotkeyevent/issues/1#issuecomment-6386495
I'm away for the next week, perhaps @skurfer - if you get the time - you could look into it? Or I'll do it when I'm back over the weekend.
skurfer
commented
12 years ago I’m going through and [trying to] fix everything the analyzer picks up. Not sure if it’ll help with this crash, but it’s probably good to do anyway.
Should I do it against release or master? If it fixes the crash, I’d say release. If it’s just optimizations, I’d say master. But I have no idea if it’ll fix the crash. :-)
pjrobertson
commented
12 years ago I'd say do it all against release. ß69 is a general clean up release, so why not put it all in? We've only released a dev release so more changes shouldn't do any harm
On 19 June 2012 19:22, Rob McBroom < reply@reply.github.com
wrote:
I’m going through and [trying to] fix everything the analyzer picks up. Not sure if it’ll help with this crash, but it’s probably good to do anyway.
Should I do it against
releaseormaster? If it fixes the crash, I’d sayrelease. If it’s just optimizations, I’d saymaster. But I have no idea if it’ll fix the crash. :-)
Reply to this email directly or view it on GitHub: https://github.com/quicksilver/Quicksilver/issues/942#issuecomment-6433302
pjrobertson
commented
12 years ago P.S. I remember the static analyser can give false positives, so watch out. I think fheckl went through it before, and has put comments in the places where there are false positives.
On 19 June 2012 23:36, Patrick Robertson robertson.patrick@gmail.comwrote:
I'd say do it all against release. ß69 is a general clean up release, so why not put it all in? We've only released a dev release so more changes shouldn't do any harm
On 19 June 2012 19:22, Rob McBroom < reply@reply.github.com
wrote:
I’m going through and [trying to] fix everything the analyzer picks up. Not sure if it’ll help with this crash, but it’s probably good to do anyway.
Should I do it against
releaseormaster? If it fixes the crash, I’d sayrelease. If it’s just optimizations, I’d saymaster. But I have no idea if it’ll fix the crash. :-)
Reply to this email directly or view it on GitHub: https://github.com/quicksilver/Quicksilver/issues/942#issuecomment-6433302
tiennou
commented
12 years ago The crash in CFRetain makes me think something weird happens with kTISPropertyUnicodeKeyLayoutData.
From Googling around, I can find :
Documentation says that it can be NULL if the current keyboard layout has no 'uchr' info. I'd say check using the Keyboard layout specified in the 2nd link above ("Kotoeri") to see if that helps pinpointing it. Or just try :
if (keyboardLayoutData)
CFRetain(keyboardLayoutData);
else
NSLog(@"Failed to get layout data for %@", aSounce);Please check that TISInputSourceRef supports being NSLog()-ed first, this is GitHub code ;-).
pjrobertson
commented
12 years ago Great! I can reproduce the crash in QS by following the steps in http://code.google.com/p/macifom/issues/detail?id=6
Thanks Etienne! :)
Now… what to do for the users who have a 'strange' keyboard layout. Maybe Nathan can come up with some solution. I have updated the issue on his repo
nathanday
commented
12 years ago Sorry I haven't been of much help I will look at this tonight to see what the best solution is.
tiennou
commented
12 years ago No problem. I've been looking at this a little, but I think we're out of luck since everything that we could do to get to the 'KCHR' data (since "Kotoeri" only provides that) looks deprecated to me...
tiennou
commented
12 years ago Since I can't test, I share ;-). @pjrobertson Could you check what happens if you swap TISCopyCurrentKeyboardInputSource() with TISCopyCurrentKeyboardLayoutInputSource() in +keyboardLayout ?
pjrobertson
commented
12 years ago Pfffft, part timer ;-)
Seems to work. No crashes here. Although the method returns <TSMInputSource 0x108f9b070> KB Layout: U.S. (id=0) for the Romanji keyboard layout. For
'British' it returns <TSMInputSource 0x1050bac10> KB Layout: British (id=2) which seems to be right.
Also, still works to fix #626 and #807, so it looks like that might be the one! Again, good work Etienne :)
Now go enjoy yourself ;-)
On 10 July 2012 15:47, Etienne Samson < reply@reply.github.com
wrote:
Since I can't test, I share ;-). @pjrobertson Could you check what happens if you swap
TISCopyCurrentKeyboardInputSource()withTISCopyCurrentKeyboardLayoutInputSource()in+keyboardLayout?
Reply to this email directly or view it on GitHub: https://github.com/quicksilver/Quicksilver/issues/942#issuecomment-6877649
tiennou
commented
12 years ago Heh ;-). I think there's some kind of Keyboard Layout inheritance thing, so "Kotoeri" inherits the U.S. keyboard layout (which I think is normal since it hopefully uses the hardware keyboard layout (or a equivalent QWERTY one) anyway, and you can't change that easily ;-)).
nathanday
commented
12 years ago I had a solution where I was using TISCopyInputSourceForLanguage if TISCopyCurrentKeyboardInputSource failed, using languages returned from NSLocale, I was looking at TISCopyCurrentKeyboardLayoutInputSource but I don't really understand exactly what it does though it does sound more robust as it seems to try TISCopyCurrentKeyboardInputSource when appropriate.
tiennou
commented
12 years ago Yeah, I guess the doc is a little unclear there (Yay, Radar time ! Filed as rdar://11846409). I'm also wondering about TISCopyCurrentASCIICapableKeyboardLayoutInputSource() which has a specific note about "key translation" (which is the very thing we're trying to do), but also states that it doesn't support layout overrides...
Maybe the correct thing to do would be to first try TISCopyInputMethodKeyboardLayoutOverride() to get that in case there's one activated, then try TISCopyCurrentKeyboardLayoutInputSource() and then fall back to TISCopyCurrentASCIICapableKeyboardLayoutInputSource() ? I'm not sure about the order of those last two though...
nathanday
commented
12 years ago OK give me another night and I will implement something like that.
nathanday
commented
12 years ago OK, I have uploaded a new version of NDHotKeyEvents that has tiennou recommend changes.
pjrobertson
commented
12 years ago Morning Nathan,
Thanks for making the changes, they look, and work, great.
I've been scratching my head over whether passing a function that retains an object (copy) as an argument (what you have done on lines 321,323 and 325) is a memory leak or not. I'll happily follow it up on the cocoa dev lists if you like. What are your views?
tiennou
commented
12 years ago It's not a memory leak if the keyboard layout doesn't change, since kCurrentKeyboardLayout is marked static (This is known as a class variable). But there's a problem (and a leak) since the keyboard layout can (and surely will, in some cases) change. It means +keyboardLayout could run while the user has "U.S." selected, then it switches to "Dvorak", but the layout stored won't follow, and you'll get a surprise when you try e.g. the "Paste" action...
The fix is to register for the CF notifications (in +load or +initialize) that are provided by Text Input Services and release kCurrentKeyboardLayout at that time so that the next time around the layout is correct.
pjrobertson
commented
12 years ago Following on from a little bit more discussion with Etienne (@tiennou) off the record, we both agree that sending TISCopyInputMethodKeyboardLayoutOverride() as an argument to the initWithInputSource is causing a memory leak, since that is a copy method and of course follows the Create Rule. (Same for the other two TISCopyCurrentASCIICapableKeyboardLayoutInputSource() and TISCopyCurrentKeyboardLayoutInputSource()
This is a memory leak, along with the one that Etienne mentions above. I also agree that, should the user switch the keyboard layout whilst the program is launched, NDKeyboardLayout should be able to handle this, and switch its kCurrentKeyboardLayout accordingly. At the moment, if a user switches keyboard layout, it requires a relaunch of the app to register this change.
Here is a 'quick' bit of code Etienne wrote that should fix the first memory leak I mention in this comment (warning, may not work directly)
void *methods = [TISCopyInputMethodKeyboardLayoutOverride, TISCopyCurrentKeyboardLayoutInputSource, TISCopyCurrentASCIICapableKeyboardLayoutInputSource];
for (void *method = methods[0]; method++; kCurrentKeyboardLayout != nil || method > methods) {
TISInputSourceRef inputSource = method();
kCurrentKeyboardLayout = [[self alloc] initWithInputSource:inputSource];
CFRelease(inputSource);
}Seems like we're almost there an this :)
nathanday
commented
12 years ago It's not a leak since its never unreachable, though yes the TISInputSourceRef is leaked, also granted its not updating if the user changes keyboard layout. I will look into updating NDKeyboardLayout to change its TIS...InputSource when the user changes keyboard layout.
nathanday
commented
12 years ago I have made some progress on updating NDHotKeyEvents to handle changing input keyboard, I have come up against a little complexity in how I should handle changing input keyboard, also my understand of what changing input keyboard meant doesn't seem to be what it actually means. I thought a good approach would be to remove the keyCode as a permanent value and instead use character and get the keyCode only when required, but how does that work when changing from an english layout to a chinese layout, also the keyCode represents the physical key in the keyboard which doesn't change when switching from one keyboard layout to another, should hot key changes because of layout change? Also the character has some issues to work out in reference to Function Keys. I might just implement things so that hotkeys can be updated but users of the library if they want to, and so some of the issues above can be resolved depending on the application. Any thoughts would be helpful, I need to think about this more.
Seems to be causing a crash for certain users. I have filed an issue with the creator of the code: https://github.com/nathanday/ndhotkeyevent/issues/1
Crash log;