[Win32] Do not constrain Shell size to owner shell bounds - #3599
HeikoKlare merged 1 commit into
Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The regression test setup may not reliably reproduce the sizing bug.
Review effort: Lite
Findings: None
What changed in this PR
Fixes Win32 owned-shell sizing by preventing owner bounds from incorrectly constraining shell dimensions.
Changes:
- Exempts all
Shellinstances from parent-bound clamping. - Adds regression coverage for owned-shell width and height sizing.
| File | Summary |
|---|---|
tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Shell.java |
Adds owned-shell sizing regression coverage; test coordinates need adjustment to reliably exercise the bug. |
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Control.java |
Prevents owner bounds from constraining shell sizes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
abca876 to
b394702
Compare
fitInParentBounds() clamps a control's requested bounds to fit inside its parent's client area, evening out off-by-one rounding errors from point/pixel conversions. Shell.setSize() (inherited from Control, not overridden) also routed through this method, treating the Shell's owner (Control.parent) as if it were a geometric container. An owned Shell is a top-level window positioned independently of its owner on screen; it is not laid out inside the owner's rectangle. The clamp could therefore spuriously shrink a Shell's requested height or width by one pixel whenever the owner's and the owned shell's on-screen positions happened to coincide within the off-by-one tolerance window, causing sporadic Test_org_eclipse_swt_widgets_Shell.test_setSizeII / test_setSizeLorg_eclipse_swt_graphics_Point failures unrelated to DPI autoscaling. Add a deterministic regression test that explicitly positions the owned shell to hit the off-by-one tolerance window on both axes. The test runs on all platforms; assertions wait via SwtTestUtil.processEvents() for the requested size to be reported, since GTK reports Shell bounds asynchronously via the window manager. Fixes eclipse-platform#3597 Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
b394702 to
3a9abc6
Compare
It correctly captures the problematic scenario and it fails reliably on Windows without the fix (on a 100% and 125% zoom system). There is a slight risk that future changes to the fitting logic and a reintroduction of the bug (i.e., removing the shell check in the fitting logic) introduce a regression without the test noting it, but it's impossible to prepare for every possible scenario. I have also removed the Windows guard from the test, as the tested behavior should obviously also be fulfilled on Linux and macOS. |
The issue
Shell.setSize() sporadically sets an owned shell one pixel smaller than requested. An owned shell is one created with another shell as its parent, as is the case in the test fixture from the issue. The failure appears only at specific sizes in a given run and is reproducible at 100% monitor zoom, ruling out DPI autoscaling.
SWT contains a sizing helper that absorbs rounding errors arising when a layout system converts a composite's client area between point and pixel units and positions a child control to fill it. This helper has a guard that is supposed to skip its logic for shells, but that guard only covers shells without a parent. An owned shell has a non-null parent, so it fell through the guard and was incorrectly subjected to the bounds clamping.
For a control nested inside a composite, comparing the control's requested size against the parent composite's bounds is geometrically sound. For an owned shell the parent is its owner window, a completely independent top-level window with no geometric containment relationship. The comparison therefore operates on unrelated quantities, and whenever they coincide within the one-pixel tolerance by chance, the size gets silently reduced by one.
The fix
The fix extends the guard to skip the sizing helper for any Shell, not just for shells without a parent. A Shell is always a geometric top-level element; even if it has an owner set as parent, that denotes a behavioral relationship (z-order, modality), not geometric containment, so the owner's bounds are irrelevant to sizing. A deterministic regression test has been added that explicitly constructs the position and size configuration that triggers the off-by-one clamping on both axes.
Relation to #3598
This PR supersedes #3598, which addresses the symptom by adjusting the test assertion tolerance. The fix here corrects the underlying production-code behaviour; in my opinion a fix in production code is preferable to working around an implementation bug in a test.
Fixes #3597