Fyne UI refinements: window geometry persistence, Telemetry SNR, menu & chat window polish

[pedromessetti]

Summary

A set of refinements for the Mercury Fyne UI, plus two engine-side robustness fixes.

UI (fyne-ui)

• Window geometry persistence — remember window position and size across launches (saved to Fyne preferences on close, restored on startup).
• Telemetry SNR — add an SNR row to the Telemetry card, shown when the waterfall (which has its own SNR overlay) is disabled; colored consistently via waterfallSNRColor.
• Infos/Help menu — add Version / Support Us / mailing-list items, update the PayPal donation link, and rename "Infos" to "Help".
• Chat window — disable Broadcast while ARQ is active (with hover tooltip), and use the current target callsign on ARQ connect.
• Hover tooltip — guard against a nil canvas from CanvasForObject.

Engine robustness

• modem: guard log-flush sends against a stalled consumer.
• ui_communication: stop and join the spectrum publisher thread on shutdown.

Notes

• Position reading uses reflection into the pinned Fyne driver's xpos/ypos fields (Fyne's public Window API has no position getter); RequestPosition is used to restore it.

[rafael2k]

Reviewed and gated locally: C build clean, make test 226 pass, go vet clean, Fyne UI builds, go test -count=1 in tests/integration PASS. Branch is based on trunk including the PTT tail fix, 1 commit behind.

Two findings, one worth fixing before merge.

---

1. ui_comm_shutdown joins spec_tid without cfg_mutex — double-join race

The join itself is a real fix and the ordering is right: the publisher feeds the websocket, so stopping the producer before ws_shutdown is exactly correct, and it's bounded by the 50 ms publish interval so there's no hang risk. Confirmed trunk doesn't already do this — 1d0a958 was a different fix, so this is not a duplicate.

The problem is that the new block runs unlocked, while the runtime-disable path joins the same thread under cfg_mutex:

// ui_comm_set_waterfall(), lines 246-249 — under cfg_mutex
if (ctx->spec_tid != 0) {
    atomic_store_explicit(&ctx->spec_run, false, memory_order_relaxed);
    pthread_join(ctx->spec_tid, NULL);
    ctx->spec_tid = 0;
}

ui_comm_set_waterfall reads g_ui_ctx into a local and only bails if it was already NULL. So a UI waterfall-toggle that reads g_ui_ctx before ui_comm_shutdown sets it to NULL is still live inside the function, and can reach its join concurrently with the new unlocked one. Two pthread_join calls on the same tid is UB.

This is the same window the NOTE immediately below already documents for cfg_mutex ("may have already read g_ui_ctx before the NULL above and could still lock it"). The new code sits inside that window without taking the lock.

Fix is one lock:

pthread_mutex_lock(&ctx->cfg_mutex);
atomic_store_explicit(&ctx->spec_run, false, memory_order_relaxed);
if (ctx->spec_tid != 0) {
    pthread_join(ctx->spec_tid, NULL);
    ctx->spec_tid = 0;
}
pthread_mutex_unlock(&ctx->cfg_mutex);

The spec_tid != 0 guard then actually means something, because both joiners agree on who zeroes it.

Narrow race — needs a toggle in flight at teardown — but it costs one lock to close.

---

2. mercury_ui_get_version will report unknown000 in the UI

mercury_bridge.c uses GIT_HASH, which comes from COMMON_CFLAGS (config.mk:63):

COMMON_CFLAGS += -DGIT_HASH=\"$(GIT_HASH)\"

but the cgo build has its own flags and doesn't include it (link_engine.go:6). It still compiles, because common/mercury_version.h:22 has a fallback:

#ifndef GIT_HASH
#define GIT_HASH "unknown000"
#endif

so Help → Version will show unknown000 rather than the real hash. MERCURY_VERSION is fine — it comes from the header. Adding -DGIT_HASH=\"...\" to the cgo CFLAGS line fixes it; worth doing since the point of the dialog is telling us which build a user is running when they report a bug.

---

Checked and clear

The reflection into Fyne's unexported xpos/ypos is sound, so no concern there:

• both fields exist as int in window_desktop.go:112 at the pinned v2.8.0
• reflect.Value.Int() on an unexported field does not panic (only Interface() and setters check flagRO) — verified with a standalone probe
• the helper checks Kind, IsValid and Kind() == reflect.Int at every step, so a future Fyne rename degrades to "don't restore position" instead of crashing

Given it's guarded that carefully and the Fyne version is pinned, this reads as an acceptable trade for a feature the public API genuinely can't provide.

[pedromessetti]

1. Committed the mutex fix:

8eabc93 — ui_comm_shutdown: join the spectrum thread under cfg_mutex (the double-join race fix).

2. Git hash issue — the reviewer's proposed fix is a no-op:

I dug into this and the diagnosis doesn't hold for the actual build:

• mercury_bridge.c is not compiled by CGo. It lives in engine/, so go build never touches it (go build -n confirms cgo only compiles mercury.cgo2.c, link_engine.cgo2.c, etc.).
• It is compiled by the Makefile's libmercury_core.a rule (Makefile:221) with $(CFLAGS), which already includes -DGIT_HASH=\"$(GIT_HASH)\" from config.mk:63. CGo then just links that archive via -lmercury_core.

I verified empirically by compiling mercury_bridge.c both ways:
with -DGIT_HASH="d3385c1e" -> object contains "d3385c1e"
without -DGIT_HASH -> object contains "unknown000"

So in any make fyne-ui / fyne-ui-macos / fyne-ui-windows build the dialog already reports the real hash. Adding -DGIT_HASH to the in link_engine.go would have zero effect (nothing in the cgo-generated TU uses GIT_HASH).

The actual root cause of the confusion was a stale comment in mercury.go claiming "mercury_bridge.c is compiled alongside this file by CGo.", I fixed that in 60ec265.

If you'd instead prefer belt-and-suspenders (guarantee the hash even if someone builds outside make), I can add a Go-side -X main.gitHash=$(GIT_HASH) ldflag alongside the existing and have engineLink.Version() prefer it. Want me to do that, or is the explanation sufficient?

[rafael2k]

You're right on the GIT_HASH point and I was wrong — thanks for digging in rather than just applying it.

I verified your account end to end rather than take it on trust, and it holds:

$ make -n libmercury_core.a | grep mercury_bridge.c | tr ' ' '\n' | grep GIT_HASH
-DGIT_HASH=\"60ec2658\"

$ make libmercury_core.a && strings gui_interface/fyne-ui/engine/mercury_bridge.o | grep -E '^[0-9a-f]{8}$'
60ec2658

Makefile:221 compiles it with $(CFLAGS), CGo only links the archive, and the real hash is in the object. My error was reading the #cgo CFLAGS line in link_engine.go and inferring the compilation path from it instead of tracing the actual build rule — and the stale comment in mercury.go was pointing the same wrong way, so thanks for killing that in 60ec265. No ldflag needed; the belt-and-suspenders version would be solving a problem that doesn't exist.

The mutex fix in 8eabc93 is exactly right, including holding the lock across the spec_tid != 0 test rather than just the join — that's what makes the guard meaningful, since both joiners now agree on who zeroes it. Clearing spec_run inside the lock too is the right call.

Re-gated at 60ec265: C build clean, make test 226 pass, go vet clean, Fyne UI builds. Integration passed at d3385c1 and neither of the two follow-up commits touches the data path.

LGTM from me — both of my points are resolved, one by a fix and one by being wrong.