<Swing Dev> Review Request of 8146321: [macosx] JInternalFrame frame icon in wrong position on Mac L&F if icon is not ImageIcon
Avik Niyogi
avik.niyogi at oracle.com
Thu Feb 4 04:51:26 UTC 2016
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/~aniyogi/8146321/webrev.05/>
With Regards,
Avik Niyogi
> On 03-Feb-2016, at 7:38 pm, Alexandr Scherbatiy <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 <alexandr.scherbatiy at oracle.com <mailto: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 < <mailto:alexandr.scherbatiy at oracle.com>alexandr.scherbatiy at oracle.com <mailto: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:
>>>>>> 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 < <mailto:alexandr.scherbatiy at oracle.com>alexandr.scherbatiy at oracle.com <mailto: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:
>>>>>>>> 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 < <mailto:Sergey.Bylokhov at oracle.com>Sergey.Bylokhov at oracle.com <mailto: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( <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" <mailto:/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
>>>>>>>>>>> < <mailto:alexandr.scherbatiy at oracle.com>alexandr.scherbatiy at oracle.com <mailto:alexandr.scherbatiy at oracle.com>
>>>>>>>>>>> < <mailto:alexandr.scherbatiy at oracle.com>mailto: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 < <mailto:avik.niyogi at oracle.com>avik.niyogi at oracle.com <mailto:avik.niyogi at oracle.com>
>>>>>>>>>>>>> < <mailto:avik.niyogi at oracle.com>mailto:avik.niyogi at oracle.com <mailto:avik.niyogi at oracle.com>> < <mailto: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/%7Eaniyogi/8146321/webrev.01/>http://cr.openjdk.java.net/~aniyogi/8146321/webrev.01/ <http://cr.openjdk.java.net/~aniyogi/8146321/webrev.01/>
>>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Eaniyogi/8146321/webrev.01/><http://cr.openjdk.java.net/%7Eaniyogi/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 < <mailto:avik.niyogi at oracle.com>avik.niyogi at oracle.com <mailto:avik.niyogi at oracle.com>
>>>>>>>>>>>>>> < <mailto:avik.niyogi at oracle.com>mailto:avik.niyogi at oracle.com <mailto:avik.niyogi at oracle.com>> < <mailto: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
>>>>>>>>>>>>>>> < <mailto:Sergey.Bylokhov at oracle.com>Sergey.Bylokhov at oracle.com <mailto:Sergey.Bylokhov at oracle.com> < <mailto:Sergey.Bylokhov at oracle.com>mailto:Sergey.Bylokhov at oracle.com <mailto:Sergey.Bylokhov at oracle.com>>
>>>>>>>>>>>>>>> < <mailto: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>https://bugs.openjdk.java.net/browse/JDK-8146321 <https://bugs.openjdk.java.net/browse/JDK-8146321>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> *Webrev:*
>>>>>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Eaniyogi/8146321/webrev.00/>http://cr.openjdk.java.net/~aniyogi/8146321/webrev.00/ <http://cr.openjdk.java.net/~aniyogi/8146321/webrev.00/>
>>>>>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Eaniyogi/8146321/webrev.00/><http://cr.openjdk.java.net/%7Eaniyogi/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.
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160204/7a1e81a6/attachment.html>
More information about the swing-dev
mailing list