Add ctrl-drag waterfall rate control#2783
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a Ctrl-drag gesture on the waterfall time-scale strip to adjust the waterfall line duration (refresh rate) directly from the spectrum display, with two-way synchronization to the Display Settings rate slider. The accepted range is tightened to the radio-supported 71–100 ms and the history-capacity computation is decoupled from the current rate.
Changes:
- New
m_draggingTimeScaleRatedrag state + Ctrl-click (Ctrl/Meta on macOS) handling in mouse press/move/release, emitting a newwaterfallLineDurationChangeRequested(int)signal wired throughMainWindowto bothsetWfLineDurationand thedisplay panafall set ... line_duration=radio command. setWfLineDurationis clamped to[kWaterfallRateMinMs=71, kWaterfallRateMaxMs=100], becomes a no-op on unchanged values, and now syncs the overlay menu rate slider via the newSpectrumOverlayMenu::syncWfLineDurationhelper (using slider-↔-duration conversion helpers).- Removes the unused client-side row-averaging members;
waterfallHistoryCapacityRows()now sizes for the minimum line duration; the time-scale label usesm_wfLineDurationinstead of the measuredm_wfMsPerRow.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/gui/SpectrumWidget.h | New rate-drag signal/state members; removes unused waterfall averaging members. |
| src/gui/SpectrumWidget.cpp | Implements Ctrl-drag rate gesture, narrows clamp range, syncs overlay, refactors updateWaterfallRow timestamp use, switches history-capacity and time-scale rendering to m_wfLineDuration. |
| src/gui/SpectrumOverlayMenu.h | Declares syncWfLineDuration. |
| src/gui/SpectrumOverlayMenu.cpp | Adds slider↔line-duration conversion helpers, factors slider sync into syncWfLineDuration, tweaks tooltip wording. |
| src/gui/MainWindow.cpp | Wires the new waterfallLineDurationChangeRequested signal to the shared apply-line-duration lambda alongside the existing overlay slider signal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| m_blackSlider->setToolTip("Waterfall black level. Decrease to darken the noise floor."); | ||
| if (m_autoBlackBtn) m_autoBlackBtn->setToolTip("Automatically adjusts the waterfall black level to match the current noise floor."); | ||
| m_rateSlider->setToolTip("Waterfall line duration. Higher values scroll faster."); | ||
| m_rateSlider->setToolTip("Waterfall rate. Higher values scroll faster."); |
| @@ -1307,7 +1324,7 @@ void SpectrumWidget::resetWfTimeScale() { | |||
|
|
|||
| int SpectrumWidget::waterfallHistoryCapacityRows() const | |||
| { | |||
| kWaterfallRateMaxMs); | ||
|
|
||
| if (newMs != m_wfLineDuration) { | ||
| setWfLineDuration(newMs); |
| const int dy = m_timeScaleDragStartY - y; | ||
| const int rangeMs = kWaterfallRateMaxMs - kWaterfallRateMinMs; | ||
| const int deltaMs = static_cast<int>( | ||
| std::round((static_cast<double>(dy) / dragHeight) * rangeMs)); | ||
| const int newMs = std::clamp(m_timeScaleDragStartLineDuration + deltaMs, | ||
| kWaterfallRateMinMs, | ||
| kWaterfallRateMaxMs); |
| const int clamped = std::clamp(ms, kWaterfallRateMinMs, kWaterfallRateMaxMs); | ||
| if (m_wfLineDuration == clamped) { | ||
| if (m_overlayMenu) { | ||
| m_overlayMenu->syncWfLineDuration(m_wfLineDuration); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| m_wfLineDuration = clamped; |
| if (y >= wfY) { | ||
| const QRect wfRect(0, wfY, width(), height() - wfY); | ||
| const QRect timeScaleRect = waterfallTimeScaleRect(wfRect); | ||
| const QPoint pos = ev->position().toPoint(); | ||
| const Qt::KeyboardModifiers modifiers = | ||
| ev->modifiers() | QGuiApplication::keyboardModifiers(); | ||
| #ifdef Q_OS_MAC | ||
| const bool rateModifier = modifiers.testFlag(Qt::ControlModifier) | ||
| || modifiers.testFlag(Qt::MetaModifier); | ||
| const bool rateClick = rateModifier | ||
| && (ev->button() == Qt::LeftButton || ev->button() == Qt::RightButton); | ||
| #else | ||
| const bool rateModifier = modifiers.testFlag(Qt::ControlModifier); | ||
| const bool rateClick = rateModifier && ev->button() == Qt::LeftButton; | ||
| #endif | ||
| if (rateClick && timeScaleRect.contains(pos)) { | ||
| m_draggingTimeScaleRate = true; | ||
| m_timeScaleDragStartY = y; | ||
| m_timeScaleDragStartLineDuration = m_wfLineDuration; | ||
| setSpectrumCursor(Qt::SizeVerCursor); | ||
| ev->accept(); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (y >= wfY && ev->button() == Qt::LeftButton) { | ||
| const QRect wfRect(0, wfY, width(), height() - wfY); | ||
| const QRect timeScaleRect = waterfallTimeScaleRect(wfRect); |
There was a problem hiding this comment.
Thanks for the contribution, @rfoust — direct-on-display rate control is a nice UX win, and keeping the overlay slider in sync through syncWfLineDuration() is the right approach. I've verified the six Copilot findings against the diff; most are valid and worth addressing before merge. Grouped by severity:
Likely bug — please address
1. setWfLineDuration clamp narrowed from 50–500 to 71–100 (SpectrumWidget.cpp:1208). This function is on the inbound path for any value the radio reports (and for restored settings). Silently snapping a radio-reported line_duration outside the UI range will desync client and radio state. The UI/slider range is fine at 71–100, but I'd keep the function-level clamp wider (matching what the firmware can report) and only enforce 71–100 at the gesture/slider input sites. Same applies to the new clamp in loadSettings() — if anyone ever has a stored value of 100+ from the old range, it'll silently become 100 instead of being restored.
2. Rate slider tooltip is inverted (SpectrumOverlayMenu.cpp:1118). Slider 1→71 ms, 30→100 ms, so higher slider value = longer per-row duration = slower scroll. The tooltip says "Higher values scroll faster." This is pre-existing wording, but this PR is the right place to fix it ("Higher values scroll slower" or "Lower values scroll faster").
3. Drag gesture direction is inconsistent with the slider. In mouseMoveEvent (SpectrumWidget.cpp:3239), dy = m_timeScaleDragStartY - y is positive on upward drag, which increases lineDuration → slows the waterfall. So drag-up = slower, but the slider currently labels "higher = faster" (see #2). Either interpretation is defensible, but pick one and make both consistent. If "drag up = expand time depth = slower" is the intent, that's reasonable — just align the tooltip.
Fragile but works today
4. Redundant re-entrant call (SpectrumWidget.cpp:3242). The drag handler calls setWfLineDuration(newMs) and emits waterfallLineDurationChangeRequested(newMs), which MainWindow::wirePanadapter routes back into setWfLineDuration(ms) before sending the radio command. The new early-return on equal value masks this. Suggest: have setWfLineDuration itself emit the request (or send the radio command), and have the drag handler only call setWfLineDuration. That keeps a single entry point and removes the need for the equal-value guard to be load-bearing.
Worth a comment
5. waterfallHistoryCapacityRows() now always uses kWaterfallRateMinMs (SpectrumWidget.cpp:1326). Presumably intentional so changing rates doesn't invalidate existing history — but the function name and the adjacent comment ("ms-per-row derived from radio tile timecodes") no longer match. Either rename to reflect "capacity for fastest rate" or add a one-liner explaining why the constant is used. Also worth confirming what the invariant is now: "20 min of history" or "buffer sized for 20 min at the fastest rate, less at slower rates" — these differ.
Nit (skip if you'd like)
6. Duplicated wfRect/timeScaleRect computation in mousePressEvent (SpectrumWidget.cpp:2724 vs the existing left-click block right below). Cheap, but hoisting the locals would keep the two adjacent if (y >= wfY ...) branches in sync.
Overall the structure looks good and the two-way sync between the menu slider and the display gesture is clean. The clamp narrowing in #1 is the only item I'd consider blocking on.
3549949 to
325d08a
Compare
|
Addressed the review feedback in the force-pushed single-commit update:
Validated locally with |
|
Claude here — landed at 03:04 UTC. Welcome back, Robbie! 🎉 Clean stale-base this time — three-way merge against current main resolved without touching the files that #2780 silently bumped into. The bidirectional sync between the slider and the drag gesture is the right pattern (`QSignalBlocker` in `syncWfLineDuration` cleanly breaks the feedback loop), and sizing the waterfall history buffer for the fastest accepted rate means rate changes no longer discard history — a thoughtful trade-off. The "Higher values scroll slower" tooltip fix was an unexpected bonus; the original was wrong. One question for you before we close the loop: The macOS branch in `mousePressEvent` accepts both `LeftButton` AND `RightButton` as starting a Ctrl/Cmd rate drag: ```cpp The Linux/Windows branch only accepts `LeftButton`. Was the Right-on-Mac inclusion deliberate (e.g., to handle trackpad-with-two-finger-click as right-button + Cmd combinations), or was it incidental from the modifier-detection rewrite? Right-click typically opens a context menu on macOS so it's not the most obvious affordance for rate drag — happy to leave it if you intended it, but if it was accidental I'd suggest a follow-up to drop the RightButton check so the gesture surface is consistent across platforms. Today's tally for you: #2780 → cherry-picked into #2786 (panadapter persistence + macOS GPU lifecycle), plus this PR (#2783). Both substantive contributions. Thanks again! 73, Jeremy KK7GWY & Claude (AI dev partner) |
…2869) ## Summary - Ctrl+wheel now zooms the panadapter bandwidth in/out by ×1.5 per scroll step, anchoring on the frequency under the mouse cursor. - Plain wheel scroll is unchanged — it continues to tune the VFO. ## Implementation Single change in `SpectrumWidget::wheelEvent`: after the debounce/step calculation, an early branch catches `Qt::ControlModifier` and runs the cursor-anchored zoom, then returns before the existing VFO tune path. The zoom math mirrors the existing NativeGesture pinch-to-zoom exactly: ```cpp const double mouseXFrac = ev->position().x() / width() - 0.5; const double anchorMhz = m_centerMhz + mouseXFrac * m_bandwidthMhz; const double newCenter = std::max(anchorMhz - mouseXFrac * newBw, newBw / 2.0); ``` The `std::max(..., newBw / 2.0)` clamp prevents the left edge from going below 0 Hz, consistent with PR #2867. ## UX consistency note The existing `+`/`−` zoom buttons use a different anchor policy: zoom-in re-centers on the active VFO frequency (added in #1932 to prevent repeated clicks pushing the slice off-screen), zoom-out keeps the pan center fixed. That behavior makes sense for a discrete click where the cursor position carries no intent. Ctrl+wheel is a continuous, cursor-positioned interaction — the user is looking at a specific part of the spectrum when they scroll. Cursor-anchored zoom is therefore the correct policy here, matching the NativeGesture pinch-to-zoom and every major spectrum/map application that implements scroll-to-zoom. The button inconsistency is deliberate and acceptable. Ctrl is already the modifier used for dBm-scale drag (PR #2717) and waterfall time-scale drag (PR #2783), so Ctrl+wheel is consistent with the existing convention of Ctrl = “modify the view, not the VFO.” ## Testing - Scroll wheel without Ctrl: VFO tunes as before, no regression. - Ctrl+scroll up: bandwidth narrows, cursor frequency stays fixed. - Ctrl+scroll down: bandwidth widens, cursor frequency stays fixed. - Ctrl+scroll at band edge: 0 Hz clamp prevents left edge going negative. - Ctrl+scroll at BW limits: clamped to `m_minBwMhz`/`m_maxBwMhz`, no crash. - Pinch-to-zoom: unaffected. Fixes #1518 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…dr#1518) (aethersdr#2869) ## Summary - Ctrl+wheel now zooms the panadapter bandwidth in/out by ×1.5 per scroll step, anchoring on the frequency under the mouse cursor. - Plain wheel scroll is unchanged — it continues to tune the VFO. ## Implementation Single change in `SpectrumWidget::wheelEvent`: after the debounce/step calculation, an early branch catches `Qt::ControlModifier` and runs the cursor-anchored zoom, then returns before the existing VFO tune path. The zoom math mirrors the existing NativeGesture pinch-to-zoom exactly: ```cpp const double mouseXFrac = ev->position().x() / width() - 0.5; const double anchorMhz = m_centerMhz + mouseXFrac * m_bandwidthMhz; const double newCenter = std::max(anchorMhz - mouseXFrac * newBw, newBw / 2.0); ``` The `std::max(..., newBw / 2.0)` clamp prevents the left edge from going below 0 Hz, consistent with PR aethersdr#2867. ## UX consistency note The existing `+`/`−` zoom buttons use a different anchor policy: zoom-in re-centers on the active VFO frequency (added in aethersdr#1932 to prevent repeated clicks pushing the slice off-screen), zoom-out keeps the pan center fixed. That behavior makes sense for a discrete click where the cursor position carries no intent. Ctrl+wheel is a continuous, cursor-positioned interaction — the user is looking at a specific part of the spectrum when they scroll. Cursor-anchored zoom is therefore the correct policy here, matching the NativeGesture pinch-to-zoom and every major spectrum/map application that implements scroll-to-zoom. The button inconsistency is deliberate and acceptable. Ctrl is already the modifier used for dBm-scale drag (PR aethersdr#2717) and waterfall time-scale drag (PR aethersdr#2783), so Ctrl+wheel is consistent with the existing convention of Ctrl = “modify the view, not the VFO.” ## Testing - Scroll wheel without Ctrl: VFO tunes as before, no regression. - Ctrl+scroll up: bandwidth narrows, cursor frequency stays fixed. - Ctrl+scroll down: bandwidth widens, cursor frequency stays fixed. - Ctrl+scroll at band edge: 0 Hz clamp prevents left edge going negative. - Ctrl+scroll at BW limits: clamped to `m_minBwMhz`/`m_maxBwMhz`, no crash. - Pinch-to-zoom: unaffected. Fixes aethersdr#1518 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Validation