Skip to content

Conversation

@MingcongBai
Copy link
Contributor

Qt 6.9 renamed <private/qdesktopunixservices_p.h> to <private/qgenericunixservices_p.h>, as well as some of the classes along the same format (QGenericUnixServices => QDesktopUnixServices).

Add a condition test with QT_VERSION to fix build with Qt >= 6.9.

deepin-ci-robot added a commit to linuxdeepin/dtk6gui that referenced this pull request Sep 23, 2025
Synchronize source files from linuxdeepin/dtkgui.

Source-pull-request: linuxdeepin/dtkgui#343
@xzl01
Copy link
Contributor

xzl01 commented Sep 24, 2025

LGTM

QPlatformServices *MinimalIntegration::services() const
{
if (!m_services)
#if QT_VERSION >= 0x060900
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

类似 #if QT_VERSION >= QT_VERSION_CHECK(5, 13, 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

目前的写法更为通用,如果用 QT_VERSION_CHECK 可能会导致 moc 不可用

https://stackoverflow.com/a/29887388

Copy link
Member

@BLumia BLumia Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个是 Qt 4 (以及很早期的 Qt 5)时代的问题,目前是 Qt 5/6 了,不需要担心这个。建议修改为使用 QT_VERSION_CHECK

Copy link
Contributor

@18202781743 18202781743 Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我们使用的qt版本应该是大于5.11,这moc会有问题么,另外,dtk里绝大多数用的都是QT_VERSION_CHECK,qt源码里也使用的是QT_VERSION,好像没出现过moc问题,是不是qt后面已经修了,
QT_VERSION_CHECK可读性也更好些,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修改

Qt 6.9 renamed <private/qdesktopunixservices_p.h> to
<private/qgenericunixservices_p.h>, as well as some of the classes along
the same format (QGenericUnixServices => QDesktopUnixServices).

Add a condition test with QT_VERSION to fix build with Qt >= 6.9.

Signed-off-by: Mingcong Bai <jeffbai@aosc.io>
deepin-ci-robot added a commit to linuxdeepin/dtk6gui that referenced this pull request Sep 24, 2025
Synchronize source files from linuxdeepin/dtkgui.

Source-pull-request: linuxdeepin/dtkgui#343
@deepin-ci-robot
Copy link
Contributor

deepin pr auto review

这段代码是一个Qt平台插件测试相关的实现,主要处理不同Qt版本间的兼容性问题。我来分析一下这段代码:

语法逻辑

代码逻辑基本正确,使用了条件编译来处理不同Qt版本间的差异。通过QT_VERSION检查来决定使用哪个服务类,这是一个合理的版本兼容处理方式。

代码质量

  1. 优点:

    • 清晰的版本检查逻辑
    • 使用智能指针管理资源
    • 良好的头文件组织
  2. 可改进之处:

    • 可以考虑将版本检查的逻辑提取为一个辅助函数,提高代码可读性
    • 可以添加注释说明版本差异的原因

代码性能

  1. 性能表现良好:

    • 使用懒加载模式初始化服务对象
    • 条件编译在预处理阶段完成,不会影响运行时性能
  2. 可改进之处:

    • 如果频繁调用services()方法,可以考虑添加缓存机制

代码安全

  1. 安全性良好:

    • 使用智能指针自动管理内存
    • 条件编译确保使用正确的API
  2. 可改进之处:

    • 可以考虑添加错误处理机制,当服务初始化失败时的处理

改进建议

// 建议添加注释说明版本差异的原因
#if QT_VERSION >= QT_VERSION_CHECK(6, 9, 0)
// Qt 6.9.0起,QGenericUnixServices已被QDesktopUnixServices替代
#include <private/qdesktopunixservices_p.h>
#else
#include <private/qgenericunixservices_p.h>
#endif

// 可以考虑将版本检查提取为辅助函数
inline bool isQt69OrLater()
{
    return QT_VERSION >= QT_VERSION_CHECK(6, 9, 0);
}

QPlatformServices *MinimalIntegration::services() const
{
    if (!m_services) {
        if (isQt69OrLater()) {
            m_services.reset(new QDesktopUnixServices);
        } else {
            m_services.reset(new QGenericUnixServices);
        }
    }
    return m_services.get();
}

总结

这段代码整体质量良好,主要处理了Qt版本兼容性问题。建议的改进主要集中在代码可读性和可维护性方面,而不是功能或安全性问题。如果项目有更高的要求,可以考虑添加错误处理和更详细的日志记录。

@deepin-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 18202781743, MingcongBai

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@BLumia BLumia merged commit af1259e into linuxdeepin:master Sep 24, 2025
20 of 23 checks passed
BLumia pushed a commit to linuxdeepin/dtk6gui that referenced this pull request Sep 24, 2025
Synchronize source files from linuxdeepin/dtkgui.

Source-pull-request: linuxdeepin/dtkgui#343
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants