Repository navigation
fix(gui): the Windows title bar answers as the caption, so Snap works again (#6825) - #6830
nigelfenton wants to merge 3 commits into
Conversation
… again (#6825). Principle VIII. Fixes #6825. ## What On Windows with the expanded client area, `MainWindow::nativeEvent` answered `WM_NCHITTEST` with the resize band at the edges and `HTCLIENT` everywhere else, so the 52 px title bar never reported `HTCAPTION`. Windows keys drag-to-edge Snap and the Snap Assist picker off that answer, so neither was offered: dragging the window to a screen edge did nothing. `expandedFrameHit()` gains a `Caption` hit and an opt-in `captionHeight`. `nativeEvent` passes the bar's height, scaled to the physical pixels `WM_NCHITTEST` works in, but only when `TitleBar::isSystemMoveAreaAt()` says the cursor is over the bar's empty background — the same test the bar's own drag already uses, so a point either starts a system move or stays client, never both. Answering `HTCAPTION` over the tabs, menus and caption buttons is what closed menus as they opened (#6272), and those keep reporting `HTCLIENT`. `captionHeight` defaults to 0, so every other caller is unchanged. `isSystemMoveAreaAt()` has been public and uncalled since #6198 removed the custom frame; this restores the `HTCAPTION` clause that #3069 added and #6198 dropped. ## Measured on Windows 11 (26300), AetherSDR 26.10.4, Qt 6.12 Hit test down the centre of the title bar, before and after: | offset from window top | before | after | Notepad (control) | |---|---|---|---| | +0..+7 px | HTTOP | HTTOP | HTTOP | | +8..+43 px | HTCLIENT | **HTCAPTION** | HTCAPTION | | +44 px and below | HTCLIENT | HTCLIENT | HTCLIENT | Scanning across the bar after the fix, the ☰ menu, the tab strip's controls and the caption buttons all still report `HTCLIENT`; only the empty background between them reports `HTCAPTION`. `WS_MAXIMIZEBOX` was already set (`applyWindowsCaptionStyles`), which is why Win+Left/Right always worked while the mouse drag did not — keyboard snap bypasses hit-testing. ## Tests `unified_title_bar_test` gains 10 assertions: the caption strip, its lower boundary, the resize band and corners still winning over it, the maximized case (the caption must answer or a maximized window cannot be dragged free), and that a 0 `captionHeight` leaves the old answer untouched. Verified they fail when the caption clause is disabled (3 fail, the rest pass). The suite's pre-existing "four radios fit whole in an 1800 px bar" failure on this machine is a font-metrics issue, confirmed identical on unmodified main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Verified by hand on Windows 11 — both reported symptoms, plus the #6272 regressionThe PR body left three things measured but not clicked, because a hit-test scan is not a Host: aurora13, Windows 11 build 26300, 2 × 1920×1080 @ 100 %, Qt 6.12.0, this branch
The third one is the regression this change could plausibly have caused: answering A note for anyone trying to re-verify this with a synthetic hit-testProbing with m_titleBar->isSystemMoveAreaAt(QCursor::pos())which keys off where the pointer actually is, not the coordinate carried in the message. On the Snap Assist half of #6825The reporter also said AetherSDR was missing from the Snap Assist picker. That half never 73, Nigel G0JKN/W3 🤖 Generated with Claude Code |
rfoust
left a comment
rfoust
left a comment
There was a problem hiding this comment.
The drag-to-edge fix is in scope for #6825, but I am requesting changes for the hit-test coordinate mismatch below and the missing accompanying user documentation. The separate report that AetherSDR is absent from Snap Assist remains unconfirmed: the follow-up says it was already listed before this change. The maximize-button hover flyout is explicitly outside this PR.
Scope
| File | Change | Claimed in title/body? | Verdict |
|---|---|---|---|
src/gui/MainWindow.cpp |
Return HTCAPTION over the title bar's draggable background |
Yes | In scope; coordinate defect below |
src/gui/WindowChrome.h |
Add optional caption classification while keeping resize edges first | Yes | In scope |
tests/unified_title_bar_test.cpp |
Caption boundaries, resize priority, maximized and default-height cases | Yes | In scope |
The single commit is focused on #6825. I found no unrelated changes or preference-driven restyling.
Changes requested
-
[P2] Classify the point in the hit-test message, rather than the live cursor.
src/gui/MainWindow.cpp:3913-3919, especially line 3915.expandedFrameHit()classifies the physical screen coordinate extracted frommsg->lParam, but its caption enablement comes fromisSystemMoveAreaAt(QCursor::pos()). Those can be different points. WM_NCHITTEST's contract is to determine which part of the window corresponds to the supplied screen coordinate; it can also be queried by functions such asWindowFromPoint.I exercised the caller calculation with the real
TitleBarwidgets and productionWindowChrome::expandedFrameHit()in a socket-free, offscreen Qt probe at DPR 1:REVIEW: pointer=background query=background => Caption REVIEW: pointer=background query=menu => Caption FAIL: review: a menu hit query must remain Client while the pointer is elsewhere REVIEW: pointer=menu query=background => ClientThe menu-point assertion passes on current main and fails on the PR merged with that same main. Thus the interactive-widget exclusion depends on an unrelated cursor position: a query for the menu button can return caption, while a query for draggable background can return client. The PR follow-up's synthetic-query observation is consistent with this mismatch, rather than a reason for the query to return the wrong hit.
Please convert the message's native screen coordinate into the Qt coordinate space expected by
isSystemMoveAreaAt()and use that point consistently, taking DPI and monitor placement into account. Simply passing raw physical pixels to that Qt method would not be sufficient. Add a socket-free regression case with the queried point and live cursor over different title-bar regions, then recheck native Windows Snap and menu interactions.This reproduction covers the caller expression with actual Qt widgets; I have not independently observed a native Windows menu click fail.
-
Include the required user-documentation update.
src/gui/MainWindow.cpp:3918-3919introduces the user-visible caption/Snap behavior without any accompanyingdocs/user/change.AGENTS.md:75 requires: “Every user-visible change updates the user docs in the same PR.” Please update the Windows guide or title-bar reference to describe drag-to-edge Snap, while retaining the separate maximize-hover limitation. No CHANGELOG entry is requested.
Non-blocking nits
- PR body, “Not verified”: please bring it into step with the later manual Windows checks.
src/gui/WindowChrome.h:94-96and the PR body: narrow the statement that caption hit testing is required for listing the window in Snap Assist. The follow-up reports that the window was listed before the fix; that part of #6825 still needs reporter confirmation.
Verification and limitations
- Reviewed head:
de3753ab33e3481d23138b8980b747850588f7ec. - Current main used for both local comparisons:
db40ab099d3ec0bff5fb412acca4a42ae6475df7. - Computed current-main-plus-PR tree:
b7c9cb344d3c25ea45fe3b80ca4265b07cf2b670; tested as an uncommitted merge in an isolated worktree. - Configured with the dedicated macOS ARM64 toolchain, RelWithDebInfo, Qt 6.12.0 and RADE enabled. Built only
unified_title_bar_test. Host/system processor and the test executable were ARM64; no RNNoise x86 sources were in the build graph. ctest --test-dir build-codex-arm64 -R '^unified_title_bar_test$' --no-tests=error --output-on-failure: 1 selected / 1 passed on base and merge. The reported font-metrics failure did not occur on this Mac.- Disabling the production caption clause made exactly three assertions fail; restoring it passed. The separate menu-coordinate probe passed on base and failed on merge; removing the temporary probe restored 1/1 passing.
- Checks are green on the unchanged PR head. The Windows CI checkout log identifies tested merge
d8a9aa344be47f44fbe3433f110475b8ee67d859against base772abac77e23c93d8e6a40daa3165725106659fa, not today's main. GitHub's current merge ref9cd66e255069738bfc17499bb31a00a564b83a30is also based on older mainee9f729bfc943b3df67a33d853d8f6db8e64f0b4. - I did not build the default all-target graph or independently run the Windows application. The affected native Windows hit-testing and desktop-shell Snap path cannot be exercised through the focused macOS target. No automation-bridge or radio session was driven. The author's later manual Windows checks are useful evidence, but are distinct from my local reproduction.
- Manual adversarial review and a second lifecycle/failure-mode pass; no separate automated review tool was available.
What I tried to break
- Resize edges/corners still beat the caption, and the caption's lower boundary and outside-window cases passed.
- A zero caption height preserves existing answers; the maximized zero-resize-band caption cases passed.
- Mutation testing confirmed that the new helper assertions detect removal of the caption clause.
- I investigated a possible Minimal Mode double-click regression and found the existing
MainWindow::changeEvent()recovery path, so I am not reporting it as a defect.
Please correct the coordinate handling and accompanying documentation, then refresh CI and the native Windows checks on the updated revision.
… cursor WM_NCHITTEST carries the screen coordinate being asked about, and the shell (or WindowFromPoint) may query it while the pointer is somewhere else. The caption clause took its point from msg->lParam but gated on isSystemMoveAreaAt(QCursor::pos()), so a query over the menu button could answer HTCAPTION and a query over draggable background could answer HTCLIENT, depending only on where the pointer happened to be. Convert the message's physical screen point into Qt's logical global space through the window's own two rects — frameGeometry() against GetWindowRect — and ask about that. Going through the rects keeps whatever rounding the platform applied instead of re-deriving it from a ratio that need not divide evenly at fractional scaling. Also document drag-to-edge Snap in the Windows user guide, keeping the maximize-hover flyout (#6224) listed as the separate limitation it is, and narrow the claim that caption hit testing is what lists the window in the Snap Assist picker — that half of #6825 did not reproduce here. Reported by @rfoust in review of #6830. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@rfoust — both addressed in 1. [P2] Classify the queried point, not the live cursor — fixedYou were right, and I want to be straight about how close I came to arguing the opposite. After opening this PR I ran a synthetic The fix adds if (m_titleBar && m_titleBar->isVisible()
&& m_titleBar->isSystemMoveAreaAt(
WindowChrome::logicalScreenPoint(point, rect, frameGeometry()))) {On your point that raw physical pixels would not be sufficient: agreed, Regression coverage (socket-free, in Mutation check: forcing Rechecked natively on Windows 11 (build 26300, Qt 6.12.0), cursor parked ~800 px from the bar: Before the fix that same scan was 2. User documentation — added
Nits — both taken
On your note that you could not exercise the native path from the macOS target: that is the right call, and the three behaviours above are the part no CI job covers either. 73, Nigel G0JKN/W3 🤖 Addressed by Claude Code |
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue fit
Yes, for the drag half of #6825. Before this PR, WM_NCHITTEST gave the resize band at the edges and HTCLIENT everywhere else, so Windows never treated a drag as a caption drag and never offered drag-to-edge Snap. The new FrameHit::Caption answers HTCAPTION only where TitleBar::isSystemMoveAreaAt() is true, which is the same test the bar's own drag uses. The reporter's other complaint, that AetherSDR was missing from the Snap Assist picker, was not reproduced in the issue thread. It plausibly follows from the same cause, but it can't be confirmed until the reporter re-tests a build. The Snap Layouts hover flyout stays open under #6224. The doc says so, which matches the known HTMAXBUTTON gap.
Scope
| File | Change | Claimed | Verdict |
|---|---|---|---|
src/gui/WindowChrome.h |
Caption hit, captionHeight parameter, logicalScreenPoint() |
yes | in scope |
src/gui/MainWindow.cpp |
HTCAPTION mapping; caption gated on isSystemMoveAreaAt() |
yes | in scope |
tests/unified_title_bar_test.cpp |
pure-function tests for both helpers | yes | in scope |
docs/user/docs/windows.md |
Snap bullet, including the #6224 flyout limitation | yes | in scope |
Everything in the diff is explained by the issue. There is no new settings key, protocol surface or default change. The two commits are both about the caption hit test.
Blockers
None.
Non-blocking findings
-
On Windows, the tooltips on two title-bar labels stop showing.
src/gui/TitleBar.cpp:333andsrc/gui/TitleBar.cpp:1331-1334(reasoned from code, not seen running):m_txTimerLabel->setToolTip(QStringLiteral("Transmit timer — elapsed key-down time")); ... markDragHandle(m_txTimerLabel);
m_experimentalRadioLabelfollows the same pattern with "%1 radio support is experimental". Both labels are drag handles, soisSystemMoveAreaAt()returns true over them and they now answerHTCAPTION. Over a caption, Windows sendsWM_NCMOUSEMOVEinstead ofWM_MOUSEMOVE. Qt delivers that asNonClientAreaMouseMove, which does not start the tooltip timer, so neither tooltip will appear on Windows. The accessible description on the experimental label still carries the warning, so screen-reader users lose nothing; sighted users lose the hover text. For the same reason, Windows (not Qt) now chooses the cursor over the gutter, brand, audio cluster and these labels. The open-hand cursor set inmarkDragHandle()(TitleBar.cpp:807, and the comment at:298) will most likely show as the standard arrow on Windows.
Two possible fixes:- Keep
HTCLIENTover drag handles that carry a tooltip. This is a Windows-only check innativeEvent, so the Linux drag is unaffected; the bar's ownstartSystemMove()path still drags from those labels, just without Snap from that spot. - Accept the trade-off and add one sentence to the PR body so the squash commit records it.
Either is fine. The maintainer should choose, because losing the experimental-radio hover text is a user-visible consequence the body does not state.
- Keep
What I tried to break
- #6272 regression (menus closing as they open):
HTCAPTIONis gated per point onisSystemMoveAreaAt(). That function returns false over the ☰ button, the menu-bar actions, the radio tabs, multiFLEX, the audio buttons and sliders, the dock and pop-out labels, and the caption buttons (TitleBar.cpp:817-842). Only drag handles and the bare background become caption. The resize band is still checked first, and the tests pin Top, TopLeft, TopRight and Left above Caption. - Maximized and fullscreen: with the band at 0/0,
Captionstill answers, so a maximized window can be dragged down off the top of the screen. While maximized,WM_NCCALCSIZEpushes the client area down byborderY, butcaptionHeightis counted fromwindow.top. So roughly the bottomborderYphysical pixels of the bar stay client, and dragging there falls back to the bar's ownstartSystemMove(). That costs Snap in a small strip and breaks nothing, so I'm not raising it as a finding. - DPI mapping:
logicalScreenPoint()scales throughGetWindowRectandframeGeometry()rather than the device pixel ratio. It guards against a zero or negative rect, and the 150% test allows ±1–2 px. Under the expanded client area the client rect equals the window rect, so the two rects describe the same window. I could not check a window straddling two monitors with different DPI. - Tests vs the unfixed code: the new checks would not compile against the old header (
Hit::Captiondidn't exist), so they do pin the new behaviour. They only cover the pure helpers, though. ThenativeEventwiring is Win32-only and has no socket-free seam beyond what is already tested, so that is a coverage limit, not a gap.unified_title_bar_testis registered intests/tests.cmake:9202, but it is not inci.yml's-Rfilters. Green CI here meanscheck-windows/check-macos/buildcompiled it; it does not mean the test ran. - Security: a security-audit (CodeGuard) pass over the four changed files found nothing. Every input is the app's own
WM_NCHITTESTand window rect. The 1326 CodeGuard hits are on main-tip workflow, script and core files this PR does not touch, and I dropped them.
No test sockets were added or modified. All findings come from reading the code; I did not run the app on Windows.
Recommendation
Approve with non-blocking findings. The fix is narrow and correct at the root cause, and it reuses the existing move-area test instead of introducing a second definition of "draggable". The only open item is the tooltip/cursor trade-off over drag-handle labels on Windows. The maintainer should either have those labels excluded from the caption or have the trade-off stated in the PR body. Thanks, Nigel. The before/after hit-code table against Notepad in the issue thread was very useful to check this against.
🤖 aethersdr-agent · cost: $1.0922 · model: claude-opus-5-5
|
Cisco CodeGuard — static analysis of this PR (1368 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: $1.1422 · model: claude-opus-5-5 |
…er text survives (#6825) Over a caption Windows sends WM_NCMOUSEMOVE, which Qt delivers as NonClientAreaMouseMove and which never starts the tooltip timer. The caption gate used isSystemMoveAreaAt(), so every drag handle answered HTCAPTION -- including the three that carry a tooltip: the brand mark ("AetherSDR"), the transmit timer, and the EXPERIMENTAL radio label. Their hover text would have stopped appearing on Windows. Measured on Windows 11 before the change: at every HTCAPTION point the tooltip was suppressed, and at every HTCLIENT point over a tooltipped widget it appeared -- so the mechanism is real, not theoretical. isCaptionMoveAreaAt() is isSystemMoveAreaAt() minus those handles. It is used only by the Windows caption gate, so the bar's own startSystemMove() is unchanged and the Linux drag is untouched; those labels still move the window, they just do not start a Snap drag. 48% of the bar still answers HTCAPTION, including one contiguous 392 px run, so drag-to-edge Snap has ample target. Found in review by rfoust. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — the tooltip finding is real, and I reproduced it before fixing it. Pushed as The drag-handle tooltip trade-off — fixed, not documented awayYou reasoned it from the code; I measured it on Windows 11 first, because this is the second time on this PR that a mechanism I could have argued either way turned out to be worth testing. Hovering each point on the bar and watching for a Every One correction to the finding: there are three affected handles, not two. I took your first option. New Snap still has ample target. Sweeping the bar at 4 px steps after the change: 716 of 1500 px answer Test: Docs: the Snap bullet in Two notes on the restThe cursor prediction is only partly right. I checked CodeGuard (the other comment): agreed, nothing here. I checked the cited paths against the PR's file list rather than taking the count at face value. All 1368 findings land in On the maximized-bar strip you decided not to raise: agreed it is not worth a finding, and it is now slightly moot, since dragging there falls back to 73, Nigel G0JKN/W3 🤖 Addressed by Claude Code |
Fixes #6825.
What
On Windows with the expanded client area,
MainWindow::nativeEventansweredWM_NCHITTESTwith the resize band at the edges andHTCLIENTeverywhere else, so the 52 px title bar never reportedHTCAPTION. Windows keys drag-to-edge Snap off that answer, so it was never offered: dragging the window to a screen edge did nothing.expandedFrameHit()gains aCaptionhit and an opt-incaptionHeight.nativeEventpasses the bar's height, scaled to the physical pixelsWM_NCHITTESTworks in, but only whenTitleBar::isSystemMoveAreaAt()says the cursor is over the bar's empty background — the same test the bar's own drag already uses, so a point either starts a system move or stays client, never both. AnsweringHTCAPTIONover the tabs, menus and caption buttons is what closed menus as they opened (#6272), and those keep reportingHTCLIENT.captionHeightdefaults to 0, so every other caller is unchanged.isSystemMoveAreaAt()has been public and uncalled since #6198 removed the custom frame; this restores theHTCAPTIONclause that #3069 added and #6198 dropped.Measured on Windows 11 (26300), AetherSDR 26.10.4, Qt 6.12
Hit test down the centre of the title bar, before and after:
Scanning across the bar after the fix, the ☰ menu, the tab strip's controls and the caption buttons all still report
HTCLIENT; only the empty background between them reportsHTCAPTION. The caption ends at ~+43 rather than +52 becauseisSystemMoveAreaAt()reports the lower strip as occupied by widgets — the two mechanisms agreeing.WS_MAXIMIZEBOXwas already set (applyWindowsCaptionStyles), which is why Win+Left/Right always worked while the mouse drag did not — keyboard snap bypasses hit-testing. That asymmetry is what isolated the fault to hit-testing rather than window styles, and Win+Arrow is a usable workaround for anyone on a release before this lands.Relationship to existing issues
Same root cause as #6224 item W1 ("Snap Layouts hover (Windows 11). This is a known regression."), but W1 covers only the hover flyout over the maximize button. #6825 adds drag-to-edge, and also reports the window missing from the Snap Assist picker. This PR fixes the drag; the
HTMAXBUTTONhalf of W1 is untouched and still open. The picker symptom did not reproduce here — the window was already listed before this change — so it still needs the reporter's confirmation.The chrome landed in v26.10.2 (
5f09371e, PR #6198), not 26.10.4 as the issue title suggests — 26.10.4's onlyTitleBar.cppchange is the update badge, and the drag path is byte-identical to 26.10.3. Worth confirming with the reporter which version was last good.Tooltipped drag handles stay client (review follow-up)
Over a caption Windows sends
WM_NCMOUSEMOVE, which Qt delivers asNonClientAreaMouseMove; that never starts the tooltip timer. Gating thecaption on
isSystemMoveAreaAt()therefore cost the hover text on the threedrag handles that carry a tooltip — the brand mark, the transmit timer and
the EXPERIMENTAL radio label. Measured on Windows 11: every
HTCAPTIONpointsuppressed its tooltip, every
HTCLIENTpoint over a tooltipped widget showedone.
TitleBar::isCaptionMoveAreaAt()isisSystemMoveAreaAt()minus thosehandles, and only the Windows caption gate uses it.
isSystemMoveAreaAt()isunchanged, so the bar's own
startSystemMove()and the Linux drag behave asbefore: those three still move the window, they just do not start a Snap drag.
After the change 716 of 1500 px of the bar still answer
HTCAPTION(48%),including one contiguous 392 px run, so drag-to-edge Snap keeps ample target.
Tests
unified_title_bar_testgains 10 assertions: the caption strip, its lower boundary, the resize band and corners still winning over it, the maximized case (the caption must answer or a maximized window cannot be dragged free of the top of the screen), and that a 0captionHeightleaves the old answer untouched.Verified the tests bite: with the caption clause disabled, exactly 3 fail and the rest pass.
The suite has a pre-existing failure on this machine at "four radios fit whole in an 1800 px bar" — font metrics, confirmed identical on unmodified
mainby stashing this change and rebuilding. Because that masks the new assertions in the full binary, they were also run in a standalone harness that includes the realWindowChrome.h: 21/21 pass.Verified by hand on Windows 11
aurora13, Windows 11 build 26300, Qt 6.12.0, this branch built locally.
Profiles, Tools, View, Window and Help all held.
After the coordinate fix below, a synthetic
SendMessage(WM_NCHITTEST, ...)scanwith the cursor parked ~800 px away from the bar reports
HTCAPTIONat +8…+43 pxand
HTCLIENTover the ☰ button, the controls and the caption buttons — so theanswer now depends on the queried point rather than on where the pointer is.
Not verified
The Snap Layouts hover flyout over the maximize button (#6224 W1) — untouched by
this PR and still open. The Snap Assist picker half of #6825 did not reproduce on
this machine and still needs the reporter's confirmation.
🤖 Generated with Claude Code