RFR: 8306707: Support pluggable image loading via javax.imageio [v6]

[John Hendrikx]

On Mon, 21 Oct 2024 13:27:08 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> This PR is an improved version of #1093.
>> 
>> JavaFX can load BMP, GIF, PNG, and JPEG images with its built-in image loaders. It has been a long-standing request to support more image formats, most notably (but not limited to) SVG. However, adding more built-in image loaders is a significant effort not only in creating the functionality, but also in maintaining the additional dependencies.
>> 
>> This will probably not happen any time soon, so we are left with three alternatives:
>> 1. Accept the fact that JavaFX will never be able to load additional image formats.
>> 2. Create a public image loader API, and hope that developers in the JavaFX ecosystem will create image loader plugins.
>> 3. Leverage the existing Java Image I/O API.
>> 
>> From these options, I think we should simply support existing Java APIs; both because it is the shortest and most realistic path forward, but also because I don't think it is sensible to bifurcate pluggable image loading in the Java ecosystem.
>> 
>> Of course, Java Image I/O is a part of the `java.desktop` module, which as of now, all JavaFX applications require. However, it has been noted in the previous PR that we shouldn't lock JavaFX into the `java.desktop` dependency even further.
>> 
>> I've improved this PR to not permanently require the `java.desktop` dependency: if the module is present, then JavaFX will use Image I/O for image formats that it can't load with the built-in loaders; if the module is not present, only the built-in loaders are available.
>> 
>> I have prepared a small sample application that showcases how the feature can be used to load SVG images in a JavaFX application: https://github.com/mstr2/jfx-imageio-sample
>
> Michael Strauß has updated the pull request incrementally with one additional commit since the last revision:
> 
>   scanline stride always measured in bytes

modules/javafx.graphics/src/main/java/com/sun/javafx/iio/javax/XImageLoader.java line 209:

> 207:                     : ImageStorage.ImageType.PALETTE;
> 208: 
> 209:                 var scanlineStride = switch(image.getSampleModel()) {

Just a general comment here, on the use of `var`; in my opinion, `var` helps the writer of the code, but almost never helps the reader of the code who now must read the RHS and manually determine what the type is.  As we should be aiming for maintainable code and code that's as easy to read as possible without having to guess (there's usually enough guessing involved already), I always find it hard how the use of `var` can be justified anywhere. 

Also in my experience, `var` complicates refactors as it will morph with the target type, making all refactor errors occur at the use location instead of the declaration site when `var` isn't used (ie. a refactor could indicate dozens of errors, while fixing 2 declarations could solve all of them).

So why make us guess that `scanlineStride` is an `int` here?  And why force us to read the right hand sides of `colorModel`, `palette` and `imageType`...

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1809990016