[core] Unify recenter policy across tuning and pan/zoom workflows#1861
[core] Unify recenter policy across tuning and pan/zoom workflows#1861ten9876 merged 1 commit intoten9876:mainfrom
Conversation
jensenpat
changed the title
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: [core] Unify recenter policy across tuning and pan/zoom workflows
Thanks for this thorough work, @jensenpat. The policy-fragmentation problem you describe is real, and centralizing intent classification in MainWindow is the right pre-1.0 approach. The PR description and RECENTER.md document are exceptionally detailed, which makes review much easier.
What looks good
- Intent model design — The five-way
TuneIntentenum (IncrementalTune,AbsoluteJump,CommandedTargetCenter,ExplicitPan,RevealOffscreen) captures the actual semantic distinctions well. This is a clean way to encode policy without rewriting the tuning subsystem. - Signal rename:
stepTuned→stepTuneRequested— Correctly shifts VfoWidget from "I already tuned" to "please tune for me", letting MainWindow own thesetFrequency+ follow decision. The old pattern of callingm_slice->setFrequency()inside VfoWidget's wheel handler and then emitting a signal for follow was fragile. frequencyClicked→incrementalTuneRequestedsplit in SpectrumWidget — Wheel/trackpad scroll and VFO drag were incorrectly using the same signal as click-to-tune. Giving them their own signal is the key fix for the "page-flip" incremental behavior.- Combined center+bandwidth for zoom — Replacing separate
bandwidthChangeRequested+centerChangeRequestedwithfrequencyRangeChangeRequestedfor zoom buttons, pinch zoom, and bandwidth drag directly addresses the P1 waterfall edge-loss bug class. The singledisplay pan set ... center=... bandwidth=...command is the right fix. - Memory spot double-dispatch fix — Routing memory spots straight to the recall path instead of through generic
frequencyClickedfirst eliminates the "reveal then re-center" double motion. Clean fix. MemoryDialogcleanup — EmittingmemoryActivated(idx)instead of directly callingsendCommand+setPanCenteris the right decoupling. The dialog shouldn't be making radio commands directly.- Step-quantized nudge —
quantizeIncrementalFollowDeltausingstd::ceilwith a small epsilon guard is a nice touch for smooth edge behavior.
Items worth discussing
-
onFrequencyChangedremoval from private slots — The header patch removesonFrequencyChanged(double mhz)fromprivate slots. This slot is currently called from twoconnectsites inwirePanadapter(lines ~6715 and ~6718 in the current code). The PR presumably replaces those call sites with the newapplyTuneRequestrouting, but since the full MainWindow.cpp diff was truncated, I want to flag this for @ten9876: verify all existingonFrequencyChangedcall sites are properly migrated to the new intent-based path, especially the cross-pan / same-pan resolution logic inwirePanadapter. -
VfoWidget::m_directEntrySourcedefault — The new member is initialized to"vfo-direct-entry"and reset to that same value after each commit. ThebeginDirectEntry(QString source)method allows overriding it (e.g., "go-to-frequency" from the keyboard shortcut). This is fine for logging, but note that ifbeginDirectEntryis called but the user cancels (presses Escape),editingFinishedfires and resetsm_directEntrySource— which is correct. However, if a future caller forgets to pass a source, it silently falls back to the default. This is acceptable for now but worth a comment. -
Animation duration change: 200ms → 110ms — The
setFrequencyRangeanimation duration dropped from 200ms to 110ms. This is a user-visible behavior change that affects all small-shift animations, not just incremental follow. The PR description documents the 110ms target for incremental follow specifically, but this change applies to everysetFrequencyRangecall (including reveals, commanded centers, etc.). Is the 110ms intended globally, or should different intents potentially use different durations? For pre-1.0 this is probably fine — 110ms feels responsive — but it's a broader change than the description suggests. -
RECENTER.mdscope — Adding architecture docs is appreciated, but at 526 lines this is substantial. A few notes:- It references the
aether/recenter-policyworktree name which is the development branch name, not something that will exist after merge. Consider updating this reference. - The mermaid diagram and exhaustive call-site lists are great for review but may drift from the code over time. Consider adding a note that this is a snapshot as of this PR.
- It references the
-
Potential merge conflict with #1858 / #1854 — Both of those draft PRs touch
MainWindow.cppband-switch logic. This PR's changes to the tuning flow in MainWindow will likely conflict. Not a blocker, but worth coordinating the merge order.
No issues found
- No
QSettingsusage (correctly usesAppSettingsper project convention) - No obvious null pointer risks — the existing guards (
if (!s),if (!pan),if (!sw)) are preserved - No resource leaks — no new heap allocations without ownership
- RAII patterns maintained
- C++20 idioms used appropriately (designated initializers in
TuneCenteringResult) - Files changed are all within the stated scope (GUI tuning/pan policy)
Summary
This is a well-structured, well-documented change that addresses a real architectural problem. The intent model is sound, the signal split is correct, and the combined zoom path directly fixes reported bugs. The main risk is the breadth of the change (9 files, 1038 additions) touching a critical user interaction path right before 1.0, but the approach of layering policy on top of the existing subsystem rather than rewriting it keeps the risk manageable.
I'd suggest @ten9876 pay closest attention to:
- The
wirePanadaptermigration ofonFrequencyChangedcall sites - Manual testing of the 110ms animation across different input modalities
- Memory spot click → single centered recall (no double motion)
Thanks for the detailed testing matrix and the honest "Known Gaps" section. Good contribution.
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Large but well-executed refactor. RECENTER.md design doc is excellent — documents the intent model, ownership boundaries, and intentional exceptions clearly. applyPanRangeRequest's combined center+bandwidth command is the right fix for the P1 waterfall-loss bug. Manual test coverage across ~20 tuning paths is reasonable given no automated test suite.
Accepting scope trade-off: architectural refactor is explicitly on the 'maintainer review required' list, but this is a community PR from a trusted contributor who bundled the P1 bug fix with the refactor. Merging under a v0.8.21 'hardening' plan — will watch for tuning regressions this week.
Points I'll specifically validate post-merge:
- Trackpad scroll / VFO wheel / keyboard arrow tuning smoothness
- Memory recall landing centered
- Ordinary spot clicks not being forced to center
- Bandwidth drag / pinch zoom preserving waterfall
- Mode jump (USB↔SAM) after memory recall
LGTM.
) Community-heavy point release — 5 community PRs + 6 AetherClaude fixes covering v0.8.20 regressions, Wayland+FFmpeg crash class, RADE TX regression from v0.8.19, and a large tuning-policy refactor. Highlights: - Unified recenter policy with TuneIntent model (#1861, jensenpat) including fix for P1 waterfall-loss during bandwidth drag/pinch zoom and P2 keyboard-zoom drift after memory recall - Band Stack management feature — clear all, band grouping, auto-expiry (#1472) - Tolerant X11 error handler for Wayland + non-free FFmpeg / VDPAU Audio-tab crash (#1840) - DXCC prefix resolution off GUI thread — fixes 0.5-1s waterfall freezes on large logbooks (#1844) - XVTR waterfall rendering for transverter IF/RF frequency mismatch (#1845) - Minimal Mode no longer freezes popped-out panadapters (#1748) - rigctld TCP CAT per-port slice binding (#1623) — multi-instance WSJT-X now isolates correctly - TCI RX meter post-gain (community RFC, #1717) - RADE TX no-waveform regression from #1780 fixed (#1865, NF0T) - Applet pop-out persistence + app-exit regression (#1860, chibondking) - ShackSwitch R4 Arduino TCP deadlock (#1859, nigelfenton) Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
2 participants
Summary
This PR makes pan-follow / recenter behavior consistent across tuning modalities without redesigning the radio/tuning subsystem before 1.0.
The core change is to move tuning intent policy into a shared
MainWindowpath so that different input surfaces stop making independent pan/center decisions. The result is that incremental tuning, jump-style tuning, commanded recalls, off-screen reveals, and explicit pan/zoom now behave predictably and use the same centering rules.It also fixes a second class of bugs on the explicit zoom side: center and bandwidth are now kept coherent during combined pan/zoom operations instead of being sent as unrelated radio commands.
Why This Change Was Needed
Before this work, several user-visible problems were all variations of the same root issue: different call sites were deciding for themselves when to tune, when to recenter, when to reveal, and when to pan.
That showed up as multiple real usability problems:
Go To Frequency, memories, and memory spots were not all expressing the same semantic intent even though the user experience strongly suggested they should.The most important user-facing failures from that last class were already showing up in bug reports:
+/-could drift or glitch after memory recalls and mode jumps such asUSB <-> SAM, especially when the zoom path and the recalled pan state did not land coherently.In short: the app was not missing one isolated recenter fix. It had a policy-fragmentation problem.
Design Approach
This change keeps the fix low-risk and localized before 1.0:
SpectrumWidget::setFrequencyRange(...)machinerysetFrequency(... autopan=0)where appropriateMain Architecture Changes
1. Shared tuning intents in
MainWindowMainWindownow classifies tune requests using a small intent model:IncrementalTuneAbsoluteJumpCommandedTargetCenterExplicitPanRevealOffscreenThese intents feed shared helpers instead of letting each UI surface improvise its own center behavior.
2. Shared tune/reveal path
Most tuning now routes through shared
MainWindowhelpers:applyTuneRequest(...)revealFrequencyIfNeeded(...)panFollowVfo(...)This keeps tune ordering, optimistic local UI updates, and pan/reveal decisions coupled in one place.
3. Step-quantized incremental follow
Incremental tuning now uses trigger-based pan follow with a smoother, deterministic nudge model:
Instead of jumping straight back to the wider settle fence, incremental follow now uses the configured step size as the pan nudge quantum when available. That keeps trackpad, keyboard, wheel, and encoder behavior smooth at the edge rather than book-page-like.
4. Commanded-target centering
A new
CommandedTargetCenterintent is used for cases where the user is explicitly commanding a known destination and expects it to become the center of attention.This now covers:
Go To Frequency5. Ordinary spots remain reveal-only
Ordinary spots remain on
AbsoluteJumprather than being promoted to commanded-center behavior.That means:
6. Explicit pan/zoom coherence
A shared explicit pan/zoom helper now keeps center and bandwidth coherent for combined range changes:
applyPanRangeRequest(...)This path updates
PanadapterModeloptimistically with both center and bandwidth together and sends a single combineddisplay pan set ... center=... bandwidth=...command.That change is specifically aimed at the P1/P2 bug class where one user-visible zoom action used to be split into multiple radio state transitions.
Changed Call Sites
IncrementalTune
These now use the shared incremental policy:
AbsoluteJump
These remain reveal-oriented jumps:
CommandedTargetCenter
These now center a known commanded destination through the shared policy path:
Go To FrequencyRevealOffscreen
These still use shared reveal behavior rather than bespoke code:
ExplicitPan / explicit range changes
These now keep center/bandwidth coherent for combined zoom operations:
+/-Memory / Spot Behavior
Memory and spot handling needed special cleanup because several bugs came from treating them as the same thing.
This PR preserves an intentional split:
A memory-spot-specific double-dispatch bug was also fixed:
Explicit Hard-Center Paths Intentionally Kept
For lowest risk before 1.0, these paths remain radio-authoritative / hard-center paths on purpose:
restoreBandState(...)Remaining Gaps / Deliberately Unchanged Paths
These were intentionally left outside this pass:
src/gui/RxApplet.cppstill has its own direct-entrytuneAndRecenter(...)pathtuneAndRecenter(...)because they are still expressing radio-authoritative external control semantics rather than the new local UI policyDocumentation And Guardrails
This PR also adds reviewer/developer guidance so the behavior is easier to preserve:
RECENTER.mddocuments the current architecture, intent model, signal flow, call-site classification, and intentional hard-center exceptionsValidation
Automated
cmake --build build -j10ctest --output-on-failure -j10No tests were found!!!Manual
Manually tested by @jensenpat across the recenter-policy branch and again after merging latest
main, including:Go To Frequency+/-USB <-> SAMReviewer Notes
The two areas worth the closest review are:
MainWindow, because that is now the main policy boundaryThe goal of this PR is not to make everything center. The goal is to make each action express the right intent consistently:
👨🏼💻 Generated with OpenAI Codex (GPT-5.4 Pro) and tested by @jensenpat