Repository navigation
feat(gui): radio tab shows an AT chip for adaptive throttle and colours its dot by network quality. Principle VIII. - #6787
Conversation
…rs its dot by network quality. Principle VIII. The unified title bar folded the heartbeat lamp into the active radio tab's dot, which lost the lamp's quality cue and left adaptive throttle visible only as a glow tint. The connected tab's dot now takes the status bar's network-quality colour (and names the grade in its tooltip/accessible description), and an "AT" chip follows the radio's name while the adaptive frame-rate throttle is enabled: outlined when armed, filled in the cap's quality colour while a cap is in force. Bridge: radios.linkQuality, linkQualityColor, adaptiveThrottle, adaptiveFpsCap and tabs[].throttleChip. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for this — the AT chip is a genuinely useful cue, the ours/session gating is carefully reasoned, and the WCAG 1.4.1 work (grade and throttle state spelled out in the tooltip and accessible description, not colour-only) is the right instinct. The code itself is clean; I tried several ways to break it and it survived all of them. My one blocker is about authority for a visual change, not about the implementation.
1. Issue fit
No linked issue and no RFC — search_issues for the adaptive-throttle indicator and for the heartbeat-lamp/tab-dot quality cue returns nothing but this PR. So I reviewed against the body's own stated intent. The body frames the change as restoring two cues lost when the unified title bar landed (#6198). One of those two premises does not hold — see Blocker 1. The AT chip half is sound: m_throttleFlashColor did exist pre-#6198 and the throttle really was visible only as a glow tint, so the chip addresses a real gap.
2. Scope
Everything in the diff is explained by the body; no unrelated files, no vendored or CI churn, no removed guards. One row is in scope but undisclosed as a visible change.
| File(s) | What it changes | Claimed? | Verdict |
|---|---|---|---|
RadioTabBar.cpp/.h |
AT chip, quality-coloured dot, 5 new bridge fields | Yes | In scope; the dot recolour is Blocker 1, the bridge surface is Decision 2 |
MainWindow.cpp/.h |
refreshAdaptiveThrottleChip(), quality push, fpsCapColor lambda → helper |
Yes | In scope |
MainWindowHelpers.cpp/.h |
adaptiveFpsCapColor() extracted |
Yes | In scope; thresholds byte-identical to the removed lambda, and it removes 3 hex literals |
ConnectionPanel.cpp/.h |
adaptiveThrottleEnabledChanged signal |
Yes | In scope; correctly under signals: |
TitleBar.cpp |
currentBeatColor() returns invalid instead of accent.success |
Partly | In scope, but changes the discovery pulse too — undisclosed (Nit 7) |
tests/unified_title_bar_test.cpp |
1 new block | Yes | In scope (Nit 1 on what it pins) |
| 4 doc files | chip/quality/bridge | Yes | In scope |
No socket test is added, modified, or removed by this PR — no QTcpServer/QUdpSocket/bind/listen/connectToHost and no fake peer in the changed test. Nothing to surface under the socket-test reporting rule.
3. Blockers
1. src/gui/RadioTabBar.cpp:317-319 — the quality-coloured dot is a new visual-design decision, not a restoration. The premise it rests on is not true, and GOVERNANCE.md requires an RFC for it.
The body's justification:
When the unified title bar landed (#6198), the standalone heartbeat lamp was folded into the active radio tab's status dot. That dropped … the lamp's network-quality colour
I checked the tree immediately before #6198 (5f09371e2^). The lamp never had a network-quality colour:
src/gui/TitleBar.cppat5f09371e2^contains zero occurrences of "quality" (57 KB file, so it is the real pre-change file).- The lamp was a 10×10
QLabel(TitleBar.h:166,TitleBar.cpp:133-138) whose own accessible description read:"Flashes green when radio discovery packets are received". Its colour states were grey/green/amber/red plusm_throttleFlashColor— no quality tier. networkQualityColor()at5f09371e2^had exactly the two consumers it has onmaintoday: the status-bar label (MainWindow.cpp:2248) and the diagnostics tooltip (MainWindowHelpers.cpp:182). The lamp was never one of them.
So #6198 dropped the throttle-tint prominence (real — the chip fixes it) but did not drop a quality cue. Moving the status bar's five-tier palette onto the tab dot is a new design choice: on main that dot means radio status, and after this PR a healthy connected radio on a Fair link shows amber and on a Poor link shows red — the same red family as the link-lost alarm.
GOVERNANCE.md, "What requires an RFC":
- Any change to visual design — colors, fonts, spacing, theme, icons
and the "does NOT require an RFC" list covers only "New applets or dialogs that don't change existing UX". This changes the meaning of an existing indicator. Sibling PRs in flight (#6753, #6757, #6759, #6762) all cite RFC #6226 for comparable work, so the project clearly gates this.
I'm not saying the idea is wrong — it may well be an improvement. It needs its own issue and a maintainer ruling rather than riding in under a restoration framing. Concretely: either land the AT chip alone (additive, fixes a cue that measurably did regress) and split the dot recolour into an RFC, or get an explicit ruling on the recolour before merge. Either way the body's #6198 sentence should be corrected.
4. Nits (all non-blocking)
tests/unified_title_bar_test.cpp:971-1020— neither visual claim is actually pinned. The block asserts tooltip strings and bridge ints only. Delete the quality branch fromdotColor()(RadioTabBar.cpp:317-319) and every new check still passes; likewisesetAdaptiveThrottle(true, 8, QColor("#cc9900"))asserts the tooltip andadaptiveFpsCapbut never that the chip tints. The dot is a solid-filled ellipse, so a centre-pixel probe ofgrab()is deterministic (unlike the glyph probes elsewhere in this suite, it isn't font-dependent).tests/unified_title_bar_test.cpp:985— thesessionhalf of the gate is untested. "another radio's tab does not claim this session's quality" passes onisCarrieralone; dropping&& !m_activeId.isEmpty()fromRadioTabBar.cpp:861leaves it green. Abar->setActiveRadio({})followed bycheck(!sessionTab->isThrottleChipShown(), …)would pin the disconnect-clears-chip path cheaply.src/gui/MainWindow.cpp:731-732— the comment is wrong. It says:// A cap can still be in force just after the box is unticked (it lifts at // the next quality update), so the chip stays until the cap is gone too.
RadioModel::applyAdaptiveFrameRate()returns early atsrc/models/RadioModel.cpp:8991whenAdaptiveThrottleEnabled != "True", so the lift at:9024is never reached — an already-applied cap lifts only on disconnect (:8639). That matcheslow-bandwidth-connections.md("reconnect to lift an already-applied cap") and the checkbox tooltip atConnectionPanel.cpp:1044. Theenabled || m_adaptiveFpsCap > 0behaviour is right; only the stated reason is wrong, and "just after" understates it — the chip stays filled for the rest of the session.src/gui/RadioTabBar.cpp:317-319— first hard-coded hex on the tab dot. Every other dot colour routes throughThemeManager:TitleBar::linkOverrideColor()usescolor.accent.danger/color.accent.warning, and the fallback usesstatusToken().networkQualityColor()returns literals (#00cc66,#00b4d8,#cc9900,#cc3333) calibrated against the status bar's background, now painted oncolor.titlebar.tab.active.background. A user theme can't restyle the dot any more.docs/a11y.mdwants 3:1 for non-text UI — worth stating the five colours' contrast against the tab background in both bundled themes rather than assuming the status bar's calibration carries over.src/gui/RadioTabBar.cpp:877-879— non-session tabs are pushed live chip state.setThrottleChip(ours && m_throttleEnabled, m_throttleCapColor, chipText)hands every tab the current tint and text even whenshownis false, so a cap change (8→4) misses the equality guard on every such tab and costs arefreshAccessibility()(three setters) plus a fullupdate()for no visible change. Also,chipTextis built withtr(...).arg(...)at:862-864on everyapplyLinkVisuals()call, and that runs once per pulse tick (:666) for the whole glow decay. Both go away with:tab->setThrottleChip(ours && m_throttleEnabled, ours ? m_throttleCapColor : QColor(), ours ? chipText : QString());docs/automation-bridge.md:3243-3247—radios.adaptiveThrottleis documented as "(enabled)" butrefreshAdaptiveThrottleChip()passesenabled || m_adaptiveFpsCap > 0, so after unticking the box with a cap in force the field readstruewhile the setting isfalse. Given the nit above that is the rest of the session. Worth wording it as "the chip is showing" rather than "enabled".src/gui/TitleBar.cpp:1669-1671— an undisclosed behaviour change. The removed line was:The consumer handles invalid correctly (return m_throttleFlashColor.isEmpty() ? ThemeManager::instance().color(this, QStringLiteral("color.accent.success"))
RadioTabBar.cpp:429,m_beatColor.isValid() ? m_beatColor : dot), so this is safe — but it also changes the discovery pulse, which previously glowed green over an amber override and now glows amber. Probably an improvement; it's explained only in a code comment, not the body.src/gui/RadioTabBar.cpp:223— pre-existing, out of scope, credit security-audit (CodeGuard).setToolTip(descriptionLine())passesm_entry.name(:194-195) unescaped, and Qt auto-detects rich text in tooltips, so a radio nickname containing markup renders as HTML. The repo already escapes one layer up (MainWindowHelpers.cpp:192calls.toHtmlEscaped()on the quality string). Fix is.toHtmlEscaped()on the name. This PR's additions to that tooltip aretr()constants and a fixed-table label, so it does not widen the surface — flagging only because you're in the function. I did not verify the agent's further claim that this could trigger a remote resource fetch, so please don't take that part as established.
5. What I tried to break (no finding)
Everything below is reasoned from reading the head checkout at /tmp/aetherclaude/pr-6787; I am headless, so nothing here was reproduced at runtime.
- Constructor ordering. The new
connect(m_connPanel, …)and seedrefreshAdaptiveThrottleChip()sit aroundMainWindow.cpp:2205-2207, butm_titleBar/m_connPanelare assigned at:5467/:5525— which looked like a null-senderconnect()that would silently never fire. Refuted: those lines are inbuildUI(), called at:1421, and the constructor spans:1085–<2618. Both pointers are live. (The diff's@@ … MainWindow::MainWindowheader is git's context heuristic, not the enclosing scope.) - The new signal's access specifier. The hunk header reads
public slots:; a signal declared underslots:would never be emittable. Refuted —ConnectionPanel.h:132-145puts it undersignals:with its siblings. - Stale quality/chip across a radio swap or disconnect. Two independent defences:
RadioModelemits bothadaptiveThrottleChanged(false, 0)(:8639) andnetworkQualityChanged("Off", 0)(:8740) on teardown, andsetActiveRadio("")→applyActiveState()→applyLinkVisuals()re-pushes to every tab, soours == falseclears both. A second radio can't inherit the first's grade. - Another writer of
AdaptiveThrottleEnabledbypassing the new signal.ConnectionPanel.cpp:1048is the only writer in the tree; the three other sites are reads. The signal covers every path. currentBeatColor()returning an invalidQColor. Only caller isTitleBar.cpp:1683; the sink atRadioTabBar.cpp:429already branches onisValid(). No null-colour paint.adaptiveFpsCapColor()drifting from the lambda it replaced. Identical:≤4 → #cc3333,≤8 → #cc9900, else#00b4d8.- Accessibility churn from the per-tick
applyLinkVisuals(). The early-return guards inRadioTab::setLinkQuality/setThrottleChipmeanrefreshAccessibility()fires only on a real change — correct for the carrier tab (the waste is on the other tabs, Nit 5). radioTabBar()null deref. Unconditionally constructed atTitleBar.cpp:227; the existing site atMainWindow.cpp:4837dereferences identically.- The security triage. CodeGuard's 97 findings are all false positives on this change — the CMDI-BACKTICK/AMP/PIPE and XSS-OBJECT hits are C++ comment prose (neither
RadioTabBar.cppnorConnectionPanel.cppcontains any shell invocation), the SSRF-PRIV/PII-IP hits aresetPlaceholderTextexamples and a test fixture, PATH-PROC is the fixed/proc/self/status, and PATH-UNC is JSON-escaped regex. Nothing real is introduced by this diff.
What I could not check: any runtime or visual behaviour (no build, no GUI); the actual contrast ratios in Nit 4; and unified_title_bar_test is not in ci.yml's per-PR ctest -R list, so the green checks prove it compiles, not that the new block passes — the body's local Windows run is the only evidence for it, and the body doesn't disclose that gap.
6. Recommendation
Needs maintainer decision. The implementation is solid and I found no correctness defect, but two calls aren't mine to make: (a) the quality-coloured dot is a visual-design change to an existing indicator whose stated justification is contradicted by the pre-#6198 source, and GOVERNANCE.md names exactly this category as RFC-gated; (b) five new bridge keys are public surface third parties will bind to, arriving without an issue.
Concrete next step: correct the #6198 sentence in the body, then either split the dot recolour into its own RFC and land the AT chip on its own — which I'd recommend, since the chip stands on a gap that genuinely did regress — or get @ten9876 to rule on the recolour directly. The nits are all cheap and worth folding in either way; Nits 1 and 3 are the two I'd most want fixed before merge.
🤖 aethersdr-agent · cost: $19.2804 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (100 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: $20.0887 · model: claude-opus-5 |
|
Antares Detector — candidate vulnerable file(s)
The NetworkDiagnostics implementation likely reads network packets and deserializes them; if it uses unsafe deserialization or untrusted network packet reads, it can lead to memory corruption or unsafe memory access. 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: $20.0887 · model: claude-opus-5 |
Live proof via the automation bridgeBuild: fork Windows Installer run 40263712412 on State was read from the bridge's Toggle sequence (one connected session)
The chip follows the checkbox immediately, through the new Cap in force (earlier session, throttle enabled, reconnected)
Notes
73, CJ (WT2P) 👨🏼💻 Co-authored by Claude Opus 5.5 |
…. Principle VIII. Operator feedback on aethersdr#6787: with adaptive throttle off the connection dot should stay the plain connected green, and any colour change should come with the AT chip that explains it. RadioTabBar now passes the quality to the session's tab only while the throttle is enabled; the bridge still publishes radios.linkQuality as the model's grade. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bridge re-proof for
|
| Step | linkQuality |
adaptiveThrottle |
throttleChip |
Dot | Tooltip |
|---|---|---|---|---|---|
| Connected, box unticked | Very Good | false | false | green | NW8S-Barn · FLEX-6600 · Connected · WT2P |
| Box ticked | Poor (#cc3333) |
true | true | red, outlined AT chip | … · WT2P · Network Poor · Adaptive throttle on |
| Box unticked, link still Poor | Poor (#cc3333) |
false | false | green | … · WT2P (no network grade) |
- Last row: this is the change in
d36fe274. The bridge still publishes the quality (linkQuality: Poor), but with the throttle off the tab doesn't colour its dot by it or mention it. - Whenever the dot is recoloured: the AT chip and the words "Network Poor" are there too.
- Tab width: the tab's on-screen width stayed at 195 px in all three states, and the grab shows the chip with no truncation of "NW8S-Barn".
- Cleanup: after the test the box was unticked, which is how this build found it. The test build was then disconnected and closed.
Not re-run here: an fps cap in force (filled chip). That code path is unchanged since the first proof above, which showed it at 8 fps / Fair.
…nd correct the throttle-cap comment. Principle VIII. Review follow-ups for the AT chip: - unified_title_bar_test probes the dot's centre pixel: plain connected colour without AT, the quality colour with it, plain again once the throttle is off. It also checks that no tab keeps the chip or quality with no session. Each check fails if its gate is removed (verified by mutation). - MainWindow: an applied cap lifts on disconnect, not at the next quality update (RadioModel::applyAdaptiveFrameRate returns early when the box is unticked). - RadioTabBar: tabs that can't show the chip get empty tint/text, and the chip text is built only when a tab can show it. - automation-bridge.md: radios.adaptiveThrottle means the chip is showing, not that the setting is on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@AetherClaude please re-review. Your review was of No issue → #6790. It's filed now and the description says Blocker 1 (premise and visual design): You're right that the old lamp never showed the live network grade, and the description now says so in a correction. It wasn't colour-free either: at The scope of the recolour has also narrowed since you looked. In Nits:
CI gap: the description now says Live proof on a FLEX-6600 through the bridge is in the two comments above. The second one covers |
There was a problem hiding this comment.
Re-review at af06e03b (previous pass was b5c0d1af). Thanks for the correction in the description, the mutation-tested checks and the two bridge proofs — the live tables made several of my attacks cheap to aim.
1. Issue fit
Closes #6790, which is this PR's own spec: it was filed 2026-10-26 06:56Z, 4h49m after the PR opened (02:07Z), by the same author, with no comments and no maintainer input. Against what it asks: item 1 (AT chip, outlined when armed / filled in the cap's colour) and item 3 (grade and throttle state in words) are implemented and restore what #6198 dropped — RadioTabBar.cpp:302-316, RadioTabBar.cpp:477-493, RadioTabBar.cpp:191-201. Item 2 (dot takes the quality colour, only while the chip shows) is implemented at RadioTabBar.cpp:314-316 + RadioTabBar.cpp:864-879, and both the issue and the description say it needs a maintainer ruling. That ruling is the one thing missing, and it's the recommendation below — the code is otherwise the cleanest part of this review.
The premise now checks out: RadioModel::applyAdaptiveFrameRate emits the cap, MainWindow maps it through what is now adaptiveFpsCapColor(), and the pre-#6198 lamp took that same tier colour. Your correction that the lamp never showed the live grade is in the description, so item 2 is correctly labelled as new rather than restored.
2. Scope
| File(s) | What it changes | Claimed in body? | Verdict |
|---|---|---|---|
src/gui/RadioTabBar.{h,cpp} |
AT chip paint/size/state, quality-coloured dot, session gate, 4 new bridge fields + tabs[].throttleChip |
Yes | In scope; new public surface (bridge fields) |
src/gui/MainWindow.{h,cpp} |
refreshAdaptiveThrottleChip(), quality push, lambda → adaptiveFpsCapColor() |
Yes | In scope |
src/gui/MainWindowHelpers.{h,cpp} |
adaptiveFpsCapColor() extracted from the inline lambda |
Yes | In scope; new exported helper |
src/gui/ConnectionPanel.{h,cpp} |
adaptiveThrottleEnabledChanged(bool) signal + emit |
Yes | In scope; new signal (internal API) |
src/gui/TitleBar.cpp |
currentBeatColor() returns invalid instead of fixed green |
Yes (incl. the amber discovery pulse consequence) | In scope |
tests/unified_title_bar_test.cpp |
19 checks: dot pixel, session gate, chip, bridge fields, width | Yes | In scope |
docs/{architecture/window-chrome,automation-bridge,user/docs/menu-reference,user/docs/low-bandwidth-connections}.md |
Documents the above | Yes | In scope |
No unrelated files, no formatting churn, no deleted guard, no CHANGELOG.md entry, and the - lines are confined to the lambda removal and the currentBeatColor() body. The checklist's "No new flat-key AppSettings calls" is accurate: MainWindow.cpp:729-730 reads the existing AdaptiveThrottleEnabled key and writes nothing.
Socket tests: none added, removed or modified. unified_title_bar_test opens no socket; the only network-looking token is the string fixture connected.transport = QStringLiteral("192.168.1.21") at tests/unified_title_bar_test.cpp:466, which is display text.
Security audit (required by the harness, 97 CodeGuard findings): ran, no findings. Every flagged ID is a lexical match with no security meaning — CS-VLN-CMDI-BACKTICK on the backticked identifiers in your new header comments (src/gui/RadioTabBar.h:186), CS-VLN-CMDI-AMP on & as address-of in connect() (src/gui/MainWindow.cpp:2173), CS-VLN-SSRF-PRIV-*/CS-PII-IP on the setPlaceholderText example addresses at src/gui/ConnectionPanel.cpp:2554-2557 (present verbatim on main, never dialled), CS-VLN-PATH-PROC on the pre-existing /proc/self/status RSS read. The new surface is bool/int/QColor moved between widgets: no parser, no sink, no stylesheet, no path. Credit: security-audit (CodeGuard).
3. Blockers
1. GOVERNANCE.md — the quality-coloured dot is a visual-design change with no approved RFC. Not a code defect; a process gate, and you flagged it yourself, so this is mostly me putting the citation on the record for the maintainer.
GOVERNANCE.md:97:
- Any change to visual design — colors, fonts, spacing, theme, icons
and GOVERNANCE.md:122,131:
Open a GitHub issue with the label
rfcand the title prefix[RFC]. […] Do not open a PR until the RFC issue is approved.
plus the normative agent limit at GOVERNANCE.md:221-223:
May not autonomously change: visual design (colours, fonts, layout, theme) […] The Project Maintainer is the sole authority on visual design and UX direction.
#6790 is labelled enhancement/GUI, has no [RFC] prefix, no rfc label, no maintainer comment, and postdates the PR, so it is not the approval that sentence asks for. The GOVERNANCE.md:110-118 exemption for "bug fixes with a clear root cause" plausibly covers items 1 and 3 — the AT chip restores a cue #6198 dropped, and that's a regression with a named root cause. It does not cover item 2: the live-grade palette on the tab dot is new, as your own correction says. Three distinct palette decisions need the ruling, not one: the dot recolour, the chip's filled tint, and the glow now following the dot (amber discovery pulse). What a fix looks like: either relabel #6790 as [RFC] and wait for @ten9876, or split item 2 (RadioTabBar.cpp:314-316, the showQuality gate at RadioTabBar.cpp:874-878, and the TitleBar.cpp:1669-1671 glow change) into a follow-up PR and merge the chip + words now, which stand on the regression exemption alone.
4. Nits (non-blocking)
-
src/gui/MainWindowHelpers.cpp:153-158/src/gui/RadioTabBar.cpp:314-316— I measured the contrast you left unmeasured, and the light theme is the problem. Compositingcolor.titlebar.tab.active.background(#1a0088b0) overcolor.titlebar.background(#ebf5f5f8) gives an effective#ddebf2; dark resolves to#0c2636. Ratios for the dot:Grade light #ddebf2dark #0c2636Excellent/Very Good #00cc661.75:1 7.30:1 Good #00b4d82.02:1 6.33:1 Fair #cc99002.12:1 6.03:1 Poor #cc33334.22:1 3.04:1 replaced color.titlebar.status.connected4.10:1 ( #1a8040)8.48:1 ( #4dd87a)Dark is fine everywhere. In the light theme three of four grades land below
docs/a11y.md:144-146's 3:1 for non-text, and the token they displace passes at 4.10:1 — precisely because the token is theme-switchable and these literals are not. I agree with your nit-4 answer that retokenisingnetworkQualityColor()would drag the status bar in and is out of scope;docs/a11y.md:155explicitly allows that ("concrete hex is a valid intermediate state — file a follow-up issue to add the token"), so the ask is just the follow-up issue, with these numbers in it. The spelled-out grade in the tooltip already covers WCAG 1.4.1; this is 1.4.11. -
docs/automation-bridge.md:3243-3249vssrc/gui/RadioTabBar.cpp:1359— nit 6 isn't fixed; the doc now states something the code doesn't do. The doc saysradios.adaptiveThrottlemeans "the chip is showing", butstate()publishes the ungated flag:{QStringLiteral("adaptiveThrottle"), m_throttleEnabled},m_throttleEnabledis the setting (OR'd with an in-force cap), with no session gate, while the chip additionally requiresours && sessionatRadioTabBar.cpp:874-884. Your own first bridge proof records the divergence: "Disconnect: … the tab showed no chip, whileadaptiveThrottlestayedtrue(the setting itself)." That's the common state — box ticked, nothing connected: fieldtrue, everytabs[].throttleChipfalse. Cheapest fix is the doc, sincetabs[].throttleChipalready carries the per-tab truth: reword to "the throttle is armed for this client (ticked, or a cap still in force after unticking);tabs[].throttleChipsays whether a tab is actually showing the chip." -
src/gui/MainWindow.cpp:7174-7175— the disconnect path clears the cap locally but never pushes it to the bar.m_adaptiveThrottleActive = false; m_adaptiveFpsCap = 0; // clear cap alongside throttle flag — see #2829 review
RadioModel::stopNetworkMonitor()emits onlynetworkQualityChanged(src/models/RadioModel.cpp:8740); the compensatingadaptiveThrottleChanged(false, 0)lives instartNetworkMonitor()(src/models/RadioModel.cpp:8639), i.e. the next connect. So between disconnect and next connectRadioTabBar::m_throttleFpsCap/m_throttleCapColorkeep the dead cap andradios.adaptiveFpsCappublishes it. Reasoned from code, not reproduced: the chip itself stays correctly hidden by the session gate, so this is bridge state only. Drop-in:m_adaptiveThrottleActive = false; m_adaptiveFpsCap = 0; // clear cap alongside throttle flag — see #2829 review refreshAdaptiveThrottleChip();
-
src/gui/TitleBar.h:249— comment invalidated by this PR's own change.QString m_throttleFlashColor; // empty = default green; set while adaptive throttle is activeAfter
TitleBar.cpp:1669-1671, empty means "glow in the dot's own colour", not green. Suggest:// empty = glow in the dot's own colour; set while adaptive throttle is active. -
src/gui/MainWindowHelpers.cpp:153—adaptiveFpsCapColor(0)returns Poor red. Unreachable today (both call sites guardcap > 0, same shape as the lambda it replaced), but it's now an exported helper in a header rather than a lambda two lines from its only caller, so the precondition is worth stating in theMainWindowHelpers.h:66-68comment.
5. What I tried to break (no finding)
- Chip overflowing a narrow tab.
nameWfloors at0.0(RadioTabBar.cpp:467), so I went looking for a squeeze that would push the chip pastkTabPadX. There isn't one:RadioTabBar.cpp:919pinsm_tabHosttom_tabsLayout->sizeHint().width()and the strip scrolls instead of shrinking, andsizeHint()already budgetschipSize().width() + kChipGap(RadioTabBar.cpp:344-347). The floor is unreachable and the chip can't overlap the name or the tab edge. Your 195 px grab agrees. - Per-tick churn on the a11y surface.
applyLinkVisuals()runs on every 80 ms pulse tick, and it now callssetLinkQuality/setThrottleChipon every tab. Both no-change-guard beforerefreshAccessibility()(RadioTabBar.cpp:290-316), andchipTextis built only when a tab can show it, so the steady state is zero tooltip/accessible-name writes and zero repaints.docs/a11y.md's throttled-update rule is satisfied. - A black glow from the
currentBeatColor()change. Returning an invalidQColorcould have painted an invalid gradient;RadioTabBar.cpp:429falls back todot(m_beatColor.isValid() ? m_beatColor : dot), which is the whole point of the change. The amber discovery pulse follows from the same line and is disclosed. - A stale grade surviving a disconnect or following the next radio.
stopNetworkMonitor()emitsnetworkQualityChanged(networkQuality(), 0)withm_netStatealreadyOff(src/models/RadioModel.cpp:8718-8740), which maps to an invalidQColoratMainWindow.cpp:2151-2153;dotColor()additionally requiresRadioTabStatus::Connected, andapplyLinkVisuals()requires a non-emptym_activeId. Three independent gates — I couldn't get a grade onto a tab that isn't the live session's. - Construction order hiding the initial push.
refreshAdaptiveThrottleChip()atMainWindow.cpp:2210is guarded byif (!m_titleBar) return;, which would silently no-op if it ran first. It doesn't:buildUI()runs atMainWindow.cpp:1422and createsm_titleBar(:5469) andm_connPanel(:5527), both before the ctor's connect block. So a ticked box shows the chip from launch, and them_connPanelconnect at:2208isn't dereferencing a null. - Unticking mid-cap.
applyAdaptiveFrameRate()returns early when the setting is off (src/models/RadioModel.cpp:8991-8992), so noadaptiveThrottleChanged(false, 0)ever arrives andm_adaptiveFpsCaplegitimately stays> 0. The corrected comment atMainWindow.cpp:731-733now matches the code; the earlier "next quality update" wording did not. - The "one shared mapping" claim.
NetworkDiagnosticsDialogconnectsadaptiveThrottleChanged(:2363) and was the obvious place for a second cap→colour table.grepfinds only chart-series colours there (:3455,:3473,:3501,:3612) — no sibling tier mapping left behind, so extracting the lambda really did unify every consumer. - The pixel probe. The usual "exact-RGB probes are font-dependent" objection doesn't apply here: the dot is an opaque filled 7 px ellipse at a fixed
kTabPadX, the glow and the active-tab tint both paint under it, and the probe samples the centre. The three-state sequence (plain → quality → plain) plus your mutation counts (4 and 2) is real evidence, and it matches the gates I read atRadioTabBar.cpp:874-884. - CI, honestly. At
af06e03b,build,check-windowsandcheck-macoswere stillin_progress;Static checksandSanitizer option configurespassed. I confirmed your CI-gap disclosure independently:unified_title_bar_testappears in noctest -Rlist in any workflow, so green proves compilation only. Your local run and the bridge proofs are the only evidence these 19 checks pass, and I can't re-run them — everything above is reasoned from the source, not observed at runtime.
6. Recommendation
Needs maintainer decision. The engineering is in good shape: the session gate, the no-change guards, the sizeHint budget and the glow fallback all survived the attacks above, the extracted helper leaves no sibling mapping behind, and the nits are a doc sentence, a stale comment, one missing refresh call and a follow-up issue. What's unresolved is the ruling GOVERNANCE.md requires before the dot recolour, chip tint and glow change can merge — and that's @ten9876's call, not mine. Concrete next step: relabel #6790 as [RFC] with the rfc label and the four RFC headings, including the light-theme contrast numbers above; or, if you'd rather land something now, split item 2 out and merge the chip and the spelled-out words, which the regression exemption covers on their own.
🤖 aethersdr-agent · cost: $10.6860 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (100 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: $11.1323 · model: claude-opus-5 |
…t, and correct three comments and docs. Principle VIII. From the PR review: - MainWindow: the disconnect path cleared m_adaptiveFpsCap but never pushed it to the bar, so radios.adaptiveFpsCap kept the dead cap until the next connect. Call refreshAdaptiveThrottleChip() there. - automation-bridge.md: radios.adaptiveThrottle is the armed flag, true with nothing connected; tabs[].throttleChip is the per-tab truth. - TitleBar.h: an empty flash colour now glows in the dot's own colour, not green. - MainWindowHelpers.h: adaptiveFpsCapColor() expects a cap in force; 0 maps to Poor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the re-review. Blocker (RFC): #6790 is now an RFC: retitled Nits, in
Local test run: |
nigelfenton
left a comment
nigelfenton
left a comment
There was a problem hiding this comment.
Reviewed at 11c57b80. The code is sound. The visual design is @ten9876's to rule on in RFC #6790, so this is a COMMENT, not an approval. I've put the contrast numbers on the RFC, where the ruling happens. One suggestion here: three checks that pin the "link state speaks first" ordering, which nothing currently tests.
Issue fit. The PR implements RFC #6790 items 1–3 as written. That covers the chip (outlined when armed, filled when capped), the dot taking the quality colour only beside the chip, and the words in the tooltip and accessible name. It also includes palette decisions (a), (b) and (c). The stale-chip defect from the triage is fixed in 11c57b80.
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
RadioTabBar.cpp/.h |
Chip painting and size, quality branch in dotColor(), words in descriptionLine(), session gate, bridge keys |
Yes | In scope; the visual surface is on RFC #6790 |
MainWindow.cpp/.h, MainWindowHelpers.* |
refreshAdaptiveThrottleChip(), quality push, adaptiveFpsCapColor() replaces the inline lambda |
Yes | In scope |
ConnectionPanel.* |
adaptiveThrottleEnabledChanged signal |
Yes | In scope |
TitleBar.cpp/.h |
currentBeatColor() returns invalid when unthrottled |
Yes (disclosed) | In scope; part of ruling (c) |
tests/unified_title_bar_test.cpp |
Dot pixel, session gate, chip, bridge checks | Yes | In scope |
docs/* |
Window chrome, bridge, menu reference, low-bandwidth | Yes | In scope |
Everything in the diff is explained by #6790. No settings keys and no CHANGELOG entry. The new titlebar.radios bridge keys are documented in automation-bridge.md.
What I ran. unified_title_bar_test isn't in ci.yml's per-PR list, so your local run was the only evidence it passes:
- Built the merge base (
1076c60e), your head, and your head merged with current main (a904cc79, merges clean) on Linux/GCC and macOS/Apple Silicon. The head was also built on Windows/MSVC. All Qt 6.12, app target included for head and merged. - Linux and macOS:
unified_title_bar_test: all checks passedon all three trees. - Windows: every check passes except "four radios fit whole in an 1800 px bar", which fails on the merge base too. That's a second Windows machine reproducing what you saw, while Linux and macOS pass it, so it is Windows-specific and pre-existing, not this PR. It does look worth the separate issue you mentioned.
Suggestion: pin the ordering in dotColor() and descriptionLine(). The PR relies on "discovery amber and link-lost red still take priority", and the comment in dotColor() says so. No check covers it with AT on. I made both mutations on Windows, and all of your new checks still passed under each:
- quality branch moved above the override branch in
dotColor(); && !m_alarmdropped fromdescriptionLine(), so a lost link keeps saying "Network Fair".
These three checks, after "with AT the dot takes the quality colour", catch both. Each fails only under its own mutation, and all pass on your head:
// Link state still speaks first (dotColor()): an override such
// as the amber discovery pulse wins over the quality colour,
// and a lost link stops announcing the old grade.
tabs->setLinkIndicator(QColor(QStringLiteral("#e0a020")), /*alarm=*/false);
check(dotPixel(sessionTab) == QColor(QStringLiteral("#e0a020")).rgb(),
"a link override still speaks over the quality colour");
tabs->setLinkIndicator(QColor(QStringLiteral("#cc3333")), /*alarm=*/true);
check(!sessionTab->toolTip().contains(QLatin1String("Network Fair")),
"a lost link does not keep announcing the old quality");
tabs->setLinkIndicator(QColor(), /*alarm=*/false);
check(dotPixel(sessionTab) == fair, "the quality colour returns once the link state clears");Nit: MainWindow.h:2076 declares refreshAdaptiveThrottleChip() in the middle of the data members. It would sit better with the other private methods.
What I tried to break that held:
- The stale chip from the triage.
refreshAdaptiveThrottleChip()now runs right afterm_adaptiveFpsCap = 0on disconnect, so an untick-with-cap followed by a reconnect no longer repaints a dead 8 fps chip. That path is inMainWindow, so I verified it by reading, not by test. - The glow change with AT off.
currentBeatColor()now hands back no colour, so the glow takescolor.titlebar.status.connectedinstead ofcolor.accent.success. Both bundled themes resolve those to the same value (#1a8040light,#4dd87adark), so with AT off nothing visibly changes. A user theme that sets them differently would see its glow change. That's worth a line under ruling (c), not a defect. - Excellent never reaches a filled chip.
adaptiveFpsCapColor()maps the caps to Poor, Fair or Good only. That matters for the contrast numbers on the RFC. - I did not drive the GUI. The demo backend never grades the link below Excellent, so the throttle can't engage there. Your two bridge sessions on the 6600 are the live evidence.
73, Nigel G0JKN/W3
… pin that link state speaks before the quality dot. Principle VIII. The capped chip drew "AT" in the cap's colour over a 22% fill of the same colour, 1.70-4.34:1 in both bundled themes against the 4.5:1 that text needs (Nigel G0JKN's numbers on RFC aethersdr#6790). The outline and fill keep the cap colour, so the tier still shows; the letters now use color.text.primary (7.3-10.2:1). Adds Nigel's three checks to unified_title_bar_test: a link override wins over the quality colour, a lost link stops announcing the old grade, and the quality colour returns once the link state clears. Moves the refreshAdaptiveThrottleChip() declaration in with the private methods. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks, @nigelfenton. Both suggestions are in Changes
Live proof via the automation bridgeBuild: fork Windows Installer run 41199940878 on State was read from
The cap goes on at the next grade change, so it engaged after a reconnect. On this LAN the link moved between Poor and Fair by itself, so the 4 fps / Poor cap is now covered live as well. Capped chip (Fair, 8 fps) in each theme:
Both bundled themes fall in your predicted 7.3–10.2:1. The third row is my own theme. It still passes 4.5:1, but amber letters on an amber chip show what happens when the letters follow the theme's text colour. Not driven live: the link-override and lost-link ordering. This LAN never produced a discovery pulse or a lost link during the run. Your three checks cover that order. The dot recolour and palette decisions (a)–(c) still wait on @ten9876's ruling in #6790. 73, CJ (WT2P) 👨🏼💻 Co-authored by Claude Opus 5.5 |
Resolves the overlap with aethersdr#6793 (merged): MainWindow keeps both restorePreCapPanRates() and refreshAdaptiveThrottleChip(), the throttle handler keeps aethersdr#6793's episode sets and restore and calls the chip refresh, and it uses adaptiveFpsCapColor() in place of the removed fpsCapColor lambda. low-bandwidth-connections.md keeps the AT chip bullet and aethersdr#6793's restore bullets. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The design is approved in #6790 (ruling): (a), (b) and (c), plus the chip and the tooltip text. One condition before merge: |
rfoust
left a comment
rfoust
left a comment
There was a problem hiding this comment.
Implements RFC #6790, including the written maintainer ruling. The earlier design-authority blocker is settled. The required shared tokenization is now in e6cc5e74457de3bd6968fba439b69aefd8c31d6d; the dark palette is unchanged and the approved light values are used by the status label, tab dot and cap tint.
Scope
| File/group | Change | In final PR body? | Verdict |
|---|---|---|---|
src/gui/RadioTabBar.{cpp,h} |
Session-only quality/chip, words, bridge fields, actual-width refresh | Yes | In scope |
src/gui/ConnectionPanel.{cpp,h} |
Signal after saving the existing throttle checkbox | Yes | In scope; no new settings key |
src/gui/MainWindow.{cpp,h}, MainWindowHelpers.{cpp,h}, MainWindow_Session.cpp |
Shared quality mapping and immediate chip/theme refresh; clear on disconnect | Yes | In scope |
src/gui/TitleBar.{cpp,h} |
Uncapped heartbeat glow follows the dot | Yes | Approved RFC item (c) |
Both bundled theme JSON files; src/core/ThemeSeedGenerated.cpp |
Five quality tokens, generated fallback seed | Yes | Required by the ruling |
tests/unified_title_bar_test.cpp, theme_manager_test.cpp |
Widget/state/pixel/geometry and contrast regressions | Yes | Socket-free, in scope |
| Five changed docs pages/references | User behavior, architecture, tokens and bridge fields | Yes | In scope |
No unrelated changes, CI edits, CHANGELOG entry, throttle-policy/default change, new radio command, or new GUI-to-engine include. The new bridge fields are documented read-only observations of this UI.
Findings disposition
No outstanding blockers or nits. Fixes in e6cc5e74457de3bd6968fba439b69aefd8c31d6d:
- P2, tab geometry:
src/gui/RadioTabBar.cpp:902refreshes the fixed-width scroll host when chip/status changes alter its preferred width;:1084sizes after the link visuals update. The unmodified full app kept a 224 px tab while requesting 226 px, eliding its name. The fixed app gives it 226 px; the strip's actual widths plus spacing match its 423 px content width. This also fixes the “Link lost” status path. Removing the geometry fix makes 11 assertions fail; restoring it passes. - RFC condition / light contrast:
src/gui/MainWindowHelpers.cpp:135resolves onecolor.networkQuality.*mapping. Dark remains exactly#00cc66/#00b4d8/#cc9900/#cc3333; Light uses#1a8040/#0088b0/#a85a00/#c02020; Off/unknown retains#8aa8c0.MainWindow_Session.cpp:120refreshes label, dot, chip and cap tint on a theme change. The contrast regression attests/theme_manager_test.cpp:1786passes both themes and fails if the old light Excellent hex returns. The PR body records Fair status text at 4.68:1, Good at 3.75:1, and the unchanged neutral Off at 2.29:1 against the sampled#f5f5f8status background, within the maintainer's explicit ruling. - Documentation accuracy: corrected the old-lamp and accessible-description comments, documented the shared tokens and actual tab resizing, and kept the new MainWindow work in its Session translation unit.
Nigel's chip-letter and override/alarm-priority requests were already implemented in 803ade43; I retained and executed those checks. The disconnect cap reset from 11c57b80 is retained. The old bot governance objection is retired by the written RFC ruling. The advisory CodeGuard entries are lexical candidates, not demonstrated source-to-sink vulnerabilities; the cited literal/documentation patterns do not establish an exploit in this diff. Antares' suggested NetworkDiagnostics.{cpp,h} files are unchanged by this PR. No additional security defect was validated from those reports.
There are no inline review threads to resolve and no CHANGES_REQUESTED reviews to dismiss.
Verification and limits
- Reviewed head:
e6cc5e74457de3bd6968fba439b69aefd8c31d6d. - Fresh-main local integration: base
57efaf8836a9c5d0ea140eebe09386893e041656, tree3a6dd4bf49b416548cde78fff1292fea0b9a2eb8. GitHub merge ref58e1f52b69da87e401ed86b76bf7e40aa9cdfea7has that same base/head and exact tree. No merge-from-main commit was pushed. - ARM64 RelWithDebInfo app built with the required toolchain and RADE enabled. Host/system/executable are ARM64; no RNNoise x86 sources. Socket-free
unified_title_bar_testandtheme_manager_testpass on both the final head and integration result, including after both mutation restorations. - Architecture/manifest/capability/command ratchets pass without baseline growth; generated theme seed and docs references pass; 767 log-rule checks pass. Docusaurus production build passes. Pre-existing log-source line hints concern unchanged files and were not bundled here.
- Full-app integration instance PID 20810 used offscreen UI, isolated settings, explicit socket and
AETHER_AUTOMATION_NO_TX=1;whoamireportedtxAllowed:false. Connected only toDEMO-0001. Verified armed chip, full tab width, dark-to-light live color refresh, tooltip words, disconnect/reconnect and session-only state. Head instance PID 10071 additionally proved throttle/theme persistence across process restart and simulator-disconnect recovery. Every owned instance closed normally. - All seven PR commits are GitHub-verified signatures. Required
build,check-windows,check-macosandStatic checksare green on this head; Docs and sanitizer configuration checks also pass. CI run 41564215543 checkout logs confirm merge58e1f52b69da87e401ed86b76bf7e40aa9cdfea7, matching the locally tested integration tree. Current main is still the tested base. The independent post-squash main run is checked after landing.
The demo never degrades below Excellent or engages a cap. Final live cap convergence/RF behavior is therefore unverified; cap state, paint, words and contrast are covered by socket-free tests. unified_title_bar_test is compiled but not executed by the per-PR CTest filter, so its local pass is explicit evidence. A separate code-review tool was unavailable; the second adversarial pass was manual.
What I tried to break
- AT on/off/on, disconnect/reconnect, no-session and other-radio gates, active-tab overflow, short names dominated by alarm text, and actual widget width rather than only
sizeHint(). - Discovery/alarm overrides against a quality-colored dot, suppression of stale quality words during link loss, armed versus capped paint, and the retained cap when its checkbox is unticked.
- Both bundled themes' dot/border and chip-letter contrast, live theme refresh, settings across a process boundary, and simulator fault recovery. Both intentional mutations failed as expected and restored builds passed.
Approving the pinned head with the recorded demo/hardware limits.
Closes #6790.
Behavior
The connected radio's tab shows an AT chip when Enable adaptive frame-rate throttle is enabled. It is outlined while armed and filled in the cap's quality color while capping. Unticking the checkbox does not lift an existing cap; the filled chip remains until disconnect, matching the existing throttle policy.
While the chip shows and the link is healthy, the dot shows network quality: green for Excellent/Very Good, blue for Good, amber for Fair and red for Poor. With AT off, it uses the normal connected green. Discovery amber and link-lost red take priority. Heartbeat glow follows the dot unless a cap supplies its tint. Tooltips and accessible descriptions spell out the grade and throttle state.
The tab's actual width follows chip and status-text changes, including disconnect/reconnect and the “Link lost” alarm. Narrow title bars scroll the tabs rather than squeezing away the chip.
This adds the chip and live-grade cue to the unified title bar. The old standalone lamp displayed the cap tier only; it did not display live network quality. Quality thresholds, throttle policy, defaults and settings ownership remain unchanged.
Approved palette
Implements @ten9876's RFC ruling. One
networkQualityColor()mapping readscolor.networkQuality.*tokens for the status label, dot andadaptiveFpsCapColor(). Theme switches refresh them immediately, including an engaged cap's glow tint. Both bundled themes and the generated fallback seed define the five tokens.excellent)#00cc66color.green.500/#1a8040good)#00b4d8color.blue.500/#0088b0fair)#cc9900#a85a00poor)#cc3333color.red.500/#c02020off)#8aa8c0#8aa8c0Status-label ratios use WCAG relative luminance against the actual Default Light status-block background sampled from the full app (
#f5f5f8). Fair is calculated from the approved token, not a live Fair simulator grade. Good remains below 4.5:1; Off retains the neutral value, as the ruling permits. The grade dots and cap outlines meet 3:1 against the active-tab background in both bundled themes. Filled-chip letters usecolor.text.primaryand meet 4.5:1 against their 22% tinted fill.Documentation and bridge
Updates the menu and low-bandwidth user guides, window-chrome architecture, canonical tokens and automation-bridge reference. Bridge
titlebar.radiosexposeslinkQuality, resolvedlinkQualityColor,adaptiveThrottle,adaptiveFpsCapandtabs[].throttleChip. Existing screenshots of the normal AT-off title bar remain representative; the new armed/capped state is described in the guides.Verification
unified_title_bar_testandtheme_manager_test: both pass. Coverage checks actual tab/host geometry on AT on/off, reconnect, link-loss text and narrow strips; dot/override/session behavior; chip paint and words; both themes' contrast.main(base and integration tree recorded in the approving review).DEMO-0001. Verified checkbox toggles, light/dark theme changes, tooltip/bridge state, disconnect/reconnect, simulator disconnect fault, and settings persistence across a process restart. All owned instances closed normally; TX explicitly disabled throughout.unified_title_bar_testis compiled but is outside the per-PR CTest filter; the local executed result above is required evidence. The simulator remains Excellent and does not engage an adaptive cap, so final live cap convergence and RF behavior were not tested. Cap rendering/state are covered socket-free; the contributor's earlier FLEX-6600 bridge proof is historical evidence, not a claim about this final build.