[V3] MacOS + Linux Screen Management Fixes#5067
Open
atterpac wants to merge 10 commits intowailsapp:v3-alphafrom
Open
[V3] MacOS + Linux Screen Management Fixes#5067atterpac wants to merge 10 commits intowailsapp:v3-alphafrom
atterpac wants to merge 10 commits intowailsapp:v3-alphafrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughAdds Screen lookup APIs (GetByID, GetByIndex) and a Window.SetScreen method; implements screen caching/processing at app startup for macOS/Linux; extends message processors and runtime bindings; and updates platform-specific window placement to support screen-relative positioning. Changes
Sequence DiagramsequenceDiagram
participant Client as Client (Desktop Runtime)
participant Handler as Runtime Handler
participant ScreenMgr as ScreenManager
participant Window as WebviewWindow
participant Platform as Platform Impl
Client->>Handler: SetScreen(screenID)
activate Handler
Handler->>ScreenMgr: GetByID(screenID)
activate ScreenMgr
ScreenMgr-->>Handler: Screen object
deactivate ScreenMgr
Handler->>Window: SetScreen(screen)
activate Window
Window->>Window: store options.Screen
alt window initialized (impl != nil)
Window->>Platform: centerOnScreen / setPosition (sync)
activate Platform
Platform->>Platform: compute pos from screen.WorkArea
Platform-->>Window: applied position
deactivate Platform
else window not initialized
Window-->>Window: deferred placement
end
Window-->>Handler: OK
deactivate Window
Handler-->>Client: Success
deactivate Handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.16.4)v3/pkg/application/application_server.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m v3/pkg/application/linux_cgo.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m v3/pkg/application/linux_purego.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v3/pkg/application/linux_purego.go (1)
771-775:⚠️ Potential issue | 🔴 CriticalDuplicate function definition will cause compilation error.
The function
windowGetPositionis defined twice in this file: once at lines 771-775 and again at lines 821-826. This will cause a Go compilation error.🐛 Proposed fix - remove the duplicate definition
Remove the first definition at lines 771-775. The second definition at lines 821-826 (which includes a TODO comment) should be kept:
-func windowGetPosition(window pointer) (int, int) { - var x, y int - gtkWindowGetPosition(window, &x, &y) - return x, y -} - func windowGetCurrentMonitor(window pointer) pointer {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/linux_purego.go` around lines 771 - 775, There are two definitions of windowGetPosition which causes a compile error; remove the earlier duplicate definition that calls gtkWindowGetPosition (the one at the first occurrence) and keep the later definition (the one with the TODO) so only the single intended windowGetPosition implementation remains.
🧹 Nitpick comments (1)
v3/pkg/application/webview_window_linux.go (1)
342-350: Extract the screen-aware placement math into one helper.The
Screen != nilbranch is duplicated before and aftershow(). Keeping that coordinate calculation in one place will make the next Linux placement fix much less likely to land in only one path.Also applies to: 398-406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/webview_window_linux.go` around lines 342 - 350, Extract the duplicated screen-aware placement logic into a single helper (e.g., a method on webviewWindow like computeAndSetPosition or getCenteredPosition) that reads w.parent.options.Screen.WorkArea, w.parent.options.InitialPosition/WindowCentered, w.parent.options.X/Y and w.size(), computes the final X/Y, and calls w.setPosition; then replace both duplicated blocks (the branch before show and the later branch) to call this helper so the center vs offset math is implemented only once (ensure helper handles nil Screen by falling back to using options.X/Y).
🤖 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/linux_cgo.go`:
- Around line 1511-1564: getPrimaryScreen currently hardcodes ID "0", breaking
the ID contract used by getScreenByIndex; instead determine the actual monitor
index and use that as the ID. In getPrimaryScreen(), call
C.gdk_display_get_n_monitors(display) and loop i from 0..n-1 calling
C.gdk_display_get_monitor(display, C.gint(i)) to find the monitor pointer that
equals the primary monitor variable, then set ID = strconv.Itoa(i) (or
C.GoString after formatting) before returning; reference getPrimaryScreen and
getScreenByIndex so the returned ID matches the index-based contract.
In `@v3/pkg/application/screen_darwin.go`:
- Around line 179-185: The getters macosApp.getPrimaryScreen and
macosApp.getScreens currently return the cached m.parent.Screen.primaryScreen /
m.parent.Screen.screens directly which can be nil/empty before
ApplicationDidFinishLaunching populates them; change both getters to lazily
hydrate the cache: if primaryScreen is nil or screens length is 0, call a helper
like Screen.ensureHydrated (create it if missing) that queries the system
display APIs (e.g. NSScreen/CGDisplay) and populates
m.parent.Screen.primaryScreen and m.parent.Screen.screens, then return the
populated values so app.Screen.GetPrimary/GetAll and WebviewWindowOptions.Screen
work during early setup.
In `@v3/pkg/application/screen_linux.go`:
- Around line 9-35: processAndCacheScreens() is publishing a
partially-initialized cache via a.parent.Screen.LayoutScreens(screens) while
calculateScreensDipCoordinates can still be running, causing
getPrimaryScreen()/getScreens() to observe nil/partial m.screens; compute and
fully initialize the new screens (including running
calculateScreensDipCoordinates) before calling LayoutScreens, and change
LayoutScreens (or its caller contract) to perform an atomic swap of m.screens
only on success (or return an error on failure) so the cache is never replaced
with a partial value; update references to processAndCacheScreens,
LayoutScreens, calculateScreensDipCoordinates, getPrimaryScreen, getScreens and
m.screens accordingly.
In `@v3/pkg/application/webview_window_darwin.go`:
- Around line 1371-1379: The placement code is using options.Screen.WorkArea
directly (when options.Screen != nil) then calling w.setPosition, but on macOS
that WorkArea is in the target NSScreen coordinate space and must be converted
to the window's coordinate space using the darwin-specific helper that
windowSetPosition uses; update the branch that handles options.Screen (and the
WindowCentered branch) to compute the position via the macOS screen-placement
helper (the same conversion windowSetPosition performs) before calling
w.setPosition so scale and Y-flip are calculated from the target screen
(referencing options.Screen.WorkArea, w.setPosition, windowSetPosition and
w.parent.options.InitialPosition).
In `@v3/pkg/application/webview_window.go`:
- Around line 953-971: SetScreen currently performs centering math in the shared
WebviewWindow layer using Screen.WorkArea coordinates and then calls
w.impl.setPosition(x, y); move this platform-specific coordinate translation out
of shared code: remove the centering math from WebviewWindow.SetScreen and
instead call a platform-specific method (e.g., impl.centerOnScreen or
impl.SetScreenWithWorkArea) that accepts the Screen or its WorkArea so each impl
(macOS, Windows, Linux) can convert to its native coordinate system; keep the
nil/impl checks and InvokeSync wrapper in SetScreen but delegate positioning to
the new/updated impl method (referencing WebviewWindow.SetScreen, w.impl,
Screen.WorkArea, and impl.setPosition) so macOS implementation can handle its
coordinate space correctly.
In `@website/src/pages/changelog.mdx`:
- Around line 18-21: The changelog uses incorrect capitalization "MacOS"; update
both occurrences in the diff: change "SetScreen api for MacOS, Windows, Linux"
to "SetScreen api for macOS, Windows, Linux" and change "app.Screen population
on startup in Linux and MacOS" to "app.Screen population on startup in Linux and
macOS" (references: the "SetScreen" API mention and the "app.Screen" startup
fix).
---
Outside diff comments:
In `@v3/pkg/application/linux_purego.go`:
- Around line 771-775: There are two definitions of windowGetPosition which
causes a compile error; remove the earlier duplicate definition that calls
gtkWindowGetPosition (the one at the first occurrence) and keep the later
definition (the one with the TODO) so only the single intended windowGetPosition
implementation remains.
---
Nitpick comments:
In `@v3/pkg/application/webview_window_linux.go`:
- Around line 342-350: Extract the duplicated screen-aware placement logic into
a single helper (e.g., a method on webviewWindow like computeAndSetPosition or
getCenteredPosition) that reads w.parent.options.Screen.WorkArea,
w.parent.options.InitialPosition/WindowCentered, w.parent.options.X/Y and
w.size(), computes the final X/Y, and calls w.setPosition; then replace both
duplicated blocks (the branch before show and the later branch) to call this
helper so the center vs offset math is implemented only once (ensure helper
handles nil Screen by falling back to using options.X/Y).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b32b7aa-2df4-4eda-8fde-3b5e1a42d764
📒 Files selected for processing (19)
v3/internal/runtime/desktop/@wailsio/runtime/src/screens.tsv3/internal/runtime/desktop/@wailsio/runtime/src/window.tsv3/pkg/application/application_darwin.gov3/pkg/application/application_linux.gov3/pkg/application/application_linux_gtk4.gov3/pkg/application/linux_cgo.gov3/pkg/application/linux_purego.gov3/pkg/application/messageprocessor_screens.gov3/pkg/application/messageprocessor_window.gov3/pkg/application/screen_darwin.gov3/pkg/application/screen_linux.gov3/pkg/application/screenmanager.gov3/pkg/application/webview_window.gov3/pkg/application/webview_window_darwin.gov3/pkg/application/webview_window_linux.gov3/pkg/application/webview_window_options.gov3/pkg/application/webview_window_windows.gov3/pkg/application/window.gowebsite/src/pages/changelog.mdx
adds centerOnScreen helper to avoid placement logic in the shared layer
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
1 participant
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.
Description
Fixes initialization of both mac and linux to include
processAndCacheScreensto match windows to properly populateapp.ScreensAdds
SetScreenacross all OS's including helpers and runtime bindings to support setting from JS runtime.Fixes # (issue)
#4000
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Test Configuration
MacOS - Macbook Pro M4 Max
Linux - Arch + Wayland
Summary by CodeRabbit
New Features
Bug Fixes
Improvements