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
|
[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
|
![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.
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. 语法逻辑问题： auto findIter = std::find_if(m_pendingTimeoutEntities.begin(), m_pendingTimeoutEntities.end(), [this](const NotifyEntity &entity) {
return entity.id() == m_blockClosedId;
});这里使用了 改进建议： // 修改后的查找逻辑
auto findIter = std::find_if(m_pendingTimeoutEntities.begin(), m_pendingTimeoutEntities.end(), [this](const QMap<qint64, NotifyEntity>::iterator::value_type &item) {
return item.value().id() == m_blockClosedId;
});或者更简洁地使用 C++ 结构化绑定（如果编译器支持）： auto findIter = std::find_if(m_pendingTimeoutEntities.begin(), m_pendingTimeoutEntities.end(), [this](const auto &item) {
return item.value().id() == m_blockClosedId;
});2. 代码质量问题 1：魔法数字 改进建议： // 在类定义中或头文件中
static constexpr qint64 INVALID_ID = 0;
static constexpr qint64 HOVER_EXTENSION_MS = 1000;问题 2：版权年份更新 3. 代码性能问题：线性查找 改进建议： 4. 代码安全问题 1：线程安全 改进建议：
问题 2：潜在的空指针解引用 改进建议： void BubblePanel::setHoveredId(qint64 id)
{
if (m_notificationServer) {
QMetaObject::invokeMethod(m_notificationServer, "setBlockClosedId", Qt::DirectConnection, Q_ARG(qint64, id));
}
}5. 其他建议问题：日志输出 改进建议： qCDebug(notifyLog) << "Block close bubble id:" << m_blockClosedId << "for the new block bubble id:" << id;总结这段代码实现了基本功能，但在逻辑正确性（ |
deepin pr auto review这段代码主要实现了通知气泡在鼠标悬停时阻止其自动关闭的功能，通过引入 以下是对这段代码的详细审查，包括语法逻辑、代码质量、代码性能和代码安全方面的改进意见： 1. 语法逻辑与功能实现审查点： 在 void NotificationManager::setBlockClosedId(qint64 id)
{
auto findIter = std::find_if(m_pendingTimeoutEntities.begin(), m_pendingTimeoutEntities.end(), [this](const NotifyEntity &entity) {
return entity.id() == m_blockClosedId;
});
const auto current = QDateTime::currentMSecsSinceEpoch();
if(m_blockClosedId != NotifyEntity::InvalidId && findIter != m_pendingTimeoutEntities.end() && current > findIter.key()) {
// ...
}
m_blockClosedId = id;
onHandingPendingEntities();
}
审查点： if (item.id() == m_blockClosedId) {
qDebug(notifyLog) << "bubble id:" << item.bubbleId() << "entity id:" << item.id();
m_pendingTimeoutEntities.insert(current + 1000, item);
continue;
}
2. 代码质量
3. 代码性能
4. 代码安全
改进建议
修正后的代码示例（部分逻辑）针对 void NotificationManager::setBlockClosedId(qint64 id)
{
// 如果设置了新的阻塞ID，且之前有旧的阻塞ID，需要处理旧ID的"恢复"逻辑
if (m_blockClosedId != NotifyEntity::InvalidId && m_blockClosedId != id) {
auto findIter = std::find_if(m_pendingTimeoutEntities.begin(), m_pendingTimeoutEntities.end(),
[this](const NotifyEntity &entity) { return entity.id() == m_blockClosedId; });
if (findIter != m_pendingTimeoutEntities.end()) {
// 找到了旧的阻塞实体，将其重新加入队列，设置新的超时时间（例如当前时间 + 默认超时）
// 注意：这里需要根据实际业务逻辑决定是恢复默认超时还是立即处理
// 如果是切换到另一个气泡悬停，可能不需要做任何事，因为新的 setBlockClosedId 会保护新的气泡
// 但如果 id == 0 (鼠标移出)，则需要让它尽快关闭或恢复倒计时
// 简化逻辑：如果切换到 0，我们将其超时设为当前时间+一小段时间以便立即处理
// 或者直接从队列中移除并触发关闭逻辑（取决于需求：鼠标移开后是立即关闭还是等待剩余时间）
// 假设需求是等待剩余时间或立即关闭，这里不做重新插入，而是让它在下次 onHandingPendingEntities 中自然处理
}
}
m_blockClosedId = id;
// 如果需要立即触发处理（例如为了响应鼠标移出）
if (id == 0) {
onHandingPendingEntities();
}
}
void NotificationManager::onHandingPendingEntities()
{
const auto current = QDateTime::currentMSecsSinceEpoch();
// 使用可修改的迭代器或先收集Key再处理，避免迭代器失效问题（虽然这里只是continue）
// 注意：不要在遍历时直接 insert 到同一个 QMap，这可能导致逻辑混乱
// 更好的做法是：如果是阻塞ID，跳过。真正的延期逻辑由定时器或其他机制触发，或者在这里只做"跳过"，
// 而在 setBlockClosedId 中更新时间戳（如果 QMap 允许修改 Key，或者 erase 后 insert）
for (auto it = m_pendingTimeoutEntities.begin(); it != m_pendingTimeoutEntities.end(); ) {
// 检查是否超时
if (current < it.key()) {
++it;
continue;
}
NotifyEntity item = it.value();
qint64 key = it.key();
// 移除当前项，防止内存泄漏或重复处理
it = m_pendingTimeoutEntities.erase(it);
if (item.id() == m_blockClosedId) {
// 如果是当前被阻塞的ID，重新插入到未来时间
// 这里使用 move 语义减少拷贝
m_pendingTimeoutEntities.insert(current + HOVER_EXTENSION_TIME_MS, std::move(item));
continue;
}
// 正常关闭逻辑
notificationClosed(item.id(), item.bubbleId(), NotifyEntity::Expired);
}
}总结： |
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: