Improve TNF notch interaction and display behavior#1547
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refines TNF (Tracking Notch Filter) interactions and UI rendering in the spectrum view, aiming to make TNF overlays behave more like spectrum-only markers while improving hover/context-menu detail formatting and enabling continuous drag-based retune/resize without “snap back”.
Changes:
- Update TNF status parsing and apply optimistic local model updates during freq/width/depth changes.
- Rework SpectrumWidget TNF rendering to be spectrum-only with depth-aware hatching, plus improved hit-testing and hover popup.
- Enhance TNF context menu to show formatted metadata and checkmarked width/depth presets, and adjust cursor/tune-guide behavior while hovering/dragging.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/models/TnfModel.cpp | Parse width defensively (Hz vs fractional MHz) and apply optimistic local updates on set commands. |
| src/gui/SpectrumWidget.h | Extend TNF-related helpers/state (hover/drag tracking, popup label, colors, hit testing). |
| src/gui/SpectrumWidget.cpp | Implement TNF hover popup, spectrum-only TNF drawing, improved hit testing/drag behavior, and overlay pipeline tweaks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!foundCursor && m_showSpots) { | ||
| bool spotHover = false; | ||
| for (const auto& hr : m_spotClickRects) { | ||
| if (hr.rect.contains(pos)) { | ||
| setCursor(Qt::PointingHandCursor); | ||
| foundCursor = true; | ||
| // Show spot tooltip | ||
| if (hr.markerIndex >= 0 && hr.markerIndex < m_spotMarkers.size()) { | ||
| const auto& sm = m_spotMarkers[hr.markerIndex]; | ||
| QString tip = QString("<b>%1</b> %2 MHz") | ||
| .arg(sm.callsign) | ||
| .arg(sm.freqMhz, 0, 'f', 4); | ||
| if (!sm.source.isEmpty()) | ||
| tip += QString("<br>Source: %1").arg(sm.source); | ||
| if (!sm.spotterCallsign.isEmpty()) | ||
| tip += QString("<br>Spotter: %1").arg(sm.spotterCallsign); | ||
| if (!sm.comment.isEmpty()) | ||
| tip += QString("<br>%1").arg(sm.comment); | ||
| if (sm.timestampMs > 0) | ||
| tip += QString("<br>Spotted: %1 UTC").arg( | ||
| QDateTime::fromMSecsSinceEpoch(sm.timestampMs, QTimeZone::utc()).toString("yyyy-MM-dd HH:mm:ss")); | ||
| QToolTip::showText(ev->globalPosition().toPoint(), tip, this); | ||
| } | ||
| spotHover = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!spotHover) | ||
| QToolTip::hideText(); | ||
| if (!foundCursor) { | ||
| for (const auto& cluster : m_spotClusters) { |
| const QRect wfRect(0, wfY, width(), height() - wfY); | ||
| const QRect timeScaleRect = waterfallTimeScaleRect(wfRect); | ||
| const QPoint pos = ev->position().toPoint(); | ||
| if (timeScaleRect.contains(pos)) { | ||
| setCursor(Qt::SizeVerCursor); | ||
| } else { | ||
| setCursor(Qt::CrossCursor); | ||
| } | ||
| } else if (y < specH) { |
| void TnfModel::setTnfWidth(int id, int widthHz) | ||
| { | ||
| // Radio expects width in MHz (e.g. 150 Hz → 0.000150) | ||
| emit commandReady(QString("tnf set %1 width=%2").arg(id).arg(widthHz / 1.0e6, 0, 'f', 6)); | ||
| const int clampedWidthHz = std::max(10, widthHz); | ||
| emit commandReady(QString("tnf set %1 width=%2").arg(id).arg(clampedWidthHz / 1.0e6, 0, 'f', 6)); | ||
|
|
||
| auto it = m_tnfs.find(id); | ||
| if (it != m_tnfs.end() && it->widthHz != clampedWidthHz) { | ||
| it->widthHz = clampedWidthHz; | ||
| emit tnfChanged(id); | ||
| } | ||
| } | ||
|
|
||
| void TnfModel::setTnfDepth(int id, int depthDb) | ||
| { | ||
| emit commandReady(QString("tnf set %1 depth=%2").arg(id).arg(depthDb)); | ||
| const int clampedDepthDb = std::clamp(depthDb, 1, 3); | ||
| emit commandReady(QString("tnf set %1 depth=%2").arg(id).arg(clampedDepthDb)); | ||
|
|
| void SpectrumWidget::updateTrackedCursorState(const QPoint& localPos, bool insideWidget) | ||
| { | ||
| const QPoint oldCursorPos = m_cursorPos; | ||
| const int oldHoveredTnfId = m_hoveredTnfId; | ||
| const bool oldTuneGuideVisible = m_tuneGuideVisible; | ||
|
|
||
| if (!insideWidget) { | ||
| m_cursorPos = {-1, -1}; | ||
| m_hoveredTnfId = -1; | ||
| m_tuneGuideVisible = false; | ||
| m_tuneGuideTimer->stop(); | ||
| QToolTip::hideText(); | ||
| } else { | ||
| const int chromeH = FREQ_SCALE_H + DIVIDER_H; | ||
| const int contentH = height() - chromeH; | ||
| const int specH = static_cast<int>(contentH * m_spectrumFrac); | ||
| const bool inSpectrum = localPos.y() >= 0 | ||
| && localPos.y() < specH | ||
| && localPos.x() >= 0 | ||
| && localPos.x() < width(); | ||
| const int preferredTnfId = (m_draggingTnfId >= 0) ? m_draggingTnfId : m_hoveredTnfId; | ||
|
|
||
| m_cursorPos = localPos; | ||
| m_hoveredTnfId = inSpectrum ? tnfAtPixel(localPos.x(), preferredTnfId) : -1; | ||
|
|
||
| if (m_showTuneGuides && m_hoveredTnfId < 0) { | ||
| m_tuneGuideVisible = true; | ||
| m_tuneGuideTimer->start(); | ||
| } else { | ||
| m_tuneGuideVisible = false; | ||
| m_tuneGuideTimer->stop(); | ||
| } | ||
|
|
||
| if (m_hoveredTnfId >= 0) { | ||
| QToolTip::hideText(); | ||
| } | ||
| } | ||
|
|
||
| if (m_cursorPos != oldCursorPos | ||
| || m_hoveredTnfId != oldHoveredTnfId | ||
| || m_tuneGuideVisible != oldTuneGuideVisible) { | ||
| markOverlayDirty(); | ||
| } |
| auto* infoAction = new QWidgetAction(&menu); | ||
| auto* infoWidget = new QWidget(&menu); | ||
| auto* infoLayout = new QVBoxLayout(infoWidget); | ||
| infoLayout->setContentsMargins(10, 6, 10, 6); | ||
| infoLayout->setSpacing(2); | ||
|
|
||
| auto* freqLabel = new QLabel(QString("%1 MHz").arg(formatFlagFrequency(tnf->freqMhz)), infoWidget); | ||
| auto* widthLabel = new QLabel(QString("Width: %1 Hz").arg(tnf->widthHz), infoWidget); | ||
| auto* separator = new QFrame(infoWidget); | ||
| separator->setFrameShape(QFrame::HLine); | ||
| separator->setFrameShadow(QFrame::Plain); | ||
| separator->setStyleSheet("color: rgba(90, 110, 130, 180);"); | ||
|
|
||
| infoWidget->setStyleSheet( | ||
| "background: rgba(40, 48, 58, 190);" | ||
| "color: rgba(200, 216, 232, 170);"); | ||
| infoLayout->addWidget(freqLabel); | ||
| infoLayout->addWidget(widthLabel); | ||
| infoLayout->addSpacing(4); | ||
| infoLayout->addWidget(separator); | ||
| infoAction->setDefaultWidget(infoWidget); | ||
| menu.addAction(infoAction); | ||
| } |
CI Failure AnalysisHey @rfoust — thanks for the contribution! I took a look at the CI failure on commit What failedThe Your code is fineThis is not caused by your code. The actual Here's the breakdown of all check runs on your commit:
Copilot review notes worth consideringWhile the build passed, Copilot flagged a few things that are worth reviewing before merge — these won't break the build but could cause runtime issues:
None of these would cause the CI failure, but addressing item #1 (the TL;DRNo action needed from you for the CI failure — that's on us. The build passed, and your changes compile. If you'd like to address the Copilot suggestions (especially the missing include), a follow-up push would be great, but it's not blocking. Thanks for your time on this! 🎙️ 73 |
TNF UX rework contributed by Robbie Foust (rfoust). Applied from PR #1547 with the two non-TNF scopes stripped out and two Copilot review items addressed: - Stripped: overlay texture format change (RGBA8 → BGRA8 with fallback, QImage Format_RGBA8888_Premultiplied → Format_ARGB32_Premultiplied). Unrelated to TNF, affects every overlay rendering, no stated reason — revisit separately if a driver-specific need surfaces. - Kept: cursor-tracking refactor (`updateTrackedCursorState`). Intertwined with TNF hover detection (`m_hoveredTnfId`), extracting it would have required rewriting the hover logic. - Fixed: added `#include <algorithm>` to TnfModel.cpp for the newly-used `std::max` / `std::clamp` — relying on transitive includes is fragile. - Fixed: TNF context-menu header `QWidgetAction` is now `WA_TransparentForMouseEvents` + `setEnabled(false)` so clicking the freq/width info block no longer closes the menu. TNF changes preserved intact: - Markers stop at the bottom of the spectrum pane (no waterfall extend). - Hatched depth visualisation (12/8/5 px spacing for depth 1/2/3). - Floating `RF Tracking Notch` popup on hover showing freq + width. - Width/depth submenu checkmarks show current value. - Bidirectional TNF drag: horizontal tunes, vertical resizes via 2^(-dy/48) octaves, clamped 10-12000 Hz. - Preferred-TNF hit-testing for overlapping notches. - Tune guides suppressed while a TNF is hovered or dragged. - TnfModel: optimistic local updates in setters, width parser tolerates both Hz ("width=100") and fractional-MHz forms for firmware variance. Rebased onto current main (#1703 cursor-snap and #1701 filter-passband changes merged cleanly into rfoust's cursor-tracking refactor). Unaddressed follow-ups (intentional deferrals): - #1547 removes `QToolTip::hideText()` when the cursor leaves a spot label; stale tooltip could linger. Needs a small re-add. - `updateTrackedCursorState` is only called for spectrum-area hover; the waterfall path still leaves `m_cursorPos` / `m_hoveredTnfId` stale in the software paintEvent render path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude here on behalf of Jeremy — landed on main as `021e182` with you preserved as git author. Skipped the PR merge to apply it direct, and in the process we trimmed scope to keep the review tractable. Full breakdown is in the commit message, summary here: Kept intact (all TNF-specific changes) — markers no longer extend into waterfall, hatched depth rendering, floating hover popup, width/depth submenu checkmarks, bidirectional drag, preferred-hit-test for overlapping notches, tune-guide suppression while hovering, TnfModel optimistic updates + tolerant width parser. Stripped — the overlay texture format change (`RGBA8 → BGRA8`, `Format_RGBA8888_Premultiplied → Format_ARGB32_Premultiplied`). It touched every overlay the widget draws, not just TNF, and there was no stated reason behind it. Happy to look at it separately if you've got a specific driver/platform it fixes — would want to understand the target before accepting a cross-cutting format change. Kept but intertwined — the cursor-tracking refactor (`updateTrackedCursorState`). TNF hover detection depends on `m_hoveredTnfId` which that function owns, so pulling it out would've meant rewriting the hover logic. Kept as-is. Fixed while landing (two of the five Copilot items from earlier review):
Still open (deferred — small and boring, happy to open as separate issues if you want to take them):
Rebased onto current main so the cursor-snap (#1369, landed via #1703) and waterfall filter-passband removal (#1270, landed via #1701) merged cleanly into your cursor refactor — both fixes preserved. Nice work on the drag UX — the octave-based resize is really pleasant. 73, Jeremy KK7GWY & Claude (AI dev partner) |
Summary
Details
Verification
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=RelWithDebInfocmake --build build -j4