Skip to content

fix(v3/linux/gtk3): resolve pre-existing bugs in legacy gtk3 path#5466

Merged
leaanthony merged 3 commits into
masterfrom
fix/issue-5465-gtk3-legacy-bugs
May 17, 2026
Merged

fix(v3/linux/gtk3): resolve pre-existing bugs in legacy gtk3 path#5466
leaanthony merged 3 commits into
masterfrom
fix/issue-5465-gtk3-legacy-bugs

Conversation

@leaanthony

@leaanthony leaanthony commented May 16, 2026

Copy link
Copy Markdown
Member

Summary

Fixes the 9 pre-existing bugs catalogued in #5465 — CodeRabbit's review of #5463 surfaced them when the *_linux.go*_gtk3.go rename made the lines visible, but the bugs themselves predate the default-flip and only affect the -tags gtk3 build path.

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. The surrounding else was also restructured so the "not a URL, potential file open?" debug log no longer fires after a successful file-association match. (applicationEvents is buffered cap 5, so sending the event before appRun won't block.)
  • getTheme(): guard body[2] with len(body) >= 3 and use comma-ok type assertions for dbus.Variant and the inner string. Previously a malformed signal would panic.

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; the caller must not free it. C.GoString already copies into Go memory.
  • clipboardGet(): nil-check the gchar* and g_free it after copying. Previously leaked on every call.
  • Calloc.String / Calloc.Free: change to pointer receivers. With value receivers append(c.pool, ...) mutated a copy, so the original pool stayed empty and Free() was a no-op — every c.String(...) allocation leaked.
  • zoomIn / zoomOut: drop the -1.10 multiplier on zoomOut (which setZoom then clamped to 1.0) and use the reciprocal of zoomInFactor. Promoted the factor to a package-level const and removed the stale // FIXME comment.
  • execJS: reuse the existing 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, so hiding a menu item showed it and vice versa.
  • setAccelerator: drop the leftover fmt.Println("setAccelerator", accelerator) development log.

Scope notes

Build verification

Verified with gofmt -d only — I'm on macOS and can't fully go build the -tags gtk3 cgo path locally. Worth a Linux CI pass before merge.

Refs #5465. CodeRabbit's full review: #5463 (review)

Test plan

  • Build with -tags gtk3 on Linux (CI)
  • Smoke-test launching a gtk3 build with a registered file association — confirm the app actually starts (window visible, startup handlers fired) instead of exiting after emitting the open-with-file event
  • Smoke-test gtk3 build: zoomOut now shrinks instead of clamping to 1.0
  • Smoke-test gtk3 build: menu item Hidden = true actually hides
  • Confirm clipboard read in a gtk3 build still returns the expected text (and no leak under repeated reads — visible via valgrind or pmap delta if anyone wants to be thorough)

Summary by CodeRabbit

  • Bug Fixes
    • Improved Linux startup so single-file arguments are correctly treated as file-open only when matching configured associations.
    • Stricter handling of system theme updates to avoid spurious changes.
    • Multiple memory-leak and allocation-tracking fixes to reduce native leaks and ensure per-window allocations are freed.
    • Safer clipboard handling when no text is present.
    • Corrected zoom scaling math.
    • Reduced unnecessary JS-evaluation allocations and removed stray debug output.

Review Change Stack

Copilot AI review requested due to automatic review settings May 16, 2026 07:49
@coderabbitai

coderabbitai Bot commented May 16, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: fbc5de6d-1bdf-496d-bc51-fbf01f3fd120

📥 Commits

Reviewing files that changed from the base of the PR and between 59e7b99 and 928e709.

📒 Files selected for processing (5)
  • v3/UNRELEASED_CHANGELOG.md
  • v3/pkg/application/application_linux_gtk3.go
  • v3/pkg/application/linux_cgo.go
  • v3/pkg/application/linux_cgo_gtk3.go
  • v3/pkg/application/menuitem_linux_gtk3.go
💤 Files with no reviewable changes (1)
  • v3/pkg/application/menuitem_linux_gtk3.go
✅ Files skipped from review due to trivial changes (1)
  • v3/UNRELEASED_CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • v3/pkg/application/application_linux_gtk3.go
  • v3/pkg/application/linux_cgo_gtk3.go

Walkthrough

Multiple 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.

Changes

GTK3 Application and Widget Fixes

Layer / File(s) Summary
Startup file matching and theme D-Bus parsing
v3/pkg/application/application_linux_gtk3.go
Startup now matches single arguments against configured file extensions to emit "opened with file" events; unmatched single args log a debug message. Theme-change parsing validates minimum body length, exact interface name, color-scheme key position, and that the value is a dbus.Variant convertible to string before accepting a theme update.
Core linux_cgo allocator pointer semantics
v3/pkg/application/linux_cgo.go
NewCalloc() now returns *Calloc and Calloc.String/Calloc.Free use pointer receivers so the internal pool is shared and can be cleared by Free (pool set to nil).
GTK3 C allocator, appName, clipboard, execJS, zoom
v3/pkg/application/linux_cgo_gtk3.go
GTK3 variant of NewCalloc()*Calloc and pointer receivers; appName() no longer frees GLib-owned pointer; clipboardGet() returns "" on null and defers g_free after conversion; execJS reuses a shared emptyWorldName C string; zoomOut() multiplies by 1.0/zoomInFactor.
Menu accelerator cleanup
v3/pkg/application/menuitem_linux_gtk3.go
Removed stray fmt.Println debug output in setAccelerator(); conditional placeholder remains.
Changelog documentation
v3/UNRELEASED_CHANGELOG.md
Adds expanded "Fixed" bullets documenting GTK3 fixes and the Calloc allocator change (also noted for the default GTK4 linux_cgo.go).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Suggested labels

Bug, Linux, go, v3, size:L

Suggested reviewers

  • atterpac

Poem

🐰 I hopped through GTK's tangled trees,
freed stray strings with nimble ease,
matched the files and fixed the zoom,
kept appName safe from free's doom—
a tiny hop, a cleaner breeze.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main focus: fixing pre-existing bugs in the legacy gtk3 build path, which aligns with all file changes in the changeset.
Description check ✅ Passed The description is comprehensive and well-structured. It clearly describes the issues fixed, references the related issue #5465, specifies the type of change (bug fix), provides scope notes, build verification details, and a test plan. All major template requirements are adequately addressed.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/issue-5465-gtk3-legacy-bugs

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 early return nil after ApplicationOpenedWithFile so startup handlers/event setup/main loop still run, and harden getTheme against malformed dbus signals (length + comma-ok type assertions).
  • linux_cgo_gtk3.go: stop freeing GLib-owned g_get_application_name storage; nil-check and g_free the gchar* from gtk_clipboard_wait_for_text; convert Calloc.String/Free to pointer receivers; replace negative zoomOut factor with reciprocal of a hoisted zoomInFactor const; reuse the shared emptyWorldName C string in execJS.
  • menuitem_linux_gtk3.go: change setHidden to pass !hidden to widgetSetVisible, remove a dev fmt.Println from setAccelerator.

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.

Comment thread v3/pkg/application/menuitem_linux_gtk3.go Outdated
Comment thread v3/pkg/application/linux_cgo_gtk3.go Outdated
Comment thread v3/UNRELEASED_CHANGELOG.md Outdated
leaanthony added a commit that referenced this pull request May 16, 2026
- 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).
leaanthony added a commit that referenced this pull request May 16, 2026
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).
@leaanthony leaanthony force-pushed the fix/issue-5465-gtk3-legacy-bugs branch from 59e7b99 to 928e709 Compare May 17, 2026 03:47
@leaanthony leaanthony merged commit 0872e5a into master May 17, 2026
58 of 59 checks passed
@leaanthony leaanthony deleted the fix/issue-5465-gtk3-legacy-bugs branch May 17, 2026 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants