feat: add hover protection for notification bubbles#1502
Open
mhduiy wants to merge 1 commit intolinuxdeepin:masterfrom
Open
feat: add hover protection for notification bubbles#1502mhduiy wants to merge 1 commit intolinuxdeepin:masterfrom
mhduiy wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
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
|
deepin pr auto review这份代码修改主要实现了通知气泡在鼠标悬停时阻止自动关闭的功能，涉及C++后端和QML前端的交互。以下是对代码的详细审查和改进建议： 1. 语法与逻辑审查
2. 代码质量
3. 代码性能
4. 代码安全
总结修改建议
这些修改将提高代码的健壮性、可维护性和安全性。 |
![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.
Comment on lines
+209
to
+212
| void BubblePanel::setHoveredId(qint64 id) | ||
| { | ||
| QMetaObject::invokeMethod(m_notificationServer, "setBlockClosedId", Qt::DirectConnection, Q_ARG(qint64, id)); | ||
| } |
There was a problem hiding this comment.
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.
Suggested change
| void BubblePanel::setHoveredId(qint64 id) | |
| { | |
| QMetaObject::invokeMethod(m_notificationServer, "setBlockClosedId", Qt::DirectConnection, Q_ARG(qint64, id)); | |
| } | |
| 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)); | |
| } |
| return m_setting->systemValue(static_cast<NotificationSetting::SystemConfigItem>(configItem)); | ||
| } | ||
|
|
||
| void NotificationManager::setBlockClosedId(qint64 id) |
There was a problem hiding this comment.
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:
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:
// 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:
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();
}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.
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
2 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: