Avoid remaining trivial uses of new ImageIcon(Image), for HiDPI icons #8109

eirikbakke · 2025-01-03T00:20:11Z

In previous PRs, e.g. #7463 and #8083, SVG versions of various NetBeans icons were added to look better on Retina/HiDPI monitors. ImageUtilities has supported automatic loading of SVG versions of icons since #1278 .

To render at full HiDPI resolution, Icon/Image instances must be created via the methods in ImageUtilities rather than, in particular, the constructors of ImageIcon. Uses of the latter must be replaced with e.g. ImageUtilities.image2Icon and ImageUtilities.loadImageIcon. Some such replacements were done in #7472 .

This PR replaces most of the remaining trivial uses of the ImageIcon(Image) constructor. This allows SVG icons, when/once present, to render in full resolution on HiDPI/Retina displays. Bitmap icons also benefit from the improved scaling hints applied by ImageUtilities.

While this PR touches many lines, the changes should be simple enough that they can be reviewed by simply reading over the diffs (rather than by opening every dialog, UI panel etc. in a running IDE). I will also run my working IDE with these changes for a while to see if I bump into any observable problems. It's not necessary to have a HiDPI screen to test this patch; merely seeing that there are no new exceptions etc. when running the IDE should be sufficient.

There are still many uses of the "new ImageIcon(String)" and "new ImageIcon(URL)" variations of the ImageIcon constructors. Removing these is left for future work.

Replace most of the remaining trivial uses of the ImageIcon(Image) constructor, replacing these with ImageUtilities.image2Icon or ImageUtilities.loadImageIcon. This allows SVG icons, when/once present, to render in full resolution on HiDPI/Retina displays. Bitmap icons also benefit from the improved scaling hints applied by ImageUtilities. Similar work was done, for the platform module only, in apache#7472 . This commit covers most of the remaining trivial cases; I grepped the codebase for 'new ImageIcon' and adjusted the code to avoid ImageIcon whenever this could be done easily (not changing APIs, requiring only a visual code review).

matthiasblaesing

Looks sane to me. Only eyeballed though. There is more cleanup possible, but that is a different task.

eirikbakke · 2025-01-04T22:37:12Z

Thanks! I will run with this in my working IDE for a few days before merging.

mbien

I got confused at the end so excuse if anything I commented on makes no sense ;)

some notes:

i think this PR would have benefited from 2 commits, one which refactors loading, the other which refactors conversion/wrapping problems
nitpick but most of the , false) postfixes could be probably removed, since most of the original code did also use the overloaded variants which set it to false by default.

mbien · 2025-01-05T10:26:30Z

ide/csl.api/src/org/netbeans/modules/csl/navigation/Icons.java

@@ -108,60 +107,47 @@ public static ImageIcon getElementIcon( ElementKind elementKind, Collection<Modi
            modifiers = Collections.<Modifier>emptyList();
        }

-        Image img = null;
+        ImageIcon icon;


move this line into the CONSTANT case?

Sure. (Declarations in a case statement will leak to the subsequent cases, so I left it at the top. But either style works.)

mbien · 2025-01-05T11:07:40Z

ide/xml.text/src/org/netbeans/modules/xml/text/navigator/NavigatorTreeCellRenderer.java

-    private ImageIcon getImageIcon(String name, boolean error){
-        ImageIcon icon = ImageUtilities.loadImageIcon(name, false);
-        if(error)
-            return new ImageIcon(ImageUtilities.mergeImages( icon.getImage(), ERROR_IMAGE, 15, 7 ));
-        else
+    private Icon getIcon(String name, boolean error){
+        Icon icon = ImageUtilities.loadImageIcon(name, false);
+        if (error) {
+            return ImageUtilities.image2Icon(ImageUtilities.mergeImages(
+                    ImageUtilities.icon2Image(icon), ERROR_IMAGE, 15, 7 ));
+        } else {
            return icon;
+        }


one call less if the variable keeps the original ImageIcon type.

private Icon getIcon(String name, boolean error){ ImageIcon icon = ImageUtilities.loadImageIcon(name, false); if (error) { return ImageUtilities.image2Icon(ImageUtilities.mergeImages(icon.getImage(), ERROR_IMAGE, 15, 7 )); } else { return icon; } }

It's another change I think can be dropped overall since mergeImages is smart enough to preserve HiDPI resolution.

mbien · 2025-01-05T11:08:16Z

java/refactoring.java/src/org/netbeans/modules/refactoring/java/WhereUsedBinaryElement.java

-            ImageIcon imageIcon = new ImageIcon(DataObject.find(fo).getNodeDelegate().getIcon(BeanInfo.ICON_COLOR_16x16));
+            icon = ImageUtilities.image2Icon(DataObject.find(fo).getNodeDelegate().getIcon(BeanInfo.ICON_COLOR_16x16));
            Boolean inTestFile = ElementGripFactory.getDefault().inTestFile(fo);
            if (Boolean.TRUE == inTestFile) {
-                Image mergeImages = ImageUtilities.mergeImages(imageIcon.getImage(),
-                        ImageUtilities.loadImageIcon("org/netbeans/modules/refactoring/java/resources/found_item_test.png", false).getImage(), 4, 4);
-                imageIcon = new ImageIcon(mergeImages);
+                icon = ImageUtilities.image2Icon(ImageUtilities.mergeImages(ImageUtilities.icon2Image(icon),
+                        ImageUtilities.loadImage("org/netbeans/modules/refactoring/java/resources/found_item_test.png", false), 4, 4));
            }
-            icon = imageIcon;
        } catch (DataObjectNotFoundException ex) {


similar here. I think ImageIcon icon would allow to skip the call to icon2image.

although the lookup registration later would have to be changed to icon.getImage()

I can simplify it a bit here; keeping the ImageIcon from loadImageIcon while keeping image2Icon instead of the "new ImageIcon" constructor.

mbien · 2025-01-05T11:08:41Z

java/refactoring.java/src/org/netbeans/modules/refactoring/java/ui/tree/FileTreeElement.java

-            ImageIcon imageIcon = new ImageIcon(DataObject.find(fo).getNodeDelegate().getIcon(BeanInfo.ICON_COLOR_16x16));
+            Icon icon = ImageUtilities.image2Icon(DataObject.find(fo).getNodeDelegate().getIcon(BeanInfo.ICON_COLOR_16x16));
            Boolean inTestFile = ElementGripFactory.getDefault().inTestFile(fo);
            if(Boolean.TRUE == inTestFile) {
-                Image mergeImages = ImageUtilities.mergeImages(imageIcon.getImage(),
-                        ImageUtilities.loadImageIcon("org/netbeans/modules/refactoring/java/resources/found_item_test.png", false).getImage(), 4, 4);
-                imageIcon = new ImageIcon(mergeImages);
+                icon = ImageUtilities.image2Icon(ImageUtilities.mergeImages(
+                    ImageUtilities.icon2Image(icon),
+                    ImageUtilities.loadImage("org/netbeans/modules/refactoring/java/resources/found_item_test.png", false), 4, 4));
            }
-            return imageIcon;
+            return icon;


mbien · 2025-01-05T11:21:33Z

.../org/netbeans/modules/javaee/project/api/ant/ui/logicalview/AbstractLogicalViewProvider.java

-                return ImageUtilities.mergeImages(original, ProjectProperties.ICON_BROKEN_BADGE.getImage(), 8, 0);
+                return ImageUtilities.mergeImages(original,
+                    ImageUtilities.icon2Image(ProjectProperties.ICON_BROKEN_BADGE), 8, 0);


why is this change needed?

the utility method will simply call getImage() anyway, no?

There's some custom handling for certain Icon subclasses, but reading over the ImageUtilities code, it indeed seems that the change is unnecessary! mergeImages is smart enough to preserve vector graphics in these cases.

I will remove the changes around mergeImages.

mbien · 2025-01-05T11:22:05Z

.../org/netbeans/modules/javaee/project/api/ant/ui/logicalview/AbstractLogicalViewProvider.java

-                return ImageUtilities.mergeImages(original, ProjectProperties.ICON_BROKEN_BADGE.getImage(), 8, 0);
+                return ImageUtilities.mergeImages(original,
+                        ImageUtilities.icon2Image(ProjectProperties.ICON_BROKEN_BADGE), 8, 0);


same question

Yup, I'll remove these from the patch, as mergeImages is smart enough to avoid the need.

mbien · 2025-01-05T11:43:46Z

...rc/org/netbeans/modules/java/api/common/project/ui/customizer/ClassPathListCellRenderer.java

many instances where imageicon.getImage() was changed to icon2image(imageicon)

Yup, will remove.

mbien · 2025-01-05T11:48:52Z

php/php.project/src/org/netbeans/modules/php/project/ui/PathUiSupport.java

here too. getImage() was changed to icon2Image()

I'm trying to reduce the use of ImageIcon, as it assumes that icons are bitmaps rather than vector graphics. When ImageIcon is used it's harder to reason about whether vector graphics will be preserved or not.

eirikbakke · 2025-01-05T17:08:47Z

Thanks for useful reviews!

i think this PR would have benefited from 2 commits, one which refactors loading, the other which refactors conversion/wrapping problems

I see now that the changes around mergeImages can probably be removed, since mergeImages is smart enough to pick up the special implementations of Image and Icon by itself.

nitpick but most of the , false) postfixes could be probably removed, since most of the original code did also use the overloaded variants which set it to false by default.

ImageUtilities has loadImage(String) and loadImage(String,boolean) but no loadIconImage(String), only loadIconImage(String,boolean).

I think I will make a separate PR with some proposed new methods in ImageUtilities that can simplify some of these changes:

loadIcon(String,boolean) would be like loadIconImage(String,boolean) but returns only a plain Icon (not IconImage). This would help discourage use of IconImage in the future.
loadIcon(String) would be like loadIcon(String,false)

eirikbakke · 2025-01-05T23:48:27Z

nitpick but most of the , false) postfixes could be probably removed, since most of the original code did also use the overloaded variants which set it to false by default.

I proposed some new utility methods for ImageUtilities in #8114. One of these will let me change "loadImageIcon(path, false)" to just "loadIcon(path)" in most of the cases here.

eirikbakke added Platform [ci] enable platform tests (platform/*) UI User Interface labels Jan 3, 2025

eirikbakke requested a review from matthiasblaesing January 4, 2025 18:06

matthiasblaesing approved these changes Jan 4, 2025

View reviewed changes

mbien reviewed Jan 5, 2025

View reviewed changes

eirikbakke mentioned this pull request Jan 5, 2025

Add ImageUtilities methods to help migrating away from "new ImageIcon" (SVG icon related) #8114

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid remaining trivial uses of new ImageIcon(Image), for HiDPI icons #8109

Avoid remaining trivial uses of new ImageIcon(Image), for HiDPI icons #8109

eirikbakke commented Jan 3, 2025 •

edited

Loading

matthiasblaesing left a comment

eirikbakke commented Jan 4, 2025

mbien left a comment •

edited

Loading

mbien Jan 5, 2025

eirikbakke Jan 5, 2025 •

edited

Loading

mbien Jan 5, 2025

eirikbakke Jan 5, 2025

mbien Jan 5, 2025

eirikbakke Jan 5, 2025

mbien Jan 5, 2025

mbien Jan 5, 2025

eirikbakke Jan 5, 2025

mbien Jan 5, 2025

eirikbakke Jan 5, 2025

mbien Jan 5, 2025

eirikbakke Jan 5, 2025

mbien Jan 5, 2025

eirikbakke Jan 5, 2025

eirikbakke commented Jan 5, 2025 •

edited

Loading

eirikbakke commented Jan 5, 2025

Avoid remaining trivial uses of new ImageIcon(Image), for HiDPI icons #8109

Are you sure you want to change the base?

Avoid remaining trivial uses of new ImageIcon(Image), for HiDPI icons #8109

Conversation

eirikbakke commented Jan 3, 2025 • edited Loading

matthiasblaesing left a comment

Choose a reason for hiding this comment

eirikbakke commented Jan 4, 2025

mbien left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirikbakke Jan 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirikbakke commented Jan 5, 2025 • edited Loading

eirikbakke commented Jan 5, 2025

eirikbakke commented Jan 3, 2025 •

edited

Loading

mbien left a comment •

edited

Loading

eirikbakke Jan 5, 2025 •

edited

Loading

eirikbakke commented Jan 5, 2025 •

edited

Loading