fix(linux): split dialog dispatch between GTK3 and GTK4#5340
Conversation
|
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 (5)
WalkthroughIntroduces a ChangesGTK Dispatch Abstraction & Migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. 🔧 golangci-lint (2.12.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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.
Pull request overview
This PR fixes Linux dialog dispatching by separating GTK3 and GTK4 implementations to avoid GTK3 thread-safety crashes while also preventing GTK4 deadlocks caused by nesting InvokeAsync around a GTK4 runQuestionDialog that already schedules onto the main loop and then blocks for a response.
Changes:
- Add a
!gtk4build constraint to the existing Linux dialogs implementation so GTK3 (and purego) keep usingInvokeAsyncfor main-thread marshaling. - Introduce a GTK4-specific Linux dialogs implementation that dispatches message/about dialogs via a goroutine instead of
InvokeAsync, avoiding main-loop deadlock with GTK4’s channel-waiting dialog flow.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| v3/pkg/application/dialogs_linux.go | Adds !gtk4 build constraint to ensure GTK3 continues using main-thread dispatch via InvokeAsync. |
| v3/pkg/application/dialogs_linux_gtk4.go | New GTK4-only dialogs implementation using goroutines for message/about dialog dispatch to avoid deadlocks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v3/pkg/application/dialogs_linux.go (1)
61-87: ⚡ Quick winExtract identical file-dialog implementations to a shared file.
linuxOpenFileDialog,linuxSaveFileDialog, their constructors, andshow()methods are byte-for-byte identical in bothdialogs_linux.goanddialogs_linux_gtk4.go. They contain no GTK-version-specific logic (both delegate torunOpenFileDialog/runSaveFileDialog). A newdialogs_linux_filedialogs.goguarded by//go:build linux && !android && !serverwould be the single source of truth and eliminate the risk of future drift.♻️ Proposed extraction — `dialogs_linux_filedialogs.go`
// new file: v3/pkg/application/dialogs_linux_filedialogs.go +//go:build linux && !android && !server + +package application + +type linuxOpenFileDialog struct { + dialog *OpenFileDialogStruct +} + +func newOpenFileDialogImpl(d *OpenFileDialogStruct) *linuxOpenFileDialog { + return &linuxOpenFileDialog{dialog: d} +} + +func (m *linuxOpenFileDialog) show() (chan string, error) { + return runOpenFileDialog(m.dialog) +} + +type linuxSaveFileDialog struct { + dialog *SaveFileDialogStruct +} + +func newSaveFileDialogImpl(d *SaveFileDialogStruct) *linuxSaveFileDialog { + return &linuxSaveFileDialog{dialog: d} +} + +func (m *linuxSaveFileDialog) show() (chan string, error) { + return runSaveFileDialog(m.dialog) +}Then remove Lines 61–87 from
dialogs_linux.goand the equivalent block fromdialogs_linux_gtk4.go.🤖 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/dialogs_linux.go` around lines 61 - 87, Create a new file guarded with the build tag "//go:build linux && !android && !server" (suggested name dialogs_linux_filedialogs.go) and move the identical types and methods there: linuxOpenFileDialog, linuxSaveFileDialog, newOpenFileDialogImpl, newSaveFileDialogImpl and their show() methods; keep the show() implementations delegating to runOpenFileDialog and runSaveFileDialog, respectively, then remove the duplicate blocks from dialogs_linux.go and dialogs_linux_gtk4.go so the new file is the single source of truth.
🤖 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/dialogs_linux.go`:
- Around line 61-87: Create a new file guarded with the build tag "//go:build
linux && !android && !server" (suggested name dialogs_linux_filedialogs.go) and
move the identical types and methods there: linuxOpenFileDialog,
linuxSaveFileDialog, newOpenFileDialogImpl, newSaveFileDialogImpl and their
show() methods; keep the show() implementations delegating to runOpenFileDialog
and runSaveFileDialog, respectively, then remove the duplicate blocks from
dialogs_linux.go and dialogs_linux_gtk4.go so the new file is the single source
of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af69b210-fad4-4cca-9be0-5e6f54e88cea
📒 Files selected for processing (2)
v3/pkg/application/dialogs_linux.gov3/pkg/application/dialogs_linux_gtk4.go
GTK3 requires all GTK calls on the main thread; GTK4 is thread-safe. Add two tiny build-tag-gated files: - gtkdispatch_linux.go (!gtk4): gtkDispatch = InvokeAsync - gtkdispatch_linux_gtk4.go (gtk4): gtkDispatch = goroutine Replace all shared-file InvokeAsync dispatch calls with gtkDispatch so a single implementation lives in each shared file: - dialogs_linux.go (showAboutDialog, linuxDialog.show) - webview_window_linux.go (JS/CSS injection on load) - systemtray_linux.go (tray menu item click dispatch) InvokeAsync calls inside gtk3/gtk4-specific functions (linux_cgo.go, linux_cgo_gtk4.go) are internal to those functions and unchanged. The GTK4 goroutine approach avoids deadlock: runQuestionDialog already uses InvokeAsync internally and blocks on a result channel, so calling it from an outer InvokeAsync (main thread) would stall the GTK loop. Fixes #5338 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
26a5231 to
57b1f69
Compare
|
Investigating build and static verification on Linux (Ubuntu 24.04, GTK 3.24.52 / GTK 4.22.2 / WebKit 2.52.0). Build results:
Unit tests: Static checks:
Pre-existing issues (not introduced by this PR):
Not yet verified: Interactive GTK4 dialog runtime test (deadlock absence). Awaiting a live GTK4 session for confirmation. |
…plit dialog dispatch between GTK3 and GTK4
Summary
dialogs_linux.gogains a!gtk4build constraint — GTK3 (and purego) continue to useInvokeAsyncto marshal dialog calls onto the GTK main thread, which is required because GTK3 is not thread-safe.dialogs_linux_gtk4.go(build taggtk4) replacesInvokeAsyncwith a plain goroutine for bothshowAboutDialogandlinuxDialog.show().Why
GTK4's
runQuestionDialogalready schedules the dialog display via its ownInvokeAsyncand then blocks on a Go channel waiting for the user's button callback. If called from within an outerInvokeAsync(i.e., from the main GTK thread), the channel block stalls the main loop, preventing the dialog callback from ever firing → deadlock.GTK4 is thread-safe, so the goroutine approach is correct:
runQuestionDialogcan be called from any thread, its internalInvokeAsyncqueues the dialog display on the main thread without blocking it, and the goroutine waits for the result channel while the main loop stays free.Test plan
go build ./...on Linux without-tags gtk4):dialogs_linux.gocompiles,showAboutDialogand message dialogs still work.go build -tags gtk4 ./...):dialogs_linux_gtk4.gocompiles,showAboutDialogand message dialogs no longer deadlock.linuxOpenFileDialog/linuxSaveFileDialogimpls are identical; they delegate torunChooserDialogwhich already handles its own threading).Fixes #5338
🤖 Generated with Claude Code
Summary by CodeRabbit