Skip to content

test: migrate unit tests from Qt5 to Qt6 compatibility#201

Merged
pengfeixx merged 1 commit into
linuxdeepin:masterfrom
pengfeixx:feat/ut-qt6-compat
Jun 4, 2026
Merged

test: migrate unit tests from Qt5 to Qt6 compatibility#201
pengfeixx merged 1 commit into
linuxdeepin:masterfrom
pengfeixx:feat/ut-qt6-compat

Conversation

@pengfeixx

Copy link
Copy Markdown
Contributor
  • Add QT_VERSION_MAJOR guards for QMouseEvent constructors (QPoint->QPointF)

  • Replace foreach macro with C++11 range-based for loops

  • Replace QProcess::start(cmd) with startCommand() for Qt6

  • Fix ASAN misaligned write crash in stub.h using memcpy

  • 为QMouseEvent构造函数添加Qt6兼容分支(QPoint→QPointF)

  • 将foreach宏替换为C++11 range-for循环

  • 将QProcess::start()单参数调用替换为startCommand()

  • 修复stub.h中ASAN非对齐写入崩溃(memcpy替代指针解引用)

Log: 迁移单元测试支持Qt6编译运行
Influence: 单元测试可在Qt6环境下编译运行,361/368测试通过,兼容Qt5编译。

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry @pengfeixx, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

- Add QT_VERSION_MAJOR guards for QMouseEvent constructors (QPoint->QPointF)
- Replace foreach macro with C++11 range-based for loops
- Replace QProcess::start(cmd) with startCommand() for Qt6
- Fix ASAN misaligned write crash in stub.h using memcpy

- 为QMouseEvent构造函数添加Qt6兼容分支(QPoint→QPointF)
- 将foreach宏替换为C++11 range-for循环
- 将QProcess::start()单参数调用替换为startCommand()
- 修复stub.h中ASAN非对齐写入崩溃(memcpy替代指针解引用)

Log: 迁移单元测试支持Qt6编译运行
Influence: 单元测试可在Qt6环境下编译运行,361/368测试通过,兼容Qt5编译。
@pengfeixx pengfeixx force-pushed the feat/ut-qt6-compat branch from e8badd0 to 9c9a420 Compare June 4, 2026 09:39
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

你好!我是CodeGeeX。我已仔细审查了你提供的Git Diff。本次提交主要进行了两项改进:一是将Qt的foreach宏替换为C++11的范围for循环并使用const auto &,二是为了适配Qt6修复了API变更(如QProcessQMouseEvent)及严格的类型转换警告。

整体来看,这次修改方向非常正确,有效提升了代码的现代性、跨版本兼容性以及类型安全性。不过,在仔细审查语法逻辑、代码质量、性能和安全性后,我发现了一些需要改进和注意的地方。

以下是详细的审查意见:

1. 语法逻辑 - ⚠️ 存在严重逻辑隐患

文件: deepin-font-manager/interfaces/dfontpreviewer.cpp

-    foreach (auto it, m_previewTexts) {
+    for (const auto &it : m_previewTexts) {
         QString text = Utils::convertToPreviewString(m_fontPath, it);
         m_previewTexts.replaceInStrings(it, text);
     }

问题:
这里将 foreach 替换为 for (const auto &it : m_previewTexts),但在循环体内调用了 m_previewTexts.replaceInStrings(it, text)replaceInStrings原地修改 m_previewTexts 容器的内容。在C++中,对正在迭代的容器进行原地修改是未定义行为,极有可能导致迭代器失效和程序崩溃。

改进建议:
如果 m_previewTextsQStringList,建议不要在循环中修改自身,而是构建一个新的列表;或者利用 QStringList::replaceInStrings 本身就是全局替换的特性,无需手动遍历。

// 方案一:构建新列表(推荐,逻辑最清晰)
QStringList newPreviewTexts;
newPreviewTexts.reserve(m_previewTexts.size());
for (const auto &it : m_previewTexts) {
    newPreviewTexts.append(Utils::convertToPreviewString(m_fontPath, it));
}
m_previewTexts = std::move(newPreviewTexts);

// 方案二:如果 convertToPreviewString 仅是简单的字符串拼接/替换,
// 可以直接使用 Qt 的 replaceInStrings 批量替换,完全去掉 for 循环。

2. 代码性能 - 🚀 可优化的性能瓶颈

文件: deepin-font-manager/views/dfontmgrmainwindow.cpp

-        foreach (auto it, files) {
+        for (const auto &it : files) {
             if (!reduceSameFiles.contains(it)) {
                 reduceSameFiles.append(it);
             }
         }

问题:
这段代码的目的是对 files 列表去重。QStringList::contains() 的时间复杂度是 $O(N)$,嵌套在循环中导致整体去重的时间复杂度达到了 $O(N^2)$。如果拖拽安装的字体文件数量较多,这里会产生不必要的性能开销。

改进建议:
利用 QSet 进行去重,时间复杂度降为 $O(N)$,然后再转回 QStringList

// 利用 QSet 去重,性能极佳
QSet<QString> seenFiles;
QStringList reduceSameFiles;
reduceSameFiles.reserve(files.size());

for (const auto &it : files) {
    if (seenFiles.insert(it).second) { // insert 返回 pair<iterator, bool>,bool 为 true 表示首次插入
        reduceSameFiles.append(it);
    }
}

// 或者更简洁的 Qt 风格写法(如果不需要保持原始顺序):
// QStringList reduceSameFiles = QSet<QString>(files.begin(), files.end()).values();

3. 代码安全 - 🛡️ 命令注入风险与进程阻塞

文件: libdeepin-font-manager/dfontinfomanager.cpp

+#if QT_VERSION_MAJOR > 5
+    process.startCommand("fc-match -v |grep file");
+#else
     process.start("fc-match -v |grep file");
+#endif
     process.waitForFinished(-1);

问题:

  1. 命令注入风险:虽然当前硬编码的字符串没有直接风险,但使用管道符 | 执行命令依赖于系统 shell 解析,这是一种不安全的编程习惯。如果未来有任何变量拼接到这个字符串中,将引发命令注入。
  2. 管道符失效:在Qt6中,startCommand 默认不通过 shell 执行,这意味着 | grep file 可能会被当作 fc-match 的参数传递,导致管道失效,无法正确过滤输出。
  3. 进程阻塞waitForFinished(-1) 会无限期阻塞当前线程。如果 fc-match 命令挂起,整个应用(如果是主线程)将冻死。

改进建议:
不要依赖系统 shell 的管道,而是在 Qt 代码层面获取全部输出后再进行字符串过滤;同时为 waitForFinished 设置合理的超时时间。

QProcess process;
#if QT_VERSION_MAJOR > 5
    process.startCommand("fc-match -v"); // 移除管道,直接获取全部输出
#else
    process.start("fc-match -v");
#endif

    // 设置超时时间,例如 3000 毫秒,防止主线程无限阻塞
    if (process.waitForFinished(3000)) {
        QString output = process.readAllStandardOutput();
        // 在代码层使用正则或字符串查找来替代 grep
        QStringList lines = output.split('\n');
        for (const QString &line : lines) {
            if (line.contains("file:", Qt::CaseInsensitive)) {
                // 处理对应的行
                break;
            }
        }
    } else {
        qWarning() << "fc-match process timed out or failed:" << process.errorString();
    }

4. 代码质量 - 🧹 代码整洁度与一致性

文件: libdeepin-font-manager/fontmanagercore.cpp & 测试文件中的 QMouseEvent 适配

  1. QProcess API 适配的一致性
    fontmanagercore.cpp 中,你使用了 #if QT_VERSION_MAJOR > 5 来区分 startDetachedstartCommand。但在 Qt 5.14 之后,QProcess 已经引入了 startCommand 方法。建议直接使用 QProcess::startCommand,并在低版本 Qt 编译时通过旧接口兼容,这样代码更干净。

  2. QMouseEvent 冗长的宏判断
    测试代码中充斥着大量的 #if QT_VERSION_MAJOR > 5 来处理 QPointFQPoint 的差异。这严重降低了代码的可读性。
    建议:在测试工程的公共头文件中定义一个内联构造辅助函数,统一屏蔽 Qt5/Qt6 的差异:

    // 在某个公共测试头文件中定义
    inline QMouseEvent* createMouseEvent(QEvent::Type type, const QPointF &pos, Qt::MouseButton button, Qt::MouseButton button2 = Qt::NoButton, Qt::KeyboardModifiers mod = Qt::NoModifier) {
    #if QT_VERSION_MAJOR > 5
        return new QMouseEvent(type, pos, QPointF(), button, button2, mod);
    #else
        return new QMouseEvent(type, pos.toPoint(), button, button2, mod);
    #endif
    }
    
    // 测试代码中使用:
    QMouseEvent *e = createMouseEvent(QEvent::MouseButtonPress, QPointF(623, 23), Qt::LeftButton);

    这样测试代码将变得非常清爽。

  3. Stub 代码的严格别名违规修复

    -        *(long long *)(fn + 8) = (long long)fn_stub;
    +        { long long _stub_val = (long long)fn_stub; std::memcpy(fn + 8, &_stub_val, sizeof(_stub_val)); }

    这里的修改非常棒!原代码通过指针强转写入内存,违反了C++的严格别名规则,在较高优化级别下可能被编译器错误优化。使用 std::memcpy 是标准推荐的安全做法,点赞!

总结

本次 Diff 的核心修改(替换 foreach、适配 Qt6 API)是积极且必要的。但请务必优先修复 dfontpreviewer.cpp 中循环内修改容器的逻辑隐患,并关注 dfontmgrmainwindow.cpp 中的去重性能问题 以及 fc-match 调用带来的潜在阻塞与安全问题。修复这些后,代码质量将得到显著提升。

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lzwind, pengfeixx

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

@pengfeixx

Copy link
Copy Markdown
Contributor Author

/merge

@pengfeixx pengfeixx merged commit 6ca3417 into linuxdeepin:master Jun 4, 2026
19 checks passed
@pengfeixx pengfeixx deleted the feat/ut-qt6-compat branch June 4, 2026 11:47
@deepin-bot

deepin-bot Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

This pr cannot be merged! (status: unknown)

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