fix(darwin): use primary screen height for SetPosition Y conversion#5304
Conversation
When calling SetPosition on macOS, the Y coordinate was calculated using the current window's screen height as the reference, which produces the wrong NSWindow frame.origin.y when the target position is on a monitor at a different vertical offset from the primary screen. macOS NSWindow coordinates have Y=0 at the bottom-left of the primary screen (always [[NSScreen screens] firstObject], which is at origin (0,0)). Both windowGetPosition and windowSetPosition now use the primary screen's height as the global Y baseline, making get and set consistent inverses regardless of which screen the window currently lives on. Fixes #5117 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:
WalkthroughNormalize macOS coordinates: screens and window Position/SetPosition now use a primary-screen–anchored logical point space with Y-down and (0,0) at the primary top-left; window position conversions no longer apply per-screen DPI scaling and use the primary screen height for Y flips. ChangesmacOS Cross-Screen Window Positioning Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v3/pkg/application/webview_window_darwin.go`:
- Around line 621-627: windowGetPosition and windowSetPosition currently mix the
window's screen backingScaleFactor with the primary screen's height, causing
drift on mixed-DPI setups; fix by obtaining the primary screen's
backingScaleFactor (e.g. from [[[NSScreen screens] firstObject]
backingScaleFactor]) and use that same primary scale for both X and Y
conversions in windowGetPosition (where *x and *y are set) and in
windowSetPosition (where you convert desired coords back to frame.origin),
replacing any use of the current window screen's backingScaleFactor so both axes
share the global primary-screen scale baseline.
🪄 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: 616c5920-52e6-4184-864a-a2ce2698afc1
📒 Files selected for processing (2)
v3/UNRELEASED_CHANGELOG.mdv3/pkg/application/webview_window_darwin.go
There was a problem hiding this comment.
Pull request overview
This PR adjusts macOS window position Y-coordinate conversion to use the primary screen’s height as the global reference, aiming to make Position() and SetPosition() behave consistently across multi-monitor layouts with vertical offsets.
Changes:
- Update
windowGetPositionto compute Y using the primary screen height baseline. - Update
windowSetPositionto compute Y using the primary screen height baseline. - Add an unreleased changelog entry for the macOS cross-screen
SetPositionfix.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
v3/pkg/application/webview_window_darwin.go |
Switches Y conversion baseline from the current window’s screen height to the primary screen height for absolute positioning. |
v3/UNRELEASED_CHANGELOG.md |
Documents the macOS SetPosition cross-screen Y conversion fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ed-DPI Address CodeRabbit and Copilot review: windowGetPosition and windowSetPosition were using the current window screen's backingScaleFactor while relying on the primary screen's height for Y. On mixed-DPI setups this mixes units — e.g. primary at 2x height 800pt with secondary at 1x produces incorrect scaled values. Both functions now obtain scale from the primary screen (firstObject) to match its height reference, making get/set strict inverses in a consistent "primary-screen physical pixel" coordinate space regardless of which monitor the window occupies. 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 (2)
v3/UNRELEASED_CHANGELOG.md (2)
29-29: ⚡ Quick winAdd PR link and author credit.
This entry is missing a PR reference and author credit, making it inconsistent with other entries in the changelog (lines 26, 28).
Additionally, the static analysis hint about "systray" is a false positive—it's a valid technical abbreviation for "system tray" commonly used in programming contexts.
As per coding guidelines: "Reference issue/PR numbers when applicable" to maintain changelog traceability and consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/UNRELEASED_CHANGELOG.md` at line 29, The changelog entry starting "Fix a family of Windows systray `SetMenu` crashes..." is missing a PR reference and author credit; update that entry to match the style of the neighboring entries by appending the PR number/link and the author name (e.g., "(`#1234` by `@username`)") in the same format used on lines 26 and 28, and leave the term "systray" as-is (it's an accepted abbreviation), ensuring punctuation and spacing align with other entries.
27-27: ⚡ Quick winAdd PR link and author credit for consistency.
This entry should follow the same format as the surrounding entries (lines 26, 28) by including a link to the PR and crediting the author.
📝 Proposed fix for consistent formatting
-- Fix `SetPosition` cross-screen Y conversion on macOS: use primary screen height as global reference so windows land at the correct position on monitors that are vertically offset from the primary display in [`#5117`](https://github.com/wailsapp/wails/issues/5117) +- Fix `SetPosition` cross-screen Y conversion on macOS: use primary screen height as global reference so windows land at the correct position on monitors that are vertically offset from the primary display in [`#5117`](https://github.com/wailsapp/wails/issues/5117) / [PR](https://github.com/wailsapp/wails/pull/5304) by `@leaanthony`As per coding guidelines: "Reference issue/PR numbers when applicable" and maintain consistency with other changelog entries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/UNRELEASED_CHANGELOG.md` at line 27, The changelog entry for the Fix to SetPosition on macOS is missing the PR link and author credit; update the line mentioning SetPosition so it matches surrounding entries by appending the PR link and author credit in the same format (e.g., include "in [`#5117`](https://github.com/wailsapp/wails/pull/5117) by `@author`"), and ensure it references SetPosition and "primary screen height" as the fix description.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v3/UNRELEASED_CHANGELOG.md`:
- Line 29: The changelog entry starting "Fix a family of Windows systray
`SetMenu` crashes..." is missing a PR reference and author credit; update that
entry to match the style of the neighboring entries by appending the PR
number/link and the author name (e.g., "(`#1234` by `@username`)") in the same
format used on lines 26 and 28, and leave the term "systray" as-is (it's an
accepted abbreviation), ensuring punctuation and spacing align with other
entries.
- Line 27: The changelog entry for the Fix to SetPosition on macOS is missing
the PR link and author credit; update the line mentioning SetPosition so it
matches surrounding entries by appending the PR link and author credit in the
same format (e.g., include "in
[`#5117`](https://github.com/wailsapp/wails/pull/5117) by `@author`"), and ensure it
references SetPosition and "primary screen height" as the fix description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5cca8d78-63cc-4582-9c61-7e3b9b2a96a2
📒 Files selected for processing (1)
v3/UNRELEASED_CHANGELOG.md
|
This PR has the "awaiting review 🕝" label. Please confirm that you have tested this on a multi-monitor setup, especially with vertically stacked monitors at different Y offsets, as mentioned in the test plan. The fix addresses window positioning across monitors on macOS by using the primary screen height as the reference for Y coordinate conversion. Please verify:
|
|
The feedback on #5117 (comment) should be probably be addressed before merging. And this change will break existing usages working around the issue - so maybe should be flagged in the release logs. |
…cal points, Y-down, top-left) flofreud's review of #5304 (#5117 (comment)) flagged that macOS still exposed three distinct coordinate spaces: * GetScreens() -> NSScreen logical points, Y-up, bottom-left of primary * Position()/Set -> NSScreen logical points * pSF, Y-down, top-left of primary * RelativePosition -> NSScreen logical points, Y-down, top-left of *current* screen This commit collapses GetScreens() and Position()/SetPosition() into a single canonical space: logical points, Y-down, with (0,0) at the top-left of the primary screen. This is the same convention exposed publicly by Electron's Display.bounds, Tauri's Monitor::position, Win32 MONITORINFO, GTK and the web (window.screenY / Screen.availTop). Windows + Linux paths in Wails already expose this space, so the macOS normalisation also makes the three platforms match each other. Changes in webview_window_darwin.go: * windowGetPosition / windowSetPosition drop the `* primaryScale` / `/ primaryScale` multiplications. Values are now logical points, not "points x primaryScale". The Y flip against primary height stays. Changes in screen_darwin.go: * processScreen flips Y on both `frame` and `visibleFrame` against the primary screen's height, so Bounds.Y is the screen's top edge and screens above the primary have negative Y. screenmanager's getScreenAlignment (which already assumed Y-down) becomes correct for macOS. After this change `SetPosition(s.Bounds.X + dx, s.Bounds.Y + dy)` lands at the expected offset on any screen, with no scale dance required. BREAKING: callers that hand-compensated with `* primaryScale` or that flipped Y against a screen height (e.g. the workaround in #5117) must drop those conversions. Position()->SetPosition() round-trips are preserved. A separate, pre-existing inconsistency in RelativePosition() (Get returns absolute X, Set treats X as screen-relative) was also flagged in the same review and should be addressed in its own issue/PR. Refs #5117
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/screen_darwin.go`:
- Around line 47-51: GetPrimaryScreen() currently uses [NSScreen mainScreen]
which can differ from the fixed primary/menu-bar display used by processScreen()
(which normalizes Y against [[NSScreen screens] firstObject]); change
GetPrimaryScreen() to use [[NSScreen screens] firstObject] instead of [NSScreen
mainScreen] so it returns the same primary screen as processScreen(), and make
the same replacement in the other occurrence (the block around the second
NSScreen retrieval) to ensure Bounds and window position normalization use a
consistent primary display.
🪄 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: 83c18806-4268-461f-82b2-3f92956afc80
📒 Files selected for processing (3)
v3/UNRELEASED_CHANGELOG.mdv3/pkg/application/screen_darwin.gov3/pkg/application/webview_window_darwin.go
✅ Files skipped from review due to trivial changes (1)
- v3/UNRELEASED_CHANGELOG.md
…cal points, Y-down, top-left) flofreud's review of #5304 (#5117 (comment)) flagged that macOS still exposed three distinct coordinate spaces: * GetScreens() -> NSScreen logical points, Y-up, bottom-left of primary * Position()/Set -> NSScreen logical points * pSF, Y-down, top-left of primary * RelativePosition -> NSScreen logical points, Y-down, top-left of *current* screen This commit collapses GetScreens() and Position()/SetPosition() into a single canonical space: logical points, Y-down, with (0,0) at the top-left of the primary screen. This is the same convention exposed publicly by Electron's Display.bounds, Win32 MONITORINFO, GTK and the web (window.screenY / Screen.availTop). Windows + Linux paths in Wails already expose this space, so the macOS normalisation also makes the three platforms match each other. Changes in webview_window_darwin.go: * windowGetPosition / windowSetPosition drop the `* primaryScale` / `/ primaryScale` multiplications. Values are now logical points, not "points x primaryScale". The Y flip against primary height stays. Changes in screen_darwin.go: * processScreen flips Y on both `frame` and `visibleFrame` against the primary screen's height, so Bounds.Y is the screen's top edge and screens above the primary have negative Y. screenmanager's getScreenAlignment (which already assumed Y-down) becomes correct for macOS. After this change `SetPosition(s.Bounds.X + dx, s.Bounds.Y + dy)` lands at the expected offset on any screen, with no scale dance required. BREAKING: callers that hand-compensated with `* primaryScale` or that flipped Y against a screen height (e.g. the workaround in #5117) must drop those conversions. Position()->SetPosition() round-trips are preserved. A separate, pre-existing inconsistency in RelativePosition() (Get returns absolute X, Set treats X as screen-relative) was filed as #5408; the mixed-DPI Bounds.X/Y scale-factor issue on non-primary screens was filed as #5409. Refs #5117
a87758c to
6975c21
Compare
Summary
SetPosition()on macOS was calculating Y using the current window's screen height as the reference. This is wrong when the target position is on a monitor at a different vertical offset from the primary screen.[[NSScreen screens] firstObject], always at origin (0,0)). The primary screen's height is therefore the correct global Y baseline.windowGetPositionandwindowSetPositionnow use[[[NSScreen screens] firstObject] frame].size.heightfor the Y conversion, makingPosition()andSetPosition()consistent inverses across all monitor layouts.Fixes #5117.
Test plan
Position(), thenSetPosition()with the returned values — window must end up at the same locationSetPosition(x, y)where x falls on the second monitor — window must appear at the correct Y position🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation