Repository navigation
fix(gui): availability registry keeps own tooltips. Principle VIII. - #6253
Conversation
ControlAvailabilityRegistry::applyOne wrote an empty tooltip, and a state description, whenever a control was not Unavailable. Every registered control therefore lost its own tooltip and accessible description (QAction: tooltip and status tip) on the radios that support it, and RxApplet re-set its squelch texts by hand afterwards. The registry now holds a text channel only while it shows text of its own there: the reason while Unavailable, and the "Available, not currently active" announcement only on a widget with no description of its own. Anything the owner wrote since the last apply is the control's own text and is what comes back when the registry lets go; an owner write while the reason shows is replaced by the reason at the next apply. The hold is kept on the control as a dynamic property, so a registry re-created while the reason shows (RxApplet and VfoWidget rebuild theirs in setRadioModel) still restores the own text. RxApplet's applySqlModeVisuals() after refreshEngaged() stays: it no longer has to restore the tooltip, but it also re-derives the slider's threshold description and the Auto SQL text, which change while the registry holds the channel. Tests: a pre-tooltipped, pre-described widget and QAction through Available -> Unavailable -> Available, owner writes in both states, a re-created registry, an action keeping its text-derived tooltip; and RxApplet's squelch button texts surviving registration and a capability round trip with no slice, a path where RxApplet re-sets nothing. Fixes aethersdr#5859 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed at 568521d1, against the PR head checkout. I could not build or run anything, so every finding below is reasoned from code, not reproduced at runtime; I say so where it matters.
1. Issue fit — yes
#5859 was re-titled on 7 Oct to "ControlAvailabilityRegistry::applyOne clears a control's own tooltip whenever it is not Unavailable", which is exactly this defect, and the thread already converged on it: the triage comment listed it as item 1 with the fix shape ("preserve the widget's own tooltip on the Available path instead of writing QString()", plus "add a test registering a widget that already has a tooltip"), and you offered to write it. The issue body is stale — it argues about zero call sites, which #5924 settled — but the title and the thread are the live ask, so Fixes #5859 is right. The maintainer may still want to read the issue body's "option 3 / header staging note" as separately closed: the header rewrite here covers it, since the staging note is moot once call sites exist.
The fix also lands the stronger half of the thread's ask. Your test asserts restoration across Unavailable→Available, not merely preservation across registration — the distinction you drew yourself in that thread ("those are different bugs and only the second is the one that bites"). The mechanism is wired correctly:
| Requirement from the thread | Where |
|---|---|
Available path stops writing QString() over the widget's tooltip |
src/gui/ControlAvailabilityRegistry.cpp:36-59 (applyText), :147-151 |
Same treatment for the QAction branch |
:177-191 |
| Test registers a pre-tooltipped widget and asserts restoration | tests/control_availability_registry_test.cpp:296-371 |
| Doctrine written down | docs/style/theme-style-guide.md:185-187 |
Test layer: both are socket-free CTest, the right layer (GUI state machine, injected capabilities). Neither adds a socket, peer process, or Fake* radio/amp/tuner class — I grepped both test files for QTcpServer/QUdpSocket/QLocalServer/bind(/listen(/connectToHost/Fake and got nothing. rx_applet_squelch_reconciliation_test is already registered (tests/tests.cmake:7467-7493) and the three new includes are headers, so no link change is needed.
One thing to know about the green badge: neither test is in any ctest -R filter in ci.yml (the only filtered steps are the DV, cross-needle, mac-NR and ASR ones). So the five green checks prove these files compile on all three platforms — which is real and is where _USE_MATH_DEFINES/NOMINMAX-class breaks would show — but they did not execute either new test. Your local 653/657 run is the only execution evidence, and I can't verify it.
2. Scope
| File | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/gui/ControlAvailabilityRegistry.cpp |
describe() → applyText() hold mechanism; widget + action text channels |
Yes | In scope |
src/gui/ControlAvailabilityRegistry.h |
registerWidget doc comment restated for restore semantics |
Yes | In scope |
docs/style/theme-style-guide.md |
+4 lines under the three-state table | Yes | In scope (see §4) |
tests/control_availability_registry_test.cpp |
+78, the restoration cases | Yes | In scope |
tests/rx_applet_squelch_reconciliation_test.cpp |
+35, real-applet round trip | Yes | In scope |
Everything in the diff is explained by the issue. No new public surface: no protocol verb, no settings key, no capability field, no CLI flag, no bridge verb. The two dynamic-property keys (ControlAvailabilityRegistry.cpp:61-62) are new internal state on the controls, but they are private to this TU — I grepped the whole head checkout for aetherAvailability and those two lines are the only hits, so nothing else reads them and no QSS selector can be relying on them. No CHANGELOG.md entry, correctly.
On the - lines: the deleted describe() helper's comment cited #4896 and #5266/#5299 for why the reason rides both channels. The reasoning survives in condensed form at :144-146, and the #5266/#5299 history is still in theme-style-guide.md:189-196. No guard was removed — setEnabled(!unavailable) and the reason-on-both-channels invariant both still fire on the Unavailable path. Not a regression.
3. Blockers
None.
4. Needs maintainer decision (one item, and it is narrower than it looks)
src/gui/ControlAvailabilityRegistry.cpp:158-165 + docs/style/theme-style-guide.md:182
return entry.state == ControlAvailability::Inactive && own.isEmpty()
? QObject::tr("Available, not currently active") : QString();The style-guide table immediately above your new prose still reads unconditionally:
| inactive | supported here, not engaged now |
color.control.inactive| "Available, not currently active" |
That row encodes the #5262 M3a ruling, and your three lines at :185-187 qualify it three lines later. A reader hitting the table first gets the old rule. You flag this as the maintainer's call in the PR body, which is the right handling — I'm just locating it and sizing it.
It sizes small. I walked every current call site to find where today's behavior actually changes:
RxApplet.cpp:2100/VfoWidget.cpp:6686(m_sqlBtn) —applySqlButtonDescription()/syncSqlVisuals()already write an own description and already won on main. No delta.RxApplet.cpp:2103/VfoWidget.cpp:6689(m_sqlSlider) — own description written bydescribeThreshold. No delta.WfmApplet.cpp:275-295— every engaged predicate is[]{ return true; }, so these are never Inactive. No delta.ReceiveCaptureAction.h:19— aQAction; the inactive phrase never went tostatusTipon either side.RtlReceiverSettingsWidget.cpp:82(m_dc) — the one site with a real engaged predicate, and it sets noaccessibleDescription(onlym_ppmgets anaccessibleName, line 43), soown.isEmpty()is true and the phrase is still written.
So as far as I can tell the user-visible delta today is zero; the ruling matters for M3b conversions, not for 26.10.x. Worth stating in the body.
And the append alternative you offer needs no ruling at all. Appending the phrase to the widget's own description satisfies the table row verbatim while still restoring the own text, which is the whole point of the PR. I'd suggest making that the default and letting the maintainer decline it — it turns a doctrine amendment into a pure bug fix. If they prefer the conditional form, the table row at :182 should pick up the qualifier so the section doesn't read two ways.
5. Nits (all non-blocking)
-
src/gui/ControlAvailabilityRegistry.cpp:177-184— the action-tooltip restore freezes a stale derived default if the action's text changes while the reason is held. The comment says:// An action with no tooltip of its own reports its text; clearing first
// keeps that default instead of freezing a copy of the text.That holds only when the text is unchanged across the hold. Walk it: action text
&Tune, no explicit tooltip, soownis captured as"Tune". While Unavailable the owner callssetText("&Stop"). On restore,set("Tune")doessetToolTip(QString())→toolTip()is now"Stop"→"Stop" != "Tune"→setToolTip("Tune"). The action is left with an explicit, wrong tooltip and no longer follows its text. Yourplaincase attests/control_availability_registry_test.cpp:363-370renames after restore, so it doesn't reach this. Narrow — I found no production action that renames while gated — but the comment overstates the guarantee. Worth either narrowing the comment or recording "had no explicit tooltip" as a third hold element instead of the derived string. -
src/gui/ControlAvailabilityRegistry.cpp:36-59— nothing releases a hold on destruction.RxApplet.cpp:2078-2079doesdelete m_filterAvailability;andVfoWidgetrebuilds likewise, so a registry can die while a control is Unavailable: the control keeps the reason, the disabled state, and an orphaned hold property. A later registry recovers it (yoursecondcase at:336-344proves that), butsetRadioModel(nullptr)builds no registry at all and the control stays dimmed with a reason that no longer applies. Pre-existing — main left the same stuck state — but now thatownis stored, releasing in the destructor is a few lines, where before there was nothing to release to. -
src/gui/ControlAvailabilityRegistry.cpp:44— an empty-but-non-nullreasonsilently reinstates the bug. The release sentinel iswant.isNull(), so a caller passingQStringLiteral("")(non-null, empty) makeswantnon-null and the registry holds an empty tooltip over the control's own — exactly the #5859 defect, through the front door. The header saysreasonis REQUIRED butregisterWidgetonly rejects a null widget/predicate. Every current call site passes a realtr()string, so this is hardening, not a live defect; an earlyif (reason.isEmpty()) return;or a comment naming the sentinel would close it. -
The PR body's
applySqlModeVisuals()paragraph covers the button, not the slider. You write that the call "no longer has to restore the squelch tooltip". Form_sqlBtnthat's right —applySqlButtonDescription()has thestateOf(...) == Unavailableguard (RxApplet.cpp:1443-1447), andVfoWidget::syncSqlVisuals()has the matchingsqlReasonOwnedguard (VfoWidget.cpp:6451-6453). The slider has no such guard:RxApplet.cpp:3259-3260runsrefreshEngaged()thenapplySqlModeVisuals(), and inSqlMode::Offthe slider is Unavailable (its predicate issupportsSquelch && m_sqlMode != SqlMode::Off) while thecase SqlMode::Off:arm at:1424-1428writesm_sqlSlider->setToolTip( "Squelch threshold. Increase to require a stronger signal " "before audio opens.");
unconditionally and last. So the slider's reason (
"Enable squelch in a supported receive mode to adjust its threshold") never reaches its tooltip. Identical on main — the owner won there too — so not a regression and not this PR's job. But the new rule makes it permanent rather than incidental: the owner's write becomes the recordedown, so restoration correctly gives back the generic text and the reason is never shown. Worth a sentence in the body so a later reader doesn't take "the registry now owns this correctly" to mean the slider's reason is reaching anyone. -
The own-text heuristic is the #5484 collision surface.
applyTextinfers "anything ≠ what I last wrote is the owner's text". #5484's proposedQEvent::ToolTipChangefilter re-annotates tooltips after the write, so the suffix would be adopted asownand then re-appended on the next restore. You disclose that the two are untested together; I'd name it as the thing to settle before #5484 lands, since the triage comment on #5859 already asked for a composition order and this PR is now the incumbent. -
docs/style/theme-style-guide.md:233-235(adjacent, untouched) still reads "M3a introduces the mechanism; those production consumers and their second pushes have not yet migrated (M3b)." Stale since #5924. Out of scope — flagging only because you're in this file.
6. What I tried to break and could not
- Null/empty round-trip through the dynamic property.
QStringList{QString(), reason}→QVariant→toStringList(): a widget with no prior tooltip stores a nullown, and the release path'sset(own)clears correctly.hold != nextcompares withQString::operator==, which treats null and empty as equal, so a null↔empty flip just skips a redundant property write. No stuck state. - Registry re-creation mid-reason, which is the design's whole risk. Traced
:296-345by hand: the innersecondregistry readshold.at(1) == current, recoversown, and releases to the owner's latest text rather than adopting the reason. The outer registry is still connected to the same model butrefreshEngaged()emits nothing, so it isn't re-entered. The test's final assertion is the one that would fail on a wrong hold, and it checksmeanwhile(the write made while unavailable), not the original — the harder of the two. - Duplicate and overlapping registrations.
RxApplet.cpp:3362andVfoWidget.cpp:5932registerm_filterUnavailablefrom a function that re-runs on mode change; the widget isdeleted and rebuilt each time andpruneDead()runs first inapplyAll, so no dangling entry. Two registries over the same widget share one property key, but with equalownand equalreasonthe writes are idempotent.RxApplet's andVfoWidget'sm_sqlBtnare distinct objects, not one widget seen twice. - Owner-write ordering in both directions. If
RxApplet's owncapabilitiesChangedhandler runs before the registry's lambda, itsstateOfguard still sees the stale Inactive and writes the own description — which the registry then correctly records asownand covers with the reason. If it runs after, the guard sees Unavailable and returns. Both orders land in the same place; I could not construct a connection order that strands the reason or loses the own text permanently. - The inactive-announcement change against every live call site. Enumerated in §4 — five sites, four with no behavioral delta and one (
m_dc) where the phrase is still written. I looked specifically for a site that would silently stop announcing today and did not find one. - Lifetime.
applyTextbindscurrentto the temporary returned byw->toolTip(); the binding lives for the full call expression, so no dangling reference.EntryholdsQPointers andpruneDead()precedes iteration. - The tests against a broken fix. The registry test's first new assertion fails on main at registration (main's Available path writes
QString()with a real tooltip present), and themeanwhile/second-registry assertions each pin a distinct part of the hold, so none of them passes against an emptiedapplyText. I could not find a check that asserts the implementation back to itself. Not reproduced — I have no build. - Could not check: anything requiring execution or a display. No Linux/Windows run, no screen reader, no confirmation that your 653/657 figure or the five mutation results are as described. The CodeGuard section and the Copilot section in my brief were both empty, which means no findings or no scan — I'm not reading either as a clean bill.
7. Recommendation
Approve with nits, with the inactive-announcement question routed to the maintainer. The defect is real and was already triaged as item 1 on #5859; the fix is well-shaped, the hold-on-the-control choice is the right one given that RxApplet/VfoWidget delete and rebuild their registries, and the tests finally cover the case the old fixture was structurally blind to. All five files are explained, nothing public was added, and no guard was removed.
Concrete next step: offer the append form of the inactive announcement (phrase appended to the control's own description). That satisfies theme-style-guide.md:182 verbatim and removes the only item on this PR that needs a ruling — if the maintainer prefers the conditional form you've written, the table row should pick up the qualifier. The nits are all optional; the action-rename one and the empty-reason sentinel are the two I'd actually spend lines on. Thanks for tracing this from your own triage thread through to a test that fails for the right reason.
🤖 aethersdr-agent · cost: $9.6850 · model: claude-opus-5
Review on aethersdr#6253. The inactive announcement is now appended to a control's own accessible description instead of being written only on a control without one, so the three-state table's "Available, not currently active" holds verbatim and no doctrine change is needed. The style-guide lines under the table say so. An action with no tooltip of its own is now held as a null own text, so releasing the hold makes it follow its text again. Before, the derived text was captured, and an action renamed while unavailable (&Tune to &Stop) was left with an explicit, stale "Tune" tooltip. An empty reason now releases instead of holding an empty tooltip over the control's own text. Tests: the inactive append (with and without a closing full stop, and an owner write while inactive), an action renamed while unavailable, and an empty reason. The squelch reconciliation test expects the appended form on the Inactive squelch button. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. Bad news first. The append form has one gap. An owner that rewrites its description right after the registry applies drops the inactive phrase until the next apply. RxApplet does exactly that for the squelch button: The inactive announcement, now the append form (your concrete next step). Done in The action tooltip that froze a stale default (nit). Confirmed by test, not just by reading. On The empty-but-non-null reason (nit). Done. An empty wanted text now lets go instead of holding, so an empty reason leaves the control's own tooltip in place rather than blanking it. The control is still disabled. New test case. Not done, and why:
Tests. Full ctest on macOS (Qt 6.12.0, offscreen), head Red on mutation (scratch commits, never pushed, full suite each):
Not verified: the empty-reason case was not mutation-tested. No Linux or Windows run, no screen reader, and the app was not launched. As you note, CI does not run either test. 73, ON8ST |
Ozy311
left a comment
There was a problem hiding this comment.
Re-review of abb02d71, the one commit since the aethersdr-agent review at 568521d1.
The one item that needed a ruling no longer does. The inactive announcement is now appended to the control's own description (withInactivePhrase(), ControlAvailabilityRegistry.cpp:67-78) instead of being used only when the control has none. That satisfies the style-guide table row ("Available, not currently active") verbatim, and the prose under the table now says the same thing (theme-style-guide.md:187). The review suggested this as the version that needs no maintainer decision.
Disclosed behaviour change. Controls that have their own description now announce the phrase while Inactive. One example is the squelch button with squelch off: rx_applet_squelch_reconciliation_test now expects own + ". Available, not currently active". That is the M3a ruling being applied as written, not a preference, but a screen-reader user will hear it, so it may be worth a line in the PR body.
Two of the nits are fixed with tests:
- An action renamed while unavailable. The hold now records the action's explicit tooltip (null when it only reports its text), through
explicitToolTip(). On release it therefore follows the new text instead of freezing the old one. The new check renames&Tuneto&Stopwhile the action is unavailable and expectsStop, and thenGoafter a further rename. - An empty reason. The release sentinel is now
isEmpty(), notisNull(), soQStringLiteral("")can no longer blank the control's own tooltip. A new check covers it.
I traced the append path across repeated applies. While the phrase is shown, current == hold[1], so own comes from the hold and the phrase is never appended twice. An owner write in the meantime becomes the new own and gets the phrase once; the "owner write while inactive" check pins that. The branch merges cleanly with current main. Not run: either test, since both need the core build.
Still open from the earlier review, and fine as follow-ups: releasing the hold in the registry destructor, and how this combines with #5484's tooltip annotation. Nothing new from me.
73, K6OZY
Generated by Claude Code
Detect stored QAction tooltip ownership through ActionChanged while suppressing the probe notifications, and normalize owner writes when releasing a held channel. Preserve explicit tooltips equal to the label and let cleared tooltips continue following later labels. Add socket-free regressions for both cases, signal blocking, and transient notification suppression. Document restored descriptions and inactive announcements in the user accessibility guide. The new assertions fail against the original PR helper and pass with this change.
rfoust
left a comment
There was a problem hiding this comment.
Reviewed at 8266702aa against current main aa0dd0e4. This solves the narrowed #5859 tooltip-restoration defect, including Available -> Unavailable -> Available restoration, owner writes, and registry recreation. The user-guide omission and both QAction ownership edge cases from my review are fixed.
Scope
| Files | Change | Verdict |
|---|---|---|
src/gui/ControlAvailabilityRegistry.{cpp,h} |
Preserve/restore control-owned text; preserve explicit and derived QAction tooltip ownership | In scope |
docs/style/theme-style-guide.md |
State the text-composition rule | In scope |
docs/user/docs/accessibility.md |
Document restored descriptions and inactive announcements for users | In scope |
tests/control_availability_registry_test.cpp |
Restoration, owner-write, recreation, explicit/derived tooltip, blocked-signal and probe-notification regressions | In scope |
tests/rx_applet_squelch_reconciliation_test.cpp |
Real-applet own-text capability round trip | In scope |
Everything in the diff is explained by the issue and the review fixes. No new protocol/settings/CLI surface, unrelated restyle, changed default, removed safety guard, or changelog entry.
Blockers
None remaining.
Nits
None outstanding from this review. Explicit tooltips equal to the label now remain explicit. Clearing a held tooltip releases it to the current text-derived default and it continues following later renames. The capture probe suppresses temporary ActionChanged events/signals and preserves existing signal blocking.
Verification
- Independent macOS ARM64 build passed with RADE enabled. Host/system/executable are ARM64 and the Ninja graph contains no RNNoise x86 sources.
- Both socket-free focused CTests passed:
control_availability_registry_testandrx_applet_squelch_reconciliation_test. - Mutation proof: substituting main's pre-fix registry made both original regression targets fail. Substituting the original PR helper (
abb02d71) made all five new follow-up assertions fail; they pass with the reviewed fix. npm ci && npm run buildpassed fordocs/user; checked the generated accessibility page contains the final wording.- Offscreen baseline and updated app bridge sessions used fresh isolated settings, explicit sockets, and only the built-in demo. Selected
get radiofields:connected: true,model: "AetherSDR Demo",serial: "DEMO-0001",transmitting: false. Squelch tooltip/description text survived the exercised clicks and disconnect. The disclosed RxApplet owner-write ordering limitation remains unchanged. All review instances were closed. - No screen-reader, live-radio, Linux runtime, or Windows runtime validation is claimed. The demo cannot prove hardware capability convergence. No separate automated review pass was available.
What I tried to break
Text restoration after unavailable state, owner writes in either state, registry recreation mid-hold, repeated inactive composition, action rename before/after release, explicit text equal to the default, clearing while held, blocked signals, transient probe notifications, and empty reasons. The focused tests survived, and the inverted implementations failed as described.
Approved after the final push and fresh required CI passed on this exact head. No administrator bypass is requested.
Fixes #5859
Result
Supported controls keep their own tooltips and descriptions. While unavailable, the registry shows the reason on the tooltip and accessibility channel; when available again, it restores the owner's latest text. Inactive widgets append "Available, not currently active" to their own description.
Implementation and scope
The temporary text hold is stored on the control as
{own, shown}, so recreating a registry while the reason is showing preserves the original text. Owner writes in either state are adopted at the next apply. An empty reason releases the hold instead of blanking the owner's text.QAction tooltip ownership is detected through Qt's ActionChanged event, with temporary probe events and signals suppressed. An explicit tooltip remains explicit even when it equals the label. An action without an explicit tooltip keeps following its text, including after the owner clears the tooltip while unavailable. Existing signal blocking is preserved.
src/gui/ControlAvailabilityRegistry.{cpp,h}docs/style/theme-style-guide.mddocs/user/docs/accessibility.mdtests/control_availability_registry_test.cpptests/rx_applet_squelch_reconciliation_test.cppNo new protocol, settings, CLI, or capability surface is added. Current main
aa0dd0e4was merged without dropping upstream changes; the three follow-up commits are SSH-signed and GitHub-verified.Independent verification at
8266702aacontrol_availability_registry_testandrx_applet_squelch_reconciliation_test.abb02d71makes all five new follow-up assertions fail; they pass with this implementation. The unchanged squelch regression still passes againstabb02d71, as expected.npm ci && npm run buildpasses indocs/user; the generated accessibility page contains the final wording.DEMO-0001, using fresh isolated settings, explicit sockets, and TX disabled. Tooltip/description text survived the exercised clicks and disconnect. Every review instance was closed.Limits
An owner write made after a registry apply is covered or composed at the next apply, not immediately. RxApplet's
applySqlModeVisuals()remains afterrefreshEngaged()because it also updates threshold and Auto SQL text. Its later squelch-button description write can temporarily omit the inactive phrase; its Off-mode slider tooltip still overrides the reason. These are existing ordering limits and remain outside this fix.Interaction with #5484's proposed shortcut tooltip annotation has not been tested; its composition order still needs settling before that proposal lands.