feat(amp): change fan-mode cycle button to a pull-down (#3905) - #4459
#4459feat(amp): change fan-mode cycle button to a pull-down (#3905)#4459M7HNF-Ian wants to merge 2 commits intoaethersdr:mainaethersdr/AetherSDR:mainfrom M7HNF-Ian:fix/3905-amp-fan-pulldownM7HNF-Ian/AetherSDR:fix/3905-amp-fan-pulldownCopy head branch name to clipboard
Conversation
Fixes aethersdr#3905. The three fan modes (STANDARD/CONTEST/BROADCAST) were hidden behind a click-to-cycle button — no way to see the choices without clicking blind through them. Swap QPushButton m_fanBtn for a QComboBox with all three modes as items (display text via the existing fanModeLabel(), uppercase mode as itemData so the fanModeChanged contract is unchanged). setFanMode() now finds and selects the matching item under QSignalBlocker when reflecting an incoming PGXL status, so it doesn't echo a redundant "setup fanmode=" command back to the amp — the same failure mode the old code guarded against with its own state tracking. No live PGXL amp available to test end-to-end, so extended tests/amp_applet_test.cpp instead: constructs a real AmpApplet headlessly and asserts setFanMode() selects the right item, shows the combo, and does NOT emit fanModeChanged (the echo-prevention case); a user selection does emit it with the correct uppercase value; and an unrecognized mode string leaves the selection unchanged rather than desyncing. All pass, plus the 13 pre-existing temperature-toggle tests (unaffected, confirms no regression).
There was a problem hiding this comment.
Nice, focused change — swapping the blind click-to-cycle button for a QComboBox is a clear UX win, and the implementation is tidy: itemData preserves the uppercase fanModeChanged contract so the protocol wiring is genuinely untouched, and the QSignalBlocker in setFanMode() is exactly the right way to reflect incoming PGXL status without echoing a command back. The connect is placed after the addItem loop so construction doesn't spuriously emit. Test coverage is thorough and pins the echo-prevention case that the triage flagged as the main risk. Conventions look good (ThemeManager, RAII, no QSettings).
Just two non-blocking notes below — nothing that needs to hold up the merge.
Polish (non-blocking)
- Accidental mouse-wheel scroll over the combo will change fan mode and send
setup fanmode=to the amp — worth suppressing for a hardware control. - Unknown-mode branch in
setFanMode()leavesm_fanMode/accessibleName set to the unrecognized string while the combo keeps its prior selection — a small state desync.
The three CodeGuard CG-PATH-001 findings are all in .github/workflows/*.yml, which this PR doesn't touch — false positives, ignore them.
🤖 aethersdr-agent · cost: $5.0931 · model: claude-opus-4-8
| for (const QString& mode : {QStringLiteral("STANDARD"), QStringLiteral("CONTEST"), QStringLiteral("BROADCAST")}) | ||
| m_fanCombo->addItem(fanModeLabel(mode), mode); | ||
| m_fanCombo->setSizePolicy(QSizePolicy::Minimum, QSizePolicy::Fixed); | ||
| m_fanCombo->setFocusPolicy(Qt::TabFocus); |
There was a problem hiding this comment.
Non-blocking UX foot-gun: a QComboBox changes its selection on mouse-wheel scroll even when the popup is closed. Since this control commands the amplifier's fan, an operator scrolling the AMP applet (or the panel it sits in) can silently flip the fan mode and fire a setup fanmode= command. Consider ignoring wheel events unless the popup is open — e.g. install an event filter that eats QEvent::Wheel on the combo, or a tiny QComboBox subclass overriding wheelEvent() to no-op. Qt::TabFocus alone doesn't prevent this.
| if (idx < 0) { | ||
| qWarning() << "AmpApplet: unknown fanmode" << m_fanMode; | ||
| } else { | ||
| // Reflecting an incoming PGXL status — block signals so this | ||
| // doesn't fire currentIndexChanged and echo a redundant | ||
| // "setup fanmode=" command back to the amp (#3905). | ||
| QSignalBlocker blocker(m_fanCombo); | ||
| m_fanCombo->setCurrentIndex(idx); | ||
| } | ||
| m_fanCombo->setAccessibleName(QString("Fan speed: %1").arg(m_fanMode)); | ||
| m_fanCombo->show(); |
There was a problem hiding this comment.
On the unknown-mode branch, the combo's selection is (correctly) left untouched, but m_fanMode is still assigned the unrecognized value at line 380 and line 391 announces it via accessibleName — so after a bogus status the widget's internal mode and the a11y name disagree with what the combo actually shows. Minor, but you could tuck the setAccessibleName/show() inside the else, or skip overwriting m_fanMode when findData misses, to keep them consistent. The test already asserts the selection stays put, so this is purely about the stale m_fanMode/name.
|
Cisco CodeGuard — static analysis of this PR (3 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $5.2299 · model: claude-opus-4-8 |
|
Antares Detector — candidate vulnerable file(s)
Localized by Cisco Foundation AI Antares-1B running locally in the AetherClaude sandbox, seeded by the Cartographer security map. Advisory only — please verify before acting. 🤖 aethersdr-agent · cost: $5.2299 · model: claude-opus-4-8 |
Address two review notes on aethersdr#4459: 1. m_fanCombo was a plain QComboBox, so hovering the mouse and scrolling would silently change fan mode and send a live "setup fanmode=" command to the amp. Swap to GuardedComboBox (already used for this exact reason elsewhere, e.g. AntennaGeniusApplet's device selector) — it only responds to wheel input while its dropdown is actually open. 2. setFanMode() unconditionally set m_fanMode to the incoming string even when it didn't match any combo item, leaving m_fanMode and the combo's displayed selection out of sync for an unrecognized mode. Now m_fanMode only updates in the branch that also updates the combo, so the two can't diverge. Verified: extended amp_applet_test.cpp already covers setFanMode's unknown-mode path (asserts the combo's selection is unchanged) — still passes, plus all 21 other assertions, confirming no regression. Wheel suppression itself isn't independently testable here (no live PGXL amp available, and GuardedComboBox is an established, already-used class) — relying on code reuse rather than a fresh test.
Closes #3905.
What
The three fan modes (STANDARD/CONTEST/BROADCAST) were hidden behind a
click-to-cycle button — no way to see the choices without clicking
blind through them.
Swaps
QPushButton m_fanBtnfor aQComboBoxwith all three modes asitems (display text via the existing
fanModeLabel(), uppercase modeas
itemDataso thefanModeChangedcontract — "uppercase, ready forsendCommand" — is unchanged).
setFanMode()now finds and selects thematching item under
QSignalBlockerwhen reflecting an incoming PGXLstatus, so it doesn't echo a redundant
setup fanmode=command backto the amp.
No protocol change —
MainWindow_Wiring.cpp's incoming-status andoutgoing-command wiring is untouched.
Test plan
No live PGXL amp available to test end-to-end (the AMP applet is
hardware-gated out of the dock without one), so extended
tests/amp_applet_test.cppinstead:setFanMode()selects the matching item and shows the combosetFanMode()does not emitfanModeChanged(theecho-prevention case — this is the main risk the triage flagged)
fanModeChangedwith thecorrect uppercase mode
instead of desyncing
(unaffected, confirms no regression)