[Win32] Apply accessibility setting to ImageDataProvider-based cursor

[HeikoKlare]

Cursors are scaled according to the accessibility settings of Windows. However, this setting is only applied to Cursor instances based on image data but not on those that are instantiated via an ImageDataProvider. This change also applies the scale factor in the latter case.

See:

• Custom cursors are scaled twice again eclipse-gef/gef-classic#872 (comment)

I consider this a safe change as it is simply an oversight in the existing implementation. The change has no effect in case the accessibility setting of Windows is not used. In case it is used, the quality of custom cursors based on an ImageDataProvider (as an example and in particuar for GEF) will have a better user experience.
This is why I would be in favor of still bringing this into the upcoming releae / RC2, even though we are already very late.

How to test

Change the accessibility settings for the mouse cursor size in the Windows settings and check with any custom cursor that uses an ImageDataProvider, such as the shared cursors of GEF.

Before

cursor_before.mp4

After

cursor_after.mp4

This is basically a follow up to the following PR which applied the accessibility setting to the image-data-based construction of cursors:

• SWT: Respect Windows accessibility cursor size setting #2493

[github-actions]

Test Results

  118 files  ±0    118 suites  ±0   16m 1s ⏱️ -14s
4 653 tests ±0  4 635 ✅  - 1  18 💤 +1  0 ❌ ±0 
  338 runs  ±0    334 ✅ ±0   4 💤 ±0  0 ❌ ±0 

Results for commit 6b11707. ± Comparison against base commit a0d6ffc.

This pull request skips 1 test.

org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser_IE ‑ test_setUrl_remote_with_post

♻️ This comment has been updated with latest results.

[ptziegler]

I think this is fine for the sake of consistency. There just seems to be one minor issue (which also happens with the other providers) that the image data is not updated when the scale factor is changed. Though I think that's best handled separately.

If I understand everything correctly, the cache stores the handle by monitor zoom level, even though the monitor zoom * scale factor would probably make more sense.

[HeikoKlare]

Failures are due to test_evalute_Cookies test in both the GHA and the Jenkins run. See:

• Test_org_eclipse_swt_browser_Browser.test_evalute_Cookies() failing on GTK x86_64 #2804

There just seems to be one minor issue (which also happens with the other providers) that the image data is not updated when the scale factor is changed. Though I think that's best handled separately.

If I understand everything correctly, the cache stores the handle by monitor zoom level, even though the monitor zoom * scale factor would probably make more sense.

That's indeed a separate issue. As you say, it would probably make sense to include the accessibility scale factor into the handle map key and it should work then.

[akoch-yatta]

Change is small and straightforward. I tested it with different cursors and accessibility settings and it worked as expected.

The change does not introduce a noticeable regression risk but provides a way better UX with accessibility settings

[HeikoKlare]

We propose to merge this for RC2. According to the comments and arguments, we consider the change of low risk while it will improve the user experience for use cases of custom cursors. As mentioned in #2811 (comment), the failing checks are due to the preexisting test_evalute_Cookies test fails.

@iloveeclipse @akurtakov @merks what do think from PL/PMC perspective? If you agree to bring this into the release, it would be great to have your PL/PMC+1.

[merks]

PMC+1

I trust that you folks have all the necessary expertise to make the best technical decisions for the good of the project.

[iloveeclipse]

I must confess the change looks non trivial for me, but I have no idea about Windows / SWT specific zoom handling.
So I abstain from voting.

[HeikoKlare]

I must confess the change looks non trivial for me, but I have no idea about Windows / SWT specific zoom handling.
So I abstain from voting.

Thank you for the feedback!
I understand that it's difficult to assess without knowing the code and the specifics around it (Windows, zoom handling etc.).
Even though we cannot complete resolve this, I would like to share some help for understanding what happens and why we consider it safe (though maybe not completely trivial):

• Cursors (like any resources) are created according to the zoom for which they are requested
• On Windows, there is an OS setting for scaling cursors (accessibility) in addition to the monitor zoom
• This accessibility factor is already applied for one way of creating a Cursor (via an ImageData object) while it was missing for the creation way that we adapt here (via an ImageDataProvider), which is why this is no completely new behavior but just extends it to all cases where it should have already been applied before. This is the existing case:

  eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Cursor.java

  Lines 654 to 658 in a0d6ffc

  	float accessibilityFactor = getPointerSizeScaleFactor();
  	int scaledZoom = (int) (zoom * accessibilityFactor);
  	ImageData scaledSource = DPIUtil.scaleImageData(device, this.source, scaledZoom, DEFAULT_ZOOM);
  	return setupCursorFromImageData(device, scaledSource, null, getHotpotXInPixels(zoom),
  	getHotpotYInPixels(zoom));

• The effect of this change is as follows:
  • The cursor size will be multiplied by the accessibility scaling factor
  • That scaling factor is 1 by default and will then have no effect at all
  • In case the value cannot be retrieved for whatever reason (it is queried from the Windows registry) it also defaults to 1
  • Only if the user has specified a non-default accessibility setting and if that value can be retrived, the behavior can change at all
• Also note that this way of creating a cursor (via ImageDataProvider) is not a long-standing behavior that could break long-standing consumers but was just introduced with the previous Eclipse/SWT release

[HeikoKlare]

I investigated the change again and found that the check against DEFAULT_ZOOM is a bit unclear to me. I adapted it with the latest commit to what I think it should be, but without properly knowing why it is necessary/useful, I am not in favor of proceeding with the change now anymore but would like to better understand that condition first.