<Swing Dev> Review Request of 8146321: [macosx] JInternalFrame frame icon in wrong position on Mac L&F if icon is not ImageIcon
Alexandr Scherbatiy
alexandr.scherbatiy at oracle.com
Thu Feb 4 09:26:55 UTC 2016
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
>> <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
>>>> <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
>>>>>> <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
>>>>>>>> <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
>>>>>>>>>> <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("/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
>>>>>>>>>>>> <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
>>>>>>>>>>>>>> <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
>>>>>>>>>>>>>>> <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
>>>>>>>>>>>>>>>> <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.
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160204/162ac1d9/attachment.html>
More information about the swing-dev
mailing list