-
Notifications
You must be signed in to change notification settings - Fork 866
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ImageUtilities methods to help migrating away from "new ImageIcon" (SVG icon related) #8114
base: master
Are you sure you want to change the base?
Conversation
platform/openide.util.ui/src/org/openide/util/ImageUtilities.java
Outdated
Show resolved
Hide resolved
platform/openide.util.ui/src/org/openide/util/ImageUtilities.java
Outdated
Show resolved
Hide resolved
There is one more method I could add which might be useful, which is a mergeIcons(Icon,Icon,int,int) which works like mergeImages(Image,Image,int,int) but takes and returns Icon instances instead of Image. It could simplify some of the cases in #8109. |
4ffa602
to
04ca39c
Compare
I pushed a revision to the two commits in this PR. Changes since your previous review:
|
04ca39c
to
ce87d72
Compare
…ding methods used throughout the codebase easier. (Additive API change.) Edit Javadoc to avoid repeating semantics that are common to many methods. Some other Javadoc cleanup.
…mageUtilities.loadImage(URI), to make SVG icons work in these cases.
(Latest push was just a rebase on master.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me. Please check the comment I added.
public static final ImageIcon toImageIcon(Icon icon) { | ||
if (icon == null) { | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be better to fail on null since (new) code which doesn't check for null Icons before passing it to a conversion method is likely buggy and might benefit from failing early.
image2Icon
will fail on null i believe, icon2Image
does accept null but will log a warning + return placeholder image.
what do you think about keeping the naming pattern? -> icon2ImageIcon
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When doing bulk refactoring in the style of #8109, it is useful if icon
and toImageIcon(icon)
is guaranteed not to change the current behavior. Otherwise, existing features could break e.g. if they previously relied on null values being quietly accepted.
(Often I can read the existing code and be 95% sure a null value is never passed... but getting the last 5% of certainty is very difficult. And I don't want to have to insert null checks everywhere, if I'm changing 20 call sites and each case is really not supposed to be null in the first place.)
Perhaps I will specify in the Javadoc that null is not a valid input, but log a warning if a null does occur, and still return null in that unexpected case?
what do you think about keeping the naming pattern? -> icon2ImageIcon?
Good idea, will rename.
* @return new merged icon | ||
* @since 7.36 | ||
*/ | ||
public static final Icon mergeIcons(Icon icon1, Icon icon2, int x, int y) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would be mergeIcons2Images2Icon
(I am kidding, please don't change :))
Background: To ensure that SVG icons are loaded and drawn at full resolution, direct use of ImageIcon constructors should be avoided in the NetBeans codebase. The ImageIcon instances returned from methods in ImageUtilities, by contrast, are instances of a special subclass of ImageIcon that support vector graphics painting.
During work to remove uses of "new ImageIcon" constructors in the codebase (including #8109), I see that a few new utility methods would be useful in ImageUtilities. This PR proposes adding the following methods to ImageUtilities:
This PR also contains, in a separate commit, migration away from the Toolkit.getDefaultToolkit().createImage() method, using the new loadImage(URL) method. This makes, for instance, the "lightbulb" icons in the editor gutter show up properly with their new SVG icons: