Skip to content

[shortcuts] Add configurable pan zoom and memories shortcuts#1248

Merged
ten9876 merged 2 commits intoten9876:mainfrom
jensenpat:aether/shortcuts
Apr 13, 2026
Merged

[shortcuts] Add configurable pan zoom and memories shortcuts#1248
ten9876 merged 2 commits intoten9876:mainfrom
jensenpat:aether/shortcuts

Conversation

@jensenpat
Copy link
Copy Markdown
Contributor

@jensenpat jensenpat commented Apr 13, 2026

Summary

  • add configurable shortcut actions for panadapter zoom in, panadapter zoom out, and opening the Memories dialog
  • route shortcut-driven pan zoom through the live SpectrumWidget center/bandwidth state so each keypress matches the on-screen zoom buttons
  • ship the new shortcuts with default bindings: = for zoom in, - for zoom out, and / for the Memories dialog

Why

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=RelWithDebInfo
  • cmake --build build -j10
  • manual app test of pan zoom in/out and opening the Memories dialog via configured shortcuts

👨🏼‍💻 Generated with OpenAI Codex (GPT-5.3 Pro) and tested by @jensenpat

@jensenpat jensenpat changed the title Add configurable pan zoom and memories shortcuts [shortcuts] Add configurable pan zoom and memories shortcuts Apr 13, 2026
@jensenpat jensenpat marked this pull request as ready for review April 13, 2026 00:52
@jensenpat jensenpat requested a review from ten9876 as a code owner April 13, 2026 00:52
@aethersdr-agent
Copy link
Copy Markdown
Contributor

CI Build Failure Analysis

Hey @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 failed

Two CI jobs failed on commit 3f4b433, both at the Build (compilation) step:

Job Step Result
build Build (cmake --build) ❌ failure
analyze (cpp) (CodeQL) Build ❌ failure
check-paths ✅ success
check-windows skipped (no build-system changes)

The Configure (CMake) step passed in both jobs, so the issue is a compiler error during compilation, not a CMake/dependency problem. The main branch itself builds successfully on all platforms (latest commit 3c2ed4c is green), so the failure is specific to changes in this PR.

Code review

I reviewed the diff and checked all referenced symbols against the current main branch:

  • SpectrumWidget::centerMhz() / bandwidthMhz()new getters you're adding, backed by existing members m_centerMhz and m_bandwidthMhz
  • SpectrumWidget::setFrequencyRange(double, double) — exists ✓
  • RadioModel::minPanBandwidthMhz() / maxPanBandwidthMhz() — exists (added in PR Add panadapter zoom +/- buttons and trackpad pinch-to-zoom #1108) ✓
  • MainWindow::spectrumForSlice(SliceModel*) — exists ✓
  • MainWindow::showMemoryDialog() — exists ✓
  • ShortcutManager::registerAction(...) signature matches ✓
  • Qt::Key_Equal, Qt::Key_Minus, Qt::Key_Slash — valid Qt key constants ✓

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 cause

The diff context lines reference registerShortcutActions() at line 6180 on your branch, but on current main the same code is at line ~6228 — about 48 lines have been added upstream since your branch was forked. While git can handle this textual offset during merge, the CI builds the merge commit of your branch + main. If your branch is based on an older main that's missing recent declarations or includes, the merged result could produce a compiler error even though the diff applies cleanly.

Suggested fix: Rebase your branch onto current main and verify a clean build locally:

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-check

You can also click the Details link next to the failed build check on the PR's Checks tab to see the full compiler error output — that will tell you the exact file, line, and error message. If you get stuck, feel free to paste it here and I'll help debug.

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! 🎯

Copy link
Copy Markdown
Contributor

@aethersdr-agent aethersdr-agent bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ten9876
Copy link
Copy Markdown
Owner

ten9876 commented Apr 13, 2026

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 1.5 zoom factor will become a named constexpr constant per project style guidelines.

73, Jeremy KK7GWY & Claude (AI dev partner)

@ten9876 ten9876 enabled auto-merge (squash) April 13, 2026 19:44
@ten9876 ten9876 merged commit 77aea94 into ten9876:main Apr 13, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants