Skip to content
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

Text in Dialog was cutoff due to invalid use of AdjustWindowRectEx #1707

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

ShahzaibIbrahim
Copy link
Contributor

AdjustWindowRectEx does not take DPI into account. So using AdjustWindowRectExForDpi and passing the zoom level to retrieve correct coordinates and scaling accordingly.

How to Reproduce

  • Run the Runtime Workspace with the following VM Arguments on 200% monitor
-Dswt.autoScale=quarter
-Dswt.autoScale.updateOnRuntime=true
  • Add a class and create a conditional breakpoint
  • Double click on Breakpoint to remove it
  • Maximize the main IDE window
  • The dialog will appear with the text cutoff
    image

Note

Replacing AdjustWindowRectEx in Decoration fixes the issue for dialogs but replacing all the non-dpi calls for AdjustWindowRectEx even though there is no visible difference (but slight difference in returned coordinates) right now but having it DPI dependent makes it consistent.

@ShahzaibIbrahim ShahzaibIbrahim linked an issue Jan 6, 2025 that may be closed by this pull request
Copy link
Contributor

github-actions bot commented Jan 6, 2025

Test Results

   494 files     494 suites   9m 3s ⏱️
 4 324 tests  4 306 ✅  18 💤 0 ❌
16 534 runs  16 386 ✅ 148 💤 0 ❌

Results for commit 9cf2c63.

♻️ This comment has been updated with latest results.

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.

One first comment on the new usage of the Windows API method: The method has been introduced for Windows 10 1607 (https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-adjustwindowrectexfordpi#requirements). This means:

  • We should guard its calls by a version check and use the old (non-DPI-aware) method if that method is not met (like, e.g., for getSystemMetrics(ForDpi), as done in Widget#getSystemMetrics()
  • We should load the native method dymically to avoid a linker error on platforms that do not provide that method. This is to avoid the issue that we had before that SWT actually required rather update-to-date Windows versions due to rather new methods being statically linked to. For example, take a look at how GetSystemMetricsForDpi is called in os.c.

@HeikoKlare HeikoKlare force-pushed the master-170 branch 2 times, most recently from e5a2838 to 17f652f Compare January 10, 2025 09:09
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.

Replacing usages of that method with the DPI-dependent version makes sense.

I have updated the PR by merging the commits and fixing copy-paste errors in the documentation.

Still I am non-deterministically getting wrongly scaled versions of dialogs, e.g., this one completely missing the checkbox to not show that message again:
image
I also have a question regarding the DPI parameter of the newly adapted method, which may be related to the issue.

@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-170 branch 2 times, most recently from fe1292c to 35b3e3b Compare January 10, 2025 10:56
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 change itself looks sound, but I cannot successfully validate that it has the intended effect. With the change, I now always get a dialog that is cut off (not showing the checkbox):

image

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.

Turns out that the native adaptation was erroneous (linking to a wrong library), so that the method always returned false. I've fixed that with 99cf55d.

For me, dialogs now show up in the correct size for all scenarios (that I have tested). E.g., in the originally documented scenario (200% monitor), it looks as expected:
image

@ShahzaibIbrahim can you please evaluate whether the change work as expected as well?

@ShahzaibIbrahim
Copy link
Contributor Author

@HeikoKlare it is working as expected.

image

AdjustWindowRectEx does not take DPI into account, leading to dialog
windows being cut off. This change adjusts usages of that method to
AdjustWindowRectExForDpi, passing the zoom level to retrieve correct
coordinates and scaling accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confirmation Dialog Text Cutoff
2 participants