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.

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.

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

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

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

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

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

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

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

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.

…. (Commit 1: The simplest/most localized changes.) 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).

…. (Commit 2: Slightly more complex cases, and mergeImages cases, separated out for ease of review.)

eirikbakke · 2025-01-24T19:52:06Z

I have revised this PR now. Changes:

Split the PR into two commits, one for the most trivial changes, and one
for the slightly more complex changes that had some review comments.
Change loadImageIcon(path, false) to ImageUtilities.loadIcon(path) where possible
(the latter utility method was added in Add ImageUtilities methods to help migrating away from "new ImageIcon" (SVG icon related) #8114 ). (In both commits.)
Simplify the mergeImages cases by using the new ImageUtilities.mergeIcons method
(also from Add ImageUtilities methods to help migrating away from "new ImageIcon" (SVG icon related) #8114). (Second commit only.)
Some other revisions in the second commit.

For those who reviewed a previous version of this PR, the changes that might need another look are in the second commit. The first commit was just edited mechanically from the patch file itself, to switch loadImageIcon to loadIcon and remove the ", false" second argument which is the default for loadIcon anyway.

This is a follow-up on apache#8114 and apache#8109 . 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. This PR, combined with the previously mentioned PRs, handles most of the remaining cases. Specifically: * Search for 'new ImageIcon(' and rewrite each case to use ImageUtilities to load icons instead. * Search for 'instanceof ImageIcon' and generalize to 'instanceof Icon' when appropriate. * Search for 'getLookAndFeel*getDisabledIcon' and switch to ImageUtilities.createDisabledIcon.

mbien

looks good. Left one question.

.../netbeans/modules/refactoring/java/ui/scope/CurrentJavaProjectDependenciesScopeProvider.java

mbien · 2025-01-25T23:15:23Z

btw @eirikbakke it might be better to wait a bit with merge, lets say until RC 2-3 - project wide refactoring can make it hard to sync changes back from delivery cc @ebarboni

eirikbakke · 2025-01-25T23:29:25Z

@mbien Yes, this can wait until the whole release is done, no hurry. Good to get it tested in dev builds first.

…an be used instead of mergeImages.

mbien

nice simplifications

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

Merged

eirikbakke mentioned this pull request Jan 14, 2025

The icon for running a test method is not displayed correctly in the dark editor theme #8151

Closed

eirikbakke added this to the NB26 milestone Jan 24, 2025

eirikbakke force-pushed the pr-loadicon branch 3 times, most recently from a27b089 to 8f8b150 Compare January 24, 2025 19:46

eirikbakke added 2 commits January 24, 2025 14:46

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

8f8b150

…. (Commit 2: Slightly more complex cases, and mergeImages cases, separated out for ease of review.)

eirikbakke mentioned this pull request Jan 25, 2025

More work on centralizing icon loading to ImageUtilities #8194

Open

mbien approved these changes Jan 25, 2025

View reviewed changes

.../netbeans/modules/refactoring/java/ui/scope/CurrentJavaProjectDependenciesScopeProvider.java Outdated Show resolved Hide resolved

Simplify a few cases where the new ImageUtilities.mergeIcons method c…

9b5eee0

…an be used instead of mergeImages.

mbien approved these changes Jan 26, 2025

View reviewed changes

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

eirikbakke commented Jan 5, 2025 •

edited

Loading

eirikbakke commented Jan 5, 2025

eirikbakke commented Jan 24, 2025 •

edited

Loading

mbien left a comment

mbien commented Jan 25, 2025

eirikbakke commented Jan 25, 2025 •

edited

Loading

mbien left a comment

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

eirikbakke commented Jan 5, 2025 • edited Loading

eirikbakke commented Jan 5, 2025

eirikbakke commented Jan 24, 2025 • edited Loading

mbien left a comment

Choose a reason for hiding this comment

mbien commented Jan 25, 2025

eirikbakke commented Jan 25, 2025 • edited Loading

mbien left a comment

Choose a reason for hiding this comment

eirikbakke commented Jan 3, 2025 •

edited

Loading

mbien left a comment •

edited

Loading

eirikbakke commented Jan 5, 2025 •

edited

Loading

eirikbakke commented Jan 24, 2025 •

edited

Loading

eirikbakke commented Jan 25, 2025 •

edited

Loading