refactor(gui): split keyboard-shortcut system into MainWindow_Shortcuts.cpp (#3351 Phase 1c)#3524
Conversation
…ts.cpp (#3351 Phase 1c) Fourth PR of the #3351 monolith-decomposition series. Pure code motion — no behavior change, no MainWindow.h changes. Moves the complete keyboard-shortcut system out of MainWindow.cpp into the new MainWindow_Shortcuts.cpp (1,087 lines): - The shortcut-state definitions (s_keyboardShortcutsEnabled, s_sliderShortcutLeaseActive) and accessors (textInputCaptured, shortcutInputCaptured, shortcutGuard, leaseHolderBusy) — declared in MainWindowShortcutState.h since Phase 1b, owned here as of this phase, completing the handoff that header promised. - registerShortcutActions() (559 lines — the full action table). - The slider shortcut lease (#745): beginSliderShortcutLease, renewSliderShortcutLease, releaseSliderShortcutLease, plus MainWindow::eventFilter() (319 lines) which drives the lease and the app-quit interception. - handleCwMomentaryShortcut() and the trivial keyPress/keyRelease overrides. Supporting moves, called out for review: - kSliderShortcutLeaseMs moved with its only user. - panCountForLayoutId promoted from MainWindow.cpp's anonymous namespace to MainWindowHelpers.{h,cpp} — registerShortcutActions (moved) and two MainWindow.cpp call sites now share it across TUs. Pure map lookup; fits the helpers file's no-state rule. - isCwMomentaryActionId NOT moved — its remaining caller is the MIDI dispatch path still in MainWindow.cpp; it stays file-static there. Breadcrumb comments at all six excision sites. MainWindow.cpp: 16,261 → 15,239 lines (series: 19,474 → 15,239, −21.7%). Verified: full build clean, all 33 ctest suites pass (incl. the new radio_discovery_test from main), a11y lint clean. Refs #3351. Principle XI.
There was a problem hiding this comment.
Reviewed with a mechanical pure-motion check: I diffed every line removed from MainWindow.cpp against the new MainWindow_Shortcuts.cpp (whitespace-normalized). Every removed line is accounted for verbatim — the only lines not landing in the Shortcuts TU are the panCountForLayoutId body, which appears identically in MainWindowHelpers.cpp. Conversely, the only content in the new file that wasn't moved is the header comment, includes, and namespace scaffolding. The "pure code motion" claim holds.
Cross-TU wiring also checks out:
- The shortcut state (
s_keyboardShortcutsEnabled,s_sliderShortcutLeaseActive) and its accessors now have exactly one definition site (the Shortcuts TU), matching theexterndeclarations inMainWindowShortcutState.hfrom Phase 1b. The only outside user isMainWindow_Menus.cpp(View-menu toggle +shortcutGuard), which is the design that header documents. isCwMomentaryActionIdcorrectly stays behind — its remaining caller is the MIDI dispatch path atMainWindow.cpp:3749.panCountForLayoutIdin the helpers file is a stateless map lookup, consistent with that file's no-state rule, and both remainingMainWindow.cppcall sites plus the movedeventFilteruse the shared helper.- All six moved definitions (
eventFilter,registerShortcutActions, the three lease functions,handleCwMomentaryShortcut, key event overrides) exist exactly once across the GUI sources. CMake entry added; noMainWindow.hchanges, as advertised.
Two minor notes, neither blocking:
- Missing
<algorithm>/<iterator>includes. The new TU usesstd::clamp,std::min/std::max, andstd::size(inregisterShortcutActions) but includes neither header — it currently compiles everywhere (all CI green, including MSVC) via transitive includes only. BothMainWindow.cppand the Phase 1b siblingMainWindow_Menus.cppinclude<algorithm>explicitly; suggest adding<algorithm>and<iterator>here to match and to avoid a fragile dependency on Qt header internals across upgrades. - Optional:
MainWindow.cppstill includesMainWindowShortcutState.hbut no longer references any of its symbols after this move — the include can be dropped (fine to leave if a later phase will need it again).
Nice work — the breadcrumbs at the excision sites, the verbatim motion, and the explicit not-pure-motion list made this straightforward to verify. The QA checklist for the bench is appreciated; the lease and CW momentary paths are exactly the ones worth a hands-on pass.
🤖 aethersdr-agent · cost: $17.2202 · model: claude-fable-5
… include Both items from the aethersdr-agent review on this PR: 1. MainWindow_Shortcuts.cpp gains explicit <algorithm> and <iterator>. registerShortcutActions uses std::clamp/std::min/std::max/std::size, which previously compiled via transitive Qt includes only — fragile across Qt upgrades and inconsistent with MainWindow.cpp and MainWindow_Menus.cpp, which both include <algorithm> explicitly. 2. MainWindow.cpp drops its now-dead MainWindowShortcutState.h include. After the Phase 1c move its only remaining "references" to the shortcut state were the breadcrumb comments. If a later phase needs the state again, that phase re-adds the include with its user. Principle XI.
|
Both nits addressed in `2670835b`:
Thanks for the mechanical pure-motion verification — diffing removed-vs-added lines whitespace-normalized is exactly the right review for this series, and it's the check I want every phase to pass. All 33 ctest suites still green after the fixes. |
jensenpat
left a comment
There was a problem hiding this comment.
Reviewed: verified pure code motion — every line removed from MainWindow.cpp reappears verbatim in MainWindow_Shortcuts.cpp/MainWindowHelpers; only new lines are includes, breadcrumbs, and the panCountForLayoutId declaration. All CI green.
Summary
Fourth PR of the #3351 series. Pure code motion — no behavior change, no MainWindow.h changes.
Moves the complete keyboard-shortcut system into the new
MainWindow_Shortcuts.cpp(1,087 lines):s_keyboardShortcutsEnabled,shortcutGuard, …)registerShortcutActions()— the full action tableeventFilter()handleCwMomentaryShortcut()+ trivialkeyPress/keyReleaseoverridesThis completes the handoff
MainWindowShortcutState.hpromised in Phase 1b — declarations there, definitions now owned by the Shortcuts TU.Not-pure-motion exceptions (the review surface)
kSliderShortcutLeaseMsmoved with its only user.panCountForLayoutIdpromoted from MainWindow.cpp's anon namespace toMainWindowHelpers.{h,cpp}—registerShortcutActions(moved) plus two MainWindow.cpp call sites now share it cross-TU. Pure map lookup, fits the helpers file's no-state rule.isCwMomentaryActionIddeliberately NOT moved — its remaining caller is the MIDI dispatch path still in MainWindow.cpp.Everything else verbatim. Breadcrumbs at all six excision sites.
Numbers
MainWindow.cpp: 16,261 → 15,239 (−1,022)Verification
radio_discovery_testfrom main)QA checklist for the bench
The shortcut system touches every keyboard interaction:
Refs #3351