refactor(gui): extract DSP-applet + CAT + DAX-IQ wiring from constructor (#3351 Phase 2d)#3541
Conversation
…tor (#3351 Phase 2d) Tenth PR of the #3351 series — completes the constructor decomposition. Same pattern throughout: each block becomes a named private wireXxx() method called at exactly the original constructor position. Three additive MainWindow.h declarations. - wireDspApplets() → new MainWindow_DspApplets.cpp (~525 lines): PooDoo RX chain status tiles, P/CW applet meters, PHNE, EQ, the client DSP applet family (EQ/Comp/Gate/DeEss/Tube/Reverb/AetherDSP/ PUDU — TX and RX tiles), TX signal-chain applet, PUDU monitor, RX chain edit + bypass. Window-side wiring (applets ↔ AudioEngine); the TU header says so. - wireCatPorts() → MainWindow_Session.cpp (~100 lines): the unified CAT port slots — session-bound, joins the RadioSession seed file. - wireDaxIq() → MainWindow_Session.cpp (~75 lines): DAX IQ wiring for no-audio-bridge platforms — likewise session-bound. Per the Phase 2c lesson, all three cut ranges were checked for stacked earlier-phase wireXxx() calls before extraction (none present) and ifdef balance (all zero). Includes carried explicitly. The constructor is now ~1,000 lines: UI construction order, geometry restore, and small chrome wiring — a legible composition root. The constructor-decomposition phase of #3351 is complete. MainWindow.cpp: 8,766 → 8,075 lines (series: 19,474 → 8,075, −58.5%). Verified: full build clean, all 33 ctest suites pass, a11y lint clean. Refs #3351 #3445. Principle XI.
There was a problem hiding this comment.
Thanks @ten9876 — solid close-out to the series. I verified the code motion mechanically: the three extracted bodies are byte-identical to the deleted constructor text (only difference: three trailing blank lines), execution order at the call sites is preserved, ifdef pairs in both new TUs are balanced, MainWindow.h declarations are additive, and the Session.cpp include additions (CatControlApplet.h, DaxApplet.h, DaxIqApplet.h, TciApplet.h) cover exactly what the moved code needs (CatPort/TciServer/CatDialect resolve via MainWindow.h). All 6 CI checks green. No behavior change.
One finding and two minor notes:
1. The cut range for wireDspApplets() swept in non-DSP code — including the Phase 2a wireMeters() / wireExternalControllers() call sites. The PR description says the ranges were "pre-checked for stacked earlier-phase wireXxx() calls (none)", but MainWindow_DspApplets.cpp:121 and :124 are now the only call sites for wireMeters() and wireExternalControllers() — they execute from inside wireDspApplets(), not the constructor. The same sweep carried along the headphone-mute wiring, the Multi-Flex title-bar indicators, the clientConnected status message, and the saved master-volume/mute restore (lines ~95–125 of the new TU). This is not a bug — it's the same single invocation in the same order — but it breaks the "constructor reads as a table of contents" goal: someone reading the constructor will no longer see meters or controller wiring at all, and the PR body's constructor listing (wireMeters(); wireExternalControllers(); wireDspApplets(); as siblings) doesn't match the actual code.
Suggested fix, in preference order: (a) split into wireDspApplets() proper plus keeping the ~30-line misc block (headphone/Multi-Flex/volume restore + the two wireXxx() calls) in the constructor between two extraction calls, or (b) keep the code where it is but make the MainWindow_DspApplets.cpp header comment honest about containing the Phase 2a call sites and the chrome/volume-restore block, and amend the PR description. Please don't just hoist the swept block out of the middle — the surrounding code is order-sensitive (e.g., the PooDoo SPEAK-tile seed reads m_audio->isMuted() relative to the saved-mute restore), and the verbatim ordering is what makes this PR safe.
2. Missing explicit #include <memory>. MainWindow_DspApplets.cpp uses std::make_shared three times (DspState, heldLevel/heldPeak) but doesn't include <memory>; the original MainWindow.cpp carried it explicitly (line 143 on main). It compiles today via transitive includes (and MainWindow_Wiring.cpp has the same pattern), but given this series' stated include discipline — and that lost transitive includes are exactly how these code-motion PRs have broken Linux CI before — worth adding the one line.
3. Nit: MainWindow_DspApplets.cpp includes CatControlApplet.h, DaxApplet.h, DaxIqApplet.h, and TciApplet.h, but the TU references none of them — looks copied from the master include list. Safe to drop.
None of these block the merge — #1 is a documentation/structure decision for you and the maintainers, #2 is one line. The mechanical verification all passed, and −58.5% across the series is a great result.
🤖 aethersdr-agent · cost: $15.1616 · model: claude-fable-5
…er review
Addresses all three findings from the aethersdr-agent review on this PR.
1. The wireDspApplets() cut range had swept in non-DSP code: the title-
bar chrome wiring (headphone mute, Multi-Flex indicators, client-
connected status), the saved master-volume/mute restore, and — worst —
the Phase 2a wireMeters() / wireExternalControllers() call sites,
which were executing from inside wireDspApplets() instead of the
constructor. (My pre-cut stacked-call check produced a false
negative; the review's line-by-line diff caught it.)
Fixed by splitting along the original seams, preserving verbatim
execution order end-to-end per the review's warning (the PooDoo
SPEAK-tile seed reads m_audio->isMuted() relative to the saved-mute
restore, so hoisting was not an option):
wirePooDooTiles(); // part 1 → DSP TU
[chrome + volume/mute restore — back inline in the constructor]
wireMeters(); // 2a call — back in the constructor
wireExternalControllers(); // 2a call — back in the constructor
wireDspApplets(); // part 4 → DSP TU
The constructor again reads as a complete wiring table of contents.
2. <memory> added explicitly to MainWindow_DspApplets.cpp (three
std::make_shared uses) and to MainWindow_Wiring.cpp (same pattern,
flagged in the same review) — transitive-include discipline.
3. Dropped four unused includes from the DSP TU (CatControlApplet,
DaxApplet, DaxIqApplet, TciApplet — copied from the Session TU's
needs, referenced nowhere in this file).
Verified: full build clean, all 33 ctest suites pass.
Refs #3351. Principle XI.
|
All three findings addressed in `79928992`. 1. The swept block — confirmed and fixed. You were right and my PR description was wrong: the cut range did sweep in the chrome wiring, the volume/mute restore, and the Phase 2a `wireMeters()` / `wireExternalControllers()` call sites. My pre-cut stacked-call check produced a false negative (the awk pattern silently failed — I trusted a verifier I hadn't tested against a known positive; that's on me, and it's now part of the checklist to validate the checker first). Fixed per your preference order, option (a), and honoring the do-not-hoist warning — split along the original seams so execution order is verbatim end-to-end: ```cpp The PooDoo SPEAK-tile seed still reads `m_audio->isMuted()` at exactly the same point relative to the saved-mute restore. Constructor reads as a full wiring table of contents again, and the PR description's listing now matches reality (description edited). 2. `` added to both `MainWindow_DspApplets.cpp` and `MainWindow_Wiring.cpp` (you flagged the same latent pattern there — confirmed, three `std::make_shared` uses each, no direct include). 3. Unused includes — all four dropped (`CatControlApplet` / `DaxApplet` / `DaxIqApplet` / `TciApplet`); they were copied from the Session TU's needs and referenced nowhere in this file. Verified by grep before deletion. Build clean, all 33 ctest suites pass on `79928992`. The line-by-line order-sensitive diff catching what my range-level check missed is the clearest demonstration yet of why this series needs that review style. Thanks. |
jensenpat
left a comment
There was a problem hiding this comment.
Reviewed for move fidelity (the only real risk class in this series):
- Diffed every removed constructor line against the new TUs after whitespace normalization — all 657 removed lines reappear verbatim in
MainWindow_DspApplets.cpp/MainWindow_Session.cpp; the only added-not-removed lines are scaffolding (includes, method signatures, namespace, file-header comments). - Call sites land at the exact original constructor positions (
wirePooDooTiles()before the headphone-mute wiring,wireDspApplets()afterwireExternalControllers(),wireCatPorts()/wireDaxIq()after the AG/ShackSwitch block) — initialization order preserved, including the documented setTxDspChainOrder → side-filter-seed ordering constraint, which moved as a unit intowireDspApplets(). - Ifdef balance:
HAVE_WEBSOCKETS,!Q_OS_MAC && !HAVE_PIPEWIRE, andQ_OS_MAC || HAVE_PIPEWIREblocks all moved intact; Session TU balances to zero. - Includes carried explicitly (CatControlApplet/Dax*/TciApplet into Session, full applet family into the new TU); Linux CI build passing confirms no Qt-floor transitive-include gaps.
CI green on all three platforms, a11y clean. LGTM.
💻 Reviewed with Claude Code (Fable 5.0)
Comment-only: the wireDaxIq() call-site comment in the constructor still said '(no-audio-bridge platforms)', stale after #3522 made the helper unconditional/all-platforms. Updated to reflect once-at-construction wiring independent of the audio bridge. Closes #2530 (the extract-helper request was already resolved by #3541 + #3522). No code/behaviour change. Co-authored-by: Ozy311 <ozy311@users.noreply.github.com>
…tor (aethersdr#3351 Phase 2d) (aethersdr#3541) ## Summary Tenth PR of the aethersdr#3351 series — **completes the constructor decomposition**. Same pattern throughout: block → named private `wireXxx()` called at exactly the original constructor position. Three additive MainWindow.h declarations. | New method | Defined in | Lines | Contents | |---|---|---|---| | `wireDspApplets()` | new `MainWindow_DspApplets.cpp` | ~525 | PooDoo tiles, P/CW meters, PHNE, EQ, client DSP family (EQ/Comp/Gate/DeEss/Tube/Reverb/AetherDSP/PUDU ×TX+RX), TX chain, PUDU monitor, RX chain edit | | `wireCatPorts()` | `MainWindow_Session.cpp` | ~100 | unified CAT port slots — session-bound, joins the RadioSession seed | | `wireDaxIq()` | `MainWindow_Session.cpp` | ~75 | DAX IQ wiring (no-audio-bridge platforms) — session-bound | Per the **Phase 2c lesson**, all three cut ranges were pre-checked for stacked earlier-phase `wireXxx()` calls (none) and ifdef balance (zero). Includes carried explicitly per the Qt 6.4.2-floor discipline. ## Where this leaves the constructor **~1,000 lines**: UI construction order, geometry restore, small chrome wiring — a legible composition root. The wiring blocks now read as a table of contents: ```cpp wireDiscovery(); wireSpotSubsystem(); wireRadioModel(); ... wirePanLifecycle(); ... wirePooDooTiles(); [title-bar chrome + volume/mute restore — inline] wireMeters(); wireExternalControllers(); wireDspApplets(); wireCatPorts(); wireDaxIq(); ``` ## Numbers - `MainWindow.cpp`: **8,766 → 8,075** (−691) - Series: 19,474 → 8,075 (**−58.5%**) ## Verification - Full build clean (Linux x86, all features) - All 33 ctest suites pass - a11y lint: 0 findings ## QA checklist for the bench - [ ] PooDoo chain tiles reflect RX DSP state; clicking tiles toggles stages - [ ] Each client DSP applet (EQ/CMP/GATE/DESS/TUBE/REVERB/PUDU, TX + RX) opens and drives audio - [ ] TX chain applet reorders stages; bypass works - [ ] PUDU monitor audio routes to selected output - [ ] CAT: rigctld client connects on configured port (WSJT-X) - [ ] DAX IQ stream starts on a platform without an audio bridge (Windows box if available) Refs aethersdr#3351 aethersdr#3445 Squashed-from: aethersdr#3541 Co-authored-by: Jeremy [KK7GWY] <105957118+ten9876@users.noreply.github.com> Signed-off-by: Pat Jensen <pat@jensencloud.net>
… (aethersdr#3566) Comment-only: the wireDaxIq() call-site comment in the constructor still said '(no-audio-bridge platforms)', stale after aethersdr#3522 made the helper unconditional/all-platforms. Updated to reflect once-at-construction wiring independent of the audio bridge. Closes aethersdr#2530 (the extract-helper request was already resolved by aethersdr#3541 + aethersdr#3522). No code/behaviour change. Co-authored-by: Ozy311 <ozy311@users.noreply.github.com>
Summary
Tenth PR of the #3351 series — completes the constructor decomposition. Same pattern throughout: block → named private
wireXxx()called at exactly the original constructor position. Three additive MainWindow.h declarations.wireDspApplets()MainWindow_DspApplets.cppwireCatPorts()MainWindow_Session.cppwireDaxIq()MainWindow_Session.cppPer the Phase 2c lesson, all three cut ranges were pre-checked for stacked earlier-phase
wireXxx()calls (none) and ifdef balance (zero). Includes carried explicitly per the Qt 6.4.2-floor discipline.Where this leaves the constructor
~1,000 lines: UI construction order, geometry restore, small chrome wiring — a legible composition root. The wiring blocks now read as a table of contents:
Numbers
MainWindow.cpp: 8,766 → 8,075 (−691)Verification
QA checklist for the bench
Refs #3351 #3445