[NETBEANS-2604] Support SVG icon loading from ImageUtilities

[eirikbakke]

This PR adds support for SVG icon loading from ImageUtilities, and tests it by adding SVG icons for the Undo and Redo actions. See https://issues.apache.org/jira/browse/NETBEANS-2604 and the attached screenshot. This PR depends on #1273, which must be applied first. This PR can be reviewed more or less independently (just ignore the first commit, which contains all the changes from the other PR).

Some questions for more experienced NetBeans veterans:

• Are my module and package naming conventions appropriate? (E.g. put SVGLoader in org.openide.util.spi, and name the implementation module "org.openide.util.ui.svg".)
• Am I filling in apichanges.xml correctly? (The previous entry, about VectorIcon, was from an earlier PR of mine as well, but I don't see that one reflected at https://bits.netbeans.org/11.0/javadoc/apichanges.html , which puzzles me.)
• Is it correct to use "OpenIDE-Module-Display-Category=Infrastructure" for the SVG Loader module? (In platform/openide.util.ui.svg/src/org/openide/util/svg/Bundle.properties.)
• Would it be better to keep the SVGLoader SPI in a private package, instead having the implementation module depend on the implementation version of openide.util.ui to gain access to it? I don't anticipate anyone ever needing to provide an alternative implementation.
• Is there anything I need to do to verify that the JAR hashes are correct? The JARs from Maven Central are not exactly the same as those from https://xmlgraphics.apache.org/batik/download.html .
• Is the inclusion of a some dependency JARs (commons-io, commons-logging, and xml-apis) from libs.batik OK? They all have the same Apache 2.0 license. There are existing wrapper modules for commons-io and commons-logging, but those are in the "ide" cluster, and I assume modules in the "platform" cluster shouldn't depend on modules in the "ide" cluster? Should the SVG loader implementation module (openide.util.ui.svg) rather be in the "ide" cluster?
• When I build my NetBeans Platform app against a NetBeans build with this patch applied, SVG support is enabled even though I didn't add the supposedly-optional openide.util.ui.svg module (which provides the SVGLoaderImpl service provider) as a maven dependency. Why is this? How can I make it so that platform application developers need to request SVG support explicitly? And related: Where do I specify which maven coordinates the openide.util.ui.svg module should use?
• Am I doing versioning right for the libs.batik library? Does it make sense to assign it version 1.0 rather than Batik's own version, which is 1.11?

Thanks for your help!

Example SVG icons loaded and displayed in the IDE:

Example renderings of other SVG files tested (from Ultorg, an application built on the NetBeans Platform), as displayed using the platform/o.n.swing.tabcontrol/test/unit/src/org/netbeans/swing/tabcontrol/plaf/VectorIconTester.java utility (which was added in an earlier commit):

[Chris2011]

Finally :) great work :)

[emilianbold]

I can't find anything obviously wrong.

+1

[matthiasblaesing]

I had a look at the license situation and I have two suggestions. One is, that that "batik-license" is not correct as it is. There are dependencies, that have separate licenses and notice files. For two of these components (commons-io and commons-logging) the work was already done, here the modules were moved and added as dependency. For The other files, license and notice files were created.

There were svg files added and these were declared to be Apache-2.0-ASF in the licenseinfo.xml. That is not necessary as svg is just XML and can perfectly hold the license header on its own.

The changes are pushed into this branch:

https://github.com/matthiasblaesing/netbeans/tree/pr-1278

feel free to pull them or just be inspired.

[timboudreau]

I don't seem to be able to add review comments here, so I'll just enumerate them:

1. For startup / loading performance, I would still advice compiling images into something vector-ish without the overhead of XML parsing at compile time, having the loader look for that first, and if that fails, then fail over to parsing SVG. That's not a blocker, as it could be added transparently later, but probably a good idea. Parsing XML is not cheap, and since icons are loaded on demand, it's not going to be predictable what it slows down, but it will slow down something.

2. In GraphicsNode, you probably want to use the rendering hints for the specific screen type, so SVG icons use subpixel antialiasing if the user's screen does (this should also be incorporated into CachedImageKey, since the value may be different on one monitor and another, e.g. RGB vs. BGR). You can get the screen settings like this:

hintsMap = (Map<RenderingHints.Key, Object>)
    (Toolkit.getDefaultToolkit().getDesktopProperty("awt.font.desktophints"));

3. I would go beyond in-memory caching, and actually use the IDE's cache dir to write out rasterized versions of the generated images, and attempt to load from there first - particularly if not doing any precompiling. Just make the image size and other properties part of the path from your cache folder, e.g. $CACHE_ROOT/svgimg/$WIDTH/$HEIGHT/$TEXT_AA/org/openide/resources/actions/redo.png - then let the original ImageUtilities raster image caching take care of it from there.

The way you'd do that "compiling" would be to load the SVG and paint it into something like this: https://github.com/timboudreau/imagine/blob/master/Imagine/vector/src/main/java/net/java/dev/imagine/api/vector/painting/VectorWrapperGraphics.java - and then have a simple way to serialize the list of objects that represent paint operations to disk; and load those at runtime. Or perhaps Batik's internal representation could be used in some tighter, more lightweight serialized form (but I doubt it, and it would be nice, under normal circumstances, not to load Batik at all).

[eirikbakke]

@matthiasblaesing Thanks! I looked through your first patch and added it to the commits in this PR.

As for SVG files, at least these particular ones, I prefer to regard them as opaque files that shouldn't be hand-edited, since they will be re-generated by Adobe Illustrator every time we make changes to the icons. (Besides, SVG icons, which are loaded at runtime, will become about 25% larger on average if we add a license comment to each. Not sure it really matters.)

[eirikbakke]

@timboudreau I did a PNG vs. SVG loading benchmark here: https://gist.github.com/eirikbakke/6519b3e623c0f4703ee82388d1587f08 . Results:

Average size of SVG icon file is 2959 bytes (max was 5244 bytes)
Average size of PNG icon file is 363 bytes (max was 652 bytes)
Loading and painting SVG took 2161.02 microseconds per icon +/- 4.49%
Loading and painting PNG took 267.02 microseconds per icon +/- 15.87%

So loading an SVG image is about 8 times slower than loading a PNG. Though it's still only about 2 milliseconds per icon, so delays shouldn't be noticeable until we've redrawn at least 100 icons. Also note that for HiDPI screens, the PNG file will have to be loaded twice, one at the standard resolution, and one at twice the resolution (4x the number of pixels). In this scenario, I estimate that the PNG approach is only about 4x faster than the SVG approach.

I kind of like the store-PNG-in-cache-dir approach--saves us from having to bundle generated PNG files. I'm more sceptical of the "store as log of Graphics2D operations"--that's a lot of code and complexity for a small performance enhancement. Once this PR is merged, I'll add a JIRA ticket reminding us to think about SVG caching in the future.

[eirikbakke]

@timboudreau On the RenderingHints comment: I believe subpixel rendering is only relevant for text rendering. And there should be no text content in the SVG files that we use for icons in any case (glyphs that appear as part of icon shapes are typically outlined to bezier curves in the drawing program before export to SVG, since the fonts available on the target machine is unpredictable).

For SVG icons, I'd rather keep the RenderingHints standardized and unaffected by the environment, to keep the rendering as close as possible to what the icon designer will see in their drawing program (e.g. when turning on Adobe Illustrator's "pixel preview"). That has the added benefit of keeping the cache of rendered images small.

[JaroslavTulach]

I have nothing against loading SVG icons in ImageUtilities, but I am concerned about making NetBeans Platform too heavyweight. I don't want to see any new JAR files in netbeans/platform/lib/ directory.

If there is no easier way to load SVG icons in Java than to use fifteen batik libraries, then it has to be done as an optional module(s) and be end up being placed in netbeans/platform/modules, in my opinion.

[emilianbold]

I have nothing against loading SVG icons in ImageUtilities, but I am concerned about making NetBeans Platform too heavyweight. I don't want to see any new JAR files in netbeans/platform/lib/ directory.

If there is no easier way to load SVG icons in Java than to use fifteen batik libraries, then it has to be done as an optional module(s) and be end up being placed in netbeans/platform/modules, in my opinion.

There is nothing holy about the platform/lib folder? macOS and Windows are moving to HiDPI screens. Most users seem to be on Windows so it makes sense to handle some sort of vectorial icons. As a desktop platform, NetBeans Platform must support vectorial icons out of the box -- any modern app would need it. This does not seem something optional, it's just getting to current-day configurations.

Anybody that wants a slimmed down Platform can probably easily recompile it and remove some modules.

I'm curious which apps use the platform and would use a modern Apache NetBeans Platform version but wouldn't want to support HiDPI screens?

[neilcsmith-net]

I'm curious which apps use the platform and would use a modern Apache NetBeans Platform version but wouldn't want to support HiDPI screens?

No, but you assume that everyone would want to approach it in this way, as opposed to icon fonts or responsive bitmaps, both of which have the benefit of relying solely on core Java desktop support.

It is also feasible to have a lightweight, single file, SVG renderer that would be enough for this - Processing has one, but not under a usable license.

I would still prefer to see some sort of generic IconLoader SPI (and similar for branding text) such that we allow for IDE modules and RCP apps to use / experiment with alternative approaches that work for them.

[eirikbakke]

No, but you assume that everyone would want to approach it in this way, as opposed to icon fonts or responsive bitmaps, both of which have the benefit of relying solely on core Java desktop support.

The good news is that the work is now done for all of these different approaches to work, in the PR that preceded this one ( #1273 ). In fact, my own NetBeans Platform application uses both SVG icons as well as a custom Icon implementation based on an icon font (for a certain subset of icons). And openide.util.ui also has an abstract base class called VectorIcon now, which can be used for custom-painted HiDPI icons (used for window system LAF icons in #859 ).

I would still prefer to see some sort of generic IconLoader SPI (and similar for branding text) such that we allow for IDE modules and RCP apps to use / experiment with alternative approaches that work for them.

See my answer to "Why not a generic IconLoader SPI and first loader that handles the URL wins?" on the mailing list:

I think the IconLoader SPI would be a separate feature that could be added later. The main reason for the SVGLoader SPI is to delay loading of the Batik SVG library JARs until after the core startup modules have loaded (e.g. the splash screen). In fact, I'd love for the SVGLoader SPI to be a non-public implementation detail--is there a way I can do this?

In your proposed IconLoader SPI, how do you image that implementation modules will specify which URLs they can handle? If the implementation modules must do this programmatically, then they cannot be loaded lazily. So in that case the IconLoader SPI ends up being separate from the SVGLoader SPI in any case. Any thoughts on this?

I did do one change to this PR, though: I ditched the feature of automatically loading an SVG file if a PNG file with the same extension is requested (e.g. loading "icon.svg" when "icon.png" is requested). That kind of substitution is something that could be implemented in the proposed future IconLoader SPI.

[neilcsmith-net]

See my answer to "Why not a generic IconLoader SPI and first loader that handles the URL wins?" on the mailing list:

Sorry, got waylaid by travels and forgot I hadn't replied there.

In your proposed IconLoader SPI, how do you image that implementation modules will specify which URLs they can handle? If the implementation modules must do this programmatically, then they cannot be loaded lazily. So in that case the IconLoader SPI ends up being separate from the SVGLoader SPI in any case. Any thoughts on this?

Ideally it would be declarative where possible, but probably not viable in this case. Personally I'd move SVGLoader and the Batik implementation outside of ImageUtilities, and have the former lazily load the latter. OTOH, maybe should just be reusing the ImageIO infrastructure for this anyway?

[eirikbakke]

@neilcsmith-net The IconLoader SPI sounds simple enough. It's just an extra level of indirection. If people want it I can make it part of this commit.

ImageIO is not the best interface to use here, because we need to load javax.swing.Icon instances, not java.awt.Image.

Personally I'd move SVGLoader and the Batik implementation outside of ImageUtilities, and have the former lazily load the latter.
To make lazy loading work, as far as I know, there would still have to be an "SVG Loader" module with an SVGLoader interface and a separate "SVG Loader Implementation" module with an SVGLoaderImpl service provider, where the latter is the lazily loaded one that depends on Batik. Though perhaps I could then make the "SVG Loader" module be only a "friend" of the "SVG Loader Implementation" module, instead of making the SVGLoader interface public.

Is something like this what you have in mind?:

public interface IconLoader {
    /**
     * @return null if this loader elects not to try to load this icon.
     * throws IOException if this loader is responsible for loading the icon,
     *        but there were errors and ImageUtilities should not attempt to
     *        load the icon using the default method
     */
    Icon loadIcon(String resource, boolean localized) throws IOException
}

[neilcsmith-net]

ImageIO is not the best interface to use here, because we need to load javax.swing.Icon instances, not java.awt.Image.

Just a thought having noticed there was a plugin for Batik. It looks like you're always using cached Images? Therefore it might be viable to use the ImageIO sized rendering support for creating the cached sizes without requiring a specific additional SPI at all. The HiDPIIcon could then theoretically be non-abstract.

To make lazy loading work, as far as I know, there would still have to be an "SVG Loader" module with an SVGLoader interface and a separate "SVG Loader Implementation" module with an SVGLoaderImpl service provider, where the latter is the lazily loaded one that depends on Batik.

That's what I meant, but I might have badly worded it!

The IconLoader interface is pretty much what I had in mind. Or anything similar that allows additional overrides and/or removes the filetype logic from inside ImageUtilities itself. I'll shut up and let anyone else agree or disagree with that point now! 😄 Other than that I think this is a great step forward.

[eirikbakke]

That's what I meant, but I might have badly worded it!

No, that's good--it just means we're on the same page! :-)

I'll let the ideas simmer for a bit, and see if I get an answer to the other question (how to make a module actually optional). Then I can have a look at the IconLoader approach.

[emilianbold]

I'm curious which apps use the platform and would use a modern Apache NetBeans Platform version but wouldn't want to support HiDPI screens?

No, but you assume that everyone would want to approach it in this way, as opposed to icon fonts or responsive bitmaps, both of which have the benefit of relying solely on core Java desktop support.

It is also feasible to have a lightweight, single file, SVG renderer that would be enough for this - Processing has one, but not under a usable license.

I would still prefer to see some sort of generic IconLoader SPI (and similar for branding text) such that we allow for IDE modules and RCP apps to use / experiment with alternative approaches that work for them.

There are actually quite smart ideas! I just think that after 6 years since the Retina display was announced any kind of support for HiDPI screen is good.

If we want to provide a HiDPI SPI so we have multiple solutions that would be even better. But remember that today we have no solution.

As far as I can tell this code doesn't introduce public API (except the .svg icon contract) so besides the JAR bloat, it's an internal feature. I dislike bloat, but remember a full NetBeans build is 1GB!

It would also make sense that when a 2nd equally good HiDPI icons solution arrives, then we take the time to invent a SPI and have both solutions implementat that SPI, etc.

Anyhow, Eirik is free to do however he decides, I reviewed the patches, they looked OK to me.

[JaroslavTulach]

There is nothing holy about the platform/lib folder?

There are three kinds of ClassLoader in NetBeans Runtime

• platform/lib JARs are loaded by application classloader
• platform/core JARs are loaded by enhanced NetBeans classloader, but fixed
• platform/modules JARs are loaded by enhanced NetBeans classloader and fully dynamic

To benefit from enhanced classloading features like compatibility patching or fast startup, then it is essential to avoid placing JARs into platform/lib (and/or platform/core) folder.

As a desktop platform, NetBeans Platform must support vectorial icons out of the box

Maybe, if desktop is your only focus, but ...

I'm curious which apps use the platform and would use a modern Apache NetBeans Platform
version but wouldn't want to support HiDPI screens?

... we are talking about the NetBeans Runtime Container. People like @jlahoda, @tzezula and @sdedic spent a significant amount of time making sure NetBeans modules can be used in a headless mode. We are not going to ruin such efforts just because we want SVG icons.

"SVG Loader" module with an SVGLoader interface and a separate "SVG Loader Implementation"

That would seem OK from the runtime container point of view. However expect troubles at early stages of startup - for example you couldn't use SVG to render splash screen...

Good luck!

[matthiasblaesing]

Am Donnerstag, den 06.06.2019, 15:38 -0700 schrieb Eirik Bakke:

As for SVG files, at least these particular ones, I prefer to regard them as opaque files that shouldn't be hand-edited, since they will be re-generated by Adobe Illustrator every time we make changes to the icons.

This raises the question whether Adobe Illustrator is the right tool. I don't know the ratio of people having access to Adobe Illustrator, but I would prefer tools directly working on the SVG code. If needed for example with pure SVG code, we could transform the files at build time and strip comments and so on. However, Apache is pretty clear, that files, that can carry the license header should (very hard should) carry it. For SVG files this is the case. If Adobe Illustrator is not able to add a fixed comment (apparantly it can, as there is already one), additional tooling needs to do it (adding an XML comment will not be hard).

(Besides, SVG icons, which are loaded at runtime, will become about 25% larger on average if we add a license comment to each. Not sure it really matters.)

Depends - I tested the raw parsing speed (file -> DOM) and got a penalty of about 5% (undo.svg with and without comment). For transcoding to PNG I got a penalty of 4%. With caching this is a one- off problem. Seinng the SVG-PNG comparison, this speed problem looks not significant. The size itself IMHO is not problem - if it is, run the files through gzip and decompress von the fly. Am Samstag, den 08.06.2019, 20:41 -0700 schrieb Jaroslav Tulach:

> I'm curious which apps use the platform and would use a modern > Apache NetBeans Platform > version but wouldn't want to support HiDPI screens? ... we are talking about the NetBeans Runtime Container. People like @jlahoda, @tzezula and @sdedic spent a significant amount of time making sure NetBeans modules can be used in a headless mode. We are not going to ruin such efforts just because we want SVG icons.

That arguement does not cut it. There is net.java.html.* already in the platform. The same argument would cut there, as they are surely not running headless.

[neilcsmith-net]

That arguement does not cut it. There is net.java.html.* already in the
platform. The same argument would cut there, as they are surely not
running headless.

Except net.java.html.* is not in the NetBeans Runtime Container.

Not sure if lacking SVG for the splash screen is much of a problem? And there was another discussion about changing splash implementation recently.

I had a quick look for lighter weight SVG options under suitable licenses - nothing in just one class, but did find https://github.com/blackears/svgSalamander Haven't tried it!

[matthiasblaesing]

Am Sonntag, den 09.06.2019, 10:01 -0700 schrieb Neil C Smith:

I had a quick look for lighter weight SVG options under suitable licenses - nothing in just one class, but did find https://github.com/blackears/svgSalamander Haven't tried it!

I only had a quick look at it in the past. At that time it lacked CSS base font support. My gut feeling is, that there is a reason, that batik has the size it does.

[neilcsmith-net]

My gut feeling is, that there is a reason, that batik has the size it does.

Yes, but I'm going on the basis that's probably a lot of features not needed for icons?

Of course, another option might be compile-time class generation from SVG using Photon from https://github.com/kirill-grouchnikov/radiance ? Closer to @timboudreau suggestion above?

[eirikbakke]

I'll make sure to put Batik support in an optional module so it's not part of the core runtime container.

If it's required, I'm fine with putting licenses in the SVG files. (Alternatively we could use SVGZ instead, which is just a gzipped SVG file--clearly a binary file...)

The tool to use for drawing icons is really up to whoever volunteers their time or money to draw them. Though ideally we want people with some graphic design experience to draw icons--and among those people Adobe Illustrator is pretty standard.

[eirikbakke]

However expect troubles at early stages of startup - for example you couldn't use SVG to render splash screen...

Yes, the proposed HiDPI splash screen code takes another approach for this, using PNG instead. (See separate PR at #1246 . The ScaledBitmapIcon class in the latter PR will be simplified a bit once this PR is merged and CachedHiDPIIcon becomes a public class that it can extend from.)

[matthiasblaesing]

Am Sonntag, den 09.06.2019, 12:05 -0700 schrieb Eirik Bakke:

If it's required, I'm fine with putting licenses in the SVG files. (Alternatively we could use SVGZ instead, which is just a gzipped SVG file--clearly a binary file...)

We wan't to be good citizens of the Apache eco system, we should not unnessarily stretch the rules (a nitpicker would tell you, that you can gzip at build time ...).

[eirikbakke]

There's a remaining test case failing which may or may not be related to the changes here:

Testcase: testReadAccess(org.netbeans.core.startup.layers.CachingPreventsFileTouchesTest):	FAILED
No reads during startup
checkRead: Z:\nbsrc\incubator-netbeans\nbbuild\netbeans\platform\modules\net-java-html-geo.jar

I'll look at this one after rebasing on top of master, in case the failure goes away then.

[sdedic]

@sdedic An alternative here would be to actually include all of the Batik library JARs--we're already including most of them. This would increase the total size of Batik JARs from 2.7MB to 3.6MB (a 32% increase). Thoughts?

On one hand, I sort of like the separation - the current platform's use-case is to read SVG, not generate. So in this sense, you're including the minimum necessary to satisfy the requirement.

But if you, for some reason, include full batik, why not distribute batik-all ?

@jtulach what's the architect's view on including more-than-required from the library ?

[eirikbakke]

@sdedic Yes, batik-all would be an option as well. That one is 50% larger (4160KB) than the current subset of JARs.

[neilcsmith-net]

Following up on comment I made way earlier, but having just looked into #1652 it's interesting to see they have svgSalamander based icon support - https://github.com/JFormDesigner/FlatLaf/blob/master/flatlaf-extras/src/main/java/com/formdev/flatlaf/extras/FlatSVGIcon.java

[eirikbakke]

@neilcsmith-net Yeah, svgSalamander is a much smaller library. Presumably it implements a smaller subset of SVG features than Batik. The SPI introduced in this PR would make it possible to switch out the Batik-based module with a svgSalamander-based one, in case anyone wants to try this at a later point.

(In FlatLaf, it looks like FlatSVGIcon is only used from the DemoFrame, not in the LAF itself. The SVG files used are all very simple ones.)

[jtulach]

Dne čt 21. 11. 2019 19:52 uživatel Svatopluk Dedic <notifications@github.com> napsal:

@sdedic <https://github.com/sdedic> An alternative here would be to actually include *all* of the Batik library JARs--we're already including most of them. This would increase the total size of Batik JARs from 2.7MB to 3.6MB (a 32% increase). Thoughts? On one hand, I sort of like the separation - the current platform's use-case is to *read* SVG, not generate. So in this sense, you're including the minimum necessary to satisfy the requirement. But if you, for some reason, include full batik, why not distribute batik-all <https://mvnrepository.com/artifact/org.apache.xmlgraphics/batik-all> ? @jtulach <https://github.com/jtulach> what's the architect's view on including more-than-required from the library ?

I can find arguments for both directions. Which one do you want? ;-) We want vector icons in the platform. Clearly that leads to Batik. Include it as 3rd party lib (no friend deps). Make it optional - e.g. a module. That's all I want for now. -jt PS: Later I'd also like to avoid startup slowdown expected by the need to load XML parser and parse the .svg files.

[eirikbakke]

I'll go with the smaller batik library subset approach then.

[eirikbakke]

Force-pushing made some earlier reviews unavailable, but the requested changes have been taken into account, so marking these reviews as "dismissed" now.

Jaroslav's review is resolved--the SVG loader module is optional and new JARs are confirmed not to end up in platform/lib. Have to use the "dismiss review" feature since earlier force-pushing made the previous comments unavailable to "resolve."

[eirikbakke]

There seems to be some transient Travis failures... I had it all green yesterday. Will wait until I can make it pass again before merging.

[eirikbakke]

Two subsequent Travis builds, on the same unchanged codebase, yielded one transient test failure each, but in different test jobs.

https://travis-ci.org/apache/netbeans/builds/616390107
https://travis-ci.org/apache/netbeans/builds/616369975

So I take this to mean that all tests have passed... (since each of the failed jobs, "Test ide modules" and "Test platform modules, Batch 1", passed once in the other Travis run)

[neilcsmith-net]

Spurious Travis failures are somewhat par for the course - you can retrigger them from the Travis UI assuming your accounts are connected? Speaking of which, why do the commits show as ultorg rather than you personally? Author email should ideally be your Apache one.

[eirikbakke]

It's my real name and email in the git logs, but it didn't match the email that was associated with this github account. Now fixed to avoid confusion. (I prefer to use a real email address over the Apache one, to avoid a proliferation of email addresses that I have to check or maintain...)

you can retrigger them from the Travis UI assuming your accounts are connected
That's easier, will try that next time...

[matthiasblaesing]

The name shown on github is mostly useless, as github tries to be clever. If you need the real information, only git log (or its companions) show the truth. That said, in these cases, where the github data is not useable, "Squash and Merge" must not be used from the github GUI, as it destroys the valid author information (git rebase does this much better).

[eirikbakke]

I did a manual squash, saw no failing Travis tests, and merged. I also tested a private build of the platform with my NetBeans Platform app, which makes extensive use of SVG icon loading.

Added the following issues for the future:

• NETBEANS-3469 Cache PNG versions of loaded SVG icons
• NETBEANS-3468 Make HiDPI (Retina) icons work in MacOS menu bar

[JaroslavTulach]

Here are some post mortem API related notes.

[JaroslavTulach]

Missing @since annotation.

[eirikbakke]

I'll add this in a separate commit.

[JaroslavTulach]

The newly visible class should also get @since annotation.

[eirikbakke]

I'll add it.

[JaroslavTulach]

protected static methods seem slightly suspicious to me. Who's supposed to call them? Any subclass? But in fact everyone can do abstract class MyDummySubclass extends CachedHiDPIIcon { /* now call the method */ } and get access to that method. E.g. it doesn't provide any encapsulation over public static.

I am just stating that for your consideration. Maybe using protected static here is correct...

[eirikbakke]

Making it "protected" mainly makes it clear, from a documentation perspective, that it's a method intended for use by implementors of CachedHiDPIIcon, rather than clients of it.

Who's supposed to call them? Any subclass?

Yes. If they want. It's just a way to show and document to implementors how to create a suitable backbuffer.

But in fact everyone can do abstract class MyDummySubclass extends CachedHiDPIIcon { /* now call the method */ } and get access to that method.

That's fine. It's static, so it doesn't actually expose any object state.