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 直接从实体读取。
e12655f to
0c427f4
Compare
| connect(NotifyAccessor::instance(), &NotifyAccessor::stagingEntityClosed, this, &NotifyStagingModel::onEntityClosed); | ||
| connect(NotifySetting::instance(), &NotifySetting::contentRowCountChanged, this, &NotifyStagingModel::updateContentRowCount); | ||
|
|
||
| connect(ExpireTimer::instance(), &ExpireTimer::expired, this, [this](qint64 id, uint bubbleId) { |
There was a problem hiding this comment.
一个通知发送了expired后，会不会被通知横幅和暂存区都close一次呀，
是不是可以直接由ExpireTimer调用server的close呀，
There was a problem hiding this comment.
已处理。
-
关于"会不会 close 两次"：确实存在这个竞态，已修复。之前暂存区在 ExpireTimer::expired 时本地 remove + 补位，而服务端关闭是异步投递到 worker 线程的，导致刚过期的通知在补位时仍被当作 NotProcessed 重新插入并重新计时，一个倒计时周期后再次触发 notificationClosed（被幂等检查挡住，但会多打一条日志、多一次空跑）。现在暂存区不再响应 expired，只在服务端 stagingEntityClosed 回环时移除行，此时实体已标记 Processed，不会被补位重新选中。
-
关于"由 ExpireTimer 直接调用 server 的 close"：采用等价且不破坏分层的做法——ExpireTimer 保持纯前端、不感知服务端，由 NotifyServerApplet 连接 ExpireTimer::expired，通过 Qt::QueuedConnection 统一转发给 NotificationManager::notificationClosed，作为过期关闭的唯一入口；notificationClosed 保留幂等检查兜底。横幅和暂存区各自只移除自己的视图，服务端每个通知只处理一次。
a26e115 to
a5757b6
Compare
1. Move the expire countdown from NotificationManager to the frontend ExpireTimer singleton, which tracks every displayed notification (bubble and staging area) with one shared QTimer and per-id absolute deadlines 2. Start the countdown in BubbleModel::insertBubble/replaceBubble and NotifyStagingModel::push/open, i.e. only when the bubble is actually shown, so queued notifications no longer expire before they are displayed 3. Let the bubble and the staging area share one countdown per id: push() of an already tracked id keeps the original deadline, and remove() suspends it via m_retired so switching between the two never restarts the countdown 4. Move hover handling into ExpireTimer::setBlockId, freezing the hovered bubble's countdown and resuming it with a short grace period afterwards 5. Make the server the single owner of the close on expiry: NotifyServerApplet forwards ExpireTimer::expired to NotificationManager::notificationClosed via Qt::QueuedConnection, whose idempotency check guards against duplicate closes 6. Add NotifyEntity::timeout()/urgency() accessors so ExpireTimer derives the effective timeout itself (0/Critical never expires, -1 defaults to 5000 ms) 7. Fix a race in NotifyStagingModel where a local remove+refill after expiry could re-insert the just-expired notification with a fresh countdown; the staging row is now dropped only via the server's stagingEntityClosed round trip 8. Remove the now-unused pending-timeout machinery and tests from NotificationManager and NotifyServerApplet Log: Defer the notification expire countdown until the bubble is displayed, and close expired notifications once through the server Influence: 1. Verify a notification disappears after the default 5 seconds when displayed normally 2. Verify a notification no longer expires before it is shown when many notifications are queued 3. Verify hovering over a bubble prevents it from expiring 4. Verify a notification shown in both the bubble and the staging area expires at its original deadline without restarting the countdown, and the server closes it only once 5. Run the notification server unit tests fix: 通知气泡显示后才开始过期计时 1. 将过期计时从 NotificationManager 迁移到前端 ExpireTimer 单例，用一个共享 QTimer 和每个 id 的绝对截止时间管理所有已显示通知（气泡与暂存区）的倒计时 2. 在 BubbleModel::insertBubble/replaceBubble 与 NotifyStagingModel::push/open 中启动倒计时，即气泡真正显示后才开始计时，排队中的通知不再提前过期 3. 气泡与暂存区对同一 id 共享一个倒计时：重复 push 保持原截止时间， remove() 通过 m_retired 挂起，切换显示位置不会重新计时 4. 悬停处理下沉到 ExpireTimer::setBlockId，冻结悬停气泡的倒计时，移开后 保留短暂宽限期再恢复 5. 由服务端统一负责过期关闭：NotifyServerApplet 通过 Qt::QueuedConnection 将 ExpireTimer::expired 转发给 NotificationManager::notificationClosed， 幂等检查防止重复关闭 6. 新增 NotifyEntity::timeout()/urgency() 访问器，由 ExpireTimer 自行推导 有效超时（0/Critical 永不过期，-1 默认 5000ms） 7. 修复 NotifyStagingModel 过期后本地 remove+补位可能把刚过期的通知重新插入 并重新计时的竞态，暂存区行改为仅通过服务端 stagingEntityClosed 回环移除 8. 移除 NotificationManager 与 NotifyServerApplet 中不再使用的挂起计时逻辑 及对应测试 Log: 将通知过期倒计时推迟到气泡显示之后，并由服务端统一关闭过期通知 Influence: 1. 验证正常显示的通知在默认 5 秒后消失 2. 验证大量通知排队时，通知不会在显示前提前过期 3. 验证鼠标悬停气泡时通知不会过期消失 4. 验证同时显示在气泡与暂存区的通知按原始截止时间过期、不重新计时， 且服务端只关闭一次 5. 运行通知服务端单元测试 PMS: BUG-372279
a5757b6 to
c7b598c
Compare
deepin pr auto review★ 总体评分：95分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 // 针对 ExpireTimer::schedule() 的轻微性能优化建议（使用优先队列替代线性查找）
// expiretimer.h
#include <queue>
#include <vector>
// 在 private 部分添加
struct DeadlineCompare {
bool operator()(const Deadline &lhs, const Deadline &rhs) const {
return lhs.point > rhs.point; // 最小堆
}
};
std::priority_queue<Deadline, std::vector<Deadline>, DeadlineCompare> m_deadlineQueue;
QHash<qint64, Deadline> m_deadlines; // 保留用于快速查找
// expiretimer.cpp
void ExpireTimer::schedule()
{
if (m_deadlines.isEmpty()) {
m_timer->stop();
return;
}
// 清理已过期的无效元素
while (!m_deadlineQueue.empty() && !m_deadlines.contains(m_deadlineQueue.top().entity.id())) {
m_deadlineQueue.pop();
}
if (m_deadlineQueue.empty()) {
m_timer->stop();
return;
}
const auto &top = m_deadlineQueue.top();
const qint64 remaining = qMax<qint64>(0, top.point - QDateTime::currentMSecsSinceEpoch());
m_timer->start(static_cast<int>(remaining));
} |
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: