Re: Fix for JDK-8065861 : Pressing Esc does not set 'canceled' property of ProgressMonitor

[Ajit Ghaisas]

Hi,

Thanks Alex for spotting probable exceptions in code changes.   
 I have corrected the code to address them.

 Here is the updated webrev. Request you to review.
 http://cr.openjdk.java.net/~aghaisas/8065861/webrev.01/

Regards,
Ajit
   

-Original Message-
From: Alexandr Scherbatiy 
Sent: Thursday, June 09, 2016 8:56 PM
To: Ajit Ghaisas; Sergey Bylokhov; Rajeev Chamyal; swing-dev@openjdk.java.net
Subject: Re: Fix for JDK-8065861 : Pressing Esc does not set 'canceled' 
property of ProgressMonitor

On 6/8/2016 5:35 PM, Ajit Ghaisas wrote:
> Hi,
>
> Bug :
> https://bugs.openjdk.java.net/browse/JDK-8065861
>
> Issue :
> Pressing Esc does not set 'canceled' property of ProgressMonitor
>
> Analysis :
> ProgressMonitor option pane only gets hidden on pressing Escape key.
> It is not truly canceled as isCanceled() method continues to return false.
>
> Fix :
> On Pressing Escape key JOptionPane.CLOSED_OPTION value is set in JOptionPane 
> from BasicOptionPaneUI.java.
> This value is used as a condition in isCanceled() to identify ProgressMonitor 
> is canceled by pressing Escape key.
>
> Please review the webrev:
> http://cr.openjdk.java.net/~aghaisas/8065861/webrev.00/
  The updated code calls methods from 'v' object before it is checked to 
the null which can lead to the NPE. The cancelOption[0] also can throw the IOBE 
before the arrays length checking.

   Thanks,
   Alexandr.
> On review completion, I will raise a CCC request for documentation change.
>
> Regards,
> Ajit

Re: Background is painted taller than needed for superscripted text

[Alexandr Scherbatiy]

On 6/8/2016 2:30 PM, Prem Balakrishnan wrote:


Hi,

Please review fix for JDK9,

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

*Webrev:* http://cr.openjdk.java.net/~pkbalakr/8017266/webrev.00/ 



*Issue:*

Background is painted taller than needed for superscripted text.

*Cause:*

Bounds(alloc.height) is used to set the height to fill the actual 
glyphs boundary


Why passed shape height is larger that the required superscripted 
text height?


  Thanks,
  Alexandr.


*Fix:*

Used painter.getHeight() instead of alloc.height to fill the actual 
glyphs boundary


Thanks,

Prem

Re: Fix for JDK-8065861 : Pressing Esc does not set 'canceled' property of ProgressMonitor

[Alexandr Scherbatiy]

On 6/8/2016 5:35 PM, Ajit Ghaisas wrote:

Hi,

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

Issue :
Pressing Esc does not set 'canceled' property of ProgressMonitor

Analysis :
ProgressMonitor option pane only gets hidden on pressing Escape key.
It is not truly canceled as isCanceled() method continues to return false.

Fix :
On Pressing Escape key JOptionPane.CLOSED_OPTION value is set in JOptionPane 
from BasicOptionPaneUI.java.
This value is used as a condition in isCanceled() to identify ProgressMonitor 
is canceled by pressing Escape key.

Please review the webrev:
http://cr.openjdk.java.net/~aghaisas/8065861/webrev.00/
 The updated code calls methods from 'v' object before it is 
checked to the null which can lead to the NPE. The cancelOption[0] also 
can throw the IOBE before the arrays length checking.


  Thanks,
  Alexandr.

On review completion, I will raise a CCC request for documentation change.

Regards,
Ajit

Re: [9] Review request for 8156185: JDK-8024858 (long tooltip delay) is not fixed but is easily fixed

[Alexandr Scherbatiy]

The fix looks good to me.

Thanks,
Alexandr.

On 6/8/2016 11:08 AM, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:

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

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

Scan through the GraphicsDevice#getConfigurations() may take 
significant time on some hardware configurations. But actually it is 
not needed to find the device configuration for the tooltip, because 
only the information about device bounds/insets is used then. So, the 
scan is replaced with device.getDefaultConfiguration().


--Semyon

Re: [9] Review request for JDK-8150176 [hidpi] wrong resolution variant of multi-res. image is used for TrayIcon

[Alexandr Scherbatiy]

On 6/9/2016 11:55 AM, Rajeev Chamyal wrote:


Hello All,

Please review the following fix.

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

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



Issue: Wrong resolution variant image is used in Tray Icon.

Fix : Applying the device transform to graphics object to select the 
correct image.


The image could be cropped on Linux because the high resolution 
icon which size is bigger that the original image is drawn to the 
buffered image with un-scaled size curW x CurH.
  It is better to get a resolution variant from the multi-resolution 
image, draw it to a buffered image with the same scaled size and then 
draw the buffered image to the paint graphics using original size:

---
Image resolutionVariant = ((MultiResolutionImage) 
image).getResolutionVariant(scaleX * curW, scaleY * curH);
BufferedImage bufImage = new BufferedImage(scaleX * curW, scaleY * 
curH, BufferedImage.TYPE_INT_ARGB);

// ...
gr.drawImage(image, 0, 0, scaleX * curW, scaleY * curH, observer);
// ...
g.drawImage(bufImage, 0, 0, curW, curH, observer); // non scaled 
width and height

---

  By the way, is the buffered image necessary in this case? Is it 
possible to draw the image directly to the paint graphics?

---
 g.drawImage(image, 0, 0, curW, curH, null);
---

Thanks,
Alexandr.


Regards,

Rajeev Chamyal

Re: [9] Review request for 8158734 JEditorPane.createEditorKitForContentType throws NPE after 6882559

[mikhail cherkasov]

On 6/9/2016 12:40 PM, Alexey Ivanov wrote:


Except for:

44 static public class MyEditorKit extends EditorKit {

Could you please use blessed keyword order: public static?

sure, I'll fix it.


And in the error message:
"Class loader was not been removed for '"…

Remove “been” as it's out of its place in this sentence.

right, it should be *has* not been removed.

Re: [9] Review request for 8158734 JEditorPane.createEditorKitForContentType throws NPE after 6882559

[Alexey Ivanov]

Hi Mikhail,

Looks fine.

Except for:

44 static public class MyEditorKit extends EditorKit {

Could you please use blessed keyword order: public static?

And in the error message:
"Class loader was not been removed for '"…

Remove “been” as it's out of its place in this sentence.


No need for additional webrev, obviously.


Regards,
Alexey

On 08.06.2016 20:23, mikhail cherkasov wrote:

The testcase was modified to check that we removes classloader:
http://cr.openjdk.java.net/~mcherkas/8158734/9/webrev.04/

On 6/8/2016 6:00 PM, Sergey Bylokhov wrote:

On 08.06.16 17:58, Alexey Ivanov wrote:

I think it should. Consider the following code:

registerEditorKitForContentType("test/test", "com.oracle.Test",
);

registerEditorKitForContentType("test/test", "com.sun.Test", null);

If the classloader is not removed from KitLoaderRegistry, then class
"com.sun.Test" would be loaded using the wrong class loader which was
set for class "com.oracle.Test".


Thanks for clarification! Then I suggest to update the test to be 
sure that this use-case works properly.






On 6/8/2016 3:16 PM, Alexander Potochkin wrote:

Hello Mikhail

This version looks good to me

Thanks
alexp


Hi Sergey,

you are right, it's easier to check classloader for null before to
add it to hashtable:
http://cr.openjdk.java.net/~mcherkas/8158734/9/webrev.01/src/java.desktop/share/classes/javax/swing/JEditorPane.java.udiff.html 





On 6/7/2016 10:51 PM, Sergey Bylokhov wrote:

HI, Mikhail.
Can you please clarify the reason of usage Optional here if the
optional itself can be null?

On 07.06.16 22:05, mikhail cherkasov wrote:

Hello,

Could you please review the fix for:
https://bugs.openjdk.java.net/browse/JDK-8158734
 webrev: 
http://cr.openjdk.java.net/~mcherkas/8158734/9/webrev.00/


A simple null check was added.

Thanks,
Mikhail.

Re: [9] Review request for 8158734 JEditorPane.createEditorKitForContentType throws NPE after 6882559

[Sergey Bylokhov]

Looks fine.

On 08.06.16 20:23, mikhail cherkasov wrote:

The testcase was modified to check that we removes classloader:
http://cr.openjdk.java.net/~mcherkas/8158734/9/webrev.04/

On 6/8/2016 6:00 PM, Sergey Bylokhov wrote:

On 08.06.16 17:58, Alexey Ivanov wrote:

I think it should. Consider the following code:

registerEditorKitForContentType("test/test", "com.oracle.Test",
);

registerEditorKitForContentType("test/test", "com.sun.Test", null);

If the classloader is not removed from KitLoaderRegistry, then class
"com.sun.Test" would be loaded using the wrong class loader which was
set for class "com.oracle.Test".


Thanks for clarification! Then I suggest to update the test to be sure
that this use-case works properly.





On 6/8/2016 3:16 PM, Alexander Potochkin wrote:

Hello Mikhail

This version looks good to me

Thanks
alexp


Hi Sergey,

you are right, it's easier to check classloader for null before to
add it to hashtable:
http://cr.openjdk.java.net/~mcherkas/8158734/9/webrev.01/src/java.desktop/share/classes/javax/swing/JEditorPane.java.udiff.html




On 6/7/2016 10:51 PM, Sergey Bylokhov wrote:

HI, Mikhail.
Can you please clarify the reason of usage Optional here if the
optional itself can be null?

On 07.06.16 22:05, mikhail cherkasov wrote:

Hello,

Could you please review the fix for:
https://bugs.openjdk.java.net/browse/JDK-8158734
 webrev: http://cr.openjdk.java.net/~mcherkas/8158734/9/webrev.00/

A simple null check was added.

Thanks,
Mikhail.
























--
Best regards, Sergey.

[9] Review request for JDK-8150176 [hidpi] wrong resolution variant of multi-res. image is used for TrayIcon

[Rajeev Chamyal]

Hello All,

 

Please review the following fix.

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

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

 

Issue: Wrong resolution variant image is used in Tray Icon.

Fix : Applying the device transform to graphics object to select the correct 
image.

 

Regards,

Rajeev Chamyal