fix(v3/linux/gtk3): resolve pre-existing bugs in legacy gtk3 path#5466
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)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughMultiple GTK3 fixes: startup single-argument file matching, stricter D‑Bus theme parsing, Calloc allocator switched to pointer semantics (GTK3 and default linux_cgo), GLib-owned appName handling, clipboard and JS CString leak fixes, corrected zoom math, menu accelerator debug removal, and changelog updates. ChangesGTK3 Application and Widget Fixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 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.2)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 a cluster of nine pre-existing bugs in the legacy -tags gtk3 Linux build path (catalogued in #5465 after the file rename in #5463 made them visible to review tooling). Changes touch the application/run path, the cgo wrapper, and the GTK3 menu item implementation.
Changes:
application_linux_gtk3.go: drop earlyreturn nilafterApplicationOpenedWithFileso startup handlers/event setup/main loop still run, and hardengetThemeagainst malformed dbus signals (length + comma-ok type assertions).linux_cgo_gtk3.go: stop freeing GLib-ownedg_get_application_namestorage; nil-check andg_freethegchar*fromgtk_clipboard_wait_for_text; convertCalloc.String/Freeto pointer receivers; replace negativezoomOutfactor with reciprocal of a hoistedzoomInFactorconst; reuse the sharedemptyWorldNameC string inexecJS.menuitem_linux_gtk3.go: changesetHiddento pass!hiddentowidgetSetVisible, remove a devfmt.PrintlnfromsetAccelerator.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| v3/UNRELEASED_CHANGELOG.md | Describes the bundle of GTK3 fixes shipped here. |
| v3/pkg/application/application_linux_gtk3.go | Fixes early-return on file-association launch and hardens getTheme parsing. |
| v3/pkg/application/linux_cgo_gtk3.go | Multiple memory-correctness, leak, and zoom math fixes in the GTK3 cgo bridge. |
| v3/pkg/application/menuitem_linux_gtk3.go | Inverts setHidden argument and removes dev log in setAccelerator. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Revert setHidden in menuitem_linux_gtk3.go: widgetSetVisible takes a `hidden` flag (true → gtk_widget_hide, false → gtk_widget_show) — the same convention as the GTK4 (linux_cgo.go) and purego (linux_purego.go) implementations. The original `widgetSetVisible(l.native, hidden)` was correct; the negation introduced in the previous commit inverted behaviour. Bug-bash item #8 in #5465 misread the function name. - Make NewCalloc return *Calloc to match the pointer-receiver String/Free so the API is consistently pointer-semantic and chained calls like `NewCalloc().String(...)` continue to compile. Existing call sites (`c := NewCalloc(); defer c.Free()`) are unchanged. - Update UNRELEASED_CHANGELOG.md accordingly: drop the (incorrect) setHidden line, note the NewCalloc return-type change. Refs #5466 review (Copilot).
Mirror the same Calloc fix from linux_cgo_gtk3.go in linux_cgo.go (the now-default GTK4 build). Without pointer receivers, `append(c.pool, ...)` in `String` mutated a copy, so the pool stayed empty and `Free()` was a no-op — every per-window `c.String(...)` allocation in `setupSignalHandlers` and `windowNewWebview` leaked. With value receivers gone, allocations are tracked and released on Free. `NewCalloc` now returns `*Calloc` so the API is consistently pointer-semantic across both build tags. Refs #5466 review (Copilot).
Cluster of pre-existing bugs in the legacy GTK3 implementation files (now `*_gtk3.go` after #5463). Surfaced by CodeRabbit on #5463; the file rename made them visible but the bugs themselves predate the default-flip. All affect the `-tags gtk3` build path only. application_linux_gtk3.go - run(): drop the early `return nil` on the file-association branch so startup handlers, common event setup, theme/power monitoring, and the GTK main loop still run after `ApplicationOpenedWithFile` is emitted. Restructure the surrounding `else` so the "not a URL, potential file open?" debug log no longer fires after a successful file-association match. - getTheme(): guard `body[2]` access with `len(body) >= 3` and use comma-ok type assertions for `dbus.Variant` and the inner `string`, returning `("", false)` on any mismatch instead of panicking. linux_cgo_gtk3.go - appName(): drop the `defer C.free` on the pointer returned by `g_get_application_name()`. That memory belongs to GLib internal storage; `C.GoString` already copies into Go memory. - clipboardGet(): nil-check the `gchar*` returned by `gtk_clipboard_wait_for_text` and `g_free` it after copying into a Go string. Previously leaked on every call. - Calloc.String / Calloc.Free: change to pointer receivers so the `append(c.pool, ...)` actually grows the original pool. With value receivers the pool stayed empty and Free was a no-op, leaking every `c.String(...)` allocation. - zoomIn / zoomOut: drop the negative `-1.10` multiplier (which `setZoom` then clamped to 1.0) and use the reciprocal of `zoomInFactor` for zoomOut. Promoted the factor to a package-level const and removed the stale "FIXME: assumed to be incorrect" comment. - execJS: reuse the file-level `emptyWorldName` C string instead of allocating a fresh `C.CString("")` per call that was never freed. menuitem_linux_gtk3.go - setHidden: pass `!hidden` to `widgetSetVisible` (previously inverted; hiding a menu item showed it and vice versa). - setAccelerator: drop the development `fmt.Println("setAccelerator", ...)`. The identical `Calloc` value-receiver bug also exists verbatim in `linux_cgo.go` (the now-default GTK4 path); intentionally left for a follow-up to keep this PR scoped to the gtk3 tracking issue. Refs #5465
- Revert setHidden in menuitem_linux_gtk3.go: widgetSetVisible takes a `hidden` flag (true → gtk_widget_hide, false → gtk_widget_show) — the same convention as the GTK4 (linux_cgo.go) and purego (linux_purego.go) implementations. The original `widgetSetVisible(l.native, hidden)` was correct; the negation introduced in the previous commit inverted behaviour. Bug-bash item #8 in #5465 misread the function name. - Make NewCalloc return *Calloc to match the pointer-receiver String/Free so the API is consistently pointer-semantic and chained calls like `NewCalloc().String(...)` continue to compile. Existing call sites (`c := NewCalloc(); defer c.Free()`) are unchanged. - Update UNRELEASED_CHANGELOG.md accordingly: drop the (incorrect) setHidden line, note the NewCalloc return-type change. Refs #5466 review (Copilot).
Mirror the same Calloc fix from linux_cgo_gtk3.go in linux_cgo.go (the now-default GTK4 build). Without pointer receivers, `append(c.pool, ...)` in `String` mutated a copy, so the pool stayed empty and `Free()` was a no-op — every per-window `c.String(...)` allocation in `setupSignalHandlers` and `windowNewWebview` leaked. With value receivers gone, allocations are tracked and released on Free. `NewCalloc` now returns `*Calloc` so the API is consistently pointer-semantic across both build tags. Refs #5466 review (Copilot).
59e7b99 to
928e709
Compare
Summary
Fixes the 9 pre-existing bugs catalogued in #5465 — CodeRabbit's review of #5463 surfaced them when the
*_linux.go→*_gtk3.gorename made the lines visible, but the bugs themselves predate the default-flip and only affect the-tags gtk3build path.application_linux_gtk3.gorun(): drop the earlyreturn nilon the file-association branch so startup handlers, common event setup, theme/power monitoring, and the GTK main loop still run afterApplicationOpenedWithFileis emitted. The surroundingelsewas also restructured so the "not a URL, potential file open?" debug log no longer fires after a successful file-association match. (applicationEventsis buffered cap 5, so sending the event beforeappRunwon't block.)getTheme(): guardbody[2]withlen(body) >= 3and use comma-ok type assertions fordbus.Variantand the innerstring. Previously a malformed signal would panic.linux_cgo_gtk3.goappName(): drop thedefer C.freeon the pointer returned byg_get_application_name()— that memory belongs to GLib internal storage; the caller must not free it.C.GoStringalready copies into Go memory.clipboardGet(): nil-check thegchar*andg_freeit after copying. Previously leaked on every call.Calloc.String/Calloc.Free: change to pointer receivers. With value receiversappend(c.pool, ...)mutated a copy, so the original pool stayed empty andFree()was a no-op — everyc.String(...)allocation leaked.zoomIn/zoomOut: drop the-1.10multiplier on zoomOut (whichsetZoomthen clamped to 1.0) and use the reciprocal ofzoomInFactor. Promoted the factor to a package-level const and removed the stale// FIXMEcomment.execJS: reuse the existing file-levelemptyWorldNameC string instead of allocating a freshC.CString("")per call that was never freed.menuitem_linux_gtk3.gosetHidden: pass!hiddentowidgetSetVisible— previously inverted, so hiding a menu item showed it and vice versa.setAccelerator: drop the leftoverfmt.Println("setAccelerator", accelerator)development log.Scope notes
Callocvalue-receiver bug also exists verbatim inlinux_cgo.go(the now-default GTK4 path). Intentionally left out of this PR to keep it scoped to the gtk3 tracking issue; happy to fold it in here or do a follow-up — your call.processMenuprocessedguard, tracked separately as [v3] Menu.Update() is a no-op after first call due to processed guard in processMenu (Linux) #5464 since it affects both stacks.Build verification
Verified with
gofmt -donly — I'm on macOS and can't fullygo buildthe-tags gtk3cgo path locally. Worth a Linux CI pass before merge.Refs #5465. CodeRabbit's full review: #5463 (review)
Test plan
-tags gtk3on Linux (CI)Hidden = trueactually hidesSummary by CodeRabbit