Skip to content

fix(multi-image): fix blank page when switching multi-page TIFF frames#292

Merged
wyu71 merged 1 commit into
linuxdeepin:masterfrom
wyu71:master
May 12, 2026
Merged

fix(multi-image): fix blank page when switching multi-page TIFF frames#292
wyu71 merged 1 commit into
linuxdeepin:masterfrom
wyu71:master

Conversation

@wyu71
Copy link
Copy Markdown
Contributor

@wyu71 wyu71 commented May 12, 2026

Cache loaded image data unconditionally in loadFinished() to prevent data loss from duplicate async load requests, and defer delayInfo cleanup until terminal status (Ready/Error) to ensure frame count update completes.

修复多页图切换第二页显示空白的问题,loadFinished() 无条件缓存
加载结果避免竞态丢数据,delayInfo 等待终态再清理确保帧数更新。

Log: 修复多页TIFF切换空白
PMS: BUG-359509
Influence: 修复后多页图(如多页TIFF)可正常逐页切换,不再出现空白或显示错误图片。

Cache loaded image data unconditionally in loadFinished() to prevent
data loss from duplicate async load requests, and defer delayInfo
cleanup until terminal status (Ready/Error) to ensure frame count
update completes.

修复多页图切换第二页显示空白的问题,loadFinished() 无条件缓存
加载结果避免竞态丢数据,delayInfo 等待终态再清理确保帧数更新。

Log: 修复多页TIFF切换空白
PMS: BUG-359509
Influence: 修复后多页图(如多页TIFF)可正常逐页切换,不再出现空白或显示错误图片。
@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

你好!我是CodeGeeX。我已仔细审查了你提供的Git Diff。本次修改主要涉及两个方面:一是修改了图片缓存插入的逻辑,二是处理了异步加载状态机中的瞬态问题。

以下是我对本次代码变更的详细审查意见,涵盖语法逻辑、代码质量、代码性能和代码安全四个维度:

1. 语法与逻辑

  • 潜在的逻辑漏洞:移除 waitSet.contains(key) 带来的缓存污染风险
    • 分析:原代码 if (data && waitSet.contains(key)) 确保了只有当该图片被请求且处于等待状态时,才会将数据插入缓存。修改后的代码 if (data) 变成了无条件插入。如果系统中存在其他未经过 waitSet 流程的图片加载完成事件,或者由于某些异常导致重复回调,缓存中可能会被插入预期之外或重复的数据,破坏了缓存与请求状态的强一致性。
    • 建议:Diff中的注释提到:“Requires loadFinished() to cache data unconditionally so that the subsequent Ready status change is guaranteed to fire.” 这说明移除条件是为了配合下文的状态修复。但如果 waitSet 机制仍然存在,建议评估是否可以用更精确的条件替换,或者确保 loadFinished 的调用方是唯一且可控的。如果确定要无条件缓存,建议在注释中更详细地说明为何此时 waitSet 检查不再必要,以防后续维护者引入回归缺陷。
  • 状态机逻辑修复的合理性
    • 分析:在 pathviewproxymodel.cpp 中增加的 if (ImageInfo::Loading == delayInfo->status()) { return; } 是非常正确的。Loading 是一个瞬态,如果在此处处理逻辑,随后再次触发 Ready 会导致逻辑重复执行。等待终态(ReadyError)再处理是标准的状态机设计模式。

2. 代码质量

  • 注释与代码的强耦合(自文档化)
    • 分析:你在 pathviewproxymodel.cpp 中添加的注释非常棒,清晰地解释了为什么要忽略 Loading 状态,并且指出了这段逻辑依赖的前提。这种注释对长期维护极具价值。
  • 版权声明修改的合理性
    • 分析:将版权从 2024 修改为 2024 - 2026。通常,开源项目(如GPL)的版权年份应该是作品首次发布年份至最新修改年份。如果该文件在2024年首次创建,且在2026年有修改,写 2024 - 2026 是合规的。但请注意,当前是2024年(或2025年初),写2026可能是一种预留策略,部分开源合规审查工具可能会对未来的年份提出警告,建议确认项目组的规范。

3. 代码性能

  • 缓存插入的性能考量
    • 分析cache.insert(key, data) 的无条件化意味着任何 data 非空的情况都会触发插入。如果 cache 是一个具有容量限制的缓存(如 LRU Cache),这可能会导致原本不需要缓存的非 waitSet 数据挤掉有用的缓存数据,增加缓存未命中率和频繁的淘汰开销。
    • 建议:确认 cache 的实现机制。如果它是无限缓存,性能影响不大;如果是有限缓存,需评估缓存命中率是否下降。
  • 信号连接与Lambda捕获
    • 分析connect(delayInfo, &ImageInfo::statusChanged, this, [this, delayInfo, sourceIndex]() 捕获了 this 指针。如果 delayInfo 的生命周期比 this(即 PathViewProxyModel)长,或者在异步回调时 Model 已被销毁,会引发崩溃。虽然这不是本次Diff引入的新问题,但在处理异步状态回调时值得警惕。

4. 代码安全

  • 内存泄漏与悬空指针风险
    • 分析:在 pathviewproxymodel.cpp 中,delayInfo 是通过 new ImageInfo(url, this) 创建的,指定了 this 作为父对象。在Qt的对象树机制下,只要 this (PathViewProxyModel) 被正确销毁,delayInfo 也会被自动释放,这部分是安全的。
    • 风险点:在 imageinfo.cpp 中,cache.insert(key, data) 中的 data 是裸指针。如果 cache 仅仅存储了指针而没有接管所有权,当 data 指向的内存被释放时,cache 中就会存在悬空指针,后续访问将导致严重的Use-After-Free安全漏洞。
    • 建议:确保 cache (如 ThumbnailCache) 具有明确的所有权语义。推荐使用智能指针(如 QSharedPointer<ImageInfo>std::shared_ptr<ImageInfo>)来管理缓存中的对象,或者在 ThumbnailCache 的析构/淘汰逻辑中严格释放对应的 data 内存。

总结与改进建议代码

本次修改的核心逻辑(处理异步状态机的瞬态问题)是正确的,但移除 waitSet 检查带来的副作用需要被明确记录或重新评估。以下是一个结合了上述建议的改进版本示例:

imageinfo.cpp 改进建议:

// 建议保留对 waitSet 的检查,或者如果确信需要无条件缓存,请完善注释说明原因。
// 如果坚持无条件缓存,建议增加防重复插入的判断,避免覆盖有效数据
if (data) {
    // [修改说明] 移除 waitSet.contains(key) 检查,因为异步状态回调依赖
    // loadFinished 必须无条件缓存数据,以保证后续 Ready 状态触发时数据可用。
    // 若无此逻辑,多页图的异步更新可能会因缓存未命中而失败。
    if (!cache.contains(key)) { // 避免可能的重复插入/覆盖
        cache.insert(key, data);
    }
    qCDebug(logImageViewer) << "Image loaded successfully:" << path << "frame:" << frameIndex
                            << "type:" << data->type << "size:" << data->size;
}

pathviewproxymodel.cpp 改进建议:

// ... 前置代码 ...
ImageInfo *delayInfo = new ImageInfo(url, this);
connect(delayInfo, &ImageInfo::statusChanged, this, [this, delayInfo, sourceIndex]() {
    qCDebug(logImageViewer) << "AsyncUpdateLoadInfo: ImageInfo status changed for:" << delayInfo->source().toString();
    
    // Loading is a transient state; wait for a terminal state (Ready/Error).
    // Requires loadFinished() to cache data unconditionally so that
    // the subsequent Ready status change is guaranteed to fire.
    if (ImageInfo::Loading == delayInfo->status()) {
        return;
    }

    // 仅更新加载成功的多页图数据,其它类型的数据无需关注,默认的 frameIndex 都是 0
    if (ImageInfo::Ready == delayInfo->status() && Types::MultiImage == delayInfo->type()) {
        qCDebug(logImageViewer) << "Async update completed for multi-image:" << delayInfo->source().toString();
        // TODO: 考虑如果 status 是 Error,是否需要清理 delayInfo 或发送加载失败信号?
    } else if (ImageInfo::Error == delayInfo->status()) {
        qCDebug(logImageViewer) << "Async update failed for:" << delayInfo->source().toString();
        // 建议处理错误状态,防止异步任务静默失败
    }
});

希望这些审查意见对你有所帮助!如果有任何需要进一步探讨的细节,欢迎随时提问。

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

@wyu71 wyu71 merged commit 23b9747 into linuxdeepin:master May 12, 2026
20 of 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.

3 participants