Caret scaling implementation improvement on StyledText DPI change #2470

amartya4256 · 2025-09-01T10:48:34Z

This PR improves the way carets of StyledText is scaled on DPI change event by utilizing APIs instead of win32 specific static methods.

Earlier we used to provide a Consumer in CommonWidgetDPIChangeHandlers.handleStyledTextDPIChange to StyledText.updateAndRefreshCarets for performing scaling on DPI change.

To move forward with DPI change performance improvement, we need to get rid of CommonWidgetsDPIChangeHandlers. Which is only possible if we move these handlers to their respective classes and register these handlers on a ZoomChanged event. Which is the next step here: #2483

However, the consumer mentioned above uses a platform specific piece of code: Caret.win32_setHeight(caretToRefresh, styledText.getLineHeight()); Which cannot be moved to Common Widgets.

Hence, we need to improve the way we scale StyledText and its carets. So, this PR does the following changes:

The StyledText at first updates the carets' location and size.
Then the DPI change handlers in the carets in the styledText are called, where we call the caret.resize() which explicitly draws the caret with the newly scaled size.
Earlier caret.resize() was called using win32_setHeight. As using any other API like setSize, setBounds, etc. had a condition that if the size (in points) doesn't change, resize will not be called leading to no repaint. However, on a DPI change, the size changes in pixels and a repaint must be triggered. Hence, resize is not explicitly called (leading to repaint) in Caret.handleDPIChange which is triggered right after StyledText has adapted the size, location and visibility of the carets.

This change removes the need to use a platform specific (win32_) static method on DPI change and enables us to move forward with implementation of asynchronous DPI change.

contributes to #62 and #128

Depends on #2469 and #2471

github-actions · 2025-09-01T11:00:52Z

Test Results

546 files ±0 546 suites ±0 44m 21s ⏱️ + 2m 48s
4 431 tests ±0 4 409 ✅ - 1 17 💤 ±0 5 ❌ +1
16 764 runs ±0 16 632 ✅ - 1 127 💤 ±0 5 ❌ +1

For more details on these failures, see this check.

Results for commit dd4e9e3. ± Comparison against base commit 440c97a.

♻️ This comment has been updated with latest results.

HeikoKlare

Can you explain what changes when replacing the calls to SystemParametersInfo to SystemParametersInfoForDpi? I.e., what will be different both in terms of user experience and/or in terms of internal functionality when not doing the changes to the DPI change handling as well.

In addition, should be split this change up into replacing the existing calls and adapting the DPI change handling? I would also appreciate an explanation of why the method calls inside updateAndRefreshCarets() have to be moved to understand what happens there.

amartya4256 · 2025-09-02T10:55:05Z

Can you explain what changes when replacing the calls to SystemParametersInfo to SystemParametersInfoForDpi? I.e., what will be different both in terms of user experience and/or in terms of internal functionality when not doing the changes to the DPI change handling as well.

In addition, should be split this change up into replacing the existing calls and adapting the DPI change handling? I would also appreciate an explanation of why the method calls inside updateAndRefreshCarets() have to be moved to understand what happens there.

It is already split. It depends on #2471. The new API is used to retrieve the system's preferred width for the cursor. Earlier, it used the system DPI aware value, which would always return the value for the first monitor on which the App was started on, could lead to the cursors looking too think or too wide. This kind of behaviour is although not always visible as mostly, all the monitors have the same width. With Per-monitor DPI aware API, we make sure that the retrieved width is specific to the current monitor's preference and doesn't appear too think or too flat.

HeikoKlare

It is already split. It depends on #2471.

I see, thank you! Just commented on the other one. Putting a change request on this one as the one it depends on (and currently included in this) introduces a regression. Thus just wanting to avoid that it merged by accident.

ShahzaibIbrahim

This change looks great. The updated sequence ensures caret state is always correct before applying updates, improving reliability and maintainability. Approved from my side, but I see @HeikoKlare still has change request as he didn't wanted it to merge before #2471 which is already merged. Maybe we can merge this now?

HeikoKlare

I am still missing an explanation of what this movement of the method calls does to resolve an issue. The commit still refers to changes are not present anymore. So this is not ready to be merged.

amartya4256 · 2025-09-11T14:55:18Z

I am still missing an explanation of what this movement of the method calls does to resolve an issue. The commit still refers to changes are not present anymore. So this is not ready to be merged.

I have extended the description with the explanation :)

HeikoKlare

Thank you for the updated explanation. The execution order now makes more sense.
Still two remarks/requests:

Why do we still need the Caret#win32_setHeight method if its only caller is replaced?
Please update the commit message. It still has the old content. Please integrate a short explanation from the PR to the commit message, as that will be the primary source of information for most people (and it will even be there if GitHub and the PRs are not anymore). Please also remove the references to #62 and #128 as they have been closed.

amartya4256 · 2025-09-12T14:04:34Z

Why do we still need the Caret#win32_setHeight method if its only caller is replaced?

Yes, I'll remove it.

HeikoKlare

This change completely breaks Caret rescaling behavior for me, just for the simplest scenario:

Open a Java editor in a window on a 100$ screen
Move it to a 150% screen

The caret looses it's width and is reset to the position at the beginning of the document:

amartya4256 · 2025-09-15T11:25:11Z

@heiko, I cannot reproduce the behaviour. Can you provide some more details about how you started your runtime, What's your primary monitor zoom, etc?

HeikoKlare · 2025-09-15T11:39:33Z

Just found out that by accident I used a module-info file for testing (which uses a different editor) which shows the issue with this PR:

Setup is 100% primary monitor and 150% secondary monitor (to the left).

amartya4256 · 2025-09-15T12:07:35Z

Setup is 100% primary monitor and 150% secondary monitor (to the left).

Interesting. I still cannot reproduce it.

HeikoKlare · 2025-09-15T14:48:31Z

Interesting. I still cannot reproduce it.

With the Manifest-Info Editor?

amartya4256 · 2025-09-15T14:55:33Z

With the Manifest-Info Editor?

Yes, I started it in a module-info editor just as you described above in the image. Are you sure you have the latest changes from this branch maybe?

HeikoKlare · 2025-09-15T15:18:57Z

Yes, I started it in a module-info editor just as you described above in the image. Are you sure you have the latest changes from this branch maybe?

I have the latest state. And I started with a clean workspace again. I see that I sometimes need to move back to the 100% monitor to break the cursor, but after moving at least two times, it breaks. The position replacement happens when calling caret.resize() in the DPI change handler of Caret where getXInPixels(), getYInPixels() returns 3,0 (i.e., the beginning of the document) instead of the actual cursor position.

HeikoKlare

Thank you for the latest update. The change is fine now and also works without the regression I found in the previous version.

The test failures are unrelated and caused by the GHA update to Windows Server 2025. I have documented them here:

#2516

The Jenkins failure is probably related to:

eclipse-platform/eclipse.platform.releng.aggregator#3329

This commit replaces the win32-specific caret scaling with API-based resizing to ensure proper repaint on DPI changes. Carets now update location/size via resize() in their DPI change handlers, removing platform dependencies. Additionally, StyledTexts set location, size and visibility of their carets first before calling the DPI change handler of Caret to make sure when Caret is repainted, the size, location and visibility are properly set.

amartya4256 linked an issue Sep 1, 2025 that may be closed by this pull request

Adapt StyledText DPI change handling with better API usage vi-eclipse/Eclipse-Platform#432

Closed

amartya4256 changed the title ~~Caret scaling design improvement on StyledText DPI change~~ Caret scaling implemetation improvement on StyledText DPI change Sep 1, 2025

HeikoKlare mentioned this pull request Sep 1, 2025

Add SystemParametersInfoForDpi Win32 API #2469

Merged

amartya4256 force-pushed the amartya4256/adapt_styled_text_dpi_change branch from 6531eae to 396a9dc Compare September 2, 2025 08:01

HeikoKlare reviewed Sep 2, 2025

View reviewed changes

HeikoKlare requested changes Sep 3, 2025

View reviewed changes

amartya4256 force-pushed the amartya4256/adapt_styled_text_dpi_change branch from 396a9dc to 86de321 Compare September 3, 2025 09:47

amartya4256 mentioned this pull request Sep 3, 2025

Scale cursor width by monitor specific zoom #2471

Merged

amartya4256 force-pushed the amartya4256/adapt_styled_text_dpi_change branch 2 times, most recently from 77fa4e3 to 69128b8 Compare September 10, 2025 08:50

ShahzaibIbrahim approved these changes Sep 11, 2025

View reviewed changes

HeikoKlare reviewed Sep 11, 2025

View reviewed changes

fedejeanne force-pushed the amartya4256/adapt_styled_text_dpi_change branch from 69128b8 to cdd8b33 Compare September 12, 2025 08:57

fedejeanne requested a review from HeikoKlare September 12, 2025 08:58

HeikoKlare requested changes Sep 12, 2025

View reviewed changes

amartya4256 force-pushed the amartya4256/adapt_styled_text_dpi_change branch from cdd8b33 to 163364f Compare September 12, 2025 14:08

HeikoKlare force-pushed the amartya4256/adapt_styled_text_dpi_change branch from 163364f to 40463df Compare September 14, 2025 13:50

HeikoKlare requested changes Sep 14, 2025

View reviewed changes

amartya4256 force-pushed the amartya4256/adapt_styled_text_dpi_change branch from 40463df to ec3d589 Compare September 16, 2025 11:52

HeikoKlare mentioned this pull request Sep 16, 2025

Some Test_org_eclipse_swt_events_KeyEvent tests are failing with Windows Server 2025 on GHA #2516

Closed

HeikoKlare changed the title ~~Caret scaling implemetation improvement on StyledText DPI change~~ Caret scaling implementation improvement on StyledText DPI change Sep 16, 2025

HeikoKlare approved these changes Sep 16, 2025

View reviewed changes

HeikoKlare force-pushed the amartya4256/adapt_styled_text_dpi_change branch from ec3d589 to dd4e9e3 Compare September 17, 2025 10:00

HeikoKlare merged commit 75c97fd into eclipse-platform:master Sep 17, 2025
15 of 17 checks passed

HeikoKlare deleted the amartya4256/adapt_styled_text_dpi_change branch September 17, 2025 11:19

amartya4256 mentioned this pull request Sep 19, 2025

WIP #2465

Closed

Caret scaling implementation improvement on StyledText DPI change #2470

Caret scaling implementation improvement on StyledText DPI change #2470

Uh oh!

Conversation

amartya4256 commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

HeikoKlare left a comment

Choose a reason for hiding this comment

Uh oh!

amartya4256 commented Sep 2, 2025

Uh oh!

HeikoKlare left a comment

Choose a reason for hiding this comment

Uh oh!

ShahzaibIbrahim left a comment

Choose a reason for hiding this comment

Uh oh!

HeikoKlare left a comment

Choose a reason for hiding this comment

Uh oh!

amartya4256 commented Sep 11, 2025

Uh oh!

HeikoKlare left a comment

Choose a reason for hiding this comment

Uh oh!

amartya4256 commented Sep 12, 2025

Uh oh!

HeikoKlare left a comment

Choose a reason for hiding this comment

Uh oh!

amartya4256 commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HeikoKlare commented Sep 15, 2025

Uh oh!

amartya4256 commented Sep 15, 2025

Uh oh!

HeikoKlare commented Sep 15, 2025

Uh oh!

amartya4256 commented Sep 15, 2025

Uh oh!

HeikoKlare commented Sep 15, 2025

Uh oh!

HeikoKlare left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

amartya4256 commented Sep 1, 2025 •

edited

Loading

github-actions bot commented Sep 1, 2025 •

edited

Loading

amartya4256 commented Sep 15, 2025 •

edited

Loading