refactor: Part of Remove Qt dependencies from utils#1573
Conversation
Move Qt/DBus related utilities from utils to common. Note: This commit changes the namespace `linglong::utils::dbus` to `linglong::common::dbus` and `linglong::utils::serialize` (partially) to `linglong::common::serialize`. Signed-off-by: reddevillg <reddevillg@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: reddevillg 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 |
Summary of ChangesHello @reddevillg, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request systematically refactors core utility components by relocating Qt and D-Bus specific functionalities from the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
deepin pr auto review代码审查报告总体概述这次变更主要是将 语法和逻辑分析1. 命名空间变更变更将以下内容从
这个变更在语法上是正确的,逻辑上也是合理的,因为这些功能确实更偏向于通用(common)功能而非特定工具(utils)功能。 2. 头文件引用变更变更更新了所有引用这些头文件的地方,从 代码质量评估优点
可能的改进点
代码性能分析这次变更主要是代码重构,不涉及性能相关的修改。代码的性能不会因为命名空间的变更而受到影响。 代码安全评估
具体改进建议
总结这次代码重构变更整体上是积极的,它提高了代码组织的合理性,没有引入明显的性能或安全问题。主要的改进点在于错误处理和日志记录方面,可以考虑进一步优化这些方面以提高代码的健壮性和可维护性。 |
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring that moves Qt and D-Bus related utilities from the utils library to a new common library, effectively reducing dependencies in utils. The changes are consistent with the stated goal and improve the project's architecture.
I've identified a few areas where error messages in the new D-Bus registration functions could be more descriptive to aid in debugging. These are minor points in an otherwise solid refactoring effort.
| return LINGLONG_ERR(conn.lastError().name() + ": " + conn.lastError().message()); | ||
| } | ||
|
|
||
| return LINGLONG_ERR("unknown"); |
There was a problem hiding this comment.
The error message "unknown" is not very descriptive. It would be better to provide a more specific message indicating why unregisterDBusService might have failed, for example, if the service was not registered by this connection or the name is invalid.
| return LINGLONG_ERR("unknown"); | |
| return LINGLONG_ERR(QString("Failed to unregister D-Bus service %1. It might not have been registered by this connection or the name is invalid.").arg(serviceName)); |
| const auto *mo = parent->metaObject(); | ||
| if (mo == nullptr) { | ||
| qFatal("couldn't find metaObject of parent."); | ||
| LogE("couldn't find metaObject of parent."); |
Move Qt/DBus related utilities from utils to common.
Note: This commit changes the namespace
linglong::utils::dbustolinglong::common::dbusandlinglong::utils::serialize(partially) tolinglong::common::serialize.