Skip to content

Conversation

@HeikoKlare
Copy link
Contributor

This fixes a regression from #1989 as reported by @sratz: #1989 (comment)

A caching mechanism for ImageData introduced to some provider implementations inside the Image class leads to Image#getImageData() returning an ImageData object, which if modified affects the underlying Image. This contradicts the contract of that method, which states that changes to that object will not affect the Image.

This change ensures that Image#getImagetData() only returns objects that are not used internal of the Image instance. To this end, clones of the ImageData are created where necessary.

@HeikoKlare HeikoKlare linked an issue Aug 18, 2025 that may be closed by this pull request
@github-actions
Copy link
Contributor

github-actions bot commented Aug 18, 2025

Test Results

   546 files  ±0     546 suites  ±0   33m 40s ⏱️ + 4m 44s
 4 426 tests +1   4 409 ✅ +1   17 💤 ±0  0 ❌ ±0 
16 750 runs  +4  16 623 ✅ +4  127 💤 ±0  0 ❌ ±0 

Results for commit 8deabcf. ± Comparison against base commit a641cdb.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@ShahzaibIbrahim ShahzaibIbrahim left a comment

Choose a reason for hiding this comment

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

It fixes the contract "Modifications made to the returned {@code ImageData} will not affect this Image" but it breaks the other tests: test_imageDataIsCached, test_getBoundsInPixels and test_getImageDataCurrentZoom.

@HeikoKlare
Copy link
Contributor Author

@ShahzaibIbrahim I adapted test_imageDataIsCached but I don't see that the other tests you mention are affected as well. Can you elaborate on them?

@ShahzaibIbrahim
Copy link
Contributor

ShahzaibIbrahim commented Aug 18, 2025

@HeikoKlare I tried with your latest changes and they are not failing anymore. I cannot mark my review stale for some reason.

A caching mechanism for ImageData introduced to some provider
implementations inside the Image class leads to Image#getImageData()
returning an ImageData object, which if modified affects the underlying
Image. This contradicts the contract of that method, which states that
changes to that object will not affect the Image.

This change ensures that Image#getImagetData() only returns objects that
are not used internal of the Image instance. To this end, clones of the
ImageData are created where necessary.
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.

Change makes sense for the described regression as it will clone the ImageData when they are returned via API call. This will ensure the Image itself to be unchanged. I tested the changed with and without monitor specific scaling for regression and didn't find any.
Good to go from my side

@HeikoKlare HeikoKlare merged commit 93c0ce9 into eclipse-platform:master Aug 19, 2025
17 checks passed
@HeikoKlare HeikoKlare deleted the regression-1989 branch August 19, 2025 09:56
@sratz
Copy link
Member

sratz commented Aug 19, 2025

Thanks @HeikoKlare! I have also verified the fix in our product.

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.

Image#getImageData() violates contract

4 participants