Skip to content

Fix: [file-enumerator] use g_file_enumerator_next_file()#263

Merged
Johnson-zs merged 1 commit intolinuxdeepin:masterfrom
GongHeng2017:202603021640-master-fix
Mar 3, 2026
Merged

Fix: [file-enumerator] use g_file_enumerator_next_file()#263
Johnson-zs merged 1 commit intolinuxdeepin:masterfrom
GongHeng2017:202603021640-master-fix

Conversation

@GongHeng2017
Copy link
Contributor

--Replace use of g_file_enumerator_iterate() with g_file_enumerator_next_file() in DEnumerator::hasNext() so the enumerator's internal cursor is advanced by GIO and error/end conditions are handled unambiguously.

--Using g_file_enumerator_next_file() yields clearer semantics: non-NULL -> next entry, NULL + NULL error -> end, NULL + non-NULL error -> error. This avoids premature termination caused by backend-specific iterate() quirks.

--Construct next entry URL via the existing buildUrl(...) helper and preserve existing logic that creates the DFileInfo from the returned GFileInfo. Preserve existing error handling and d->checkFilter() behavior and recursive skipping of filtered entries.

--Fix URL construction so joining directory path and filename does not introduce an extra '/' (previously produced e.g. .../vault_unlocked//file). The logic now concatenates carefully (handles root path and trailing slashes) to avoid duplicate separators.

Log: fix issue
Bug: https://pms.uniontech.com//bug-view-351177.html

--Replace use of g_file_enumerator_iterate() with g_file_enumerator_next_file()
in DEnumerator::hasNext() so the enumerator's internal cursor is advanced by
GIO and error/end conditions are handled unambiguously.

--Using g_file_enumerator_next_file() yields clearer semantics: non-NULL -> next entry,
NULL + NULL error -> end, NULL + non-NULL error -> error. This avoids premature termination
caused by backend-specific iterate() quirks.

--Construct next entry URL via the existing buildUrl(...) helper and preserve
existing logic that creates the DFileInfo from the returned GFileInfo. Preserve
existing error handling and d->checkFilter() behavior and recursive skipping of filtered entries.

--Fix URL construction so joining directory path and filename does not introduce
an extra '/' (previously produced e.g. .../vault_unlocked//file). The logic now
concatenates carefully (handles root path and trailing slashes) to avoid duplicate separators.

Log: fix issue
Bug: https://pms.uniontech.com//bug-view-351177.html
@GongHeng2017 GongHeng2017 force-pushed the 202603021640-master-fix branch from d88e228 to def2b7c Compare March 2, 2026 09:14
@deepin-ci-robot
Copy link

deepin pr auto review

这段代码的修改主要涉及两个部分:buildUrl 函数的逻辑优化和 hasNext 函数中文件枚举逻辑的重构。下面我将从语法逻辑、代码质量、代码性能和代码安全四个方面进行详细审查。

1. 语法逻辑审查

buildUrl 函数

  • 改进点:原代码使用三元运算符处理路径拼接,新代码将其改为 if-else 结构,并增加了对路径末尾斜杠的检查。
  • 问题:逻辑正确,但可以进一步简化。例如,QString(fileName) 的转换可以优化为 QLatin1String 或直接使用 QStringLiteral,避免不必要的内存分配。

hasNext 函数

  • 改进点
    • 使用 g_file_enumerator_next_file 替代 g_file_enumerator_iterate,避免了 iterate 函数在不同后端上的行为不一致问题。
    • 简化了错误处理逻辑,当 nextInfoNULL 时,统一处理错误和枚举结束的情况。
  • 问题
    • 错误处理逻辑中,当 gerror 存在时,仍然返回 true,这可能导致调用者误认为还有下一个文件。建议在错误情况下返回 false,除非有特殊需求。
    • g_object_unref(nextInfo) 的调用位置正确,但可以结合 g_autoptr 自动管理内存,避免手动释放。

2. 代码质量审查

buildUrl 函数

  • 改进点:代码可读性提高,逻辑更清晰。
  • 问题
    • 变量命名 pathdirPath 可以更明确,例如 fullPathdirectoryPath
    • 可以添加注释说明路径拼接的逻辑,特别是对根目录的特殊处理。

hasNext 函数

  • 改进点
    • 减少了冗余代码,例如移除了 GFile 的获取和路径处理逻辑。
    • 错误处理更集中。
  • 问题
    • 注释 Use g_file_enumerator_next_file()... 很好,但可以补充说明为什么 iterate 的行为不一致。
    • 错误处理逻辑中,d->nextUrl = QUrl()d->dfileInfoNext.reset() 的操作可以封装为一个私有函数,例如 resetEnumeratorState(),提高复用性。

3. 代码性能审查

buildUrl 函数

  • 改进点:无显著性能影响。
  • 问题
    • QString(fileName) 的转换可以优化为 QLatin1StringQStringLiteral,减少临时对象的创建。
    • dirPath.endsWith('/') 的检查可以优化为 dirPath.at(dirPath.size() - 1) == '/',但 endsWith 的实现已经足够高效,影响不大。

hasNext 函数

  • 改进点
    • 使用 g_file_enumerator_next_file 可能比 iterate 更高效,因为 iterate 同时返回 GFileInfoGFile,而 next_file 只返回 GFileInfo
  • 问题
    • g_file_info_dup(nextInfo) 会复制 GFileInfo,如果 createFileInfoByUri 不需要所有权转移,可以考虑直接传递 nextInfo 并避免复制。
    • g_object_unref(nextInfo) 的调用可以结合 g_autoptr 自动管理,减少手动内存管理的风险。

4. 代码安全审查

buildUrl 函数

  • 改进点:无显著安全问题。
  • 问题
    • fileNameconst char* 类型,未检查是否为 nullptr 或空字符串。建议添加断言或检查。
    • 路径拼接逻辑中,未检查 fileName 是否包含路径分隔符(如 /),可能导致路径拼接错误。

hasNext 函数

  • 改进点
    • 错误处理更集中,减少了潜在的内存泄漏风险。
  • 问题
    • g_file_enumerator_next_file 返回的 nextInfo 未检查是否为 nullptr,虽然后续逻辑中有 if (nextInfo),但可以更明确。
    • 错误处理逻辑中,d->nextUrl = QUrl()d->dfileInfoNext.reset() 的操作可以封装为私有函数,避免重复代码。
    • g_file_info_dup(nextInfo) 的复制操作可能导致性能问题,如果 createFileInfoByUri 不需要所有权转移,可以直接传递 nextInfo

改进建议

buildUrl 函数

QUrl DEnumeratorPrivate::buildUrl(const QUrl &url, const char *fileName)
{
    if (!fileName || !*fileName) {
        qWarning() << "Invalid fileName";
        return QUrl();
    }

    const QString &dirPath = url.path();
    QString fullPath;

    if (dirPath == "/") {
        fullPath = "/" + QLatin1String(fileName);
    } else {
        fullPath = dirPath.endsWith('/') 
                   ? dirPath + QLatin1String(fileName) 
                   : dirPath + "/" + QLatin1String(fileName);
    }

    QUrl nextUrl = QUrl::fromLocalFile(fullPath);

    if (url.userInfo().startsWith("originPath::")) {
        nextUrl.setUserInfo(url.userInfo());
    }

    return nextUrl;
}

hasNext 函数

bool DEnumerator::hasNext() const
{
    if (d->enumFinished) {
        return false;
    }

    while (!d->stackEnumerator.isEmpty()) {
        GFileEnumerator *enumerator = d->stackEnumerator.top();
        g_autoptr(GError) gerror = nullptr;
        d->checkAndResetCancel();

        // Use g_file_enumerator_next_file() which returns a GFileInfo* for the next
        // entry (or NULL). This avoids ambiguous iterate() behavior across backends.
        GFileInfo *nextInfo = g_file_enumerator_next_file(enumerator, d->cancellable, &gerror);
        
        if (nextInfo) {
            d->nextUrl = d->buildUrl(d->uri, g_file_info_get_name(nextInfo));
            // Avoid duplicating GFileInfo if not needed
            d->dfileInfoNext = DLocalHelper::createFileInfoByUri(
                d->nextUrl, 
                nextInfo, 
                FILE_DEFAULT_ATTRIBUTES,
                d->enumLinks ? DFileInfo::FileQueryInfoFlags::kTypeNone : DFileInfo::FileQueryInfoFlags::kTypeNoFollowSymlinks
            );

            // 如果是目录且需要遍历子目录
            if (d->enumSubDir && d->dfileInfoNext && d->dfileInfoNext->attribute(DFileInfo::AttributeID::kStandardIsDir).toBool()) {
                // 处理子目录逻辑...
            }

            return true;
        }

        // Handle errors
        if (gerror) {
            d->setErrorFromGError(gerror);
            d->resetEnumeratorState(); // Reset state on error
            return false; // Return false to indicate no more items
        }

        // Current enumerator finished, pop and continue
        GFileEnumerator *enumeratorPop = d->stackEnumerator.pop();
        g_object_unref(enumeratorPop);
    }

    d->enumFinished = true;
    return false;
}

总结

  1. 语法逻辑:整体逻辑正确,但可以进一步优化路径拼接和错误处理。
  2. 代码质量:可读性提高,但可以增加注释和封装重复代码。
  3. 代码性能g_file_enumerator_next_file 的使用是改进点,但可以减少不必要的 GFileInfo 复制。
  4. 代码安全:需要添加对 fileName 的检查,并优化错误处理逻辑。

以上改进建议可以提高代码的健壮性、可读性和性能。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: GongHeng2017, 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

@Johnson-zs Johnson-zs merged commit 8bdccf3 into linuxdeepin:master Mar 3, 2026
18 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.

3 participants