Skip to content

Conversation

@eirikbakke
Copy link
Contributor

@eirikbakke eirikbakke commented May 30, 2019

This PR contains most of the changes that are needed to make ImageUtilities work with HiDPI icons. No publicly visible API changes are needed; the old javax.swing.Icon interface is actually well-suited for providing scalable graphics. The main challenge is to ensure that conversions between javax.swing.Icon and java.awt.Image, which are frequent throughout the NetBeans codebase, always retain the underlying scalable Icon instance.

The main motivation for this PR is to enable SVG icon support, which I will contribute in a separate PR. There's an added bonus, though: With all icon painting now centralized through ImageUtilities.ToolTipImage.paintIcon, we can add some rendering tweaks that make existing low-resolution icons look slightly better on HiDPI displays. See the attached before/after screenshots.

This issue is tracked under https://issues.apache.org/jira/browse/NETBEANS-2614 . SVG icon support is tracked under https://issues.apache.org/jira/browse/NETBEANS-2604 .

Credit: Emilian Bold did a lot of initial work on this before, at emilianbold/nextbeans@0f99dba . The main improvement in this PR is that arbitrary HiDPI scaling factors are supported. This is important on Windows, where non-integral scaling factors such as 150% can occur (MacOS, in contrast, always uses 200% for Retina screens). This PR also avoids the use of MultiResolutionImage, which does not work properly on Windows (see https://bugs.openjdk.java.net/browse/JDK-8212226 ).

To make reviewing easier, I have split up this PR into several commits, which can be reviewed one-at-a-time.

(To compare before/after screenshots, be sure to view the images at 100% scaling.)
Windows 150pct HiDPI Scaling, Before Patch
Windows 150pct HiDPI Scaling, After Patch
Windows 200pct HiDPI Scaling, Before Patch
Windows 200pct HiDPI Scaling, After Patch

The ImageUtilities class now supports HiDPI icons via custom implementations of
the Icon interface. For this to work, client code must let ImageUtilities manage
the creation of new Icon instances, using "ImageUtilities.image2Icon(image)"
instead of "new ImageIcon(image)". This is already common throughout the
NetBeans codebase, but there are still quite a few direct uses of ImageIcon.

This commit replaces a few of the direct uses of ImageIcon, for most of the
icons that are commonly seen during a Java editing session in the IDE.
Most of the icons that are seen in the IDE during a standard Java editing
session are already painted via a call to Icon.paintIcon. One exception is the
separator icon that is used in the breadcrumb component under the code editor,
which is painted via Graphics.drawImage. Migrate this one to Icon.paintIcon to
make the icon look better on HiDPI displays (taking advantage of the HiDPI
scaling hints that were recently implemented in ImageUtilities).

This also serves as an example of how to fix other similar cases in the future.
…I icons

Introduced a FilteredIcon class which extends from a new CachedHiDPIIcon class.
The CachedHiDPIIcon class will be used again in the future; it was originally
designed for an SVG-based Icon implementation, which will be contributed
separately in the future. It is generic enough to work with any kind of custom
Icon implementation that needs caching.

Also make color filtering of icons for dark themes work with HiDPI icons, using
the same FilteredIcon class.

Also improved a null handling case in ImageUtilities.loadImage. Passing a null
resource used to be valid before (yielding a null return value), but caused a
NPE on dark LAFs. Since it worked in the common non-dark case, it should
arguably work in the dark case as well. This might fix NETBEANS-2401, or it
might just lead to an NPE elsewhere in that case.
…olTipImage is being passed around.

No public APIs are affected.
This commit prepares ImageUtilities to work with scalable Icon implementations,
such as a future SVGIcon class. Conversions such image2icon(icon2Image(icon))
are now lossless with respect to scalable Icon implementations. This allows
HiDPI support to be implemented via custom Icon classes while keeping existing
Image-based APIs (such as org.openide.nodes.Node.getIcon) unchanged.

The internal ImageUtilities.ToolTipImage class, which also implements the Icon
interface, is now used in most cases for ImageUtilities methods that return an
Image. Modifying ToolTipImage's paint() method to paint an extra debugging
stroke yields a quick confirmation that most of the icons visible in the IDE
now successfully have their painting delegated through this method. This will
come in handy in a subsequent commit, when we try to do some centralized scaling
improvements for HiDPI displays.

It is no longer safe to assume that ImageUtilities.image2Icon returns an
ImageIcon. Grepped the entire codebase for such casts (searched for cast to
ImageIcon within 3 lines of 'image2Icon'), and found only one, which was fixed.

Manually tested, including for the case where
ImageUtilities.getImageIconFilter() returns a non-null filter. Fixed a deadlock
bug that was unearthed in the latter case.
Having centralized icon painting into ImageUtilities.ToolTipImage.paintIcon in
the previous commits, we can now add a few painting tweaks to improve the
appearance of low-resolution bitmap icons that must be scaled up for HiDPI
displays. See the before/after screenshots that are attached to JIRA ticket
NETBEANS-2614.
The old Image-based implementation of mergeImages remains  together with the new
Icon-based implementation. This ensures backwards-compatibility when icons are
drawn using Graphics.drawImage instead of Icon.paintIcon. (The new
implementation was confirmed to be invoked and working by adding some
now-removed temporary drawing code.)

This completes the changes needed to make all of ImageUtilities' APIs work with
scalable Icon implementations. An SVGIcon class will be added in a future
commit.
Copy link

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel qualified to review the graphics parts. If the existing tests pass and if some new are written, then OK.

eirikbakke added a commit to eirikbakke/incubator-netbeans that referenced this pull request Jun 1, 2019
…1273.

When the aforementioned PR is merged, this branch should be rebased to drop this commit.

See apache#1273 .
eirikbakke added a commit to eirikbakke/incubator-netbeans that referenced this pull request Jun 1, 2019
…1273.

When the aforementioned PR is merged, this branch should be rebased to drop this commit.

See apache#1273 .
matthiasblaesing pushed a commit to matthiasblaesing/netbeans that referenced this pull request Jun 3, 2019
…1273.

When the aforementioned PR is merged, this branch should be rebased to drop this commit.

See apache#1273 .
This was left over from a previous experiment. Also fix a case where
ImageUtilities.icon2Image could return null even when the icon is not null.

(Include this fix in this PR rather than in the SVG support PR, to
ensure I don't commit bugs if this one is merged before the other.)
@eirikbakke
Copy link
Contributor Author

Fixed a few test failures, and added a bunch more tests. I have had this patch running in my IDE for a week; will soon merge this one (and then continue work on the separate SVG support PR).

@eirikbakke
Copy link
Contributor Author

Just tested this patch (+the SVG patch) on MacOS as well. Seem not to be causing problems. Will merge this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants