On Thu, 31 Oct 2024 15:42:56 GMT, Michael Strauß <mstra...@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 with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 23 additional > commits since the last revision: > > - Merge branch 'master' into feature/ximageloader > - review comments > - Merge branch 'master' into feature/ximageloader > - Merge branch 'master' into feature/ximageloader > - review changes > - scanline stride always measured in bytes > - remove unused code > - fix line endings > - fix line endings > - add support for palette-based image formats > - ... and 13 more: https://git.openjdk.org/jfx/compare/7ec744a2...ed478762 I'm just starting to look at this. The tests I ran look good so far, with two things to note: 1. If `XImageLoaderFactory` fails to load, it with throw an `Error` that terminates the app. I left a comment and suggestion inline. 2. This patch causes a failure in one of our closed white-box tests, which depends on internal API that has changed. It will be easy for us to fix it, but it will mean that down the road, when this is ready to go in, we will need to coordinate the timing of the integration with you. modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java line 633: > 631: Class<?> factoryClass = > Class.forName("com.sun.javafx.iio.javax.XImageLoaderFactory"); > 632: xImageLoaderFactory = > Optional.of((ImageLoaderFactory)factoryClass.getMethod("getInstance").invoke(null)); > 633: } catch (ReflectiveOperationException e) { This looks like a good approach. I simulated a failure (by adding a dependency on an unrelated module that was there at compile time, but I excluded at runtime) and it gets a `NoClassDefFoundError`. Suggestion: } catch (NoClassDefFoundError | ReflectiveOperationException e) { ------------- PR Review: https://git.openjdk.org/jfx/pull/1593#pullrequestreview-2410484943 PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1826059863
