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

[Anthony Petrov]

+1

--
best regards,
Anthony

On 10/21/2013 12:36 PM, Sergey Bylokhov wrote:
> Hi, Leonid.
> Thie fix looks good. Let's hope that in this area we will have no more bugs.
>
> On 21.10.2013 15:26, Leonid Romanov wrote:
>> Here is an updated version of the fix with the expanded comment:
>>
>> http://cr.openjdk.java.net/~leonidr/8020209/webrev.03/
>> <http://cr.openjdk.java.net/%7Eleonidr/8020209/webrev.03/>
>>
>> On 17.10.2013, at 21:48, Anthony Petrov <anthony.petrov at oracle.com
>> <mailto:anthony.petrov at oracle.com>> 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
>>>> <anthony.petrov at oracle.com <mailto: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.
>>>>>>
>>>>>>
>>>>>>
>>>>
>>
>
>
> --
> Best regards, Sergey.
>