Repository navigation
fix(gui): menus stay open and click-to-tune works with Frameless Window on (#6272). Principle VIII. - #6328
Conversation
…ow on (aethersdr#6272). Principle VIII. With Frameless Window on, the main window uses Qt's expanded client area with CustomizeWindowHint and no title or caption-button hints. Qt 6.12 answers WM_NCHITTEST for exactly those flags (qwindowswindow.cpp:3415-3427) by reading the live button state with GetAsyncKeyState. Whenever that state differs from the last hit test (a function-static shared by every window), it synthesizes a mouse press or release with handleMouseEvent(SynchronousDelivery) at the cursor, anywhere in the window. Windows sends WM_NCHITTEST around a real click, so the click arrives twice. The hamburger menu opens on the first press and closes on the second, the panadapter's exec() context menu closes the same way, and the extra press/release pairs break double-click to tune. The bridge's own click verbs never cause a WM_NCHITTEST, so they could not reproduce it. The outcome also depends on hit-test timing, which is why one earlier real-click run passed. MainWindow::nativeEvent() runs before Qt's hit test (qwindowscontext.cpp :858 vs :1019) and now answers WM_NCHITTEST itself under the expanded client area. It returns the resize codes on a band of SM_CXSIZEFRAME + SM_CXPADDEDBORDER at the window edges (none while maximized or fullscreen), the same band Qt used, and HTCLIENT everywhere else, with no synthetic events. WindowChrome::expandedFrameHit() is the pure part, covered in unified_title_bar_test; an off-by-one in the band and an always-client answer each fail it. Popped-out panadapters use plain FramelessWindowHint, which Qt hit-tests on its ordinary path; they are unaffected and unchanged. window-chrome.md now says edge resize on this path rests on this handler. 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.
Reviewed the head checkout at 1c51a580, not just the diff. All five checks are green, including check-windows — the lane that actually compiles the new #ifdef Q_OS_WIN block.
1. Issue fit — Yes
#6272 reports three symptoms with Frameless Window on: the ☰ menu flashes, the panadapter context menu flashes, and click-to-tune doesn't fire. The before/after bridge table covers all three with real SendInput input (0/5 → 5/5 each), which is exactly the evidence the maintainer asked for in the second issue comment ("the bridge's own click verbs don't reproduce this"). The mechanism — answering WM_NCHITTEST in MainWindow::nativeEvent() before QWindowsContext reaches its own hit-test switch — is sound independently of whether the GetAsyncKeyState/shared-static narrative is exactly right.
Not claimed and correctly out of scope: the issue thread's side note about the window growing ~7 px per Frameless toggle. That is the native-client-rect geometry path (#6303), untouched here.
2. Scope
| File | Change | Claimed? | Verdict |
|---|---|---|---|
src/gui/MainWindow.cpp |
frameHitCode() + the WM_NCHITTEST branch in nativeEvent(); the old WM_NCCALCSIZE comment reworded |
yes | In scope |
src/gui/WindowChrome.h |
FrameHit enum + pure expandedFrameHit(); <QPoint> include |
yes | In scope |
tests/unified_title_bar_test.cpp |
10 expandedFrameHit checks; <QPoint>/<QRect> includes |
yes | In scope |
docs/architecture/window-chrome.md |
Windows bullet rewritten for the new handler | yes | In scope |
Everything in the diff is explained by the issue. No new public/protocol surface (FrameHit/expandedFrameHit are internal to the GUI). No CHANGELOG.md entry — correct. No removed guard: the only deleted lines are the two comment lines the new comment replaces, and the deleted doc sentence ("recheck edge resize after a Qt bump") is obsoleted rather than lost, since the handler is ours now.
No sibling left behind. MainWindow is the only window that reaches WindowChrome::configure() (MainWindow.cpp:1140, :10143), and it is the only holder of either Qt::ExpandedClientAreaHint or Qt::CustomizeWindowHint in the tree — I grepped both, since the PR's root cause names the CustomizeWindowHint branch, which could in principle have snared unrelated dialogs. It doesn't: nothing else sets that flag.
Security audit (required by the harness). Ran; it refuted all 36 CodeGuard findings and found nothing independently. Every category resolves to C++/Qt idiom or pre-existing test fixtures: && read as a shell AND, markdown backticks in comments, // ── box-drawing read as homoglyphs, the window-role parameter read as a prompt role override, and RFC 5737 192.0.2.x display strings in untouched radio-switcher fixtures at tests/unified_title_bar_test.cpp:431. The CS-VLN-PATH-PROC/CG-PATH-001 lines don't correspond to any path handling — the three files contain no QFile/QDir/QStandardPaths at all. Nothing to carry forward.
3. Blockers
None.
4. Nits (all non-blocking)
-
src/gui/MainWindow.cpp:3946-3948— the vertical band uses the horizontal metric.border = GetSystemMetricsForDpi(SM_CXSIZEFRAME, dpi) + GetSystemMetricsForDpi(SM_CXPADDEDBORDER, dpi);
One scalar serves all four edges, so the top/bottom band is
SM_CXSIZEFRAME-wide. The siblingWM_NCCALCSIZEblock 15 lines below deliberately distinguishes them (borderXvsborderYfromSM_CXSIZEFRAME/SM_CYSIZEFRAME). They're equal on every stock Windows theme, so this is cosmetic rather than wrong — but the inconsistency between two adjacent blocks will read as an oversight later. If you'd rather keepexpandedFrameHit()single-scalar, a one-line comment saying the two metrics are equal in practice would settle it. -
docs/architecture/window-chrome.md:92-99— #6224 now lands in our handler. The bullet above still says the Snap Layouts flyout is missing because "nothing returnsHTMAXBUTTONon hover", and the PR's "Not verified" says that is unchanged — both true. What changed is where a future fix goes: before this PR it was a Qt-side gap, now it has to be aFrameHit::MaxButtoncase inexpandedFrameHit()plus the caption-button rect. Worth one sentence in the doc so #6224 isn't re-diagnosed as a Qt limitation. -
Top 8 px of the title bar is a resize zone.
expandedFrameHit()applies the band at the top as well, andTitleBar's layout has a zero top margin (src/gui/TitleBar.cpp:231,setContentsMargins(16, 0, 16, 0)), so the topmost ~8 physical px of ☰ and of the caption buttons answerHTTOP/HTTOPRIGHTrather thanHTCLIENT. The 5/5 ☰ result shows the centre is fine, and Qt's own band did the same, so this is pre-existing rather than introduced. But note the Linux fallback deliberately reservesTitleBar::kHeightfrom the top band for precisely this reason (#4886,FramelessResizer::install(this, 6, TitleBar::kHeight)), and Windows now has an equivalent knob for the first time. Your call whether to match; it is not a regression either way. -
Window move and double-click-maximize aren't in the proof matrix. The handler answers
HTCLIENTacross the whole caption region, so anything that depended on a caption hit code there is gone. Reading the tree, this should be fine:TitleBar::startWindowMove()(src/gui/TitleBar.cpp:850-886) drags viaQWindow::startSystemMove()from a Qt mouse press, andmouseDoubleClickEvent→handleTitleDoubleClicktoggles maximize the same way — neither routes throughWM_NCHITTEST. Since the resize table covers only bottom and left drags, one real-SendInputdrag on a bare title-bar gap (window position changes) and one real double-click (window maximizes) would close the gap; failing that, listing them under "Not verified" is enough.
5. What I tried to break
- A sibling window with the same defect. Grepped
ExpandedClientAreaHint,NoTitleBarBackgroundHintandCustomizeWindowHintacrosssrc/andtests/:MainWindowis the only holder of any of them, and the only caller ofWindowChrome::configure(). The PR's claim that popped-out panadapters are unaffected holds for a second reason beyond the 5/5 control rows — they never carry the flag the Qt branch keys on. - Window dragging and double-click-maximize as collateral of blanket
HTCLIENT. This was my leading blocker candidate; it died on readingTitleBar::startWindowMove()andhandleTitleDoubleClick(), both of which work from Qt-level mouse events on the client area. Reasoned from code, not run — hence the nit above. - The boundary arithmetic in
expandedFrameHit(), againstGetWindowRect's exclusive right/bottom vsQRect's inclusiveright()/bottom(). The conversion atMainWindow.cpp:3952-3953isQRect(left, top, right-left, bottom-top), sorect.right() == window.right - 1, the last valid column — andpoint.x() > window.right() - bordercatches it. The test vectors agree (win(100,50,1000,600) →right()1099,bottom()649, both asserted as edges). No off-by-one, no gap, no overlap:108is the first client column withborder == 8. - Win32 failure paths.
GetWindowRect's return is unchecked, but the zero-initialisedRECT window{}yields an emptyQRectwhosecontains()is false →FrameHit::Client→HTCLIENT.GetDpiForWindowreturning 0 makes both metrics 0 →border <= 0→HTCLIENT. Both degrade to the safe answer rather than a wrong resize code.GET_X_LPARAM/GET_Y_LPARAM(notLOWORD) sign-extend, which is required sinceWM_NCHITTESTscreen coordinates go negative on a left-hand monitor — the classic bug here, and it isn't present. - Maximized and fullscreen.
IsZoomed || isFullScreen()zeroes the band, so a maximized window answersHTCLIENTacross the inset region theWM_NCCALCSIZEbranch carves out. Consistent with the gate in that branch, and matches the doc. - The tests, for proving-the-implementation-back-to-itself. They test the new pure function, so by construction they can't fail against unfixed code — but they are not tautological: the band width, the corner precedence and the one-pixel-past-the-band row would each fail against a plausible wrong implementation, and the PR records the two mutations that kill them. What they cannot reach is the part that actually fixes #6272: that
nativeEvent()claims the message before Qt, and thatIsZoomed/isFullScreen()zero the band. There is no socket-free seam for aWM_NCHITTESTdispatch, so perAGENTS.md's test-layer boundary this is correctly bridge/real-input evidence rather than CTest, and the PR supplies it.unified_title_bar_testis in noci.yml-Rfilter, so CI's green does not include these checks. - What I could not test. The root-cause narrative — Qt's
else if (CustomizeWindowHint)branch atqwindowswindow.cpp:3415-3427readingGetAsyncKeyStateand synthesising presses off a function-staticoldMouseButtonState. There is no Qt source tree in this environment, so I am taking that on the PR's word; a read of those lines would settle it. It doesn't change the verdict: the before/after table is the load-bearing evidence, and the fix is correct whatever the precise shape of Qt's answer, as long as ours doesn't synthesise events — which it demonstrably doesn't. No socket tests added or removed. Every finding above is reasoned from code; I have no build and ran nothing.
6. Recommendation
Approve with nits. The change is narrow, platform-gated, root-caused rather than papered over, and backed by the real-OS-input proof the issue thread explicitly required — including control rows showing the popped-out pan path is untouched and that edge resize survives. The four nits are all documentation or disclosure, none of them merge-gating. Concrete next step: add the title-bar drag and double-click-maximize rows to the proof (or to "Not verified"), and consider the one-sentence #6224 pointer in window-chrome.md. Thanks for the unusually clear root-cause write-up and for the bridge-gap notes at the end — the "real-input mode" request is worth its own issue.
🤖 aethersdr-agent · cost: $10.4070 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (41 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: $10.5668 · model: claude-opus-5 |
…or the top and bottom band (aethersdr#6272). Principle VIII. Review follow-up. expandedFrameHit() took one band width for all four edges, built from SM_CXSIZEFRAME, while the WM_NCCALCSIZE block beside it uses SM_CXSIZEFRAME for left/right and SM_CYSIZEFRAME for top/bottom. It now takes borderX and borderY, and nativeEvent() passes SM_CXSIZEFRAME / SM_CYSIZEFRAME (+ SM_CXPADDEDBORDER) to match. The two metrics are equal on stock Windows, so behaviour there is unchanged. unified_title_bar_test adds a check with unequal bands. Making the top band read borderX fails it. window-chrome.md now says a Snap Layouts fix (aethersdr#6224) belongs in this handler (HTMAXBUTTON over the bar's maximize button), not in Qt, and that the top band covers the top few pixels of the bar, as Qt's did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@AetherClaude Thanks for the review. All four nits are answered below, at head 1. Vertical band from the horizontal metric: fixed in
2. Snap Layouts pointer: fixed in 3. Top band over the title bar: kept as is, deliberately. It matches what Qt's own hit test did before this PR, so it's not a regression. Matching Linux (#4886, no top band under the bar) would remove top-edge resizing on Windows, which is a behaviour change better decided on its own than folded into this fix. 4. Title-bar move and double-click-maximize: proven on the
I also re-ran the issue's trials on the same build to make sure the follow-up commit changed nothing: ☰ menu 5/5, panadapter right-click menu 5/5, double-click tune 5/5. 🤖 Generated with Claude Code |
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Approving at 21cae89fe. There were no outstanding findings: no review threads, no blocking reviews, and the bot's four nits are answered in 21cae89f:
- separate X/Y bands;
- the #6224 pointer in
window-chrome.md; - the top band kept deliberately and documented;
- title-bar drag and double-click-maximize proven with real
SendInput.
Verified here (Linux, Qt 6.12, offscreen):
unified_title_bar_testpasses all checks, including the newexpandedFrameHitcases.- Making the top band read
borderXfails "the left/right band and the top/bottom band follow their own widths". AetherSDRbuilds, and required CI is green, includingcheck-windows, which compiles the#ifdef Q_OS_WINhandler.
Not verified here: the Windows runtime behaviour itself. I'm relying on the author's real-input before/after proof on Windows 10 (☰ menu, panadapter context menu and double-click tune went 0/5 → 5/5; edge resize, drag and maximize still work).
Fixes #6272.
What
On Windows with Frameless Window on, the main window's ☰ menu and the panadapter's right-click menu flashed open and closed immediately, and double-click to tune did nothing. All three work with Frameless off. This regressed with the unified title bar (#6198) and is still on
mainafter #6269.Root cause
With Frameless on, the main window uses Qt's expanded client area with
CustomizeWindowHintand no title or caption-button hints (WindowChrome::chromeFlags). For exactly those flags, Qt 6.12 answersWM_NCHITTESTin theelse if (CustomizeWindowHint)branch ofQWindowsWindow::handleNonClientHitTest(qwindowswindow.cpp:3415-3427):GetAsyncKeyState.oldMouseButtonState, a function-static shared by every window), it synthesizes a mouse press or release withQWindowSystemInterface::handleMouseEvent<SynchronousDelivery>at the cursor, anywhere in the window.Windows sends
WM_NCHITTESTaround a real click, so the click arrives twice:popup()onpressed), the second press lands outside it and dismisses it, and the replay guard stops it reopening.exec()context menu is closed the same way.This also explains two puzzles from the reproduction on the issue:
WM_NCHITTEST.How
QWindowsContextruns the window's ownnativeEvent()before its hit-test switch (qwindowscontext.cpp:858vs:1019). Under the expanded client area,MainWindow::nativeEvent()now answersWM_NCHITTESTitself, with no synthetic events:HTLEFT…HTBOTTOMRIGHT) on a band ofSM_CXSIZEFRAME + SM_CXPADDEDBORDERat the window edges, the same band Qt used;HTCLIENTeverywhere else.WindowChrome::expandedFrameHit()is the pure part.window-chrome.mdnow says edge resize on this path rests on this handler. It applies on Windows 10 and 11 alike, since the Qt branch doesn't depend on the OS version.Popped-out panadapters use plain
Qt::FramelessWindowHint, which Qt hit-tests on its ordinary path. They aren't affected and aren't changed (proven below).+114 / −6 in 4 files; every behaviour change is inside
#ifdef Q_OS_WIN.Testing
Proof with the agent automation bridge, using real OS input
Run on Windows 10 22H2 (Intel HD 520), FLEX-6600 connected, AetherSDR UI scale 85%, your real settings with Frameless Window on. Builds came from the fork's Windows Installer workflow (Qt 6.12):
maine1785e78, run 37902283037.1c51a580, run 37902771818.Method:
SendInputat positions fromdump_tree, with physical = Qt logical × 0.85.Qt…PopupSaveBitswindows, 300 ms after each click.pan floatand targeted by its window title. Its click points sit below the slice flag; my first popped-out run was invalid because the click landed on the flag.Edge resize still works on the fix build. Real
SendInputdrags, with the window rect fromGetWindowRect:WM_NCHITTESTthereHTBOTTOM)HTLEFT)Three earlier drags didn't resize because the press landed on another window (the floating pan, then the floating TX Controls).
WindowFromPointconfirmed that, and those drags are excluded.Unit test (local, Qt 6.12.0 pinned, MSVC 2022, offscreen)
The new
expandedFrameHitchecks inunified_title_bar_testall pass:Mutation-checked:
One existing check in the same binary, "four radios fit whole in an 1800 px bar", fails in my local Windows offscreen run with and without this change (also on plain
main). CI onmainis green.Not verified
HTMAXBUTTONbefore either.Agent automation bridge: gaps found
clickAt,rightClickanddoubleClickdeliver Qt events and skipWM_NCHITTEST, so they can't reproduce input bugs below Qt. A real-input mode (SendInputat a widget's centre) would let the bridge prove this class of bug on its own.Theme Editor…); plain tooltips are a workaround.🤖 Generated with Claude Code