Improve CTCSS tone dropdown readability and scrolling affordance - #5259
Improve CTCSS tone dropdown readability and scrolling affordance#5259w5jwp wants to merge 2 commits into
Conversation
|
@aethersdrbot Please perform an adversarial review of this PR. In particular, verify the shared RX/VFO behavior, preservation of bare-frequency item data and numeric tone metadata, theme-token use, Qt 6.8/Linux portability, rendered scrollbar behavior, CI test registration, and that the DTCS discussion remains future scope rather than an implicit feature addition. |
087b695 to
a98a956
Compare
|
The assignment claim attempt failed because w5jwp lacks assignee permissions, so this comment records the active review request. @aethersdr-agent please perform an adversarial review of the current head, including shared RX/VFO behavior, item-data preservation, theme-token use, Qt portability, rendered scrollbar behavior, CI registration, and visual or UX decisions requiring maintainer authority. |
|
Hi @aethersdr-bot and @jensenpat — when you have a chance, would you please review this PR? I would appreciate any feedback or changes you recommend. Thank you for your time. |
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
1. Issue fit
Partially. #5258's acceptance criteria map cleanly onto the diff and most are genuinely met: one shared population path (populateCtcssToneCombo), <frequency> <designation> rendering, blank designation column for interstitials, 69.3 WZ with code untouched, an 18-row cap, an always-on themed scrollbar, and a CI gate that actually renders the delegate rather than just linking it. Qt::UserRole still carries the bare frequency string, so setFmToneValue/setFmToneRxValue/findData at RxApplet.cpp:735,748,2533,2550 and the slice tone bridge are untouched — the "no command/automation change" claim holds.
The gap is one criterion the issue didn't anticipate: the delegate discards the DisplayRole entirely, which silently drops the TX:/RX: role prefixes that #5203 put on these two combos. Details in Blocker 1.
2. Scope
| File | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/core/CtcssTones.h |
69.3 Hz "" → "WZ"; comment rewrite |
Yes (#5258 "Authority and provenance") | In scope. designation has no non-presentation consumer — grepped the whole tree, only RxApplet/VfoWidget/CtcssToneLabel.h/the new test read it. code and frequency unchanged, so isCtcssFrequency(), AutomationServer, and IcomCivBackend are unaffected. |
src/gui/CtcssToneLabel.h (new) |
Shared label helper, delegate, popup style rules, population helper | Yes | In scope; referenced by both consumers and the test — not a dead addition. |
src/gui/RxApplet.cpp |
Drops local table alias, routes both combos through the shared path, label loses the numeric code | Yes (#5258 explicitly calls out "internal numeric indexes") | In scope. |
src/gui/VfoWidget.cpp |
Routes both combos through the shared path, adds designation to a previously bare-frequency list | Yes | In scope, but this is a user-visible change to an existing control (see §Nits). |
tests/ctcss_tone_label_test.cpp, tests/tests.cmake |
New offscreen render test + registration | Yes | In scope. |
.github/workflows/ci.yml |
New PR gate with a count pin | Yes ("Automated CI coverage" is an acceptance criterion) | In scope, and it follows the house -N count-pin idiom from the Icom gate directly above it rather than inventing one. |
Nothing in the diff is unexplained by #5258. No CHANGELOG.md entry — correct.
Preference check (§6): this modifies existing UI, so it needs an authority. It has one — #5258 states the defect with before/after evidence and carries enhancement + maintainer-review. That label is still open, though, so the presentation itself is a maintainer call, not something this review can bless.
CodeGuard: all nine findings are in RigctlProtocol.cpp, MainWindow.cpp, MemoryDialog.cpp, and tools/aether_mcp.py — none of those files is in this diff. Pre-existing; dropped.
3. Blockers
-
The delegate discards the DisplayRole, which silently removes the
TX:/RX:role prefixes from the popup on CTCSS-presentation backends.CtcssToneLabel.h:92clearsbackgroundOption.textand then paints onlykCtcssToneFrequencyRole/kCtcssToneDesignationRole. ButconfigureFmToneControls()in both files still callsfmToneDisplayLabel(...), which forFmTonePresentation::Ctcssproduces"TX: 123.0 3Z"/"RX: 123.0 3Z"(FmTonePresentation.h:20-23). On the IC-9700 — the only backend that setsCtcss(IcomCivBackend.cpp:262), and the radio you validated on — the TX and RX tone combos are visible simultaneously (RxApplet.cpp:1931-1936), and #5203 added those prefixes precisely to tell them apart. After this PR the prefix survives only in the closed combo; both open popups render identically. The twosetItemTextloops still run and are now pure overhead for the popup.This is deliberate rather than accidental — the new test at
tests/ctcss_tone_label_test.cpp:150sets a sentinel display string and asserts the render is byte-identical, so the loss is pinned by CI. That's what makes it worth blocking on rather than filing as a nit: a future contributor restoring the prefix will be told by a green-gate test that they are wrong. Either the delegate should paint the role prefix as a third element, orconfigureFmToneControlsshould stop decorating item text for these combos and the prefix should move somewhere the delegate honours. Flagging as a maintainer call if the ruling is that popup-level role labels were redundant all along — but then #5203's behaviour is being reverted, and the PR body should say so instead of claiming presentation parity.
4. Nits (non-blocking)
sizeHint()adjusts height but never width (CtcssToneLabel.h:79-84). The baseQStyledItemDelegate::sizeHintmeasures the display string, whilepaint()lays out8px + frequencyColumn + 3×spaceWidth + designation + 8px. Those are not the same number, and nothing guarantees the drawn layout fits the width the view was sized to — a wider UI font or a theme with different metrics clips the designation column at the right edge. #5258 lists "columns remain aligned without clipping under the active font" as acceptance criteria, and the test only asserts the frequency sub-column is wide enough (tests/ctcss_tone_label_test.cpp:158-164), never the total. Cheap fix: set the width insizeHintfrom the same expressionpaintuses.ctcssToneFrequencyColumnWidth()is recomputed inside everypaint()(CtcssToneLabel.h:115), so each row repaint runs 50horizontalAdvance()calls — ~900 per popup repaint. It only depends on the font; caching it per-font (or hoisting to the delegate with a font-keyed memo) costs nothing.- Sibling surface left uncapped. #5294 landed
m_dtcsCodeCmbon main after this branch's base (RxApplet.cpp:774-777, plainapplyComboStyle, noextraRules). It carries 104 entries — a worse instance of exactly the "very long popup with no visible scrollbar" problem #5258 describes. Not this PR's fault chronologically, but after a rebase the reviewed fix will sit next to an unfixed twin, and the shared style rules are already the right seam for it. - Stale base. CI is green against merge-base
3b838c0d; main has since moved toade7564c(#5294), which edits bothconfigureFmToneControlsbodies this PR rewrites. Worth a rebase before merge — and note the source-contract assertions (source.count("populateCtcssToneCombo(") == 2) are string counts over.cppfiles, so they will need revisiting the moment a third CTCSS combo is legitimately added. - VFO label content changed, not just its layout: that dropdown previously showed a bare
123.0and now shows123.0 3Z. Sanctioned by #5258 ("one shared presentation"), but the PR body frames the change as consistency/scrolling and doesn't call out that VFO users gain new text.
5. What I tried to break (and could not)
- "Stored item data and automation behavior remain unchanged." Held.
populateCtcssToneCombostill doesaddItem(label, frequency), soQt::UserRoleis the bare frequency; I checked every read site —RxApplet.cpp:735,748,2533,2542,2550andVfoWidget'sfindDatapath — and none was migrated to the newUserRole+1. The new roles are additive. - Does the 69.3
WZedit leak into protocol or validation? No.designationhas zero non-presentation readers in the tree;isCtcssFrequency(),AutomationServer.cpp, andIcomCivBackend.cppkey onfrequency/code, both untouched. The Motorola PL table does list 69.3 Hz as WZ, so the citation checks out. - Is the test self-satisfying? Mostly no, and this is the strongest part of the PR. Deleting
setItemDelegatemakes the sentinel assertion and the uncoded-row blank-column assertion fail; droppingsetMaxVisibleItemsfails line 119; droppingScrollBarAlwaysOnfails line 121. The palette-group assertions genuinely exercise the Active/Inactive/Disabled branches inpaint()by dyeing each group a different channel. The one weak spot: the production application ofctcssToneComboStyleRules()is never exercised — the test applies the rules to its own combo and then falls back to source-text greps for the real call sites. That's honest about itself in the comment, but it means "the shipping popup is styled" is asserted bygrep, not by rendering. - Does the new CI gate erode silently? No — the
-Ncount pin (= "1") is the same guard the Icom gate above it uses, so a renamed or unregistered target fails the step rather than passing vacuously. applyComboStyle(combo, extraRules)is a pre-existing seam (ComboStyle.h:104), the{{token}}placeholders resolve throughThemeManagerin the same pass as the base rules, and thethemeChangedlambda capturesextraRulesby value — so the scrollbar theming survives a live theme switch.combobox-popup: 0is the correct idiom for makingmaxVisibleItemsbind. This all checks out.
Everything above is reasoned from reading the head checkout at a98a9560; I am headless and did not build, run the test, or drive the popup. In particular I could not confirm whether the width shortfall in the nit above actually clips at your font/DPI — your screenshot suggests it does not, but the code doesn't guarantee it.
6. Recommendation
Needs maintainer decision. The engineering is careful — real shared seam, real render test with teeth, correct reuse of applyComboStyle's extraRules, correct CI idiom — and the scope is honest. Two things want a ruling before merge: whether dropping #5203's TX:/RX: popup prefixes is intended (Blocker 1, and if so the test should stop pinning the loss), and whether #5258's presentation change is approved on its own terms, since that issue still carries maintainer-review. Concrete next step: rebase onto ade7564c, resolve Blocker 1 one way or the other, and consider folding the 104-entry DTCS combo into the same popup rules while you're in there.
Thanks for the thorough issue write-up and the before/after screenshots — the provenance section made the WZ change trivial to verify.
🤖 aethersdr-agent · cost: $3.6871 · model: claude-opus-5
| { | ||
| QStyleOptionViewItem backgroundOption(option); | ||
| initStyleOption(&backgroundOption, index); | ||
| backgroundOption.text.clear(); |
There was a problem hiding this comment.
Blocker — this silently drops the TX:/RX: role prefixes from the popup.
Clearing backgroundOption.text and painting only the two roles means the DisplayRole is never rendered in the list. But configureFmToneControls() still decorates item text via fmToneDisplayLabel(), which for FmTonePresentation::Ctcss produces "TX: 123.0 3Z" / "RX: 123.0 3Z" (FmTonePresentation.h:20-23).
On the IC-9700 — the only backend setting Ctcss (IcomCivBackend.cpp:262), and the one you validated on — both tone combos are visible at once (RxApplet.cpp:1931-1936), and #5203 added those prefixes to distinguish them. After this change the prefix survives only in the closed combo; the two open popups are visually identical, and the setItemText loops become dead work for the popup.
Either paint the prefix as a third element here, or stop decorating item text for these combos so the role label lives somewhere the delegate honours. If maintainers rule the popup-level prefixes were always redundant, that's a revert of #5203's behaviour and belongs in the PR body.
| combo.setPalette(palette); | ||
|
|
||
| const QImage expected = renderRow(combo, codedRow, selectedState); | ||
| combo.setItemText(codedRow, QStringLiteral("OVERLAY SENTINEL")); |
There was a problem hiding this comment.
This is the assertion that makes the delegate's DisplayRole loss a blocker rather than a nit: it pins "the delegate never renders item text" into the CI gate. Anyone later restoring the TX:/RX: prefix to the popup (see the comment on CtcssToneLabel.h:92) will be told by a green gate that they're wrong.
The assertion is right about what the code does — it's the behaviour underneath that needs a ruling first.
| QSize sizeHint(const QStyleOptionViewItem& option, | ||
| const QModelIndex& index) const override | ||
| { | ||
| QSize size = QStyledItemDelegate::sizeHint(option, index); | ||
| size.setHeight(std::max(size.height(), kCtcssToneRowHeight)); | ||
| return size; |
There was a problem hiding this comment.
sizeHint adjusts height but never width. The base implementation measures the display string, while paint() below lays out kCtcssToneHorizontalPadding + frequencyColumn + 3×spaceWidth + designation + kCtcssToneHorizontalPadding. Nothing guarantees those agree, so a wider UI font can leave the designation column clipped at the right edge — and #5258 lists "columns remain aligned without clipping under the active font" as an acceptance criterion.
The test only checks the frequency sub-column is wide enough (line 158-164), never the total width. Non-blocking, but cheap to close:
| QSize sizeHint(const QStyleOptionViewItem& option, | |
| const QModelIndex& index) const override | |
| { | |
| QSize size = QStyledItemDelegate::sizeHint(option, index); | |
| size.setHeight(std::max(size.height(), kCtcssToneRowHeight)); | |
| return size; | |
| QSize sizeHint(const QStyleOptionViewItem& option, | |
| const QModelIndex& index) const override | |
| { | |
| QSize size = QStyledItemDelegate::sizeHint(option, index); | |
| size.setHeight(std::max(size.height(), kCtcssToneRowHeight)); | |
| const QFontMetrics metrics(option.font); | |
| const int designation = | |
| metrics.horizontalAdvance(index.data(kCtcssToneDesignationRole).toString()); | |
| size.setWidth(std::max( | |
| size.width(), | |
| kCtcssToneHorizontalPadding * 2 | |
| + ctcssToneFrequencyColumnWidth(metrics) | |
| + metrics.horizontalAdvance(QLatin1Char(' ')) * 3 | |
| + designation)); | |
| return size; | |
| } |
| painter->setPen(option.palette.color(group, role)); | ||
|
|
||
| const QFontMetrics metrics(option.font); | ||
| const int frequencyWidth = ctcssToneFrequencyColumnWidth(metrics); |
There was a problem hiding this comment.
Nit (efficiency): this walks all 50 tones and calls horizontalAdvance() on each, on every paint() — roughly 900 text measurements per popup repaint with the 18-row cap. The value depends only on the font, so a font-keyed cache on the delegate would make it once.
| m_toneValueCmb->addItem(label, frequency); | ||
| } | ||
| AetherSDR::applyComboStyle(m_toneValueCmb); | ||
| AetherSDR::populateCtcssToneCombo(m_toneValueCmb); |
There was a problem hiding this comment.
Completeness note, not a defect in this PR: #5294 landed after this branch's base and added m_dtcsCodeCmb at line 774, styled with a plain applyComboStyle(...) and carrying 104 codes — a worse instance of the exact "very long popup, no visible scrollbar" problem #5258 describes. ctcssToneComboStyleRules() is already the right seam for it. Worth folding in during the rebase so the reviewed fix doesn't ship next to an unfixed twin.
| for (const auto& [source, label] : { | ||
| std::pair{rxSource, "RX applet"}, | ||
| std::pair{vfoSource, "VFO widget"}}) { | ||
| ok &= expect(source.count(QStringLiteral("populateCtcssToneCombo(")) == 2, |
There was a problem hiding this comment.
Nit: these are string counts over .cpp source, so they encode "exactly two CTCSS combos per file" as a CI invariant. That's fine today, but it will fail on any legitimate third selector rather than on an actual regression. The comment above is honest that these stand in for behaviour the test can't reach — worth a follow-up that constructs the widgets instead, if the radio/session graph ever becomes cheap enough to fake.
|
Cisco CodeGuard — static analysis of this PR (9 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: $4.2413 · model: claude-opus-5 |
|
Antares Detector — candidate vulnerable file(s)
The constexpr CtcssTone table stores CTCSS frequencies to one decimal (e.g., 100.0 Hz), and isCtcssFrequency() compares callers against that precision, rejecting values outside the defined set. This validates tone frequency input/output safely; there's no unvalidated external input reaching a dangerous operation. 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: $4.2413 · model: claude-opus-5 |
a98a956 to
7a3bce7
Compare
|
Updated the current head after the adversarial review. Self-assignment was attempted again first and remains unavailable to The branch is now rebased onto current Local validation: 2/2 focused tests passed, strict engine-boundary and test-registration checks passed, the exact colour ratchet reported zero deltas (613 / 2724 / 1090), DTCS presentation was deliberately left unchanged because #5258 explicitly places DTCS outside this PR. @aethersdr-agent please re-review the new head when available. Thank you. |
7a3bce7 to
2632569
Compare
|
Rebased onto current Local verification after the rebase:
Fresh exact-head CI is now running. There are no known outstanding code changes; @aethersdr-agent, please rereview the corrected current head when available. |
1 participant
Summary
frequency designationpresentation.TX:andRX:role prefixes in both closed controls and open popups.WZPL designation without changing its numeric metadata.Why
The previous RX dropdown exposed internal numeric indexes, ordered its fields differently from the VFO dropdown, expanded into an exceptionally long popup, and offered no visible indication that more tones could be reached by scrolling. The revised presentation is shorter, consistent, easier to scan, and makes navigation discoverable.
The selectable frequencies remain the existing 50-tone set from #5140. PL designations were checked against Appendix G of the CISA Auxiliary Communications Field Operations Guide, including
69.3 WZ,123.0 3Z, and the M-series designations.DTCS controls and protocol behavior remain explicitly outside this PR. The newer DTCS implementation from current
mainwas preserved unchanged during the rebase.Scope and compatibility
This is shared FM-control presentation, not an IC-9700-only change. It affects every backend that exposes the RX or VFO CTCSS controls.
The bare frequency remains the combo-box item data. No radio commands, automation values, persistence semantics, tone ordering, or radio-authoritative behavior change.
Review follow-up on the rebased head:
TX:andRX:remain distinguishable.sizeHint()covers the complete rendered row under a stress font.__FILE__path. Rendered behavior is tested directly; the repository's existing focused wiring contract pins each RX/VFO combo to the shared population, label, and style paths.Visual comparison
Before: internal indexes, inconsistent field ordering, a full-height popup, and no visible scrollbar.
After: aligned frequency/designation columns, consistent spacing, an 18-row cap, and an always-visible themed scrollbar.
Validation
mainatbe5b5a54.ctcss_tone_label_test,fm_tone_presentation_contract).python3 tools/check_engine_boundary.py --strict: passed with tracked legacy warnings only.python3 tools/check_test_registration.py --strict: passed.be5b5a54: 613 unique colours, 2724 references, 1090setStyleSheet()sites; all deltas zero.git diff --check: passed.iconutil: Invalid Iconsetfailure; the focused Qt target builds and runs successfully. Fresh Linux, Windows, and macOS CI is pending on the new exact head.Guidance read
Repository
AGENTS.md, Constitution, Governance, contributing guide, developer guide, theme style guide, accessibility guidance, and the shared/project private developer guidance were applied. The private current-state file is an uninitialized placeholder; live Git and GitHub state were used instead.Fixes #5258