Re: Input needed: JDK-8161664: Memory leak in com.apple.laf.AquaProgressBarUI: removed progress bar still referenced

[Robin Stevens]

Alexandr,

that sounds right.
It took me a while to figure out the exact circumstances to trigger this
memory leak.

In my application where I found this bug, I have a progress bar which is
repeatedly switched from between visible and invisible, and between
determinate and indeterminate.
I could probably work around this issue by altering the sequence in which
setVisible and setIndeterminate is called, but I can just as well fix the
problem in the JDK.

Anyway, adding the isDisplayable checks in the AquaProgressBarUI class
seems to work. Test runs fine when those are added.
I will send a new email to the list tomorrow with a request for a review.

Thanks for your valuable input,

Robin

On Tue, Jul 19, 2016 at 6:06 PM, Alexandr Scherbatiy <
alexandr.scherba...@oracle.com> wrote:

> On 7/19/2016 4:33 PM, Robin Stevens wrote:
>
> Hello Alexandr,
>
> very valid remark.
> Running that same test program on Linux with the metal look and feel
> reveals no memory leak. I have no access to a Windows machine, so I
> couldn't get the Windows specific look and feel.
>
> The other ProgressBarUI implementations seem to extend from
> BasicProgressBarUI, which has the same mechanism of an Animator which uses
> a Timer.
> However, in the test program the Timer does not get started on Linux
> (while it gets started on OS X).
>
> In the BasicProgressBarUI class, all calls to startAnimationTimer are
> wrapped with an if check:
>
> if (progressBar.isDisplayable()) {
> startAnimationTimer();
> }
>
> In the scenario from my test, the isDisplayable method returns false. On
> OS X, this check is missing so the timer is started.
>
>I believe that the changed AquaProgressBarUIMemoryLeakTest where the
> progress bar is visible and indeterminate value is set to true at the end
> should also not have the memory leaks.
>
>   Thanks,
>   Alexandr.
>
>
> I assume adding that same check in the AquaProgressBarUI will fix the
> problem as well. So that is a third approach to solve the issue.
>
> Robin
>
> On Tue, Jul 19, 2016 at 1:14 PM, Alexandr Scherbatiy <
> alexandr.scherba...@oracle.com> wrote:
>
>> On 7/19/2016 12:27 PM, Robin Stevens wrote:
>>
>>> Hello,
>>>
>>> I wanted to discuss my approach for issue JDK-8161664 (
>>> https://bugs.openjdk.java.net/browse/JDK-8161664) before I started
>>> working on this issue.
>>>
>>> In certain scenarios (see the JIRA issue for an example), the Timer in
>>> the Animator inner class of the AquaProgressBarUI class remains running,
>>> even when the JProgressBar has already been removed from the UI. This
>>> causes a memory leak, as that running Timer avoids that the JProgressBar
>>> can be GC-ed. As long as the Timer is running, the JProgressBar is
>>> referenced through
>>>
>>> Timer -> ActionListener (=Animator inner class) -> AquaProgressBarUI
>>> outer class -> JProgressBar field
>>>
>>>
>>>
>>> I see two possible approaches to fix this:
>>>
>>> 1) I carefully investigate the particular scenario I found, and try to
>>> figure out why the Timer is not stopped and fix this particular scenario.
>>> This offers of course no guarantees that there are no other scenarios which
>>> keep the Timer running.
>>>
>>> 2) I replace one of the hard references with a weak reference, hence
>>> avoiding the memory leak in all cases.
>>> If I do not attach the Animator inner class directly as listener to the
>>> timer, but use another ActionListener which only has a WeakReference to the
>>> Animator class, the memory leak is solved.
>>> The ActionListener could then stop the timer when the timer is fired and
>>> the WeakReference#get returns null.
>>>
>>>
>>>
>>> I prefer the second approach. By cutting the hard reference between the
>>> Timer and the Animator + stopping the Timer when the Animator is GC-ed, I
>>> ensure that the Timer cannot cause a memory leak anymore. This avoids
>>> overlooking certain scenarios.
>>>
>>> Any input on this ? Any preferences for a certain approach, or proposal
>>> for another approach.
>>>
>>Does other L (for example Metal) have the same memory leak with the
>> JProgressBar? If no, it would be interesting to know what is the difference
>> between them and the AquaProgressBarUI.
>>
>>   Thanks,
>>   Alexandr.
>>
>>>
>>>
>>> Robin
>>>
>>
>>
>
>

Re: [9] Review Request JDK-8147648 [hidpi] multiresolution image: wrong resolution variant is used as icon in the Unity panel

[Rajeev Chamyal]

Hello Semyon,

 

Please review the updated webrev.

http://cr.openjdk.java.net/~rchamyal/8147648/webrev.03/

 

Regards,

Rajeev Chamyal

 

From: Semyon Sadetsky 
Sent: 14 July 2016 16:58
To: Rajeev Chamyal; swing-dev@openjdk.java.net; Sergey Bylokhov; Alexander 
Scherbatiy
Subject: Re: [9] Review Request JDK-8147648 [hidpi] multiresolution 
image: wrong resolution variant is used as icon in the Unity panel

 

Hi Rajeev,

I have added 1px border to the icon in your test:

private static BufferedImage generateImage(int scale, Color c) {
int x = SZ * scale;
BufferedImage img = new BufferedImage(x, x, BufferedImage.TYPE_INT_RGB);
Graphics g = img.getGraphics();
if (g != null) {
g.setColor(c);
g.fillRect(0, 0, x, x);
g.setColor(Color.YELLOW);
g.drawRect(0, 0, x-1, x-1);
}
return img;
}

It seems the icon in the taskbar is not correct for UI scale > 1.

By the way, graphics object should be disposed using g.dispose() when it is not 
needed anymore.

--Semyon



On 14.07.2016 10:08, Rajeev Chamyal wrote:

Hello All,

 

Gentle reminder. Please review the updated webrev.

http://cr.openjdk.java.net/~rchamyal/8147648/webrev.02/

 

Update: simplified the test.

 

Regards,

Rajeev Chamyal

 

From: Alexandr Scherbatiy 
Sent: 22 June 2016 15:46
To: Rajeev Chamyal; Sergey Bylokhov; HYPERLINK 
"mailto:swing-dev@openjdk.java.net"swing-dev@openjdk.java.net
Subject: Re: [9] Review Request JDK-8147648 [hidpi] multiresolution 
image: wrong resolution variant is used as icon in the Unity panel

 

The fix looks good to me.

Thanks,
Alexandr.

On 6/22/2016 10:49 AM, Rajeev Chamyal wrote:

Hello Alexandr,

 

Thanks for the review. I have updated webrev as per comments.

http://cr.openjdk.java.net/~rchamyal/8147648/webrev.01/

 

Regards,

Rajeev Chamyal

 

From: Alexandr Scherbatiy 
Sent: 21 June 2016 17:37
To: Rajeev Chamyal; Sergey Bylokhov; HYPERLINK 
"mailto:swing-dev@openjdk.java.net"swing-dev@openjdk.java.net
Subject: Re: [9] Review Request JDK-8147648 [hidpi] multiresolution 
image: wrong resolution variant is used as icon in the Unity panel

 

On 6/21/2016 12:16 PM, Rajeev Chamyal wrote:

Hello All,

 

Please review the following webrev.

 

Webrev: HYPERLINK 
"http://cr.openjdk.java.net/%7Erchamyal/8147648/webrev.00/"http://cr.openjdk.java.net/~rchamyal/8147648/webrev.00/
 

Bug: https://bugs.openjdk.java.net/browse/JDK-8147648 

 

Issue: Wrong resolution variant is used as icon in the Unity panel.

Cause: The screen transforms are not applied to find the correct resolution 
variant image in current implementation.

Fix: Applied the screen transforms to graphics object.


 222 int scaleX = (int)tx.getScaleX();
 223 int scaleY = (int)tx.getScaleY();
 224 DataBufferInt buffer = new DataBufferInt(scaleX * width * scaleY * 
height);

  The fix is in the shared code and the scale factor can have floating point 
value on Windows. (for example 1.5).
  It is better to round the final width and height after scaling them.
 
  Thanks,
  Alexandr.

 

Regards,

Rajeev Chamyal

 

 

 

Re: Review Request JDK-8159168 [hidpi] Window.setShape() works incorrectly on HiDPI

[Rajeev Chamyal]

Hello Sergey,

It works fine on linux.

Regards,
Rajeev Chamyal

-Original Message-
From: Sergey Bylokhov 
Sent: 20 July 2016 01:13
To: Rajeev Chamyal; Semyon Sadetsky; Alexander Scherbatiy; 
swing-dev@openjdk.java.net
Subject: Re:  Review Request JDK-8159168 [hidpi] Window.setShape() 
works incorrectly on HiDPI

Hi, Rajeev.
Can you please clarify how it works(if it works) on Linux/Solaris?

On 11.07.16 12:08, Rajeev Chamyal wrote:
> Hello Semyon,
>
>
>
> Please review the updated webrev as per review comments.
>
> http://cr.openjdk.java.net/~rchamyal/8159168/webrev.04/
>
>
>
> Regards,
>
> Rajeev Chamyal
>
>
>
> *From:*Semyon Sadetsky
> *Sent:* 11 July 2016 14:29
> *To:* Rajeev Chamyal; Alexander Scherbatiy; 
> swing-dev@openjdk.java.net; Sergey Bylokhov
> *Subject:* Re:  Review Request JDK-8159168 [hidpi]
> Window.setShape() works incorrectly on HiDPI
>
>
>
> On 7/11/2016 11:10 AM, Rajeev Chamyal wrote:
>
> Hello Semyon,
>
>
>
> Thanks for the review. Yes, mouse move is not required I have
> removed it.
>
> Please review the updated webrev.
>
> http://cr.openjdk.java.net/~rchamyal/8159168/webrev.03/
>
> Thanks.
> I also cannot see the reason to wait 1 second in line 59.
> It seems tx variable from line 53 is never used.
>
> --Semyon
>
>
>
> Regards,
>
> Rajeev Chamyal
>
>
>
> *From:*Semyon Sadetsky
> *Sent:* 11 July 2016 12:30
> *To:* Rajeev Chamyal; Alexander Scherbatiy;
> swing-dev@openjdk.java.net ;
> Sergey Bylokhov
> *Subject:* Re:  Review Request JDK-8159168 [hidpi]
> Window.setShape() works incorrectly on HiDPI
>
>
>
> Hi Rajeev,
>
> The fix itself looks good. I only did not get for what purpose mouse
> pointer is moved in the test?
>
> --Semyon
>
>
>
> On 7/5/2016 9:16 AM, Rajeev Chamyal wrote:
>
> Hello Alexandr,
>
>
>
> Please review updated webrev.
>
> http://cr.openjdk.java.net/~rchamyal/8159168/webrev.02/
>
>
>
> Regards,
>
> Rajeev Chamyal
>
>
>
> *From:*Alexandr Scherbatiy
> *Sent:* 05 July 2016 11:38
> *To:* Rajeev Chamyal; swing-dev@openjdk.java.net
> ; Sergey Bylokhov
> *Subject:* Re:  Review Request JDK-8159168 [hidpi]
> Window.setShape() works incorrectly on HiDPI
>
>
>
> On 7/5/2016 8:25 AM, Rajeev Chamyal wrote:
>
>
>
> Hello Alexandr,
>
>
>
> Thanks for the review.
>
> As per windows specification X & Y scale are always equal
> that's why I have put scaleX == scaleY check.
>
> But it may change in future so I have removed this check.
>
>
>
> http://cr.openjdk.java.net/~rchamyal/8159168/webrev.01/
>
>
> 1135 if (scaleX != 1 && scaleY != 1) {
>
>   It is better to use 'or' operator because the shape should be
> scaled when on of the scales is differ from 1.
>
> Thanks,
> Alexandr.
>
>
>
>
>
>
> Regards,
>
> Rajeev Chamyal
>
>
>
> *From:*Alexandr Scherbatiy
> *Sent:* 04 July 2016 18:03
> *To:* Rajeev Chamyal; swing-dev@openjdk.java.net
> ; Sergey Bylokhov
> *Subject:* Re:  Review Request JDK-8159168
> [hidpi] Window.setShape() works incorrectly on HiDPI
>
>
>
> On 7/4/2016 3:09 PM, Rajeev Chamyal wrote:
>
>
>
>
> Hello All,
>
>
>
> Please review the following webrev.
>
>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8159168
>
> Webrev:
> http://cr.openjdk.java.net/~rchamyal/8159168/webrev.00/
> 
> 
>
>
>
> Issue: In HiDPI screen shape set through
> window::setShape API is not scaled based on system scale.
>
> Fix:. Updated the WComponentPeer::applyShape to update
> shape based on system scale.
>
> 1131 double scaleX =
> winGraphicsConfig.getDefaultTransform().getScaleX();
> 1132 double scaleY =
> winGraphicsConfig.getDefaultTransform().getScaleY();
>
>  The getDefaultTransform() is called twice which leads that
> AffineTransform object is created two times
> 1133 if (scaleX != 1 && scaleY != 1 && scaleX ==
> scaleY) {
>
>Is the check scaleX == scaleY really necessary here?
>
>Is it possible to make the test automated? Just run it
> with option "@run main/othervm -Dsun.java2d.uiScale=2
> TestName" and check the area where the shape is drawn?
>
>   Thanks,
>   Alexandr.
>
>
>
>
>
>
> Regards,
>
> Rajeev Chamyal
>
>
>
>
>

Re: Review Request JDK-8159168 [hidpi] Window.setShape() works incorrectly on HiDPI

[Sergey Bylokhov]

Hi, Rajeev.
Can you please clarify how it works(if it works) on Linux/Solaris?

On 11.07.16 12:08, Rajeev Chamyal wrote:

Hello Semyon,



Please review the updated webrev as per review comments.

http://cr.openjdk.java.net/~rchamyal/8159168/webrev.04/



Regards,

Rajeev Chamyal



*From:*Semyon Sadetsky
*Sent:* 11 July 2016 14:29
*To:* Rajeev Chamyal; Alexander Scherbatiy; swing-dev@openjdk.java.net;
Sergey Bylokhov
*Subject:* Re:  Review Request JDK-8159168 [hidpi]
Window.setShape() works incorrectly on HiDPI



On 7/11/2016 11:10 AM, Rajeev Chamyal wrote:

Hello Semyon,



Thanks for the review. Yes, mouse move is not required I have
removed it.

Please review the updated webrev.

http://cr.openjdk.java.net/~rchamyal/8159168/webrev.03/

Thanks.
I also cannot see the reason to wait 1 second in line 59.
It seems tx variable from line 53 is never used.

--Semyon



Regards,

Rajeev Chamyal



*From:*Semyon Sadetsky
*Sent:* 11 July 2016 12:30
*To:* Rajeev Chamyal; Alexander Scherbatiy;
swing-dev@openjdk.java.net ;
Sergey Bylokhov
*Subject:* Re:  Review Request JDK-8159168 [hidpi]
Window.setShape() works incorrectly on HiDPI



Hi Rajeev,

The fix itself looks good. I only did not get for what purpose mouse
pointer is moved in the test?

--Semyon



On 7/5/2016 9:16 AM, Rajeev Chamyal wrote:

Hello Alexandr,



Please review updated webrev.

http://cr.openjdk.java.net/~rchamyal/8159168/webrev.02/



Regards,

Rajeev Chamyal



*From:*Alexandr Scherbatiy
*Sent:* 05 July 2016 11:38
*To:* Rajeev Chamyal; swing-dev@openjdk.java.net
; Sergey Bylokhov
*Subject:* Re:  Review Request JDK-8159168 [hidpi]
Window.setShape() works incorrectly on HiDPI



On 7/5/2016 8:25 AM, Rajeev Chamyal wrote:



Hello Alexandr,



Thanks for the review.

As per windows specification X & Y scale are always equal
that’s why I have put scaleX == scaleY check.

But it may change in future so I have removed this check.



http://cr.openjdk.java.net/~rchamyal/8159168/webrev.01/


1135 if (scaleX != 1 && scaleY != 1) {

  It is better to use 'or' operator because the shape should be
scaled when on of the scales is differ from 1.

Thanks,
Alexandr.






Regards,

Rajeev Chamyal



*From:*Alexandr Scherbatiy
*Sent:* 04 July 2016 18:03
*To:* Rajeev Chamyal; swing-dev@openjdk.java.net
; Sergey Bylokhov
*Subject:* Re:  Review Request JDK-8159168
[hidpi] Window.setShape() works incorrectly on HiDPI



On 7/4/2016 3:09 PM, Rajeev Chamyal wrote:




Hello All,



Please review the following webrev.



Bug: https://bugs.openjdk.java.net/browse/JDK-8159168

Webrev:
http://cr.openjdk.java.net/~rchamyal/8159168/webrev.00/




Issue: In HiDPI screen shape set through
window::setShape API is not scaled based on system scale.

Fix:. Updated the WComponentPeer::applyShape to update
shape based on system scale.

1131 double scaleX =
winGraphicsConfig.getDefaultTransform().getScaleX();
1132 double scaleY =
winGraphicsConfig.getDefaultTransform().getScaleY();

 The getDefaultTransform() is called twice which leads that
AffineTransform object is created two times
1133 if (scaleX != 1 && scaleY != 1 && scaleX ==
scaleY) {

   Is the check scaleX == scaleY really necessary here?

   Is it possible to make the test automated? Just run it
with option "@run main/othervm -Dsun.java2d.uiScale=2
TestName" and check the area where the shape is drawn?

  Thanks,
  Alexandr.






Regards,

Rajeev Chamyal














--
Best regards, Sergey.

Re: [OpenJDK 2D-Dev] RFR(L): 8160974: [TESTBUG] Mark more headful tests with @key headful.

[Lindenmaier, Goetz]

Hi Sergey,

thanks a lot for looking at this!

Best regards,
  Goetz.

> -Original Message-
> From: Sergey Bylokhov [mailto:sergey.bylok...@oracle.com]
> Sent: Tuesday, July 19, 2016 9:17 PM
> To: Lindenmaier, Goetz ; awt-
> d...@openjdk.java.net; swing-dev@openjdk.java.net; 2d-dev <2d-
> d...@openjdk.java.net>
> Subject: Re: [OpenJDK 2D-Dev] RFR(L): 8160974: [TESTBUG] Mark more
> headful tests with @key headful.
> 
> Look fine to me.
> 
> On 07.07.16 18:01, Lindenmaier, Goetz wrote:
> > Hi,
> >
> >
> >
> > This change is 'L' because there are changes to a lot of files, but the
> > changes
> >
> > are all similar, so it's rather easy to review.
> >
> > Similar to 8159690 I added @key headful to another around 600 tests.
> >
> > I used different patterns to grep for the headful exceptions.
> >
> >
> >
> > These are now all I could find with grepping and the like. I have around
> >
> > 80 failing tests remaining, where a row will probably also depend on
> >
> > a display, but I want to look at them more closely, so I don't want
> >
> > to include them here.
> >
> >
> >
> > Please review this change:
> >
> > http://cr.openjdk.java.net/~goetz/wr16/8160974-headful/webrev.01/
> >
> >
> >
> > Best regards,
> >
> >   Goetz.
> >
> >
> >
> >
> >
> 
> 
> --
> Best regards, Sergey.

Re: [OpenJDK 2D-Dev] RFR(L): 8160974: [TESTBUG] Mark more headful tests with @key headful.

[Sergey Bylokhov]

Look fine to me.

On 07.07.16 18:01, Lindenmaier, Goetz wrote:

Hi,



This change is ‘L’ because there are changes to a lot of files, but the
changes

are all similar, so it’s rather easy to review.

Similar to 8159690 I added @key headful to another around 600 tests.

I used different patterns to grep for the headful exceptions.



These are now all I could find with grepping and the like. I have around

80 failing tests remaining, where a row will probably also depend on

a display, but I want to look at them more closely, so I don’t want

to include them here.



Please review this change:

http://cr.openjdk.java.net/~goetz/wr16/8160974-headful/webrev.01/



Best regards,

  Goetz.








--
Best regards, Sergey.

Re: RfR JDK-8161483 Implement AccessibleAction interface in JList.AccessibleJList.AccessibleJListChild

[Pete Brunet]

So rename Bug8161483 to AccessibleActionOnAccessibleJListChild?

Do we need input from an SQE lead for guidance for this bug and all
future bugs?

Pete

On 7/19/16 2:02 PM, Phil Race wrote:
> A meaningful name with some relevance to what the test tests ?
>
> -phil.
>
> On 07/19/2016 12:03 PM, Pete Brunet wrote:
>>
>>
>> On 7/19/16 1:52 PM, Phil Race wrote:
>>> The fix is fine but as I've said elsewhere I really don't like
>>> BugXX as test names.
>> Hi Phil, I haven't seen any of your prior comments about this
>> matter.  I haven't seen any other style so just assumed that is/was
>> the standard.  Is there an alternative standard I should start to use?
>>
>> Pete
>>>
>>> -phil.
>>>
>>> On 07/19/2016 11:43 AM, Alexandr Scherbatiy wrote:
 The fix looks good to me.

 Thanks,
 Alexandr.

 On 7/19/2016 8:50 PM, Pete Brunet wrote:
> Look at .02 instead.  I had an extraneous println left in .01.
> http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.02/
>
> On 7/19/16 11:48 AM, Pete Brunet wrote:
>> I added a regression test:
>>  http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.01/
>>
>> Could someone please review that?
>>
>> I also need one more +1 on the code change.
>>
>> TiA, Pete
>>
>> On 7/19/16 3:17 AM, Alexandr Scherbatiy wrote:
>>>
>>> The fix looks good to me.
>>>
>>> Thanks,
>>> Alexandr.
>>>
>>> On 7/19/2016 5:10 AM, Pete Brunet wrote:
 Please review the following:

 Bug: https://bugs.openjdk.java.net/browse/JDK-8161483
 Patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.00/

 This is a followon to the patch for
 JDK-8145207 [macosx] JList, VO can't access non-visible list items

 In order to fixJDK-8145207the AccessibleAction interface was
 needed for JList.AccessibleJList.AccessibleJListChild but a
 backport of this fix has been requested and the released public
 API can not be changed in 8u or earlier.  The workaround for
 JDK-8145207 is to create and use a private subclass of
 JList.AccessibleJList.AccessibleJListChild,
 JList.AccessibleJList.ActionableAccessibleJListChild.  The
 downside of this fix is that it returns a subclass of the
 JList.AccessibleJList.AccessibleJListChild.  If a user
 overrides the class and returns from its code it will not
 inherit the AccessibleAction behavior.  For JDK 9 the
 ActionableAccessibleJListChild subclass should be removed and
 the AccessibleAction implementation moved to
 JList.AccessibleJList.AccessibleJListChild.

 TiA,
 Pete 
>>>
>>
>

>>>
>>
>

Re: RfR JDK-8161483 Implement AccessibleAction interface in JList.AccessibleJList.AccessibleJListChild

[Phil Race]

A meaningful name with some relevance to what the test tests ?

-phil.

On 07/19/2016 12:03 PM, Pete Brunet wrote:



On 7/19/16 1:52 PM, Phil Race wrote:
The fix is fine but as I've said elsewhere I really don't like 
BugXX as test names.
Hi Phil, I haven't seen any of your prior comments about this matter.  
I haven't seen any other style so just assumed that is/was the 
standard.  Is there an alternative standard I should start to use?


Pete


-phil.

On 07/19/2016 11:43 AM, Alexandr Scherbatiy wrote:

The fix looks good to me.

Thanks,
Alexandr.

On 7/19/2016 8:50 PM, Pete Brunet wrote:

Look at .02 instead.  I had an extraneous println left in .01.
http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.02/

On 7/19/16 11:48 AM, Pete Brunet wrote:

I added a regression test:
http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.01/

Could someone please review that?

I also need one more +1 on the code change.

TiA, Pete

On 7/19/16 3:17 AM, Alexandr Scherbatiy wrote:


The fix looks good to me.

Thanks,
Alexandr.

On 7/19/2016 5:10 AM, Pete Brunet wrote:

Please review the following:

Bug: https://bugs.openjdk.java.net/browse/JDK-8161483
Patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.00/

This is a followon to the patch for
JDK-8145207 [macosx] JList, VO can't access non-visible list items

In order to fixJDK-8145207the AccessibleAction interface was 
needed for JList.AccessibleJList.AccessibleJListChild but a 
backport of this fix has been requested and the released public 
API can not be changed in 8u or earlier.  The workaround for 
JDK-8145207 is to create and use a private subclass of 
JList.AccessibleJList.AccessibleJListChild, 
JList.AccessibleJList.ActionableAccessibleJListChild. The 
downside of this fix is that it returns a subclass of the 
JList.AccessibleJList.AccessibleJListChild.  If a user overrides 
the class and returns from its code it will not inherit the 
AccessibleAction behavior. For JDK 9 the 
ActionableAccessibleJListChild subclass should be removed and 
the AccessibleAction implementation moved to 
JList.AccessibleJList.AccessibleJListChild.


TiA,
Pete 

Re: RfR JDK-8161483 Implement AccessibleAction interface in JList.AccessibleJList.AccessibleJListChild

[Pete Brunet]

On 7/19/16 1:52 PM, Phil Race wrote:
> The fix is fine but as I've said elsewhere I really don't like
> BugXX as test names.
Hi Phil, I haven't seen any of your prior comments about this matter.  I
haven't seen any other style so just assumed that is/was the standard. 
Is there an alternative standard I should start to use?

Pete
>
> -phil.
>
> On 07/19/2016 11:43 AM, Alexandr Scherbatiy wrote:
>> The fix looks good to me.
>>
>> Thanks,
>> Alexandr.
>>
>> On 7/19/2016 8:50 PM, Pete Brunet wrote:
>>> Look at .02 instead.  I had an extraneous println left in .01.
>>> http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.02/
>>>
>>> On 7/19/16 11:48 AM, Pete Brunet wrote:
 I added a regression test:
  http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.01/

 Could someone please review that?

 I also need one more +1 on the code change.

 TiA, Pete

 On 7/19/16 3:17 AM, Alexandr Scherbatiy wrote:
>
> The fix looks good to me.
>
> Thanks,
> Alexandr.
>
> On 7/19/2016 5:10 AM, Pete Brunet wrote:
>> Please review the following:
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8161483
>> Patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.00/
>>
>> This is a followon to the patch for
>> JDK-8145207 [macosx] JList, VO can't access non-visible list items
>>
>> In order to fixJDK-8145207the AccessibleAction interface was
>> needed for JList.AccessibleJList.AccessibleJListChild but a
>> backport of this fix has been requested and the released public
>> API can not be changed in 8u or earlier.  The workaround for
>> JDK-8145207 is to create and use a private subclass of
>> JList.AccessibleJList.AccessibleJListChild,
>> JList.AccessibleJList.ActionableAccessibleJListChild.  The
>> downside of this fix is that it returns a subclass of the
>> JList.AccessibleJList.AccessibleJListChild.  If a user overrides
>> the class and returns from its code it will not inherit the
>> AccessibleAction behavior.  For JDK 9 the
>> ActionableAccessibleJListChild subclass should be removed and the
>> AccessibleAction implementation moved to
>> JList.AccessibleJList.AccessibleJListChild.
>>
>> TiA,
>> Pete 
>

>>>
>>
>

Re: RfR JDK-8161483 Implement AccessibleAction interface in JList.AccessibleJList.AccessibleJListChild

[Phil Race]

The fix is fine but as I've said elsewhere I really don't like BugXX 
as test names.


-phil.

On 07/19/2016 11:43 AM, Alexandr Scherbatiy wrote:

The fix looks good to me.

Thanks,
Alexandr.

On 7/19/2016 8:50 PM, Pete Brunet wrote:

Look at .02 instead.  I had an extraneous println left in .01.
http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.02/

On 7/19/16 11:48 AM, Pete Brunet wrote:

I added a regression test:
http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.01/

Could someone please review that?

I also need one more +1 on the code change.

TiA, Pete

On 7/19/16 3:17 AM, Alexandr Scherbatiy wrote:


The fix looks good to me.

Thanks,
Alexandr.

On 7/19/2016 5:10 AM, Pete Brunet wrote:

Please review the following:

Bug: https://bugs.openjdk.java.net/browse/JDK-8161483
Patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.00/

This is a followon to the patch for
JDK-8145207 [macosx] JList, VO can't access non-visible list items

In order to fixJDK-8145207the AccessibleAction interface was 
needed for JList.AccessibleJList.AccessibleJListChild but a 
backport of this fix has been requested and the released public 
API can not be changed in 8u or earlier.  The workaround for 
JDK-8145207 is to create and use a private subclass of 
JList.AccessibleJList.AccessibleJListChild, 
JList.AccessibleJList.ActionableAccessibleJListChild. The downside 
of this fix is that it returns a subclass of the 
JList.AccessibleJList.AccessibleJListChild.  If a user overrides 
the class and returns from its code it will not inherit the 
AccessibleAction behavior. For JDK 9 the 
ActionableAccessibleJListChild subclass should be removed and the 
AccessibleAction implementation moved to 
JList.AccessibleJList.AccessibleJListChild.


TiA,
Pete 

Re: RfR JDK-8161483 Implement AccessibleAction interface in JList.AccessibleJList.AccessibleJListChild

[Alexandr Scherbatiy]

The fix looks good to me.

Thanks,
Alexandr.

On 7/19/2016 8:50 PM, Pete Brunet wrote:

Look at .02 instead.  I had an extraneous println left in .01.
http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.02/

On 7/19/16 11:48 AM, Pete Brunet wrote:

I added a regression test:
http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.01/

Could someone please review that?

I also need one more +1 on the code change.

TiA, Pete

On 7/19/16 3:17 AM, Alexandr Scherbatiy wrote:


The fix looks good to me.

Thanks,
Alexandr.

On 7/19/2016 5:10 AM, Pete Brunet wrote:

Please review the following:

Bug: https://bugs.openjdk.java.net/browse/JDK-8161483
Patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.00/

This is a followon to the patch for
JDK-8145207 [macosx] JList, VO can't access non-visible list items

In order to fixJDK-8145207the AccessibleAction interface was needed 
for JList.AccessibleJList.AccessibleJListChild but a backport of 
this fix has been requested and the released public API can not be 
changed in 8u or earlier.  The workaround for JDK-8145207 is to 
create and use a private subclass of 
JList.AccessibleJList.AccessibleJListChild, 
JList.AccessibleJList.ActionableAccessibleJListChild. The downside 
of this fix is that it returns a subclass of the 
JList.AccessibleJList.AccessibleJListChild.  If a user overrides 
the class and returns from its code it will not inherit the 
AccessibleAction behavior. For JDK 9 the 
ActionableAccessibleJListChild subclass should be removed and the 
AccessibleAction implementation moved to 
JList.AccessibleJList.AccessibleJListChild.


TiA,
Pete 

Re: RfR JDK-8161483 Implement AccessibleAction interface in JList.AccessibleJList.AccessibleJListChild

[Pete Brunet]

Look at .02 instead.  I had an extraneous println left in .01.
http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.02/

On 7/19/16 11:48 AM, Pete Brunet wrote:
> I added a regression test:
>  http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.01/
>
> Could someone please review that?
>
> I also need one more +1 on the code change.
>
> TiA, Pete
>
> On 7/19/16 3:17 AM, Alexandr Scherbatiy wrote:
>>
>> The fix looks good to me.
>>
>> Thanks,
>> Alexandr.
>>
>> On 7/19/2016 5:10 AM, Pete Brunet wrote:
>>> Please review the following:
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8161483
>>> Patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.00/
>>>
>>> This is a followon to the patch for
>>> JDK-8145207 [macosx] JList, VO can't access non-visible list items
>>>
>>> In order to fixJDK-8145207the AccessibleAction interface was needed
>>> for JList.AccessibleJList.AccessibleJListChild but a backport of
>>> this fix has been requested and the released public API can not be
>>> changed in 8u or earlier.  The workaround for JDK-8145207 is to
>>> create and use a private subclass of
>>> JList.AccessibleJList.AccessibleJListChild,
>>> JList.AccessibleJList.ActionableAccessibleJListChild.  The downside
>>> of this fix is that it returns a subclass of the
>>> JList.AccessibleJList.AccessibleJListChild.  If a user overrides the
>>> class and returns from its code it will not inherit the
>>> AccessibleAction behavior.  For JDK 9 the
>>> ActionableAccessibleJListChild subclass should be removed and the
>>> AccessibleAction implementation moved to
>>> JList.AccessibleJList.AccessibleJListChild.
>>>
>>> TiA,
>>> Pete 
>>
>

Re: RfR JDK-8161483 Implement AccessibleAction interface in JList.AccessibleJList.AccessibleJListChild

[Pete Brunet]

I added a regression test:
 http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.01/

Could someone please review that?

I also need one more +1 on the code change.

TiA, Pete

On 7/19/16 3:17 AM, Alexandr Scherbatiy wrote:
>
> The fix looks good to me.
>
> Thanks,
> Alexandr.
>
> On 7/19/2016 5:10 AM, Pete Brunet wrote:
>> Please review the following:
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8161483
>> Patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.00/
>>
>> This is a followon to the patch for
>> JDK-8145207 [macosx] JList, VO can't access non-visible list items
>>
>> In order to fixJDK-8145207the AccessibleAction interface was needed
>> for JList.AccessibleJList.AccessibleJListChild but a backport of this
>> fix has been requested and the released public API can not be changed
>> in 8u or earlier.  The workaround for JDK-8145207 is to create and
>> use a private subclass of JList.AccessibleJList.AccessibleJListChild,
>> JList.AccessibleJList.ActionableAccessibleJListChild.  The downside
>> of this fix is that it returns a subclass of the
>> JList.AccessibleJList.AccessibleJListChild.  If a user overrides the
>> class and returns from its code it will not inherit the
>> AccessibleAction behavior.  For JDK 9 the
>> ActionableAccessibleJListChild subclass should be removed and the
>> AccessibleAction implementation moved to
>> JList.AccessibleJList.AccessibleJListChild.
>>
>> TiA,
>> Pete 
>

Re: Input needed: JDK-8161664: Memory leak in com.apple.laf.AquaProgressBarUI: removed progress bar still referenced

[Alexandr Scherbatiy]

On 7/19/2016 4:33 PM, Robin Stevens wrote:

Hello Alexandr,

very valid remark.
Running that same test program on Linux with the metal look and feel 
reveals no memory leak. I have no access to a Windows machine, so I 
couldn't get the Windows specific look and feel.


The other ProgressBarUI implementations seem to extend from 
BasicProgressBarUI, which has the same mechanism of an Animator which 
uses a Timer.
However, in the test program the Timer does not get started on Linux 
(while it gets started on OS X).


In the BasicProgressBarUI class, all calls to startAnimationTimer are 
wrapped with an if check:


if (progressBar.isDisplayable()) {
startAnimationTimer();
}

In the scenario from my test, the isDisplayable method returns false. 
On OS X, this check is missing so the timer is started.
   I believe that the changed AquaProgressBarUIMemoryLeakTest where the 
progress bar is visible and indeterminate value is set to true at the 
end should also not have the memory leaks.


  Thanks,
  Alexandr.


I assume adding that same check in the AquaProgressBarUI will fix the 
problem as well. So that is a third approach to solve the issue.


Robin

On Tue, Jul 19, 2016 at 1:14 PM, Alexandr Scherbatiy 
> wrote:


On 7/19/2016 12:27 PM, Robin Stevens wrote:

Hello,

I wanted to discuss my approach for issue JDK-8161664
(https://bugs.openjdk.java.net/browse/JDK-8161664) before I
started working on this issue.

In certain scenarios (see the JIRA issue for an example), the
Timer in the Animator inner class of the AquaProgressBarUI
class remains running, even when the JProgressBar has already
been removed from the UI. This causes a memory leak, as that
running Timer avoids that the JProgressBar can be GC-ed. As
long as the Timer is running, the JProgressBar is referenced
through

Timer -> ActionListener (=Animator inner class) ->
AquaProgressBarUI outer class -> JProgressBar field



I see two possible approaches to fix this:

1) I carefully investigate the particular scenario I found,
and try to figure out why the Timer is not stopped and fix
this particular scenario. This offers of course no guarantees
that there are no other scenarios which keep the Timer running.

2) I replace one of the hard references with a weak reference,
hence avoiding the memory leak in all cases.
If I do not attach the Animator inner class directly as
listener to the timer, but use another ActionListener which
only has a WeakReference to the Animator class, the memory
leak is solved.
The ActionListener could then stop the timer when the timer is
fired and the WeakReference#get returns null.



I prefer the second approach. By cutting the hard reference
between the Timer and the Animator + stopping the Timer when
the Animator is GC-ed, I ensure that the Timer cannot cause a
memory leak anymore. This avoids overlooking certain scenarios.

Any input on this ? Any preferences for a certain approach, or
proposal for another approach.

   Does other L (for example Metal) have the same memory leak
with the JProgressBar? If no, it would be interesting to know what
is the difference between them and the AquaProgressBarUI.

  Thanks,
  Alexandr.



Robin

Re: [9] Review request for 8160246: Regression: 4410243 reproducible with GTK LaF

[Alexandr Scherbatiy]

The fix looks good to me.

Thanks,
Alexandr.

On 7/19/2016 4:56 PM, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:

bug: https://bugs.openjdk.java.net/browse/JDK-8160246

webrev: http://cr.openjdk.java.net/~ssadetsky/8160246/webrev.00/

When the scrolled text component width is set to zero the text 
rootView is initialized and its preferred spans becomes unconstrained. 
But at the same time, the rootView preffered size is used to determine 
scroll bars visibility using getScrollableTracksViewportWidth/Height() 
and if unconstrained (and unwrapped) rootView height is less than the 
current viewport height then the vertical scroll bar is hidden which 
adds +15px to the viewport width and makes the rootView constrained in 
the next layout iteration which makes vertical scroll bar visible 
again... This layout cycle is infinite.


The solution is to initialize the rootView only once when text 
component is firstly lay-outed.


--Semyon

[9] Review request for 8160246: Regression: 4410243 reproducible with GTK LaF

[Semyon Sadetsky]

Hello,

Please review fix for JDK9:

bug: https://bugs.openjdk.java.net/browse/JDK-8160246

webrev: http://cr.openjdk.java.net/~ssadetsky/8160246/webrev.00/

When the scrolled text component width is set to zero the text rootView 
is initialized and its preferred spans becomes unconstrained. But at the 
same time, the rootView preffered size is used to determine scroll bars 
visibility using getScrollableTracksViewportWidth/Height() and if 
unconstrained (and unwrapped) rootView height is less than the current 
viewport height then the vertical scroll bar is hidden which adds +15px 
to the viewport width and makes the rootView constrained in the next 
layout iteration which makes vertical scroll bar visible again... This 
layout cycle is infinite.


The solution is to initialize the rootView only once when text component 
is firstly lay-outed.


--Semyon

Re: Input needed: JDK-8161664: Memory leak in com.apple.laf.AquaProgressBarUI: removed progress bar still referenced

[Robin Stevens]

Hello Alexandr,

very valid remark.
Running that same test program on Linux with the metal look and feel
reveals no memory leak. I have no access to a Windows machine, so I
couldn't get the Windows specific look and feel.

The other ProgressBarUI implementations seem to extend from
BasicProgressBarUI, which has the same mechanism of an Animator which uses
a Timer.
However, in the test program the Timer does not get started on Linux (while
it gets started on OS X).

In the BasicProgressBarUI class, all calls to startAnimationTimer are
wrapped with an if check:

if (progressBar.isDisplayable()) {
startAnimationTimer();
}

In the scenario from my test, the isDisplayable method returns false. On OS
X, this check is missing so the timer is started.

I assume adding that same check in the AquaProgressBarUI will fix the
problem as well. So that is a third approach to solve the issue.

Robin

On Tue, Jul 19, 2016 at 1:14 PM, Alexandr Scherbatiy <
alexandr.scherba...@oracle.com> wrote:

> On 7/19/2016 12:27 PM, Robin Stevens wrote:
>
>> Hello,
>>
>> I wanted to discuss my approach for issue JDK-8161664 (
>> https://bugs.openjdk.java.net/browse/JDK-8161664) before I started
>> working on this issue.
>>
>> In certain scenarios (see the JIRA issue for an example), the Timer in
>> the Animator inner class of the AquaProgressBarUI class remains running,
>> even when the JProgressBar has already been removed from the UI. This
>> causes a memory leak, as that running Timer avoids that the JProgressBar
>> can be GC-ed. As long as the Timer is running, the JProgressBar is
>> referenced through
>>
>> Timer -> ActionListener (=Animator inner class) -> AquaProgressBarUI
>> outer class -> JProgressBar field
>>
>>
>>
>> I see two possible approaches to fix this:
>>
>> 1) I carefully investigate the particular scenario I found, and try to
>> figure out why the Timer is not stopped and fix this particular scenario.
>> This offers of course no guarantees that there are no other scenarios which
>> keep the Timer running.
>>
>> 2) I replace one of the hard references with a weak reference, hence
>> avoiding the memory leak in all cases.
>> If I do not attach the Animator inner class directly as listener to the
>> timer, but use another ActionListener which only has a WeakReference to the
>> Animator class, the memory leak is solved.
>> The ActionListener could then stop the timer when the timer is fired and
>> the WeakReference#get returns null.
>>
>>
>>
>> I prefer the second approach. By cutting the hard reference between the
>> Timer and the Animator + stopping the Timer when the Animator is GC-ed, I
>> ensure that the Timer cannot cause a memory leak anymore. This avoids
>> overlooking certain scenarios.
>>
>> Any input on this ? Any preferences for a certain approach, or proposal
>> for another approach.
>>
>Does other L (for example Metal) have the same memory leak with the
> JProgressBar? If no, it would be interesting to know what is the difference
> between them and the AquaProgressBarUI.
>
>   Thanks,
>   Alexandr.
>
>>
>>
>> Robin
>>
>
>

Re: [9] Review request for 8160087: Change IOOBE to warning in the scenarios when it had not being thrown before the JDK-8078514

[Semyon Sadetsky]

On 19.07.2016 14:20, Alexandr Scherbatiy wrote:


The fix prints the warning method in case of wrong row sorter usage. 
How often this can happen? Could the large number of the messages 
overflow a user output?

In the FilePane this happened only once after the initial file list loading.

--Semyon


Thanks,
Alexandr.

On 7/19/2016 12:30 PM, Semyon Sadetsky wrote:



On 19.07.2016 12:18, Alexandr Scherbatiy wrote:

On 7/18/2016 11:46 AM, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:

bug: https://bugs.openjdk.java.net/browse/JDK-8160087

webrev: http://cr.openjdk.java.net/~ssadetsky/8160087/webrev.00/

A warning is added to avoid issues in user code to throw exceptions 
which were masked before. See bug descriptions for details.
  Should this behavior (which exists for long time) be specified in 
the 
DefaultRowSorter.convertRowIndexToView()/convertRowIndexToModel() 
javadoc?
This was not a 
DefaultRowSorter.convertRowIndexToView()/convertRowIndexToModel() 
issue. It was a mistake in the FilePane class.

RowSorter's javadoc mentions the correct way to use it:

The view invokes a model change method when the underlying model has 
changed. There may be order dependencies in how the events are 
delivered, so a RowSorter should not update its mapping until one of 
these methods is invoked.


--Semyon


  Thanks,
  Alexandr.


--Semyon

Re: [9] Review request for 8160087: Change IOOBE to warning in the scenarios when it had not being thrown before the JDK-8078514

[Alexandr Scherbatiy]

The fix prints the warning method in case of wrong row sorter usage. How 
often this can happen? Could the large number of the messages overflow a 
user output?


Thanks,
Alexandr.

On 7/19/2016 12:30 PM, Semyon Sadetsky wrote:



On 19.07.2016 12:18, Alexandr Scherbatiy wrote:

On 7/18/2016 11:46 AM, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:

bug: https://bugs.openjdk.java.net/browse/JDK-8160087

webrev: http://cr.openjdk.java.net/~ssadetsky/8160087/webrev.00/

A warning is added to avoid issues in user code to throw exceptions 
which were masked before. See bug descriptions for details.
  Should this behavior (which exists for long time) be specified in 
the DefaultRowSorter.convertRowIndexToView()/convertRowIndexToModel() 
javadoc?
This was not a 
DefaultRowSorter.convertRowIndexToView()/convertRowIndexToModel() 
issue. It was a mistake in the FilePane class.

RowSorter's javadoc mentions the correct way to use it:

The view invokes a model change method when the underlying model has 
changed. There may be order dependencies in how the events are 
delivered, so a RowSorter should not update its mapping until one of 
these methods is invoked.


--Semyon


  Thanks,
  Alexandr.


--Semyon

Re: [9] Review request for 8160087: Change IOOBE to warning in the scenarios when it had not being thrown before the JDK-8078514

[Semyon Sadetsky]

On 19.07.2016 12:18, Alexandr Scherbatiy wrote:

On 7/18/2016 11:46 AM, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:

bug: https://bugs.openjdk.java.net/browse/JDK-8160087

webrev: http://cr.openjdk.java.net/~ssadetsky/8160087/webrev.00/

A warning is added to avoid issues in user code to throw exceptions 
which were masked before. See bug descriptions for details.
  Should this behavior (which exists for long time) be specified in 
the DefaultRowSorter.convertRowIndexToView()/convertRowIndexToModel() 
javadoc?
This was not a 
DefaultRowSorter.convertRowIndexToView()/convertRowIndexToModel() issue. 
It was a mistake in the FilePane class.

RowSorter's javadoc mentions the correct way to use it:

The view invokes a model change method when the underlying model has 
changed. There may be order dependencies in how the events are 
delivered, so a RowSorter should not update its mapping until one of 
these methods is invoked.


--Semyon


  Thanks,
  Alexandr.


--Semyon

Input needed: JDK-8161664: Memory leak in com.apple.laf.AquaProgressBarUI: removed progress bar still referenced

[Robin Stevens]

Hello,

I wanted to discuss my approach for issue JDK-8161664 (
https://bugs.openjdk.java.net/browse/JDK-8161664) before I started working
on this issue.

In certain scenarios (see the JIRA issue for an example), the Timer in the
Animator inner class of the AquaProgressBarUI class remains running, even
when the JProgressBar has already been removed from the UI. This causes a
memory leak, as that running Timer avoids that the JProgressBar can be
GC-ed. As long as the Timer is running, the JProgressBar is referenced
through

Timer -> ActionListener (=Animator inner class) -> AquaProgressBarUI outer
class -> JProgressBar field



I see two possible approaches to fix this:

1) I carefully investigate the particular scenario I found, and try to
figure out why the Timer is not stopped and fix this particular scenario.
This offers of course no guarantees that there are no other scenarios which
keep the Timer running.

2) I replace one of the hard references with a weak reference, hence
avoiding the memory leak in all cases.
If I do not attach the Animator inner class directly as listener to the
timer, but use another ActionListener which only has a WeakReference to the
Animator class, the memory leak is solved.
The ActionListener could then stop the timer when the timer is fired and
the WeakReference#get returns null.



I prefer the second approach. By cutting the hard reference between the
Timer and the Animator + stopping the Timer when the Animator is GC-ed, I
ensure that the Timer cannot cause a memory leak anymore. This avoids
overlooking certain scenarios.

Any input on this ? Any preferences for a certain approach, or proposal for
another approach.


Robin

Re: [9] Review request for 8160087: Change IOOBE to warning in the scenarios when it had not being thrown before the JDK-8078514

[Alexandr Scherbatiy]

On 7/18/2016 11:46 AM, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:

bug: https://bugs.openjdk.java.net/browse/JDK-8160087

webrev: http://cr.openjdk.java.net/~ssadetsky/8160087/webrev.00/

A warning is added to avoid issues in user code to throw exceptions 
which were masked before. See bug descriptions for details.
  Should this behavior (which exists for long time) be specified in the 
DefaultRowSorter.convertRowIndexToView()/convertRowIndexToModel() javadoc?


  Thanks,
  Alexandr.


--Semyon

Re: [9] Fix for JDK-7096375 : Swing ignores first click after decreasing system's time

[Alexandr Scherbatiy]

On 7/18/2016 3:27 PM, Ajit Ghaisas wrote:

Hi,

Bug :
 https://bugs.openjdk.java.net/browse/JDK-7096375
 Swing ignores first click after decreasing system's time.

Fix :
 BasicButtonListener keeps track of the last time when a button is pressed.
 This is used while discarding mouse press events to handle 
multiClickThreshold.
 The condition to discard mouse press event is corrected.

Webrev :
 http://cr.openjdk.java.net/~aghaisas/7096375/webrev.00/

 Request you to review.
  The template used for the test is rather old. It is better to use 
CountDownLatch for the manual test synchronization.

  Could you rewrite the test using the TitledBorderTest as a sample:
http://hg.openjdk.java.net/jdk9/client/jdk/file/233b59b7ea2f/test/javax/swing/LookAndFeel/6439354/TitledBorderTest.java

  There can be added two simple changes. The thread creation is not 
necessary because the runnable can be  directly executed by 
SwingUtilities.invokeAndWait(). The timeout can be added to the 
latch.await() call.


  Thanks,
  Alexandr.


Regards,
Ajit

Re: RfR JDK-8161483 Implement AccessibleAction interface in JList.AccessibleJList.AccessibleJListChild

[Alexandr Scherbatiy]

The fix looks good to me.

Thanks,
Alexandr.

On 7/19/2016 5:10 AM, Pete Brunet wrote:

Please review the following:

Bug: https://bugs.openjdk.java.net/browse/JDK-8161483
Patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8161483/webrev.00/

This is a followon to the patch for
JDK-8145207 [macosx] JList, VO can't access non-visible list items

In order to fixJDK-8145207the AccessibleAction interface was needed 
for JList.AccessibleJList.AccessibleJListChild but a backport of this 
fix has been requested and the released public API can not be changed 
in 8u or earlier.  The workaround for JDK-8145207 is to create and use 
a private subclass of JList.AccessibleJList.AccessibleJListChild, 
JList.AccessibleJList.ActionableAccessibleJListChild. The downside of 
this fix is that it returns a subclass of the 
JList.AccessibleJList.AccessibleJListChild.  If a user overrides the 
class and returns from its code it will not inherit the 
AccessibleAction behavior. For JDK 9 the 
ActionableAccessibleJListChild subclass should be removed and the 
AccessibleAction implementation moved to 
JList.AccessibleJList.AccessibleJListChild.


TiA,
Pete 

Re: 8161470: [TEST_BUG] Failure javax/swing/JRadioButton/FocusTraversal/FocusTraversal.java

[Alexandr Scherbatiy]

The fix looks good to me.

Thanks,
Alexandr.

On 7/19/2016 9:45 AM, Avik Niyogi wrote:

Hi All,

Kindly review the fix for JDK9.

*Bug: https://bugs.openjdk.java.net/browse/JDK-8161470*

*Webrev: http://cr.openjdk.java.net/~aniyogi/8161470/webrev.00/ 
*


*Issue: *This test consistently (12/12) fails on OS X 10.10 and also 
fails on Ubuntu 14.04.


*Cause: * Due to quirk in robot.delay the UI was not getting 
appropriate behaviour response.


*Fix:* Appropriate Robot methods were used to fix this.

With Regards,
Avik Niyogi

Re: 8160438: [PIT][macosx] [TEST_BUG] javax/swing/plaf/nimbus/8057791/bug8057791.java fails

[Rajeev Chamyal]

Looks good to me.

 

Regards,

Rajeev Chamyal

 

From: Avik Niyogi 
Sent: 19 July 2016 12:12
To: Alexandr Scherbatiy
Cc: Rajeev Chamyal; Semyon Sadetsky; Yuri Nesterenko; swing-dev@openjdk.java.net
Subject: Re:  8160438: [PIT][macosx] [TEST_BUG] 
javax/swing/plaf/nimbus/8057791/bug8057791.java fails

 

Hi All,

Another gentle reminder. Please review the changes made.

On 15-Jul-2016, at 2:50 pm, Avik Niyogi mailto:avik.niy...@oracle.com"avik.niy...@oracle.com> wrote:

 

A gentle reminder, please review the changes

 

On 14-Jul-2016, at 8:46 pm, Alexandr Scherbatiy mailto:alexandr.scherba...@oracle.com"alexandr.scherba...@oracle.com> wrote:

 


The fix looks good to me.

Thanks,
Alexandr.

On 7/14/2016 9:53 AM, Avik Niyogi wrote:

Hi All, 

Please find my webrev below for review which includes changes as perceived from 
inputs provided.

 

HYPERLINK 
"http://cr.openjdk.java.net/%7Eaniyogi/8160438/webrev.02/"http://cr.openjdk.java.net/~aniyogi/8160438/webrev.02/

 

With Regards,

Avik Niyogi

On 12-Jul-2016, at 7:12 pm, Alexandr Scherbatiy mailto:alexandr.scherba...@oracle.com"alexandr.scherba...@oracle.com> wrote:

 

On 7/12/2016 8:24 AM, Avik Niyogi wrote:



Hi All,
A gentle reminder, please review my code changes.

With Regards,
Avik Niyogi




On 08-Jul-2016, at 4:24 pm, Yuri Nesterenko mailto:yuri.nestere...@oracle.com"yuri.nestere...@oracle.com> wrote:

It pass for me if I provide some time to process native events
after the user activity simulation. For instance,
setAutoDelay(50) for robot does that plus waitForIdle()
after every mouseMove(). In this case, the part with that additional
check and a (misleading, I think) comment about the color profiles
may be removed. The test would take much more time though, and
  @run main/timeout=600 bug8057791
line would be necessary.

   Some more comments to the previous ones:
 - runColorTestCase() uses JList and should be run on EDT
 - "if (tryNimbusLookAndFeel()) {" It is supposed that the Nimbus L is 
supported on all platforms. May be it is better to fail the test if the Nimbus 
L is not set.
 - "if (osName.contains("Mac")) { isMac = true; }" can be simplified to "return 
osName.contains("Mac")"
 - " if (!"".equals(errorString)) {" can be simplified to !errorString.isEmpty()

 Thanks,
 Alexandr.




Thanks,
-yan

On 07/08/2016 04:28 AM, Avik Niyogi wrote:



The test does not pass if mac specific check is not done for
backgroundcolor.
The check is required to get the same values from BufferedImage as they
are the same as found with Digital Color Meter.
This test case fixes that.
Please provide inputs if this fix will get a +1 or if not any positive
inputs to modify the test case will be welcome.

With Regards,
Avik Niyogi



On 07-Jul-2016, at 9:51 pm, Semyon Sadetsky
mailto:semyon.sadet...@oracle.com"semyon.sadet...@oracle.com 
> wrote:



On 7/7/2016 6:30 PM, Yuri Nesterenko wrote:



On 07/07/2016 06:15 PM, Semyon Sadetsky wrote:




On 7/7/2016 5:58 PM, Yuri Nesterenko wrote:



On 07/07/2016 05:35 PM, Yuri Nesterenko wrote:



On 07/07/2016 05:04 PM, Semyon Sadetsky wrote:




On 07.07.2016 16:35, Avik Niyogi wrote:



Hi Semyon,

Thank you for the review comment.

In Mac OS X, *System Preferences > Displays > Colors > Display
Profile*  section, the default value is *Color LCD*.
This causes a failure in some test cases which uses robot.The colour
configuration it expects to use is the *Generic RGB Profile*.
That is what "Non-generic display settings" means.

AFAIK there are instruction that tests should be executed using color
profile with no color corrections on OS X. I guess there are many
other
tests that fail with color correction.
If this is a root cause than the bug doesn't need to be fixed.

Well, I filed this bug and I used Generic RGB on all my
test machines. There may be additional precautions in the tests
about that but misconfiguration is not the root case here.
That said, I feel vary about attempts to guess Apple colors

  wary I mean



in non-generic profiles.

Yuri. Do you mean that the non-generic is not a root case?

No: I had Generic RGB everywhere, and it failed for me.
I should say that the new version of the test properly
fails with b120 and pass with current PIT. And colors
are visibly different in the two builds: so the test works OK now.

That contradicts with the description of the fix.
Probably the test does unnecessary care about the non-Generic profiles.

159 if (!foundMatch && isMac()) {
160 //One more chance for Mac due to non-Generic
display setting
161 detectedColor = new Color(img.getRGB(x, y), true);
162 if (detectedColor.equals(colorCheck)) {
163 foundMatch = true;
164 break checkLoops;
165 }
166 }

Does it mean that the code above, which is necessary to serve
non-Generic profiles on OS X, may be 

8161470: [TEST_BUG] Failure javax/swing/JRadioButton/FocusTraversal/FocusTraversal.java

[Avik Niyogi]

Hi All,

Kindly review the fix for JDK9.

Bug: https://bugs.openjdk.java.net/browse/JDK-8161470 


Webrev: http://cr.openjdk.java.net/~aniyogi/8161470/webrev.00/ 


Issue: This test consistently (12/12) fails on OS X 10.10 and also fails on 
Ubuntu 14.04.

Cause:  Due to quirk in robot.delay the UI was not getting appropriate 
behaviour response.

Fix: Appropriate Robot methods were used to fix this.

With Regards,
Avik Niyogi

Re: 8160438: [PIT][macosx] [TEST_BUG] javax/swing/plaf/nimbus/8057791/bug8057791.java fails

[Avik Niyogi]

Hi All,
Another gentle reminder. Please review the changes made.
> On 15-Jul-2016, at 2:50 pm, Avik Niyogi  wrote:
> 
> A gentle reminder, please review the changes
> 
>> On 14-Jul-2016, at 8:46 pm, Alexandr Scherbatiy 
>> > 
>> wrote:
>> 
>> 
>> The fix looks good to me.
>> 
>> Thanks,
>> Alexandr.
>> 
>> On 7/14/2016 9:53 AM, Avik Niyogi wrote:
>>> Hi All,
>>> Please find my webrev below for review which includes changes as perceived 
>>> from inputs provided.
>>> 
>>> http://cr.openjdk.java.net/~aniyogi/8160438/webrev.02/ 
>>> 
>>> 
>>> With Regards,
>>> Avik Niyogi
 On 12-Jul-2016, at 7:12 pm, Alexandr Scherbatiy 
 > 
 wrote:
 
 On 7/12/2016 8:24 AM, Avik Niyogi wrote:
> Hi All,
> A gentle reminder, please review my code changes.
> 
> With Regards,
> Avik Niyogi
> 
>> On 08-Jul-2016, at 4:24 pm, Yuri Nesterenko > > wrote:
>> 
>> It pass for me if I provide some time to process native events
>> after the user activity simulation. For instance,
>> setAutoDelay(50) for robot does that plus waitForIdle()
>> after every mouseMove(). In this case, the part with that additional
>> check and a (misleading, I think) comment about the color profiles
>> may be removed. The test would take much more time though, and
>>   @run main/timeout=600 bug8057791
>> line would be necessary.
Some more comments to the previous ones:
  - runColorTestCase() uses JList and should be run on EDT
  - "if (tryNimbusLookAndFeel()) {" It is supposed that the Nimbus L is 
 supported on all platforms. May be it is better to fail the test if the 
 Nimbus L is not set.
  - "if (osName.contains("Mac")) { isMac = true; }" can be simplified to 
 "return osName.contains("Mac")"
  - " if (!"".equals(errorString)) {" can be simplified to 
 !errorString.isEmpty()
 
  Thanks,
  Alexandr.
>> 
>> Thanks,
>> -yan
>> 
>> On 07/08/2016 04:28 AM, Avik Niyogi wrote:
>>> The test does not pass if mac specific check is not done for
>>> backgroundcolor.
>>> The check is required to get the same values from BufferedImage as they
>>> are the same as found with Digital Color Meter.
>>> This test case fixes that.
>>> Please provide inputs if this fix will get a +1 or if not any positive
>>> inputs to modify the test case will be welcome.
>>> 
>>> With Regards,
>>> Avik Niyogi
 On 07-Jul-2016, at 9:51 pm, Semyon Sadetsky
  
 >> wrote:
 
 
 
 On 7/7/2016 6:30 PM, Yuri Nesterenko wrote:
> On 07/07/2016 06:15 PM, Semyon Sadetsky wrote:
>> 
>> On 7/7/2016 5:58 PM, Yuri Nesterenko wrote:
>>> On 07/07/2016 05:35 PM, Yuri Nesterenko wrote:
 On 07/07/2016 05:04 PM, Semyon Sadetsky wrote:
> 
> On 07.07.2016 16:35, Avik Niyogi wrote:
>> Hi Semyon,
>> 
>> Thank you for the review comment.
>> 
>> In Mac OS X, *System Preferences > Displays > Colors > Display
>> Profile*  section, the default value is *Color LCD*.
>> This causes a failure in some test cases which uses robot.The 
>> colour
>> configuration it expects to use is the *Generic RGB Profile*.
>> That is what “Non-generic display settings” means.
> AFAIK there are instruction that tests should be executed using 
> color
> profile with no color corrections on OS X. I guess there are many
> other
> tests that fail with color correction.
> If this is a root cause than the bug doesn't need to be fixed.
 Well, I filed this bug and I used Generic RGB on all my
 test machines. There may be additional precautions in the tests
 about that but misconfiguration is not the root case here.
 That said, I feel vary about attempts to guess Apple colors
>>>   wary I mean
 in non-generic profiles.
>> Yuri. Do you mean that the non-generic is not a root case?
> No: I had Generic RGB everywhere, and it failed for me.
> I should say that the new version of the test properly
> fails with b120 and pass with current PIT. And colors
> are visibly different in the two builds: so the test works OK now.
 That contradicts with the description of the fix.
 Probably the test