feat: add hover protection for notification bubbles#1502
Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom Mar 17, 2026
Merged
feat: add hover protection for notification bubbles#1502deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
Conversation
Reviewer's GuideImplements hover-based protection for notification bubbles by tracking the currently hovered bubble id from QML through the bubble panel and applet into the NotificationManager, which defers timeout-based closure for that specific notification entity while it is hovered, plus minor metadata updates. Sequence diagram for hover-based notification bubble protectionsequenceDiagram
actor User
participant BubbleQml as Bubble_QML
participant BubblePanel
participant NotifyServerApplet
participant NotificationManager
participant TimeoutHandler as Timeout_processing
User ->> BubbleQml: hover enter on bubble
BubbleQml ->> BubblePanel: setHoveredId(bubble.id)
BubblePanel ->> NotifyServerApplet: setBlockClosedId(id)
NotifyServerApplet ->> NotificationManager: setBlockClosedId(id)
NotificationManager ->> NotificationManager: update m_blockClosedId
loop Existing blocked bubble handling
TimeoutHandler ->> NotificationManager: setBlockClosedId(new_id)
NotificationManager ->> NotificationManager: find previous m_blockClosedId in m_pendingTimeoutEntities
alt previous blocked entity is pending and expired
NotificationManager ->> NotificationManager: reinsert previous entity with current+1000ms
end
NotificationManager ->> NotificationManager: m_blockClosedId = new_id
NotificationManager ->> NotificationManager: onHandingPendingEntities()
end
loop Periodic timeout processing
TimeoutHandler ->> NotificationManager: onHandingPendingEntities()
NotificationManager ->> NotificationManager: iterate m_pendingTimeoutEntities
alt entity.id == m_blockClosedId
NotificationManager ->> NotificationManager: reinsert entity with current+1000ms
else entity expired and not hovered
NotificationManager ->> NotificationManager: notificationClosed(entity.id, entity.bubbleId, Expired)
end
end
Updated class diagram for notification hover blockingclassDiagram
class BubblePanel {
+setEnabled(newEnabled bool) void
+setHoveredId(id qint64) void
+close(bubbleIndex int, reason int) void
+delayProcess(bubbleIndex int) void
}
class NotifyServerApplet {
+removeNotifications(appName QString) void
+removeNotifications() void
+removeExpiredNotifications() void
+setBlockClosedId(id qint64) void
-m_manager NotificationManager*
}
class NotificationManager {
+SetSystemInfo(configItem uint, value QVariant) void
+GetSystemInfo(configItem uint) QVariant
+setBlockClosedId(id qint64) void
-isDoNotDisturb() bool
-recordNotification(entity NotifyEntity) bool
-onHandingPendingEntities() void
-m_blockClosedId qint64
}
class NotifyEntity {
+id() qint64
+bubbleId() qint64
+appName() QString
<<enum>> InvalidId
}
BubblePanel --> NotifyServerApplet : calls_setBlockClosedId
NotifyServerApplet --> NotificationManager : forwards_setBlockClosedId
NotificationManager ..> NotifyEntity : manages_pending_timeout_entities
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 - I've found 2 issues, and left some high level feedback:
- In
NotificationManager::setBlockClosedId, thestd::find_iflambda compares againstm_blockClosedIdinstead of the incomingidparameter, which means it will search using the old value rather than the new one and likely not behave as intended when changing the hovered bubble. - The
std::find_ifpredicate insetBlockClosedIdtakes aconst NotifyEntity &argument, butm_pendingTimeoutEntitiesappears to be a keyed container (e.g.QMap<qint64, NotifyEntity>), so the lambda parameter should be adjusted (or replaced with a simple iterator loop) to match the actual element type and avoid compilation or runtime issues when accessingentity.id()andfindIter.key()/value().
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `NotificationManager::setBlockClosedId`, the `std::find_if` lambda compares against `m_blockClosedId` instead of the incoming `id` parameter, which means it will search using the old value rather than the new one and likely not behave as intended when changing the hovered bubble.
- The `std::find_if` predicate in `setBlockClosedId` takes a `const NotifyEntity &` argument, but `m_pendingTimeoutEntities` appears to be a keyed container (e.g. `QMap<qint64, NotifyEntity>`), so the lambda parameter should be adjusted (or replaced with a simple iterator loop) to match the actual element type and avoid compilation or runtime issues when accessing `entity.id()` and `findIter.key()/value()`.
## Individual Comments
### Comment 1
<location path="panels/notification/bubble/bubblepanel.cpp" line_range="209-212" />
<code_context>
setVisible(!isEmpty && enabled());
}
+void BubblePanel::setHoveredId(qint64 id)
+{
+ QMetaObject::invokeMethod(m_notificationServer, "setBlockClosedId", Qt::DirectConnection, Q_ARG(qint64, id));
+}
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `Qt::DirectConnection` here could be unsafe if `m_notificationServer` lives in another thread.
`QMetaObject::invokeMethod` with `Qt::DirectConnection` will run `setBlockClosedId` in the caller’s thread. If `BubblePanel` and `m_notificationServer` ever end up in different threads, this will break Qt’s threading rules and can cause race conditions.
If cross-thread calls are possible (now or later), prefer the default connection type or `Qt::QueuedConnection`. If they are guaranteed to share a thread, either call the slot directly or add a short comment documenting that assumption to avoid future misuse.
```suggestion
void BubblePanel::setHoveredId(qint64 id)
{
// Use a queued connection to be safe if BubblePanel and m_notificationServer live in different threads.
// This avoids violating Qt's threading rules if this ever becomes a cross-thread call.
QMetaObject::invokeMethod(
m_notificationServer,
"setBlockClosedId",
Qt::QueuedConnection,
Q_ARG(qint64, id));
}
```
</issue_to_address>
### Comment 2
<location path="panels/notification/server/notificationmanager.cpp" line_range="350" />
<code_context>
return m_setting->systemValue(static_cast<NotificationSetting::SystemConfigItem>(configItem));
}
+void NotificationManager::setBlockClosedId(qint64 id)
+{
+ auto findIter = std::find_if(m_pendingTimeoutEntities.begin(), m_pendingTimeoutEntities.end(), [this](const NotifyEntity &entity) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider clarifying which id is used in the timeout search and extracting the common 1000ms postponement logic into a helper to make the behavior more explicit and maintainable.
You can keep the feature as‑is but reduce the cognitive load a bit by (1) avoiding hidden state in the `find_if` lambda and (2) centralising the “postpone by N ms” logic.
### 1. Make it clear which id `find_if` is using
Right now the lambda closes over `this` and uses `m_blockClosedId` while the function takes a new `id` parameter. This is exactly the “which id is this using?” confusion the other reviewer mentioned.
You can make the intent explicit by capturing the previous id in a local and capturing that:
```cpp
void NotificationManager::setBlockClosedId(qint64 id)
{
const auto previousBlockedId = m_blockClosedId;
const auto current = QDateTime::currentMSecsSinceEpoch();
auto findIter = std::find_if(m_pendingTimeoutEntities.begin(),
m_pendingTimeoutEntities.end(),
[previousBlockedId](const NotifyEntity &entity) {
return entity.id() == previousBlockedId;
});
if (previousBlockedId != NotifyEntity::InvalidId
&& findIter != m_pendingTimeoutEntities.end()
&& current > findIter.key()) {
qDebug(notifyLog) << "Block close bubble id:" << previousBlockedId
<< "for the new block bubble id:" << id;
m_pendingTimeoutEntities.insert(current + 1000, findIter.value());
m_pendingTimeoutEntities.erase(findIter);
}
m_blockClosedId = id;
onHandingPendingEntities();
}
```
This keeps all behaviour intact but removes the implicit dependency on mutable member state inside the lambda.
### 2. Centralise the “postpone by 1000ms” behaviour
You have the same “insert with `+ 1000`” logic in two places. A tiny helper (or local lambda) will make the intent obvious and avoid subtle divergence later:
```cpp
// Member helper (header + cpp) if you prefer:
void NotificationManager::postponeTimeout(const NotifyEntity &entity,
qint64 baseTimeMs,
qint64 delayMs)
{
m_pendingTimeoutEntities.insert(baseTimeMs + delayMs, entity);
}
```
Use it in both call sites:
```cpp
void NotificationManager::setBlockClosedId(qint64 id)
{
const auto previousBlockedId = m_blockClosedId;
const auto current = QDateTime::currentMSecsSinceEpoch();
auto findIter = std::find_if(m_pendingTimeoutEntities.begin(),
m_pendingTimeoutEntities.end(),
[previousBlockedId](const NotifyEntity &entity) {
return entity.id() == previousBlockedId;
});
if (previousBlockedId != NotifyEntity::InvalidId
&& findIter != m_pendingTimeoutEntities.end()
&& current > findIter.key()) {
qDebug(notifyLog) << "Block close bubble id:" << previousBlockedId
<< "for the new block bubble id:" << id;
postponeTimeout(findIter.value(), current, 1000);
m_pendingTimeoutEntities.erase(findIter);
}
m_blockClosedId = id;
onHandingPendingEntities();
}
```
```cpp
for (const auto &item : timeoutEntities) {
if (!item.isValid()) {
// ...
continue;
}
if (item.id() == m_blockClosedId) {
qDebug(notifyLog) << "bubble id:" << item.bubbleId()
<< "entity id:" << item.id();
postponeTimeout(item, current, 1000);
continue;
}
qDebug(notifyLog) << "Expired for the notification " << item.id()
<< item.appName();
notificationClosed(item.id(), item.bubbleId(), NotifyEntity::Expired);
}
```
This keeps:
- the “reschedule previous blocked entity if already expired” behaviour, and
- the “keep postponing the currently blocked entity by 1s on each timeout pass”
but makes the postponement rule explicit and removes duplicated “`current + 1000`” logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
mhduiy
force-pushed
the
noti
branch
4 times, most recently
from
cfc8603 to
08bfe0e
Compare
18202781743
reviewed
Mar 17, 2026
18202781743
approved these changes
Mar 17, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, mhduiy 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 |
Added bubble ID tracking to prevent premature closure when hovering over notifications. The BubbleModel now exposes bubbleId role for QML access. When a bubble is hovered, its ID is passed to the notification server which blocks timeout-based closure for that specific bubble. This prevents notifications from disappearing while users are interacting with them. Log: Notification bubbles now remain visible when hovered over, preventing accidental closure feat: 为通知气泡添加悬停保护功能 在 BubbleModel 中添加了 bubbleId 角色供 QML 访问。当气泡被悬停时，其 ID 会传递给通知服务器，服务器会阻止该特定气泡的超时关闭。这防止了用户与通知 交互时通知意外消失的问题。 Log: 通知气泡在悬停时保持可见，防止意外关闭 PMS: BUG-352577
deepin pr auto review代码审查意见总体评价本次代码变更重构了通知气泡的延迟移除机制，将原本在 详细审查意见1. 语法和逻辑1.1 BubbleModel 类改进
1.2 BubblePanel 类改进
1.3 NotificationManager 类改进
2. QML 代码改进2.1 Bubble.qml 改进
2.2 main.qml 改进
3. 代码质量
4. 性能考虑
5. 安全性
总结这次重构整体上改进了代码结构和职责分离，使延迟关闭逻辑更加集中和清晰。主要的改进点包括：
但也有一些需要改进的地方，特别是在性能和边界条件处理方面。建议在合并前考虑上述改进意见，特别是关于性能优化和边界条件处理的部分。 |
Contributor
Author
|
/forcemerge |
deepin pr auto reviewGit Diff 代码审查报告概述本次代码变更主要重构了通知气泡的延迟关闭机制，将原来在 语法逻辑分析
代码质量评估
代码性能建议
代码安全建议
其他建议
总结本次代码变更将延迟关闭逻辑从 |
|
This pr force merged! (status: blocked) |
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
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.
Added bubble ID tracking to prevent premature closure when hovering over notifications. The BubbleModel now exposes bubbleId role for QML access. When a bubble is hovered, its ID is passed to the notification server which blocks timeout-based closure for that specific bubble. This prevents notifications from disappearing while users are interacting with them.
Log: Notification bubbles now remain visible when hovered over, preventing accidental closure
feat: 为通知气泡添加悬停保护功能
在 BubbleModel 中添加了 bubbleId 角色供 QML 访问。当气泡被悬停时，其 ID 会传递给通知服务器，服务器会阻止该特定气泡的超时关闭。这防止了用户与通知
交互时通知意外消失的问题。
Log: 通知气泡在悬停时保持可见，防止意外关闭
PMS: BUG-352577
Summary by Sourcery
Add hover-based protection to notification bubbles to prevent them from closing while the user is interacting with them.
New Features:
Enhancements:
Chores: