<AWT Dev> [9] Review Request: 8143054 [macosx] KeyEvent modifiers do not contain information about mouse buttons

[Semyon Sadetsky]

On 12/23/2015 7:33 PM, Sergey Bylokhov wrote:
> Hi, Semyon.
> Thanks for review, see my comments inline.
>
> On 23/12/15 18:37, Semyon Sadetsky wrote:
>> 1. That would be nice to test that modifiers are exactly as expected to
>> ensure that no extra modifier bits are set.
>
> Yes, but according to the spec it is not guaranteed that some other 
> modifiers are set or not, so I just check the modifiers which must be 
> set in this case only.
But I would interpret that as an issue if button 1 is pressed but the 
modifiers contains button 2 or 3 as well for example. Since there 
complex mapping between modifiers this risk takes place. It is a very 
tiny improvement for your test. Please...
>
>> 2. Test summary and author are omitted
>
> I am not sure what I can add to the summary, other than information 
> which already existed in the bug description. The author tag is 
> undesired in the openjdk and its usage is not strictly necessary.
We use to add those fields usually but I don't see any obstacles to make 
the test anonymous. :)
>> 3. In the test you are creating a new window for  each button. That is
>> not optimal for test execution time.
>
> It is done intentionally to skip any side effects caused by the 
> previous keyup/keydown, so each time it is new frame, texteditor, 
> listeners etc.
Hmm... What side effects you'd expect after a single key press?
>
>> 4. Test fails on Linux, but you've filed a separate bug already. That
>> should be ok.
>>
>> --Semyon
>>
>> On 11/18/2015 6:50 PM, Sergey Bylokhov wrote:
>>> Hello.
>>> Please review the fix for jdk9.
>>>
>>> On macosx when we create a KeyEvent we ignore the mouse state of all
>>> mouse buttons, which means that mouse modifiers are missing.
>>> The only KeyEvent is affected, because when we convert NS modifiers to
>>> java modifiers we use nsToJavaKeyModifiers(), but in all other cases
>>> we use nsToJavaMouseModifiers() which is the same but adds mouse
>>> modifiers.
>>>
>>> In the fix:
>>> - The button parameter of nsToJavaMouseModifiers() was removed because
>>> it was unused.
>>> - nsToJavaMouseModifiers() was renamed to nsToJavaModifiers() and now
>>> is used when keyEvent is generated.
>>> - nsToJavaKeyModifiers() was removed because it unused now.
>>>
>>> The similar bug for extended(id>=3) mouse buttons was filed for linux:
>>> https://bugs.openjdk.java.net/browse/JDK-8143240
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8143054
>>> Webrev can be found at:
>>> http://cr.openjdk.java.net/~serb/8143054/webrev.00
>>>
>>
>
>