[10] Review request for 8178448: MenuBar item handler fired twice

[Alexander Zvegintsev]

Hi all,

please review the fix

http://cr.openjdk.java.net/~azvegint/jdk/10/8178448/00/

for the issue

https://bugs.openjdk.java.net/browse/JDK-8178448

This Mac specific issue happens if setDefaultMenuBar() is called after 
setJMenuBar() with same instance of JMenuBar.


JFrame.setJMenuBar(mb);
Desktop.getDesktop().setDefaultMenuBar(mb);

--
Thanks,
Alexander.

Re: [OpenJDK 2D-Dev] [10] JDK-8169044: The tray icon color is not green

[Sergey Bylokhov]

Hi, Shashi.
As far as I understand the "width/height" in the IconCanvas is a size of the 
TrayIcon in pixels. So on the normal screen it is should be 24(TRAY_ICON_WIDTH) 
pixels and on a HiDPI screen it should be 24*ScreenScale. The 
IconCanvas.width/height fields should reflect this size.

I am not sure but it looks like the current fix changes the "size" of the frame 
just for rendering(so we select HiDPI image) but did not change the size of the 
frame. So we draw the HiDPI image to non-HiDPI embedded frame.

- shashidhara.veerabhadra...@oracle.com wrote:

> Yes Jim. Per the current code base, it is defaulted to default
> screen.
> 
> To have the same tray icon being represented on multiple task bars
> would require updates to XSystemTrayPeer.java to replicate the message
> being sent to X11 to add a new tray icon for a particular
> screen(Currently it is done only for the default screen). The current
> X11 tray system uses XEMBED protocol to add system tray icons. 
> Currently the screen is set to 0 which I think would refer to default
> screen and hence it adds tray icons only to the default screen. The
> X11 property '_NET_SYSTEM_TRAY_S[SCREEN_NUMBER]' allows access to the
> system tray of screen 'SCREEN_ NUMBER '. So we may need to use such
> mechanism to add the tray icon onto different screens. This may be
> different for windows and mac but will have a similar mechanism.
> 
> Thanks and regards,
> Shashi
> 
> -Original Message-
> From: Jim Graham 
> Sent: Tuesday, August 1, 2017 2:14 AM
> To: Shashidhara Veerabhadraiah
> ; awt-dev@openjdk.java.net;
> 2d-dev <2d-...@openjdk.java.net>
> Subject: Re: [OpenJDK 2D-Dev] [10] JDK-8169044: The tray icon color is
> not green
> 
> Will the tray icon canvas always be on the default screen?  I believe
> the latest MacOS and Win10 both allow menu bars and task bars on all
> monitors.  Linux may not be far behind...
> 
>   ...jim
> 
> On 7/30/17 11:59 PM, Shashidhara Veerabhadraiah wrote:
> > Hi, Kindly review a fix for JDK-8169044 where the non hi dpi icon
> was 
> > picked among the icon set of hi dpi and a non hi dpi icons for a hi
> dpi display screen.
> > 
> > Issue: The non hi dpi icon is red in color (and hi dpi icon is green
> 
> > in color) and was getting picked up among the set for a hi dpi
> display screen as shown below in the picture:
> > 
> > Solution and fix: The icon's buffered images are not subjected to
> the 
> > scaling because of the hi dpi screen. Hence, the default non hi dpi
> 
> > icon was getting picked up for rendering the tray icon. Now the
> source 
> > code modified to apply necessary transformations to the
> bufferedimages to get the default icon based on the default display
> screen. Below is the output after the fix:
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8169044
> > 
> > Webrev:
> http://cr.openjdk.java.net/~pkbalakr/shashi/8169044/webrev.00/
> > 
> > Thanks and regards,
> > 
> > Shashi
> >

Re: [10][JDK-8027154] [TESTBUG] Test java/awt/Mouse/GetMousePositionTest/GetMousePositionWithPopup.java fails

[Sergey Bylokhov]

Ok, looks fine. 
Thank you for clarification. 

- krishna.addepa...@oracle.com wrote: 
> 
> 
> 

Hi Sergey, 



Based on our conversation, could you provide your review comments for the fix? 



Pasting the mail exchange below for reference: 





The intent of the test is not modified by my changes. By having the second 
frame register a mouse move callback, it is made more explicit - when the mouse 
position should be queried, and what is the expected position it should 
contain. Also, since Frame2 has observed a mouse move, consequently, Frame1 
should not report any mouse position. 





-Original Message- 

From: Sergey Bylokhov 

Sent: Tuesday, August 1, 2017 2:13 AM 

To: Krishna Addepalli < krishna.addepa...@oracle.com > 

Cc: Ambarish Rapte < ambarish.ra...@oracle.com >; Ajit Ghaisas < 
ajit.ghai...@oracle.com >; Prem Balakrishnan < prem.balakrish...@oracle.com >; 
Praveen Srivastava < praveen.s.srivast...@oracle.com >; Prasanta Sadhukhan ( 
prasanta.sadhuk...@oracle.com ) < prasanta.sadhuk...@oracle.com > 

Subject: Re: awt-dev Digest, Vol 123, Issue 9 



You can modify the test as you wish to make it stable, just make sure that 
initial use case is covered. 



On 27.07.2017 4:29, Krishna Addepalli wrote: 

> Hi Sergey, 

> 

> I have tested the modified test that you provided on Windows and Linux. 

> It was working fine on Windows without the sleep, but on Linux, the 

> behavior is a bit random. If I removed the sleep, it is crashing 

> always, but occasionally it crashed even for a 2 second sleep time. 

> 

> I think, it is better not to rely on sleep, and instead, let frame2 

> have a mouse move callback registered, and then perform the required 

> checks, which guarantees a deterministic behavior, and also holds the 

> spirit of the intended test. 

> 

> Could you review the patch that I submitted and let me know? 

> 

> Thanks, 

> 

> Krishna 

> 

> *From:* Krishna Addepalli 

> *Sent:* Tuesday, July 25, 2017 8:41 PM 

> *To:* Sergey Bylokhov < sergey.bylok...@oracle.com > 

> *Cc:* Ambarish Rapte < ambarish.ra...@oracle.com >; Ajit Ghaisas 

> < ajit.ghai...@oracle.com >; Prem Balakrishnan 

> < prem.balakrish...@oracle.com >; Praveen Srivastava 

> < praveen.s.srivast...@oracle.com > 

> *Subject:* RE: awt-dev Digest, Vol 123, Issue 9 

> 

> Hi Sergey, 

> 

> Thanks for your clarifications. That explains the reasons for 

> Util.waitForIdle() not working in earlier versions. 

> 

> So to summarize, the intent of the test is to create a new frame 

> (“Popup 

> Frame”) for the MouseMove callback, and then have the new frame report 

> the expected mouse position, while the underlying frame should report 

> that mouse position is null, since it is no longer the active frame. 

> 

> I have one more question: 

> 

> In your code, you are sleeping for 2 seconds, after making the frame2 

> visible. Is this necessary? I have seen that, once we set frame2 

> visible, the mouse position can be immediately queried without the 

> need to wait. Is it done to make sure that the test works on older builds? 

> 

> Out of curiosity, I wanted to know if you could see the same 

> failure(“Exception in thread "AWT-EventQueue-2" 

> java.lang.RuntimeException: Not implemented”)that was reported in the 

> original bug? 

> 

> Thanks, 

> 

> Krishna 

> 

> *From:* Sergey Bylokhov 

> *Sent:* Tuesday, July 25, 2017 7:04 AM 

> *To:* Krishna Addepalli  < mailto:krishna.addepa...@oracle.com >> 

> *Cc:* Ambarish Rapte  < mailto:ambarish.ra...@oracle.com >>; Ajit Ghaisas 

> < ajit.ghai...@oracle.com >; Prem 

> Balakrishnan  < mailto:prem.balakrish...@oracle.com >>; Praveen Srivastava 

>  < mailto:praveen.s.srivast...@oracle.com >> 

> *Subject:* Re: awt-dev Digest, Vol 123, Issue 9 

> 

> Hi, Krishna. 

> Here is my observation. 

> When 8012026 bug was fixed the robot.waitForIdle() does not work 

> properly, but the test uses Util.waitForIdle(). When the robot was 

> fixed the implementation of Util.waitForIdle() was reworked. 

> So if you use the old jdk and a new utils+test you will need to add 

> some delays. 

> Another issue is for coordinates. When this bug was fixed we do not 

> get a mouse move event for the first mouseMove(149,149). But this bug 

> was fixed on osx long time ago. 

> 

> I have attached an updated version of the test which fails on b109 and 

> pass on 110. For the current version of jdk delays/sleeps are not 

> necessary since waitForIdle should work. 

> The steps which are tested: 

> - Show frame1 

> - Move the mouse over frame1 

> - Show frame2 under the mouse 

> - Check the mouse position reported by frame2 

> - Check the mouse position reported by frame1 

> 

> - krishna.addepa...@oracle.com 

> < mailto:krishna.addepa...@oracle.com > 

> wrote: 

>> 

> 

>> 

Re: [OpenJDK 2D-Dev] [10] JDK-8169044: The tray icon color is not green

[Shashidhara Veerabhadraiah]

Yes Jim. Per the current code base, it is defaulted to default screen.

To have the same tray icon being represented on multiple task bars would 
require updates to XSystemTrayPeer.java to replicate the message being sent to 
X11 to add a new tray icon for a particular screen(Currently it is done only 
for the default screen). The current X11 tray system uses XEMBED protocol to 
add system tray icons. 
Currently the screen is set to 0 which I think would refer to default screen 
and hence it adds tray icons only to the default screen. The X11 property 
'_NET_SYSTEM_TRAY_S[SCREEN_NUMBER]' allows access to the system tray of screen 
'SCREEN_ NUMBER '. So we may need to use such mechanism to add the tray icon 
onto different screens. This may be different for windows and mac but will have 
a similar mechanism.

Thanks and regards,
Shashi

-Original Message-
From: Jim Graham 
Sent: Tuesday, August 1, 2017 2:14 AM
To: Shashidhara Veerabhadraiah ; 
awt-dev@openjdk.java.net; 2d-dev <2d-...@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] [10] JDK-8169044: The tray icon color is not green

Will the tray icon canvas always be on the default screen?  I believe the 
latest MacOS and Win10 both allow menu bars and task bars on all monitors.  
Linux may not be far behind...

...jim

On 7/30/17 11:59 PM, Shashidhara Veerabhadraiah wrote:
> Hi, Kindly review a fix for JDK-8169044 where the non hi dpi icon was 
> picked among the icon set of hi dpi and a non hi dpi icons for a hi dpi 
> display screen.
> 
> Issue: The non hi dpi icon is red in color (and hi dpi icon is green 
> in color) and was getting picked up among the set for a hi dpi display screen 
> as shown below in the picture:
> 
> Solution and fix: The icon's buffered images are not subjected to the 
> scaling because of the hi dpi screen. Hence, the default non hi dpi 
> icon was getting picked up for rendering the tray icon. Now the source 
> code modified to apply necessary transformations to the bufferedimages to get 
> the default icon based on the default display screen. Below is the output 
> after the fix:
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8169044
> 
> Webrev: http://cr.openjdk.java.net/~pkbalakr/shashi/8169044/webrev.00/
> 
> Thanks and regards,
> 
> Shashi
> 

Re: [10][JDK-8027154] [TESTBUG] Test java/awt/Mouse/GetMousePositionTest/GetMousePositionWithPopup.java fails

[Krishna Addepalli]

Hi Sergey,

 

Based on our conversation, could you provide your review comments for the fix?

 

Pasting the mail exchange below for reference:

 

 

The intent of the test is not modified by my changes. By having the second 
frame register a mouse move callback, it is made more explicit - when the mouse 
position should be queried, and what is the expected position it should 
contain. Also, since Frame2 has observed a mouse move, consequently, Frame1 
should not report any mouse position.

 

 

-Original Message-

From: Sergey Bylokhov

Sent: Tuesday, August 1, 2017 2:13 AM

To: Krishna Addepalli mailto:krishna.addepa...@oracle.com"krishna.addepa...@oracle.com>

Cc: Ambarish Rapte mailto:ambarish.ra...@oracle.com"ambarish.ra...@oracle.com>; Ajit Ghaisas 
mailto:ajit.ghai...@oracle.com"ajit.ghai...@oracle.com>; Prem 
Balakrishnan mailto:prem.balakrish...@oracle.com"prem.balakrish...@oracle.com>; Praveen 
Srivastava mailto:praveen.s.srivast...@oracle.com"praveen.s.srivast...@oracle.com>; 
Prasanta Sadhukhan (HYPERLINK 
"mailto:prasanta.sadhuk...@oracle.com"prasanta.sadhuk...@oracle.com) mailto:prasanta.sadhuk...@oracle.com"prasanta.sadhuk...@oracle.com>

Subject: Re: awt-dev Digest, Vol 123, Issue 9

 

You can modify the test as you wish to make it stable, just make sure that 
initial use case is covered.

 

On 27.07.2017 4:29, Krishna Addepalli wrote:

> Hi Sergey,

> 

> I have tested the modified test that you provided on Windows and Linux. 

> It was working fine on Windows without the sleep, but on Linux, the 

> behavior is a bit random. If I removed the sleep, it is crashing 

> always, but occasionally it crashed even for a 2 second sleep time.

> 

> I think, it is better not to rely on sleep, and instead, let frame2 

> have a mouse move callback registered, and then perform the required 

> checks, which guarantees a deterministic behavior, and also holds the 

> spirit of the intended test.

> 

> Could you review the patch that I submitted and let me know?

> 

> Thanks,

> 

> Krishna

> 

> *From:* Krishna Addepalli

> *Sent:* Tuesday, July 25, 2017 8:41 PM

> *To:* Sergey Bylokhov  "mailto:sergey.bylok...@oracle.com"sergey.bylok...@oracle.com>

> *Cc:* Ambarish Rapte  "mailto:ambarish.ra...@oracle.com"ambarish.ra...@oracle.com>; Ajit Ghaisas 

> mailto:ajit.ghai...@oracle.com"ajit.ghai...@oracle.com>; Prem 
> Balakrishnan 

>  "mailto:prem.balakrish...@oracle.com"prem.balakrish...@oracle.com>; Praveen 
> Srivastava 

>  "mailto:praveen.s.srivast...@oracle.com"praveen.s.srivast...@oracle.com>

> *Subject:* RE: awt-dev Digest, Vol 123, Issue 9

> 

> Hi Sergey,

> 

> Thanks for your clarifications. That explains the reasons for

> Util.waitForIdle() not working in earlier versions.

> 

> So to summarize, the intent of the test is to create a new frame 

> (“Popup

> Frame”) for the MouseMove callback, and then have the new frame report 

> the expected mouse position, while the underlying frame should report 

> that mouse position is null, since it is no longer the active frame.

> 

> I have one more question:

> 

> In your code, you are sleeping for 2 seconds, after making the frame2 

> visible. Is this necessary? I have seen that, once we set frame2 

> visible, the mouse position can be immediately queried without the 

> need to wait. Is it done to make sure that the test works on older builds?

> 

> Out of curiosity, I wanted to know if you could see the same 

> failure(“Exception in thread "AWT-EventQueue-2"

> java.lang.RuntimeException: Not implemented”)that was reported in the 

> original bug?

> 

> Thanks,

> 

> Krishna

> 

> *From:* Sergey Bylokhov

> *Sent:* Tuesday, July 25, 2017 7:04 AM

> *To:* Krishna Addepalli  >

> *Cc:* Ambarish Rapte  >; Ajit Ghaisas 

>  "mailto:ajit.ghai...@oracle.com%20%3cmailto:ajit.ghai...@oracle.com"ajit.ghai...@oracle.com
>  >; Prem 

> Balakrishnan  >; Praveen Srivastava 

>  >

> *Subject:* Re: awt-dev Digest, Vol 123, Issue 9

> 

> Hi, Krishna.

> Here is my observation.

> When 8012026 bug was fixed the robot.waitForIdle() does not work 

> properly, but the test uses Util.waitForIdle(). When the robot was 

> fixed the implementation of Util.waitForIdle() was reworked.

> So if you use the old jdk and a new utils+test you will need to add 

> some delays.

> Another issue is for coordinates. When this bug was fixed we do not 

> get a mouse move event for the first mouseMove(149,149). But this bug 

> was fixed on osx long time ago.

> 

> I have attached an updated version of the test which fails on b109 and 

> pass on 110. For the current version of jdk delays/sleeps are not 

> necessary since waitForIdle