Fix image handle creation to use correct native zoom

[arunjose696]

In some cases where an image handle is passed to drawImage, the handle was created with nativeZoom set to Gc::getZoom(). This caused incorrect font sizes when drawing text with an imageGCDrawer.
This change ensures that the image handle is created with the correct nativeZoom value from the GC, fixing the font size issue.

Steps to reproduce

Open a runtime workspace at 175% primary monitor zoom with monitor-specific scaling disabled. The line number ruler will have text cut off without this change

Before	After

[HeikoKlare]

The change looks totally reasonable. But I wonder which change introduced the broken behavior. @arunjose696 did you trace back the issue to the causing commit?

When taking a look at the history, I also found this change: https://github.com/eclipse-platform/eclipse.platform.swt/pull/2570/files
There we introduced that ImageGcDrawerWrapper#getImageData() delegates to ImageGcDrawerWrapper#loadImageData() only considering the targetZoom but not the nativeZoom passed to the method. It does not seem to directly be related to this issue, was it's similar in the sense that native zoom seems to not properly be considered.

[arunjose696]

The change looks totally reasonable. But I wonder which change introduced the broken behavior. @arunjose696 did you trace back the issue to the causing commit?

The regression was introduced in eclipse-platform/eclipse.platform.swt#2526.
In that pull request, the drawImage method was modified to receive the image handle instead of the zoom.
Before this change, the image handle for the non-GDIP case (data.gdipGraphics == 0) was created with the native zoom.
This patch restores that behavior.

before that PR, in the data.gdipGraphics != 0 case, the image handle was created without the native zoom. However, upon reviewing the logic, it still makes sense to create the image handle with the correct native zoom for both cases.

[github-actions]

Test Results

  118 files  ±0    118 suites  ±0   16m 28s ⏱️ +45s
4 652 tests ±0  4 635 ✅ ±0  17 💤 ±0  0 ❌ ±0 
  338 runs  ±0    334 ✅ ±0   4 💤 ±0  0 ❌ ±0 

Results for commit 9b0df54. ± Comparison against base commit 65ec239.

♻️ This comment has been updated with latest results.

[arunjose696]

When taking a look at the history, I also found this change: https://github.com/eclipse-platform/eclipse.platform.swt/pull/2570/files There we introduced that ImageGcDrawerWrapper#getImageData() delegates to ImageGcDrawerWrapper#loadImageData() only considering the targetZoom but not the nativeZoom passed to the method. It does not seem to directly be related to this issue, was it's similar in the sense that native zoom seems to not properly be considered.

I think you meant ImageGcDrawerWrapper#newImageData() instead of ImageGcDrawerWrapper#getImageData().
The reasoning for only considering targetZoom when delegating from newImageData() to loadImageData() was based on the understanding that all callers of newImageData() invoked it using a zoom context derived from a single zoom making targetZoom and nativeZoom the same.

As a cleanup, we could simplify this by changing the signature of newImageData() to accept just a single zoom instead of a zoom context.

[HeikoKlare]

The regression was introduced in eclipse-platform/eclipse.platform.swt#2526. In that pull request, the drawImage method was modified to receive the image handle instead of the zoom. Before this change, the image handle for the non-GDIP case (data.gdipGraphics == 0) was created with the native zoom. This patch restores that behavior.

before that PR, in the data.gdipGraphics != 0 case, the image handle was created without the native zoom. However, upon reviewing the logic, it still makes sense to create the image handle with the correct native zoom for both cases.

Thank you for the detailed explanation. I took a look at the causing change and with that the proposed change totally makes sense. I propose that we test this change with the autoscale disablement mechanism as the different zooms of GC are highly relevant there (i.e., best test with GEF), just for the sake of being sure that we do not break anything there.

I think you meant ImageGcDrawerWrapper#newImageData() instead of ImageGcDrawerWrapper#getImageData(). The reasoning for only considering targetZoom when delegating from newImageData() to loadImageData() was based on the understanding that all callers of newImageData() invoked it using a zoom context derived from a single zoom making targetZoom and nativeZoom the same.

As a cleanup, we could simplify this by changing the signature of newImageData() to accept just a single zoom instead of a zoom context.

Yes, I meant the newImageData() method, sorry for the confusion. I see that all callers pass a ZoomContext with just a single zoom, which makes it valid to consider any of the zooms in that context object. Sound to me as if it would make sense to clean that up. @akoch-yatta do you remember if we had something in mind for which we introduced the ZoomContext as parameter for that method or was it maybe only relevant at some past point in time and just became obsolete after further adaptations that we made?

[HeikoKlare]

I also tested the change now and it properly fixes the issue and I did not find anything that breaks. As already mentioned before, the change is sound anyway.

With respect to autoscale-disabled use cases, I also did not find any issues either and since every involved zoom value should be 100 then anyway, it should not lead to any behavioral changes there at all.

@akoch-yatta would like to have your opinion and whether you see any risk that we may have missed before merging

[akoch-yatta]

I gave it a short test and it looks good to me as well. Conceptually it makes sense anyway