-
Notifications
You must be signed in to change notification settings - Fork 182
Make DPI_CHANGED event handling async #2520
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
Make DPI_CHANGED event handling async #2520
Conversation
What's the relation of this to ... ? Changes look quite similar. |
Okay, then it would be good to close that other PR since it misses a title and description anyway. |
f5e9146
to
e65d64d
Compare
bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/internal/DPITestUtil.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Widget.java
Outdated
Show resolved
Hide resolved
32a70b6
to
3063414
Compare
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Control.java
Show resolved
Hide resolved
8254b76
to
c6bd75f
Compare
} | ||
|
||
void handleMonitorSpecificDpiChange(int newNativeZoom, Rectangle newBoundsInPixels) { | ||
private void handleMonitorSpecificDpiChange(int newNativeZoom, Rectangle newBoundsInPixels) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WM_DPICHANGED
does nothing if nativeZoom == newNativeZoom
. Since all events are processed asynchronous (even handling the initial DPI change event for the shell) it can happen that a shell is moved back and forth and the second event is not processed because of the above condition, leading to the shell having a wrong scaling. As an example, imagine you move a shell from 100% to 125% monitor. WM_DPICHANGED
forwards this event as 100 != 125
and the event is queued. Then the shell is immediately moved back from 125% to 100%. WM_DPICHANGED
does not forward the event because the shell's native zoom is still 100 (the event to change it is only queued but not processed), just like the new zoom. So only the first event will be processed, leaving behind a 125% shell on a 100% monitor.
A solution would be to remove the condition such that every call to WM_DPICHANGED
will result in an event to be processed. As an alternative, the initial event on shell level could be processed synchronously to immediately set the shell's native zoom.
Another effect of the above issue is that the shell can shrink or enlarge when rapidly moving around. That will also be fixed with the solution proposals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I removed this condition. I don't see any harm in doing it, actually I just verified it by injecting the same multiple events for the same zoom change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you now made the event processing at shell level synchronous, it would probably be possible to revert that change.
return; | ||
} | ||
if (handleDPIChangedScheduledTasksCount.decrementAndGet() <= 0) { | ||
currentDpiChangeEvent = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this only be done when event.doit
? Otherwise the event currently being processed could have been cancelled at the very end of processing it (thus counter is 0), so the current event would be cleared even though it is still being processed (as it's a different event than the current). Another option would be to only clear currentDpiChangeEvent
if currentDpiChangeEvent == event
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both conditions should be fine, as long as currentDpiChangeEvent
is only changed when event.doit
is falsified. I still took the second proposal as it seemed safer to me.
cb98f63
to
cf46e6e
Compare
@fedejeanne Could you have a second look on this PR. I noticed an issue when changing monitors quite fast so the second DPI change event was triggered before the first one was fully handled. This issue should not appear anymore, at least I could not reproduce it anyhow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw. I'm on Windows 11 since this morning. Everything looks good on the |
cf46e6e
to
7ea01d5
Compare
@fedejeanne Good catch with the dark theme. As light and dark theme menu bar is rendered differently this was caused as the call to getBoundsInPixels triggered a re-render of the menu bar, but the menu bar was not yet updated - the async call was not yet processed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akoch-yatta I tested again and I still see some issues.
My setup (same as before)
- 175% monitor on the left (primary)
- 100% monitor on the right
Issue 1: Icons have the wrong size

To reproduce
- Start the workbench on the 100% monitor (if necessary, move it there and restart it).
Issue Nr. 2: Editors are cut-off

To reproduce (only happens sporadically)
- Either move back and forth really fast between the 2 monitors and then leave the workbench in the 175% monitor or
- Restart the application in the 175% monitor
The last one is really hard to reproduce (I've only seen it once when moving back and forth and once when restarting).
7ea01d5
to
323ee58
Compare
@fedejeanne the first issue was caused by ane interfering DPI change events triggered by moving the shell and changing the parent of ImageBasedFrame + the ZoomChange event listener in ImageBasedFrame. This listener currently relies on a Resize event from the Shell, which is not thrown in the scenario, when the parent is changed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (only an unnecessary empty line)
I wasn't able to reproduce error Nr. 2 either, thank you for the fix @akoch-yatta !
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Shell.java
Outdated
Show resolved
Hide resolved
db80447
to
313bfcf
Compare
With this commit, all the widgets scale themselves asynchronously independent of the order saving the wait time over their children to scale.
313bfcf
to
1bda21e
Compare
With this PR, all the widgets scale themselves asynchronously independent of the order saving the wait time over their children to scale.
Earlier, the widgets would scale synchronously on a
DPI_CHANGED
event from the OS, i.e., the shell receives the callback from the OS and calls and waits for its children to execute theirhandleDPIChange
(Over all the superclasses of course) and every widget onhandleDPIChange
method being triggered, would callhandDPIChange
for their children widgets or associated widgets and wait for them to finish and them execute he remaining scaling statements and pass the control to their parents. This whole flow would keep the thread busy for a significant amount of time making the UI freeze for sometime.With this approach, the Shell receives the callback from the OS and executes its
handleDPIChange
(Over all the superclasses of course) and sends a notification asynchronously, i.e., queues thenotifyListeners
to be executed for its children when the thread is free. Once the first queuednotifyListeners
runs, it would invoke thehandleDPIChange
for that particular widget, which would then schedule DPI scaling calls for its children and associated widgets and move on with itshandleDPIChange
execution. This would definitely change the hierarchical order in which the widgets scale themselves. Considering that the parents need to resize them on the basis of the size of the children, there needs to be a recalibration of the bounds for the widgets once all the widgets have scaled themselves. Hence, we keep a note of when all the widgets have completed their scaling, the last widget to scale itself triggers a Resize event, leading every widget to recalibrate their bounds. This leads to a better UI freeze free scaling where all the widgets inside the shell are still operable/clickable amidst the scaling.Only the last commit is relevant to this PR.
Depends on #2483