Skip to content

feat: add hover protection for notification bubbles #1502

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mhduiy wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: add hover protection for notification bubbles #1502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions panels/notification/bubble/bubblepanel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,10 @@
setVisible(!isEmpty && enabled());
}

void BubblePanel::setHoveredId(qint64 id)
{
QMetaObject::invokeMethod(m_notificationServer, "setBlockClosedId", Qt::DirectConnection, Q_ARG(qint64, id));
}
Comment on lines +209 to +212
Copy link

@sourcery-ai sourcery-ai bot Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
}

}

#include "bubblepanel.moc"

Check warning on line 215 in panels/notification/bubble/bubblepanel.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: "bubblepanel.moc" not found.
3 changes: 2 additions & 1 deletion panels/notification/bubble/bubblepanel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// SPDX-FileCopyrightText: 2024 UnionTech Software Technology Co., Ltd.
// SPDX-FileCopyrightText: 2024 - 2026 UnionTech Software Technology Co., Ltd.
//
// SPDX-License-Identifier: GPL-3.0-or-later

Expand Down Expand Up @@ -39,6 +39,7 @@ public Q_SLOTS:
void close(int bubbleIndex, int reason);
void delayProcess(int bubbleIndex);
void setEnabled(bool newEnabled);
void setHoveredId(qint64 id);

Q_SIGNALS:
void visibleChanged();
Expand Down
4 changes: 2 additions & 2 deletions panels/notification/bubble/package/Bubble.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ Control {
property var bubble
onHoveredChanged: function () {
if (control.hovered) {
Applet.bubbles.delayRemovedBubble = bubble.id
Applet.setHoveredId(control.bubble.id)
} else {
Applet.bubbles.delayRemovedBubble = 0
Applet.setHoveredId(0)
}
}

Expand Down
23 changes: 22 additions & 1 deletion panels/notification/server/notificationmanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// SPDX-FileCopyrightText: 2024 UnionTech Software Technology Co., Ltd.
// SPDX-FileCopyrightText: 2024 - 2026 UnionTech Software Technology Co., Ltd.
//
// SPDX-License-Identifier: GPL-3.0-or-later

Expand Down Expand Up @@ -347,6 +347,21 @@ QVariant NotificationManager::GetSystemInfo(uint configItem)
return m_setting->systemValue(static_cast<NotificationSetting::SystemConfigItem>(configItem));
}

void NotificationManager::setBlockClosedId(qint64 id)
Copy link

@sourcery-ai sourcery-ai bot Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
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()) {
qDebug(notifyLog) << "Block close bubble id:" << m_blockClosedId << "for the new block bubble id:" << id;
m_pendingTimeoutEntities.insert(QDateTime::currentMSecsSinceEpoch() + 1000, findIter.value());
m_pendingTimeoutEntities.erase(findIter);
}
m_blockClosedId = id;
onHandingPendingEntities();
}

bool NotificationManager::isDoNotDisturb() const
{
if (!m_setting->systemValue(NotificationSetting::DNDMode).toBool())
Expand Down Expand Up @@ -673,6 +688,12 @@ void NotificationManager::onHandingPendingEntities()
continue;
}

if (item.id() == m_blockClosedId) {
qDebug(notifyLog) << "bubble id:" << item.bubbleId() << "entity id:" << item.id();
m_pendingTimeoutEntities.insert(current + 1000, item);
continue;
}

qDebug(notifyLog) << "Expired for the notification " << item.id() << item.appName();
notificationClosed(item.id(), item.bubbleId(), NotifyEntity::Expired);
}
Expand Down
4 changes: 3 additions & 1 deletion panels/notification/server/notificationmanager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// SPDX-FileCopyrightText: 2024 UnionTech Software Technology Co., Ltd.
// SPDX-FileCopyrightText: 2024 - 2026 UnionTech Software Technology Co., Ltd.
//
// SPDX-License-Identifier: GPL-3.0-or-later

Expand Down Expand Up @@ -65,6 +65,7 @@ public Q_SLOTS:
void SetSystemInfo(uint configItem, const QVariant &value);
QVariant GetSystemInfo(uint configItem);

void setBlockClosedId(qint64 id);
private:
bool isDoNotDisturb() const;
bool recordNotification(NotifyEntity &entity);
Expand Down Expand Up @@ -97,6 +98,7 @@ private slots:
QStringList m_systemApps;
QMap<QString, QVariant> m_appNamesMap;
int m_cleanupDays = 7;
qint64 m_blockClosedId = 0;
};

} // notification
7 changes: 6 additions & 1 deletion panels/notification/server/notifyserverapplet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// SPDX-FileCopyrightText: 2024 UnionTech Software Technology Co., Ltd.
// SPDX-FileCopyrightText: 2024 - 2026 UnionTech Software Technology Co., Ltd.
//
// SPDX-License-Identifier: GPL-3.0-or-later

Expand Down Expand Up @@ -104,6 +104,11 @@ void NotifyServerApplet::removeExpiredNotifications()
m_manager->removeExpiredNotifications();
}

void NotifyServerApplet::setBlockClosedId(qint64 id)
{
m_manager->setBlockClosedId(id);
}

D_APPLET_CLASS(NotifyServerApplet)

}
Expand Down
3 changes: 2 additions & 1 deletion panels/notification/server/notifyserverapplet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// SPDX-FileCopyrightText: 2024 UnionTech Software Technology Co., Ltd.
// SPDX-FileCopyrightText: 2024 - 2026 UnionTech Software Technology Co., Ltd.
//
// SPDX-License-Identifier: GPL-3.0-or-later

Expand Down Expand Up @@ -31,6 +31,7 @@ public Q_SLOTS:
void removeNotifications(const QString &appName);
void removeNotifications();
void removeExpiredNotifications();
void setBlockClosedId(qint64 id);

private:
NotificationManager *m_manager = nullptr;
Expand Down
Loading