Skip to content

Conversation

@HeikoKlare
Copy link
Contributor

When drawing an image at a specified size, currently a new image handle is always created based on on-demand loaded image data. In case the fallback to loading image data for the best fitting zoom is used, a handle for that zoom may already exist and could be reused but is currently not considered.

With this change, an existing handle is used if appropriate instead of unnecessarily creating a temporary handle.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 15, 2025

Test Results

  118 files    118 suites   14m 14s ⏱️
4 649 tests 4 632 ✅ 17 💤 0 ❌
  330 runs    326 ✅  4 💤 0 ❌

Results for commit 3d1c6f4.

♻️ This comment has been updated with latest results.

@HeikoKlare HeikoKlare marked this pull request as ready for review October 15, 2025 16:00
@akoch-yatta
Copy link
Contributor

akoch-yatta commented Oct 29, 2025

Overall this PR looks good and seem to work properly due to my testing.

But I was wondering, why don't we do a similar check in HandleAtSize#createHandleAtExactSize as well? So iterating over the handles and seeing of one size matches - each ImageHandle knows its size. With CTabRendering you immediately run into the scenario where a handle (for a SVG) is created unnecessarily. I am fine with doing that in a separate PR, but does anything speak against it?

Copy link
Contributor

@akoch-yatta akoch-yatta left a comment

Choose a reason for hiding this comment

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

Code looks solid. I did not really run into the scenario to resuse a handle from the zoomLevelToImageHandle map in HandleAtSize#getOrCreateImageHandleAtClosestSize without making sure to create a fitting handle first in the runtime. But doing that it worked as expected.

@HeikoKlare HeikoKlare force-pushed the imageatsize-reuse-zoom-handle branch from 3830985 to 12d5fc5 Compare October 29, 2025 10:15
@HeikoKlare
Copy link
Contributor Author

But I was wondering, why don't we do a similar check in HandleAtSize#createHandleAtExactSize as well? So iterating over the handles and seeing of one size matches - each ImageHandle knows its size. With CTabRendering you immediately run into the scenario where a handle (for a SVG) is created unnecessarily. I am fine with doing that in a separate PR, but does anything speak against it?

That definitely makes sense. I added the simple required logic with 12d5fc5.
I am not completely sure anymore why I didn't do it before. As you pointed out for the already implemented case of searching for a fitting handle first, you need to be in a situation where a zoom-based handle has been created at all. In many cases, you may only use the draw-at-size functionality without ever requesting a handle at zoom. Maybe that's why I didn't implement it (along with thinking that the implementation would be more complicated). Anyway, now all potential reuse scenarios should be covered.


private Optional<ImageHandle> getHandleAtExactSize(int width, int height) {
for (ImageHandle handle : zoomLevelToImageHandle.values()) {
if (handle.width == width && handle.height == height) {
Copy link
Contributor

@arunjose696 arunjose696 Oct 29, 2025

Choose a reason for hiding this comment

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

I think this would cause a slight issue as if the image was requested at for eg 2X the height elsewhere, they would not be loaded with imageDataAtSizeProvider, we would then end up using this image while doing the drawing even though it is possible to load the image at the given target size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! That was exactly the reason for not doing it. I just adapted the PR again. Maybe we can still integrate something like this as a follow up in case we maybe revise the ImageDataAtSizeProvider.

@HeikoKlare HeikoKlare force-pushed the imageatsize-reuse-zoom-handle branch from 12d5fc5 to 005dc7f Compare October 29, 2025 12:40
When drawing an image at a specified size, currently a new image handle
is always created based on on-demand loaded image data. In case the
fallback to loading image data for the best fitting zoom is used, a
handle for that zoom may already exist and could be reused but is
currently not considered.

With this change, an existing handle is used if appropriate instead of
unnecessarily creating a temporary handle.
@akoch-yatta akoch-yatta force-pushed the imageatsize-reuse-zoom-handle branch from 005dc7f to 3d1c6f4 Compare October 29, 2025 16:52
@akoch-yatta akoch-yatta merged commit 3c87f89 into eclipse-platform:master Oct 29, 2025
17 checks passed
@akoch-yatta akoch-yatta deleted the imageatsize-reuse-zoom-handle branch October 29, 2025 17:20
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.

Reuse existing handles when drawing image at size

3 participants