fix: prevent notifications when user inactive#490
Merged
caixr23 merged 1 commit intolinuxdeepin:masterfrom Feb 24, 2026
Merged
Conversation
Reviewer's GuideAdds a user-activity check to the network notification path so that network notifications are suppressed when the current systemd-logind user session is inactive or remote, using D-Bus queries in a new helper method. Sequence diagram for network notifications with user activity checksequenceDiagram
participant NetworkStateHandler
participant SystemDBus
participant Login1Manager
participant Login1User
participant Login1Session
participant NotificationDaemon
NetworkStateHandler->>NetworkStateHandler: notify(icon, summary, body)
alt notifications_disabled
NetworkStateHandler-->>NetworkStateHandler: return (m_notifyEnabled is false)
else notifications_enabled
NetworkStateHandler->>NetworkStateHandler: isUserActive()
Note over NetworkStateHandler,SystemDBus: Query login1 for current user
NetworkStateHandler->>SystemDBus: org.freedesktop.login1.Manager.GetUser(uid)
SystemDBus->>Login1Manager: GetUser(uid)
Login1Manager-->>SystemDBus: QDBusObjectPath userPath
SystemDBus-->>NetworkStateHandler: userPath
alt get_user_error
NetworkStateHandler-->>NetworkStateHandler: isUserActive returns false
else user_path_ok
NetworkStateHandler->>SystemDBus: org.freedesktop.DBus.Properties.GetAll(User)
SystemDBus->>Login1User: GetAll(org.freedesktop.login1.User)
Login1User-->>SystemDBus: QVariantMap userInfo
SystemDBus-->>NetworkStateHandler: userInfo
alt user_state_not_active
NetworkStateHandler-->>NetworkStateHandler: isUserActive returns false
else user_state_active
NetworkStateHandler->>NetworkStateHandler: parse Display(id, displayPath)
alt display_path_empty
NetworkStateHandler-->>NetworkStateHandler: isUserActive returns false
else display_path_valid
NetworkStateHandler->>SystemDBus: org.freedesktop.DBus.Properties.GetAll(Session)
SystemDBus->>Login1Session: GetAll(org.freedesktop.login1.Session)
Login1Session-->>SystemDBus: QVariantMap sessionInfo
SystemDBus-->>NetworkStateHandler: sessionInfo
alt inactive_or_remote_session
NetworkStateHandler-->>NetworkStateHandler: isUserActive returns false
else active_local_session
NetworkStateHandler-->>NetworkStateHandler: isUserActive returns true
end
end
end
end
alt user_active
NetworkStateHandler->>NotificationDaemon: show notification(icon, summary, body)
NotificationDaemon-->>NetworkStateHandler: ack
else user_inactive
NetworkStateHandler-->>NetworkStateHandler: skip notification
end
end
Updated class diagram for NetworkStateHandler with user activity checkclassDiagram
class NetworkStateHandler {
- bool m_notifyEnabled
+ void notify(QString icon, QString summary, QString body)
+ void notifyVpnFailed(QString id, uint reason)
+ bool isUserActive()
}
class SystemDBus {
+ QDBusPendingReply asyncCall(QDBusMessage message)
}
class Login1Manager {
+ QDBusObjectPath GetUser(uid_t uid)
}
class Login1User {
+ QVariantMap GetAllUserProperties()
}
class Login1Session {
+ QVariantMap GetAllSessionProperties()
}
NetworkStateHandler ..> SystemDBus : uses
SystemDBus ..> Login1Manager : forwards_calls
SystemDBus ..> Login1User : forwards_calls
SystemDBus ..> Login1Session : forwards_calls
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 1 issue, and left some high level feedback:
- isUserActive() performs multiple synchronous D-Bus round-trips on every notification; consider caching the activity state or listening to logind signals (e.g., Session.New/Removed, PropertiesChanged) to avoid adding latency and load to frequent notify() calls.
- The new logic returns false (disabling notifications) on any D-Bus error; if logind is unavailable or transiently failing this will silently suppress all notifications, so you may want to fail open (default to active) or distinguish between "inactive" and "unknown" states.
- When reading the "Display" property, you assume the QVariant contains a valid QDBusArgument structure; adding checks for canConvert() and validating the structure before beginStructure()/operator>> would make this more robust against unexpected logind responses.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- isUserActive() performs multiple synchronous D-Bus round-trips on every notification; consider caching the activity state or listening to logind signals (e.g., Session.New/Removed, PropertiesChanged) to avoid adding latency and load to frequent notify() calls.
- The new logic returns false (disabling notifications) on any D-Bus error; if logind is unavailable or transiently failing this will silently suppress all notifications, so you may want to fail open (default to active) or distinguish between "inactive" and "unknown" states.
- When reading the "Display" property, you assume the QVariant contains a valid QDBusArgument structure; adding checks for canConvert<QDBusArgument>() and validating the structure before beginStructure()/operator>> would make this more robust against unexpected logind responses.
## Individual Comments
### Comment 1
<location> `network-service-plugin/src/session/networkstatehandler.cpp:862` </location>
<code_context>
notify(notifyIconVpnDisconnected, tr("Disconnected"), m_vpnErrorTable[reason]);
}
+bool NetworkStateHandler::isUserActive()
+{
+ uid_t uid = getuid();
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring `isUserActive()` into smaller helpers with centralized D-Bus constants and blocking calls to simplify the control flow and encapsulate low-level details.
You can keep the same behavior but significantly reduce complexity in `isUserActive()` by:
1. **Splitting D-Bus orchestration into small helpers**
2. **Hiding `QDBusArgument` unpacking behind a dedicated helper**
3. **Using synchronous calls for these status checks**
4. **Centralizing repeated D-Bus strings**
Example refactor sketch:
```c++
// 1) Centralize constants (top of .cpp or in anonymous namespace)
namespace {
constexpr auto LOGIN1_SERVICE = "org.freedesktop.login1";
constexpr auto LOGIN1_MANAGER_PATH = "/org/freedesktop/login1";
constexpr auto LOGIN1_MANAGER_IFACE = "org.freedesktop.login1.Manager";
constexpr auto DBUS_PROPERTIES_IFACE = "org.freedesktop.DBus.Properties";
constexpr auto LOGIN1_USER_IFACE = "org.freedesktop.login1.User";
constexpr auto LOGIN1_SESSION_IFACE = "org.freedesktop.login1.Session";
}
// 2) Small helpers for D-Bus calls
static QDBusObjectPath getLogin1UserPath(uid_t uid)
{
QDBusMessage msg = QDBusMessage::createMethodCall(
LOGIN1_SERVICE, LOGIN1_MANAGER_PATH, LOGIN1_MANAGER_IFACE, "GetUser");
msg << uid;
const auto reply = QDBusConnection::systemBus().call(msg);
if (reply.type() == QDBusMessage::ErrorMessage) {
qCWarning(DSM()) << "get User error:" << reply.errorMessage();
return {};
}
return reply.arguments().value(0).value<QDBusObjectPath>();
}
static QVariantMap getAllProperties(const QDBusObjectPath &path, const char *iface)
{
QDBusMessage msg = QDBusMessage::createMethodCall(
LOGIN1_SERVICE, path.path(), DBUS_PROPERTIES_IFACE, "GetAll");
msg << QString::fromLatin1(iface);
const auto reply = QDBusConnection::systemBus().call(msg);
if (reply.type() == QDBusMessage::ErrorMessage) {
qCWarning(DSM()) << "get" << iface << "info error:" << reply.errorMessage();
return {};
}
return reply.arguments().value(0).toMap();
}
```
Hide the low‑level `Display` unpacking:
```c++
static QDBusObjectPath extractDisplayPath(const QVariant &displayVariant)
{
const QDBusArgument arg = displayVariant.value<QDBusArgument>();
QString id;
QDBusObjectPath path;
arg.beginStructure();
arg >> id;
arg >> path;
arg.endStructure();
return path;
}
```
Then `isUserActive()` itself becomes mainly high‑level checks:
```c++
bool NetworkStateHandler::isUserActive()
{
const uid_t uid = getuid();
const QDBusObjectPath userPath = getLogin1UserPath(uid);
if (userPath.path().isEmpty()) {
return false;
}
const QVariantMap userInfo = getAllProperties(userPath, LOGIN1_USER_IFACE);
if (userInfo.isEmpty() || userInfo.value("State") != "active") {
return false;
}
const QDBusObjectPath displayPath = extractDisplayPath(userInfo.value("Display"));
if (displayPath.path().isEmpty()) {
return false;
}
const QVariantMap sessionInfo = getAllProperties(displayPath, LOGIN1_SESSION_IFACE);
if (sessionInfo.isEmpty()) {
return false;
}
if (!sessionInfo.value("Active").toBool() ||
sessionInfo.value("Remote").toBool()) {
return false;
}
return true;
}
```
This keeps all behavior (same interfaces, properties, checks) but:
- Removes deep inline D-Bus orchestration
- Encapsulates `QDBusArgument` handling
- Uses straightforward blocking `call()` for a simple boolean check
- Reduces magic strings and repeated error handling code
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
caixr23
force-pushed
the
master
branch
3 times, most recently
from
95af48e to
8527d80
Compare
caixr23
force-pushed
the
master
branch
5 times, most recently
from
62e60e2 to
673f825
Compare
Contributor
|
TAG Bot New tag: 2.0.81 |
Added user activity check to notification system to prevent sending notifications when user is not actively using the system. The new isUserActive() method queries systemd-logind via D-Bus to determine if the current user session is active and not remote. The change ensures notifications are only shown when the user is present and actively using the system, preventing unnecessary distractions during inactive periods or remote sessions. This improves user experience by reducing notification spam when the user cannot see them. Log: Network notifications now respect user activity status Influence: 1. Test network notifications appear when user is actively using the system 2. Verify notifications are suppressed when user session is inactive 3. Test notification behavior during remote desktop sessions 4. Verify notification suppression when system is locked or user is away 5. Test different network state changes (connection/disconnection/VPN events) fix: 防止用户非活动时发送通知 在通知系统中添加用户活动状态检查，防止在用户未主动使用系统时发送通知。新 增的 isUserActive() 方法通过 D-Bus 查询 systemd-logind 来确定当前用户会 话是否活跃且非远程会话。 该变更确保通知仅在用户在场且主动使用系统时显示，避免在用户非活动期间或远 程会话时产生不必要的干扰。通过减少用户无法看到通知时的通知垃圾，提升了用 户体验。 Log: 网络通知现在会尊重用户活动状态 Influence: 1. 测试用户主动使用系统时网络通知是否正常显示 2. 验证用户会话非活动时通知是否被抑制 3. 测试远程桌面会话期间的通知行为 4. 验证系统锁定或用户离开时通知抑制功能 5. 测试不同网络状态变化（连接/断开/VPN事件）的通知行为 PMS: BUG-349057
deepin pr auto review代码审查报告总体评价这段代码实现了通过 D-Bus 与 具体审查意见1. 语法逻辑问题 1：成员变量初始化不完整在 建议： // 在头文件中添加注释说明默认值
bool m_sessionActive { false }; // 默认为非活动会话，不显示通知
bool m_sessionRemote { false }; // 默认为非远程会话，不显示通知问题 2：D-Bus 调用错误处理不足
建议： void NetworkStateHandler::initUserDisplay(const QDBusMessage &msg)
{
if (msg.type() == QDBusMessage::ErrorMessage) {
qCWarning(DSM()) << "Failed to get user display:" << msg.errorMessage();
return;
}
if (msg.arguments().isEmpty()) {
qCWarning(DSM()) << "Empty arguments in user display message";
return;
}
// 其余代码...
}2. 代码质量问题 1：魔法字符串代码中使用了多个字符串字面量作为 D-Bus 接口和服务名称，虽然已经提取了部分常量，但仍有改进空间。 建议： // 在类定义前添加更多常量
constexpr auto LOGIN1_USER_IFACE = "org.freedesktop.login1.User";
constexpr auto LOGIN1_SESSION_IFACE = "org.freedesktop.login1.Session";
constexpr auto LOGIN1_DISPLAY_PROPERTY = "Display";
constexpr auto LOGIN1_ACTIVE_PROPERTY = "Active";
constexpr auto LOGIN1_REMOTE_PROPERTY = "Remote";问题 2：函数职责单一性
建议：拆分为多个函数： void NetworkStateHandler::initUserDisplay(const QDBusMessage &msg)
{
auto displayInfo = parseUserDisplayMessage(msg);
if (!displayInfo.isValid()) {
return;
}
fetchSessionProperties(displayInfo.sessionPath);
connectToSessionSignals(displayInfo.sessionPath);
}3. 代码性能问题 1：不必要的 D-Bus 调用在构造函数中同时发起了两个异步 D-Bus 调用（ 建议：考虑将 问题 2：频繁的字符串操作在 建议：如果这个路径在运行时不会改变，可以缓存它。 4. 代码安全问题 1：缺少 D-Bus 调用超时处理所有 建议： QDBusConnection::systemBus().callWithCallback(
userMsg,
this,
SLOT(initUserDisplay(QDBusMessage)),
SLOT(onDBusError(QDBusError)),
5000 // 5秒超时
);问题 2：缺少对 D-Bus 连接状态的检查没有检查 D-Bus 系统总线是否可用。 建议：在构造函数中添加检查： if (!QDBusConnection::systemBus().isConnected()) {
qCWarning(DSM()) << "Cannot connect to system bus";
// 适当的错误处理
}问题 3：类型转换安全性在 建议： QDBusVariant display = msg.arguments().first().value<QDBusVariant>();
if (!display.variant().isValid()) {
qCWarning(DSM()) << "Invalid DBusVariant in user display message";
return;
}5. 其他建议
修改后的代码示例以下是部分关键代码的修改建议： // 在头文件中添加常量
constexpr auto DBUS_PROPERTIES_IFACE = "org.freedesktop.DBus.Properties";
constexpr auto LOGIN1_SERVICE = "org.freedesktop.login1";
constexpr auto LOGIN1_USER_IFACE = "org.freedesktop.login1.User";
constexpr auto LOGIN1_SESSION_IFACE = "org.freedesktop.login1.Session";
// 在构造函数中添加 D-Bus 连接检查
NetworkStateHandler::NetworkStateHandler(QObject *parent)
: QObject(parent)
, m_notifyEnabled(true)
// ... 其他初始化
, m_sessionActive(false) // 默认为非活动会话，不显示通知
, m_sessionRemote(false) // 默认为非远程会话，不显示通知
{
if (!QDBusConnection::systemBus().isConnected()) {
qCWarning(DSM()) << "Cannot connect to system bus";
// 适当的错误处理
}
// ... 其他初始化代码
// 添加超时和错误处理
uid_t uid = getuid();
QDBusMessage userMsg = QDBusMessage::createMethodCall(
LOGIN1_SERVICE,
QString("/org/freedesktop/login1/user/_%1").arg(uid),
DBUS_PROPERTIES_IFACE,
"Get"
);
userMsg << LOGIN1_USER_IFACE << "Display";
QDBusConnection::systemBus().callWithCallback(
userMsg,
this,
SLOT(initUserDisplay(QDBusMessage)),
SLOT(onDBusError(QDBusError)),
5000 // 5秒超时
);
}
// 添加错误处理槽
void NetworkStateHandler::onDBusError(const QDBusError &error)
{
qCWarning(DSM()) << "D-Bus error:" << error.name() << error.message();
}
// 改进后的 initUserDisplay
void NetworkStateHandler::initUserDisplay(const QDBusMessage &msg)
{
if (msg.type() == QDBusMessage::ErrorMessage) {
qCWarning(DSM()) << "Failed to get user display:" << msg.errorMessage();
return;
}
if (msg.arguments().isEmpty()) {
qCWarning(DSM()) << "Empty arguments in user display message";
return;
}
QDBusVariant display = msg.arguments().first().value<QDBusVariant>();
if (!display.variant().isValid()) {
qCWarning(DSM()) << "Invalid DBusVariant in user display message";
return;
}
const QDBusArgument arg = display.variant().value<QDBusArgument>();
if (arg.currentType() != QDBusArgument::StructureType) {
qCWarning(DSM()) << "Invalid argument type, expected StructureType";
return;
}
QString id;
QDBusObjectPath displayPath;
arg.beginStructure();
arg >> id >> displayPath;
arg.endStructure();
QString sessionPath = displayPath.path();
if (sessionPath.isEmpty()) {
qCWarning(DSM()) << "Empty session path";
return;
}
fetchSessionProperties(sessionPath);
connectToSessionSignals(sessionPath);
}
// 新增辅助方法
void NetworkStateHandler::fetchSessionProperties(const QString &sessionPath)
{
QDBusMessage sessionMsg = QDBusMessage::createMethodCall(
LOGIN1_SERVICE,
sessionPath,
DBUS_PROPERTIES_IFACE,
"GetAll"
);
sessionMsg << LOGIN1_SESSION_IFACE;
QDBusConnection::systemBus().callWithCallback(
sessionMsg,
this,
SLOT(updateLogin1Properties(QVariantMap)),
SLOT(onDBusError(QDBusError)),
5000 // 5秒超时
);
}
void NetworkStateHandler::connectToSessionSignals(const QString &sessionPath)
{
QDBusConnection::systemBus().connect(
LOGIN1_SERVICE,
sessionPath,
DBUS_PROPERTIES_IFACE,
"PropertiesChanged",
this,
SLOT(onLogin1PropertiesChanged(QString, QVariantMap, QStringList))
);
}
// 改进后的属性更新方法
void NetworkStateHandler::updateLogin1Properties(const QVariantMap &properties)
{
if (properties.contains(LOGIN1_ACTIVE_PROPERTY)) {
m_sessionActive = properties.value(LOGIN1_ACTIVE_PROPERTY).toBool();
qCDebug(DSM()) << "Session active state changed to:" << m_sessionActive;
}
if (properties.contains(LOGIN1_REMOTE_PROPERTY)) {
m_sessionRemote = properties.value(LOGIN1_REMOTE_PROPERTY).toBool();
qCDebug(DSM()) << "Session remote state changed to:" << m_sessionRemote;
}
}这些修改提高了代码的健壮性、可维护性和安全性，同时保持了原有功能。 |
mhduiy
approved these changes
Feb 24, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23, 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 |
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 user activity check to notification system to prevent sending notifications when user is not actively using the system. The new isUserActive() method queries systemd-logind via D-Bus to determine if the current user session is active and not remote.
The change ensures notifications are only shown when the user is present and actively using the system, preventing unnecessary distractions during inactive periods or remote sessions. This improves user experience by reducing notification spam when the user cannot see them.
Log: Network notifications now respect user activity status
Influence:
fix: 防止用户非活动时发送通知
在通知系统中添加用户活动状态检查，防止在用户未主动使用系统时发送通知。新
增的 isUserActive() 方法通过 D-Bus 查询 systemd-logind 来确定当前用户会 话是否活跃且非远程会话。
该变更确保通知仅在用户在场且主动使用系统时显示，避免在用户非活动期间或远
程会话时产生不必要的干扰。通过减少用户无法看到通知时的通知垃圾，提升了用
户体验。
Log: 网络通知现在会尊重用户活动状态
Influence:
PMS: BUG-349057
Summary by Sourcery
Bug Fixes: