Skip to content

Scale cursor width by monitor specific zoom #2471

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 9, 2025
Merged

Scale cursor width by monitor specific zoom #2471

merged 1 commit into from
Sep 9, 2025
+33 −28

Conversation

amartya4256
Copy link
Contributor

@amartya4256 amartya4256 commented Sep 1, 2025 •
edited
Loading

This PR fixes a bug where the cursor width was not scaled according to monitor DPI settings. Previously, the cursor width was applied as a fixed pixel value retrieved from OS.SystemParametersInfo which returns the caret width for the primary zoom at startup, resulting in inconsistent appearance across monitors.

Fix

The cursor width now scales correctly with the monitor’s DPI setting, ensuring consistent visual size across different display scales.

@amartya4256 amartya4256 linked an issue Sep 1, 2025 that may be closed by this pull request
Use SystemParametersInfoForDpi in Caret vi-eclipse/Eclipse-Platform#433
Closed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 1, 2025 •
edited
Loading

Test Results

   546 files  ±0     546 suites  ±0   31m 32s ⏱️ -45s
 4 431 tests ±0   4 414 ✅ ±0   17 💤 ±0  0 ❌ ±0 
16 764 runs  ±0  16 637 ✅ ±0  127 💤 ±0  0 ❌ ±0 

Results for commit 6d39102. ± Comparison against base commit f858239.

♻️ This comment has been updated with latest results.

@amartya4256 amartya4256 force-pushed the amartya4256/use_new_api_in_caret branch from e13d50f to 38cac4f Compare September 2, 2025 07:57
@amartya4256 amartya4256 mentioned this pull request Sep 2, 2025
Open
HeikoKlare
HeikoKlare requested changes Sep 3, 2025
View reviewed changes
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change currently introduces a regression:
I tested the change and printed out the old and new values for comparison. Turns out that the new method call always writes 0 to the passed buffer and, in particular, the method call always returns false, such that the size is not taken into account.
One example consequence is that if I set a high OS cursor size (like 10), the cursor is still 1 pixel wide. For example, in a console window it used to look like this (with OS cursor size 10):
image
And now it looks like this:
image

Note that the documentation of the used method states that it cannot be called with SPI_GETCARETWIDTH as done here but will always return 0 in that case: https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-systemparametersinfofordpi#remarks

@amartya4256 amartya4256 force-pushed the amartya4256/use_new_api_in_caret branch from 38cac4f to ac7d9d5 Compare September 3, 2025 09:47
@amartya4256
Copy link
Contributor Author

@HeikoKlare Turns out SystemParametersInfo doesn't work with SPI_GETCARETWIDTH : https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-systemparametersinfofordpi
So, the other way of having DPI specific value is by scaling it ourselves. Hence, I have readapted this PR. Please have a look. #2470 has the final implementation with the refactored handler. Hence, this PR only acts as an intermediate state. The goal should be to make sure this doesn't cause any regression.

@amartya4256 amartya4256 changed the title Use SystemParametersInfoForDpi in Caret Scale width of Caret w.r.t. wthe native zoom Sep 3, 2025
@amartya4256 amartya4256 changed the title Scale width of Caret w.r.t. wthe native zoom Scale width of Caret w.r.t. the native zoom Sep 3, 2025
HeikoKlare
HeikoKlare reviewed Sep 3, 2025
View reviewed changes
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.

The goal should be to make sure this doesn't cause any regression.

If I understand correctly, there was actually a bug that this PR now resolves: before, the cursor width was not scaled according to the monitor. So, e.g., a Windows setting of cursor size 10 would have resulted in pixel-width 10 for the cursor on both a 100% and 150% monitor whereas now on the 150% monitor the width will be 15 pixels.
So would be good to have that properly documented in the commit and PR messages.

@amartya4256 amartya4256 force-pushed the amartya4256/use_new_api_in_caret branch 3 times, most recently from 3f54b85 to 3e7a9fb Compare September 8, 2025 11:00
@amartya4256 amartya4256 changed the title Scale width of Caret w.r.t. the native zoom Scale cursor width by monitor specific zoom Sep 8, 2025
@amartya4256 amartya4256 force-pushed the amartya4256/use_new_api_in_caret branch from 3e7a9fb to 83de15b Compare September 8, 2025 12:39
HeikoKlare
HeikoKlare reviewed Sep 9, 2025
View reviewed changes
@amartya4256 amartya4256 force-pushed the amartya4256/use_new_api_in_caret branch from 83de15b to bb94992 Compare September 9, 2025 10:02
HeikoKlare
HeikoKlare reviewed Sep 9, 2025
View reviewed changes
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Caret.java Outdated
if (OS.SystemParametersInfo (OS.SPI_GETCARETWIDTH, 0, buffer, 0)) {
return new Rectangle (getXInPixels(), getYInPixels(), buffer [0], getHeightInPixels());
int widthInPixels = getSystemCaretWidthInPixelsForCurrentMonitor();
if (widthInPixels != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this now a change in behavior? It also uses getWidthInPixels() in case the width value delivered by SystemParameterInfo is zero and not only if the return value of that method is zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we use -1 then? Since -1 cannot be a width. I thoughtof using null first but then we need to have type Integer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used OptionalInt instead.

@amartya4256 amartya4256 force-pushed the amartya4256/use_new_api_in_caret branch from bb94992 to fc727a9 Compare September 9, 2025 12:24
HeikoKlare
HeikoKlare approved these changes Sep 9, 2025
View reviewed changes
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.

Looks and works good. Thank you!

@amartya4256 @HeikoKlare
Scale cursor width by monitor specific zoom
6d39102 
This commit adapts the scaling of Cursor width with respect to the
monitor zoom since the OS call OS.SystemParametersInfo returns the caret
width for the primary zoom at startup.
@HeikoKlare HeikoKlare force-pushed the amartya4256/use_new_api_in_caret branch from fc727a9 to 6d39102 Compare September 9, 2025 13:03
@HeikoKlare HeikoKlare merged commit c6a29a9 into eclipse-platform:master Sep 9, 2025
17 checks passed
@HeikoKlare HeikoKlare deleted the amartya4256/use_new_api_in_caret branch September 9, 2025 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HeikoKlare HeikoKlare HeikoKlare approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use SystemParametersInfoForDpi in Caret
2 participants
@amartya4256 @HeikoKlare