Skip to content

[win32] Adapt GC zoom when using plain images #2180

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

@akoch-yatta akoch-yatta commented May 23, 2025

This PR adapts the zoom passed to the GC when it is initialized for a plain Image created via Image#Image(Device, int, int) with a given width and height. It will fall back to DPIUtil#getDeviceZoom() again to revert to original bevahiour and prevent blurry rescaling of old usages.

Copy link
Contributor

github-actions bot commented May 23, 2025

Test Results

   539 files   -  6     539 suites   - 6   33m 28s ⏱️ + 4m 6s
 4 361 tests  - 37   4 345 ✅  - 35   15 💤  - 3  1 ❌ +1 
16 682 runs   - 37  16 543 ✅  - 35  138 💤  - 3  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 7f4f512. ± Comparison against base commit 9d3227d.

This pull request removes 37 tests.
AllWin32Tests org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageDataForDifferentFractionalZoomsShouldBeDifferent
AllWin32Tests org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageShouldHaveDimesionAsPerZoomLevel
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testByteArrayTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testFileTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testHtmlTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromCopiedImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageData
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageDataFromImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testRtfTransfer
…

♻️ This comment has been updated with latest results.

This commit adapts the zoom passed to the GC when it is initialized for
a plain Image created via Image#Image(Device, int, int) with a given width
and height. It will fall back to DPIUtil#getDeviceZoom() again to revert
to original bevahiour and prevent blurry rescaling of old usages.
@HeikoKlare HeikoKlare force-pushed the adapt-default-image-gc-zoom branch from a2bef72 to 7f4f512 Compare May 23, 2025 13:16
@HeikoKlare
Copy link
Contributor

The issue was introduced with this PR:

It can, e.g., be seen in double-buffering scenarios. One such scenario can be reproduced with the WindowBuilder project when going back to this commit: eclipse-windowbuilder/windowbuilder@f69b7dd

Starting the application on Windows without monitor-specific scaling at 175% (or above) and creating a sample projects leads to this wrong scaling:
image

With the proposed fix, it will look like this:
image

With monitor-specific scaling enabled, it will like the above instead of having blurry scaling as before:
image

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.

This change should go in the upcoming release (and also in RC1 to have some remaining time of testing) to preserve the behavior of users of the Image(device, width, height) constructor on Windows.

I consider this change rather safe to merge as it consistutes a partial revert of the recently merged change:

So the risk that consumers already adapted to the changed behavior is low. At the same time, the risk that consumers that did not adapt will face unintended behavior is probably not that low.

@ptziegler
Copy link
Contributor

You guys really are the best! ❤️

@HeikoKlare
Copy link
Contributor

The thanks are yours, Patrick! ❤️ You have pointed us to that potential issue, otherwise we would have probably missed it.
Do you have any objections on merging this for RC1, @ptziegler?

@ptziegler
Copy link
Contributor

ptziegler commented May 23, 2025

Do you have any objections on merging this for RC1

Go for it.

@jonahgraham
Copy link
Contributor

Do you have any objections on merging this for RC1

IIUC the RC1 is already complete and about to be submitted to SimRel - see https://www.eclipse.org/lists/eclipse-dev/msg12407.html

@iloveeclipse
Copy link
Member

Do you have any objections on merging this for RC1

IIUC the RC1 is already complete and about to be submitted to SimRel - see https://www.eclipse.org/lists/eclipse-dev/msg12407.html

Not really, see eclipse-platform/eclipse.platform.releng.aggregator#3055

@merks
Copy link
Contributor

merks commented May 23, 2025

We've been spinning a bit...

We should contribute something today, and then we/I can contribute something newer tomorrow.

I have to run now though...

@HeikoKlare HeikoKlare added this to the 4.36 RC1 milestone May 23, 2025
@HeikoKlare
Copy link
Contributor

Thank you all! I will merge this now and then trigger another I-Build, which we may then contribute as (an update for) RC1.

The failing test is unrelated and already documented: #2098

@HeikoKlare HeikoKlare merged commit 925a294 into eclipse-platform:master May 23, 2025
14 of 17 checks passed
@HeikoKlare HeikoKlare deleted the adapt-default-image-gc-zoom branch May 23, 2025 15:01
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.

6 participants