[codex] Fix narrow passband drag hit testing#3523
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes hit-testing for very narrow rendered RX filter passbands in SpectrumWidget so dragging inside the passband reliably moves the slice (VFO drag) instead of accidentally initiating a filter-edge resize when zoomed out.
Changes:
- Added shared pixel-based passband hit-test helpers to classify edge vs body interactions for narrow passbands.
- Updated active-slice
mousePressEvent()logic to use the new edge/body classifier. - Updated hover cursor edge detection in
mouseMoveEvent()to use the same classifier.
| static int filterInteriorGrabPx(int loX, int hiX, int grabPx) | ||
| { | ||
| const int widthPx = std::abs(hiX - loX); | ||
| if (widthPx <= kFilterPassbandMinBodyPx) { | ||
| return 0; | ||
| } | ||
| return std::min(grabPx, std::max(1, (widthPx - kFilterPassbandMinBodyPx) / 2)); | ||
| } |
| @@ -4273,7 +4312,7 @@ | |||
| const int hiX = mhzToX(ao->freqMhz + ao->filterHighHz / 1.0e6); | |||
| constexpr int GRAB = 5; | |||
| if (!foundCursor | |||
| && (std::abs(mx - loX) <= GRAB || std::abs(mx - hiX) <= GRAB)) { | |||
| && filterEdgeHitAtPixel(mx, loX, hiX, GRAB) != 0) { | |||
| setSpectrumCursor(Qt::SizeHorCursor); | |||
24ea85c to
d0f9b48
Compare
There was a problem hiding this comment.
Thanks @rfoust — this is a well-structured fix. Centralizing the edge/body classification so press, hover, the VFO-child event filter, and the QRhi polling path all agree is the right shape, and I verified the supporting details: setSpectrumCursor() already guards redundant installs (so the per-frame call in renderGpuFrame is cheap), VfoWidget builds all its children in buildUI() at construction (so installVfoCursorEventFilter catches them all), and the event filter never consumes events, so child buttons keep their clicks as described.
One real inconsistency — hover/press ordering for overlapping slices
sliceCursorShapeAt() checks the active overlay first, then inactive slices. mousePressEvent() does the opposite: the inactive-slice loop (badge → centerline → passband body, all emitting sliceClicked) runs before the active overlay's edge/body checks. So when an inactive slice's badge or passband overlaps the active slice's edge zone or passband — exactly the zoomed-out, slices-close scenario this PR targets — the cursor shows SizeHor/OpenHand (active-slice action) but the click activates the inactive slice instead. Since the whole point of the cursor work is previewing the action, suggest reordering sliceCursorShapeAt() to check inactive slices first, mirroring press order.
On the Copilot findings
- "
filterInteriorGrabPxforcesinsideGrabPx >= 1, leaving a 7px passband only 5px of body" — mostly a false positive. There is no>= 1clamp; forwidthPx = 7,(7 - 6) / 2 == 0, so no interior pixels are stolen and the body is the full 6px. However, there is a genuine off-by-one for even widths: the edge zones include their boundary pixel (mx <= loX + insideGrabPx), so the body iswidthPx − 2·insideGrabPx − 1pixels — 5px instead of 6 for widths 8, 10, …, 22. If the ≥6px guarantee is meant to be strict,(widthPx - kFilterPassbandMinBodyPx - 1) / 2fixes it. Minor either way. - "Hover still uses a 5px grab radius vs 8px on press" — false positive. This PR deletes the old
GRAB = 5hover path; hover now goes throughsliceCursorShapeAt()→filterEdgeHitAtPixel()with the samekFilterEdgeGrabPx = 8as press.
Two small notes
- The
childAt()guard bypass inmouseMoveEventmeans the #2355 tooltip protection no longer applies where overlay-menu buttons overlap passband/edge zones —QToolTip::hideText()paths can kill their tooltips again there. Probably an acceptable tradeoff for the cursor affordance, but worth being aware it partially reverts #2355 in those zones. - The validation section says "Manual reporter retest confirmed the cursor behavior was better before this push" — as written that says the reporter preferred the previous behavior. Assuming that's a typo for "better than before this push", but please confirm.
No scope, settings-convention, or resource concerns — the diff matches the stated scope and the new helpers are pure functions.
🤖 aethersdr-agent · cost: $10.5651 · model: claude-fable-5
d0f9b48 to
697b28b
Compare
|
Updated after the latest review and upstream rebase. Addressed review feedback:
Notes on prior comments:
Validation on the rebased head
|
Root cause
SpectrumWidget::mousePressEvent()used a fixed 8 px grab zone for each filter edge before checking whether the click was inside the passband body. When the panadapter was zoomed far enough out, a narrow slice passband could render at only a few pixels wide, so the edge grab zones overlapped most or all of the passband. A click intended to drag the whole slice was then classified as a filter-edge drag and changed the passband width instead.A second cursor-specific issue hid the intended affordance after the hit testing was corrected: the hover cursor path could be bypassed when VFO child widgets covered the passband area, and the QRhi render-time cursor polling path updated tracking overlays without also updating the OS cursor shape. That left users seeing the inherited crosshair, or a pointing-hand cursor from an overlaid VFO child button, instead of the resize cursor at the true filter edges.
Review also caught one ordering mismatch: the cursor helper checked the active slice before inactive-slice activation targets, while press handling activates inactive slice targets first. In overlapping-slice cases, that could make the cursor preview a different action than the click would perform.
User impact
Users with narrow RX filters on a zoomed-out panadapter can move a slice by dragging inside the passband instead of accidentally latching onto an edge. Edge resizing remains available near the passband edges.
The hover cursor now previews the action before clicking: horizontal resize at filter edges, open hand for passband-body moves, closed hand once the move drag is active, and existing pointing-hand behavior for actual slice/button activation targets.
Change summary
SpectrumWidgetwhen they overlap slice passband/edge zones, without consuming their clicks.upstream/mainand kept the new adjustablefreqScaleH()layout math in the cursor helper and conflict resolution.Validation
git diff --checkcmake --build build -j8after rebasing ontoupstream/mainat6a0648b5python3 tools/check_a11y.py src/gui/SpectrumWidget.cpp src/gui/SpectrumWidget.hctest --test-dir build --output-on-failure --parallel 8 -E theme_manager_testpassed 32/32 tests