BUG: fix(diagnostic): 修复诊断工具误报与“再次诊断”无响应#109
Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom Sep 16, 2025
Merged
BUG: fix(diagnostic): 修复诊断工具误报与“再次诊断”无响应#109deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
Conversation
Reviewer's GuideRefactors the diagnostic tool’s sequencing to await aria2 options before running checks, swaps external network probes for Qt HEAD requests, relaxes DHT detection by removing file checks, and leverages proper model reset to enable stable “redo diagnostics” behavior. Sequence diagram for improved diagnostic tool workflowsequenceDiagram
participant User as actor User
participant DiagnosticTool
participant Aria2RPCInterface
participant DiagnosticModel
User->>DiagnosticTool: Click "再次诊断" button
DiagnosticTool->>DiagnosticModel: clearData() (beginResetModel/endResetModel)
DiagnosticTool->>Aria2RPCInterface: getGlobalOption()
Aria2RPCInterface-->>DiagnosticTool: ariaOption(tracker, enable-dht)
DiagnosticTool->>DiagnosticTool: Set m_OptionReady = true
DiagnosticTool->>DiagnosticModel: Run diagnostic checks (appendData)
DiagnosticTool->>User: Enable "再次诊断" button
Updated class diagram for DiagnosticTool and DiagnosticModelclassDiagram
class DiagnosticTool {
- bool m_IsHasTracks
- bool m_IsHasDHT
- bool m_OptionReady
+ void startDiagnostic()
+ void onAriaOption(bool, bool)
}
class DiagnosticModel {
- QList<bool> m_DiagnosticStatusList
+ void appendData(bool)
+ void clearData()
+ bool setData(const QModelIndex &, const QVariant &, int)
}
DiagnosticTool --> DiagnosticModel : uses
Class diagram for Func network connectivity check refactorclassDiagram
class Func {
+ bool isNetConnect()
+ bool isLanConnect()
}
Func : isNetConnect() now uses QNetworkAccessManager
Func : isNetConnect() falls back to isLanConnect() if HTTP probe fails
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Use logical operators (&&, ||) for combining boolean checks instead of bitwise (&, |) for clarity and readability.
- Rather than polling m_OptionReady with a QTimer and a heap‐allocated counter, consider invoking runChecks directly from the onAriaOption callback to simplify flow and avoid manual memory management.
- Avoid dynamically allocating the attempts counter; use a local variable captured by reference or a QSharedPointer to keep the code more idiomatic and safer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Use logical operators (&&, ||) for combining boolean checks instead of bitwise (&, |) for clarity and readability.
- Rather than polling m_OptionReady with a QTimer and a heap‐allocated counter, consider invoking runChecks directly from the onAriaOption callback to simplify flow and avoid manual memory management.
- Avoid dynamically allocating the attempts counter; use a local variable captured by reference or a QSharedPointer to keep the code more idiomatic and safer.
## Individual Comments
### Comment 1
<location> `src/src/ui/settings/diagnostictool.cpp:99-104` </location>
<code_context>
m_Button->setFixedWidth(202);
connect(m_Button, &QPushButton::clicked, this, [=]() {
m_Model->clearData();
+ m_OptionReady = false;
QTimer::singleShot(500, this, [=]() {
</code_context>
<issue_to_address>
**suggestion:** Calling clearData twice in startDiagnostic may be redundant.
Since clearData is invoked in both the button handler and startDiagnostic, this may lead to redundant operations. Review if both calls are necessary and remove one if possible.
```suggestion
connect(m_Button, &QPushButton::clicked, this, [=]() {
m_OptionReady = false;
QTimer::singleShot(500, this, [=]() {
startDiagnostic();
});
```
</issue_to_address>
### Comment 2
<location> `src/src/ui/settings/diagnostictool.cpp:208-217` </location>
<code_context>
- m_Button->setEnabled(true);
- });
-#endif
+ if (m_OptionReady) {
+ QTimer::singleShot(0, this, runChecks);
+ } else {
+ // Wait up to ~2.5s for options; poll in small intervals
+ int *attempts = new int(0);
+ QTimer *poller = new QTimer(this);
+ poller->setInterval(100);
+ connect(poller, &QTimer::timeout, this, [=]() {
+ if (m_OptionReady || *attempts >= 25) {
+ poller->stop();
+ poller->deleteLater();
+ delete attempts;
+ runChecks();
+ return;
+ }
+ (*attempts)++;
+ });
+ poller->start();
+ }
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Polling for m_OptionReady with a QTimer may leak memory if DiagnosticTool is destroyed early.
Since both the int pointer and QTimer are dynamically allocated, ensure they are properly cleaned up if DiagnosticTool is destroyed before polling finishes. Using QPointer or handling cleanup in the destructor can help prevent memory leaks.
</issue_to_address>
### Comment 3
<location> `src/src/ui/func.cpp:26-35` </location>
<code_context>
+ QNetworkAccessManager manager;
</code_context>
<issue_to_address>
**issue (performance):** Using QNetworkAccessManager synchronously in isNetConnect may block the UI thread.
Refactor to use asynchronous network checks or move this logic to a separate thread to prevent blocking the main thread.
</issue_to_address>
### Comment 4
<location> `src/src/ui/settings/diagnostictool.cpp:180` </location>
<code_context>
//m_Model->setData(isHasTracks);
}
void DiagnosticTool::startDiagnostic()
{
qDebug() << "Starting diagnostic process";
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the polling and lambda logic into a dedicated Qt slot to simplify control flow and resource management.
Here’s one way to collapse the manual‐polling, raw pointers and nested lambdas into a single Qt slot. The idea is:
1. Move your `runChecks` lambda into its own `performChecks()` slot.
2. In `startDiagnostic()`, just kick off the RPC call and clear your UI.
3. In `onAriaOption()`, set your flags *and* trigger `performChecks()` (via a single‐shot or direct call).
4. Remove the `new int`, `QTimer* poller`, and delete logic entirely.
— DiagnosticTool.h —
```cpp
class DiagnosticTool : public DDialog {
Q_OBJECT
private slots:
void startDiagnostic();
void onAriaOption(bool isHasTracks, bool isHasDHT);
void performChecks();
// …
};
```
— DiagnosticTool.cpp —
```cpp
void DiagnosticTool::startDiagnostic()
{
qDebug() << "Starting diagnostic process";
m_OptionReady = false;
m_Button->setEnabled(false);
m_Model->clearData();
Aria2RPCInterface::instance()->getGlobalOption();
// if options already arrived
if (m_OptionReady) {
QTimer::singleShot(0, this, SLOT(performChecks()));
}
}
void DiagnosticTool::onAriaOption(bool isHasTracks, bool isHasDHT)
{
qDebug() << "Received aria2 options:" << isHasTracks << isHasDHT;
m_IsHasTracks = isHasTracks;
m_IsHasDHT = isHasDHT;
m_OptionReady = true;
// now that options are ready, run checks
QTimer::singleShot(0, this, SLOT(performChecks()));
}
void DiagnosticTool::performChecks()
{
// Row 0
m_Model->appendData(Func::isIPV6Connect());
// Row 1
m_Model->appendData(m_IsHasDHT & Func::isNetConnect());
// Row 2
m_Model->appendData(Func::isHTTPConnect());
// Row 3 & 4
bool btOk = (m_IsHasTracks | m_IsHasDHT) & Func::isNetConnect();
m_Model->appendData(btOk);
m_Model->appendData(btOk);
// Row 5
m_Model->appendData(Func::isNetConnect());
m_Button->setEnabled(true);
}
```
Benefits:
• No raw `new int` or manual `delete`
• No polling loop – Qt’s signal/slot delivers exactly when options arrive
• All timing remains effectively “immediate” but you can still sprinkle in `QTimer::singleShot(...)` delays if you really need them (e.g. to animate row‐by‐row appending) without the bookkeeping.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
208
to
217
| if (m_OptionReady) { | ||
| QTimer::singleShot(0, this, runChecks); | ||
| } else { | ||
| // Wait up to ~2.5s for options; poll in small intervals | ||
| int *attempts = new int(0); | ||
| QTimer *poller = new QTimer(this); | ||
| poller->setInterval(100); | ||
| connect(poller, &QTimer::timeout, this, [=]() { | ||
| if (m_OptionReady || *attempts >= 25) { | ||
| poller->stop(); |
There was a problem hiding this comment.
issue (bug_risk): Polling for m_OptionReady with a QTimer may leak memory if DiagnosticTool is destroyed early.
Since both the int pointer and QTimer are dynamically allocated, ensure they are properly cleaned up if DiagnosticTool is destroyed before polling finishes. Using QPointer or handling cleanup in the destructor can help prevent memory leaks.
src/src/ui/func.cpp
Outdated
Comment on lines
26
to
35
| QNetworkAccessManager manager; | ||
| auto checkUrl = [&](const QUrl &url, int timeoutMs) -> bool { | ||
| QNetworkRequest req(url); | ||
| QNetworkReply *reply = manager.head(req); | ||
| QEventLoop loop; | ||
| QTimer timer; | ||
| timer.setSingleShot(true); | ||
| QObject::connect(&timer, &QTimer::timeout, &loop, &QEventLoop::quit); | ||
| QObject::connect(reply, &QNetworkReply::finished, &loop, &QEventLoop::quit); | ||
| timer.start(timeoutMs); |
There was a problem hiding this comment.
issue (performance): Using QNetworkAccessManager synchronously in isNetConnect may block the UI thread.
Refactor to use asynchronous network checks or move this logic to a separate thread to prevent blocking the main thread.
| //m_Model->setData(isHasTracks); | ||
| } | ||
|
|
||
| void DiagnosticTool::startDiagnostic() |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the polling and lambda logic into a dedicated Qt slot to simplify control flow and resource management.
Here’s one way to collapse the manual‐polling, raw pointers and nested lambdas into a single Qt slot. The idea is:
- Move your
runCheckslambda into its ownperformChecks()slot. - In
startDiagnostic(), just kick off the RPC call and clear your UI. - In
onAriaOption(), set your flags and triggerperformChecks()(via a single‐shot or direct call). - Remove the
new int,QTimer* poller, and delete logic entirely.
— DiagnosticTool.h —
class DiagnosticTool : public DDialog {
Q_OBJECT
private slots:
void startDiagnostic();
void onAriaOption(bool isHasTracks, bool isHasDHT);
void performChecks();
// …
};— DiagnosticTool.cpp —
void DiagnosticTool::startDiagnostic()
{
qDebug() << "Starting diagnostic process";
m_OptionReady = false;
m_Button->setEnabled(false);
m_Model->clearData();
Aria2RPCInterface::instance()->getGlobalOption();
// if options already arrived
if (m_OptionReady) {
QTimer::singleShot(0, this, SLOT(performChecks()));
}
}
void DiagnosticTool::onAriaOption(bool isHasTracks, bool isHasDHT)
{
qDebug() << "Received aria2 options:" << isHasTracks << isHasDHT;
m_IsHasTracks = isHasTracks;
m_IsHasDHT = isHasDHT;
m_OptionReady = true;
// now that options are ready, run checks
QTimer::singleShot(0, this, SLOT(performChecks()));
}
void DiagnosticTool::performChecks()
{
// Row 0
m_Model->appendData(Func::isIPV6Connect());
// Row 1
m_Model->appendData(m_IsHasDHT & Func::isNetConnect());
// Row 2
m_Model->appendData(Func::isHTTPConnect());
// Row 3 & 4
bool btOk = (m_IsHasTracks | m_IsHasDHT) & Func::isNetConnect();
m_Model->appendData(btOk);
m_Model->appendData(btOk);
// Row 5
m_Model->appendData(Func::isNetConnect());
m_Button->setEnabled(true);
}Benefits:
• No raw new int or manual delete
• No polling loop – Qt’s signal/slot delivers exactly when options arrive
• All timing remains effectively “immediate” but you can still sprinkle in QTimer::singleShot(...) delays if you really need them (e.g. to animate row‐by‐row appending) without the bookkeeping.
…resh of diagnostic results Issues - Download succeeds but diagnostic tool still shows “DTS download failed” - Occasional incorrect DHT status detection - Diagnostic results require mouse click to refresh window Cause - aria2's `enable-dht` response may be string/boolean, causing unstable detection - Diagnostic model fixed at 6 rows but updated via “insert row” method, failing to trigger view refresh Minimal Changes & Fixes - src/src/ui/mainFrame/mainframe.cpp - DHT check now uses obj.value(“enable-dht”).toVariant().toBool() to handle different types and prevent misjudgments - Pre-warm Aria2RPCInterface::getGlobalOption() before opening diagnostic window to eliminate timing-related errors (No changes to diagnostic tool logic) - src/src/ui/settings/diagnostictool.cpp - appendData: No longer inserts rows; instead appends to internal list and emits dataChanged for all three columns of that row to ensure immediate view refresh - clearData: Uses beginResetModel/endResetModel to ensure complete reset during clearing Verification - After successful download, open “Diagnostic Tool”; DHT/HTTP/BT/magnet links display “Normal” when network is available - Click “Diagnose Again”; list results update automatically without window interaction BUG: https://pms.uniontech.com/bug-view-331441.html
Contributor
deepin pr auto review我来对这段代码进行详细审查，并提出改进建议： 1. 代码逻辑审查mainframe.cpp 的改进：
diagnostictool.cpp 的改进：
2. 语法逻辑问题
3. 代码质量改进
4. 代码性能优化
5. 代码安全考虑
改进建议代码示例：// diagnostictool.cpp
const int MAX_DIAGNOSTIC_ROWS = 6;
void DiagnosticModel::appendData(bool b)
{
qDebug() << "Appending diagnostic data, status:" << b;
const int row = m_DiagnosticStatusList.size();
if (row >= MAX_DIAGNOSTIC_ROWS) {
qWarning() << "Diagnostic status list is full, ignoring new data";
return;
}
m_DiagnosticStatusList.append(b);
emit dataChanged(index(row, 0), index(row, columnCount() - 1));
}
// mainframe.cpp
void MainFrame::showDiagnosticTool()
{
DiagnosticTool control(this);
connect(this, &MainFrame::ariaOption, &control, &DiagnosticTool::onAriaOption);
// 获取全局配置并处理可能的错误
QJsonObject globalOptions;
if (!Aria2RPCInterface::instance()->getGlobalOption(&globalOptions)) {
qWarning() << "Failed to get global options for diagnostic";
// 可以考虑使用默认值或显示错误信息
}
control.exec();
}
void MainFrame::onRpcSuccess(QString method, QJsonObject json)
{
if (json.isEmpty()) {
qWarning() << "Empty JSON received for method:" << method;
return;
}
if (method == ARIA2C_METHOD_GET_GLOBAL_OPTION) {
QJsonObject obj = json.value("result").toObject();
if (obj.isEmpty()) {
qWarning() << "Empty result object in getGlobalOption";
return;
}
QString tracker = obj.value("bt-tracker").toString();
// 安全地获取 enable-dht 值
bool isHasDHT = false;
if (obj.contains("enable-dht")) {
isHasDHT = obj.value("enable-dht").toVariant().toBool();
}
emit ariaOption(!tracker.isEmpty(), isHasDHT);
}
}总结：这些修改提高了代码的健壮性和安全性，同时保持了良好的性能。建议在实际应用中添加适当的错误处理和日志记录，以便于问题排查。 |
lzwind
approved these changes
Sep 16, 2025
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JWWTSL, lzwind The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Author
|
/forcemerge |
Contributor
|
This pr force merged! (status: unstable) |
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
3 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.
问题
原因
最小改动与解决
验证