Repository navigation
fix(windows): use physical pixels in SetRelativePosition to fix DPI scaling - #5372
leaanthony wants to merge 7 commits into
Conversation
…tion On Windows with non-100% display scaling, SetRelativePosition incorrectly applied DPI scaling to the input coordinates. Since Win32 APIs such as GetCursorPos return physical pixel coordinates, passing those directly to SetRelativePosition caused the window to be placed 25% further from the origin at 125% scaling (error scales linearly with the coordinate value). The old implementation treated (x, y) as DIP and converted via DipToPhysicalRect, so a physical position (600, 400) became physical (750, 500) at 125% - a 25% overshoot growing with distance from origin. Fix: use physical pixel coordinates exclusively in both setRelativePosition and relativePosition. The implementation now adds PhysicalWorkArea.X/Y to get the absolute physical position and calls setPhysicalBounds directly, bypassing any DPI scaling. relativePosition subtracts PhysicalWorkArea.X/Y from GetWindowRect output (which is always physical on a DPI-aware process). Fixes: #4300 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. Walkthrough
ChangesWindow Positioning Coordinate System
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to This change updates Windows relative window positioning to use physical pixels, preventing DPI scaling from being applied twice. No remaining merge-readiness risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. I hop through pixels, crisp and bright, Comment |
There was a problem hiding this comment.
Pull request overview
Fixes Windows DPI-scaling misplacement for SetRelativePosition by treating (x, y) as physical pixel coordinates (consistent with Win32 APIs like GetCursorPos) rather than DIP/logical pixels.
Changes:
- Updated Windows
relativePosition()to compute offsets usingphysicalBounds()andscreen.PhysicalWorkArea. - Updated Windows
setRelativePosition()to compute absolute physical coordinates and callsetPhysicalBoundsdirectly (no DPI conversion).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Get window physical pixel position relative to the screen WorkArea on which it is | ||
| func (w *windowsWebviewWindow) relativePosition() (int, int) { | ||
| screen, _ := w.getScreen() | ||
| pos := screen.absoluteToRelativeDipPoint(w.bounds().Origin()) | ||
| // Relative to WorkArea origin | ||
| pos.X -= (screen.WorkArea.X - screen.X) | ||
| pos.Y -= (screen.WorkArea.Y - screen.Y) | ||
| return pos.X, pos.Y | ||
| physBounds := w.physicalBounds() | ||
| return physBounds.X - screen.PhysicalWorkArea.X, physBounds.Y - screen.PhysicalWorkArea.Y |
| // Set window position using physical pixel coordinates relative to the screen WorkArea. | ||
| // Using physical pixels matches Win32 APIs (e.g. GetCursorPos) and avoids DPI scaling | ||
| // errors at non-100% display scaling factors. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v3/pkg/application/webview_window_windows.go`:
- Around line 759-778: Both functions call screen, _ := w.getScreen() and assume
screen is non-nil; guard against a nil screen to avoid panics. In
relativePosition(), if getScreen() returns an error or nil screen, return the
window's absolute physical X,Y (from physBounds) or (0,0) as a safe fallback
instead of dereferencing screen.PhysicalWorkArea; in setRelativePosition(), if
screen is nil, treat the provided x,y as absolute physical coordinates (or keep
current physBounds if you prefer no-op) and call w.setPhysicalBounds with a safe
X/Y (do not access screen.PhysicalWorkArea when screen==nil). Update the
functions relativePosition() and setRelativePosition() accordingly to check for
nil and handle the fallback.
- Around line 759-778: The Windows implementation is using physical pixels while
other platforms use DIPs; update windowsWebviewWindow.relativePosition and
windowsWebviewWindow.setRelativePosition to convert between DIPs and physical
pixels using the display scale returned by getScreen(): compute scale :=
screen.Scale (or ScaleFactor) and return/consume DIP coords (relativePosition
should divide physical offsets by scale before returning; setRelativePosition
should multiply the DIP x,y by scale before adding screen.PhysicalWorkArea and
calling setPhysicalBounds). Keep references to getScreen(), physicalBounds(),
setPhysicalBounds(Rect{}) and ensure both methods consistently accept/return
DIPs like macOS/Linux.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75f2ef3a-c2f2-4e9e-a3c4-e47f29cc4363
📒 Files selected for processing (1)
v3/pkg/application/webview_window_windows.go
…ePosition Add nil check for screen returned by getScreen() to prevent a potential panic when the window is not found on any known monitor. Also clarify doc comments to explicitly reference PhysicalWorkArea. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
…ePosition arithmetic Covers the DPI regression (GitHub #4300) and the nil-screen guard added in the previous commit. Tests verify that physical-pixel coordinates are not re-scaled at 100%, 125%, 150%, and 200% DPI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
v3/pkg/application/webview_window_windows_test.go (3)
62-75: 🏗️ Heavy liftTests validate inlined arithmetic, not the production functions
Both arithmetic suites replicate the formula (
PhysicalWorkArea.X + inputX/physBounds.X - PhysicalWorkArea.X) directly in the test body rather than calling the actualsetRelativePosition/relativePositionmethods. If the production code ever drifts from this formula, these tests will stay green while the bug re-appears.This is likely an intentional trade-off because both methods call Win32 APIs (
GetWindowRect,SetWindowPos, etc.) that are hard to stub in pure Go unit tests. If that's the case, a short comment acknowledging the constraint would prevent future contributors from wondering why the real functions aren't invoked. Alternatively, consider extracting the coordinate-translation logic into small, unexported pure functions (e.g.,physToRelative,relativeToPhys) that take onlyRect/Screenarguments — those could be tested directly with zero Win32 dependency.Also applies to: 145-159
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/pkg/application/webview_window_windows_test.go` around lines 62 - 75, Tests are inlining the arithmetic instead of exercising the production functions (setRelativePosition, relativePosition) which can mask regressions; either add a short comment in webview_window_windows_test.go explaining the Win32 dependency prevents calling those functions directly, or better yet extract the pure coordinate-translation logic into small unexported helpers (e.g., physToRelative, relativeToPhys) that accept Screen/Rect and perform the arithmetic, update setRelativePosition/relativePosition to call those helpers, and change the tests to call the new helpers directly to verify translation behavior without Win32 stubs.
17-26: ⚡ Quick win
scaleFactorfield is populated but never read in the test bodyIn both
TestSetRelativePositionArithmeticandTestRelativePositionArithmetic,scaleFactoris set in every case buttt.scaleFactoris never referenced inside the loop body. The intent — demonstrating that scale is not applied — is valid and meaningful, but without a comment the field looks like a mistake or an incomplete assertion. A future reader may addtt.scaleFactorto the arithmetic thinking they're "fixing" it.✏️ Suggested clarification (same change applies to the analogous field in
TestRelativePositionArithmetic)tests := []struct { name string - scaleFactor float32 + scaleFactor float32 // intentionally unused: proves the fix — scale must NOT be re-applied workAreaX int workAreaY int inputX int inputY int wantX int wantY int }{Also applies to: 103-112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/pkg/application/webview_window_windows_test.go` around lines 17 - 26, The table-driven tests define a scaleFactor field that is never used (see the tests slice in TestSetRelativePositionArithmetic and TestRelativePositionArithmetic and the tt.scaleFactor identifier), which looks like a leftover; either remove the unused scaleFactor field from the test struct in both tests or explicitly document its intentional omission by adding a brief comment where the table is defined (e.g., “scaleFactor included to show scale is not applied — intentionally unused”) so future readers won't introduce tt.scaleFactor into the assertions by mistake.
84-91: ⚡ Quick winDead
elsebranches in nil-screen tests make it appear both paths are exercisedIn both
TestSetRelativePositionNilScreenandTestRelativePositionNilScreen,var screen *Screenis alwaysnil, so theelseblock is unreachable dead code. A reader could reasonably believe theelsepath is being covered here, which it isn't.Since the non-nil path is already covered by the arithmetic table tests, the cleanest fix is to drop the unreachable branch entirely:
✏️ Proposed fix for
TestSetRelativePositionNilScreenvar screen *Screen var result Rect - if screen == nil { - result = Rect{X: x, Y: y, Width: physBounds.Width, Height: physBounds.Height} - } else { - result = Rect{X: screen.PhysicalWorkArea.X + x, Y: screen.PhysicalWorkArea.Y + y, - Width: physBounds.Width, Height: physBounds.Height} - } + _ = screen // always nil; nil-path is the case under test + result = Rect{X: x, Y: y, Width: physBounds.Width, Height: physBounds.Height}✏️ Proposed fix for
TestRelativePositionNilScreenvar screen *Screen var gotX, gotY int - if screen == nil { - gotX, gotY = physBounds.X, physBounds.Y - } else { - gotX = physBounds.X - screen.PhysicalWorkArea.X - gotY = physBounds.Y - screen.PhysicalWorkArea.Y - } + _ = screen // always nil; nil-path is the case under test + gotX, gotY = physBounds.X, physBounds.YAlso applies to: 167-174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/pkg/application/webview_window_windows_test.go` around lines 84 - 91, The nil-screen tests (TestSetRelativePositionNilScreen and TestRelativePositionNilScreen) declare var screen *Screen which is always nil, so the else branch that uses screen.PhysicalWorkArea is dead code and should be removed; update the code that sets result to unconditionally assign Rect{X: x, Y: y, Width: physBounds.Width, Height: physBounds.Height} (remove the conditional and the else block), and apply the same cleanup to the duplicate occurrence in the other test (the second nil-screen test around the later block) so only the reachable nil-screen path remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@v3/pkg/application/webview_window_windows_test.go`:
- Around line 62-75: Tests are inlining the arithmetic instead of exercising the
production functions (setRelativePosition, relativePosition) which can mask
regressions; either add a short comment in webview_window_windows_test.go
explaining the Win32 dependency prevents calling those functions directly, or
better yet extract the pure coordinate-translation logic into small unexported
helpers (e.g., physToRelative, relativeToPhys) that accept Screen/Rect and
perform the arithmetic, update setRelativePosition/relativePosition to call
those helpers, and change the tests to call the new helpers directly to verify
translation behavior without Win32 stubs.
- Around line 17-26: The table-driven tests define a scaleFactor field that is
never used (see the tests slice in TestSetRelativePositionArithmetic and
TestRelativePositionArithmetic and the tt.scaleFactor identifier), which looks
like a leftover; either remove the unused scaleFactor field from the test struct
in both tests or explicitly document its intentional omission by adding a brief
comment where the table is defined (e.g., “scaleFactor included to show scale is
not applied — intentionally unused”) so future readers won't introduce
tt.scaleFactor into the assertions by mistake.
- Around line 84-91: The nil-screen tests (TestSetRelativePositionNilScreen and
TestRelativePositionNilScreen) declare var screen *Screen which is always nil,
so the else branch that uses screen.PhysicalWorkArea is dead code and should be
removed; update the code that sets result to unconditionally assign Rect{X: x,
Y: y, Width: physBounds.Width, Height: physBounds.Height} (remove the
conditional and the else block), and apply the same cleanup to the duplicate
occurrence in the other test (the second nil-screen test around the later block)
so only the reachable nil-screen path remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fadd97f4-e346-461d-b7a8-e62351c66387
📒 Files selected for processing (1)
v3/pkg/application/webview_window_windows_test.go
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Thank you for investigating this and putting together the fix. After reviewing the coordinate contract and testing the production methods on Windows, we're closing this PR because changing Wails' window positions, dimensions, and screen work areas use device-independent (logical) pixels. This change makes the relative-position APIs inconsistent with For the mouse-following case in #4300, the workaround is to convert the absolute physical mouse coordinates to logical coordinates and use the absolute-position API: x, y := robotgo.Location()
p := application.PhysicalToDipPoint(application.Point{X: x, Y: y})
wd.SetPosition(p.X, p.Y)This preserves the existing API contract and avoids passing absolute desktop coordinates to a work-area-relative API. Please validate this adjustment in the original reproducer, including displays with nonzero origins; #4300 can remain open for that follow-up. Thanks again for the contribution. The reported symptom deserves addressing, but changing the units of these existing APIs is not a compatible fix. |
Summary
SetRelativePositionon Windows incorrectly treated its(x, y)input as DIP (logical pixels) and applied a DPI scale factor. On a 125%-scaled display, passing coordinates fromGetCursorPos/robotgo.Location()(physical pixels) caused the window to be placed 25% further from origin than intended — a deviation that grows linearly with the coordinate value.setRelativePositionandrelativePositioninwebview_window_windows.goto work exclusively in physical pixels, consistent with Win32 APIs that return physical screen coordinates on DPI-aware processes. The implementation now addsscreen.PhysicalWorkArea.X/Yto compute the absolute physical position and callssetPhysicalBoundsdirectly without any DPI conversion.Root cause
setRelativePositionpreviously calledrelativeToAbsoluteDipPoint(treating input as DIP) thensetPosition(which appliesDipToPhysicalRect). At 125% scaling this multiplied user-provided coordinates by 1.25, so physical (600, 400) became physical (750, 500). The error scales with the distance from the origin, matching the reported symptom.Reproduce steps (before fix)
window.SetRelativePosition(600, 400)(e.g. from mouse cursor coordinates)After fix
SetRelativePosition(x, y)places the window at physical position(PhysicalWorkArea.X + x, PhysicalWorkArea.Y + y)— no DPI scaling applied to the user-supplied coordinates.RelativePosition()returns(physX - PhysicalWorkArea.X, physY - PhysicalWorkArea.Y)so the round-trip is correct.Test plan
go build ./pkg/application/✅go test ./pkg/application/✅Fixes #4300
CC @leaanthony
Summary by CodeRabbit
Bug Fixes
Tests