RFR: 8329098: Support "@1x" image naming convention as fallback

[Kevin Rushforth]

On Fri, 11 Oct 2024 10:57:34 GMT, Lukasz Kostyra <lkostyra at 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 `checkers at 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