Repository navigation
fix(gui): inset the status bar container so its click targets clear the frameless bottom resize band (#4886). Principle VIII. - #6018
aethersdr-agent[bot] wants to merge 1 commit into
Conversation
…he frameless bottom resize band (#4886). Principle VIII. FramelessResizer claims a 6px band at every edge of a frameless window and its filter lives on the QWindow, so Qt runs it ahead of the receiver's own event() — a press in the band is consumed as a resize before any widget sees it. MainWindow's status bar is docked flush to the window's bottom edge and m_statusBarContainer's QHBoxLayout had no bottom margin, so every child with a vertically-Preferred/Minimum policy was stretched into that band: the gpsStack button and all ~20 plain-QLabel click targets (m_stationNickLabel, m_bandStackIndicator, addPanBtn, the TNF/CWX/ASR/DVK/ FDX indicators, m_paTempLabel, m_networkLabel, the TGXL/PGXL containers), plus m_txIndicator's fixed 36x36 rect by ~1px. Fixed with one bottom inset at the container rather than per-control maximumHeight() caps: one inset covers every current child and every one added later, whereas ~20 individual caps would drift as controls are added. The inset is FramelessResizer::kDefaultMargin, a new named constant replacing the bare `margin = 6` default, so the band width and the inset that has to clear it cannot silently diverge (issue follow-on note 1). MainWindow's own install() call uses the constant too. Evidence (Principle VIII): measured offscreen under Qt 6.11 with a throwaway probe replicating the status bar (fixed height 46, border-top 1px, the same stylesheet, a 36x36 fixed child and a two-row telemetry stack). Before: nick label and GPS button bottoms sit 2px from the window edge — 4px inside the band, which starts 6px up — and m_txIndicator 5px, matching the ~4px/~1px figures from the issue's hitTest probe. After: worst-case clearance is 8px, m_txIndicator 7px, so nothing lands in the band, and the 36px indicator is repositioned rather than clipped (still h=36). Cost is the AlignVCenter content shifting up ~3px uniformly across the bar; worth an eyeball on a running instance before merge, which is the one thing the offscreen probe cannot judge. m_sizeGrip (16x16 at height()-17) sits entirely inside the corner band and so has had dead drag handling for as long as the resizer has been installed. Left alone — the resizer performs the same resize, making the grip a visual affordance. Also corrects the FramelessResizer header's bottom-edge note, which documented #4886 as deferred. No new external symbol references and no cross-TU motion, so no test target link surface changed; MainWindow.cpp is listed only in GUI_SOURCES and frameless_resizer_test's source list is untouched. FramelessResizer.cpp and the new constant's use sites were clang -fsyntax-only clean (no cmake in this environment for a full target build). Blast radius: risk_score=0.237, 12 high-risk affected (top: MainWindow::MainWindow, Ax25HfPacketDecodeDialog::Ax25HfPacketDecodeDialog, MainWindow::wirePanadapter). All 91 upstream callers reach this header through FramelessResizer::install; the change to it is value-identical (kDefaultMargin == the former literal 6 default), so no adopter's band width changes. The three top callers were read to confirm it: MainWindow passes the margin explicitly, PersistentDialog (which Ax25HfPacketDecodeDialog derives from) and the rest take the default. The MainWindow.cpp edit is a layout margin inside buildUI() with no signature or symbol change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
There was a problem hiding this comment.
Issue fit
Yes, for the reported symptom. #4886 asks that status-bar click targets stop short of the frameless bottom resize band, and explicitly says the fix belongs in the status-bar layout rather than in FramelessResizer. The diff does that, and it takes the better of the two shapes the issue thread proposed: one bottom inset on m_statusBarContainer's layout instead of ~20 per-control maximumHeight() caps. Both of the issue's follow-on notes are handled — note 1 (derive the inset from the resizer's margin, don't hard-code 6) becomes FramelessResizer::kDefaultMargin, and note 2 (m_sizeGrip) is deliberately left alone. The header's stale "deferred, tracked as #4886" paragraph is corrected in the same commit, which is the right place for it.
I confirmed the mechanism rather than taking the body's word for it: FramelessResizer installs its filter on QCoreApplication (FramelessResizer.cpp:41-43), and computeEdges() claims p.y() >= rect.height() - margin (FramelessResizer.cpp:109), so the precedence over the widget tree is structural, as claimed. m_statusBarContainer is the only widget added to the status bar (MainWindow.cpp:6307; no addPermanentWidget anywhere), so one inset really does cover every click target listed in the issue.
Everything below is reasoned from the code in /tmp/aetherclaude/pr-6018. I have no build, no GUI and no way to re-run the author's offscreen probe, so the pixel measurements in the commit message are the one class of claim I could not independently reproduce — I checked them for internal consistency instead.
Scope
| File / group | What it changes | Claimed by title/body? | Verdict |
|---|---|---|---|
src/gui/MainWindow.cpp:1201 |
install(this, 6, …) → install(this, FramelessResizer::kDefaultMargin, …) |
yes (issue follow-on note 1) | In scope — value-identical, no behavior change |
src/gui/MainWindow.cpp:5738-5753 |
the bottom inset + its rationale comment | yes, this is the fix | In scope |
src/gui/FramelessResizer.h:136-141 |
new kDefaultMargin, install() default now names it |
yes | In scope |
src/gui/FramelessResizer.h:101-119 |
rewrites the bottom-edge note from "deferred" to "fixed" | yes | In scope — a - block that names a symptom, but it is a deferral note being retired by the fix it deferred, not a guard removal |
src/gui/FramelessResizer.h:132 |
usage example install(win, 6) → install(win, 8) |
no | Cosmetic, explained by context (a 6 example now reads as the default and no longer illustrates "explicit margin"). Named here, not a blocker |
No new public surface beyond kDefaultMargin, which is a named constant for a value that was already the compiled-in default — no adopter's band width changes. CHANGELOG.md correctly untouched. No settings, no capability, no protocol surface. No sibling implementations left behind: the twelve other FramelessResizer::install call sites are dialogs/floating windows, none of which the issue reports a flush-docked click target on.
Blockers
None.
Nits (non-blocking)
FramelessResizer.h:138— the inset rule in the new comment is off by one for the left and top edges.computeEdges()usesp.x() <= marginandp.y() <= marginfor left/top butp.y() >= rect.height() - marginfor right/bottom (FramelessResizer.cpp:105-110), so the left/top bands aremargin + 1px wide (columns0..margin) while right/bottom aremarginpx. "Must inset it by at least this much" is exact for the edge this PR fixes and one pixel short for the other two. Inline comment has a suggested wording.MainWindow's own left/right margins of6are unchanged by this PR, so this is a note for the next adopter who follows the comment, not a regression here.- The inset comes out of content height, and the diff's own arithmetic says the tallest child doesn't fit in what's left. 34px of content vs
m_txIndicator's fixed 36×36 (MainWindow.cpp:6271) — the comment acknowledges this with "positioned, not clipped", and the commit message measures its clearance at 7px against a 6px band. That is 1px of slack for the one control the issue measured as the worst case, and the same halving would apply to the two-row 12px-font telemetry stacks (statusBarCompactLabelStyle,MainWindow.cpp:314-317) on any platform whose font metrics render them above 34px. It works as measured; it is the part most likely to be re-broken by an unrelated change tosetFixedHeight(46), the 1pxborder-top, or the label font size. Worth a sentence in the comment saying the slack is thin and why, so the next person editing the bar height knows it is load-bearing. - The commit message's
m_sizeGripclaim is inherited from the issue comment and is not quite right. 16×16 at(width()-17, height()-17)(MainWindow.cpp:3976) puts only its bottom and right 6px strips inside the band — its top-left ~11×11 is still live, so its drag handling has not been wholly dead. The decision (leave it alone; the resizer performs the same resize) is right either way, so nothing to change in the code. - No test, and I am not asking for one.
tests/tests.cmake:5724-5725states outright that "MainWindow is not linked into test targets", so there is no seam that asserts realm_statusBarContainerchild rects against the real window rect. A probe replicating the layout — which is what the author built and discarded — would assert the fix back to itself.frameless_resizer_testalready coverscomputeEdges()as pure arithmetic and correctly needed no change. Honest coverage boundary: the geometry half of this fix is verifiable only on a running instance.
Socket tests
None added, modified or removed. No QTcpServer/QUdpSocket/QLocalServer/bind/listen/connectToHost and no Fake* peer anywhere in the diff; no test sources are touched at all.
Other tools' findings
CodeGuard's five CG-PATH-001 hits are all false positives for this PR and I am not carrying them forward: MainWindow.cpp:9556 and :9668 are QLabel user-facing strings about FFTW wisdom generation, and :9787-9789 walk up from QCoreApplication::applicationFilePath() to the macOS bundle root with no external input. All five are pre-existing lines far from this diff (which touches :1201 and :5738-5753). No Copilot comments were present.
What I tried to break
- "One inset covers every child." Grepped for other widgets in the status bar —
addWidget(m_statusBarContainer, 1)at:6307is the only one, noaddPermanentWidget, so nothing clickable escapes the container.m_sizeGripis a directMainWindowchild outside the container and is the one deliberate exception. - "Headroom: nothing gets squeezed." Grepped the whole status-bar construction region (
:5736-6310) forsetFixedSize/setFixedHeight/setMinimumHeight:m_txIndicatoris the only fixed-height child, so the comment's "tallest content is the two-row telemetry stacks" is right about the flexible children. The fixed one is the nit above. - "Value-identical, no adopter's band changes." Read all twelve
install()call sites:MainWindowandAetherialAudioStrippass a margin explicitly (both6), the other ten take the default.kDefaultMargin == 6 ==the former literal default, so no adopter's geometry moves. - Deleted lines. Read the
-block in the header as carefully as the+block: it is a deferral note and a measurement correction, not a guard, and the symptom it names is the one this PR fixes. - Off-by-one in the band itself. Walked
computeEdges()per edge. Bottom clearance of 8px vs rowsh-6..h-1is genuinely clear; the asymmetry only bites the left/top edges, which is the first nit. - Could not check: the offscreen 2px→8px and 5px→7px measurements, and the ~3px upward shift of the
AlignVCentercontent across the whole bar. That shift is the one user-visible consequence and the commit says so honestly — it needs an eyeball on a running instance.
Recommendation
Approve with nits. The fix is the right shape at the right seam, it addresses both of the issue's follow-on notes, and the scope is exactly two files that the stated fix explains. Nothing I found rises above a nit: the left/top wording in the new comment is worth a one-line correction while the file is open, and the 1px of slack on m_txIndicator is worth documenting so a future bar-height change doesn't silently re-break it. Concrete next step for the maintainer: give the status bar an eyeball on a running frameless instance to confirm the uniform ~3px upward shift reads as intentional — that is the one claim no reviewer here can settle from the code.
🤖 aethersdr-agent · cost: $6.2892 · model: claude-opus-5
| public: | ||
| // Width (px) of the edge-resize band every adopter gets unless it passes | ||
| // its own `margin`. Named rather than a bare literal because adopters that | ||
| // dock a control flush to an edge must inset it by at least this much to |
There was a problem hiding this comment.
This rule is exact for the bottom and right edges and one pixel short for the left and top. computeEdges() claims p.x() <= margin and p.y() <= margin but p.x() >= rect.width() - margin / p.y() >= rect.height() - margin (FramelessResizer.cpp:105-110), so the left/top bands are margin + 1 px wide — columns 0..margin inclusive — while right/bottom are margin px. An adopter that follows this comment literally and insets a left-docked control by exactly kDefaultMargin leaves its first pixel column still claimed by the resizer.
Doesn't affect this PR's fix (bottom edge, and the measured clearance is 8px), but the comment is the thing the next adopter will read.
| // dock a control flush to an edge must inset it by at least this much to | |
| // dock a control flush to an edge must inset it by at least this much to | |
| // stay out of the band — one more than this on the left and top edges, | |
| // whose bands are `margin + 1` px wide because computeEdges() tests them | |
| // with `<=` (see FramelessResizer.cpp). See MainWindow's status bar, | |
| // #4886 — a hard-coded 6 on both sides could silently diverge from this | |
| // default. |
| // ~5px status-bar margins − 6px inset leaves 34px of content, and the | ||
| // tallest content is the two-row telemetry stacks at ~30px. Measured | ||
| // offscreen (Qt 6.11): worst-case clearance from the window edge goes 2px → | ||
| // 8px, and the fixed 36x36 m_txIndicator is positioned, not clipped. |
There was a problem hiding this comment.
The headroom paragraph is doing something slightly awkward: it computes 34px of content and then immediately names a 36×36 child that doesn't fit in it. The commit message resolves this (measured clearance 7px against a 6px band), but that is 1px of slack for m_txIndicator — which is a real click target (installEventFilter(this) at :6277) and the control the issue measured as the narrowest miss at ~1px.
The mechanism generalises: any child whose natural height exceeds the post-inset content rect gets back roughly half its overflow in lost clearance, so the same applies to the two-row 12px-font stacks if font metrics on some platform/DPI push them past 34px.
Not asking for a code change — the measurement says it clears. Worth one more sentence here saying the slack is thin, so whoever next edits setFixedHeight(46), the 1px border-top, or statusBarCompactLabelStyle's font size knows those numbers are load-bearing for #4886 and not just cosmetic.
|
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: $6.8080 · model: claude-opus-5 |
ten9876
left a comment
There was a problem hiding this comment.
Issue fit: Yes, in the exercised layout. #4886 reports status-bar controls extending into the frameless bottom resize band. The container inset fixes that overlap in the actual application, and the shared constant keeps its width synchronized with the resizer. The issue's separate size-grip behavior remains outside this change.
| File/group | Change | Claimed? | Scope verdict |
|---|---|---|---|
src/gui/MainWindow.cpp:1201 |
Use the named 6px resize margin | Yes, issue follow-on requirement | In scope; value unchanged |
src/gui/MainWindow.cpp:5738–5753 |
Add bottom inset to the single status-bar container | Yes | The requested fix; covers its child layout |
src/gui/FramelessResizer.h:103–117 |
Update the former bottom-edge deferral explanation | Related explanation | In scope, but too much history remains |
src/gui/FramelessResizer.h:132–149 |
Expose kDefaultMargin, use it as the default, change explicit-margin example to 8 |
Constant is required; example cosmetic | In scope; no adopter's default changes |
One commit, authored September 29; both changed files are disclosed. No settings, radio commands, new capability, test registration, guard removal, or changelog changes. The deleted header text describes a deferred symptom rather than removing its protection. statusBar()->addWidget(m_statusBarContainer, 1) is the sole status-bar insertion; the existing independent size grip is deliberately separate.
Blockers: none.
Non-blocking nits
src/gui/MainWindow.cpp:5738–5752andsrc/gui/FramelessResizer.h:103–117: shorten the new incident narrative and measurements to the current invariant. CurrentAGENTS.mdasks for concise comments about what is true now; history and review evidence belong in the commit/PR. The MainWindow comment also says the filter is “on the window,” whereasFramelessResizer.cpp:41–43installs it on the application. A suitable replacement above the layout call is:// Keep status-bar click targets above the bottom resize band (#4886). // Share the inset with FramelessResizer so the two widths stay aligned. // The fixed 36px TX indicator must also remain clear of that band.
src/gui/FramelessResizer.h:138–141: “inset it by at least this much” is not universally sufficient for every edge.computeEdges()atFramelessResizer.cpp:106–109includes coordinates0..marginon left/top, so those bands occupymargin + 1pixels; bottom/right occupymarginpixels. Qualify this guidance as applying to bottom/right. This PR's bottom inset is correct; it does not introduce the existing asymmetry.src/gui/MainWindow.cpp:5738–5753: platform/font headroom remains a verification limit. The fixed TX label clears the band by only one additional logical pixel on the tested configuration. No clipping occurred, but the full range of native styles, DPI configurations, optional peripheral stacks and macOS/Windows layouts was not exercised. No synthetic layout-copy test is requested: the actual running widget tree is the relevant seam.
Empirical verification and attempted falsification
Built exact head af8272071ac6a330a7df70f96a2a3fdfa19f94cd in an isolated scratch worktree (Release, GCC 16.2.1, Qt 6.11.2, CMake 4.4.3; existing CMake build, cmake --build ... --target AetherSDR -j4). Ran the actual application with QT_QPA_PLATFORM=offscreen, empty QT_QPA_PLATFORMTHEME, Fusion style, fresh AETHER_SETTINGS_DIR/XDG directories, explicit private bridge socket, and AETHER_AUTOMATION_NO_TX=1. whoami returned the owned PID/identity and "txAllowed": false; only DEMO-0001 was connected, with "model": "AetherSDR Demo" and "transmitting": false.
Used connect list, connect local serial DEMO-0001, connect wait, get radio, dumpTree, hitTest, and grab QStatusBar. Then rebuilt a negative control changing only the new bottom inset from 6 to 0. This is a behavioral control on the same head, not a separately built full merge-base checkout. It restores the entire behavioral change while keeping the application, settings, style and measurement procedure the same.
For the actual 1400×800 window and 46px bar:
| Measured control | Zero-inset control | PR head |
|---|---|---|
| Add Panadapter, TNF, ASR, station label and other visible flexible click targets | 2px bottom clearance; 4px inside resize band | 8px clearance; no overlap |
| Fixed 36×36 Cancel transmit label | 5px clearance; 1px inside resize band | 7px clearance; no overlap; still 36px high |
hitTest MainWindow 235 796 |
QLabel, objectName: addPanadapterButton |
Inner QWidget, no control at this band point |
This reproduces the original overlap and removes it with the fix. Inspected both actual status-bar PNGs: the content moves upward as expected, and the fixed indicator remains intact. The active station text and two-row telemetry remain readable. Geometry/hit-test evidence establishes the overlap; this was not a native compositor drag test. Hidden GPS/peripheral controls were not individually enabled, so their future layout behavior is code-reviewed, not claimed as runtime coverage. Both owned instances were closed; the scratch source was restored to exact head. No operator process was driven and no transmit control was invoked.
No tests changed, and the exact diff introduces no socket-owning test or synthetic firmware peer. Existing pure resizer arithmetic was inspected but not presented as coverage of this layout change. The seven displayed GitHub checks are successful, including all four required checks, but the PR-triggered CI/Static Checks runs were created September 29 (2026-09-29T14:35:17Z), before current main's October 4 tip. That green is stale merge evidence, not a fresh full-suite result. The local application build passed; no broad suite was run.
Previous bot concerns were checked independently: the left/top wording and thin TX slack are real nits; the size grip is not entirely inside the corner band (its top-left portion lies outside), but this PR does not alter it. CodeGuard's cited path-handling locations and the accessibility warnings concern pre-existing code outside these hunks, not new behavior from the inset.
Recommendation: Approve with nits. The actual application plus zero-inset control demonstrates the load-bearing fix. Shorten the comments, qualify the general inset guidance, and obtain current-base CI and the maintainer's usual platform/layout check before landing.
— Jeremy KK7GWY + Codex
ten9876
left a comment
There was a problem hiding this comment.
Issue fit: yes. #4886 asks that status-bar click targets stop short of the frameless bottom resize band, and that the fix live in the status-bar layout. One bottom inset on the single status-bar container does that. I re-verified it on current main (below), not only on the September base.
What changed since the earlier reviews: the PR now conflicts with main (mergeable: CONFLICTING), and that is the one blocker. Squash auto-merge is still armed by aethersdr-agent (since 2026-09-29), so a rebase plus an approval merges it immediately.
Scope
| File/group | Change | Claimed? | Verdict |
|---|---|---|---|
src/gui/MainWindow.cpp:1201 |
install(this, 6, …) → FramelessResizer::kDefaultMargin |
Yes (issue note 1) | In scope, value unchanged |
src/gui/MainWindow.cpp:5738-5753 |
Bottom inset on m_statusBarContainer's layout, plus a 15-line comment |
Yes | In scope; the comment is nit 2 |
src/gui/FramelessResizer.h:136-141 |
kDefaultMargin constant, used as install()'s default |
Yes | In scope |
src/gui/FramelessResizer.h:101-132 |
Rewrites the "deferred to #4886" note; usage example 6 → 8 |
Implicit | Main has since deleted this block (see blocker 1) |
One commit, authored 2026-09-29. There are no test changes, settings, protocol or capability surface, or CHANGELOG entry. Socket preflight: no test sources touched.
Blockers
-
It conflicts with
mainin both files.git merge-tree origin/main af827207reports content conflicts insrc/gui/FramelessResizer.handsrc/gui/MainWindow.cpp. #6072 (comment-policy pass) condensedFramelessResizer.h, deleting the whole bottom-edge paragraph this PR rewrites, and moved and edited theMainWindow.cppregions.When rebasing, don't re-introduce the deleted header paragraph: on
mainit no longer exists, and that was deliberate. The whole fix rebased ontomain85f2a1249is three lines. I applied exactly this, built it, and drove it:// src/gui/FramelessResizer.h, replacing the install() declaration static constexpr int kDefaultMargin = 6; static void install(QWidget* window, int margin = kDefaultMargin, int topMoveReserve = 0);
// src/gui/MainWindow.cpp (main:1170) FramelessResizer::install(this, FramelessResizer::kDefaultMargin, TitleBar::kHeight);
// src/gui/MainWindow.cpp (main:5626) // Keep status-bar click targets above FramelessResizer's bottom resize band (#4886). hbox->setContentsMargins(6, 0, 6, FramelessResizer::kDefaultMargin);
Nits (non-blocking)
src/gui/AetherialAudioStrip.cpp:127still passes a literal6(install(this, 6, kTitleBarHeight)). The PR's own rationale forkDefaultMargin, that a hard-coded 6 "could silently diverge from this default", applies to it too. It's a one-token change.MainWindow.cpp:5738-5752: the 15-line comment narrates the measurement history. AGENTS.md §"Comments" asks for the current invariant only, and the earlier review's 3-line replacement still fits. The one-line version in the blocker fix above is enough.FramelessResizer.h:138: "inset it by at least this much" is exact for bottom/right but one pixel short for left/top.computeEdges()uses<= marginthere (FramelessResizer.cpp). This is carried over from the earlier reviews and is still true onmain.- Two
aethersdr-agentinline threads (FramelessResizer.h:138,MainWindow.cpp:5752) are unresolved.mainrequires conversation resolution, so they block the merge independently of this review.
Verified empirically, and what I tried to break
-
Re-measured on current
main, not the September base.mainhas taken 20 commits throughMainWindow.cppsince this branch, including new peripheral and status entries, so the earlier measurements described a status bar that no longer exists.- Setup: I built
main85f2a1249plus the 3-line rebase above (Release), and drove it offscreen with isolated settings, the explicit socketaethersdr-pr6018andAETHER_AUTOMATION_NO_TX=1. whoamireturned"txAllowed": false. I connected explicitly toDEMO-0001, andget radioreturned"model": "AetherSDR Demo".- From
dumpTreegeometry on the 1400×800 window (status bar at y 754, height 46), I computed every visible status-bar descendant's clearance from the window's bottom edge:
zero-inset control (main as-is) with the fix Flexible click targets ( addPanadapterButton,TNF,ASR, station label,automationChip, separators)2 px (4 px inside the 6 px band), height 41 8 px, height 35 Fixed 36×36 "Cancel transmit" 5 px (1 px inside the band) 7 px, still 36 high hitTest MainWindow 235 796QLabel addPanadapterButtoncontainer QWidget, no controlThe bug reproduces on today's
main, and the fix clears it with 1–2 px of margin over the band. The zero-inset control is the same tree with only the inset reverted, rebuilt. - Setup: I built
-
"One inset covers every child." On
main,m_statusBarContaineris still the status bar's only widget, so all 28 visible descendants I enumerated sit inside it. -
Not tested: a native compositor drag (offscreen geometry only), and hidden GPS or peripheral entries, which are reviewed from code.
-
CI is from 2026-09-29, before
main's 2026-10-04 tip, and it describes a merge that no longer applies.
Recommendation: Request changes, for the rebase only. The fix itself is right and re-verified on current main. After the rebase, the only things left are resolving the two bot threads and the operator's approval, which merges it straight away because auto-merge is armed.
— Jeremy KK7GWY + Claude
|
Superseded by #6198 |
Pull request was closed
Summary
Fixes #4886
What was changed
fix(gui): inset the status bar container so its click targets clear the frameless bottom resize band (#4886). Principle VIII.
Files modified
src/gui/FramelessResizer.hsrc/gui/MainWindow.cppGenerated by AetherClaude (automated agent for AetherSDR)
🤖 aethersdr-agent · cost: $4.9684 · model: claude-opus-5