Fix RX slice tab capacity initialization#2243
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect RX slice-tab capacity initialization by ensuring the UI doesn’t create slice selector buttons until the radio’s actual model (and therefore slice capacity) is known, and by refreshing the selector state immediately after creation.
Changes:
- Recompute
m_maxSliceswhen the TCPinforesponse provides the radiomodel. - Delay RX slice-tab button creation until the radio model is known.
- Immediately refresh slice button enabled/checked state after creating the selector to reflect already-open slices.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/models/RadioModel.cpp |
Updates max slice capacity when model is parsed from the info response. |
src/gui/RxApplet.cpp |
Minor guard formatting in setMaxSlices (no behavior change). |
src/gui/MainWindow.cpp |
Defers slice-tab initialization until model is known and refreshes button state after creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| connect(&m_radioModel, &RadioModel::infoChanged, this, [this]() { | ||
| m_appletPanel->setMaxSlices(m_radioModel.maxSlices()); | ||
| if (!m_radioModel.model().isEmpty()) { | ||
| m_appletPanel->setMaxSlices(m_radioModel.maxSlices()); | ||
| m_appletPanel->updateSliceButtons(m_radioModel.slices(), m_activeSliceId); | ||
| } |
There was a problem hiding this comment.
The new infoChanged handler calls updateSliceButtons(...) on every RadioModel::infoChanged emission once model() is non-empty. Since infoChanged is emitted for many unrelated radio status updates (e.g., oscillator, licensing, filter settings), this introduces repeated UI work that isn’t needed after the initial slice-tab creation. Consider disconnecting this specific connection after the first successful setMaxSlices(...)/updateSliceButtons(...), or gating so updateSliceButtons only runs when the slice-tab buttons were just created (or when maxSlices/model actually changes).
|
Hey @rfoust — thanks for the contribution! I took a look at the CI failure on commit CI Failure AnalysisThe failure is not in your code. It's a CI infrastructure issue. The failed job is "Cleanup artifacts" (link), specifically the "Delete artifacts" step. This is a post-build housekeeping job that cleans up temporary CI artifacts — it has nothing to do with your changes. The other jobs look fine:
This kind of transient failure usually resolves on a re-run. You can trigger one from the Actions tab, or a maintainer can re-run the failed job. Separate note: Copilot review feedbackCopilot flagged something worth considering in
This isn't related to the CI failure, but it's worth addressing before merge to avoid unnecessary UI churn. Thanks again for your time on this — appreciate the contribution! 🎙️ |
rfoust
force-pushed
the
codex/rx-controls-h-slice
branch
from
1556c85 to
226496e
Compare
|
Addressed the infoChanged concern in the latest push. The RX slice-tab initialization handler now captures an initialized flag and returns after the first successful initialization, so unrelated later RadioModel::infoChanged emissions no longer call updateSliceButtons repeatedly. It still waits until RadioModel::model is known before creating the selector, then immediately refreshes updateSliceButtons once so slices that arrived earlier, such as slice A during startup, are enabled and checked as soon as the tabs appear. Re-ran:
|
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the fix, @rfoust — this is a clean, well-scoped change. A few notes:
Looks good:
-
The
maxSlicesForModel()call inregisterAsGuiClient's info handler is correct and consistent with the existing patterns inhandleRadioStatus(line 3210) andsetRadioInfo(line 453). Previouslym_maxSliceswasn't being set in this code path, so the tab count could be wrong until a laterradio statusmessage arrived. -
The
[this, initialized = false]() mutableone-shot pattern inMainWindow.cppis a nice use of C++14 init-capture to gate the handler. It firessetMaxSlices+updateSliceButtonsexactly once (on firstinfoChangedwheremodel()is non-empty), which avoids the repeated-work concern that Copilot flagged. BothsetMaxSlicesandupdateSliceButtonsalso have their own early-return guards (m_sliceBtns.isEmpty()), so there's defense in depth. -
Adding
updateSliceButtonsright aftersetMaxSlicesensures slices that were already opened before the model string arrived get their tabs highlighted immediately — good catch. -
The
RxApplet.cppchange is a no-op brace-style fix, fine.
Re: Copilot's comment — the concern about infoChanged firing repeatedly is valid in the abstract, but the initialized flag in this PR already handles it. The handler short-circuits after the first successful run. No further action needed there.
One minor thought (non-blocking): The initialized flag lives in the lambda capture, so if infoChanged is ever disconnected and reconnected (e.g., across a reconnect cycle), the one-shot won't re-arm. This seems fine for the current connection lifecycle, but worth keeping in mind if reconnect logic changes later.
All three files are within scope. No bugs, null-pointer risks, or convention issues spotted. Tests passing per the PR description. Nice contribution!
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Reviewed — small, focused, correct. Three layers (model parser, MainWindow listener, RxApplet brace cleanup) are mutually reinforcing rather than redundant. Approving.
Summary
Testing