fix incorrect window destroy#4323
Conversation
WalkthroughThe changes remove duplicate macOS event mappings, add debug logging for window lifecycle events on macOS, and introduce a thread-safe atomic flag to control unconditional window closing. A new platform-specific Go function is exposed to Objective-C to determine if a window should close unconditionally, integrating conditional close logic between Go and native layers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Native (Obj-C)
participant App (Go)
User->>Native: Initiate window close
Native->>App (Go): windowShouldUnconditionallyClose(windowId)
App (Go)->>App (Go): Retrieve window, atomically read close flag
App (Go)-->>Native: Return true/false
alt Close unconditionally
Native-->>User: Close window immediately
else User-initiated close
Native->>App (Go): Emit WindowShouldClose event
App (Go)-->>User: Application handles close event
end
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
atterpac
force-pushed
the
v3/mac-window-hide-fix
branch
from
42c9016 to
0434e94
Compare
|
@atterpac @leaanthony I'm trying to apply this fix to resolve a crash during macOS window destroy, but it seems to be stopping the window close button from working at all.
Am I missing some other event handling I need to do to get the window to close when the close button is clicked with this change, or does the window delegate need a corresponding change? UPDATE: |
There was a problem hiding this comment.
@atterpac - If I run the "events" example, none of the windows close.
Interesting, I'll have to take the look I was only testing with the window example |
dosubot
Bot
added
the
size:S
dosubot
Bot
added
size:M
github-actions
Bot
removed
the
size:M
|
Fix Summary for Issue #4251 I've successfully fixed the duplicate symbol compilation issue that was preventing this PR from building. Here's a comprehensive summary of the solution: Problem Addressed This PR fixes issue #4251 where macOS windows would crash the system when using hide/show functionality. The core issue was that window.Close() and user-initiated window closes weren't properly Root Cause The original issue was in the event mapping in v3/pkg/events/defaults.go where Mac.WindowWillClose was incorrectly mapped to Common.WindowClosing. This caused every window hide operation to Solution Components
How It Works For Hide-on-Close Applications:
For Normal Close Operations:
Compilation Fix The original code had 60 duplicate C symbols during linking. Fixed by:
Debug Messages Added I've added comprehensive debug logging for testing purposes: Go Debug Messages:
Objective-C NSLog Messages:
Testing ✅ Compilation: All examples build successfully Example logs for hide-on-close: Next Steps for @atterpac
|
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v3/pkg/application/webview_window_close_darwin.go(1 hunks)v3/pkg/application/webview_window_darwin.go(3 hunks)v3/pkg/application/webview_window_darwin.m(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- v3/pkg/application/webview_window_darwin.go
- v3/pkg/application/webview_window_darwin.m
🧰 Additional context used
🧬 Code Graph Analysis (1)
v3/pkg/application/webview_window_close_darwin.go (1)
v3/pkg/application/webview_window.go (1)
WebviewWindow(136-170)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
dosubot
Bot
added
the
size:M
ABI Safety Fix - Addressed CodeRabbit FeedbackFixed the ABI safety issues identified by @coderabbitai[bot] in the exported function: Changes Made:Function Signature - Now uses proper C types: // Before
func windowShouldUnconditionallyClose(windowId uint) bool
// After
func windowShouldUnconditionallyClose(windowId C.uint) C.boolImplementation Updates:
Why This Matters:
Testing:
This follows CGo best practices and ensures the exported function has a stable, compiler-verified ABI between the Go and Objective-C layers. Thanks @coderabbitai[bot] for the excellent feedback! |
|
Tip For best results, initiate chat on the files or code changes. @leaanthony Awesome – thanks for jumping on that so quickly! I skimmed the latest diff and everything looks consistent: • No further changes required from my side – feel free to proceed with the cleanup of the temporary debug logs whenever you’re ready. Nice work! 🐇✨ |
Address CodeRabbit feedback on ABI safety and data races: - Change exported function signature to use C types (C.uint, C.bool) for ABI safety - Convert unconditionallyClose field from bool to atomic uint32 for thread safety - Update all read/write operations to use atomic operations across platforms 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v3/pkg/application/webview_window.go (1)
169-171: Flag type switched touint32– consideratomic.Uint32wrapperMoving from
booltouint32is mandatory for the low-levelatomicfunctions you’re using.
As a minor readability enhancement you could adopt the Go 1.19+ struct-friendly wrapper:- // unconditionallyClose marks the window to be unconditionally closed (atomic) - unconditionallyClose uint32 + // unconditionallyClose marks the window to be unconditionally closed. + unconditionallyClose atomic.Uint32This eliminates manual addr-of calls (
&w.unconditionallyClose) and makes intent explicit.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v3/pkg/application/webview_window.go(3 hunks)v3/pkg/application/webview_window_close_darwin.go(1 hunks)v3/pkg/application/webview_window_darwin.go(4 hunks)v3/pkg/application/webview_window_windows.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- v3/pkg/application/webview_window_close_darwin.go
- v3/pkg/application/webview_window_darwin.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
🔇 Additional comments (3)
v3/pkg/application/webview_window_windows.go (1)
1132-1136: Thread-safe flag read is correct – 👍Switching to
atomic.LoadUint32removes the data-race that existed when the flag could be mutated from Go while being read inside the window procedure that runs on a different OS thread.
Logic (== 0) still matches the previous boolean semantics, so behaviour is preserved.No further action required.
v3/pkg/application/webview_window.go (2)
10-10: Necessary import added
sync/atomicis now required after the migration touint32+ atomic ops. Import looks good and keeps the set alphabetically sorted.
277-278: Atomic store before re-entering native close path – looks goodUsing
atomic.StoreUint32(&result.unconditionallyClose, 1)guarantees the secondWM_CLOSEpass is recognised as “unconditional”. The store happens synchronously insideInvokeSync, so window-procedure reads have the correct happens-before relation.
|
|
Merging with debug statements for now. |
* fix incorrect window destroy * remove duplicate common signals * Better handling of "shouldclose" * Better handling of "shouldclose" * Fix ABI safety issues in exported function for macOS window closing Address CodeRabbit feedback on ABI safety and data races: - Change exported function signature to use C types (C.uint, C.bool) for ABI safety - Convert unconditionallyClose field from bool to atomic uint32 for thread safety - Update all read/write operations to use atomic operations across platforms 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Lea Anthony <lea.anthony@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
* fix incorrect window destroy * remove duplicate common signals * Better handling of "shouldclose" * Better handling of "shouldclose" * Fix ABI safety issues in exported function for macOS window closing Address CodeRabbit feedback on ABI safety and data races: - Change exported function signature to use C types (C.uint, C.bool) for ABI safety - Convert unconditionallyClose field from bool to atomic uint32 for thread safety - Update all read/write operations to use atomic operations across platforms 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Lea Anthony <lea.anthony@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
3 participants
removes a mapping for WindowShouldClose on Mac, this incorrecty triggers a window.destroy call on every hide()
Summary by CodeRabbit