Rescaling of widgets on DPI zoom changes for basic controls (win32)

[akoch-yatta]

Hello everyone,

DISCLAIMER: We are currently testing it in more complex application to identify blockers, that could result in changes in the PR. BUT, except from the ongoing testing the feature set in this PR includes everything we target for this first PR.
We used the branch nmodi/DynamicDPI_4909 as source of ideas and a starting point for our approach.

We are working on improving the HiDPI behaviour in SWT in the implementation for Windows for quite some time now together with @HeikoKlare. We try to split the adaptions up into smaller contributions to make the PRs easier to review and to discuss. This is the smallest set we managed to bundle having a usuable state of SWT for a subset of widgets.
Due to this, the PR is not yet suitable to provide a proper scaling for the Eclipse IDE as a whole - some controls will not be scaled correctly or you will experience side effects.

How to test
As said above, the PR is just the first step, so testing it with the Eclipse IDE is not really a good idea (except to rule out side effect when not activating the feature. Best way to play around with the PR is to use the ControlExample with the VM arguments: -Dswt.autoScale.updateOnRuntime=true -Dswt.autoScale=quarter. Still expect Glitches in the UI like cut off texts after DPI changes.

In the following I try to give a structured insight in our approach:

Limitations: (that will be covered separately in other PRs)

• With this PR there is still a limitation for one zoom level per application (DPIUtil.deviceZoom, see [Multi-Monitor] SWT applications to support and work with different zoom values per Shell. #131)
• Widgets in the common-package (e.g. CTabFolder) are not yet contained in this PR (except Item)
• There are multiple static attributes or attributes that e.g. are initialized on startup with the primary monitor zoom level and are currently not updated

Opt-in feature configuration:
The new behaviour is implemented as opt-in feature activated with the swt.autoScale.updateOnRuntime flag. The logic how the DPI level is calculated is unchanged. In Windows using it with -Dswt.autoScale=quarter could make sense, because the recent Windows versions usually propose scaling in 25% steps.

DPI zoom update propagation:
The whole propagation is integrated into the event handling triggered by Control::WM_DPICHANGED. The event triggers a listener in Shell::handleZoomEvent that propagates it into a custom compoanont with a lifecycle managed by DPIZoomChangeRegistry. We evaluated multiple ideas for that, like

• Adding a method directly inside Widget, that takes care of neccessary updates. This method would be needed to be overriden by sublassed to add additional update logic. Two major drawback for that were:
  • Exposing an additional method into the API of Widget (it must be public to be accessible from all neccessary loocations)
  • Unclear how to integrate this (Windows specific) behaviour into the common widgets, like CTabFolder
• Propagating the SWT event for a zoom change through each widget in the hierarchy and bind the update behaviour via listener (similar to Shell::handleZoomEvent for the "main" event). Two major drawback for that were:
  • How to register the listener? Doing it in the constructor leads to problems to distinguish if a listener must be added or not. If a Composite would be instantiated directly, you would need to register it, but not, when a subclass of it was instantiated. We didn't came a with a reliable solution to cover all issues regarding this approach.
  • How to structure the callbacks? One implementation per class that is individually called or having the listeners match the class hierarchy of the widget to delegate the event directly in the listener. Both approaches had not solvable drawback either caused by similar issues like in point 1 or by having unsorted handling of events.

In the end we decided to implement an independent workflow to have full control over the order how the updates are handled. The execution order is starting with the most general class in the class hierarchy, e.g. if a Composite is updated, the handlers are executed like (Widget -> Control -> Scrollable -> Composite). This way each class can provide a handler that only takes care for the attributes/method it adds, but the order is reliable and consistent.

Fonts:
With fonts there are two main challenges: How to scale fonts and how to make sure fonts are used in the correct scale?
Scaling fonts
The main issue arises from the conversion of pixels to points of a font. The conversion is dependent on the primary monitor zoom (see Device::computePoints(LOGFONT logFont, long hFont)). Therefor we need additional informartion to adapt this conversion to the correct target zoom. We achieve this by adding a package protected attribute zoomLevel to fonts, that is used to correct this conversion. According to our testing we see this approach works reliable.
Identify the correct scaled variant of a font
As a font (instance) can and is usually reused for many controls, we must assume these controls will be rendered on monitors with different zoom levels. Each font (instance) stores the handle that links to the font resource in the OS. In case a control is affected by a zoom level change, this change must be applied to the assigned font as well -> Solution: update the font to a scaled variant and pass it to the control. This would be sufficient, if SWT would have the full control of font creation. But fonts are almost always created (and commonly cached) outside of SWT and we cannot assume, that a font is scaled to the correct size when passed to a control. If we would not consider this as well, e.g. a scaled font can be replaced by a non-scaled variant at any time. To cover both scenarios we adapt fonts in two places:

• Zoom level change of the Control will update the font (if != null) as well
• Update the font (usually via setFont) must check for the correct zoom level and replace it by a proper one, if necessary.

Both scenarios make use of (1) Font:scaleFor(zoomLevel) to derive scaled font variants from each font and (2) caching them in ScalingFontRegistry to be able to reuse scaled variants of a font that is used for multiple controls.

Note: In default or if swt.autoScale.updateOnRuntime flag is set to false DefaultSWTFontRegistry is used instead of ScalingFontRegistry

Images:
There was already automatic scaling in Image. This was adapted to cover additional issues we faced when testing. To reuse an image for multiple zoom levels at the same time, there will be additional adaptions necessary, according to our testing so far.

Any feedback is highly appreciated. We hope we can provide a proper starting point for improving the HiDPI behaviour in Windows and are planning to continue with this topic in the following week.

[github-actions]

Test Results

   298 files   -     1    298 suites   - 1   1h 7m 59s ⏱️ + 1h 1m 46s
 4 111 tests +   10    171 ✅  - 3 922   6 💤  -  2  0 ❌ ±0  3 934 🔥 +3 934 
15 963 runs  +3 748  8 230 ✅  - 3 910  61 💤  - 14  0 ❌ ±0  7 672 🔥 +7 672 

For more details on these errors, see this check.

Results for commit 6e42b69. ± Comparison against base commit 979d3f1.

♻️ This comment has been updated with latest results.

[HeikoKlare]

I have taken an initial look at the changes. I have not found the time for an in-depth review yet, but since it's most important to agree on the general architecture of providing multi-monitor HIDPI support on Windows, that may be fine for now.

In addition to the fine-grained comments I have made, I have some general remarks:

• The propagation of DPI change events in any Control instance sets the static currentDeviceZoom of DPIUtil as well as the containing shell's native zoom level. What is the contract regarding these two values? I am a little confused about what I can expect from the shell's native zoom and from DPIUtil's current device zoom whenever I retrieve it. My understanding is: as soon as the DPI of any control changes, this value is updated. Is it guaranteed that it does not conflict with the pending DPI updates of all other controls?
• I found some changes that may fix some issues independent from the goals of this PR or may even be completely unrelated. It would be great if we can extract every plain refactoring or fix of independent issues from this PR, as it will make reviewing easier. Currently, it is quite hard to see which changes are safe (do only apply when activating the newly introduced flag) and which have to be considered very thoroughly as they may affect existing functionality. I know it is a trade-off whether refactoring should only be merged with the new functionality that requires them or independent from it, but in this case I feel a little lost in reviewing everything together and feeling confident w.r.t. not producing regressions by merging the PR.

[HeikoKlare]

I want to emphasize that this PR is supposed to be an enabler for providing a complete multi-monitor HiDPI support for Windows. It aims to touch as few existing functionality as possible and to put most additional logic behind the new swt.autoScale.updateOnRuntime flag, to reduce the risk of regressions. This will allow to address smaller issues and features in dedicated, subsequent PRs.
However, the earlier we can merge this PR, the easier and faster the work on these subsequent issues will be. Still, this PR manifests the essential architectural additions, which is why feedback and agreement on them is rather important. Thus, I would like to encourage every interested person to give feedback to the general architecture for the HiDPI support as proposed by @akoch-yatta, in particular regarding the way the HiDPI update logic is provided and regarding the necessary addition of platform-specific API.

[akoch-yatta]

In addition to the fine-grained comments I have made, I have some general remarks:

• The propagation of DPI change events in any Control instance sets the static currentDeviceZoom of DPIUtil as well as the containing shell's native zoom level. What is the contract regarding these two values? I am a little confused about what I can expect from the shell's native zoom and from DPIUtil's current device zoom whenever I retrieve it. My understanding is: as soon as the DPI of any control changes, this value is updated. Is it guaranteed that it does not conflict with the pending DPI updates of all other controls?

Storing this native zoom per Shell is only required for the fonts to be scaled correctly to "real" monitor zoom levels instead of the artificial one that is stored as deviceZoom in SWT.
As far as I know and understood the behaviour of the Windows API for the message that is handled here, it will be sent per top-window only. That should always be the Shell here. I could overwrite the WM_DPICHANGED method in Shell to have all update logic contained there instead of in Control. Although I cannot think of a real scenarios, where a conflicting update could appear.

• I found some changes that may fix some issues independent from the goals of this PR or may even be completely unrelated. It would be great if we can extract every plain refactoring or fix of independent issues from this PR, as it will make reviewing easier. Currently, it is quite hard to see which changes are safe (do only apply when activating the newly introduced flag) and which have to be considered very thoroughly as they may affect existing functionality. I know it is a trade-off whether refactoring should only be merged with the new functionality that requires them or independent from it, but in this case I feel a little lost in reviewing everything together and feeling confident w.r.t. not producing regressions by merging the PR.

I removed those you mentioned. There are still some adaptions included besides the basic update behaviour. Handling of fonts, calls regarding image bounds, replace theming OS calls with DPI aware alternative ot the replacement of icons of text to dpi aware alternative. Taking out the theming changes would be easy, at is one separate commit anyway, but would of course lead to weird glitches when theming is used. Taking out the other parts could be more tricky and I am unsure about the effect or the effort required for that.

[HeikoKlare]

In general, the changes look sound to me. Even though I still have a bunch of comments, they should be rather local. This review is almost complete, but currently excludes Font, Image and GC, which I will have a look at separately.

[HeikoKlare]

I have now also taken a look at Font, Image and GC and in general their changes also look sound to me.

In addition to my detailed comments, I have a general comment on tests: since this PR introduces new functionality, which is in parts also publicly available, I would expect the contracts of the new API to be tested. I would consider the following kinds of added tests relevant:

• Tests for the SWTFontRegistry implementations
• Tests for new public methods, such as Font.scaleFor(), Image.handleDPIChange() etc.
• At least rudimentary tests for rescaling widgets. E.g., for every kind of widget create a shell with an instance of that widget, rescale it, and check that important properties of that widget are changed (such as bounds, font size etc.). With "rudimentary" I mean that I would see priority in the tests to run through the complete rescaling logic to find potential errors (throwing exception etc.) without requiring anyone to run some SWT application with the activated updateOnRuntime option and having monitors with different scalings to find potential severe errors. The assertion of correct functionality would be of lower priority for me, as an incorrectly scaled font would be less severe than complete crashes of the rescaling logic leaving an application in an inconsistent scaling state.

[akoch-yatta]

I have now also taken a look at Font, Image and GC and in general their changes also look sound to me.

In addition to my detailed comments, I have a general comment on tests: since this PR introduces new functionality, which is in parts also publicly available, I would expect the contracts of the new API to be tested. I would consider the following kinds of added tests relevant:

• Tests for the SWTFontRegistry implementations
• Tests for new public methods, such as Font.scaleFor(), Image.handleDPIChange() etc.
• At least rudimentary tests for rescaling widgets. E.g., for every kind of widget create a shell with an instance of that widget, rescale it, and check that important properties of that widget are changed (such as bounds, font size etc.). With "rudimentary" I mean that I would see priority in the tests to run through the complete rescaling logic to find potential errors (throwing exception etc.) without requiring anyone to run some SWT application with the activated updateOnRuntime option and having monitors with different scalings to find potential severe errors. The assertion of correct functionality would be of lower priority for me, as an incorrectly scaled font would be less severe than complete crashes of the rescaling logic leaving an application in an inconsistent scaling state.

Definitely valid points. we will have a look at separate tests for all mentioned components. Still, I want to raise the point, whether we want to wait for those tests with this PR or contribute them with a separate PR afterwards. As a lot more contributions are waiting for this PR, I would like to avoid to many changes to pile up locally.

[HeikoKlare]

Still, I want to raise the point, whether we want to wait for those tests with this PR or contribute them with a separate PR afterwards.

With respect to the tests for widget rescaling, it would be okay for me to have them in a follow-up PR. For the existing API extensions, however, I would be in favor of having tests now, as we do not know when that functionality may be affected by other changes and then we want to find potential regressions immediately and not only once tests are added.

[fedejeanne]

@fedejeanne From all previous replies on the topic - you seem to be the most qualified and active SWT on windows contirbutor so it's up to you when/what/how to merge.

Then we are in deep trouble, @akurtakov 😅 (see the last sentence here)

[HeikoKlare]

To clarify what we have done so far to ensure that this is valuable but also "safe" contribution (i.e., not producing any regressions) even with few to no experienced contributors available for deeper review:

• Stripped down this initial contribution to just set the basis for providing holistic multi-monitor HiDPI support by follow-up contributions, so that as few existing code is affected as possible by this initial PR.
• Placed most of the added code behind a release feature toggle to not be executed at all unless the added ini flag (serving as the feature toggle) is activated.
• Thoroughly reviewed in particular those changes affecting existing code (and being executed even with the feature toggle turned off).
• Manually tested this contribution together with the contributions to be made as follow-ups in an SDK product and also in our RCP-based product.

Our query for reviews particularly aimed at giving everyone enough chance to comment on the overall architecture of how multi-monitor HiDPI support will be provided for Windows. If everyone is fine with trusting us (in particular @akoch-yatta) in doing our attempt to provide multi-monitor HiDPI support properly and thoroughly, that's of course fine for us as well.

[HeikoKlare]

Thanks for the recent changes, @akoch-yatta! I have taken a final look at them and only have minor comments that should be easy and fast to answer and/or address. All the changes look still as sound to me as in the previous review. My review focus was on finding potential regressions in existing code. Added functionality is kept internal as far as possible, so even if follow-up PRs require adaptations, they should be easy to realize as we do not fix too much public API with this PR.

I have tested the changes manually, also with a focus on finding regressions in existing functionality. I did not face any regressions yet, neither in a "default" setup with 100% monitor scaling, nor with other monitor scalings, both with -Dswt.autoScale=quarter and also with the flag unset (i.e., with -Dswt.autoScale=integer200). In particular, I have compared the l look (in particular the sizes and sharpness) of UI elements with and without this PR and they look exactly the same.

So, as soon as my final remarks are processed, there is a go from my side for merging this PR.

[HeikoKlare]

Thank you for the latest updates, @akoch-yatta! From my side, everything is fine now, and we also have the approval of @fedejeanne. So I will merge this now to have the changes in 2024-06 M2.

The failing MacOS build is unrelated:

• It fails in the same way for other PRs: https://github.com/eclipse-platform/eclipse.platform.swt/actions/runs/8799682907
• This PR only affects Win32 code, except for DPIUtil in which only extensions have been made. In addition, the extension were already present in previous builds of this PR, in which MacOS verification still worked and succeeded, e.g., https://github.com/eclipse-platform/eclipse.platform.swt/actions/runs/8750612935

[HeikoKlare]

Just saw that the aarch64 Windows build fails on Jenkins. @akoch-yatta do you know whether that may be related to the changes in the native code you made or is it a problem with the pipeline?

[akoch-yatta]

@HeikoKlare It is just running into a timeout, isn't it? I see the same issue on builds in the other PRs.

[HeikoKlare]

Yes, it runs into a timeout in the step building natives: https://ci.eclipse.org/releng/job/eclipse.platform.swt/view/change-requests/job/PR-1064/58/pipeline-console/?selected-node=333

Can you refer to any other PR with the same problem? I did not find any other PR compiling the binaries, as they do not perform any changes to native code.

[akoch-yatta]

E.g. in for PR 1188, see https://ci.eclipse.org/releng/job/eclipse.platform.swt/job/PR-1188/1/pipeline-graph/

Isn't just the node for the aarch64 build offline?

[HeikoKlare]

You are right, it is probably because of the Jenkins node being offline.

I am concerned that if we still merged, the Jenkins builds on master will not succeed and may not deploy the new binaries to the repo, such that the Java code becomes incompatible with the provided binaries.
@akurtakov @HannesWell maybe one of you knows if that assumption correct (i.e., whether we should not merge this as long as the Windows ARM binaries compilation does not work) and/or whether we can restart the Jenkins node (https://ci.eclipse.org/releng/computer/rie8t%2Dwin11%2Darm64/)?

[iloveeclipse]

whether we can restart the Jenkins node (https://ci.eclipse.org/releng/computer/rie8t%2Dwin11%2Darm64/)?

Create ticket here: https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues

[HannesWell]

I am concerned that if we still merged, the Jenkins builds on master will not succeed and may not deploy the new binaries to the repo, such that the Java code becomes incompatible with the provided binaries.

That's all correct and therefore should be avoided.

[HeikoKlare]

Thanks for your quick replies, @iloveeclipse @HannesWell! That's what I've expected.
I've created a ticket for the issue: https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/4587

[HeikoKlare]

Thank you for the latest updates, @akoch-yatta! From my side, everything is fine now, and we also have the approval of @fedejeanne. So I will merge this now to have the changes in 2024-06 M2.

The failing MacOS build is unrelated:

• It fails in the same way for other PRs: https://github.com/eclipse-platform/eclipse.platform.swt/actions/runs/8799682907
• This PR only affects Win32 code, except for DPIUtil in which only extensions have been made. In addition, the extension were already present in previous builds of this PR, in which MacOS verification still worked and succeeded, e.g., https://github.com/eclipse-platform/eclipse.platform.swt/actions/runs/8750612935

The Jenkins build has been fixed. GH Actions builds for MacOS and Linux fail (for different reasons), but both related to infrastructure issues and unrelated to the changes in this PR (see explanation above for MacOS, now there is also a signing issue on Linux).

[Phillipus]

Are these changes entirely opt-in by specifying VM arguments? I looked at some of the code and I see fundamental changes that don't look optional. Can we expect possible regressions in Eclipse 4.32?

[HeikoKlare]

Most of the changes are optional (to be activated via flag), but some changes had to be made in existing code. It is unavoidable to adapt existing code in order to provide multi-monitor HiDPI support for Windows, but we have, of course, thoroughly reviewed in particular those changes that are not opt-in to avoid regressions. If you still find regressions, please report them.