Skip to content

refactor: optimize file sorting implementation#276

Merged
deepin-bot[bot] merged 2 commits intolinuxdeepin:masterfrom
Johnson-zs:feat_ocr
Apr 21, 2026
Merged

refactor: optimize file sorting implementation#276
deepin-bot[bot] merged 2 commits intolinuxdeepin:masterfrom
Johnson-zs:feat_ocr

Conversation

@Johnson-zs
Copy link
Copy Markdown
Contributor

  1. Refactor file sorting logic into dedicated DFileSorter class with
    SortRole enum and SortConfig struct
  2. Implement DSortKeyCache for performance optimization of string
    comparisons
  3. Add thread-safe QCollator usage with thread_local storage
  4. Simplify enumerator code by delegating sorting to DFileSorter
  5. Add comprehensive unit tests covering sorting by name, size, time
    etc.
  6. Support both mixed and separated directory/file sorting modes
  7. Implement natural number sorting via QCollator numeric mode

Log: Improved file sorting performance and maintainability

Influence:

  1. Test various sorting combinations (name/size/time ascending/
    descending)
  2. Verify mixed vs separated directory/file sorting modes
  3. Check natural number sorting (e.g. file2 < file10)
  4. Test edge cases (empty lists, single item)
  5. Verify thread safety with concurrent sorting
  6. Check localized string sorting behavior

refactor: 优化文件排序实现

  1. 将文件排序逻辑重构为专用的 DFileSorter 类,包含 SortRole 枚举和
    SortConfig 结构体
  2. 实现 DSortKeyCache 用于字符串比较的性能优化
  3. 使用 thread_local 存储实现线程安全的 QCollator
  4. 简化枚举器代码,将排序委托给 DFileSorter
  5. 添加全面的单元测试,覆盖按名称、大小、时间等排序
  6. 支持混合和分离的目录/文件排序模式
  7. 通过 QCollator 数字模式实现自然数字排序

Log: 提升文件排序性能和可维护性

Influence:

  1. 测试各种排序组合(名称/大小/时间 升序/降序)
  2. 验证混合与分离的目录/文件排序模式
  3. 检查自然数字排序(例如 file2 < file10)
  4. 测试边界情况(空列表、单项)
  5. 验证并发排序时的线程安全性
  6. 检查本地化字符串排序行为

1. Refactor file sorting logic into dedicated DFileSorter class with
SortRole enum and SortConfig struct
2. Implement DSortKeyCache for performance optimization of string
comparisons
3. Add thread-safe QCollator usage with thread_local storage
4. Simplify enumerator code by delegating sorting to DFileSorter
5. Add comprehensive unit tests covering sorting by name, size, time
etc.
6. Support both mixed and separated directory/file sorting modes
7. Implement natural number sorting via QCollator numeric mode

Log: Improved file sorting performance and maintainability

Influence:
1. Test various sorting combinations (name/size/time ascending/
descending)
2. Verify mixed vs separated directory/file sorting modes
3. Check natural number sorting (e.g. file2 < file10)
4. Test edge cases (empty lists, single item)
5. Verify thread safety with concurrent sorting
6. Check localized string sorting behavior

refactor: 优化文件排序实现

1. 将文件排序逻辑重构为专用的 DFileSorter 类,包含 SortRole 枚举和
SortConfig 结构体
2. 实现 DSortKeyCache 用于字符串比较的性能优化
3. 使用 thread_local 存储实现线程安全的 QCollator
4. 简化枚举器代码,将排序委托给 DFileSorter
5. 添加全面的单元测试,覆盖按名称、大小、时间等排序
6. 支持混合和分离的目录/文件排序模式
7. 通过 QCollator 数字模式实现自然数字排序

Log: 提升文件排序性能和可维护性

Influence:
1. 测试各种排序组合(名称/大小/时间 升序/降序)
2. 验证混合与分离的目录/文件排序模式
3. 检查自然数字排序(例如 file2 < file10)
4. 测试边界情况(空列表、单项)
5. 验证并发排序时的线程安全性
6. 检查本地化字符串排序行为
@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Johnson-zs

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

Added comprehensive test cases for DFileSorter to better handle:
1. Sorting by size with directories (directories should have consistent
effective size -1)
2. Size sorting in descending order (directories move to end)
3. Last modified time sorting with symlinks (use target file time)
4. Last read time sorting with symlinks (use target file time)

The changes include new unit tests that verify:
- Directory sorting behavior in size comparisons
- Correct ordering in descending size sort
- Proper handling of symbolic links when sorting by time attributes
- Mixing of directories and files in different sort scenarios

Influence:
1. Test file sorting by size with directories and mixed files
2. Verify descending size sorting places directories correctly
3. Check symlink time sorting uses target file times
4. Validate directory/file separation maintains correct order
5. Test various combinations of the above scenarios with different file
sets

test: 增强文件排序测试,支持目录和符号链接特殊情况

新增了针对DFileSorter的全面测试用例,主要涵盖:
1. 包含目录的按大小排序(目录始终使用-1作为有效大小)
2. 大小降序排序(目录应排在最后)
3. 带符号链接的按修改时间排序(使用目标文件时间)
4. 带符号链接的按访问时间排序(使用目标文件时间)

变更内容包括:
- 验证目录在大小排序中的行为
- 降序排序时目录的正确位置
- 符号链接时间属性使用目标文件时间的处理
- 混合目录和文件在不同排序场景中的表现

Influence:
1. 测试包含目录的按大小文件排序
2. 验证降序大小排序中目录的位置是否正确
3. 检查符号链接时间排序是否使用目标文件时间
4. 验证目录和文件分离时是否保持正确顺序
5. 测试不同文件组合下的各种排序场景
@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

Git Diff 代码审查报告

我已仔细审查了提供的 git diff 内容,主要涉及文件排序功能的重构和测试代码的添加。以下是详细的审查意见:

1. 语法逻辑

1.1 CMakeLists.txt 修改

优点

  • 将测试源文件从 GLOB_RECURSE 改为显式列出,更加明确和可控
  • 添加了共享的测试入口点 test_main.cpp
  • 简化了重复的 target_include_directories 代码

建议

  • 可以考虑将 TEST_CLASS_SRCS 改为使用 file(GLOB ...) 但限制在当前目录,这样添加新测试文件时不需要修改 CMakeLists.txt
  • 添加对 TEST_COMMON_SRCSTEST_CLASS_SRCS 是否存在的检查,避免文件不存在时构建失败

1.2 test_main.cpp

问题

  • 使用了 extern 声明测试运行函数,但没有确保这些函数在其他文件中确实被正确导出
  • 没有包含必要的头文件,如 tst_dfm_io.htst_dfilesorter.h

建议

// 在 test_main.cpp 中添加
#include "tst_dfm_io.h"
#include "tst_dfilesorter.h"

// 移除 extern 声明,直接使用类
int main(int argc, char *argv[])
{
    int status = 0;
    tst_DfmIO tc1;
    status |= QTest::qExec(&tc1, argc, argv);
    
    tst_DFileSorter tc2;
    status |= QTest::qExec(&tc2, argc, argv);
    
    return status;
}

1.3 tst_dfilesorter.cpp

优点

  • 测试用例覆盖全面,包括基本功能、边界条件和特殊场景
  • 使用了 QTemporaryDir 进行临时文件测试,确保测试隔离性
  • 添加了对符号链接的特殊处理测试

问题

  • fileSorter_sortByLastModified_withSymlinkfileSorter_sortByLastRead_withSymlink 测试中,创建文件后没有验证文件时间是否设置成功
  • 没有测试 sortKeyCache_clear() 方法

建议

// 在 fileSorter_sortByLastModified_withSymlink 中添加验证
QFile f(oldFile);
QVERIFY(f.open(QIODevice::ReadOnly));
QDateTime modTime = f.fileTime(QFileDevice::FileModificationTime);
QVERIFY(modTime.toSecsSinceEpoch() == 1000); // 验证时间设置成功
f.close();

1.4 dfilesorter.cpp 和 dfilesorter.h

优点

  • 使用了移动语义优化性能
  • 实现了多种排序方式,包括名称、大小、修改时间等
  • 对符号链接有特殊处理,使用目标文件的时间进行排序
  • 使用了 std::stable_sort 保持相等元素的相对顺序

问题

  • compareBySize 中对目录的处理方式(返回-1)可能导致与其他排序逻辑不一致
  • 没有处理 SortConfig 中无效值的情况
  • 缺少对 symlinkUrl 为空但 isSymLink 为 true 的边界情况处理

建议

// 在 compareBySize 中添加更明确的注释和逻辑
auto getEffectiveSize = [](const QSharedPointer<DEnumerator::SortFileInfo> &info) -> qint64 {
    // 目录始终排在最前面,使用特殊值 -1
    if (info->isDir) {
        return -1;
    }
    // 对于符号链接,如果无法获取目标大小,返回0
    if (info->isSymLink && !info->symlinkUrl.isValid()) {
        qWarning() << "Invalid symlink URL for:" << info->url;
        return 0;
    }
    return info->filesize;
};

// 在 DFileSorter 构造函数中添加配置验证
DFileSorter::DFileSorter(const SortConfig &config)
    : m_config(config)
{
    // 验证配置有效性
    if (m_config.role > SortRole::LastRead) {
        qWarning() << "Invalid sort role, falling back to Name";
        m_config.role = SortRole::Name;
    }
}

1.5 dsortkeycache.cpp 和 dsortkeycache.h

优点

  • 使用了单例模式和线程安全的缓存实现
  • 使用了读写锁提高并发性能
  • 使用了 thread_local 存储线程特定的 QCollator 实例

问题

  • 没有实现缓存大小限制,可能导致内存无限增长
  • clear() 方法没有在语言环境变化时自动调用
  • 没有处理 QCollator 初始化失败的情况

建议

// 在 dsortkeycache.h 中添加缓存大小限制
class DSortKeyCache
{
private:
    static const int MAX_CACHE_SIZE = 10000; // 最大缓存条目数
    
    // 添加LRU缓存替代简单的QHash
    QCache<QString, QCollatorSortKey> m_cache;
    
    // 添加语言环境变化监听
    void setupLocaleChangeListener();
};

// 在 dsortkeycache.cpp 中实现
void DSortKeyCache::setupLocaleChangeListener()
{
    // 监听系统语言环境变化,自动清除缓存
    QObject::connect(qApp, &QApplication::localeChanged, []() {
        DSortKeyCache::instance().clear();
    });
}

QCollatorSortKey DSortKeyCache::sortKey(const QString &text)
{
    // 先尝试读锁查找缓存
    {
        QReadLocker locker(&m_lock);
        if (m_cache.contains(text)) {
            return *m_cache.object(text);
        }
    }

    // 缓存未命中,需要创建
    QWriteLocker locker(&m_lock);
    
    // 双重检查
    if (m_cache.contains(text)) {
        return *m_cache.object(text);
    }

    QCollatorSortKey key = collator().sortKey(text);
    m_cache.insert(text, new QCollatorSortKey(key));
    return key;
}

2. 代码质量

2.1 命名规范

  • 整体命名规范良好,使用了驼峰命名法和有意义的名称
  • 类名、方法名、变量名都符合 Qt 命名约定
  • 建议将 sortKeyCache_basic 等测试方法名改为更描述性的名称,如 testSortKeyCacheBasic

2.2 注释和文档

  • 类和方法有良好的文档注释
  • 建议为复杂逻辑添加更多行内注释,特别是排序比较函数
  • 建议为 SortRole 枚举值添加更详细的说明,特别是排序行为的细节

2.3 代码结构

  • 代码结构清晰,职责分离良好
  • 建议将 getSymlinkTargetTime 函数移到单独的工具类中,提高可重用性
  • 考虑将排序比较函数提取为独立的策略类,便于扩展新的排序方式

3. 代码性能

3.1 优化建议

  1. 缓存策略

    • 当前 DSortKeyCache 没有大小限制,建议添加 LRU 缓存策略
    • 考虑添加缓存命中率统计,便于性能分析
  2. 排序性能

    • 对于大型文件列表,可以考虑使用并行排序算法
    • 对于 compareByName,可以考虑预先生成所有排序键,减少重复计算
  3. 内存使用

    • sort 方法的移动语义实现很好,但可以考虑添加原地排序选项
    • 对于 separateDirAndFile,可以考虑使用预分配的容器减少内存重新分配
// 优化示例:预生成排序键
QList<QSharedPointer<DEnumerator::SortFileInfo>> DFileSorter::sort(
        QList<QSharedPointer<DEnumerator::SortFileInfo>> &&files)
{
    if (files.isEmpty()) {
        return files;
    }

    // 预生成所有排序键
    QHash<QSharedPointer<DEnumerator::SortFileInfo>, QCollatorSortKey> sortKeys;
    for (const auto &file : files) {
        QString name = file->url.fileName();
        sortKeys[file] = DSortKeyCache::instance().sortKey(name);
    }

    // 使用预生成的排序键进行比较
    auto compareFunc = [&sortKeys](const auto &a, const auto &b) {
        return sortKeys[a] < sortKeys[b];
    };

    // ... 其余排序逻辑
}

4. 代码安全

4.1 潜在安全问题

  1. 符号链接处理

    • 当前代码正确处理了符号链接,但建议添加对循环链接的检测
    • 考虑添加对符号链接目标不存在的情况处理
  2. 输入验证

    • 建议添加对 SortConfig 参数的验证,防止无效输入
    • 考虑添加对文件路径的验证,防止路径遍历攻击
  3. 线程安全

    • DSortKeyCache 的实现是线程安全的,但建议添加单元测试验证
    • DFileSorter 不是线程安全的,建议在文档中明确说明
// 安全增强示例:循环链接检测
SymlinkTimeInfo getSymlinkTargetTime(const QUrl &symlinkUrl, QSet<QString> &visited)
{
    SymlinkTimeInfo info;
    if (!symlinkUrl.isValid() || !symlinkUrl.isLocalFile()) {
        return info;
    }

    QString path = symlinkUrl.path();
    if (visited.contains(path)) {
        qWarning() << "Detected symlink loop:" << path;
        return info; // 返回无效信息
    }
    visited.insert(path);

    struct stat st;
    if (stat(path.toUtf8().constData(), &st) == 0) {
        info.lastModified = st.st_mtim.tv_sec;
        info.lastModifiedNs = st.st_mtim.tv_nsec;
        info.lastRead = st.st_atim.tv_sec;
        info.lastReadNs = st.st_atim.tv_nsec;
        info.valid = true;
    }
    return info;
}

5. 其他建议

  1. 错误处理

    • 添加更详细的错误日志,便于调试
    • 考虑添加错误回调机制,让调用者可以自定义错误处理
  2. 测试覆盖

    • 添加对边界条件的测试,如空文件名、特殊字符文件名
    • 添加对大型文件列表的性能测试
    • 添加对多线程环境的测试
  3. 文档

    • 添加使用示例和最佳实践文档
    • 添加性能基准测试结果文档

总结

整体来看,这段代码质量较高,实现了文件排序的核心功能,并考虑了性能优化和线程安全。主要改进点在于:

  1. 增强输入验证和错误处理
  2. 优化缓存策略,防止内存无限增长
  3. 添加更全面的测试用例
  4. 改进符号链接处理,特别是循环链接的情况
  5. 增强文档和注释

这些改进将使代码更加健壮、可维护和高效。

@Johnson-zs
Copy link
Copy Markdown
Contributor Author

/forcemerge

@deepin-bot
Copy link
Copy Markdown

deepin-bot Bot commented Apr 21, 2026

This pr force merged! (status: blocked)

@deepin-bot deepin-bot Bot merged commit ec24947 into linuxdeepin:master Apr 21, 2026
21 checks passed
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.

2 participants