Feat/macos non activating panel - #5360
phoenixsheppard28 wants to merge 5 commits into
Conversation
WalkthroughAdds a macOS NonActivatingPanel option: WebviewWindow becomes an NSPanel, lifecycle functions explicitly release Objective‑C objects and clear Go pointers, the NonActivatingPanel flag is wired through creation, and manual tests and docs are added. ChangesmacOS NonActivatingPanel Window Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/webview_window_darwin.go (1)
1630-1638:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
destroy()is missing theunconditionallyCloseflag, risking[w release]on an AppKit-open window.
close()setsatomic.StoreUint32(&w.parent.unconditionallyClose, 1)before callingC.windowClose, so the delegate'swindowShouldClose:returnsYESand[w close]completes cleanly.destroy()makes no such guarantee. When called without a priorclose()(e.g.,Window.Destroy(), or app-quit where windows were never individually closed),C.windowDestroycalls[w close]on an unconditional-flag-unset window: the delegate firesEventWindowShouldCloseand returnsNO, abandoning the close sequence.[w release]then executes unconditionally, freeing the Obj-C object while AppKit still considers the window open — a use-after-free.🐛 Proposed fix
func (w *macosWebviewWindow) destroy() { w.parent.markAsDestroyed() clearWindowDragCache(w.parent.id) if w.nsWindow != nil { + // Mirror close(): ensure windowShouldClose: returns YES so [w close] + // completes the close sequence before [w release] in windowDestroy. + atomic.StoreUint32(&w.parent.unconditionallyClose, 1) C.windowDestroy(w.nsWindow) w.nsWindow = nil } }🤖 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_darwin.go` around lines 1630 - 1638, The destroy() path can call C.windowDestroy which may trigger Obj-C [w close] while the parent.unconditionallyClose flag is not set, causing the delegate to cancel the close and later release the window leading to use-after-free; modify macosWebviewWindow.destroy to set atomic.StoreUint32(&w.parent.unconditionallyClose, 1) (same flag used in close()) before invoking C.windowDestroy so the delegate's windowShouldClose: returns YES and the close/destroy sequence completes safely; reference functions/fields: macosWebviewWindow.destroy, macosWebviewWindow.close, parent.unconditionallyClose, C.windowDestroy, C.windowClose, and the windowShouldClose/EventWindowShouldClose delegate behavior to mirror close()'s ordering.
🧹 Nitpick comments (1)
v3/pkg/application/webview_window_darwin.go (1)
706-731: 💤 Low value
windowDestroyand staticwindowCloseare now functionally identical.Both functions execute
[w close]; [w release]. If the implementations are intended to remain in sync, consider factoring the shared body into an inline helper or adding a comment linking them. This prevents a future divergence where one is updated but the other isn't.🤖 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_darwin.go` around lines 706 - 731, windowDestroy and the static function windowClose both perform identical actions ([w close]; [w release]) on a WebviewWindow, risking divergence; refactor by extracting the shared behavior into a single helper (e.g., windowCloseHelper or inline function) and have both windowDestroy and windowClose call that helper, or implement windowClose to simply call windowDestroy with the same WebviewWindow* parameter to keep them in sync (refer to windowDestroy, windowClose, and WebviewWindow).
🤖 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.
Outside diff comments:
In `@v3/pkg/application/webview_window_darwin.go`:
- Around line 1630-1638: The destroy() path can call C.windowDestroy which may
trigger Obj-C [w close] while the parent.unconditionallyClose flag is not set,
causing the delegate to cancel the close and later release the window leading to
use-after-free; modify macosWebviewWindow.destroy to set
atomic.StoreUint32(&w.parent.unconditionallyClose, 1) (same flag used in
close()) before invoking C.windowDestroy so the delegate's windowShouldClose:
returns YES and the close/destroy sequence completes safely; reference
functions/fields: macosWebviewWindow.destroy, macosWebviewWindow.close,
parent.unconditionallyClose, C.windowDestroy, C.windowClose, and the
windowShouldClose/EventWindowShouldClose delegate behavior to mirror close()'s
ordering.
---
Nitpick comments:
In `@v3/pkg/application/webview_window_darwin.go`:
- Around line 706-731: windowDestroy and the static function windowClose both
perform identical actions ([w close]; [w release]) on a WebviewWindow, risking
divergence; refactor by extracting the shared behavior into a single helper
(e.g., windowCloseHelper or inline function) and have both windowDestroy and
windowClose call that helper, or implement windowClose to simply call
windowDestroy with the same WebviewWindow* parameter to keep them in sync (refer
to windowDestroy, windowClose, and WebviewWindow).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 872406b9-c101-4a4d-bd09-bc8b288e44de
📒 Files selected for processing (6)
v3/pkg/application/webview_window_darwin.gov3/pkg/application/webview_window_darwin.hv3/pkg/application/webview_window_darwin.mv3/pkg/application/webview_window_options.gov3/test/manual/macos/README.mdv3/test/manual/macos/non-activating-panel/main.go
|
This is a significant new feature adding macOS non-activating panel support. The implementation looks comprehensive with good documentation and manual tests, but I'd like a maintainer to review the NSPanel integration and window lifecycle changes before proceeding. CC @leaanthony |
|
Thank you for the detailed implementation and test plan, @phoenixsheppard28. Your analysis of non-activating We have consolidated the work in #6008. It keeps existing windows backed by To avoid maintaining two overlapping implementations, I am closing this PR as superseded by #6008. Your contribution is credited there, and a review of the successor would be very welcome. Thank you for moving the non-activating-panel behavior and lifecycle testing forward. |
2 participants
Description
This PR adds macOS support for non-activating panels (Swift’s
nonactivatingPanel–style behaviour): optionalNSWindowStyleMaskNonactivatingPanelon Wails webview windows so users can build floating palettes, Spotlight-style overlays, and menu-bar detail windows without stealing activation from the app that currently has focus.Summary of changes:
MacWindow.NonActivatingPanel(webview_window_options.go) — opt-in flag with GoDoc that separates applicationOptions.Mac/MacOptions.ActivationPolicyfrom per-windowWebviewWindowOptions.Mac(MacWindow) settings.WebviewWindowsubclassesNSPanel(webview_window_darwin.h) so the non-activating style mask is honoured by AppKit.windowNewORsNSWindowStyleMaskNonactivatingPanelwhen the option is set;canBecomeMainWindowreturnsNOwhen that mask is present (panel stays key-capable for text input but not main).windowFocusskipsactivateIgnoringOtherApps:when the non-activating mask is set so “focus window” does not force app activation.hidesOnDeactivateforced toNOso windows do not disappear on app switch;releasedWhenClosedstaysNOby design.releasedWhenClosed == NO,windowCloseandwindowDestroynow[release]after[close]to balancewindowNew’salloc; Go nil-snsWindowafter teardown to avoid dangling pointers.v3/test/manual/macos/non-activating-panel/plusv3/test/manual/macos/README.mdwith a reproducible checklist.Pairings users will typically use (documented on the field):
Mac.WindowLevel,Mac.CollectionBehavior,Frameless/Mac.Backdrop; dock-hidden / accessory behaviour remainsOptions.Mac.ActivationPolicy(MacOptions).Fixes #5359
Type of change
Please select the option that is relevant.
(Bug-fix aspects: NSPanel
hidesOnDeactivateregression, ObjC window lifetime / release after close whenreleasedWhenClosedis NO.)How Has This Been Tested?
macOS
go build ./pkg/application/andgo vet ./pkg/application/fromv3/.cd v3/test/manual/macos/non-activating-panel && go run .— followv3/test/manual/macos/README.md(non-activating vs normal control window,hidesOnDeactivate, focus/activation,canBecomeMainWindow, text input, close/re-run).Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PR (v3 changelog entries are added automatically)Summary by CodeRabbit
New Features
Improvements
Documentation
Tests