<AWT Dev> [8] Review request for JDK-8020209: [macosx] Mac OS X key event confusion for "COMMAND PLUS" (plan B)

[Leonid Romanov]

On Oct 16, 2013, at 9:37 PM, Anthony Petrov <anthony.petrov at oracle.com> 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.
>> 
>> 
>>