Skip to content

Conversation

@akoch-yatta
Copy link
Contributor

@akoch-yatta akoch-yatta commented Mar 17, 2025

This PR adapts the win32 implementation of Image, so ensure that the background color of an image is always set to all existing of later created image handles. Currently all handles that are created after the background color was set, would not receive the background color.

Prerequisite for

@github-actions
Copy link
Contributor

github-actions bot commented Mar 17, 2025

Test Results

   510 files  + 1     510 suites  +1   8m 26s ⏱️ -10s
 4 345 tests +37   4 331 ✅ +35   14 💤 +3  0 ❌  - 1 
16 613 runs  +37  16 502 ✅ +35  111 💤 +3  0 ❌  - 1 

Results for commit cc39aa1. ± Comparison against base commit 9440140.

♻️ This comment has been updated with latest results.

Copy link
Member

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

I would like to have a concrete way to test this though.
Also:

  • what was the bug that's being fixed?
  • Is it a bug or simply an inconsistency in the API contract?
  • Is there a test to make sure that an exception is thrown if setBackground(...) is called twice? If not, can you add it?
  • Is there a test that makes sure that images created afterward maintain the background? If not, can you add it?

@akoch-yatta
Copy link
Contributor Author

akoch-yatta commented Mar 17, 2025

I would like to have a concrete way to test this though. Also:

  • what was the bug that's being fixed?
  • Is it a bug or simply an inconsistency in the API contract?

I didn't actively saw the issue somewhere as this functionality is not used very much, but we should be able to create an issue with a Snippet. I will have a look into that.

  • Is there a test that makes sure that images created afterward maintain the background? If not, can you add it?

Yes, I can add one in the win32 specific tests

  • Is there a test to make sure that an exception is thrown if setBackground(...) is called twice? If not, can you add it?

No, but I can add one

This commit adapts the win32 implementation of Image, so ensure that the
background color of an image is always set to all existing of later
created image handles. Currently all handles that are created after the
background color was set, would not receive the background color.
@akoch-yatta akoch-yatta force-pushed the set-backgroundcolor-on-all-handles branch from e941580 to cc39aa1 Compare March 20, 2025 10:18
@akoch-yatta
Copy link
Contributor Author

I would like to have a concrete way to test this though. Also:

  • what was the bug that's being fixed?
  • Is it a bug or simply an inconsistency in the API contract?

I didn't actively saw the issue somewhere as this functionality is not used very much, but we should be able to create an issue with a Snippet. I will have a look into that.

@fedejeanne After having a more thorough look into the implementation I ran into the problem, that this functionality doesn't seem to have any (visible) effect anymore. I assume that was originally added to fix issues with transparency with older windows versions. I was not able to trigger any visible effect by this -> transparencyPixel always worked.

  • Is there a test that makes sure that images created afterward maintain the background? If not, can you add it?

Yes, I can add one in the win32 specific tests

I don't really see a way to test that automatically. As it was not possible to create a manual test as well, I don't see a chance here.

  • Is there a test to make sure that an exception is thrown if setBackground(...) is called twice? If not, can you add it?

No, but I can add one

I would rather push that to a separate PR or even leave it out. If there is no real use case of this functionality I don't see value in adding this.

Copy link
Member

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

The changes look OK. One final question: is transparentColor even needed anymore? I see that it's only used outside of Image (in GC) but it looks to me as if it can be replaced by the newly added backgroundColor.

WDYT @akoch-yatta ?

@akoch-yatta
Copy link
Contributor Author

The changes look OK. One final question: is transparentColor even needed anymore? I see that it's only used outside of Image (in GC) but it looks to me as if it can be replaced by the newly added backgroundColor.

WDYT @akoch-yatta ?

There is quite a similar logic involved there. I am just a bit hesitant to do something like that, because I don't fully understand the whole logic there.

@fedejeanne fedejeanne merged commit b91e012 into eclipse-platform:master Mar 21, 2025
14 checks passed
@fedejeanne fedejeanne deleted the set-backgroundcolor-on-all-handles branch March 21, 2025 07:50
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.

Set background color on all image handles

2 participants