Skip to content

Conversation

@ComixHe
Copy link
Contributor

@ComixHe ComixHe commented Oct 13, 2025

Also adjusting the logic for finding QtXDGIconLoader. This library only be used in Qt 5.

Also adjusting the logic for finding QtXDGIconLoader.
This library only be used in Qt 5.

Signed-off-by: ComixHe <heyuming@deepin.org>
@ComixHe ComixHe requested a review from 18202781743 October 13, 2025 02:37
deepin-ci-robot added a commit to linuxdeepin/dtk6gui that referenced this pull request Oct 13, 2025
Synchronize source files from linuxdeepin/dtkgui.

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

deepin pr auto review

代码审查报告

总体评价

这段代码主要涉及DTK GUI库的CMake配置和核心功能实现,主要改进了Qt版本兼容性处理,特别是Qt6的支持。代码整体结构清晰,但存在一些可以优化的地方。

具体改进建议

1. CMake配置改进 (dtkgui.cmake)

优点:

  • 正确处理了Qt5和Qt6的版本差异
  • 添加了条件编译来控制libxdg的使用

改进建议:

# 建议将版本检查提取为变量,提高可维护性
set(USE_LIBXDG OFF)
if(NOT DTK_DISABLE_LIBXDG AND ${QT_VERSION_MAJOR} STREQUAL "5")
    find_package(Qt5XdgIconLoader REQUIRED)
    if(NOT Qt5XdgIconLoader_FOUND)
        message(WARNING "XdgIconLoader Not Found, DISABLE LIBXDG !")
        set(USE_LIBXDG OFF)
    else()
        set(USE_LIBXDG ON)
    endif()
endif()

# 在后续代码中使用USE_LIBXDG变量而不是DTK_DISABLE_LIBXDG

2. 源码依赖管理 (src/CMakeLists.txt)

优点:

  • 正确处理了Qt6的依赖关系
  • 添加了Qt6.10.0的私有模块支持

改进建议:

# 建议将版本检查逻辑提取为函数
function(qt_private_modules_check version)
    if(version VERSION_GREATER_EQUAL "6.10.0")
        set(QT_NO_PRIVATE_MODULE_WARNING ON PARENT_SCOPE)
        find_package(Qt${version} REQUIRED COMPONENTS 
            CorePrivate 
            GuiPrivate 
            WaylandClientPrivate
        )
    endif()
endfunction()

qt_private_modules_check(${QT_VERSION_MAJOR})

3. 主题变更处理 (dguiapplicationhelper.cpp)

优点:

  • 正确处理了Qt6.10.0的主题变更API变化
  • 添加了适当的版本检查

改进建议:

// 建议将主题变更事件创建封装为函数
void createThemeChangeEvent(QWindowSystemInterfacePrivate::ThemeChangeEvent &event)
{
#if QT_VERSION >= QT_VERSION_CHECK(6, 10, 0)
    // Qt6.10.0+ 不需要参数
#else
    event = QWindowSystemInterfacePrivate::ThemeChangeEvent(nullptr);
#endif
}

// 使用时:
void DGuiApplicationHelperPrivate::notifyAppThemeChangedByEvent()
{
    QWindowSystemInterfacePrivate::ThemeChangeEvent event;
    createThemeChangeEvent(event);
    QGuiApplicationPrivate::processThemeChanged(&event);
}

4. 测试配置 (tests/CMakeLists.txt)

优点:

  • 正确配置了测试所需的Qt组件
  • 处理了可选依赖

改进建议:

# 建议将Qt依赖配置提取为变量
set(QT_TEST_COMPONENTS Core Gui Widgets DBus Network Test)
if(${QT_VERSION_MAJOR} STREQUAL "6")
    list(APPEND QT_TEST_COMPONENTS WaylandClient)
    qt_private_modules_check(6)
endif()

find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS ${QT_TEST_COMPONENTS})

5. 代码风格和格式化

问题:

  • 部分代码缩进不一致
  • 空行使用不规范

改进建议:

  1. 统一使用4空格缩进
  2. 在逻辑块之间添加适当的空行
  3. 保持括号风格一致

6. 安全性考虑

建议:

  1. 在find_package调用后添加版本检查:
find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS ...)
if(NOT Qt${QT_VERSION_MAJOR}_FOUND VERSION_EQUAL ${QT_VERSION})
    message(FATAL_ERROR "Qt version mismatch")
endif()
  1. 对外部依赖进行更严格的版本检查:
pkg_check_modules(librsvg REQUIRED IMPORTED_TARGET librsvg-2.0)
if(NOT librsvg_VERSION VERSION_GREATER_EQUAL "2.40.0")
    message(FATAL_ERROR "librsvg version must be at least 2.40.0")
endif()

7. 性能优化建议

  1. 在dguiapplicationhelper.cpp中:
// 缓存localeNames检查结果
static QSet<QString> missingLocales;
if(!missingLocales.contains(fileName)) {
    if(!localeNames.isEmpty()) {
        qWarning() << fileName << "can not find qm files for locales" << localeNames;
        missingLocales.insert(fileName);
    }
}
  1. 考虑在频繁调用的函数中使用const引用:
void DGuiApplicationHelper::someFunction(const QString &fileName) // 使用const引用
{
    // ...
}

总结

代码整体质量良好,主要改进方向包括:

  1. 提取重复代码为可复用函数或变量
  2. 增强版本兼容性检查
  3. 改进代码组织和可维护性
  4. 加强错误处理和安全性检查
  5. 优化性能敏感部分的实现

这些改进将使代码更加健壮、可维护,并提高其在新版本Qt环境下的兼容性。

@github-actions
Copy link
Contributor

  • 敏感词检查失败, 检测到1个文件存在敏感词
详情
{
    "src/kernel/dguiapplicationhelper.cpp": [
        {
            "line": "    QString socket_key = \"_d_dtk_single_instance_\";",
            "line_number": 1484,
            "rule": "S106",
            "reason": "Var naming | 2ad926d35b"
        }
    ]
}

@deepin-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

@ComixHe ComixHe merged commit 80db4dd into linuxdeepin:master Oct 13, 2025
22 of 23 checks passed
ComixHe pushed a commit to linuxdeepin/dtk6gui that referenced this pull request Oct 13, 2025
Synchronize source files from linuxdeepin/dtkgui.

Source-pull-request: linuxdeepin/dtkgui#345
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.

3 participants