Repository navigation
fix(titlebar): keep the TX timer's slot so the audio row stops shifting. Principle VIII. - #6456
Conversation
There was a problem hiding this comment.
Issue fit — partially, and the body's mechanism is refuted by the issue thread
The fix cures the real defect, but not for the reason the PR gives, and the thread moved under it 25 minutes after it was opened.
The body's cause section says that on expiry "PC Audio, the line-out speaker button, the master volume slider and readout, and the headphone control slid left by the timer's width, and slid back right at the next key-up." The triage pass on #6455 measured that and it does not happen in the settled layout: the audio cluster sits immediately right of the drag gutter, which is the only non-zero stretch in m_hbox (src/gui/TitleBar.cpp:332), so when the timer's 52 + 8 px slot collapses the gutter takes the width and the cluster's left edge moves right by the same amount. m_pcBtn's screen x is identical shown vs hidden, at every window width including the two where the gutter is already squeezed to 0.
What @jwpauler actually photographed — "a fraction of a second, followed by a shift back to the right" — is the nested cluster's intermediate layout pass: pass 1 re-lays the cluster's children inside its still-272 px rect (PC Audio slides left), pass 2 shrinks the cluster and snaps everything back. retainSizeWhenHidden does cure that, because the cluster's size hint stops changing at all, so there is no second pass to be caught between. The fix works; the stated mechanism is not why.
Two further body claims that the same measurement contradicts:
- "Trade-off. While idle there is now a permanent gap of about 52 px … to the left of PC Audio." It is lost gutter, not a gap — visually indistinguishable from the empty drag area that was already there. The real undisclosed cost is that the bar's
minimumSizeHintgrows by 52 + 3 px, which only bites when the bar is over-full and the gutter is already at 0. - The title claims Principle VIII. The screenshots did establish the symptom, so the evidence is real — but the root-cause paragraph is the pre-triage reading. Worth rewriting the body around what was measured, since the comment added at
:388-390enshrines the refuted version in the source.
To be clear on timing: this PR was opened at 02:39 and the reproduction comment landed at 03:04, so the author was not ignoring it.
Scope
One file, two changes, nothing unexplained by #6455.
| Change | Claimed? | Verdict |
|---|---|---|
src/gui/TitleBar.cpp:388-394 — setRetainSizeWhenHidden(true) on m_txTimerLabel |
Yes — the issue's proposed fix, and Option B in the thread | In scope; see Blockers 1 and the maintainer item |
src/gui/TitleBar.cpp:398-400 — audio->addSpacing(3) + comment |
Yes — issue "Proposed fix" bullet 2 | In scope by the issue body, but the triage called it "purely cosmetic … a padding/spacing taste call rather than a defect, and worth settling separately from the shift." Needs the maintainer item below. |
No build/CI/docs/vendor churn, no new public surface, no settings keys, no deleted guards, no CHANGELOG.md edit. The body's self-certifications check out against the diff.
Blockers
1. src/gui/TitleBar.cpp:1726-1754 — minimal mode now reserves the timer's slot in the narrow strip.
setMinimalMode() hides every audio control to free horizontal space, and deliberately does not touch the timer:
m_pcBtn->setVisible(!on);
m_speakerBtn->setVisible(!on);
m_headphoneBtn->setVisible(!on);
m_masterSlider->setVisible(!on);
m_hpSlider->setVisible(!on);
m_masterLabel->setVisible(!on);
m_hpLabel->setVisible(!on);None of those carry retainSizeWhenHidden, so they collapse properly and today the cluster shrinks to nothing in minimal mode. After this change the idle timer holds 52 px (plus the new 3 px spacer) in exactly the mode that exists to reclaim width. The thread named this as the one addition Option B still needs; it is not in the diff. Drop-in, in setMinimalMode() alongside the other visibility lines:
// Minimal mode hides the audio row to free the strip; don't let the idle
// timer's retained slot hold 52 px of it.
{
QSizePolicy sp = m_txTimerLabel->sizePolicy();
sp.setRetainSizeWhenHidden(!on);
m_txTimerLabel->setSizePolicy(sp);
}The 3 px spacer stays reserved either way; at 3 px that is negligible and not worth plumbing.
2. No regression test, and the stated reason for omitting one is incorrect.
The body says "there is no existing widget-layout test harness for TitleBar, and the property under test (row position across a show/hide cycle) is purely visual." There is one: unified_title_bar_test (tests/tests.cmake:7971-7999) compiles src/gui/TitleBar.cpp, runs under QT_QPA_PLATFORM=offscreen, and already asserts bar geometry (offsetInWindow, bar height) for exactly this kind of regression.
The property is also deterministic rather than visual, and the thread already drove it: show the timer, record m_pcBtn's window x, hide the label directly to skip the 15 s hold plus 800 ms fade, pump exactly one layout pass, assert x is unchanged.
bar.setOperatorTransmitting(true);
const int shownX = pcBtn->mapTo(&bar, QPoint(0, 0)).x();
bar.findChild<QLabel*>(QStringLiteral("txTimerLabel"))->setVisible(false);
QCoreApplication::sendPostedEvents(nullptr, QEvent::LayoutRequest);
QCOMPARE(pcBtn->mapTo(&bar, QPoint(0, 0)).x(), shownX);That is an assertion about equality across a cycle, not absolute pixels, so it does not inherit the font-dependence that makes other geometry probes here brittle. It fails on main (978 vs 1026 in the thread's replica), passes with this change, and it is the only thing in the PR that would pin the fix to the mechanism that was actually measured rather than the one the body describes. Per AGENTS.md's test-layer boundary this is a socket-free CTest in an existing registered target.
Needs maintainer decision
3. src/gui/TitleBar.cpp:388-394 and :398-400 — Option A vs B is still open, and the spacer was explicitly deferred.
#6455 carries maintainer-review and ends "I'd lean A — it removes the two-pass mechanism rather than papering over one instance of it — but B is smaller and keeps the cluster's grouping intent. Maintainer's call." Option A promotes the label out of the nested cluster into m_hbox, which removes the two-pass mechanism, needs no setMinimalMode() change, reserves nothing while idle, and gives the gap to PC Audio the bar's own 16 px inter-group spacing — close to what the 3 px spacer is reaching for. This PR implements B. Both measured flat; the choice is a judgment about whether to remove the mechanism or one instance of it, and no human has ruled yet. Not something I should adjudicate.
The spacer rides along in the same decision: it is listed in the issue's own proposed fix, so it is disclosed rather than smuggled, but the triage asked for it to be settled separately from the shift, and it is a permanent 3 px change to every operator's title bar with no defect behind it. If the maintainer takes Option A the spacer is moot anyway.
Nits (non-blocking)
-
src/gui/TitleBar.cpp:388-390— the comment states the refuted mechanism and will mislead the next reader:// doesn't reflow the row (PC Audio / volume / headphone would otherwise
// jump left on expiry and right on the next key-up).Suggested replacement:
// Keep the slot reserved while idle so the cluster's size hint never // changes. The settled row does not move (the drag gutter absorbs the // 52+8 px), but hiding the label made the nested cluster re-lay its // children inside its old rect for one pass, flicking PC Audio and // everything right of it left for a frame before m_hbox caught up (#6455).
-
src/gui/TitleBar.cpp:423-425— the label's text is never reset when it is hidden, so the retained slot keeps the width of the last reading. After an over past an hour (H:MM:SS,formatTxElapsed,:1363-1376) the idle gap stays wider for the rest of the session. The body's "Known limit" says "the row will resize once at that point, as it already did before this change" — before this change the width came back on hide; now it does not. One line in the fade-finished lambda fixes it:m_txTimerLabel->setText(QStringLiteral("0:00"));next to thesetVisible(false). -
src/gui/TitleBar.cpp:398— the reserved band is now bareaudioCluster, which is not passed throughmarkDragHandle(), so the cursor over those 52 px is the default arrow rather than theOpenHandCursorthe gutter shows there today. Window dragging itself is unaffected (see below).markDragHandle(audioCluster);after the cluster is created would restore the affordance; children keep their own cursors. -
src/gui/TitleBar.cpp:352-354—m_otherTxLabelsits in the same cluster and hides/shows the same way, so the same one-frame transient is still reachable when another client's TX indicator clears. Completeness only, andretainSizeWhenHiddenis an awkward fit there because the label is text-sized — which is part of why the thread leaned toward Option A.
Security
Audited by the security-audit subagent against the CodeGuard findings. All 5 are false positives — none is a finding:
src/gui/TitleBar.cpp:739(CS-VLN-CMDI-AMP) isvoid TitleBar::setActiveRadio(const QString& id)— the&is a C++ reference qualifier, not a shell operator.:217,:661,:874,:1267(CS-VLN-CMDI-BACKTICK) are all//comments whose backticks quote a QSS selector, an env-var assignment and a Qt signal name — the known lexical-match pattern.
The decisive point is structural: the translation unit has no process-spawning sink at all (no QProcess, system(, popen, execve; the two exec( hits are comments explaining why QMenu::popup() is used instead), so no CMDI source can be reachable. All five lines are pre-existing and outside the diff, consistent with a whole-file scan. The diff itself touches no input boundary, string formatting or dangerous API. Credit: security-audit (CodeGuard).
One pre-existing item outside the diff, noted and not asked for here: :1335 puts a peer's station name into m_otherTxLabel without setTextFormat(Qt::PlainText), and Qt auto-detects rich text.
What I tried to break
- The drag-dead-band theory. With the label hidden,
childAt()returnsaudioCluster, which is not a drag handle, soisSystemMoveAreaAt()(:836-861) returns false for the reserved band — that looked like ~55 px of newly undraggable title bar. It is not: that function has zero callers anywhere insrc/, and the live path isaudioCluster(a plainQWidget, so it ignores presses) propagating toTitleBar::mousePressEvent(:1061) →startWindowMove(), which does not consult it. Dragging over the band still works; only the cursor changes, demoted to the nit above. - The bridge verb.
txTimerState()(:1420-1432) still derivesvisiblefromm_txTimerLabel->isVisible(), whichretainSizeWhenHiddendoes not affect, soget txtimerreads the same as before. The body's claim here holds. - The timer state machine.
setOperatorTransmitting()(:1384-1417) is untouched, including the duplicate-rising-edge guard that protects an in-progress over and the spurious-unkey early return. No change to show/hold/fade ordering, no new path that keys or restores into a keyed transmitter. - Over-constrained widths. The thread's harness probed 1400 / 900 / 700 / 560 / 480 px, including two widths where the gutter is already at 0 and the bar is over its minimum hint — zero delta at every one. That is what rules out the body's steady-state claim rather than just doubting it, and it is also why the only genuine width cost here is the grown
minimumSizeHint. - CI. All five checks green on
338c72a(Static checks, build, check-windows, check-macos, Sanitizer option configures). Normal base, not a stacked temp branch — butctestin these lanes is-R-filtered, and no existing test asserts title-bar cluster geometry across a show/hide cycle, so green here is build evidence, not behavior evidence for this change.
Honesty boundary: I ran nothing. There is no build, no test run and no way to drive the GUI here, so every statement above is reasoned from the source in the PR head checkout. The layout numbers I rely on (pcX 1026 settled, 978 after one layout pass) are the triage harness's measurements quoted from the #6455 thread, not mine — I verified the code structure they describe (gutter stretch, cluster nesting, setMinimalMode's omission of the timer, isSystemMoveAreaAt's lack of callers) but not the measurements themselves.
Recommendation
Needs maintainer decision. The defect is real and retainSizeWhenHidden genuinely cures it, so there is a good fix in here — but the A-vs-B choice on #6455 is still unruled and this PR commits to B, and B as submitted is missing the setMinimalMode() clearing the thread named as its prerequisite. Concrete next step: a maintainer picks A or B on the issue. If B, the two blockers are small — the setRetainSizeWhenHidden(!on) block above, plus the regression test in unified_title_bar_test, which is also what would stop this from being re-litigated. Either way the body deserves a rewrite around the measured transient, since the current root-cause text is what the triage disproved.
Thanks for the unusually careful write-up on both the issue and the PR — the source reading was accurate, the trade-off was disclosed up front rather than buried, and the screenshot follow-up with "a fraction of a second, then it shifts back" is the single detail that cracked this open. The first triage pass got it wrong without it.
🤖 aethersdr-agent · cost: $11.6266 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (5 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.8016 · model: claude-opus-5 |
|
Antares Detector — candidate vulnerable file(s)
The transmit timer is allocated in TitleBar.cpp (lines 408–409) and connected to a timeout. The cleanup logic in the timeout handler stops and restarts the 15s hold/fade timer (lines ~1394–1417), which properly manages the timer slot lifecycle. The allocation and connection are safe; the vulnerability is not in allocation but in the missing explicit deletion of the timer slot in the timeout handler (the code stops/restarts the timer rather than deleting the slot), which could lead to a retained timer slot and memory-safety issues. 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: $11.8016 · model: claude-opus-5 |
…pin the layout shift. Principle VIII. Review follow-up on aethersdr#6456: minimal mode drops the retained timer slot, the timer text resets on hide so an over past an hour doesn't leave a wide slot, the reserved band keeps the drag cursor, the comment states the measured one-frame cluster transient, and unified_title_bar_test pins PC Audio's x across a timer / other-client-TX hide. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks for the thorough review. Addressed in bf2d8f7:
Left to the maintainer: Option A vs B on #6455 (this PR is B), and whether the 3 px spacer stays (it is in the issue's proposed fix; trivial to drop). |
|
Follow-up (verifier finding): marking the cluster as a drag handle made the master and headphone sliders inherit the open-hand cursor; both now set an explicit arrow cursor, and the test checks neither slider reports OpenHandCursor. Also worth noting: the other-client test is a guard rather than a mechanism pin, since the visible timer label absorbs the freed width in the first pass. |
|
@aethersdr-agent please re-review at head |
…ng. Principle VIII. The transmit timer sat in the audio cluster's QHBoxLayout and was hidden once its post-unkey fade finished. A hidden widget gives up its layout space, so PC Audio, the line-out button, the master volume slider and the headphone control all jumped left when the timer expired and right again on the next key-up. Set retainSizeWhenHidden on the timer label so its slot stays reserved while idle. Add a 3 px spacer after the label so the gap to PC Audio matches the visual gap on the other side of the button, where the speaker icon is inset in its 22 px box. Fixes aethersdr#6455 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…pin the layout shift. Principle VIII. Review follow-up on aethersdr#6456: minimal mode drops the retained timer slot, the timer text resets on hide so an over past an hour doesn't leave a wide slot, the reserved band keeps the drag cursor, the comment states the measured one-frame cluster transient, and unified_title_bar_test pins PC Audio's x across a timer / other-client-TX hide. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…rsor. Principle VIII. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
27dafca to
a67ea01
Compare
jensenpat
left a comment
There was a problem hiding this comment.
Reviewed the rebased head a67ea013867300dc67fe17e70177f8b896a53d00 against current main. The requested landing retains this PR's small reserved-slot implementation and 3 px spacer.
The earlier review's minimal-mode reservation and missing-test findings are fixed. The revised description matches the transient nested-layout mechanism. The fade resets the elapsed text; the reserved band has a drag cursor while the sliders retain arrow cursors. User-guide pages now describe stable control positions and minimal-mode behavior.
Local macOS arm64 / Qt 6.12.0: unified_title_bar_test builds and passes 1/1 under QT_QPA_PLATFORM=offscreen. Disabling retention makes the first-pass geometry check fail (PC Audio x=362 versus 415); restoring it passes. The suite also checks minimal-mode retention, other-client label clearing and both slider cursors. Engine-boundary, command-plane, capability-record, manifest, generated-reference and log-rule checks pass without baseline growth. All four commits are GitHub-verified.
CodeGuard's command-injection reports are lexical false positives: C++ references and backtick-containing comments, with no process-spawning sink in TitleBar.cpp. Antares' deletion suggestion is also invalid: the timers/animation use Qt parent ownership; a hidden label's retained layout slot is intentional and does not allocate repeatedly or require timeout-time deletion.
The automated review tool was unavailable; the diff, surrounding lifecycle and failure paths were reviewed manually. No live-radio/TX, native-window screenshot or visual inspection was performed. The geometry regression is exercised directly by the offscreen production TitleBar test; the bridge cannot produce those keyed timer transitions with TX disabled.
Hosted Docs and Static checks pass. Required platform builds are still in progress; the guarded auto-merge must wait for them.
Summary
Fixes #6455
The title-bar audio controls no longer flick horizontally for a frame when the TX timer appears and disappears.
Cause. The transmit timer label (
m_txTimerLabel) sits in the nested audio cluster just before PC Audio. The settled layout does not move when it hides: the drag gutter is the only stretch inm_hboxand absorbs the timer's 52 + 8 px, so the cluster's left edge moves, not PC Audio. What the operator saw is a one-pass transient: hiding the label made the cluster re-lay its children inside its still-old rect (PC Audio and everything right of it slide left), and the next pass shrank the cluster and snapped them back.Change (
src/gui/TitleBar.cpp,tests/unified_title_bar_test.cpp):QSizePolicy::setRetainSizeWhenHidden(true)on the timer label, so the cluster's size hint never changes and there is no second pass to catch. Visibility semantics are unchanged;txTimerState()(get txtimer) still reportsvisiblefromisVisible().setMinimalMode()), so the idle slot does not hold 52 px of the narrow strip.0:00when the fade finishes, so the retained slot returns to normal width after an over longer than an hour.markDragHandle(audioCluster)so the reserved band keeps the open-hand cursor.audio->addSpacing(3), from the issue's proposed fix: the speaker icon is inset in its 22 px button, so the gap to its right read ~3 px wider than the 8 px on the timer side.Trade-off. The reserved band is lost drag gutter, visually the same as the empty drag area that was there before. The real cost is that the bar's
minimumSizeHintgrows by about 55 px (52 + 3), which only bites when the bar is over-full and the gutter is already at 0.Known limit. None for the hour case now: the text reset returns the slot to its normal width.
Constitution principle honored
Principle VIII — Evidence Over Assertion. The transient was measured (PC Audio x 1026 settled vs 978 after one layout pass in the #6455 thread) and is now pinned by a test that fails without the fix.
Test plan
unified_title_bar_testbuild and CTest pass on the rebased code (macOS arm64, Qt 6.12.0, offscreen). Mutation: disabling size retention fails with PC Audio x=362 versus 415; restored fix passes.unified_title_bar_test: new checks hide the timer directly (skipping the hold and fade), pump oneLayoutRequestpass and assert PC Audio's x is unchanged; same forsetOtherClientTx(true,"X")then(false); minimal mode does not retain the timer's slot. The timer check fails (362 vs 415) withretainSizeWhenHiddenreverted.m_otherTxLabelis covered by a test, but it did not reproduce the transient in one layout pass on the offscreen platform, so no code change was made for it.Checklist
docs/COMMIT-SIGNING.md) — SSH-signed as w5jwpAppSettingscalls — none added (Principle V)MeterSmoother— not applicable, no meter UI touchedCHANGELOG.mdnot touched.🤖 Generated with Claude Code
Landing validation: signed rebase onto current main; the timer-label policy, minimal-mode reservation, other-client label and slider cursor checks pass. No live-radio/TX or native-window visual validation was performed.
Generated with OpenAI Codex (GPT-6 Astra)