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

[Sergey Bylokhov]

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.

> 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.

> 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.

> 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
>>
>


-- 
Best regards, Sergey.