On Fri, 11 Oct 2024 10:57:34 GMT, Lukasz Kostyra <lkost...@openjdk.org> wrote:
> This PR adds a title-mentioned fallback to `ImageStorage.java` and a test for
> that specific scenario.
>
> In `ImageStorage.loadAll()` we still prefer the Image path that was provided
> by the user, but if it is missing we will now try to add a `@1x` suffix to
> Image name and give it one last try.
>
> Added test includes a binary file `check...@1x.png`. This was added to
> differentiate it from the collection of `checker.png` files and their higher
> scale variants, but it is a direct copy of already existing `checker.png`
> test resource, just with a different name and suffix. Test should fail
> without a change in `ImageStorage.java` and pass with it.
I left one minor comment on the fallback and one minor comment on the test. I
don't think either of them are that important, so I'll approve it as is. I'll
reapprove if you decide to make any changes.
modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java line
392:
> 390: // last fallback, try to see if the file exists
> with @1x suffix
> 391: String scaled1xName =
> ImageTools.getScaledImageName(input, 1);
> 392: theStream =
> ImageTools.createInputStream(scaled1xName);
The fallback code looks good with one comment. Because you catch and ignore the
previous exception and let this one propagate, if neither the original image
name nor any `@1x`, `@2x`, ... name can be found, the exception thrown will no
longer have the original name in the exception message, but will instead have
the `@1x` name.
You can see this if you run with `-Dprism.verbose=true` and try to load a
non-existent file or URL.
Maybe catch the exception on line 386 and then rethrow that so the error
message is more understandable (alternatively, wrap the exception in a message
that has the original input String as part of its message)?
modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java
line 63:
> 61:
> 62: @Test
> 63: public void testImageNameFallbackTo1X() throws ImageStorageException {
This will test that the fallback works if there is an `@1x` name without the
base name being present. Good.
Can you think of a good way to test that it will _only_ fall back if there is
no image name without any `@Nx`? Other than creating a pair of images that are
different (e.g., differently sized) and seeing if it loads the one you expect,
I can't think of a good way. It might or might not be worth doing.
-------------
Marked as reviewed by kcr (Lead).
PR Review: https://git.openjdk.org/jfx/pull/1598#pullrequestreview-2367833926
PR Review Comment: https://git.openjdk.org/jfx/pull/1598#discussion_r1800240151
PR Review Comment: https://git.openjdk.org/jfx/pull/1598#discussion_r1800246848
