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.scherba...@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.scherba...@oracle.com <mailto: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 < >>>>> <mailto:alexandr.scherba...@oracle.com>alexandr.scherba...@oracle.com >>>>> <mailto:alexandr.scherba...@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.scherba...@oracle.com>alexandr.scherba...@oracle.com >>>>>>> <mailto:alexandr.scherba...@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.bylok...@oracle.com>sergey.bylok...@oracle.com >>>>>>>>> <mailto:sergey.bylok...@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_128x...@2x.png>"/Users/avniyogi/Downloads/FeedbinNotifier-master/FeedbinNotifier/Images.xcassets/AppIcon.appiconset/icon_128x...@2x.png" >>>>>>>>>> >>>>>>>>>> <mailto:/Users/avniyogi/Downloads/FeedbinNotifier-master/FeedbinNotifier/Images.xcassets/AppIcon.appiconset/icon_128x...@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.scherba...@oracle.com>alexandr.scherba...@oracle.com >>>>>>>>>>> <mailto:alexandr.scherba...@oracle.com> >>>>>>>>>>> < >>>>>>>>>>> <mailto:alexandr.scherba...@oracle.com>mailto:alexandr.scherba...@oracle.com >>>>>>>>>>> <mailto:alexandr.scherba...@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.niy...@oracle.com>avik.niy...@oracle.com >>>>>>>>>>>>> <mailto:avik.niy...@oracle.com> >>>>>>>>>>>>> < <mailto:avik.niy...@oracle.com>mailto:avik.niy...@oracle.com >>>>>>>>>>>>> <mailto:avik.niy...@oracle.com>> < >>>>>>>>>>>>> <mailto:avik.niy...@oracle.com>mailto:avik.niy...@oracle.com >>>>>>>>>>>>> <mailto:avik.niy...@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.niy...@oracle.com>avik.niy...@oracle.com >>>>>>>>>>>>>> <mailto:avik.niy...@oracle.com> >>>>>>>>>>>>>> < <mailto:avik.niy...@oracle.com>mailto:avik.niy...@oracle.com >>>>>>>>>>>>>> <mailto:avik.niy...@oracle.com>> < >>>>>>>>>>>>>> <mailto:avik.niy...@oracle.com>mailto:avik.niy...@oracle.com >>>>>>>>>>>>>> <mailto:avik.niy...@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.bylok...@oracle.com>sergey.bylok...@oracle.com >>>>>>>>>>>>>>> <mailto:sergey.bylok...@oracle.com> < >>>>>>>>>>>>>>> <mailto:sergey.bylok...@oracle.com>mailto:sergey.bylok...@oracle.com >>>>>>>>>>>>>>> <mailto:sergey.bylok...@oracle.com>> >>>>>>>>>>>>>>> < >>>>>>>>>>>>>>> <mailto:sergey.bylok...@oracle.com>mailto:sergey.bylok...@oracle.com >>>>>>>>>>>>>>> <mailto:sergey.bylok...@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. >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
