refactor: optimize expired notification cleanup implementation #1314
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,
wjyrich and
yixinshark
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider emitting notificationClosed signals or an explicit update notification after direct deletes so clients can react to timeouts just like before.
- Double-check that QList::removeIf is available on your target Qt/C++ version or switch to a portable loop-and-erase implementation.
- You may still end up with stale records if the center is never opened—consider running cleanup on startup or at another strategic point.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider emitting notificationClosed signals or an explicit update notification after direct deletes so clients can react to timeouts just like before.
- Double-check that QList::removeIf is available on your target Qt/C++ version or switch to a portable loop-and-erase implementation.
- You may still end up with stale records if the center is never opened—consider running cleanup on startup or at another strategic point.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Reviewer's GuideThis refactor replaces the previous fetch-and-close expired notifications workflow with a direct deletion API in both database and memory layers, removes the periodic cleanup timer in favor of an on-demand trigger when the notification center is shown, and wires up a new D-Bus slot to invoke this more efficient cleanup. Sequence diagram for on-demand expired notification cleanup when notification center is shownsequenceDiagram
actor User
participant NotificationCenterPanel
participant DAppletBridge
participant NotifyServerApplet
participant NotificationManager
participant DataAccessor (DB/Memory)
User->>NotificationCenterPanel: setVisible(true)
NotificationCenterPanel->>DAppletBridge: bridge.applet().removeExpiredNotifications()
DAppletBridge->>NotifyServerApplet: removeExpiredNotifications()
NotifyServerApplet->>NotificationManager: removeExpiredNotifications()
NotificationManager->>DataAccessor (DB/Memory): removeEntitiesByExpiredTime(cutoffTime)
DataAccessor (DB/Memory)-->>NotificationManager: expired notifications deleted
NotificationManager-->>NotifyServerApplet: cleanup complete
NotifyServerApplet-->>DAppletBridge: cleanup complete
DAppletBridge-->>NotificationCenterPanel: cleanup complete
Class diagram for updated notification cleanup APIclassDiagram
class DataAccessor {
+removeEntitiesByExpiredTime(qint64 expiredTime)
}
class DBAccessor {
+removeEntitiesByExpiredTime(qint64 expiredTime)
}
class MemoryAccessor {
+removeEntitiesByExpiredTime(qint64 expiredTime)
}
class NotificationManager {
+removeExpiredNotifications()
}
class NotifyServerApplet {
+removeExpiredNotifications()
}
DataAccessor <|-- DBAccessor
DataAccessor <|-- MemoryAccessor
NotificationManager <.. NotifyServerApplet : used by
DBAccessor <.. NotificationManager : persistence
MemoryAccessor <.. NotificationManager : persistence
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
18202781743
force-pushed
the
master
branch
3 times, most recently
from
e85baee to
d8c62f2
Compare
deepin pr auto review我来对这个代码变更进行审查：
// 在 NotificationCenterPanel::setVisible 中添加错误处理
if (m_visible) {
qDebug(notifyLog) << "Try to remove expired notifications.";
DAppletBridge bridge("org.deepin.ds.notificationserver");
if (auto applet = bridge.applet()) {
if (!QMetaObject::invokeMethod(applet, "removeExpiredNotifications", Qt::DirectConnection)) {
qWarning(notifyLog) << "Failed to invoke removeExpiredNotifications";
}
} else {
qWarning(notifyLog) << "Failed to get notification server applet";
}
}
总体来说，这次变更是一个很好的优化，将定时清理改为按需清理更符合实际需求，代码结构也更加清晰。只需要在错误处理和日志记录方面做一些小的改进。 |
mhduiy
approved these changes
Oct 24, 2025
wjyrich
approved these changes
Oct 27, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, mhduiy, wjyrich 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 |
Changed the expired notification cleanup mechanism from fetching and processing expired entities to directly deleting them from storage. The previous implementation used fetchExpiredEntities to get expired notifications and then closed them individually, which was inefficient for large datasets. The new approach adds removeEntitiesByExpiredTime method that directly deletes expired records from database and memory storage, improving performance by eliminating unnecessary data retrieval and processing. Added trigger to automatically clean expired notifications when notification center becomes visible, ensuring users always see current notifications without expired ones cluttering the interface. This on- demand cleanup approach is more efficient than the previous timer-based periodic cleanup. Log: Optimized notification cleanup to remove expired notifications when opening notification center Influence: 1. Test opening notification center to verify expired notifications are automatically removed 2. Verify notification count displays correctly after cleanup 3. Test notification persistence for non-expired notifications 4. Check database storage efficiency after implementing direct deletion 5. Verify memory usage improvement with the optimized cleanup approach 6. Test notification center performance with large number of notifications refactor: 优化过期通知清理实现 将过期通知清理机制从获取和处理过期实体改为直接从存储中删除。之前的实现使 用 fetchExpiredEntities 获取过期通知然后逐个关闭，对于大数据集效率低下。 新方法添加了 removeEntitiesByExpiredTime 方法，直接从数据库和内存存储中 删除过期记录，通过消除不必要的数据检索和处理来提高性能。 添加了在通知中心变为可见时自动清理过期通知的触发器，确保用户始终看到当前 通知而不会被过期通知干扰界面。这种按需清理方法比之前的基于定时器的定期清 理更高效。 Log: 优化通知清理功能，在打开通知中心时自动移除过期通知 Influence: 1. 测试打开通知中心验证过期通知是否自动移除 2. 验证清理后通知计数显示是否正确 3. 测试非过期通知的持久化存储 4. 检查实现直接删除后的数据库存储效率 5. 验证优化清理方法后的内存使用改进 6. 测试通知中心在大量通知情况下的性能表现
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
4 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.
Changed the expired notification cleanup mechanism from fetching and
processing expired entities to directly deleting them from storage.
The previous implementation used fetchExpiredEntities to get expired
notifications and then closed them individually, which was inefficient
for large datasets. The new approach adds removeEntitiesByExpiredTime
method that directly deletes expired records from database and memory
storage, improving performance by eliminating unnecessary data retrieval
and processing.
Added trigger to automatically clean expired notifications when
notification center becomes visible, ensuring users always see current
notifications without expired ones cluttering the interface. This on-
demand cleanup approach is more efficient than the previous timer-based
periodic cleanup.
Log: Optimized notification cleanup to remove expired notifications when
opening notification center
Influence:
automatically removed
notifications
refactor: 优化过期通知清理实现
将过期通知清理机制从获取和处理过期实体改为直接从存储中删除。之前的实现使
用 fetchExpiredEntities 获取过期通知然后逐个关闭，对于大数据集效率低下。
新方法添加了 removeEntitiesByExpiredTime 方法，直接从数据库和内存存储中
删除过期记录，通过消除不必要的数据检索和处理来提高性能。
添加了在通知中心变为可见时自动清理过期通知的触发器，确保用户始终看到当前
通知而不会被过期通知干扰界面。这种按需清理方法比之前的基于定时器的定期清
理更高效。
Log: 优化通知清理功能，在打开通知中心时自动移除过期通知
Influence:
Summary by Sourcery
Refactor the expired notification cleanup to directly delete outdated records and invoke cleanup on demand when opening the notification center, improving performance and eliminating unnecessary data fetching.
Enhancements: