<Swing Dev> Review Request of 8146321: [macosx] JInternalFrame frame icon in wrong position on Mac L&F if icon is not ImageIcon
Rajeev Chamyal
rajeev.chamyal at oracle.com
Wed Feb 10 06:04:16 UTC 2016
The fix looks good to me.
Regards,
Rajeev Chamyal
From: Avik Niyogi
Sent: 09 February 2016 11:43
To: Sergey Bylokhov; Alexandr Scherbatiy; Rajeev Chamyal
Cc: swing-dev at openjdk.java.net
Subject: Re: <Swing Dev> Review Request of 8146321: [macosx] JInternalFrame frame icon in wrong position on Mac L&F if icon is not ImageIcon
A gentle reminder. Please review webrev with changes as per the inputs provided:
HYPERLINK "http://cr.openjdk.java.net/~aniyogi/8146321/webrev.06/"cr.openjdk.java.net/~aniyogi/8146321/webrev.06/
With Regards,
Avik Niyogi
On 08-Feb-2016, at 12:42 pm, Avik Niyogi <HYPERLINK "mailto:avik.niyogi at oracle.com"avik.niyogi at oracle.com> wrote:
Hi Sergey,
Please review webrev with changes as per the inputs provided:
HYPERLINK "http://cr.openjdk.java.net/~aniyogi/8146321/webrev.06/"cr.openjdk.java.net/~aniyogi/8146321/webrev.06/
With Regards,
Avik Niyogi
On 08-Feb-2016, at 2:29 am, Sergey Bylokhov <HYPERLINK "mailto:Sergey.Bylokhov at oracle.com"Sergey.Bylokhov at oracle.com> 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/
<http://cr.openjdk.java.net/%7Eaniyogi/8146321/webrev.05/>
With Regards,
Avik Niyogi
On 03-Feb-2016, at 7:38 pm, Alexandr Scherbatiy
<HYPERLINK "mailto:alexandr.scherbatiy at oracle.com"alexandr.scherbatiy at oracle.com
<mailto:alexandr.scherbatiy at oracle.com>> 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
<http://cr.openjdk.java.net/%7Eaniyogi/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
<<mailto:alexandr.scherbatiy at oracle.com>HYPERLINK "mailto:alexandr.scherbatiy at oracle.com"alexandr.scherbatiy at 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
<HYPERLINK "mailto:alexandr.scherbatiy at oracle.com"alexandr.scherbatiy at oracle.com> wrote:
On 2/2/2016 1:50 AM, Avik Niyogi wrote:
Hi All,
Please review the code changes made as per the inputs provided:
HYPERLINK "http://cr.openjdk.java.net/~aniyogi/8146321/webrev.03"cr.openjdk.java.net/~aniyogi/8146321/webrev.03
<http://cr.openjdk.java.net/%7Eaniyogi/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("<path to the duke
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();
}
}
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() {
try {
UIManager.setLookAndFeel("com.apple.laf.AquaLookAndFeel"); }
catch(Exception e) {
System.out.println("This is not a Mac.");
return;
}
JFrame f = new JFrame();
f.setSize(500, 500);
JDesktopPane dtp = new JDesktopPane();
JInternalFrame jif = new JInternalFrame();
jif.setTitle("Test");
jif.setFrameIcon(
new ImageIcon() {
public int getIconWidth() {
return 16;
}
public int getIconHeight() {
return 16;
}
public void paintIcon(Component c, Graphics g, int x, int y) {
g.setColor(java.awt.Color.green);
g.fillRect(x, y, 16, 16);
}
});
jif.setSize(400, 400);
jif.setVisible(true);
dtp.add(jif);
f.getContentPane().setLayout(new BorderLayout());
f.getContentPane().add(dtp, "Center");
f.setVisible(true);
}
});
}
}
Thanks,
Alexandr.
With Regards,
Avik Niyogi
On 02-Feb-2016, at 3:02 pm, Alexandr Scherbatiy
<HYPERLINK "mailto:alexandr.scherbatiy at oracle.com"alexandr.scherbatiy at oracle.com> wrote:
On 2/1/2016 11:25 PM, Avik Niyogi wrote:
Hi All,
Please review the code changes made as per inputs provided:
HYPERLINK "http://cr.openjdk.java.net/~aniyogi/8146321/webrev.02"cr.openjdk.java.net/~aniyogi/8146321/webrev.02
<http://cr.openjdk.java.net/%7Eaniyogi/8146321/webrev.02>
- is it possible to skip the ImageIcon parsing and handle it
as others icons (may be applying some translation to the graphics)?
- "(icon instanceof ImageIcon || icon instanceof Icon):
ImageIcons is also Icon so the whole condition should be
the same as just (icon instanceof Icon)
Thanks,
Alexandr.
With Regards,
Avik Niyogi
On 20-Jan-2016, at 10:35 pm, Sergey Bylokhov
<HYPERLINK "mailto:Sergey.Bylokhov at oracle.com"Sergey.Bylokhov at oracle.com> wrote:
On 20/01/16 18:43, Avik Niyogi wrote:
if ((icon.getIconWidth() > sMaxIconWidth
|| icon.getIconHeight() > sMaxIconHeight)) {
final Graphics2D g2 = (Graphics2D) g;
final AffineTransform savedAT = g2.getTransform();
g2.scale((float)sMaxIconWidth/icon.getIconWidth(),
(float)sMaxIconWidth/icon.getIconHeight());
icon.paintIcon(frame, g2, x, y);
g2.setTransform(savedAT);
}
This code does not take into account that x,y whcih are
passed to the paintIcon should be adjusted, because after the
scale they contain incorrect starting points.
then for a test code with the following:
import java.awt.*;
import javax.swing.*;
public class JInternalFrameBug {
public static void main(String[] args) {
SwingUtilities.invokeLater(
new Runnable() {
public void run() {
try {
UIManager.setLookAndFeel("com.apple.laf.AquaLookAndFeel"); }
catch(Exception e) {
System.out.println("This is not a Mac.");
return;
}
JFrame f = new JFrame();
f.setSize(400, 400);
JDesktopPane dtp = new JDesktopPane();
JInternalFrame jif = new JInternalFrame();
jif.setTitle("Test");
jif.setFrameIcon(new
ImageIcon("HYPERLINK "mailto:/Users/avniyogi/Downloads/FeedbinNotifier-master/FeedbinNotifier/Images.xcassets/AppIcon.appiconset/icon_128x128 at 2x.png"/Users/avniyogi/Downloads/FeedbinNotifier-master/FeedbinNotifier/Images.xcassets/AppIcon.appiconset/icon_128x128 at 2x.png"));
jif.setSize(200, 200);
jif.setVisible(true);
dtp.add(jif);
f.getContentPane().setLayout(new
BorderLayout());
f.getContentPane().add(dtp, "Center");
f.setVisible(true);
}
});
}
}
results in this:
On 20-Jan-2016, at 4:42 pm, Alexander Scherbatiy
<HYPERLINK "mailto:alexandr.scherbatiy at oracle.com"alexandr.scherbatiy at oracle.com
<mailto:alexandr.scherbatiy at oracle.com>> wrote:
On 1/20/2016 7:59 AM, Avik Niyogi wrote:
Hi All,
A gentle reminder, please review my code changes as in the
webrev
below in the mail trail.
With Regards,
Avik Niyogi
On 18-Jan-2016, at 3:01 pm, Avik Niyogi
<HYPERLINK "mailto:avik.niyogi at oracle.com"avik.niyogi at oracle.com
<mailto:avik.niyogi at oracle.com>
<mailto:avik.niyogi at oracle.com>> wrote:
Hi Sergey,
Please review the webrev taking inputs as per the
discussion before:
http://cr.openjdk.java.net/~aniyogi/8146321/webrev.01/
<http://cr.openjdk.java.net/%7Eaniyogi/8146321/webrev.01/>
The idea was to scale the graphics that the drawn icon
fits to the
target sizes.
Something like:
--------
g2d.scale(targetIconWidth/icon.getWidth(),
targetIconHeight/icon.getHeight());
icon.paintIcon(frame, g2d, x, y);
---------
This should work not only for ImageIcon but for any type
of icons.
Thanks,
Alexandr.
With Regards,
Avik Niyogi
On 14-Jan-2016, at 12:55 pm, Avik Niyogi
<HYPERLINK "mailto:avik.niyogi at oracle.com"avik.niyogi at oracle.com
<mailto:avik.niyogi at oracle.com>
<mailto:avik.niyogi at oracle.com>> wrote:
Hi Sergey,
I have verified it with the test case as well. If a test
case
overrides these methods to imply a change with icons
larger than
16x16 it will show that for ImageIcon and Icon as
before. The
resize of ImageIcon is only in case it has an image file
that it
will try to fit. I had a similar query myself and have
found out
that *getImage()* exists for ImageIcon class only and
not Icon class.
Example:
private static void createImageIconUI(final String
lookAndFeelString)
throws Exception {
SwingUtilities.invokeAndWait(new Runnable() {
@Override
public void run() {
desktopPane = new JDesktopPane();
internalFrame = new JInternalFrame();
frame = new JFrame();
internalFrame.setTitle(lookAndFeelString);
titleImageIcon = new ImageIcon() {
@Override
public int getIconWidth() {
return 16;
}
@Override
public int getIconHeight() {
return 16;
}
@Override
public void paintIcon(
Component c, Graphics g, int x, int y) {
g.setColor(java.awt.Color.black);
*g.fillRect(x, y, 50, 50);*
}
};
internalFrame.setFrameIcon(titleImageIcon);
internalFrame.setSize(500, 200);
internalFrame.setVisible(true);
desktopPane.add(internalFrame);
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
frame.getContentPane().setLayout(new BorderLayout());
frame.getContentPane().add(desktopPane, "Center");
frame.setSize(500, 500);
frame.setLocationRelativeTo(null);
frame.setVisible(true);
frame.toFront();
}
});
}
In this case the ImageIcon will NOT be resized to 16 x
16 even
before my fix was implemented. That resize as shown in
code is
*only for Image files to fit in default ImageIcon* size
as returned
from the getIconHeight() and getIconWidth() methods. If user
changes the ImageIcon size as in the example, that is
upto to user
to choose to do so and no control exists to prevent
that. *So, with
or without my fix, this behaviour will be same for
ImageIcon and
Icon custom instances.*
Hope this clarifies the query.
With Regards,
Avik Niyogi
On 14-Jan-2016, at 11:36 am, Sergey Bylokhov
<HYPERLINK "mailto:Sergey.Bylokhov at oracle.com"Sergey.Bylokhov at oracle.com
<mailto:Sergey.Bylokhov at oracle.com>
<mailto:Sergey.Bylokhov at oracle.com>> wrote:
Hi, Avik.
In the fix you update getIconWidth() and getIconHeight,
so now we
take the Icon into account. but it seems if the Icon is
bigger
that the maximum size it will not be resided to 16x16,
right?
On 14/01/16 07:49, Avik Niyogi wrote:
Hi All,
Kindly review the bug fix for JDK 9.
*Bug:*
https://bugs.openjdk.java.net/browse/JDK-8146321
*Webrev:*
http://cr.openjdk.java.net/~aniyogi/8146321/webrev.00/
<http://cr.openjdk.java.net/%7Eaniyogi/8146321/webrev.00/>
*Issue:*
Under the Mac Look&Feel, if an icon type other than an
ImageIcon
is used
in JInternalFrame.setFrameIcon(),
the icon will show up in the wrong position.
*Cause:*
the "instanceof Icon" was not checked for. Also, customs
ImageIcon with
color fill (and no image URL) which would
have resulted in null value for resized ImageIcon
image was not well
handled.
*Fix:*
All places in Aqua LAF where "instanceof Icon” (and
not just
ImageIcon
class) is required,
inputs were added after significant analyses.
Check for null for getImage was done to remove a Null
Pointer
Exception.
With Regards,
Avik Niyogi
--
Best regards, Sergey.
--
Best regards, Sergey.
--
Best regards, Sergey.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160209/22d06328/attachment.html>
More information about the swing-dev
mailing list