feat: enhance notification icon handling #1187
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yixinshark 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 |
Reviewer's GuideThis PR refactors notification icon handling in BubbleItem by improving temporary file creation and cleanup, caching icon paths, and streamlining fallback logic to return an empty path when no icon is available. Class diagram for updated BubbleItem icon handlingclassDiagram
class BubbleItem {
+BubbleItem(QObject* parent = nullptr)
+BubbleItem(const NotifyEntity &entity, QObject* parent = nullptr)
+~BubbleItem()
+void setEntity(const NotifyEntity &entity)
+qint64 id() const
+uint bubbleId() const
+QString appName() const
+QString appIcon()
+QString summary() const
+QString body() const
+uint replacesId() const
+bool m_enablePreview
+QVariantList m_actions
+QString m_defaultAction
+QString m_tempIconPath
}
Flow diagram for enhanced notification icon handling and cleanupflowchart TD
A[Notification received] --> B{Does entity have appIcon?}
B -- Yes --> C[Return appIcon]
B -- No --> D{Is m_tempIconPath set and file exists?}
D -- Yes --> E[Return m_tempIconPath]
D -- No --> F[Call imagePathOfNotification]
F --> G[Set m_tempIconPath]
G --> H[Return m_tempIconPath]
subgraph Cleanup
I[BubbleItem destroyed]
J{m_tempIconPath exists?}
I --> J
J -- Yes --> K[Remove temp icon file]
J -- No --> L[Do nothing]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @yixinshark - I've reviewed your changes - here's some feedback:
- Instead of returning an empty string when image decoding fails, return the default application icon name to preserve the previous fallback behavior.
- You changed appIcon() from const to non-const; consider restoring const-correctness or documenting why mutation on a const object is acceptable.
- Add error logging for cases where the temporary file cannot be created or the image cannot be saved to assist in troubleshooting failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of returning an empty string when image decoding fails, return the default application icon name to preserve the previous fallback behavior.
- You changed appIcon() from const to non-const; consider restoring const-correctness or documenting why mutation on a const object is acceptable.
- Add error logging for cases where the temporary file cannot be created or the image cannot be saved to assist in troubleshooting failures.
## Individual Comments
### Comment 1
<location> `panels/notification/bubble/bubbleitem.cpp:177` </location>
<code_context>
setEntity(entity);
}
+BubbleItem::~BubbleItem()
+{
+ // clean up temporary icon file.
</code_context>
<issue_to_address>
Destructor cleanup may not handle all edge cases for temporary file removal.
Currently, only the most recent temp file is removed. If multiple temp files can be created, consider tracking all of them or restricting creation to a single file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (!m_tempIconPath.isEmpty() && QFile::exists(m_tempIconPath)) { | ||
| QFile::remove(m_tempIconPath); | ||
| } | ||
| } |
There was a problem hiding this comment.
issue: Destructor cleanup may not handle all edge cases for temporary file removal.
Currently, only the most recent temp file is removed. If multiple temp files can be created, consider tracking all of them or restricting creation to a single file.
yixinshark
force-pushed
the
fix-enhanceTempIcon
branch
from
bc7b1c7 to
5e04c51
Compare
as title Log: as title
yixinshark
force-pushed
the
fix-enhanceTempIcon
branch
from
5e04c51 to
0047a56
Compare
deepin pr auto review关键摘要:
是否建议立即修改: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
as title
Log: as title
Summary by Sourcery
Enhance notification icon handling by generating unique temporary icon files in the system temp directory, caching and cleaning them up in the BubbleItem lifecycle, and delegating fallback to the UI when no icon is available
Enhancements: