Add right-click context menu on off-screen slice indicators#1126
Conversation
Right-clicking an off-screen slice helper icon now shows a context menu with three options: - Close Slice: removes the slice receiver - Move Slice Here: tunes the slice to the panadapter center frequency - Center Slice: recenters the panadapter display around the slice Left-click behavior (activate slice) is unchanged.
There was a problem hiding this comment.
Pull request overview
Adds a right-click context menu to the off-screen slice indicator widgets, enabling quick slice actions (close, move-to-center, center display) while preserving existing left-click activation behavior.
Changes:
- Added a new
SpectrumWidget::sliceTuneRequested(int sliceId, double freqMhz)signal for requesting slice frequency changes. - Implemented a right-click context menu on off-screen slice indicators and restricted the existing indicator click handler to
Qt::LeftButton. - Wired the new signal in
MainWindowto tune the slice viaSliceModel::setFrequency().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/gui/SpectrumWidget.h | Adds a new signal to request tuning a specific slice to a given MHz frequency. |
| src/gui/SpectrumWidget.cpp | Adds right-click context menu behavior for off-screen slice indicators and gates indicator activation to left-click only. |
| src/gui/MainWindow.cpp | Connects the new tune-request signal to the slice model’s frequency setter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (int oi = 0; oi < m_offScreenRects.size(); ++oi) { | ||
| if (!m_offScreenRects[oi].isNull() && | ||
| m_offScreenRects[oi].contains(QPoint(static_cast<int>(ev->position().x()), y))) { | ||
| const auto& so = m_sliceOverlays[oi]; | ||
| if (!so.isActive) emit sliceClicked(so.sliceId); |
There was a problem hiding this comment.
Potential out-of-bounds access: the loop iterates over m_offScreenRects.size(), but then indexes m_sliceOverlays[oi]. If a slice is removed and m_offScreenRects hasn't been resized/rebuilt yet (until next paint), m_offScreenRects can be larger than m_sliceOverlays and a click in a stale rect could crash. Iterate up to min(m_offScreenRects.size(), m_sliceOverlays.size()) or guard oi < m_sliceOverlays.size() before dereferencing.
| // Right-click on off-screen slice indicator → slice context menu | ||
| for (int oi = 0; oi < m_offScreenRects.size(); ++oi) { | ||
| if (!m_offScreenRects[oi].isNull() && | ||
| m_offScreenRects[oi].contains(QPoint(mx, y))) { |
There was a problem hiding this comment.
Same bounds issue here: indexing m_sliceOverlays[oi] while iterating m_offScreenRects can crash if m_offScreenRects is stale after slice removal (until next repaint). Guard the index or keep a rect→sliceId mapping instead of relying on parallel vectors staying in sync across events.
| m_offScreenRects[oi].contains(QPoint(mx, y))) { | |
| m_offScreenRects[oi].contains(QPoint(mx, y))) { | |
| if (oi >= m_sliceOverlays.size()) { | |
| continue; | |
| } |
| connect(sw, &SpectrumWidget::sliceTuneRequested, | ||
| this, [this](int sliceId, double freqMhz) { | ||
| if (auto* s = m_radioModel.slice(sliceId)) | ||
| s->setFrequency(freqMhz); | ||
| }); |
There was a problem hiding this comment.
wirePanadapter() is documented as being callable multiple times for the same SpectrumWidget, and other command-sending signals are disconnected first to avoid duplicate sends. Add a matching disconnect(sw, &SpectrumWidget::sliceTuneRequested, this, nullptr) (or use Qt::UniqueConnection) before this connect to prevent multiple setFrequency() calls after re-wiring.
There was a problem hiding this comment.
Thanks for the contribution, @rfoust — this is a clean, well-scoped addition. The left-click guard fix is correct, the lambda captures are all by-value where they need to be, and the null check in MainWindow is handled properly. A couple of things worth discussing:
m_centerMhz mutated directly inside the "Center Slice" action
[this, freq = so.freqMhz]{
m_centerMhz = freq; // ← direct widget state mutation
markOverlayDirty();
emit centerChangeRequested(m_centerMhz);
}This is an optimistic update — the widget repaints with the new center before the signal is even dispatched. If the centerChangeRequested handler can reject or adjust the value (e.g. band-edge clamping), the widget momentarily shows a center that the model won't confirm, and the subsequent feedback write will then snap it again. The other two actions go signal-only and let the model drive state. It's worth checking whether center changes elsewhere in SpectrumWidget follow the optimistic-update pattern — if they do, this is fine and consistent; if not, consider removing the direct m_centerMhz = freq and letting the existing centerChangeRequested → model → widget callback path handle the repaint.
sliceId & 3 for the menu label letter
const QChar letter = QChar('A' + (so.sliceId & 3));Functionally fine for FLEX's 4-slice limit, but worth verifying this matches the letter derivation used in paintOffScreenIndicators() (or wherever the overlay labels are generated) so the menu always shows the same letter the indicator does.
No concerns about scope, resource management, or the signal wiring in MainWindow.cpp. The PR is solid — those two points are the only things I'd want eyes on before merge.
|
Hey @rfoust — thanks for the contribution! The CI "failure" here is not a problem with your code. Here's what actually happened: What failed and whyThe only failed step is "Delete artifacts" inside the "Cleanup artifacts" post-build job (run 24248554422). This is a housekeeping step that runs after the real work is done — it tries to clean up temporary build artifacts from the Actions cache. It has no bearing on whether your code builds or is correct. All the checks that actually matter passed:
The cleanup job likely failed because the artifact had already expired or been deleted by a concurrent run — this is a known flakiness in the artifact-deletion step of the CI infrastructure, not anything you introduced. What you need to doNothing on your end. Your code is fine. A maintainer can re-run just the "Cleanup artifacts" job if they want a clean green checkmark, or simply ignore it — it doesn't block the merge. Thanks again for taking the time to contribute — the build looks solid! |
Right-clicking an off-screen slice helper icon now shows a context menu with three options:
Left-click behavior (activate slice) is unchanged.
Changes
SpectrumWidget.h: AddedsliceTuneRequested(int sliceId, double freqMhz)signalSpectrumWidget.cpp: Added right-click context menu for off-screen indicators; gated existing left-click handler toQt::LeftButtononlyMainWindow.cpp: WiredsliceTuneRequestedtoSliceModel::setFrequency()