Feature/panlock: add PanLock button to TitleBar#3408
Conversation
NF0T
left a comment
There was a problem hiding this comment.
Thanks for this, @ea5wa — the core problem you're solving is real and the implementation is technically correct. Before this can merge there are two hard items to fix, and a design question that needs @ten9876's input. Covering all three below.
1. Personal scratch file in .gitignore — must fix
+Resumen para empezar de nuevo.txtThis is a personal scratch file that should live in your global gitignore (~/.gitignore_global, configured via git config --global core.excludesFile) rather than the project's tracked .gitignore. If merged upstream this entry becomes permanent in the shared repo. Please remove this line.
2. Button state not persisted across sessions — must fix
Every other checkable button in TitleBar reads its initial state from AppSettings and writes on toggle — m_pcBtn is the direct pattern to follow. Pan Lock currently resets to off on every launch, which defeats the purpose for satellite operators who need it on every session. Two lines in the constructor, two in the toggled handler:
// In constructor, after setCheckable:
bool panLockOn = s.value("PanLockEnabled", "False").toString() == "True";
m_panFollowBtn->setChecked(panLockOn);
// In toggled handler, before emit:
auto& ss = AppSettings::instance();
ss.setValue("PanLockEnabled", on ? "True" : "False");
ss.save();3. Design direction — needs @ten9876 to weigh in
@ten9876 — context for your decision follows.
Why this PR fills a genuine gap: The existing "Pan Follows VFO" (View menu, PanFollowVfo AppSettings key) is wired into AetherSDR's internal tuning pipeline — it only fires when AetherSDR itself commands a frequency change (TuneIntent::IncrementalTune). When SatPC32 or another external program sends Doppler corrections directly to the radio's CAT interface, the frequency change arrives as a radio status update. SliceModel::frequencyChanged fires and AetherSDR updates its display — but the panFollowVfo() path is never entered and the panadapter center stays fixed. For a satellite pass with continuous Doppler correction, the signal walks off-screen within minutes.
Why the title bar may be the wrong surface: Pan Lock is a button that will be visible to every user on every launch, but the Doppler-tracking use case it serves represents a small fraction of operators. The View menu is the established home for display-behavior toggles and is where "Pan Follows VFO" already lives. Adding a permanent title bar widget for a niche feature adds chrome to the UI that most users will never use.
Why extending "Pan Follows VFO" is achievable: setPanFollow already uses the simple direct path — applyPanStatus + sendCommand — not the full policy system. The same two-line center call could live behind an extension of the existing panFollowEnabled() flag (or a companion flag like panFollowExternal) wired into the frequencyChanged handlers in onSliceAdded. The existing anti-rubber-band guards (m_flexCoalesceTimer) only need protecting in the human-tuning path; an external-change re-center can bypass them cleanly since it lives in a different code path entirely.
The question for Jeremy: Should this be a title bar button (accepted as-is once items 1–2 are fixed), a View menu item extending or complementing the existing "Pan Follows VFO" concept, or something else? The underlying need is valid — the right surface is a maintainer call per the autonomy boundaries in CLAUDE.md.
@ea5wa — once Jeremy has weighed in on the direction, happy to re-review promptly. The two hard fixes (items 1 and 2) can go in now regardless of which direction the design takes, since they'd be needed either way.
73,
Ryan NF0T
|
Hi Ryan, Thanks for the thorough review. Items 1 and 2 are fixed in the latest push (6f5e217):
On point 3 — I'll wait for Jeremy's call on the right surface before making any further changes. The underlying need is real (external CAT Doppler corrections walking the signal off-screen within minutes during a satellite pass is a genuine pain point), but I fully agree the title bar vs. View menu question is a maintainer decision. Happy to move it to the View menu, extend the existing Thanks again for the detailed context on the existing 73 de EA5WA Juan Carlos |
There was a problem hiding this comment.
Thanks @ea5wa — nice tightly-scoped feature, and the use of a persistent QMetaObject::Connection for clean teardown is exactly right. The diff is otherwise additive and respects the "no behavior change for users who don't press it" goal you cite. A couple of things I'd suggest tightening before merge:
1. Persisted PanLockEnabled state doesn't actually engage tracking on startup
TitleBar.cpp reads PanLockEnabled and calls setChecked(panLockOn) before the toggled lambda is wired up — and MainWindow connects to TitleBar::panFollowToggled after the TitleBar is already constructed. So when a user relaunches with Pan Lock previously enabled, the button paints blue/active but setPanFollow(true) is never invoked, and the panadapter doesn't follow Slice A until the user clicks the button off and on again.
A simple fix is to emit/invoke the slot once after MainWindow has wired everything up, e.g. right after the connect(...) you added at MainWindow.cpp:3391:
connect(m_titleBar, &TitleBar::panFollowToggled,
this, &MainWindow::setPanFollow);
if (m_titleBar->isPanFollowChecked()) setPanFollow(true);(or just call setPanFollow(panLockOn) once after the title bar is built).
2. Pan Lock silently stops working if Slice A is destroyed and re-created
setPanFollow(true) captures m_radioModel.slice(0) and connects to its frequencyChanged. On a radio reconnect (or any flow that tears slice 0 down and re-adds it), Qt auto-disconnects the dead connection cleanly — no UAF — but the lock is silently lost: the button stays checked but no recentering happens. Worth either listening to RadioModel::sliceAdded and re-attaching when a new slice 0 appears, or at least flipping the button off so the UI matches reality.
Related: if slice(0) is null at the moment of toggling (e.g. before any slice exists), setPanFollow returns silently and the button is left in a checked-but-inert state. A defensive un-check would be friendlier.
Out of scope (minor)
The PR body mentions adding Resumen para empezar de nuevo.txt to .gitignore, but I don't see that in the diff — looks like it was already cleaned up, just flagging in case the description is now stale.
Nothing structural — the AppSettings "True"/"False" string convention matches the surrounding PcAudio/Heartbeat code, the qFuzzyCompare short-circuit on equal centers is fine for MHz-scale values, and the connection lifetime is properly managed via the captured m_panFollowConn + this context.
🤖 aethersdr-agent · cost: $12.2338 · model: claude-opus-4-7
|
Both issues addressed in the latest push (ebd3a15): 1. PanLock not engaging on startup if (m_titleBar->isPanFollowChecked()) setPanFollow(true);This ensures the persisted state actually activates tracking on launch, not just paints the button blue. 2. Silent loss of tracking when Slice 0 is recreated I hope will be OK now 73 de EA5WA Juan Carlos |
|
Hi @ea5wa — good news: this CI failure is not caused by your code. It's a CI infrastructure bug that was fixed on What failedThe Qt Accessibility Static Analysis job failed at the Run accessibility check step. The accessibility checker itself ( Root cause (CI infrastructure, not your code)The old COUNT=$(grep -c "^::warning" /tmp/a11y_output.txt || echo "0")
This is exactly the issue Ian fixed in #3442 (commit How to get green CIJust rebase or merge git fetch upstream
git merge upstream/main # or: git rebase upstream/main
git pushNo code changes needed on your side. Other checkThe Thanks for your patience with this and for the careful follow-ups on the review comments — the 🤖 aethersdr-agent · cost: $18.2092 · model: claude-opus-4-7 |
|
Hola @ea5wa — pushed two fix-ups (`b9a3eafd`) plus a rebase onto current main so this PR builds and the a11y CI passes. The build was actually failing for a real reason — not just the a11y workflow bug @aethersdr-agent flagged. Two failures, two causes1. The real build break — `signals:` vs `public:` In `ebd3a154` you added two accessors: Qt's MOC parses everything inside a `signals:` block as a signal declaration, so it rejected the two non-void methods with `error: Not a signal declaration` at `TitleBar.h:56`. That's what broke `build`, `check-macos`, `check-windows` and `analyze (cpp)` — all four show the same error in the logs. There was a second latent issue: even after moving them out of `signals:`, the inline bodies touch `m_panFollowBtn->isChecked()` and `QSignalBlocker(QPushButton*)`, both of which need the full `QPushButton` type — but `TitleBar.h` only forward-declares `QPushButton`. So I moved both definitions out-of-line into `TitleBar.cpp` (which already includes `` and now also includes ``). Matches the style of neighbours like `isSystemMoveAreaAt`. 2. The a11y CI failure — workflow bug, not your code @aethersdr-agent's explanation was correct: pre-#3442, the workflow's `COUNT=$(grep -c "^::warning" ... || echo "0")` produced `"0\\n0"` on zero findings and `$GITHUB_OUTPUT` rejected the malformed line. #3442 (commit `95c43a57`) merged a couple of hours after your `ebd3a154` push and is now on main. The rebase picks it up. What I pushedSame maintainer fix-up pattern as #3279/#3286/#3289/#3381/#3398/#3417/#3439/#3441 (we landed a lot of these today). Your 4 commits replayed verbatim onto current main as ea5wa-authored; my single fix-up commit `b9a3eafd` is the `signals:`-to-`public:` move + out-of-line definitions + the `` include in the .cpp. Verified locally on Arch Linux x86 — full `AetherSDR` build clean (632/632, only the pre-existing unrelated `macDaxDriverInstalled` warning). CI should now pass cleanly. Real-talk on the actual fix in `ebd3a154`: nice — the `attachToSlice0` + persistent `m_panFollowSliceConn` to re-wire on `RadioModel::sliceAdded` is exactly the right shape for handling radio-reconnect, and the `QSignalBlocker` on `setPanFollowChecked` to avoid feedback loops is the correct idiom. NF0T's review items are properly addressed. Happy to revert any piece of the fix-up if you'd rather take a different cut. Principle XI. |
Adds a toggleable Pan Lock button to the title bar that keeps the panadapter centered on Slice A frequency. Useful for Doppler tracking via CAT (e.g. SatPC32). - TitleBar: new m_panFollowBtn widget + panFollowToggled signal - MainWindow: setPanFollow() slot wired to TitleBar::panFollowToggled
- TitleBar: read PanLockEnabled from AppSettings on init so button restores its last state on every launch - TitleBar: write PanLockEnabled to AppSettings on every toggle - .gitignore: remove personal scratch file entry (moved to ~/.gitignore_global per project conventions)
- TitleBar: add isPanFollowChecked() and setPanFollowChecked() accessors - MainWindow: call setPanFollow(true) after wiring panFollowToggled so the persisted state actually engages tracking on launch - MainWindow: add m_panFollowSliceConn to re-attach frequency tracking whenever slice 0 is destroyed and recreated (radio reconnect, etc.); uncheck button if slice 0 is null to keep UI in sync with reality
Two related fixes to TitleBar's new PanLock accessors so all four platforms compile again: 1. signals: vs. public: — MOC parses anything inside a `signals:` block as a signal declaration, so the inline `bool isPanFollowChecked() const` and `void setPanFollowChecked(bool)` were rejected at moc time with "Not a signal declaration" (TitleBar.h:56). Both methods are accessors, not signals — they belong in `public:`. 2. Out-of-line over inline. Even after the section move, the inline bodies referenced `m_panFollowBtn->isChecked()` and `QSignalBlocker`, which need the full QPushButton type. The header only forward-declares QPushButton, so inline bodies couldn't compile in callers either (the moc error masked this until now). Moving the definitions to TitleBar.cpp keeps the header light and matches the style of neighbours like `isSystemMoveAreaAt`. Build verified locally on Arch Linux x86 — 632/632 clean (only the pre-existing unrelated macDaxDriverInstalled warning). Same maintainer fix-up pattern as aethersdr#3279/aethersdr#3286/aethersdr#3289/aethersdr#3381/aethersdr#3398/aethersdr#3417/aethersdr#3439/aethersdr#3441. Principle XI.
The flat AppSettings key "PanLockEnabled" added in this PR violated
Constitution Principle V (nested-JSON-per-feature config). New
TitleBarSettings.h header-only helper mirrors the established
DisplaySettings.h / CwDecodeSettings.h pattern — single AppSettings
root "TitleBar" holding a nested JSON blob with feature toggles, with
a one-shot migrateLegacy() that reads the legacy flat key on first
launch and writes it into the new blob.
TitleBar.cpp now reads via TitleBarSettings::panLockEnabled() and
writes via TitleBarSettings::setPanLockEnabled(bool); the migration is
called once at the top of the TitleBar constructor before any other
title-bar-settings reader runs. The legacy "PanLockEnabled" flat key
is intentionally left in place — harmless after migration, and the
same future-cleanup-PR pattern as Lean Mode's "LeanMode" key
(DisplaySettings.h).
Settings key shape after this commit:
TitleBar = { "panLockEnabled": "True"|"False" }
Future title-bar toggles (heartbeat blink, etc.) can land as
additional fields in the same blob without re-introducing flat keys.
Verified locally on Arch Linux x86 — full AetherSDR build clean
(634/635, only the pre-existing unrelated macDaxDriverInstalled
warning).
Principle V.
|
Pushed `e27f2d22` migrating the new `PanLockEnabled` setting from a flat AppSettings key to the nested-JSON-per-feature pattern (Constitution Principle V). Also rebased onto current main — `main` had moved ~5 PRs since this morning's `b9a3eafd` fix-up. WhyThe PR added `s.value("PanLockEnabled", "False")` / `ss.setValue("PanLockEnabled", ...)` — flat top-level keys. Constitution Principle V requires new feature settings to live under a single nested-JSON root, not as flat keys. Tonight's #3439 (FreeDV Reporter) followed it; Lean Mode (`DisplaySettings.h`) and CW decode (`CwDecodeSettings.h`) are the existing precedents. What I pushed
The migration runs at the top of the TitleBar constructor (before `m_panFollowBtn` reads its initial state) so existing setups load with the right toggle. A future title-bar setting (heartbeat blink, etc.) can land as additional fields in the same blob without re-introducing flat keys. What I didn't touch
Verified locallyArch Linux x86 — full `AetherSDR` build clean (634/635, only the pre-existing unrelated `macDaxDriverInstalled` warning). a11y lint passes on the changed GUI files. Same maintainer fix-up pattern as #3279 / #3286 / #3289 / #3381 / #3398 / #3417 / #3439 / #3441 / #3443. Happy to revert any piece if you'd rather take a different cut. @NF0T — heads-up that this addresses the Principle V tangent on top of your earlier review (which ea5wa addressed in `9dc9055a`). The functional code from your review pass is untouched. Principle V. |
Summary:
Adds a toggleable Pan Lock button to the title bar that keeps the
panadapter centered on Slice A's frequency at all times.
When active, any frequency change on Slice A — whether tuned by hand, by CAT, or by
Doppler-correction software such as SatPC32 — immediately recenters the
panadapter so the signal stays visible in the middle of the waterfall all the time.
Deactivating the button stops the tracking and leaves the panadapter at
its current center, allowing the operator to scroll freely across the
full waterfall width while Slice A continues to track independently.
Constitution principle honored: Principle III — the feature adds
capability without altering any existing behavior; operators who never
press the button experience no change.
Changes:
TitleBar: new m_panFollowBtn checkable widget with accessible name
and tooltip; emits panFollowToggled(bool) signal.
MainWindow: setPanFollow(bool) slot wired to
TitleBar::panFollowToggled; uses a persistent QMetaObject::Connection
(m_panFollowConn) so tracking is torn down cleanly on deactivation.
Test plan:
Platform: Windows 11, Flex-6600.
PanLock behavior:
Connected to the radio and opened the main window.
Pressed Pan Lock in the title bar — button turns blue/active.
Tuned Slice A manually across several MHz: panadapter recentered on
every frequency change, keeping the slice marker in the middle of the
waterfall at all times.
Sent CAT frequency commands via SatPC32 (Doppler tracking): panadapter
followed each correction in real time with no visible lag.
Pressed Pan Lock again to deactivate: panadapter stopped following
and remained at its last center position while Slice A could be moved
freely to any point in the waterfall.
Re-enabled and disabled the button multiple times — behavior was
consistent across all cycles with no crashes or stale connections.