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

[win32] Refresh thread dpi awareness on hooks #1813

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

akoch-yatta
Copy link
Contributor

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

This commit resets the DPI awareness for the WH_MSGFILTER hook, if called. For performance reasons that is only done, when runAsyncMessages will be triggered, as this can cause UI updates, which would be executed with the wrong DPI awareness context and cause UI glitches.

How to reproduce

Have monitor specific scaling active and at least two monitors.

  1. Open an IDE opened on a primary monitor with a low zoom (e.g. 100%)
  2. Open a Java File in the Editor
  3. Move the IDE to a secondary monitor with a higher zoom (e.g. 200%)
  4. Use the vertical scrollbar to scroll up and down and you should see an effect, e.g. on the LineNumberRuler
Bildschirmaufnahme.2025-02-10.121127.mp4

Copy link
Contributor

github-actions bot commented Feb 7, 2025

Test Results

   502 files  ±0     502 suites  ±0   10m 23s ⏱️ + 2m 28s
 4 334 tests ±0   4 320 ✅ ±0   14 💤 ±0  0 ❌ ±0 
16 575 runs  ±0  16 466 ✅ ±0  109 💤 ±0  0 ❌ ±0 

Results for commit 918acf9. ± Comparison against base commit df1c412.

♻️ This comment has been updated with latest results.

@akoch-yatta akoch-yatta force-pushed the ensure-thread-dpi-awareness branch from 4e3ac99 to c9dc6e1 Compare February 10, 2025 10:41
@akoch-yatta akoch-yatta marked this pull request as ready for review February 10, 2025 10:44
Comment on lines 3434 to 3438
long previousDPIAwareness = refreshDPIAwareness();
if (runAsyncMessages (false)) wakeThread ();
if (previousDPIAwareness > 0) {
OS.SetThreadDpiAwarenessContext(previousDPIAwareness);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What about extracting the whole set/reset logic into a separate method, so that this call looks something like this:

Suggested change
long previousDPIAwareness = refreshDPIAwareness();
if (runAsyncMessages (false)) wakeThread ();
if (previousDPIAwareness > 0) {
OS.SetThreadDpiAwarenessContext(previousDPIAwareness);
}
runWithProperDPIAwareness(() -> {
if (runAsyncMessages(false)) wakeThread();
});

and refreshDPIAwareness() is extended to runWithProperDPIAwareness(Runnable operation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adapted it accordingly

@akoch-yatta akoch-yatta force-pushed the ensure-thread-dpi-awareness branch from c9dc6e1 to e36c58e Compare February 10, 2025 16:57
This commit resets the DPI awareness for the WH_MSGFILTER hook, if called.
For performance reasons that is only done, when runAsyncMessages will be
triggered, as this can cause UI updates, which would be executed with the
wrong DPI awareness context and cause UI glitches.
@akoch-yatta akoch-yatta force-pushed the ensure-thread-dpi-awareness branch from e36c58e to 918acf9 Compare February 11, 2025 08:16
@@ -5382,4 +5398,18 @@ private boolean setDPIAwareness(int desiredDpiAwareness) {
return true;
}

private void runWithProperDPIAwareness(Runnable operation) {
if (isRescalingAtRuntime()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice to still execute the passed operation if isRescalingAtRuntime() == false, but just without correcting DPI awareness 😉

int flags = OS.PM_NOREMOVE | OS.PM_NOYIELD | OS.PM_QS_INPUT;
if (!OS.PeekMessage (msg, 0, 0, 0, flags)) wakeThread ();
sendPreExternalEventDispatchEvent ();
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that preserving formatting should not be the primary goal when processing changes, but if so much code like here is affected where the history might be interesting for different reasons (e.g., to find the commit when this handling of the "bug in Windows" was introduced, I was wondering whether we could still achieve that here without reducing comprehensibility. For example, wrapping the whole block inside the if (!synchronizer.isMessagesEmpty()) { into a processMessages runnable or the like, which is then executed with proper DPI awareness inside that if block, i.e.:

if (code >= 0) {
   Runnable processMessages = () -> {
      // all the existing code
   }
   if (!synchronizer.isMessagesEmpty()) {
      runWithProperDPIAwareness(processMessages);
   }
}

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.

Sidebar images misplaces when scrolling with Scrollbar
2 participants