Skip to content

Allow to test for loading image at zoom and fix faulty Win32 image load #1987

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

HeikoKlare
Copy link
Contributor

@HeikoKlare HeikoKlare commented Apr 3, 2025

The ImageLoader and FileFormat implementations currently combine the check whether an image can be provided at a specific scale value and the provision of an image in the scale itself. In case a consumer wants to test whether an image can be provided at a specific scale, it needs to request that image at that scale, even if it will not be used later one, e.g., because it cannot be provided in the required scale.

With this change, ImageLoader and FileFormat provide separate methods for validating whether an image can be retrieved at a specific zoom (such as from an SVG) and the retrieval of the image itself. This is employed by the Cocoa Image implementation to avoid unnecessary load operations and in the Win32 Image implementation to correct erroneous scaling from an already initialized handle instead of loading at proper zoom (such as for an SVG). In addition, this fixes a faulty reuse of the
same stream in the Cocoa Image implementation at the same place. See eclipse-platform/eclipse.platform#1797 (review) and eclipse-platform/eclipse.platform.ui#2643 (review).

This will specifically be useful for consumers that need to load images on their own (such as the URLImageFileNameProvider and URLImageDataProvider in JFace), as they would otherwise need to test for the ability to retrieve an image at a specific scale by actually retrieving the image without using the loaded image data later on.

Copy link
Contributor

github-actions bot commented Apr 3, 2025

Test Results

   545 files  + 6     545 suites  +6   34m 32s ⏱️ + 3m 33s
 4 369 tests +37   4 357 ✅ +36   12 💤 +3  0 ❌  - 2 
16 624 runs  +37  16 515 ✅ +36  109 💤 +3  0 ❌  - 2 

Results for commit 3057ab7. ± Comparison against base commit 6a2b931.

♻️ This comment has been updated with latest results.

@HeikoKlare HeikoKlare force-pushed the image-file-zoom-check branch from 4ad1d7b to 4b61103 Compare April 3, 2025 18:41
@HeikoKlare HeikoKlare changed the title Allow to test for loading image at zoom Allow to test for loading image at zoom and fix faulty Win32 image load Apr 3, 2025
Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case a consumer wants to test whether an image can be provided at a specific scale, it needs to request that image at that scale, even if it will not be used later one, e.g., because it cannot be provided in the required scale.

This is definitively use-full, but the current implementation seems to have the problem that it re-uses the initially given InputStream without resetting it because the check if an image can be loaded at a specific zoom involves determining it's format, which reads a portion of the beginning of the stream. IIRC this was also the reason why the check for scaling and loading the image was combined in one method, because internally the stream was wrapped into a LEDataStream that was reset then.
I assume that wrapping has to be moved further up the call-chain and then, maybe it would help to retrieve the FileFormat instance for a given stream and reuse it. Then it doesn't has to determined multiple times. Of course one has to pay attention for Linux where the FileFormat is not to load static images.

Besides that I currently just have a few minor remarks how this could be a bit simpler.

@HeikoKlare HeikoKlare force-pushed the image-file-zoom-check branch 2 times, most recently from a88300f to f767746 Compare April 4, 2025 09:54
@HeikoKlare
Copy link
Contributor Author

but the current implementation seems to have the problem that it re-uses the initially given InputStream without resetting it because the check if an image can be loaded at a specific zoom involves determining it's format,

Indeed, I missed that point. Actually, there was a mistake in the Cocoa implementation reusing the same stream for loading an image multiple times as well, which I now fixed in this PR. To avoid overcomplicating this, I have reduced the PR to only add the new method. In many use cases, there will be no reuse necessity for the stream anyway, which is why I now simply kept the existing implementation of the load method and added a validation method, which is used at specific places. As said, in the Cocoa implementation a stream was erroneously reused anyway, which I solved in the same way the Win32 implementation already does it: store the stream contents and wrap then into a stream on demand.

@HeikoKlare HeikoKlare force-pushed the image-file-zoom-check branch from f767746 to b3bcd2e Compare April 4, 2025 10:00
@HeikoKlare HeikoKlare marked this pull request as ready for review April 4, 2025 10:36
@HeikoKlare HeikoKlare force-pushed the image-file-zoom-check branch from b3bcd2e to f98d55a Compare April 4, 2025 14:13
The ImageLoader and FileFormat implementations currently combine the
check whether an image can be provided at a specific scale value and the
provision of an image in the scale itself. In case a consumer wants to
only test whether an image can be provided at a specific scale, it needs
to request that image at that scale, even if it will not be used later
one, e.g., because it cannot be provided in the required scale.

With this change, ImageLoader and FileFormat provide separate methods
for validating whether an image can be retrieved at a specific zoom
(such as from an SVG) and the retrieval of the image itself. This is
employed by the Cocoa Image implementation to avoid unnecessary load
operations and in the Win32 Image implementation to correct erroneous
scaling from an already initialized handle instead of loading at proper
zoom (such as for an SVG). In addition, this fixes a faulty reuse of the
same stream in the Cocoa Image implementation at the same place.
@HeikoKlare HeikoKlare force-pushed the image-file-zoom-check branch from f98d55a to 3057ab7 Compare April 4, 2025 14:41
Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I missed that point. Actually, there was a mistake in the Cocoa implementation reusing the same stream for loading an image multiple times as well, which I now fixed in this PR. To avoid overcomplicating this, I have reduced the PR to only add the new method.

Yes, indeed. Thank you for the update and the fix. This looks good to me now.

@HannesWell HannesWell merged commit 83257cc into eclipse-platform:master Apr 5, 2025
15 checks passed
@HeikoKlare HeikoKlare deleted the image-file-zoom-check branch April 27, 2025 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants