Skip to content

[win32] Cleanup image wrappers #2036

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

akoch-yatta
Copy link
Contributor

This PR refactors Image in the win32 implementation as Image#imageProvider is final and always != null now. With this the handling and creation of new handles and ImageData was unified.

Relies on #2027 being merged first and includes this commit because of that.

Copy link
Contributor

github-actions bot commented Apr 17, 2025

Test Results

   545 files  + 6     545 suites  +6   28m 15s ⏱️ - 2m 53s
 4 373 tests +37   4 355 ✅ +35   18 💤 +3  0 ❌  - 1 
16 634 runs  +37  16 496 ✅ +35  138 💤 +3  0 ❌  - 1 

Results for commit 3accd93. ± Comparison against base commit 9859aae.

♻️ This comment has been updated with latest results.

@akoch-yatta akoch-yatta marked this pull request as ready for review April 22, 2025 07:27
@HeikoKlare HeikoKlare force-pushed the win32-cleanup-image-wrapper branch from 5de2869 to 0786ad3 Compare April 23, 2025 07:48
@akoch-yatta akoch-yatta force-pushed the win32-cleanup-image-wrapper branch 2 times, most recently from 616acf8 to 2e9c992 Compare April 23, 2025 11:06
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.

The overall change looks sound and is great cleanup step.
I only have one question regarding the adaptation for disabled/gray images.

return imageProvider.getImageMetadata(zoom);
} else {
ImageData resizedData = getImageData(zoom);
ImageData newData = adaptImageDataIfDisabledOrGray(resizedData);
Copy link
Contributor

Choose a reason for hiding this comment

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

This was removed at the place where this code was moved to. Was that intended, i.e., it is sufficient to only do the adaptation at the remaining places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, it was not completly correct. I want to cover all places, where scaled variants of an ImageData is created and those could be not adapted. My latest commit removed an unnecessary call (calling place already does it) and adds one call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! It looks fine now. I didn't find a path that is not covered properly. Still, we might think about if we could further improve this in the future as the it's quite difficult to understand where this adaptation logic has to be placed and why it is correct right now. And since it's hard to test, it may easily be removed at some of the placed accidentally, leading to errors that are not easily perceived.
But for now, it's fine as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. We should try to better streamline ImageData creation/scaling in Image

@akoch-yatta akoch-yatta force-pushed the win32-cleanup-image-wrapper branch from 2e9c992 to 7502fc6 Compare April 23, 2025 16:02
This commit refactors Image in the win32 implementation as
Image#imageProvider is final and always != null now. With this the handling
and creation of new handles and ImageData was unified.
@HeikoKlare HeikoKlare force-pushed the win32-cleanup-image-wrapper branch from 7502fc6 to 3accd93 Compare April 24, 2025 15:20
@HeikoKlare
Copy link
Contributor

Version increment check is failing for infrastructure reasons and no further version bump is required here.

@HeikoKlare HeikoKlare merged commit 62c2cb0 into eclipse-platform:master Apr 24, 2025
9 of 10 checks passed
@HeikoKlare HeikoKlare deleted the win32-cleanup-image-wrapper branch April 24, 2025 15:31
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