Skip to content

Conversation

@lichaofan2008
Copy link

@lichaofan2008 lichaofan2008 commented Jan 21, 2026

Some corrupted PNG files will trigger error messages during parsing, but data is still read from them, and we process them following the normal procedure.
某些已损坏的PNG文件解析时会报错,但同时也读取到了数据,我们按正常流程处理。

Bug: https://pms.uniontech.com//bug-view-346475.html

v20 BUG 分支合一到v25主线
Task: https://pms.uniontech.com/task-view-383477.html

Summary by Sourcery

Handle partially readable images from corrupted PNG files during image loading and add logging around the read process.

Bug Fixes:

  • Ensure corrupted PNG files that yield readable image data are still processed through the normal image loading flow.

Enhancements:

  • Add diagnostic logging for image readability and read success/failure to aid debugging of image loading issues.

Some corrupted PNG files will trigger error messages during parsing, but data is still read from them, and we process them following the normal procedure.
某些已损坏的PNG文件解析时会报错,但同时也读取到了数据,我们按正常流程处理。

Bug: https://pms.uniontech.com//bug-view-346475.html

v20 BUG 分支合一到v25主线
Task: https://pms.uniontech.com/task-view-383477.html
@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lichaofan2008

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

@sourcery-ai
Copy link

sourcery-ai bot commented Jan 21, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adjusts PNG image loading to use the boolean-returning overload of QImageReader::read, adds logging around reading behavior, and preserves the existing DPI adjustment logic.

Sequence diagram for updated PNG image loading and logging

sequenceDiagram
    participant FileHander
    participant loadImage_helper
    participant QImageReader as QImageReader_reader
    participant Logger

    FileHander->>loadImage_helper: loadImage_helper(path, hander)
    activate loadImage_helper
    loadImage_helper->>QImageReader_reader: construct with path
    loadImage_helper->>QImageReader_reader: canRead()
    QImageReader_reader-->>loadImage_helper: bool canRead

    alt reader can read image data
        loadImage_helper->>Logger: qInfo img can read, file path
        loadImage_helper->>QImageReader_reader: read(img)
        QImageReader_reader-->>loadImage_helper: bool readOk
        alt readOk is true
            loadImage_helper->>Logger: qInfo img read success after can read, file path
        else readOk is false
            loadImage_helper->>Logger: qWarning img read failed after can read, file path
        end
        loadImage_helper->>loadImage_helper: adjust DPI if needed (Qt5)
    else reader cannot read image data
        loadImage_helper->>loadImage_helper: handle unreadable image
    end

    loadImage_helper-->>FileHander: QImage img
    deactivate loadImage_helper
Loading

Flow diagram for updated loadImage_helper PNG handling

flowchart TD
    A[Start loadImage_helper with path] --> B[Create QImageReader with path]
    B --> C{reader.canRead}
    C -- No --> D[Handle unreadable image and return]
    C -- Yes --> E[qInfo img can read, file path]
    E --> F[Call reader.read into img]
    F --> G{read returned true}
    G -- Yes --> H[qInfo img read success after can read, file path]
    G -- No --> I[qWarning img read failed after can read, file path]
    H --> J[Adjust DPI if needed - Qt5]
    I --> J[Adjust DPI if needed - Qt5]
    J --> K[Return QImage img]
    D --> K[Return QImage img]
Loading

File-Level Changes

Change Details Files
Use the QImageReader::read(QImage*) overload to better handle partially corrupted PNGs and add diagnostics around read outcomes.
  • Replace direct initialization via QImage img = reader.read() with a default-constructed QImage and a call to reader.read(&img).
  • Add qInfo logging when reader.canRead() is true to record the image file path being processed.
  • Log success with qInfo or failure with qWarning based on the boolean result of reader.read(&img).
  • Keep subsequent DPI correction and normal image-processing logic unchanged so that successfully read data from partially corrupted PNGs are still processed.
src/service/filehander.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@deepin-ci-robot
Copy link

deepin pr auto review

这段代码主要针对图片加载逻辑进行了修改,增加了一些日志输出,并修改了图片读取的判断逻辑。以下是对这段 diff 的详细审查和改进意见:

1. 语法逻辑审查

  • 现状:代码使用了 reader.read(&img) 来尝试读取图片,并利用 if 语句判断返回值,但无论读取成功(true)还是失败(false),代码都会继续向下执行(进入后续的 DPI 处理逻辑)。
  • 问题:根据注释“某些已损坏的PNG文件这里会返回false,但同时也读取到了数据,我们按正常流程处理”,代码意图是容错处理。然而,如果 reader.read(&img) 返回 falseimg 为空(未读取到任何有效像素数据),继续使用这个空的 QImage 对象可能会导致后续逻辑(如 DPI 计算、绘制)崩溃或产生未定义行为。
  • 改进建议:在判断为 false 后,应增加对 img.isNull() 的检查。如果图片为空,应终止处理或返回错误,而不是盲目继续。

2. 代码质量审查

  • 日志泛滥
    • qInfo() << "img can read, file:" << path;:对于图片加载这种高频操作,使用 qInfo 会在控制台产生大量日志,影响性能且难以排查关键信息。
    • qWarning():在读取失败时打印警告是合理的,但如果因为“损坏但可读”的情况导致每次都打印警告,会造成日志噪音。
  • 注释清晰度:注释解释了为什么忽略返回值,这很好,但逻辑上没有完全落实(见上文逻辑审查)。
  • 改进建议
    • 移除 img can read 的普通日志。
    • 仅在读取完全失败(返回 false 且 img 为空)时打印 qWarning
    • 如果是为了调试特定损坏文件,建议使用 #ifdef QT_DEBUG 包裹日志。

3. 代码性能审查

  • 对象构造QImage img; 默认构造是轻量的,这里没有问题。
  • I/O 操作reader.read(&img) 是主要的性能消耗点,这是必须的操作。
  • 改进建议:目前的改动对性能影响较小,主要影响在于日志输出。减少不必要的日志输出可以略微提升 I/O 性能。

4. 代码安全审查

  • 空指针/空对象风险
    • 如果 reader.read(&img) 失败且 img 为空,后续代码访问 img.logicalDpiX() 可能会导致程序崩溃(取决于 Qt 实现,通常空图返回 0,但访问成员函数仍有风险)。
  • 日志注入
    • path 来自外部输入。虽然 qInfo 通常输出到控制台或文件,直接拼接路径字符串一般不会导致代码执行漏洞,但在某些日志解析系统中,特殊字符可能导致日志格式混乱。
  • 改进建议
    • 务必检查 !img.isNull()
    • 确保在 img 无效时,函数有明确的错误返回路径(例如返回空 QImage 或错误码),而不是继续执行。

综合改进后的代码建议

    if (reader.canRead()) {
        QImage img;
        
        // 尝试读取图片
        if (!reader.read(&img)) {
            // 读取失败,检查是否读取到了部分数据(针对特定损坏文件)
            if (img.isNull()) {
                qWarning() << "Failed to read image and data is null, file:" << path;
                return QImage(); // 或者根据函数签名返回适当的错误状态
            }
            // 如果 img 不为空,虽然 read 返回 false,我们按注释要求继续处理
            #ifdef QT_DEBUG
            qWarning() << "Image read returned false but data is available (possibly corrupted), file:" << path;
            #endif
        }

        // 确保 img 有效后再进行后续操作
        if (!img.isNull()) {
#if (QT_VERSION_MAJOR == 5)
            auto desktop = QApplication::desktop();
            // 增加对 desktop 的有效性检查,尽管上面有检查,但这里再次确保 img 有效
            if (Q_NULLPTR != desktop && img.logicalDpiX() != desktop->logicalDpiX()) {
                // ... DPI 处理逻辑
            }
#endif
            // ... 其他后续逻辑
            return img;
        }
    }

总结修改点:

  1. 逻辑修正:增加了 img.isNull() 检查,防止在读取完全失败后使用空对象。
  2. 日志优化:移除了冗余的 qInfo,将 qWarning 限制在真正需要关注的场景(完全失败或调试模式下)。
  3. 安全性增强:确保后续 DPI 逻辑仅在 img 有效时执行。
  4. 容错性:明确区分“完全失败”和“部分读取失败(损坏但可用)”两种情况。

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • The new qInfo/qWarning logs on every image load may be quite noisy in normal operation; consider downgrading to qDebug or guarding them behind a verbose/debug flag so routine loads don't flood the logs.
  • When logging failures in reader.read(&img), it could be more actionable to include reader.error() / reader.errorString() in the message so that log output directly indicates the underlying parsing issue.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The new qInfo/qWarning logs on every image load may be quite noisy in normal operation; consider downgrading to qDebug or guarding them behind a verbose/debug flag so routine loads don't flood the logs.
- When logging failures in `reader.read(&img)`, it could be more actionable to include `reader.error()` / `reader.errorString()` in the message so that log output directly indicates the underlying parsing issue.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@lichaofan2008
Copy link
Author

/merge

@deepin-bot deepin-bot bot merged commit e42fce8 into linuxdeepin:develop/snipe Jan 21, 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.

2 participants