fix: start notification expire timer only after bubble is displayed - #1691
fix: start notification expire timer only after bubble is displayed#1691Ivy233 wants to merge 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ivy233 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 defers starting the notification expiration timer until a bubble is actually inserted into the UI model, wiring a new bubbleDisplayed signal through BubblePanel to NotificationManager, which now schedules timeouts based on the stored client expire timeout only when notifications are displayed, with thread-safe forwarding from the applet and added unit tests. Sequence diagram for deferred notification timeout start when bubble is displayedsequenceDiagram
participant BubbleModel
participant BubblePanel
participant NotifyServerApplet
participant NotificationManager
BubbleModel->>BubbleModel: insertBubble / replaceBubble
BubbleModel-->>BubblePanel: bubbleDisplayed(id)
BubblePanel->>NotifyServerApplet: notificationDisplayed(id)
NotifyServerApplet->>NotificationManager: notificationDisplayed(id)
NotificationManager->>NotificationManager: fetchEntity(id)
NotificationManager->>NotificationManager: pushPendingEntity(entity, entity.timeout())
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}
430dd63 to
fcf70d8
Compare
| if (interval <= 0) | ||
| return; | ||
|
|
||
| auto *timer = new QTimer(this); |
| timer->setSingleShot(true); | ||
| timer->setInterval(interval); | ||
| connect(timer, &QTimer::timeout, this, [this, id, bubbleId = bubble->bubbleId()] { | ||
| m_timeoutTimers.remove(id); |
| } | ||
| } | ||
|
|
||
| void BubbleModel::setBlockedId(qint64 id) |
There was a problem hiding this comment.
这样的话，进入了暂存区域的通知没有超时的机制了，
There was a problem hiding this comment.
已解决。暂存区模型复用同一个 ExpireTimer 单例，NotifyStagingModel::push 时同样调用 ExpireTimer::push 按实体超时启动倒计时，暂存区通知到期后照常超时并移出暂存区。
0b0eb8c to
2ae625f
Compare
2ae625f to
5ef467e
Compare
|
|
||
| Q_EMIT NotificationStateChanged(entity.id(), entity.processedType()); | ||
|
|
||
| bool critical = false; |
There was a problem hiding this comment.
服务端定时器逻辑整体移除后，超时计算完全由前端 ExpireTimer 负责，ExpireTimer 内部通过 NotifyEntity::timeout()/urgency() 自行推导有效超时时间（-1 默认 5000ms，0/Critical 永不过期），因此无需再把 expireTimeout 参数单独传给定时器。
|
|
||
| bool contains(qint64 key) const; | ||
| // Milliseconds left for key, or 0 when it is not tracked. | ||
| int remaining(qint64 key) const; |
There was a problem hiding this comment.
按操作来定义接口吧，不用搞这么通用的，然后让调用者去组合，
这里只有start，stop，clear吧，resume类似传递需要停止的entity，逻辑在内部封装，
| * that key. All bookkeeping lives in hash maps, so no QTimer is allocated per | ||
| * key and nothing leaks when a key expires or is stopped. | ||
| */ | ||
| class ExpireTimer : public QObject |
There was a problem hiding this comment.
暂存区和通知横幅的是不是共用同一个定时器管理的呀，不然这里会不会一个通知有两个定时器在弄呀，
There was a problem hiding this comment.
是共用的。ExpireTimer 是进程级单例，仅一个共享 QTimer，气泡与暂存区都通过它管理。同一通知 id 同时在气泡和暂存区显示时，ExpireTimer::push 发现 id 已在跟踪中会保持原截止时间而不重建倒计时，不会出现一个通知被两个定时器同时计时的现象。
5ef467e to
aa5a30c
Compare
aa5a30c to
fadfda6
Compare
| return &expireTimer; | ||
| } | ||
|
|
||
| void ExpireTimer::start(qint64 key, const NotifyEntity &entity) |
There was a problem hiding this comment.
这里里的key就是entity的id吧，直接从entity里获取就行了，这个key名称换成id吧，
| if (id == m_blockedId) | ||
| return; | ||
|
|
||
| if (m_blockedId != NotifyEntity::InvalidId) |
There was a problem hiding this comment.
这个hover就blocked的逻辑可以放在ExpirreTimer里实现吧，不暴露resume和pause了，
There was a problem hiding this comment.
已经把block逻辑迁移到expiretimer里面，并替代掉resume和pause。
| // A bubble that has been pushed off the display (overflow) is no longer | ||
| // in the model, but its countdown still finishes and the notification | ||
| // should still be closed, so fall back to the persisted bubble id. | ||
| Q_EMIT bubbleExpired(id, DataAccessorProxy::instance()->fetchEntity(id).bubbleId()); |
There was a problem hiding this comment.
既然bubbleExpired需要bubbleId，那在ExpireTimer::expired里就直接添加这个参数就行了，
There was a problem hiding this comment.
已按建议处理。ExpireTimer::expired 的签名就是 expired(qint64 id, uint bubbleId)，到期时从实体读取 bubbleId 随信号一并发出，BubbleModel::bubbleExpired(id, bubbleId) 直接转发，无需自行拼接。
| if (it == m_deadlines.cend()) | ||
| return; | ||
|
|
||
| m_paused.insert(key, static_cast<int>(qMax<qint64>(0, it.value() - QDateTime::currentMSecsSinceEpoch()))); |
There was a problem hiding this comment.
pause的只会是一个吧，最起码目前可以只是一个，
| if (m_deadlines.contains(key) || m_paused.contains(key)) | ||
| return; | ||
|
|
||
| m_deadlines.insert(key, QDateTime::currentMSecsSinceEpoch() + interval); |
There was a problem hiding this comment.
这个还是解决不了在通知横幅定时器跑了一半的时候，再切到暂存区定时器又重新计时的问题呀，
There was a problem hiding this comment.
已解决。ExpireTimer::remove 在气泡移出模型时挂起该 id 的倒计时（把绝对截止时间记入 m_retired），随后暂存区 NotifyStagingModel::push 调用 ExpireTimer::push 时从 m_retired 恢复原截止时间而不是重新计时；若暂停期间已过截止点，恢复时会按当前时间立即到期，不再从横幅跑了一半的地方重头开始。
| return; | ||
|
|
||
| // Critical notifications must not disappear on their own. | ||
| if (reason == NotifyEntity::Expired && entity.urgency() == NotifyEntity::Critical) |
There was a problem hiding this comment.
Expired的不会是Critical类型的吧，不需要这个判断吧，
fadfda6 to
e12655f
Compare
| if (id == m_blockedId) | ||
| return; | ||
|
|
||
| m_blockedId = id; |
There was a problem hiding this comment.
BubbleModel里不需要m_blockedId了吧，
There was a problem hiding this comment.
已删除。悬停冻结逻辑整体下沉到 ExpireTimer::setBlockId（内部维护单一悬停 id），BubblePanel::setHoveredId 直接调用 ExpireTimer::instance()->setBlockId(id)，BubbleModel 不再保存 m_blockedId。
| // times out, close it and notify the server so it moves the notification | ||
| // from the in-memory store to the center database and emits the signals. | ||
| connect(m_bubbles, &BubbleModel::bubbleExpired, this, [this](qint64 id, uint bubbleId) { | ||
| closeBubble(id); |
There was a problem hiding this comment.
不需要这里close吧，它会被server发送过来的吧，
There was a problem hiding this comment.
已去掉。到期时 BubblePanel 只向服务端发送 notificationClosed(id, bubbleId, Expired)，服务端处理后发出 NotificationStateChanged，气泡经由 onNotificationStateChanged 的正常流程关闭，不再本地 closeBubble。
| const auto replaceIndex = replaceBubbleIndex(bubble); | ||
| const auto oldBubble = m_bubbles[replaceIndex]; | ||
|
|
||
| ExpireTimer::instance()->stop(oldBubble->id()); |
There was a problem hiding this comment.
这个替换的逻辑，放在ExpireTimer里吧，只需要跟insert一样，push进去就行，
There was a problem hiding this comment.
已迁移。replaceBubble 现在与 insertBubble 一样只调用 ExpireTimer::push；替换逻辑（取消同一气泡槽位旧通知的倒计时、悬停块转移给新通知）封装在 push 内部的 cancelReplacement 中统一处理。
| // Absolute deadlines of ids stopped by stop(); restored by start() so a | ||
| // context switch (bubble <-> staging) resumes the countdown. | ||
| QHash<qint64, qint64> m_retired; | ||
| QHash<qint64, uint> m_bubbleIds; |
There was a problem hiding this comment.
不需要m_bubbleIds和m_deadlines两个吧，放一个QHash<qint64, NotifyEntity>是不是就可以了，
There was a problem hiding this comment.
已合并。m_bubbleIds 已删除，m_deadlines 改为 QHash<qint64, Deadline>，Deadline 同时保存实体与绝对截止时间（point），到期时所需的 bubbleId 直接从实体读取。
The notification expire timer previously ran in the notification server (worker thread) and was started when the notification was received, so a notification waiting in a long display queue could expire before its bubble was shown. The timeout is now owned by the bubble frontend and only starts once the bubble is actually displayed. 1. Add an ExpireTimer process-wide singleton that tracks each notification id's deadline with one shared single-shot QTimer. It exposes operation-based APIs (push/remove/setBlockId) instead of lower-level get/set ones and derives the effective timeout from the entity internally, so no QTimer is allocated per notification and nothing leaks on expiry. remove() suspends a countdown by remembering its absolute deadline and push() resumes it, so a notification moving between the bubble and the staging area never restarts its countdown. push() also handles replacement internally: a replace notification occupies the same bubble slot, so the old countdown is cancelled there before the new one starts, and a hovered replaced bubble keeps its block on the new id. 2. BubbleModel starts the expire timer when a bubble is shown (insert/replace) using the client expireTimeout (0: never expire, -1: 5000ms default, Critical urgency: never expire). On expiry it emits bubbleExpired(id, bubbleId). The hovered bubble is frozen by ExpireTimer::setBlockId (a single hover block, 1s grace after unhover), so BubbleModel no longer keeps its own blocked-id state. 3. On expiry BubblePanel notifies the server with notificationClosed(id, bubbleId, Expired); the server then emits NotificationStateChanged which closes the bubble through the normal flow, so no local close is needed. 4. The notification center staging model reuses the same timer so notifications shown in the staging area also time out. 5. Make NotificationManager::notificationClosed idempotent: a notification is tracked by both the bubble and the staging expire timers, so the close is reported only while the entity still exists, preventing a notification from being closed twice. Critical notifications never expire on their own, enforced both when computing the frontend timeout and on the server. 6. Remove the server-side timeout bookkeeping: setBlockClosedId, pushPendingEntity, onHandingPendingEntities, removePendingEntity and the pending-timeout QTimer. 7. Expose BubbleItem::entity() and NotifyEntity::timeout()/urgency() for the frontend timeout computation. 8. Remove the obsolete SetBlockClosedId tests. Log: Fixed notifications expiring before their bubble was displayed. Influence: 1. Send several notifications at once and verify each bubble stays for the full expire timeout. 2. Verify critical notifications and expireTimeout 0 never close. 3. Hover a bubble and verify it does not expire, then closes 1s after unhover. 4. Verify an expired notification is closed only once and moved to the notification center, and the DBus NotificationClosed(Expired) signal is emitted once. 5. Open the notification center and verify staged notifications expire after their timeout and leave the staging area. 6. Run the notification server unit tests. fix: 将通知过期计时迁移到气泡前端 通知过期计时此前在通知服务器（工作线程）中运行，收到通知时即启动，导致 长显示队列中的通知可能在显示前就已过期。现将超时逻辑交由气泡前端持有， 气泡真正显示后才开始计时。 1. 新增 ExpireTimer 进程级单例：用一个共享的单次 QTimer 管理每个通知 id 的 过期时间，提供 push/remove/setBlockId 操作化接口，并由实体内部计算有效 超时时间，不再为每个通知分配 QTimer，到期后也不会泄漏对象。remove() 通过 记住绝对截止时间挂起倒计时，push() 恢复它，因此通知在气泡与暂存区之间 切换时不会重新计时。push() 同时内部处理替换：替换通知占用同一气泡槽位， 开始新倒计时前先取消旧槽位的倒计时，且被替换气泡处于悬停时，悬停块会 转移到新通知。 2. BubbleModel 在气泡显示（插入/替换）时根据客户端 expireTimeout 启动过期 计时（0：永不过期，-1：默认 5000ms，Critical 优先级：永不过期），到期时 发出 bubbleExpired(id, bubbleId)。悬停气泡由 ExpireTimer::setBlockId 冻结 （仅一个悬停块，取消悬停后保留 1s 缓冲），BubbleModel 不再维护自己的 悬停 id 状态。 3. 到期后 BubblePanel 调用服务器 notificationClosed(id, bubbleId, Expired)； 服务器再发出 NotificationStateChanged，气泡经正常流程关闭，无需本地关闭。 4. 通知中心暂存模型复用同一计时器，暂存区展示的通知同样会超时。 5. 使 NotificationManager::notificationClosed 幂等：同一通知会被气泡与暂存 模型的过期计时同时跟踪，因此在实体仍存在时才上报关闭，避免通知被关闭 两次。Critical 通知永不自动过期，前端超时计算与服务端均强制执行。 6. 移除服务端超时簿记：setBlockClosedId、pushPendingEntity、 onHandingPendingEntities、removePendingEntity 以及 pending-timeout 定时器。 7. 为前端超时计算暴露 BubbleItem::entity() 与 NotifyEntity::timeout()/ urgency()。 8. 移除过时的 SetBlockClosedId 测试。 Log: 修复通知在气泡显示前就过期的问题。 Influence: 1. 一次性发送多条通知，验证每个气泡都能保持完整的过期时间。 2. 验证 Critical 通知与 expireTimeout 为 0 的通知永不过期。 3. 悬停气泡验证其不关闭，取消悬停 1s 后关闭。 4. 验证过期通知只被关闭一次并进入通知中心，DBus NotificationClosed(Expired) 信号只发送一次。 5. 打开通知中心，验证暂存区的通知到期后超时并移出暂存区。 6. 运行通知服务器单元测试。 PMS: BUG-372279
e12655f to
0c427f4
Compare
deepin pr auto review★ 总体评分：75分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 diff --git a/panels/notification/bubble/bubblemodel.h b/panels/notification/bubble/bubblemodel.h
index 78cc5b086..corrected_encapsulation.h 100644
--- a/panels/notification/bubble/bubblemodel.h
+++ b/panels/notification/bubble/bubblemodel.h
@@ -5,7 +5,6 @@
#include "bubbleitem.h"
#include "notifyentity.h"
-#include <QHash>
#include <QQueue>
class QTimer;
@@ -69,6 +68,7 @@ class BubbleModel : public QAbstractListModel
void updateBubbleTimeTip();
void updateContentRowCount(int rowCount);
+private:
QTimer *m_updateTimeTipTimer = nullptr;
QTimer *m_processPendingTimer = nullptr;
QList<BubbleItem *> m_bubbles; |
3 participants
pushPendingEntitycall fromNotify(), so the expire timer no longer starts as soon as a notification is receivedbubbleDisplayedsignal toBubbleModel, emitted when a bubble is actually inserted into the display modelbubbleDisplayedinBubblePaneland forward it to the notification server throughnotificationDisplayedtimeout()accessor toNotifyEntityto read the client provided expire timeoutNotificationManager::notificationDisplayedto schedule the timeout only when the bubble is shown, avoiding premature expiry while notifications are still queuednotificationDisplayedviaQt::QueuedConnectioninNotifyServerAppletso the pending timeout timer is started in the worker thread it belongs tonotificationDisplayedLog: Defer the notification expire timer until the bubble is actually displayed on screen
Influence:
fix: 通知气泡显示后才启动过期计时
Notify()中立即调用pushPendingEntity的逻辑，通知收到后不再 马上启动过期计时BubbleModel中新增bubbleDisplayed信号，在气泡实际插入显示模型 时发出BubblePanel中连接bubbleDisplayed，通过notificationDisplayed转发给通知服务端NotifyEntity新增timeout()访问器，用于读取客户端传入的过期时间NotificationManager::notificationDisplayed，仅在气泡显示时才调度 超时，避免通知在排队期间提前过期NotifyServerApplet中通过Qt::QueuedConnection转发notificationDisplayed，确保过期定时器在其所属的 worker 线程中启动notificationDisplayed补充单元测试Log: 将通知过期计时推迟到气泡真正显示之后
Influence:
PMS: BUG-372279
Summary by Sourcery
Defer starting notification expiry timers until bubbles are actually displayed on screen.
New Features:
Bug Fixes:
Enhancements:
Tests: