fix: prevent notifications when user inactive#490
Open
caixr23 wants to merge 1 commit intolinuxdeepin:masterfrom
Open
fix: prevent notifications when user inactive#490caixr23 wants to merge 1 commit intolinuxdeepin:masterfrom
caixr23 wants to merge 1 commit intolinuxdeepin:masterfrom
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
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23 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 |
![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
4 times, most recently
from
dd783f0 to
62e60e2
Compare
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. 语法逻辑与正确性
2. 代码质量
3. 代码性能
4. 代码安全
5. 详细代码修改建议以下是基于上述意见的具体修改建议示例： 头文件: // 添加常量定义
constexpr auto LOGIN1_SESSION_IFACE = "org.freedesktop.login1.Session";
constexpr auto LOGIN1_USER_IFACE = "org.freedesktop.login1.User";
constexpr auto PROPERTY_ACTIVE = "Active";
constexpr auto PROPERTY_REMOTE = "Remote";
constexpr auto PROPERTY_DISPLAY = "Display";
// ... 在类定义中 ...
private slots:
// ...
// 修改错误处理槽声明（如果决定添加错误处理）
void onDbusError(const QDBusError &error);源文件: // 构造函数初始化列表顺序修正（假设头文件声明顺序为 m_notifyEnabled, m_wifiEnabled, m_delayShowWifiOSD, m_resetWifiOSDEnableTimer, m_sessionActive, m_sessionRemote）
NetworkStateHandler::NetworkStateHandler(QObject *parent)
: QObject(parent)
, m_notifyEnabled(true)
, m_wifiEnabled(true)
, m_delayShowWifiOSD(new QTimer(this))
, m_resetWifiOSDEnableTimer(new QTimer(this))
, m_sessionActive(false) // 移动到对应位置
, m_sessionRemote(false) // 移动到对应位置
{
// ... 原有代码 ...
// 检查 D-Bus 连接结果
if (!QDBusConnection::systemBus().connect(LOGIN1_SERVICE, sessionPath, DBUS_PROPERTIES_IFACE, "PropertiesChanged",
this, SLOT(onLogin1PropertiesChanged(QString, QVariantMap, QStringList)))) {
qCWarning(DSM()) << "Failed to connect to login1 PropertiesChanged signal";
}
}
void NetworkStateHandler::initUserDisplay(const QDBusMessage &msg)
{
if (msg.arguments().isEmpty()) {
qCWarning(DSM()) << "initUserDisplay: Empty arguments";
return;
}
// 使用常量
QDBusVariant display = msg.arguments().first().value<QDBusVariant>();
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;
}
// 使用常量
QDBusMessage sessionMsg = QDBusMessage::createMethodCall(LOGIN1_SERVICE, sessionPath, DBUS_PROPERTIES_IFACE, "GetAll");
sessionMsg << LOGIN1_SESSION_IFACE;
// 添加错误处理回调
QDBusPendingCall async = QDBusConnection::systemBus().asyncCall(sessionMsg);
QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(async, this);
connect(watcher, &QDBusPendingCallWatcher::finished, this, [this](QDBusPendingCallWatcher *watcher) {
QDBusPendingReply<QVariantMap> reply = *watcher;
if (reply.isError()) {
qCWarning(DSM()) << "Failed to get login1 session properties:" << reply.error().message();
// 可以在这里设置默认状态或重试逻辑
} else {
updateLogin1Properties(reply.value());
}
watcher->deleteLater();
});
// 注意：原代码中的 connect 依赖于 sessionPath，如果 sessionPath 无效，connect 也会失败。
// 这里的逻辑保持原样，但建议检查返回值。
if (!QDBusConnection::systemBus().connect(LOGIN1_SERVICE, sessionPath, DBUS_PROPERTIES_IFACE, "PropertiesChanged",
this, SLOT(onLogin1PropertiesChanged(QString, QVariantMap, QStringList)))) {
qCWarning(DSM()) << "Failed to connect to session" << sessionPath << "PropertiesChanged";
}
}
void NetworkStateHandler::updateLogin1Properties(const QVariantMap &properties)
{
// 使用常量
if (properties.contains(PROPERTY_ACTIVE)) {
m_sessionActive = properties.value(PROPERTY_ACTIVE).toBool();
qCDebug(DSM()) << "Session Active changed to:" << m_sessionActive;
}
if (properties.contains(PROPERTY_REMOTE)) {
m_sessionRemote = properties.value(PROPERTY_REMOTE).toBool();
qCDebug(DSM()) << "Session Remote changed to:" << m_sessionRemote;
}
}总结： |
Contributor
|
TAG Bot New tag: 2.0.81 |
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 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: