feat(cpn): add detailed progress to read and write radio#7450
Merged
Conversation
elecpower
added
enhancement ✨
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughThis PR wires progress reporting into storage formats, updates radio and SD-card read/write flows to use progress dialogs/widgets, and refactors related UI paths to the new progress-aware write and import methods. ChangesProgress and Storage Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
companion/src/mdichild.cpp (1)
625-625:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTypo in error message string.
Line 625 has "Models and Setting" but should be "Models and Settings" to match the rest of the codebase.
📝 Proposed fix
- TR("Failed to write Models and Setting to") % " " + TR("Failed to write Models and Settings to") % " "🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@companion/src/mdichild.cpp` at line 625, Typo in the error message inside MdiChild::onItemActivated — change the user-visible string "Models and Setting" to "Models and Settings" so it matches the rest of the codebase; locate the string literal in the MdiChild::onItemActivated method and update it accordingly.
🧹 Nitpick comments (1)
companion/src/progress/progresswidget.cpp (1)
72-72: ⚖️ Poor tradeoffConsider the implications of frequent
processEvents()calls.
QCoreApplication::processEvents()is called after nearly every UI update to reduce latency. While this can improve responsiveness, it introduces re-entrancy risk where events triggered duringprocessEvents()could call back into these same methods or modify state unexpectedly.Ensure that:
- No critical state mutations can be triggered by re-entrant calls
- Consider whether Qt's built-in event coalescing and deferred updates would suffice
- Document this pattern if it's a deliberate design choice for progress feedback
Also applies to: 80-80, 87-87, 93-93, 104-104, 189-189, 252-252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@companion/src/progress/progresswidget.cpp` at line 72, Multiple methods in ProgressWidget call QCoreApplication::processEvents() frequently, which risks re-entrancy and unexpected state mutation; add a lightweight re-entrancy guard (e.g., a private bool like m_inProcessEvents) around every place you call QCoreApplication::processEvents() in ProgressWidget so the call is skipped when already running, or replace immediate processing with a safer alternative (QCoreApplication::processEvents(QEventLoop::AllEvents, timeout) or posting deferred updates via QMetaObject::invokeMethod(..., Qt::QueuedConnection)), ensure any critical state mutations in methods such as the ProgressWidget update/paint/slot functions are protected (no mutations while m_inProcessEvents is true), and add a brief comment documenting why processEvents is used and the guard to prevent re-entrancy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@companion/src/progress/progresswidget.cpp`:
- Line 56: The constructor of ProgressWidget calls
QCoreApplication::processEvents(), which risks re-entrancy during construction;
remove that direct call from ProgressWidget::ProgressWidget and instead defer
processing by scheduling a zero-delay callback (e.g., via QTimer::singleShot(0,
...) or by triggering the work from showEvent/initialize method) so events are
processed only after the object is fully constructed and shown.
- Around line 153-158: The current allowed predicate special-cases QtInfoMsg and
prevents it when m_logLevel is Warning/Critical/Fatal; if you want a standard
severity threshold, replace the expression with a simple comparison like "const
bool allowed = (type >= m_logLevel);" (ensuring QtMsgType ordering is used
correctly) and add a short comment near m_logLevel/allowed explaining the
threshold policy for QtInfoMsg; if you instead want to preserve the
special-case, add an explicit comment documenting why QtInfoMsg is filtered when
m_logLevel is QtWarningMsg/QtCriticalMsg/QtFatalMsg.
In `@companion/src/storage/sdcard.cpp`:
- Around line 106-112: The current write validation only checks for zero-length
writes via writesz and misses partial or error cases; update the logic around
qint64 writesz = file.write(data.data(), data.size()) to treat any writesz < 0
(error) or writesz != qint64(data.size()) (partial write) as failures: close the
file if needed, log/fatal the path with a descriptive message including expected
vs written byte counts, and return false instead of treating partial writes as
success (refer to the writesz variable, file.write call, and data.size()).
---
Outside diff comments:
In `@companion/src/mdichild.cpp`:
- Line 625: Typo in the error message inside MdiChild::onItemActivated — change
the user-visible string "Models and Setting" to "Models and Settings" so it
matches the rest of the codebase; locate the string literal in the
MdiChild::onItemActivated method and update it accordingly.
---
Nitpick comments:
In `@companion/src/progress/progresswidget.cpp`:
- Line 72: Multiple methods in ProgressWidget call
QCoreApplication::processEvents() frequently, which risks re-entrancy and
unexpected state mutation; add a lightweight re-entrancy guard (e.g., a private
bool like m_inProcessEvents) around every place you call
QCoreApplication::processEvents() in ProgressWidget so the call is skipped when
already running, or replace immediate processing with a safer alternative
(QCoreApplication::processEvents(QEventLoop::AllEvents, timeout) or posting
deferred updates via QMetaObject::invokeMethod(..., Qt::QueuedConnection)),
ensure any critical state mutations in methods such as the ProgressWidget
update/paint/slot functions are protected (no mutations while m_inProcessEvents
is true), and add a brief comment documenting why processEvents is used and the
guard to prevent re-entrancy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0910acfc-0a31-4339-bddc-19bdb07bfe86
📒 Files selected for processing (13)
companion/src/flash/radiointerface.cppcompanion/src/mainwindow.cppcompanion/src/mainwindow.hcompanion/src/mdichild.cppcompanion/src/mdichild.hcompanion/src/modeledit/setup.cppcompanion/src/progress/progresswidget.cppcompanion/src/progress/progresswidget.hcompanion/src/storage/etx.cppcompanion/src/storage/labeled.cppcompanion/src/storage/sdcard.cppcompanion/src/storage/storage.cppcompanion/src/storage/storage.h
💤 Files with no reviewable changes (1)
- companion/src/mainwindow.h
pfeerick
force-pushed
the
elecpower/add-detailed-status
branch
from
09f0517 to
0d16491
Compare
Member
|
LGTM, and is a nice QoL improvement... thanks Neil! :) A future tweak might be to have some way to have it auto-close... particularly for read, as it does add an extra click if it is successful (i.e. nothing to see here, move along)... not sure what the best option is though... a auto-close on success option in the settings, or a checkbox on the dialog (as I can see you trying to frantically unclick it if you do want to see the log even though all is good). Also, for what it is worth, I see two lines when deleting the models (PA01, if that makes any difference)... one with full path, one with relative path. |
Collaborator
Author
|
Agree some more tweaks would be nice. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes:
Summary by CodeRabbit
Bug Fixes
Refactor