Skip to content
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

[NETBEANS-2614,NETBEANS-1586] Improve icon scaling on HiDPI displays, and prepare ImageUtilities for HiDPI icons #1273

Merged
merged 10 commits into from Jun 7, 2019

Conversation

@eirikbakke
Copy link
Contributor

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

ultorg added some commits May 29, 2019

Replace 'new ImageIcon' with image2Icon for commonly visible icons.
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.
Use Icon.paintIcon instead of Graphics.drawImage (one example).
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.
[NETBEANS-1586] Make ImageUtilities.createDisabledIcon work with HiDP…
…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.
Adjust data types in ImageUtilities to make it easier to see where To…
…olTipImage is being passed around.

No public APIs are affected.
Make ImageUtilities handle scalable Icon implementations.
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.
Improve rendering quality of scaled bitmap icons on HiDPI displays.
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.

ultorg added some commits May 30, 2019

Support HiDPI icons from ImageUtilities.mergeImages.
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.

@eirikbakke eirikbakke force-pushed the eirikbakke:NETBEANS-2614 branch from 30446a5 to f1c2f77 Jun 1, 2019

@JaroslavTulach
Copy link
Contributor

left a comment

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

eirikbakke pushed a commit to eirikbakke/incubator-netbeans that referenced this pull request Jun 1, 2019

This commit contains the patch from the separate Pull Request apache#…
…1273.

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

See apache#1273 .

eirikbakke pushed a commit to eirikbakke/incubator-netbeans that referenced this pull request Jun 1, 2019

This commit contains the patch from the separate Pull Request apache#…
…1273.

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

See apache#1273 .

matthiasblaesing added a commit to matthiasblaesing/netbeans that referenced this pull request Jun 3, 2019

This commit contains the patch from the separate Pull Request apache#…
…1273.

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

See apache#1273 .

ultorg added some commits Jun 2, 2019

Don't have CachedHiDPIIcon extend from ImageIcon.
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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

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

@eirikbakke eirikbakke merged commit 5f78200 into apache:master Jun 7, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.