-
Notifications
You must be signed in to change notification settings - Fork 48
chore: replace qWarning with categorized logging #470
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideReplaces direct qWarning/qInfo calls with categorized logging macros (qCWarning/qCInfo) using the DNC and DSM logging categories, adds required network logging/header dependencies, removes an obsolete default logger category configuration, and marks NetUtils.js as a QML library script for better reuse. Flow diagram for QML NetUtils.js as a shared library scriptgraph TD
NetUtils[NetUtils.js with .pragma library]
QMLCompA[QML component A]
QMLCompB[QML component B]
QMLCompC[QML component C]
QMLCompA -->|import and call functions| NetUtils
QMLCompB -->|import and call functions| NetUtils
QMLCompC -->|import and call functions| NetUtils
NetUtils -->|shared script state and functions| NetUtils
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 2 issues, and left some high level feedback:
- In NetworkController, removing
Dtk::Core::loggerInstance()->setDefaultCategory(DNC().categoryName());changes the global default logging category; if other components rely on that default, consider either keeping it or explicitly setting categories at those call sites to avoid unexpected logging behavior. - In BubbleManager::CloseNotification the
qDebugcalls were removed rather than replaced with categorized logging; if these messages are still useful for diagnosing notification issues, consider reintroducing them asqCDebug(DNC())so they can be filtered via the category.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In NetworkController, removing `Dtk::Core::loggerInstance()->setDefaultCategory(DNC().categoryName());` changes the global default logging category; if other components rely on that default, consider either keeping it or explicitly setting categories at those call sites to avoid unexpected logging behavior.
- In BubbleManager::CloseNotification the `qDebug` calls were removed rather than replaced with categorized logging; if these messages are still useful for diagnosing notification issues, consider reintroducing them as `qCDebug(DNC())` so they can be filtered via the category.
## Individual Comments
### Comment 1
<location> `dss-network-plugin/networkmodule.cpp:292-296` </location>
<code_context>
QJsonObject jsonObject = json.object();
if (!jsonObject.contains("data")) {
- qWarning() << "msgData don't containt data" << msgData;
+ qCWarning(DNC()) << "msgData don't containt data" << msgData;
QJsonDocument jsonResult;
QJsonObject resultObject;
</code_context>
<issue_to_address>
**nitpick (typo):** Fix typos and wording in user-visible log / response strings.
These user-facing strings have small English errors: `"containt"` → `"contain"`, and `"msgData don't containt data"` could be rephrased for clarity (e.g. `"msgData does not contain data"`). Since they appear in logs/JSON responses, please correct the wording.
```suggestion
if (!jsonObject.contains("data")) {
qCWarning(DNC()) << "msgData does not contain 'data' field" << msgData;
QJsonDocument jsonResult;
QJsonObject resultObject;
resultObject.insert("data", QString("msgData does not contain 'data' field: %1").arg(msgData));
QDBusPendingCall reply = dbusInter.asyncCall("UpdateLanguage", locale);
reply.waitForFinished();
} else {
qCWarning(DNC()) << networkService << "has not started yet; waiting for it to start";
```
</issue_to_address>
### Comment 2
<location> `src/dbus/proxychains.cpp:116` </location>
<code_context>
}
- qWarning() << "On property changed,property not handle: " << propName;
+ qCWarning(DNC()) << "On property changed,property not handle: " << propName;
return;
}
</code_context>
<issue_to_address>
**suggestion (typo):** Improve log message wording and punctuation.
This log string is a bit unclear. Consider wording like `"Property change not handled:"` or `"Unhandled property change:"`. Also, the comma is a full-width Chinese `,`; unless that’s intentional, use a standard ASCII comma for consistency with other logs.
```suggestion
qCWarning(DNC()) << "Unhandled property change:" << propName;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| qWarning() << "On property changed,property not handle: " << propName; | ||
| qCWarning(DNC()) << "On property changed,property not handle: " << propName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (typo): Improve log message wording and punctuation.
This log string is a bit unclear. Consider wording like "Property change not handled:" or "Unhandled property change:". Also, the comma is a full-width Chinese ,; unless that’s intentional, use a standard ASCII comma for consistency with other logs.
| qCWarning(DNC()) << "On property changed,property not handle: " << propName; | |
| qCWarning(DNC()) << "Unhandled property change:" << propName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request replaces direct qWarning() and qInfo() calls with categorized logging macros (qCWarning(), qCInfo()) to improve logging consistency and enable better log filtering. The PR also removes obsolete logging configuration, cleans up redundant debug statements, and adds a .pragma library directive to a QML utility script.
Changes:
- Replaced all
qWarning()andqInfo()calls with categorized equivalents usingDNC()orDSMlogging categories - Removed obsolete
DLoginclude andsetDefaultCategory()call from NetworkController - Removed redundant
qDebug()statements from BubbleManager - Added
.pragma librarydirective to NetUtils.js for proper QML module handling
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/networkcontroller.cpp | Removed DLog dependency and setDefaultCategory call; converted qInfo/qWarning to qCInfo/qCWarning with DNC category |
| src/dbus/proxychains.cpp | Added networkconst.h include; converted qWarning to qCWarning with DNC category |
| network-service-plugin/src/utils/constants.cpp | Converted qWarning calls to qCWarning with DSM category in dbusDebug function |
| network-service-plugin/src/system/networkinitialization.cpp | Converted qWarning to qCWarning with DSM category |
| dss-network-plugin/notification/bubblemanager.cpp | Removed redundant qDebug logging statements |
| dss-network-plugin/networkmodule.cpp | Converted qWarning to qCWarning with DNC category |
| dcc-network/qml/NetUtils.js | Added .pragma library directive for proper QML module handling |
| dcc-network/operation/dccnetwork.cpp | Added networkconst.h include; converted qWarning to qCWarning with DNC category; cleaned up trailing whitespace |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace all direct qWarning and qInfo calls with categorized logging macros (qCWarning, qCInfo) using appropriate logging categories (DNC, DSM). Add necessary includes for NetworkConst header where needed. Add .pragma library directive to NetUtils.js for better QML module handling. This change improves logging consistency and allows for better log filtering and management. Categorized logging provides more control over log output levels and destinations. The .pragma library directive ensures NetUtils.js functions are properly shared across QML components. Influence: 1. Verify that logging still works correctly in all modules 2. Test network operations to ensure no functional regressions 3. Check that QML components using NetUtils.js function properly 4. Verify log filtering capabilities with different log levels chore: 将 qWarning 替换为分类日志记录 将所有直接的 qWarning 和 qInfo 调用替换为使用适当日志分类(DNC、DSM)的 分类日志宏(qCWarning、qCInfo)。在需要的地方添加 NetworkConst 头文件包 含。为 NetUtils.js 添加 .pragma library 指令以改进 QML 模块处理。 此更改提高了日志记录的一致性,并允许更好的日志过滤和管理。分类日志记录提 供了对日志输出级别和目标的更多控制。.pragma library 指令确保 NetUtils.js 函数在 QML 组件之间正确共享。 Influence: 1. 验证所有模块中的日志记录是否仍然正常工作 2. 测试网络操作以确保没有功能回归 3. 检查使用 NetUtils.js 的 QML 组件是否正常运行 4. 使用不同日志级别验证日志过滤功能
deepin pr auto review这段代码主要涉及网络模块的日志系统改造和一些代码格式调整。我将从语法逻辑、代码质量、性能和安全四个方面进行审查: 1. 语法逻辑优点:
改进建议:
2. 代码质量优点:
改进建议:
3. 代码性能优点:
改进建议:
4. 代码安全优点:
改进建议:
总结这次代码修改主要集中在对日志系统的规范化改造,整体方向正确。建议重点关注:
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, caixr23 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 |
Replace all direct qWarning and qInfo calls with categorized logging macros (qCWarning, qCInfo) using appropriate logging categories (DNC, DSM). Add necessary includes for NetworkConst header where needed. Add .pragma library directive to NetUtils.js for better QML module handling.
This change improves logging consistency and allows for better log filtering and management. Categorized logging provides more control over log output levels and destinations. The .pragma library directive ensures NetUtils.js functions are properly shared across QML components.
Influence:
chore: 将 qWarning 替换为分类日志记录
将所有直接的 qWarning 和 qInfo 调用替换为使用适当日志分类(DNC、DSM)的
分类日志宏(qCWarning、qCInfo)。在需要的地方添加 NetworkConst 头文件包 含。为 NetUtils.js 添加 .pragma library 指令以改进 QML 模块处理。
此更改提高了日志记录的一致性,并允许更好的日志过滤和管理。分类日志记录提
供了对日志输出级别和目标的更多控制。.pragma library 指令确保 NetUtils.js 函数在 QML 组件之间正确共享。
Influence:
Summary by Sourcery
Adopt categorized logging and minor QML utility adjustments across network components.
Enhancements: