Refactor fillGradientRectangle to avoid DPIUtil.autoScaleDown()

[arunjose696]

Summary:

This PR refactors the image creation logic in fillGradientRectangle() by using an ImageDataProvider instead of directly creating ImageData. There is no visual impact for the changes the behavior should remain as before.

Steps to reproduce

Reaching this function on Windows is difficult, as the check if (OS.GetROP2(handle) != OS.R2_XORPEN && OS.GetDeviceCaps(handle, OS.TECHNOLOGY) != OS.DT_RASPRINTER) usually returns true, causing the method to be bypassed. For testing purposes, this condition can be manually set to false to ensure that fillGradientRectangle is executed.

When the check is disabled, the method is invoked, for example, when using palettes in GEF. After applying this change, the gradient rectangles in the palette render identical as before.

[github-actions]

Test Results

   546 files  ±0     546 suites  ±0   27m 8s ⏱️ - 2m 2s
 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 77cf2b3. ± Comparison against base commit 15c0a34.

♻️ This comment has been updated with latest results.

[ShahzaibIbrahim]

The changes made here are similar to this PR about refactoring the DPIUtil. We might be working on the same thing, could you check and confirm?

[arunjose696]

The changes made here are similar to this PR about refactoring the DPIUtil. We might be working on the same thing, could you check and confirm?

Yes, the changes are mostly similar. The only additional thing I'm doing is replacing Image constructors that take ImageData with ones that use an ImageDataProvider. I think this change can wait until #2281 is merged

[ShahzaibIbrahim]

@arunjose696 #2281 is merged. You can resolve the conflicts and mark this one as ready for review.

[arunjose696]

@arunjose696 #2281 is merged. You can resolve the conflicts and mark this one as ready for review.

Have resolved conflicts

[amartya4256]

@arunjose696 Please adapt the description of the PR in comparison to the new version of the base branch. It's not relevant anymore.

[arunjose696]

@arunjose696 Please adapt the description of the PR in comparison to the new version of the base branch. It's not relevant anymore.

done

[amartya4256]

@arunjose696 Can you provide a snippet with which I could test it? I didn't understand which condition you meant to set false. Is it if (data.gdipGraphics != 0)?

[arunjose696]

@arunjose696 Can you provide a snippet with which I could test it? I didn't understand which condition you meant to set false. Is it if (data.gdipGraphics != 0)?

What I meant is that, by replacing the original condition:
if (OS.GetROP2(handle) != OS.R2_XORPEN && OS.GetDeviceCaps(handle, OS.TECHNOLOGY) != OS.DT_RASPRINTER) {
with:
if (false && OS.GetROP2(handle) != OS.R2_XORPEN && OS.GetDeviceCaps(handle, OS.TECHNOLOGY) != OS.DT_RASPRINTER) {
you can force it to run through the changed code.

I had originally linked to a specific line in the file, but that became outdated after the file changed. I wasn't able to provide a proper snippet because I couldn’t determine under what conditions the original check would actually evaluate to false.

[fedejeanne]

It's not clear to me how to test this change and why is it necessary. Since you mentioned that "There is no visual impact for the changes the behavior should remain as before" in the description, I assume it is about performance?

I don't have GEF to test this change. Is there any other way to test it? Some screenshots/video to showcase the use case would be helpful too.

[HeikoKlare]

It's not clear to me how to test this change and why is it necessary. Since you mentioned that "There is no visual impact for the changes the behavior should remain as before" in the description, I assume it is about performance?

The reason is simplier than one might think: the Image constructor just taking an ImageData object is not that reasonable in multi-zoom scenarios, which is why we would like to deprecate it (maybe not for removal, but just to inform that it's usually not a good choice). As a preparation, we wanted to adapt all consumers that we have access to, such that they use a dynamic constructor based on an ImageDataProvider.

I tried to test the change, but unfortunately I was not able to trigger the affected code. The only caller seems to be GC#fillGradientRectangleInPixels(). I have tested several (transitive) callers of that (such as several SWT snippets), but even though they all trigger the GC method, none of them go to the ImageData call. When taking a look at the GC#fillGradientRectangleInPixels() implementation, it even looks as if that call is just some fallback for printer devices. Given that all this makes it more difficult to test, maybe we should just keep the code as is (and in case we deprecate the Image constructor, we may accept/suppress the warning that it will introduce, as in this case the usage of that constructor might actually be okay).

[arunjose696]

I tried to test the change, but unfortunately I was not able to trigger the affected code. The only caller seems to be GC#fillGradientRectangleInPixels().

Reaching this function on Windows is difficult , as the check if (OS.GetROP2(handle) != OS.R2_XORPEN && OS.GetDeviceCaps(handle, OS.TECHNOLOGY) != OS.DT_RASPRINTER) in GC#fillGradientRectangleInPixels() usually returns true, causing the method to be bypassed. For testing purposes, this condition can be manually set to false to ensure that fillGradientRectangle is executed.

[HeikoKlare]

I somehow missed that you already described that in the original post. Sorry for that.

I have just tested with the according code block disabled and then this PR even seems to introduce a quality degradation. Using GEF on a 175% screen, sometimes white lines show up in the gradients (at least when hovering over the items):

[arunjose696]

I have just tested with the according code block disabled and then this PR even seems to introduce a quality degradation.

I’ve tested this after rebasing the branch on the current master. When comparing GEF at 175% zoom, I observed that the quality appears consistent with master.

I've attached screenshots showing the result with and without the change for reference.

I did notice the white line before the rebase. However, I assume that was due to similar behavior already present in the earlier state of master .

[arunjose696]

I have added an additional commit that moves the Win32DPIUtils.pointToPixel(size, zoom) method to DPIUtils, so the existing logic in pointToPixels can be reused instead of manually computing the scaling factor. Since DPIUtils already contains pixelToPoint(size, zoom) methods, it makes sense to place the complementary pointToPixel(size, zoom) method there as well.