feat: enhance notification validation and cleanup #1184
Merged
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
18202781743
requested review from
BLumia,
fly602,
mhduiy,
robertkill and
yixinshark
deepin pr auto review代码审查意见：
总体来说，这些改进提高了代码的健壮性和可维护性。但是，需要确保所有相关的代码都已经更新以处理新的成员变量和函数。 |
Reviewer's GuideThis PR strengthens the notification pipeline by introducing validity checks and cleanup routines: it adds entity validity methods, prevents invalid bubbles from being created or closed, purges stale items automatically, enhances timestamp-based validation in entities, and injects logging to trace invalid cases. Sequence diagram for adding a bubble with entity validationsequenceDiagram
participant User as actor User
participant BubblePanel
participant m_accessor as EntityAccessor
participant BubbleItem
participant BubbleModel
User->>BubblePanel: addBubble(id)
BubblePanel->>m_accessor: fetchEntity(id)
m_accessor-->>BubblePanel: entity
alt entity is invalid
BubblePanel-->>User: Log warning, abort
else entity is valid
BubblePanel->>BubbleItem: BubbleItem(entity)
BubblePanel->>BubbleModel: add BubbleItem
end
Sequence diagram for clearing invalid bubblessequenceDiagram
participant BubblePanel
participant BubbleModel
participant BubbleItem
BubblePanel->>BubbleModel: clearInvalidBubbles()
loop for each BubbleItem in m_bubbles
BubbleModel->>BubbleItem: isValid()
alt not valid
BubbleModel->>BubbleModel: remove(bubble)
end
end
Sequence diagram for notification timeout processing with validationsequenceDiagram
participant NotificationManager
participant NotifyEntity
participant Logger
NotificationManager->>NotifyEntity: isValid()
alt entity is invalid
NotificationManager->>Logger: Log warning, skip
else entity is valid
NotificationManager->>NotificationManager: notificationClosed(...)
end
Class diagram for enhanced notification validation and cleanupclassDiagram
class NotifyEntity {
+qint64 id
+QString appName
+qint64 cTime
+bool isValid() const
}
class BubbleItem {
+NotifyEntity m_entity
+bool isValid() const
}
class BubbleModel {
+QList<BubbleItem*> m_bubbles
+void clearInvalidBubbles()
}
class BubblePanel {
+void addBubble(qint64 id)
+void closeBubble(qint64 id)
+void clearInvalidBubbles()
}
class NotificationManager {
+void onHandingPendingEntities()
}
BubblePanel --> BubbleModel : uses
BubbleModel --> BubbleItem : contains
BubbleItem --> NotifyEntity : has
NotificationManager --> NotifyEntity : processes
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 @18202781743 - I've reviewed your changes - here's some feedback:
- NotifyEntity only sets cTime in one constructor—add timestamp initialization to all constructors to avoid unintentionally invalidating new entities.
- The reverse loop in BubbleModel::clearInvalidBubbles has a redundant boundary check and may skip items after remove; consider simplifying with a filtered removal (e.g. using std::remove_if or a two-pass approach).
- Entity validity checks are scattered across BubblePanel and NotificationManager—extract and centralize validation logic into a shared helper to avoid duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- NotifyEntity only sets cTime in one constructor—add timestamp initialization to all constructors to avoid unintentionally invalidating new entities.
- The reverse loop in BubbleModel::clearInvalidBubbles has a redundant boundary check and may skip items after remove; consider simplifying with a filtered removal (e.g. using std::remove_if or a two-pass approach).
- Entity validity checks are scattered across BubblePanel and NotificationManager—extract and centralize validation logic into a shared helper to avoid duplication.
## Individual Comments
### Comment 1
<location> `panels/notification/bubble/bubblepanel.cpp:122` </location>
<code_context>
+
+ // Validate entity before creating bubble to prevent invalid notification banners
+ if (!entity.isValid()) {
+ qWarning(notifyLog) << "Failed to add bubble: invalid entity for id" << id << "appName:" << entity.appName() << "cTime:" << entity.cTime();
+ return;
+ }
</code_context>
<issue_to_address>
Logging entity.appName() and entity.cTime() for invalid entities may not always be safe.
Accessing properties of an invalid entity may yield incorrect log data. Consider logging only the id or adding checks before logging other properties.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (!entity.isValid()) {
qWarning(notifyLog) << "Failed to add bubble: invalid entity for id" << id << "appName:" << entity.appName() << "cTime:" << entity.cTime();
return;
}
=======
if (!entity.isValid()) {
qWarning(notifyLog) << "Failed to add bubble: invalid entity for id" << id;
return;
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+121
to
+124
| if (!entity.isValid()) { | ||
| qWarning(notifyLog) << "Failed to add bubble: invalid entity for id" << id << "appName:" << entity.appName() << "cTime:" << entity.cTime(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Logging entity.appName() and entity.cTime() for invalid entities may not always be safe.
Accessing properties of an invalid entity may yield incorrect log data. Consider logging only the id or adding checks before logging other properties.
Suggested change
| if (!entity.isValid()) { | |
| qWarning(notifyLog) << "Failed to add bubble: invalid entity for id" << id << "appName:" << entity.appName() << "cTime:" << entity.cTime(); | |
| return; | |
| } | |
| if (!entity.isValid()) { | |
| qWarning(notifyLog) << "Failed to add bubble: invalid entity for id" << id; | |
| return; | |
| } |
yixinshark
previously approved these changes
Jul 10, 2025
1. Added isValid() method to BubbleItem to check entity validity 2. Implemented clearInvalidBubbles() in BubbleModel to remove invalid notifications 3. Added entity validation before creating new bubbles in BubblePanel 4. Enhanced NotifyEntity validation to include timestamp check 5. Added validation in NotificationManager timeout processing 6. Added debug logging for invalid notification cases These changes improve notification system reliability by: - Preventing invalid notifications from being displayed - Cleaning up stale notifications automatically - Adding better error handling and logging - Preventing race conditions in timeout processing - Ensuring only valid entities are processed feat: 增强通知验证和清理功能 1. 在 BubbleItem 中添加 isValid() 方法检查实体有效性 2. 在 BubbleModel 中实现 clearInvalidBubbles() 清理无效通知 3. 在 BubblePanel 创建新气泡前添加实体验证 4. 增强 NotifyEntity 验证包含时间戳检查 5. 在 NotificationManager 超时处理中添加验证 6. 为无效通知情况添加调试日志 这些改进通过以下方式提升通知系统可靠性： - 防止显示无效通知 - 自动清理陈旧通知 - 添加更好的错误处理和日志记录 - 防止超时处理中的竞态条件 - 确保只处理有效实体 Issue: https://bbs.deepin.org/post/289141
yixinshark
approved these changes
Jul 10, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, 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 |
Contributor
Author
|
/forcemerge |
|
This pr force merged! (status: blocked) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
notifications
These changes improve notification system reliability by:
feat: 增强通知验证和清理功能
这些改进通过以下方式提升通知系统可靠性：
Issue: https://bbs.deepin.org/post/289141
Summary by Sourcery
Enhance notification validation and cleanup to prevent invalid or stale notifications and improve logging
Enhancements: