Re: Review Request of 8146321: [macosx] JInternalFrame frame icon in wrong position on Mac L if icon is not ImageIcon

2016-02-07 Thread Sergey Bylokhov 
Both updated methods have a typo, 0 will be returned if 
frame.getFrameIcon()==null:


protected int getIconWidth(final JInternalFrame frame) {
int width = 0;

Icon icon = frame.getFrameIcon();
if (icon == null) {
icon = UIManager.getIcon("InternalFrame.icon");
}
else {
width = Math.min(icon.getIconWidth(), sMaxIconWidth);
}

return width;
}

protected int getIconHeight(final JInternalFrame frame) {
int height = 0;

Icon icon = frame.getFrameIcon();
if (icon == null) {
icon = UIManager.getIcon("InternalFrame.icon");
}
else{
height = Math.min(icon.getIconHeight(), sMaxIconHeight);
}

return height;
}

Note that getIconWidth/getIconHeight should return width/height of the 
icon, but this is not true anymore, because paintTitleIcon() now use the 
aspect ratio, which can produce the different Width/Height for the same 
icon.



On 04.02.16 12:26, Alexandr Scherbatiy wrote:


   The fix looks good to me.

   Thanks,
   Alexandr.

On 2/3/2016 8:51 PM, Avik Niyogi wrote:

Hi All,
Please review the code changes made as per the inputs provided:
http://cr.openjdk.java.net/~aniyogi/8146321/webrev.05/


With Regards,
Avik Niyogi


On 03-Feb-2016, at 7:38 pm, Alexandr Scherbatiy
> wrote:

On 2/3/2016 12:47 AM, Avik Niyogi wrote:

Hi All,
Please review the code changes made as per the inputs provided:
http://cr.openjdk.java.net/~aniyogi/8146321/webrev.04


  326 g2.translate(0, 0);
  The translation to the zero vector leaves the coordinate system the
same.

  327 float xScaleFactor = (float) sMaxIconWidth /
icon.getIconWidth();
  It is better to use double values because the graphics transform
methods use them.

  332 g2.scale(scaleFactorAspectRatio,
scaleFactorAspectRatio);
  333 g2.translate(x / scaleFactorAspectRatio, y /
scaleFactorAspectRatio);
  Is it possible to use the translation first and the scale the
second? I this case where no need to re-scale translation coordinates.

  Thanks,
  Alexandr.



With Regards,
Avik Niyogi

On 02-Feb-2016, at 5:55 pm, Alexandr Scherbatiy
<alexandr.scherba...@oracle.com>
wrote:

On 2/2/2016 3:41 AM, Avik Niyogi wrote:

Hi Alexander,


On 02-Feb-2016, at 3:44 pm, Alexandr Scherbatiy
 wrote:

On 2/2/2016 1:50 AM, Avik Niyogi wrote:

Hi All,
Please review the code changes made as per the inputs provided:
cr.openjdk.java.net/~aniyogi/8146321/webrev.03



 -  Will it work with custom implementation of the Icon interface
which just draws an image?
  For example:
 --
 public class DukeIcon implements Icon {

private BufferedImage dukeImage;

public DukeIcon() throws IOException {
dukeImage = ImageIO.read(new File(""));
}

@Override
public void paintIcon(Component c, Graphics g, int x, int y) {
g.drawImage(dukeImage, x, y, c);
}

@Override
public int getIconWidth() {
return dukeImage.getWidth();
}

@Override
public int getIconHeight() {
return dukeImage.getHeight();
}
}


This is a limitation for custom Icons because they will need to
use toe drawImage with appropriate implementation.
To fix that will be a major change and may need change in the
implementation of drawImage method.


  It looks like the code below from the fix doesn't work for the
ImageIcon because x and y are now scaled. Is it possible to apply
some other transformations (may be some translations) to the
graphics to draw the image at the right position?
---
 334 g2.scale((float) sMaxIconWidth /
icon.getIconWidth(),
 335 (float) sMaxIconWidth /
icon.getIconHeight());
 336 icon.paintIcon(frame, g2, x, y);
---

--

- "(icon != null && (icon instanceof Icon))"
  Could the check to null also be omitted here?
  In other words, are the "(icon != null && (icon instanceof
Icon))" and "(icon instanceof Icon)" checks return the same result?


If we remove the check, the cases where custom ImageIcon have no
images will fail.


The provided example  should work with the check: "(icon
instanceof Icon)" in the same way as with the check "(icon != null
&& (icon instanceof Icon))" because the used icon is not null and
it implements Icon interface, should not it?

 Thanks,
 Alexandr.


Example:


import java.awt.*;
import javax.swing.*;

public class JInternalFrameBug {

   public static void main(String[] args) {
SwingUtilities.invokeLater(
new Runnable() {
   public void run() {

Re: Review request for JDK-8131751: Test javax/swing/plaf/gtk/crash/RenderBadPictureCrash.java fails UnsupportedOperationException

2016-02-07 Thread Vikrant Agarwal 
Friendly  Reminder.

 

From: Alexander Scherbatiy 
Sent: Monday, February 01, 2016 6:05 PM
To: Vikrant Agarwal; swing-dev@openjdk.java.net
Subject: Re:  Review request for JDK-8131751: Test 
javax/swing/plaf/gtk/crash/RenderBadPictureCrash.java fails 
UnsupportedOperationException

 


 The fix looks good to me.

  Thanks,
  Alexandr.

On 2/1/2016 4:32 AM, Vikrant Agarwal wrote:

Thanks Alexander, I have updated the webrev for the same.

Updated Webrev:  HYPERLINK 
"http://cr.openjdk.java.net/%7Entv/vikrant/8131751/webrev.01/"http://cr.openjdk.java.net/~ntv/vikrant/8131751/webrev.01/

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

 

Best Regards,

Vikrant

 

From: Alexander Scherbatiy 
Sent: Thursday, January 28, 2016 11:13 PM
To: Vikrant Agarwal; HYPERLINK 
"mailto:swing-dev@openjdk.java.net"swing-dev@openjdk.java.net
Subject: Re:  Review request for JDK-8131751: Test 
javax/swing/plaf/gtk/crash/RenderBadPictureCrash.java fails 
UnsupportedOperationException

 


The fix looks good to me.

Just a small note: it could be better to get the graphics device from the frame 
itself (f.getGraphicsConfiguration().getDevice()).

Thanks,
Alexandr.

On 1/28/2016 2:32 AM, Vikrant Agarwal wrote:




Hi All,

Kindly review the fix for JDK9.

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

Webrev: HYPERLINK 
"http://cr.openjdk.java.net/%7Esrastogi/8131751/webrev.00/"http://cr.openjdk.java.net/~srastogi/8131751/webrev.00/

Issue: [TEST_BUG] Test javax/swing/plaf/gtk/crash/RenderBadPictureCrash.java 
fails UnsupportedOperationException

Cause: test failed on those systems where PERPIXEL_TRANSLUCENT translucency is 
not supported as it is doing  f.setBackground(new Color(0, 0, 0, 0)); (making 
alpha value 0)

Fix: Added a check for checking support for PERPIXEL_TRANSLUCENT translucency 
before f.setBackground(new Color(0, 0, 0, 0));

Best Regards,

Vikrant Agarwal

Re: Review Request of 8146321: [macosx] JInternalFrame frame icon in wrong position on Mac L if icon is not ImageIcon

2016-02-07 Thread Avik Niyogi 
Hi Sergey,
Please review webrev with changes as per the inputs provided:
cr.openjdk.java.net/~aniyogi/8146321/webrev.06/ 


With Regards,
Avik Niyogi

> On 08-Feb-2016, at 2:29 am, Sergey Bylokhov  
> wrote:
> 
> Both updated methods have a typo, 0 will be returned if 
> frame.getFrameIcon()==null:
> 
>protected int getIconWidth(final JInternalFrame frame) {
>int width = 0;
> 
>Icon icon = frame.getFrameIcon();
>if (icon == null) {
>icon = UIManager.getIcon("InternalFrame.icon");
>}
>else {
>width = Math.min(icon.getIconWidth(), sMaxIconWidth);
>}
> 
>return width;
>}
> 
>protected int getIconHeight(final JInternalFrame frame) {
>int height = 0;
> 
>Icon icon = frame.getFrameIcon();
>if (icon == null) {
>icon = UIManager.getIcon("InternalFrame.icon");
>}
>else{
>height = Math.min(icon.getIconHeight(), sMaxIconHeight);
>}
> 
>return height;
>}
> 
> Note that getIconWidth/getIconHeight should return width/height of the icon, 
> but this is not true anymore, because paintTitleIcon() now use the aspect 
> ratio, which can produce the different Width/Height for the same icon.
> 
> 
> On 04.02.16 12:26, Alexandr Scherbatiy wrote:
>> 
>>   The fix looks good to me.
>> 
>>   Thanks,
>>   Alexandr.
>> 
>> On 2/3/2016 8:51 PM, Avik Niyogi wrote:
>>> Hi All,
>>> Please review the code changes made as per the inputs provided:
>>> http://cr.openjdk.java.net/~aniyogi/8146321/webrev.05/ 
>>> 
>>> >> >
>>> 
>>> With Regards,
>>> Avik Niyogi
>>> 
 On 03-Feb-2016, at 7:38 pm, Alexandr Scherbatiy
 
 >> wrote:
 
 On 2/3/2016 12:47 AM, Avik Niyogi wrote:
> Hi All,
> Please review the code changes made as per the inputs provided:
> http://cr.openjdk.java.net/~aniyogi/8146321/webrev.04 
> 
>  >
  326 g2.translate(0, 0);
  The translation to the zero vector leaves the coordinate system the
 same.
 
  327 float xScaleFactor = (float) sMaxIconWidth /
 icon.getIconWidth();
  It is better to use double values because the graphics transform
 methods use them.
 
  332 g2.scale(scaleFactorAspectRatio,
 scaleFactorAspectRatio);
  333 g2.translate(x / scaleFactorAspectRatio, y /
 scaleFactorAspectRatio);
  Is it possible to use the translation first and the scale the
 second? I this case where no need to re-scale translation coordinates.
 
  Thanks,
  Alexandr.
 
> 
> With Regards,
> Avik Niyogi
>> On 02-Feb-2016, at 5:55 pm, Alexandr Scherbatiy
>> <> >alexandr.scherba...@oracle.com 
>> >
>> wrote:
>> 
>> On 2/2/2016 3:41 AM, Avik Niyogi wrote:
>>> Hi Alexander,
>>> 
 On 02-Feb-2016, at 3:44 pm, Alexandr Scherbatiy
 > wrote:
 
 On 2/2/2016 1:50 AM, Avik Niyogi wrote:
> Hi All,
> Please review the code changes made as per the inputs provided:
> cr.openjdk.java.net/~aniyogi/8146321/webrev.03 
> 
>  >
 
 -  Will it work with custom implementation of the Icon interface
 which just draws an image?
  For example:
 --
 public class DukeIcon implements Icon {
 
private BufferedImage dukeImage;
 
public DukeIcon() throws IOException {
dukeImage = ImageIO.read(new File(">>> image>"));
}
 
@Override
public void paintIcon(Component c, Graphics g, int x, int y) {
g.drawImage(dukeImage, x, y, c);
}
 
@Override
public int getIconWidth() {
return dukeImage.getWidth();
}
 
@Override
public int getIconHeight() {
return dukeImage.getHeight();