Skip to content

Conversation

@deepin-ci-robot
Copy link
Contributor

Synchronize source files from linuxdeepin/dtkgui.

Source-pull-request: linuxdeepin/dtkgui#345

Synchronize source files from linuxdeepin/dtkgui.

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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deepin-ci-robot

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

@deepin-ci-robot
Copy link
Contributor Author

deepin pr auto review

Git Diff 代码审查报告

总体评价

这次提交主要涉及DTK GUI库的CMake配置更新和Qt版本兼容性改进,特别是针对Qt6.10版本的适配。整体改动合理,但有一些细节可以优化。

具体问题与改进建议

1. CMake配置改进

问题1:dtkgui.cmake中,libxdg的检查逻辑从无条件检查改为仅在Qt5下检查。

改进建议:

# 建议添加明确的版本检查注释
# Only use libxdg under Qt5, as Qt6 has different icon loading mechanisms
if(NOT DTK_DISABLE_LIBXDG AND ${QT_VERSION_MAJOR} STREQUAL "5")

问题2: 在多个地方添加了Qt6.10版本的私有模块检查,但没有统一处理。

改进建议:
建议创建一个CMake函数来统一处理Qt版本检查和私有模块查找:

function(find_qt_private_modules)
    if(${QT_VERSION_MAJOR} STREQUAL "6" AND ${Qt6Core_VERSION} VERSION_GREATER_EQUAL "6.10.0")
        set(QT_NO_PRIVATE_MODULE_WARNING ON PARENT_SCOPE)
        find_package(Qt6 REQUIRED COMPONENTS CorePrivate GuiPrivate WaylandClientPrivate)
    endif()
endfunction()

2. 代码逻辑改进

问题1:dguiapplicationhelper.cpp中,主题变更事件处理使用了条件编译。

改进建议:
建议将版本检查定义为宏,提高可读性:

#define THEME_CHANGE_EVENT_ARGS \
    (QT_VERSION >= QT_VERSION_CHECK(6, 10, 0) ? QWindowSystemInterfacePrivate::ThemeChangeEvent() \
                                              : QWindowSystemInterfacePrivate::ThemeChangeEvent(nullptr))

void DGuiApplicationHelperPrivate::notifyAppThemeChangedByEvent()
{
    THEME_CHANGE_EVENT_ARGS event;
    QGuiApplicationPrivate::processThemeChanged(&event);
}

问题2: 文档注释中的空格不一致。

改进建议:
统一文档注释中的空格格式,保持风格一致:

/*!
  \enum DGuiApplicationHelper::ColorType
  DGuiApplicationHelper::ColorType 定义了主题类型.

  \var DGuiApplicationHelper::ColorType DGuiApplicationHelper::UnknownType
  未知主题(浅色主题或深色主题).

  \var DGuiApplicationHelper::ColorType DGuiApplicationHelper::LightType
  浅色主题.

  \var DGuiApplicationHelper::ColorType DGuiApplicationHelper::DarkType
  深色主题.
*/

3. 性能与安全考虑

问题1: 在加载翻译文件时,对每个locale都进行了检查,可能存在性能问题。

改进建议:

bool DGuiApplicationHelper::loadTranslator(const QString &fileName, const QList<QLocale> &locales)
{
    bool found = false;
    QList<QString> localeNames;
    
    for (const QLocale &locale : locales) {
        QString path = QString(":/translations/%1_%2.qm").arg(fileName).arg(locale.name());
        if (QFile::exists(path)) {
            found = true;
            continue;
        }
        if (locale.language() != QLocale::English) {
            localeNames << locale.name();
        }
    }
    
    if (!found && !localeNames.isEmpty()) {
        qWarning() << fileName << "can not find qm files for locales" << localeNames;
        return false;
    }
    return true;
}

4. 代码格式与风格

问题1: 一些地方的缩进和空格不一致。

改进建议:

  • 统一使用4个空格缩进
  • 运算符前后添加空格
  • 函数参数之间添加空格

问题2: 长行没有适当换行。

改进建议:

# 不好的写法
target_link_libraries(${BIN_NAME} PRIVATE Qt${QT_VERSION_MAJOR}XdgIconLoader)

# 改进后的写法
target_link_libraries(${BIN_NAME} 
    PRIVATE 
    Qt${QT_VERSION_MAJOR}XdgIconLoader
)

总结

  1. 建议统一Qt版本检查和私有模块查找的处理方式
  2. 改进代码注释的格式和一致性
  3. 优化翻译文件加载的性能
  4. 统一代码格式和风格
  5. 添加必要的错误处理和日志记录

这些改进将提高代码的可维护性、可读性和性能,同时保持向后兼容性。

@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"
        }
    ]
}

@ComixHe ComixHe merged commit b4c81e8 into master Oct 13, 2025
11 of 15 checks passed
@ComixHe ComixHe deleted the sync-pr-345-nosync branch October 13, 2025 05:03
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