Skip to content

[win32] fallback for missing DPI change event #1863

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.

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

Merged

Conversation

akoch-yatta
Copy link
Contributor

@akoch-yatta akoch-yatta commented Feb 26, 2025

This commit adds a fallback mechanism if the OS doesn't send a DPI change as expected. When the process is started with System DPI awareness and only the thread is PerMonitorV2 aware, there are some scenarios, when the OS does not send a DPI change event when a child Shell is positioned and opened on another monitor as its parent Shell. To work around that limitation a check is added to Shell::WM_WINDOWPOSCHANGED to trigger a dpi change event if an unexpected DPI value is detected.

This PR needs #1862 to be merged before

Important for testing: You will not run into the handleMonitorSpecificDpiChange in Shell::WM_WINDOWPOSCHANGED when testing this PR on runtime. To me this scenario only appears, when starting the RCP from the Explorer (e.g. not a command prompt!)

Copy link
Contributor

github-actions bot commented Feb 26, 2025

Test Results

   506 files  ±0     506 suites  ±0   8m 47s ⏱️ +49s
 4 341 tests ±0   4 327 ✅ ±0   14 💤 ±0  0 ❌ ±0 
16 597 runs  ±0  16 488 ✅ ±0  109 💤 ±0  0 ❌ ±0 

Results for commit 7b29ac0. ± Comparison against base commit 31a54c4.

♻️ This comment has been updated with latest results.

@HeikoKlare
Copy link
Contributor

This PR needs #1862 to be merged before

So maybe integrate the commit of that PR into this PR to test everything together?

@akoch-yatta
Copy link
Contributor Author

This PR needs #1862 to be merged before

So maybe integrate the commit of that PR into this PR to test everything together?

I can do that. In the past we handled all OS related changes in separate PRs, therefor I did that here as well, but I can merge them.

@HeikoKlare
Copy link
Contributor

We can still keep the other PR to be merged independently.
But since this PR depends on it, it should be based on it (i.e., contain the according commit as well). Otherwise we cannot reasonably test this PR (both automatically and manually) until the other PR is merged and this one is rebased on top of it. Once the other PR is merged and we rebased this on top of master, that commit will simply be dropped.

@akoch-yatta akoch-yatta force-pushed the repair-missed-dpi-change branch from 0dc4317 to d8976da Compare February 26, 2025 15:18
@akoch-yatta
Copy link
Contributor Author

We can still keep the other PR to be merged independently. But since this PR depends on it, it should be based on it (i.e., contain the according commit as well). Otherwise we cannot reasonably test this PR (both automatically and manually) until the other PR is merged and this one is rebased on top of it. Once the other PR is merged and we rebased this on top of master, that commit will simply be dropped.

Adapted it accordingly

@HeikoKlare
Copy link
Contributor

Thank you. Now we see test failures that may be related to the change?

@akoch-yatta akoch-yatta force-pushed the repair-missed-dpi-change branch 2 times, most recently from 1c8baa6 to 07fa0f8 Compare February 27, 2025 09:40
@akoch-yatta
Copy link
Contributor Author

Thank you. Now we see test failures that may be related to the change?

Ah yes, sorry, I missed that. The fix triggers in the tests at some places and "fixes" some DPI values that were changed in the tests. I adapted the tests a bit to not use calls (e.g. Shell::open) that trigger these 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 proposed change completely breaks shell movement between screens for me. It becomes impossible to drag the shell to specific screens, both using drag-and-drop via mouse and also using system shortcuts like Windows+Left/Right. At some point, the shell is moved back to another screen.

This is how it behaves when trying to move between two secondary monitors, one at 100% (left) and one at 150% (right). When I try to move from the 100% to the 150% monitor, it jumps back. This is a video of the 100% screen:
shell_jump_back

@akoch-yatta
Copy link
Contributor Author

The proposed change completely breaks shell movement between screens for me. It becomes impossible to drag the shell to specific screens, both using drag-and-drop via mouse and also using system shortcuts like Windows+Left/Right. At some point, the shell is moved back to another screen.

This is how it behaves when trying to move between two secondary monitors, one at 100% (left) and one at 150% (right). When I try to move from the 100% to the 150% monitor, it jumps back. This is a video of the 100% screen: shell_jump_back shell_jump_back

Oh ok, I'll have a look.

@akoch-yatta akoch-yatta force-pushed the repair-missed-dpi-change branch 2 times, most recently from fbc29e4 to 3b58e06 Compare February 27, 2025 11:07
@akoch-yatta
Copy link
Contributor Author

Sorry for that @HeikoKlare I messed up when cleaning up the code: I mixed up x and y when setting the bounds.

@akoch-yatta
Copy link
Contributor Author

Just to be clear. You will not run into the handleMonitorSpecificDpiChange call in your testing. To me this scenario only appears, when starting the RCP from the Explorer (e.g. not a command prompt!)

@HeikoKlare
Copy link
Contributor

Just to be clear. You will not run into the handleMonitorSpecificDpiChange call in your testing. To me this scenario only appears, when starting the RCP from the Explorer (e.g. not a command prompt!)

Yes, so to test this we have to generate an SWT Jar for the modified state and drop it into an installed Eclipse instance, right?

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.

Thank you for the update!

I have tested the proposed change again:

  1. I could not find any kind of regression with respect to existing behavior anymore.
  2. The change properly fixes the issue when wrongly scaled shells, particularly on detaching onto another monitor.

To test (2), I have built the according DLLs and then built and packages the JARs with Tycho, to then place them into an installed Eclipse (modifying the SWT bundles in the simpleconfigurator). I started the installation once with the current SWT state and once with the replacement based on this change and found the shell scaling issue to be fixed.

This commit adds a fallback mechanism if the OS doesn't send a DPI change
as expected. When the process is started with System DPI awareness and
only the thread is PerMonitorV2 aware, there are some scenarios, when the
OS does not send a DPI change event when a child Shell is positioned and
opened on another monitor as its parent Shell. To work around that limitation
a check is added to Shell::WM_WINDOWPOSCHANGED to trigger a dpi change
event if an unexpected DPI value is detected.
@HeikoKlare HeikoKlare force-pushed the repair-missed-dpi-change branch from 549c3d5 to 7b29ac0 Compare March 8, 2025 11:43
@HeikoKlare HeikoKlare merged commit 2974864 into eclipse-platform:master Mar 8, 2025
14 checks passed
@HeikoKlare HeikoKlare deleted the repair-missed-dpi-change branch March 8, 2025 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants