fix: Synchronize dss-network-plugin 107x interface #330
Conversation
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
Reviewer's Guide by SourceryThis pull request introduces a new D-Bus method Sequence diagram for locale update via D-BussequenceDiagram
participant NP as NetworkPlugin
participant SN as SystemNetworkService
participant DBus as D-Bus
NP->>NP: message(msgData)
NP->>NP: Parse msgData for locale
NP->>NP: installTranslator(locale)
alt SN is registered
NP->>DBus: asyncCall("UpdateLanguage", locale) to SN
DBus-->>SN: UpdateLanguage(locale)
SN-->>DBus: Reply
DBus-->>NP: Reply
else SN is not registered
NP->>NP: Create QDBusServiceWatcher
NP->>NP: Connect to serviceRegistered signal
NP->>NP: Wait for SN to register
activate SN
SN->>DBus: Register service
DBus->>NP: serviceRegistered(networkService)
NP->>DBus: asyncCall("UpdateLanguage", locale) to SN
DBus-->>SN: UpdateLanguage(locale)
SN-->>DBus: Reply
DBus-->>NP: Reply
deactivate SN
end
NP->>NP: Return success JSON
Updated class diagram for NetworkPluginclassDiagram
class NetworkPlugin {
-QWidget* m_contentWidget
-dde::network::NetManager* m_manager
-dde::network::NetStatus* m_netStatus
-QList<QObject*> m_networkItems
-QTimer* m_timer
-bool m_visible
-QString m_userJson
+void requestShowContent()
+void setMessage(bool visible)
+QString message(QString msgData)
<<signals>>
+void notifyNetworkStateChanged(bool)
}
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 @caixr23 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a more specific name than
messagefor the new method inNetworkPluginto better reflect its purpose. - The
UpdateLanguagecalls on the D-Bus interface should check for errors and log them appropriately.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+309
to
+310
| } else { | ||
| qWarning() << networkService << "don't start, wait for it start"; |
There was a problem hiding this comment.
suggestion (bug_risk): Consider handling DBus call errors.
Using reply.waitForFinished() blocks the thread until the call completes, but there is no subsequent error checking. It might be beneficial to inspect the reply for any errors to avoid silent failures in UpdateLanguage.
Suggested implementation:
QDBusPendingCall reply = dbusInter.asyncCall("UpdateLanguage", locale);
reply.waitForFinished();
QDBusPendingReply<void> replyCheck = reply;
if (replyCheck.isError()) {
qWarning() << "Error updating language:" << replyCheck.error().message();
} else {
qDebug() << "UpdateLanguage call succeeded.";
}
QDBusPendingCall reply = dbusInter.asyncCall("UpdateLanguage", locale);
reply.waitForFinished();
QDBusPendingReply<void> replyCheck = reply;
if (replyCheck.isError()) {
qWarning() << "Error updating language on service registration:" << replyCheck.error().message();
} else {
qDebug() << "UpdateLanguage call succeeded on service registration.";
}
If further error handling is needed (for example, retrying the call or handling specific DBus error codes), additional adjustments may be required.
| QJsonObject jsonObject = json.object(); | ||
| if (!jsonObject.contains("data")) { | ||
| qWarning() << "msgData don't containt data" << msgData; | ||
| QJsonDocument jsonResult; |
There was a problem hiding this comment.
nitpick (typo): Fix typo in error message for missing data.
The error message uses "msgData don't containt data"; consider correcting it to "msgData doesn't contain data" to improve clarity and professionalism.
Suggested implementation:
qWarning() << "msgData doesn't contain data" << msgData;
resultObject.insert("data", QString("msgData doesn't contain data %1").arg(msgData));
|
|
||
| QString NetworkPlugin::message(const QString &msgData) | ||
| { | ||
| qDebug() << "message" << msgData; |
There was a problem hiding this comment.
issue (complexity): Consider extracting the DBus update logic into a separate helper function to improve code clarity and reduce nesting in the message function, focusing it on JSON handling and translation installation..
Consider extracting the DBus update logic into a separate helper to reduce nesting and improve clarity. For instance, create a function like updateNetworkLanguage(QString locale) that handles both the synchronous and asynchronous cases. This makes the main message function more focused on JSON handling and translation installation.
Example:
QString NetworkPlugin::updateNetworkLanguage(const QString &locale) {
if (QDBusConnection::systemBus().interface()->isServiceRegistered(networkService)) {
qDebug() << "update SystemNetwork Language" << locale;
QDBusInterface dbusInter(networkService, networkPath, networkInterface, QDBusConnection::systemBus());
QDBusPendingCall reply = dbusInter.asyncCall("UpdateLanguage", locale);
reply.waitForFinished();
} else {
qWarning() << networkService << "don't start, wait for it start";
QDBusServiceWatcher *serviceWatcher = new QDBusServiceWatcher(this);
serviceWatcher->setConnection(QDBusConnection::systemBus());
serviceWatcher->addWatchedService(networkService);
connect(serviceWatcher, &QDBusServiceWatcher::serviceRegistered, this, [locale](const QString &service) {
if (service == networkService) {
QDBusInterface dbusInter(networkService, networkPath, networkInterface, QDBusConnection::systemBus());
QDBusPendingCall reply = dbusInter.asyncCall("UpdateLanguage", locale);
reply.waitForFinished();
}
});
}
return "success";
}Then simplify the message function:
QString NetworkPlugin::message(const QString &msgData) {
qDebug() << "message" << msgData;
QJsonDocument json = QJsonDocument::fromJson(msgData.toLatin1());
QJsonObject jsonObject = json.object();
if (!jsonObject.contains("data")) {
qWarning() << "msgData doesn't contain data:" << msgData;
QJsonObject resultObject{{"data", QString("msgData doesn't contain data %1").arg(msgData)}};
return QJsonDocument(resultObject).toJson();
}
QJsonObject dataObject = jsonObject.value("data").toObject();
QString locale = dataObject.value("locale").toString();
qDebug() << "read locale" << locale;
m_network->installTranslator(locale);
QString status = updateNetworkLanguage(locale);
QJsonObject resultObject{{"data", status}};
return QJsonDocument(resultObject).toJson();
}By moving the DBus logic into a helper, the control flow in message becomes easier to follow and maintain.
robertkill
previously approved these changes
Apr 17, 2025
Synchronize dss-network-plugin 107x interface pms: TASK-361719
deepin pr auto review关键摘要:
是否建议立即修改: |
mhduiy
approved these changes
Apr 17, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23, mhduiy 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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Synchronize dss-network-plugin 107x interface
pms: TASK-361719
Summary by Sourcery
Enhance network plugin localization by adding support for dynamic language updates across network services
New Features:
Bug Fixes:
Enhancements: