On Thu, 7 Nov 2024 19:04:07 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 incrementally with two additional > commits since the last revision: > > - move automatically added imports > - rename test...javax -> test...java2d > 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. I have a patch for our closed test that I've been using for testing. I ran a Jenkins headful test job and with that (closed) patch + the latest patch in this PR and all looks good. I've looked over most of the code changes and it all looks good. I'll finish up next week. @johanvos might want to review the changes in `IosImageLoader.java` modules/javafx.graphics/src/main/java/com/sun/javafx/iio/png/PNGImageLoader2.java line 316: > 314: : ImageStorage.ImageType.RGB; > 315: case PNG_COLOR_PALETTE: > 316: return ImageStorage.ImageType.PALETTE; Why was this case removed? Is it not used? ------------- PR Review: https://git.openjdk.org/jfx/pull/1593#pullrequestreview-2424642136 PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1834895733
