Skip to content

Conversation

arunjose696
Copy link
Contributor

@arunjose696 arunjose696 commented Sep 15, 2025

Extend ImageLoader to able to load images at a given target height and width in addition to the current behavior of only being able to load images at a target zoom. This is required to load images in svg format at custom height and width.

The extensions added in this change is used in #2526 That PR also contains snippets through which these changes are tested.

This depends on #2514, Only last commit is relevant for this PR

Copy link
Contributor

github-actions bot commented Sep 15, 2025

Test Results

  118 files  ±0    118 suites  ±0   10m 21s ⏱️ +5s
4 440 tests ±0  4 418 ✅  - 2  17 💤 ±0  5 ❌ +2 
  298 runs  ±0    289 ✅  - 2   4 💤 ±0  5 ❌ +2 

For more details on these failures, see this check.

Results for commit 920ecbb. ± Comparison against base commit 810cbcc.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

Overall, this change looks good to me. I am just wondering if the term "target" in all the methods/variables is appropriate and helpful. In the zoom case, we have the distinction between fileZoom and targetZoom as we need to know which zoom an image represents. But for the size case there is no such distinction, as you just need to tell the size you want to have. So I think we should maybe remove all the "target" and just call everything width, height, loadBySize etc.

@arunjose696 arunjose696 force-pushed the arunjose696/326/imageLoader branch from ecd82db to fb8859f Compare September 24, 2025 16:17
@arunjose696
Copy link
Contributor Author

Overall, this change looks good to me. I am just wondering if the term "target" in all the methods/variables is appropriate and helpful. In the zoom case, we have the distinction between fileZoom and targetZoom as we need to know which zoom an image represents. But for the size case there is no such distinction, as you just need to tell the size you want to have. So I think we should maybe remove all the "target" and just call everything width, height, loadBySize etc.

Done

Extend ImageLoader to be able to load images at a given height and width
in addition to the current behaviour of only being able to load images
at a target zoom. This is required to load images in SVG format
at custom height and width.

Co-authored-by: Michael Bangas <michael.bangas@vector.com>
@HeikoKlare HeikoKlare force-pushed the arunjose696/326/imageLoader branch from fb8859f to 920ecbb Compare September 25, 2025 13:21
@HeikoKlare HeikoKlare changed the title Extend ImageLoader to able to load images at a given target height and width Extend ImageLoader to be able to load images at a given height and width Sep 25, 2025
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

I fixed a typo in the commit message and added @Michael5601 as co author as the code for the initial prototype was developed by him: Michael5601#2

@fedejeanne fedejeanne merged commit 7882209 into eclipse-platform:master Sep 26, 2025
15 of 17 checks passed
@fedejeanne fedejeanne deleted the arunjose696/326/imageLoader branch September 26, 2025 08:14
@Michael5601
Copy link
Contributor

Thank you for finishing the prototype. It's very nice to see all the work getting merged now :).
And thank you @HeikoKlare for remembering to mention my name on the changes.

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.

Extend ImageLoader to able to load images at a given target height and width
4 participants