<AWT Dev> Review request for 6191390: Action Event triggered by list does not reflect the modifiers properly on win32

[Sergey Bylokhov]

The fix looks fine to me.

On 31.03.16 13:30, Ambarish Rapte wrote:
> Hi Sergey,
>
> 	Thanks for the review.
> 	There are three more components in addition to List which require similar change to List.
> 	Components are,	Button, TrayIcon, MenuItem.
>
> 	Have updated the webrev.02 with related changes,
> 	Changes:
> 	1.	Windows 	: Button, TrayIcon, MenuItem will call AwtComponent::GetActionModifiers() instead of AwtComponent::GetJavaModifiers() when creating ActionEvent
>
> 	2.	Tests		: Added a ActionEventTest for each component
> 				: The new Tests & JCK test Pass with the patch.
>
> 	3.	Unix   	: MenuItem ActionEvent was created with no modifiers set. Hence a Unix fix for MenuItem is provided in this patch.
> 				: MenuBar/ActionEventTest.java  test fails before fix and passes after on Ubuntu64.
> 				: Other three tests pass with or without this patch.
>
>
> 	Please review the updated webrev.02
> 	http://cr.openjdk.java.net/~arapte/6191390/webrev.02/
>
> Thanks,
> Ambarish
>
>
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Tuesday, March 29, 2016 5:37 PM
> To: Ambarish Rapte; Semyon Sadetsky; Prasanta Sadhukhan; awt-dev at openjdk.java.net
> Subject: Re: Review request for 6191390: Action Event triggered by list does not reflect the modifiers properly on win32
>
> Can you please confirm that this is the only place where we create ActionEvent w/o modifiers? What about awt_Button.cpp and others?
>
> On 28.03.16 17:56, Ambarish Rapte wrote:
>> Hi Sergey,
>>
>> 	Thanks for the review comments,
>>
>> 	As you pointed out,
>> 	1. ALT_GRAPH_MASK is not needed.
>> 	2. Should use ActionEvent masks, instead of InputEvent.
>> 	
>> 	=> Both the above are corrected in webrev.01,
>> 	http://cr.openjdk.java.net/~arapte/6191390/webrev.01/
>>
>> 	Please review the updated webrev.01
>>
>> Regards,
>> Ambarish
>> 	
>>
>> -----Original Message-----
>> From: Sergey Bylokhov
>> Sent: Friday, March 25, 2016 3:48 AM
>> To: Ambarish Rapte; Semyon Sadetsky; Prasanta Sadhukhan;
>> awt-dev at openjdk.java.net
>> Subject: Re: Review request for 6191390: Action Event triggered by
>> list does not reflect the modifiers properly on win32
>>
>> Hi, Ambarish.
>> You add a correct comment to the method:
>> "modifiers provided by ActionEvent class should be set"
>>
>> But the method itself sets the modifiers from the InputEvent. Yes in most cases the id is the same, but for example the ActionEvent class has no ALT_GRAPH_MASK. Are you sure that it should be set?
>>
>> On 24.03.16 12:56, Ambarish Rapte wrote:
>>> Hi,
>>>
>>>                    Please review fix for JDK9,
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-6191390
>>>
>>>                    Webrev:
>>> http://cr.openjdk.java.net/~arapte/6191390/webrev.00/
>>>
>>> Issue:
>>>
>>>                    Only on windows:
>>>
>>> ActionEvent received by ActionListener, does not contain modifiers
>>> specific to ActionEvent class.
>>>
>>> On Linux & Mac:
>>>
>>> Correct modifiers are received.
>>>
>>> Cause:
>>>
>>>                    The actionEvent created in windows is created only
>>> with Extended modifiers.
>>>
>>> Fix:
>>>
>>>                    While creating ActionEvent, added code to include
>>> ActionEvent modifiers in addition to earlier included extended modifiers.
>>>
>>> Verification:
>>>
>>>                    The new test passes on Windows with fix, & also on
>>> ubuntu & mac.
>>>
>>>                    No Regression & JCK test fails due to this change.
>>>
>>> Regards,
>>>
>>> Ambarish
>>>
>>
>>
>> --
>> Best regards, Sergey.
>>
>
>
> --
> Best regards, Sergey.
>


-- 
Best regards, Sergey.