[shortcuts] Add configurable pan zoom and memories shortcuts#1248
[shortcuts] Add configurable pan zoom and memories shortcuts#1248ten9876 merged 2 commits intoten9876:mainfrom
Conversation
CI Build Failure AnalysisHey @jensenpat — thanks for contributing these keyboard shortcuts! Having pan zoom and the Memories dialog accessible from configurable shortcuts is a great addition. Here's what I found looking into the CI failure. What failedTwo CI jobs failed on commit
The Code reviewI reviewed the diff and checked all referenced symbols against the current
The logic and API usage all look correct. Unfortunately I wasn't able to retrieve the full compiler output from the CI logs to see the exact error message. Most likely causeThe diff context lines reference Suggested fix: Rebase your branch onto current git fetch origin
git rebase origin/main
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=RelWithDebInfo
cmake --build build -j$(nproc)If the rebase introduces conflicts, that will pinpoint exactly what changed upstream. If it builds cleanly after rebase, push and the CI should go green. One thing to double-checkYou can also click the Details link next to the failed Thanks again for the contribution — the shortcut system has been a highly requested feature area and this is a clean approach to wiring it up! 🎯 |
3f4b433 to
685d088
Compare
There was a problem hiding this comment.
Thanks for the contribution, @jensenpat — this is a clean, well-scoped PR. The null checks are thorough, the zoom factor matches the existing UI buttons, and the new SpectrumWidget accessors are minimal. CI is all green.
A couple of things worth discussing:
1. Potential race between the two sendCommand calls (MainWindow.cpp)
The zoom lambda sends two separate commands — one for bandwidth and one for center:
m_radioModel.sendCommand(
QString("display pan set %1 bandwidth=%2").arg(s->panId()).arg(newBw, 0, 'f', 6));
m_radioModel.sendCommand(
QString("display pan set %1 center=%2").arg(s->panId()).arg(currentCenter, 0, 'f', 6));The radio processes these sequentially, and after the bandwidth change it may auto-adjust the center to keep things in-band. By the time the second command arrives, the radio's idea of center may have already shifted, causing a visible "jump then snap-back" on the panadapter. The existing tuneToFrequency() path (line ~6029) only sends center and lets the widget handle bandwidth locally — worth checking if this two-command approach causes any flicker in practice. If it does, sending them as a single combined command (bandwidth=%1 center=%2 in one display pan set) would avoid the intermediate state.
2. Default key bindings: - and = may conflict with existing shortcuts
Key_Minus and Key_Equal are commonly used keys. I didn't spot an existing conflict in the shortcut manager, but it's worth confirming these don't shadow any platform-level or accessibility shortcuts. Since the shortcut system is configurable, this is low risk — users can remap — but documenting the defaults somewhere (e.g. in the shortcuts settings UI tooltip or a wiki page) would be helpful.
3. Minor: lambda capture
The zoomActivePanadapter lambda is captured by copy into the registerAction closures, which is fine since it itself captures this — just noting it's correct as-is.
Overall this looks good to merge. Nice work keeping it focused on just the shortcut plumbing without touching unrelated code.
|
Claude here. Clean implementation — the zoom factor matches the existing on-screen buttons and the shortcut registrations follow the established pattern. Merging. One thing we'll clean up post-merge: the 73, Jeremy KK7GWY & Claude (AI dev partner) |
Summary
SpectrumWidgetcenter/bandwidth state so each keypress matches the on-screen zoom buttons=for zoom in,-for zoom out, and/for the Memories dialogWhy
These controls were available in the UI but not in the configurable keyboard shortcut system. The shortcut zoom path also needed to mirror the widget's live zoom state so it would step cleanly instead of jumping between coarse widths.
Validation
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=RelWithDebInfocmake --build build -j10👨🏼💻 Generated with OpenAI Codex (GPT-5.3 Pro) and tested by @jensenpat