Here is an updated version of the fix with the expanded comment: http://cr.openjdk.java.net/~leonidr/8020209/webrev.03/
On 17.10.2013, at 21:48, Anthony Petrov <[email protected]> wrote: > Thanks for the clarifications. They sound good to me. > > Regarding NSApp instance, your understanding is correct. Although I'm not > sure if this is a good idea to tell embedders to fix their code in this case. > But I'm sort of OK with the current approach for now. > > So, I'd like to see the final version of the fix with an updated comment, and > then I approve it. > > -- > best regards, > Anthony > > On 10/16/2013 11:19 PM, Leonid Romanov wrote: >> >> On Oct 16, 2013, at 9:37 PM, Anthony Petrov <[email protected]> >> wrote: >> >>> Hi Leonid, >>> >>> The problem with overriding NSApplication -sendEvent: is that you can't be >>> sure AWT is running with the NSApplicationAWT instance. If using SWT, FX, >>> or otherwise embedding AWT into another application, the NSApp will point >>> to an instance of another application class, and your sendEvent: will never >>> be called. I'd suggest to avoid using this method altogether if possible. >> >> Do I understand you correctly that in case of AWT embedding NSApp could be >> some NSApplication subclass other than NSApplicationAWT? If so, then this >> hypothetical subclass might have the very same code I've added to >> NSApplicationAWT since it's the most common approach for solving the problem >> of missing NSKeyUp events. In this case, our attempt to solve it in some >> other way will only make things worse: we might end up with duplicate >> NSKeyUp events. >> So, my suggestions is to leave that code as it is and if embedders complain >> about aforementioned problem, tell them to fix it on theirs app level, as >> we've done it in our NSApplicationAWT. >> >>> >>> I see that webrev.01 also includes the changes in NSApplicationAWT.m. Is >>> this really necessary for that version of the fix? >>> >>> I recall in your original review request you stated that the code currently >>> consists of multiple workarounds, and adding another one could just bring >>> more regressions or unexpected behaviors. So I'd actually prefer the >>> version .01. >> >> So do I. There are circumstances beyond my control that prevent me from >> landing the fix into JDK 8 GA. However, I still see .01 as the definitive >> version of the fix, and planning to include it into JDK 8 Update 1/JDK 9 >> (I'll file a separate bug for it). Version .02 is a not a long term solution >> for me. >> >> >>> >>> Anyway, here's a couple of comments regarding the new fix: >>> >>> src/macosx/native/sun/awt/AWTView.m >>>> 313 NSUInteger modFlags = [event modifierFlags] & >>>> 314 (NSCommandKeyMask | NSAlternateKeyMask | NSShiftKeyMask | >>>> NSControlKeyMask); >>>> 315 if (modFlags == NSCommandKeyMask) { >>> >>> Do I understand correctly that OS X is fine with e.g. Shift+Cmd+'+', and >>> only Cmd+'+' is causing a problem? >> >> Yep. >> >>> >>>> 311 // Workaround for 8020209: special case for "Cmd =" and "Cmd ." >>>> 312 // because Cocoa calls performKeyEquivalent twice for these >>>> keystrokes >>> >>> Interesting that you say that Cocoa sends multiple events and I'd guess one >>> wants to filter some events out. However, at line 320 you call >>> performKeyEquivalent yourself. Is this like the third call or something? Or >>> the comment seems to be misleading otherwise. >> >> Ok, looks like I need to extend the comment. Cocoa calls >> -performKeyEquivalent twice only if we return NO from the first >> -performKeyEquivalent call. Normally, what happens when performKeyEquivalent >> returns NO is that Cocoa calls -performKeyEquivalent for the next responder >> in the responders chain, until it finds the one which would return YES. "Cmd >> =" and "Cmd ." key strokes, however, are really unusual: if we and all other >> responders in the chain return NO, then Cocoa will construct another NSEvent >> and call -performKeyEquivalent for that event. Returning YES from the >> -performKeyEquivalent prevents that. However, this means that Cocoa won't >> call -performKeyEquivalent for the menu bar, so we have to do it manually. >> >>> >>> -- >>> best regards, >>> Anthony >>> >>> On 10/16/2013 08:53 PM, Leonid Romanov wrote: >>>> Hello, >>>> This is plan B version of the fix for JDK-8020209: [macosx] Mac OS X key >>>> event confusion for "COMMAND PLUS". The previous, proper version of the >>>> fix has been reviewed here: >>>> http://mail.openjdk.java.net/pipermail/awt-dev/2013-September/005441.html >>>> Unfortunately, I can't proceed with that version because there are some >>>> difficulties with submitting private JDK 8 build to Apple for approval. >>>> Since we are short on time and I want to fix this bug in JDK 8, I've had >>>> to use a workaround. >>>> >>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8020209 >>>> webrev: http://cr.openjdk.java.net/~leonidr/8020209/webrev.02/ >>>> >>>> Thanks, >>>> Leonid. >>>> >>>> >>>> >>
