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

[Ambarish Rapte]

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.