On Wed, 16 Oct 2024 11:30:26 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.
>
> Lukasz Kostyra has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review fixes
>
> - Change exception message in loadAll() to include original resource
> path
> - Add a test case to ImageStorageTest checking if we load a correct
> resource when both <res> and <res>@1x are present
@mstr2 as you're busy in the `ImageStorage` class as well and know how it
works, perhaps you could take a peek at this PR as well.
modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java line
385:
> 383: try {
> 384: theStream =
> ImageTools.createInputStream(input);
> 385: } catch (IOException ignored) {
It looks like this abstracts away errors like `FileNotFoundException`, and it
will do a fallback in all cases, even if it is not a 404 or
`FileNotFoundException` -- errors like network problems, permission problems,
etc, will also trigger a fallback (likely resulting in the same error). But it
could also be a different error, hiding the original problem.
In Java we have the option to add a 'surpressed' exception. You could attach
it to the 2nd exception, so the original cause of the problem (which could be a
permission problem for example) is not lost.
Maybe I'm being too picky :)
modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java line
392:
> 390: try {
> 391: // last fallback, try to see if the file
> exists with @1x suffix
> 392: String scaled1xName =
> ImageTools.getScaledImageName(input, 1);
I think this function does not properly handle cases where the input may
already have a scale specified.
If I want to load `test...@1x.png` then this function will return
`testimg@1...@1x.png`
I'm not sure what is normal here, but I'd think that if a scale was already
specified that we shouldn't do any fallback.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1598#pullrequestreview-2372422385
PR Review Comment: https://git.openjdk.org/jfx/pull/1598#discussion_r1803081373
PR Review Comment: https://git.openjdk.org/jfx/pull/1598#discussion_r1803073434
