search

apache / netbeans

Apache NetBeans
https://netbeans.apache.org/
Apache License 2.0
2.66k stars 851 forks source link

[NETBEANS-2604] Support SVG icon loading from ImageUtilities #1278

Closed eirikbakke closed 4 years ago

eirikbakke commented 5 years ago

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 https://github.com/apache/netbeans/pull/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:

Thanks for your help!

Example SVG icons loaded and displayed in the IDE: SVG icon screenshot

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): SVG Icons Tested

Chris2011 commented 5 years ago

Finally :) great work :)

matthiasblaesing commented 5 years ago

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 commented 5 years ago

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"));
  1. 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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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.

emilianbold commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 ( https://github.com/apache/netbeans/pull/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 https://github.com/apache/netbeans/pull/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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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! :smile: Other than that I think this is a great step forward.

eirikbakke commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

There is nothing holy about the platform/lib folder?

There are three kinds of ClassLoader in NetBeans Runtime

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/apache/netbeans/pull/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 commented 5 years ago

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 commented 5 years ago

Today's changes above are just a rebase on top of master, removing the commit that represented the now-merged https://github.com/apache/netbeans/pull/1273 . Changes that deal with the remaining TODOs will be added as separate commits later.

eirikbakke commented 5 years ago

I'm now moving Batik libraries out of the "platform" cluster and into the "ide" cluster. I'm also moving the o.apache.commons.io and o.apache.commons.logging modules back into "ide".

When I now run my NetBeans Platform app, I get the following error message:

Warning - could not install some modules:
  org.netbeans.libs.batik - The module named org.apache.commons.logging was needed and not found.
  org.openide.util.ui.svg - The module org.netbeans.libs.batik would also need to be installed.

Presumably this is because my Maven-based platform app does not include the "ide" cluster (which would have included org.apache.commons.logging). I tried to include a direct dependency on the org.apache.commons.logging module from my platform app's pom.xml, like this:

<dependency>
  <groupId>org.netbeans.external</groupId>
  <artifactId>org-apache-commons-logging</artifactId>
  <version>${netbeans.version}</version>
  <scope>runtime</scope>
</dependency>

However, I still get the same error message ("The module named org.apache.commons.logging was needed and not found."). Does anyone know how I might fix this?

ebarboni commented 5 years ago

@eirikbakke would it be possible have the Manifest.MF of the jar for org.netbeans.libs.batik module you generate.

You do it with nbm-maven-plugin ?

For our project we have a wrapper on batik but its excluding every commons-logging to use the external as you did. But this is not possible I guess to build with ant.

eirikbakke commented 5 years ago

The org.netbeans.libs.batik module will be part of the netbeans codebase, so that one I'm compiling with ant as part of the entire NetBeans build. But my NetBeans Platform application is built with nbm-maven-plugin. I just want to make sure that the new modules added in this PR plays well both within the IDE and from Platform applications.

The org.netbeans.libs.batik module has the following manifest:

Manifest-Version: 1.0
OpenIDE-Module: org.netbeans.libs.batik
OpenIDE-Module-Implementation-Version: 1
OpenIDE-Module-Localizing-Bundle: org/netbeans/libs/batik/Bundle.properties
AutoUpdate-Show-In-Client: false

and the following is its project.xml:

<project xmlns="http://www.netbeans.org/ns/project/1">
    <type>org.netbeans.modules.apisupport.project</type>
    <configuration>
        <data xmlns="http://www.netbeans.org/ns/nb-module-project/2">
            <code-name-base>org.netbeans.libs.batik</code-name-base>
            <module-dependencies>
                <dependency>
                    <code-name-base>org.apache.commons.io</code-name-base>
                    <run-dependency/>
                </dependency>
                <dependency>
                    <code-name-base>org.apache.commons.logging</code-name-base>
                    <run-dependency/>
                </dependency>
            </module-dependencies>
            <public-packages>
                <package>org.apache.batik.anim.dom</package>
                <package>org.apache.batik.bridge</package>
                <package>org.apache.batik.constants</package>
                <!-- ... More packages go here ... -->
            </public-packages>
            <class-path-extension>
                <runtime-relative-path>ext/batik-svg-dom-1.11.jar</runtime-relative-path>
                <binary-origin>external/batik-svg-dom-1.11.jar</binary-origin>
            </class-path-extension>
            <!-- ... Lots more batik-XXX-1.11.jars go here ... -->
            <class-path-extension>
                <runtime-relative-path>ext/xmlgraphics-commons-2.3.jar</runtime-relative-path>
                <binary-origin>external/xmlgraphics-commons-2.3.jar</binary-origin>
            </class-path-extension>
            <class-path-extension>
                <runtime-relative-path>ext/xml-apis-ext-1.3.04.jar</runtime-relative-path>
                <binary-origin>external/xml-apis-ext-1.3.04.jar</binary-origin>
            </class-path-extension>
        </data>
    </configuration>
</project>
ebarboni commented 5 years ago

How you generate the batik module mavenized ?

I would suggest to populate complely using nb-maven-plugin using a forced version on your own after ant, ant build-javadoc,ant build-source-zips ant build-nbms

mvn org.apache.netbeans.utilities:nb-repository-plugin:1.5-SNAPSHOT:download -DnexusIndexDirectory=${env.WORKSPACE}/repoindex -Dmaven.repo.local=${env.WORKSPACE}/.repository -DrepositoryUrl=https://repo.maven.apache.org/maven2 mvn org.apache.netbeans.utilities:nb-repository-plugin:1.5-SNAPSHOT:populate -DnexusIndexDirectory=${env.WORKSPACE}/repoindex -DnetbeansNbmDirectory=${env.WORKSPACE}/netbeanssources/nbbuild/nbms -DnetbeansInstallDirectory=${env.WORKSPACE}/netbeanssources/nbbuild/netbeans -DnetbeansSourcesDirectory=${env.WORKSPACE}/netbeanssources/nbbuild/build/source-zips -DnetbeansJavadocDirectory=${env.WORKSPACE}/netbeanssources/nbbuild/build/javadoc -Dmaven.repo.local=${env.WORKSPACE}/.repository -DparentGAV=org.apache.netbeans:netbeans-parent:1 -DforcedVersion=11.1-TEST -DskipInstall=true -DdeployUrl=file://${env.WORKSPACE}/testrepo/.m2

eirikbakke commented 5 years ago

Yes, that's what I'm doing ("ant build-nbms", and then using nb-repository-plugin to populate my local Maven repo with a forced version). Then I build my Platform app with the custom-built version of the platform.

eirikbakke commented 5 years ago

OK, I found out how to avoid the error. The behavior is still a bit puzzling, because the org.apache.commons.logging module seems to behave differently than other runtime dependencies.

My platform app's dependencies work like this:

Application module (packaging=nbm-application), depends on:
* platform cluster
* My application's Branding module
* My application's "Kit" module

My application's "Kit" module (packaging=nbm), depends on:
* Main application dependencies.
* org-netbeans-core-output2 (runtime dependency)
* org-netbeans-modules-notifications (runtime dependency)
* JDBC drivers (runtime dependency)
* ...various other runtime dependencies...
* org-openide-util-ui-svg (runtime dependency)
* org-apache-commons-logging (runtime dependency)

Turns out, if I move the org-apache-commons-logging dependency to the "Application" module instead of the "Kit" module, the error goes away...

eirikbakke commented 5 years ago

@neilcsmith-net I'm now revisiting the IconLoader approach. For this approach, the following modules would be required:

In contrast, the previous approach taken in this PR required the following:

Do you think the added complexity (2 extra modules) justifies adding the IconLoader API at this point?

(Note that in both of these approaches, the SVGLoader and Batik Library modules are optional.)

eirikbakke commented 5 years ago

I might have some time to work on this again two weeks from now... if someone prefers one or the other approach (3-module vs. 5-module approach, see previous comment), just let me know.

eirikbakke commented 5 years ago

Hearing no objections, I ended up going back to the 3-module approach, since the more general IconLoader approach turned out to be more complex (see my previous post).

Today's commits should address the remaining issues. Most notably, the SVG loader module and the associated Batik libraries have been moved out of the platform cluster, and verified to be actually optional. When the SVG module is present, ImageUtilities supports loading of explicitly named SVG files. Additionally, attempting to load "icon.png" will load "icon.svg" instead if the latter file is present in the same location. This will allow SVG icons to be added to NetBeans libraries without breaking compatibility with platform applications that do not wish to add a dependency on the SVG loader module.

(One of the 12 Travis checks fail, but I think it's unrelated to these commits, since it's a mysql-related thing.)

neilcsmith-net commented 5 years ago

Most notably, the SVG loader module and the associated Batik libraries have been moved out of the platform cluster, and verified to be actually optional.

Personally still prefer them in the Platform cluster, just actually optional. There's a difference there, and surely an RCP app is likely to want to use this?

(One of the 12 Travis checks fail, but I think it's unrelated to these commits, since it's a mysql-related thing.)

Yes, breaking 50% of the time at the moment - I'll retrigger (again!)

eirikbakke commented 5 years ago

Personally still prefer them in the Platform cluster, just actually optional.

I'm not very familiar with the clusters system--could you explain how to do this? Is this different from what I started with in the first version of this PR? For that version, @JaroslavTulach objected that new libraries were ending up in netbeans/platform/lib, which was undesirable. That included libraries that were previously already in the ide cluster (o.apache.commons.io and o.apache.commons.logging).

timboudreau commented 5 years ago

I'm not very familiar with the clusters system--could you explain how to do this? Is this different from what I started with in the first version of this PR? For that version, @JaroslavTulach https://github.com/JaroslavTulach objected that new libraries were ending up in netbeans/platform/lib, which was undesirable. That included libraries that were previously already in the ide cluster ( o.apache.commons.io and o.apache.commons.logging).

The cluster system actually dates back to Sun's architecture review requirements - but it is a generally good idea. The idea - driven by requirements from Solaris - was that an application should only install ONE copy of any shared libraries on a system; and Sun was building multiple products on top of NetBeans. So if there were two NetBeans-based applications on a system, there should only be one copy of, say, the platform modules or the editor modules, which both applications simply use as "clusters" of libraries. So the cluster system was invented - carve the IDE and platform into chunks of relatied (and interdependent) functionality, such that inter-cluster dependencies are unidirectional and you - a directed graph.

In practice, in terms of people installing multiple copies of software on systems, I don't think that is an enormous concern these days (this was back in the era of thin-clients, so a "system" was likely big iron and the "desktop" was a remote X connection). But it forces us to think about both macro-level dependencies - what functionality is and isn't related, what is "essential" and what isn't, and what is "platform" vs. something else, and factor things accordingly. That keeps a clean definition of what is "platform" and what isn't, and keeps the IDE and platform from merging into a monolithic hairball (which it once, circa 1999, was). So it's a good thing to keep.

Now, re SVG icons, that seems to me to be something which obviously belongs in the platform. Display resolution is not going to go down and we are past the point where raster formats for this sort of thing are inadequate; mobile platforms typically have you create raster icons in multiple resolutions, which is annoying and goes out of date with the next model; and on the desktop, we're not dealing with standardized screen sizes and resolutions, so the only sane way to deal that is a resolution-independent graphics format.

So, this belongs in the platform, period.

If the objection is that the IDE itself is not yet using SVG icons, so it will result in library dependencies getting packaged which nothing uses, that's a build-system problem, not an architecture problem. Fix the build system so the application-building phase detects that there are libraries which are not referenced in the manifest of any module or other library, and deletes those. That's as simple as building a graph of inter-JAR dependencies and then seeing if what you actually get is more than one distinct graph, and deleting everything that is not in the largest graph which is the actual application.

-Tim

eirikbakke commented 4 years ago

Thanks! Based on @timboudreau's comment, I will move the optional SVG loader implementation module (openide.util.ui.svg), as well as its dependencies libs.batik, o.apache.commons.io, and o.apache.commons.logging, back into the platform cluster.

The SVG loader implementation module is still written to be optional, but will be included in any platform application that depends on the entire "platform" cluster. Is this acceptable?

JaroslavTulach commented 4 years ago

The SVG loader implementation module is still written to be optional, but will be included in any platform application that depends on the entire "platform" cluster. Is this acceptable?

I think so. It seems acceptable. The problem would be "unavoidable dependency ... into core NetBeans Platform" - e.g. if the modules ended up in platform/lib directory. If they are in platform/modules, then be it.

eirikbakke commented 4 years ago

Alright, the last commit moves the modules back into the platform cluster (I'll squash all of these before merging). I have confirmed that the new JARs do not end up in platform/lib. Here are the JARs that result:

platform/modules/org-openide-util-ui-svg.jar

platform/modules/ext/batik-anim-1.11.jar
platform/modules/ext/batik-awt-util-1.11.jar
platform/modules/ext/batik-bridge-1.11.jar
platform/modules/ext/batik-constants-1.11.jar
platform/modules/ext/batik-css-1.11.jar
platform/modules/ext/batik-dom-1.11.jar
platform/modules/ext/batik-ext-1.11.jar
platform/modules/ext/batik-gvt-1.11.jar
platform/modules/ext/batik-i18n-1.11.jar
platform/modules/ext/batik-parser-1.11.jar
platform/modules/ext/batik-script-1.11.jar
platform/modules/ext/batik-svg-dom-1.11.jar
platform/modules/ext/batik-util-1.11.jar
platform/modules/ext/batik-xml-1.11.jar
platform/modules/org-netbeans-libs-batik.jar

platform/modules/org-apache-commons-io.jar (moved from ide/modules)
platform/modules/org-apache-commons-logging.jar (moved from ide/modules)

This should make the PR ready to merge. I'll merge in a week or two if there are no objections.

sdedic commented 4 years ago

@eirikbakke Since there are so many batik* libraries, please consider to collect them to a subfolder.

I'd suggest to rename the org.netbeans.libs.batik to org.netbeans.libs.batik.read or something to indicate it's not complete Batik. For example I have an application that writes SVGs through Batik (using its Grahics2D) - right now it embeds batik-all, so I'll have to adapt to include just the svggen + rely on this platform lib in the future.

eirikbakke commented 4 years ago

@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?

The additional JARs in this case would be the following:

batik-svggen-1.11.jar
batik-swing-1.11.jar
batik-gui-util-1.11.jar
batik-transcoder-1.11.jar
batik-codec-1.11.jar
batik-extension-1.11.jar

I'd still be excluding Batik command-line app JARs, their tests, and some other XML libraries whose name do not start with "batik-". The still-excluded JARs would be:

batik-all-1.11.jar
batik-rasterizer-1.11.jar
batik-slideshow-1.11.jar
batik-squiggle-1.11.jar
batik-svgbrowser-1.11.jar
batik-svgpp-1.11.jar
batik-svgrasterizer-1.11.jar
batik-test-1.11.jar
batik-test-svg-1.11.jar
batik-test-swing-1.11.jar
batik-ttf2svg-1.11.jar
batik-util-1.11-tests.jar
extensions/batik-rasterizer-ext-1.11.jar
extensions/batik-squiggle-ext-1.11.jar
fop-transcoder-allinone-2.3.jar
js.jar
serializer-2.7.2.jar
xalan-2.7.2.jar
xercesImpl-2.9.1.jar
xml-apis-ext-1.3.04.jar
eirikbakke commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

neilcsmith-net commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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

eirikbakke commented 4 years ago

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

eirikbakke commented 4 years ago

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 commented 4 years ago

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)