Skip to content

[win32] Fix inconsistency when an Image with GC is drawn on a GC #2387

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

Conversation

akoch-yatta
Copy link
Contributor

@akoch-yatta akoch-yatta commented Aug 6, 2025

This PR adapts the logic when a bitmap or icon is drawn with the GC in the windows implementation. As of now, the autoscale zoom was used to identify the image handle to use. If the image is itself drawn with a ImageGCDrawer, information about the original native zoom was lost, which led to inconsistencies in fonts depending on the auto scale mode. This PR forwards the native zoom in this case to enable the Image to initialize the GC accordingly. This was e.g. an issue with auto scale mode integer200 as described in #2385.

Copy link
Contributor

github-actions bot commented Aug 6, 2025

Test Results

   546 files  ±0     546 suites  ±0   31m 45s ⏱️ +29s
 4 425 tests ±0   4 408 ✅ ±0   17 💤 ±0  0 ❌ ±0 
16 746 runs  ±0  16 619 ✅ ±0  127 💤 ±0  0 ❌ ±0 

Results for commit b0b70ab. ± Comparison against base commit a71bea9.

♻️ This comment has been updated with latest results.

@akoch-yatta akoch-yatta force-pushed the win32-fix-concatenated-GC-zooms-with-images branch from b55ff03 to e462bce Compare August 7, 2025 14:49
@akoch-yatta
Copy link
Contributor Author

I noticed that getImageData(int) checks if the image is disposed but getImageMetadata(ZoomContext) doesn't. Couldn't that be a source of issues?

@fedejeanne The image is created few lines above and it will be disposed immediately after it anyway, but I don't see a change this can be disposed before.

@fedejeanne
Copy link
Contributor

In the call that I highlighted everything is fine because image.getImageData(nativeZoom) does check for dispose and, as you say, the image is disposed right away 👍

I was thinking more on the lines of: would it be worth playing it safe and checking for disposal when calling getImageMetadata(ZoomContext)?

At the moment, all calls seem safe because the (direct and indirect) callers check for disposal:

image

But I was just thinking: is checking for disposal a responsibility of the caller or should the method itself do it?

@akoch-yatta akoch-yatta force-pushed the win32-fix-concatenated-GC-zooms-with-images branch 2 times, most recently from 710097a to fa9747f Compare August 11, 2025 14:06
@HeikoKlare HeikoKlare force-pushed the win32-fix-concatenated-GC-zooms-with-images branch from fa9747f to b9f096c Compare August 11, 2025 15:03
@akoch-yatta akoch-yatta force-pushed the win32-fix-concatenated-GC-zooms-with-images branch 4 times, most recently from 61097cd to d77cbb7 Compare August 12, 2025 09:01
Copy link
Contributor

@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 fine and I see no regressions.

One nit comment: you need to add this to your commit message @akoch-yatta

Fixes https://github.com/eclipse-platform/eclipse.platform.swt/issues/2311

This commit adapts the logic when a bitmap or icon is drawn with the GC
in the windows implementation. As of now,  the autoscale zoom was used to
identify the image handle to use. If the image is itself drawn with a
ImageGCDrawer, information about the original native zoom was lost, which
led to inconsistencies in fonts depending on the auto scale mode. This
commit fowards the native zoom in this case to enable the Image to
initialize the GC accordingly.

fixes eclipse-platform#2385
fixes eclipse-platform#2311
@akoch-yatta akoch-yatta force-pushed the win32-fix-concatenated-GC-zooms-with-images branch from d77cbb7 to b0b70ab Compare August 12, 2025 09:22
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 have tested the proposed change with several different setups including combinations of

  • two monitors (175% and 100%), each of them once used as primary
  • monitor-specific scaling enabled/disabled
  • swt.autoScale set to a fixed value (150) vs. not custom specified

and particularly with StyledText (incl. LineNumberRuler) and GEF diagrams.

I did not find any regressions with respect to existing behavior (and with respect to 2025-06) but found the two mentioned issues to be successfully resolved:

@HeikoKlare
Copy link
Contributor

ftr: this also contributes to eclipse-platform/eclipse.platform.ui#3061

@akoch-yatta akoch-yatta merged commit d4c3036 into eclipse-platform:master Aug 12, 2025
21 of 22 checks passed
@akoch-yatta akoch-yatta deleted the win32-fix-concatenated-GC-zooms-with-images branch August 12, 2025 09:59
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.

Font size of Line number ruler is different when using swt.autoScale=int200
4 participants