[Win32] Lay out the shell once per zoom change - #3564
Open
HeikoKlare wants to merge 1 commit into
Open
Conversation
Contributor
The processing of a zoom change is split into tasks, one per control, which may be performed asynchronously. The shell is laid out when the last of those tasks has been completed, which is how the layout is deferred until all controls have been adapted to the new zoom. The shell itself does not hold such a task while it propagates the zoom changed event through the widget tree. Whenever the controls are adapted synchronously, as done for child shells and for composites without a layout, the number of outstanding tasks therefore drops to zero after every single control, so the shell is laid out once per control instead of once per zoom change. Doing so is not only superfluous work, it also lays out the shell while parts of it have not been adapted yet. The propagation of the zoom changed event is thus made a task of the zoom change as well, so that the zoom change cannot be considered complete before all controls have been adapted, independently of whether they are adapted synchronously or asynchronously. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
HeikoKlare
force-pushed
the
zoom-change-layout-once
branch
from
September 3, 2026 15:57
8ddd4e0 to
b1dae79
Compare
HeikoKlare
marked this pull request as ready for review
September 3, 2026 16:36
There was a problem hiding this comment.
🟡 Changes recommended
The new zoom-change task completion logic can still trigger multiple layouts if the task counter goes negative due to an imbalance, which should be guarded to ensure exactly-once layout.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adjusts Win32 DPI/zoom-change handling so that a shell is laid out exactly once per zoom change, even when parts of the zoom propagation run synchronously. It does this by counting the shell’s propagation work as a zoom-change “task”, ensuring the final layout is deferred until all zoom-change tasks (sync or async) have completed.
Changes:
- Wrap shell zoom-change propagation (
notifyListeners+ bounds update) in a zoom-change task so completion/layout can’t occur mid-propagation. - Refactor zoom-change task tracking into
DPIChangeExecutionwith explicitstartTask/completeTaskhelpers and exposeisComplete()for tests. - Add a Win32 test that reproduces the “multiple layouts per zoom change” regression and asserts a single layout.
File summaries
| File | Description |
|---|---|
| bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Shell.java | Treats shell zoom-change propagation as a counted task to prevent premature layouts. |
| bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Control.java | Refactors zoom-change execution/task accounting and centralizes layout-on-completion behavior. |
| bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/widgets/WidgetWin32Tests.java | Adds regression test asserting one layout per synchronous zoom change. |
| bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/widgets/DPITestUtil.java | Adds a helper to drive the monitor-change zoom-change entry point and wait for completion. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+6062
to
+6066
| private void completeTask(Shell shell) { | ||
| if (taskCount.decrementAndGet() <= 0 && event.doit && !shell.isDisposed()) { | ||
| shell.layout(true, true); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The processing of a zoom change is split into tasks, one per control, which may be performed asynchronously. The shell is laid out once the last of those tasks has been completed, which is how the layout is deferred until all controls have been adapted to the new zoom.
The shell itself holds no such task while it propagates the zoom changed event through the widget tree. Whenever the controls are adapted synchronously, as done for child shells and for composites without a layout, the number of outstanding tasks therefore drops to zero after every single control: a shell with three children is laid out three times instead of once, and each of those layouts runs while the remaining children still carry the old zoom. On the asynchronous path the problem does not appear, because there all controls are scheduled, and counted, before the first one completes.
The propagation of the zoom changed event is thus made a task of the zoom change as well, so that it cannot be considered complete before all controls have been adapted, independently of how they are adapted. The added test drives the entry point that is used when the operating system reports a monitor change, for which the shell's handling had to become accessible to the test, and asserts that a single zoom change causes a single layout; it fails with three layouts without the fix.
Two further consequences: the concluding layout now happens after the shell has been moved to the new monitor instead of before, and
Control#setAutoscalingModetriggers one layout for a control that is not a composite, where it previously triggered none.🤖 Generated with Claude Code