feat(freedv-reporter): 6m+ band, multi-select, 3 fixes. Principles V, XIV.#3591
feat(freedv-reporter): 6m+ band, multi-select, 3 fixes. Principles V, XIV.#3591NF0T wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Thanks @NF0T — this is a clean, well-reasoned omnibus. The multi-select rewrite (single QSet<int> replacing the scalar index), the m_initializing gate matching the MqttSettingsDialog pattern, the versioned nested-JSON schema via AppSettings (Principle V), and the explicit old→new migration are all idiomatic for the codebase. CI is green on all six checks, and nothing outside the dialog references the changed private signatures. The three bug fixes each have a clear root cause and the diff matches the description.
A few observations, none blocking:
1. (minor, partly pre-existing) restoreSettings() always leaves the proxy in Band mode. The new flow ends with an unconditional applyBandFilter(), and setBandFilters() sets m_mode = Band on the proxy. So when restoring in Freq mode (bandMode == false) with Track off, the table ends up band-filtered rather than freq-filtered until the next retune. The old code had the same shape, so it's not a regression — but since this PR rewrote the restore path, it's a natural spot to fix it, e.g.:
if (bandMode) {
m_bandRadio->setChecked(true);
applyBandFilter();
} else {
m_freqRadio->setChecked(true);
// Freq mode: proxy is driven by the next slice update; syncing the
// band-button highlight only.
syncButtonStates();
}Practical impact is small (Freq mode has no frequency to filter on without a tracked slice, and m_slice is typically null at restore), hence low priority.
2. (minor) Per-retune persistence while tracking. In Band + Track, every onSliceFrequencyChanged that lands in a band runs applyBandFilter() → persistSettings() → AppSettings::save(), i.e. a disk write on each band-crossing tune. Pre-existing and unchanged by this PR, just flagging since the hoisted loop now routes through it in both branches. Not worth addressing unless you're already in here.
3. (note, no action) Track collapses a multi-band selection. Enabling Track with several bands Ctrl-selected will overwrite the set with the single tracked band on the first slice update, and that's now persisted. That reads as the intended semantics of Track mode, so just confirming it's deliberate.
The migration test cases (last two checkboxes) are still unchecked in the plan — worth ticking those off before merge since the bandVersion-absence detection and the old index-9→All mapping are the trickiest part to get right. Nice work.
🤖 aethersdr-agent · cost: $4.5796 · model: claude-opus-4-8
c046549 to
66c7eb1
Compare
|
Amended the branch commit (force-pushed) to fold in a small follow-on fix discovered during the compiler warning cleanup feasibility pass run after this PR was opened. What changed: Why in this commit rather than a follow-on: The two affected methods are in the proxy class added by this PR, so the deprecation is our introduction. Folding the fix here keeps the warning count from going up on merge rather than requiring an immediate cleanup PR. No other changes to the diff. History is linear — one commit on the branch. |
|
Thanks for this, @NF0T — the multi-band selection work looks great, and I appreciate you folding the proxy cleanup in rather than leaving a warning behind. The CI failure is a small, easy one, and it traces directly back to that folded-in cleanup. Good news: it's a real source-portability issue, not CI infrastructure, so it's fully in your control to fix. What failedOn commit
That "mac passes, Windows + Linux fail" split is the tell: the diff uses an API that only exists in the newest Qt. Root causeYour force-push comment says the swap was made because
(Worth flagging since the rationale came from a "compiler warning cleanup" pass — I'd double-check whatever surfaced the "deprecated" claim; it doesn't match Qt's actual API history.) The fixRevert just those two call sites back to the portable API. In beginFilterChange();
m_mode = Band; // (your state mutation)
m_bands = bands;
endFilterChange();with: m_mode = Band; // (your state mutation)
m_bands = bands;
invalidateFilter();It's functionally equivalent for this use — set the members, then invalidate — and it mirrors the existing The minimum Qt across CI is 6.4.2 (Linux floor), so anything newer than that needs a version guard or an alternative — a handy rule of thumb when CI is green on mac but red on Windows/Linux. Push that one-line-per-method change and CI should go green. Nice feature — thanks for volunteering the time on it. 🙂 🤖 aethersdr-agent · cost: $3.0255 · model: claude-opus-4-8 |
… XIV.
Add "6m+" catch-all band button (index 9, 50–1300 MHz) mirroring
FreeDV-GUI's "Other" filter so VHF/UHF RADE stations can be isolated
without losing HF context.
Add Ctrl+click multi-band selection (⌘-click on macOS): plain click
retains single-select; Ctrl+click toggles bands in/out of the active
set; deselecting the last active band reverts to All mode. QSet<int>
replaces scalar m_activeBandIndex; proxy gains setBandFilters() with
an OR-of-ranges Band path.
Fix A: in Track+Freq mode, band buttons didn't update visually when
the slice frequency changed. Hoist the band-finding loop before the
Band/Freq branch split so syncButtonStates() fires in both modes.
Fix B: persistSettings() fired inside buildBody() before
restoreSettings() ran, overwriting saved state on every startup.
Guard with m_initializing flag (pattern from MqttSettingsDialog).
Fix C: switching to Band mode with Track enabled triggered three
redundant persistSettings() calls. Remove the trailing explicit call
from onBandModeToggled(); applyBandFilter() already persists.
Settings migrated: new format adds bandVersion:1 + bandIndices array;
old scalar bandIndex (0–8) maps 1:1; old index 9 ("All") → empty set.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
66c7eb1 to
eb4035f
Compare
|
Pushed a second amendment — reverts the The aethersdr-agent review above is correct: Fix is |
Summary
Follow-on omnibus PR to the FreeDV Reporter dialog, adding two user-facing enhancements and fixing three pre-existing bugs discovered during review and OTA testing.
Enhancements:
Bug fixes:
onSliceFrequencyChanged. Fixed by hoisting the loop before the Band/Freq split sosyncButtonStates()fires in both branches.persistSettings()was called insidebuildBody()(viaapplyBandFilter()) beforerestoreSettings()ran, overwriting the user's saved selection with defaults on every startup. Fixed with anm_initializingflag (pattern fromMqttSettingsDialog) that gates all saves until construction completes.persistSettings()calls. Removed the redundant explicit trailing call fromonBandModeToggled();applyBandFilter()already persists.Settings migration: Old format (
bandIndexscalar, 10-band schema) is detected by the absence ofbandVersion. Indices 0–8 (160m–10m) map 1:1. Old index 9 ("All") migrates to empty set → All mode. New format usesbandVersion: 1+bandIndicesJSON array (Principle V).Files changed:
src/gui/FreeDvReporterDialog.{h,cpp}only (default CODEOWNERS tier).Principles V (nested-JSON config), XIV (atomic persistence).
Test plan
HAVE_WEBSOCKETSenabled; zero new warnings{"bandMode":true,"bandIndex":5,"track":false}toFreeDvReporterAppSettings key → reopen → "17m" correctly restoredbandIndex: 9) migrates to All mode🤖 Generated with Claude Code