-
Notifications
You must be signed in to change notification settings - Fork 38
fix: issue of corrupted PNG file. #184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: issue of corrupted PNG file. #184
Conversation
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
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts image loading for readable PNGs by switching from value-returning read() to pointer-based read(QImage*) with added logging around success/failure, ensuring partially readable corrupted PNGs still go through normal processing. Sequence diagram for updated PNG image loading and loggingsequenceDiagram
participant Caller
participant FileHander
participant QImageReader
participant QApplication
participant Desktop
Caller->>FileHander: loadImage_helper(path, hander)
FileHander->>QImageReader: construct_with_path(path)
QImageReader-->>FileHander: instance_ready
FileHander->>QImageReader: canRead()
QImageReader-->>FileHander: true
FileHander->>FileHander: create QImage img
FileHander->>FileHander: log(info) img can read, file path
FileHander->>QImageReader: read(&img)
QImageReader-->>FileHander: bool success
alt read success
FileHander->>FileHander: log(info) img read success after can read
else read failed
FileHander->>FileHander: log(warning) img read failed after can read
end
FileHander->>QApplication: desktop()
QApplication-->>FileHander: Desktop
alt desktop not null and img dpi != desktop dpi
FileHander->>Desktop: logicalDpiX()
Desktop-->>FileHander: dpiX
FileHander->>FileHander: img.setDotsPerMeterX(...)
FileHander->>FileHander: img.setDotsPerMeterY(...)
end
FileHander-->>Caller: QImage img
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- The new qInfo/qWarning logs inside loadImage_helper will run for every readable image and may be noisy in normal use; consider downgrading to qDebug or guarding them behind a verbose/log-level flag.
- When reader.read(&img) fails, you log a generic warning but still proceed with img; consider including reader.error() / reader.errorString() in the warning to make diagnosing corrupted PNG cases easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new qInfo/qWarning logs inside loadImage_helper will run for every readable image and may be noisy in normal use; consider downgrading to qDebug or guarding them behind a verbose/log-level flag.
- When reader.read(&img) fails, you log a generic warning but still proceed with img; consider including reader.error() / reader.errorString() in the warning to make diagnosing corrupted PNG cases easier.
## Individual Comments
### Comment 1
<location> `src/service/filehander.cpp:337-345` </location>
<code_context>
if (reader.canRead()) {
- QImage img = reader.read();
+ QImage img;
+ qInfo() << "img can read, file:" << path;
+ // 某些已损坏的PNG文件这里会返回false,但同时也读取到了数据,我们按正常流程处理
+ if (reader.read(&img)) {
+ qInfo() << "img read success after can read, file:" << path;
+ } else {
+ qWarning() << "img read failed after can read, file:" << path;
+ }
auto desktop = QApplication::desktop();
</code_context>
<issue_to_address>
**suggestion:** Consider lowering log level or reducing verbosity for per-image success logs.
These `qInfo()` calls will run for every successfully readable image and can flood production logs when this path is hot. Consider either switching success logs to `qDebug()`, guarding them behind a verbose/debug flag, or relying only on the `qWarning()` on failures to keep logs focused and useful.
```suggestion
if (reader.canRead()) {
QImage img;
qDebug() << "img can read, file:" << path;
// 某些已损坏的PNG文件这里会返回false,但同时也读取到了数据,我们按正常流程处理
if (reader.read(&img)) {
qDebug() << "img read success after can read, file:" << path;
} else {
qWarning() << "img read failed after can read, file:" << path;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
/forcemerge |
|
This pr force merged! (status: unstable) |
b013f77
into
linuxdeepin:release/eagle
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lichaofan2008, max-lvs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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
Summary by Sourcery
Handle image loading from PNG files that report parse errors but still contain readable data, adding logging around the read process.
Bug Fixes:
Enhancements: