Skip to content

Commit 0300566

Browse files
committed
fix(cooperation): start new transfer requests with a fresh notification
Transfer notifications reused the previous notification id because every call passes a non-empty hint map. After the user rejected a request or the sender cancelled it, the old bubble was already dismissed (or still running its close animation), so the next transfer request updated a notification that was no longer visible and the receiver never saw it again until the application restarted. Make replacement explicit: only progress updates reuse the current id, all new notifications start at replaces_id 0. Drop the id when a notification is closed, acted upon, or fails to appear, so a stale id can never wedge later notifications. Also log received transfer requests at warning level to tell a missing RPC apart from a suppressed notification in field logs. Add a regression test for the notification id lifecycle. Log: start new transfer requests with a fresh notification PMS: TASK-396845 Influence: a rejected or cancelled request no longer hides later transfer notifications until restart.
1 parent eb2e0fb commit 0300566

7 files changed

Lines changed: 170 additions & 16 deletions

File tree

‎src/lib/cooperation/core/net/helper/transferhelper.cpp‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// SPDX-FileCopyrightText: 2023 - 2024 UnionTech Software Technology Co., Ltd.
1+
// SPDX-FileCopyrightText: 2023 - 2026 UnionTech Software Technology Co., Ltd.
22
//
33
// SPDX-License-Identifier: GPL-3.0-or-later
44

@@ -129,15 +129,17 @@ void TransferHelperPrivate::reportTransferResult(bool result)
129129
#endif
130130
}
131131

132-
void TransferHelperPrivate::notifyMessage(const QString &body, const QStringList &actions, int expireTimeout, const QVariantMap &hitMap)
132+
void TransferHelperPrivate::notifyMessage(const QString &body, const QStringList &actions, int expireTimeout,
133+
const QVariantMap &hitMap, bool replaceExisting)
133134
{
134135
#ifdef __linux__
135-
notice->notifyMessage(tr("File transfer"), body, actions, hitMap, expireTimeout);
136+
notice->notifyMessage(tr("File transfer"), body, actions, hitMap, expireTimeout, replaceExisting);
136137
#else
137138
Q_UNUSED(body)
138139
Q_UNUSED(actions)
139140
Q_UNUSED(expireTimeout)
140141
Q_UNUSED(hitMap)
142+
Q_UNUSED(replaceExisting)
141143
DLOG << "Non-Linux platform, skipping notification message";
142144
#endif
143145
}
@@ -365,7 +367,7 @@ void TransferHelper::updateProgress(int value, const QString &remainTime)
365367
QVariantMap hitMap { { "x-deepin-ShowInNotifyCenter", false } };
366368
QString msg(tr("File receiving %1% | Remaining time %2").arg(QString::number(value), remainTime));
367369

368-
d->notifyMessage(msg, actions, 15 * 1000, hitMap);
370+
d->notifyMessage(msg, actions, 15 * 1000, hitMap, true);
369371
return;
370372
}
371373
#endif

‎src/lib/cooperation/core/net/helper/transferhelper_p.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// SPDX-FileCopyrightText: 2023 UnionTech Software Technology Co., Ltd.
1+
// SPDX-FileCopyrightText: 2023 - 2026 UnionTech Software Technology Co., Ltd.
22
//
33
// SPDX-License-Identifier: GPL-3.0-or-later
44

@@ -44,7 +44,8 @@ class TransferHelperPrivate : public QObject
4444
CooperationTransDialog *transDialog();
4545

4646
void reportTransferResult(bool result);
47-
void notifyMessage(const QString &body, const QStringList &actions, int expireTimeout, const QVariantMap &hitMap = QVariantMap());
47+
void notifyMessage(const QString &body, const QStringList &actions, int expireTimeout,
48+
const QVariantMap &hitMap = QVariantMap(), bool replaceExisting = false);
4849
void initConnect();
4950
QVariantMap createViewFileHints(const QString &path);
5051

‎src/lib/cooperation/core/net/linux/noticeutil.cpp‎

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -51,24 +51,54 @@ void NoticeUtil::onActionTriggered(uint replacesId, const QString &action)
5151
return;
5252
}
5353

54+
// Invoking an action dismisses the notification, so its id is no longer
55+
// usable for follow-up updates.
56+
recvNotifyId = 0;
5457
emit ActionInvoked(action);
5558
}
5659

57-
void NoticeUtil::notifyMessage(const QString &title, const QString &body, const QStringList &actions, QVariantMap hitMap, int expireTimeout)
60+
void NoticeUtil::notifyMessage(const QString &title, const QString &body, const QStringList &actions,
61+
QVariantMap hitMap, int expireTimeout, bool replaceExisting)
5862
{
59-
// If hitMap is not empty, use previous recvNotifyId for progress updates
60-
uint notifyId = hitMap.isEmpty() ? 0 : recvNotifyId;
61-
62-
QDBusReply<uint> reply = notifyIfc->call(QString("Notify"), QString("dde-cooperation"), notifyId,
63+
uint notifyId = replaceExisting ? recvNotifyId : 0;
64+
uint shownId = sendNotify(title, body, actions, hitMap, expireTimeout, notifyId);
65+
if (shownId == 0) {
66+
// Do not keep a stale id around: the next notification must be able to
67+
// create a fresh bubble.
68+
WLOG << "Failed to show notification, replaces_id:" << notifyId;
69+
recvNotifyId = 0;
70+
return;
71+
}
72+
73+
recvNotifyId = shownId;
74+
}
75+
76+
uint NoticeUtil::sendNotify(const QString &title, const QString &body, const QStringList &actions,
77+
const QVariantMap &hints, int expireTimeout, uint replacesId)
78+
{
79+
QDBusReply<uint> reply = notifyIfc->call(QString("Notify"), QString("dde-cooperation"), replacesId,
6380
QString("dde-cooperation"), title, body,
64-
actions, hitMap, expireTimeout);
81+
actions, hints, expireTimeout);
82+
if (!reply.isValid()) {
83+
WLOG << "Notification D-Bus call failed:" << reply.error().message().toStdString();
84+
return 0;
85+
}
86+
87+
return reply.value();
88+
}
6589

66-
recvNotifyId = reply.value();
90+
void NoticeUtil::sendClose(uint notificationId)
91+
{
92+
notifyIfc->call("CloseNotification", notificationId);
6793
}
6894

6995
void NoticeUtil::closeNotification()
7096
{
71-
notifyIfc->call("CloseNotification", recvNotifyId);
97+
if (recvNotifyId == 0)
98+
return;
99+
100+
sendClose(recvNotifyId);
101+
recvNotifyId = 0;
72102
}
73103

74104
void NoticeUtil::resetNotifyId()

‎src/lib/cooperation/core/net/linux/noticeutil.h‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// SPDX-FileCopyrightText: 2023 UnionTech Software Technology Co., Ltd.
1+
// SPDX-FileCopyrightText: 2023 - 2026 UnionTech Software Technology Co., Ltd.
22
//
33
// SPDX-License-Identifier: GPL-3.0-or-later
44

@@ -19,10 +19,22 @@ class NoticeUtil : public QObject
1919
explicit NoticeUtil(QObject *parent = nullptr);
2020
~NoticeUtil();
2121

22-
void notifyMessage(const QString &title, const QString &body, const QStringList &actions, QVariantMap hitMap, int expireTimeout);
22+
// replaceExisting must only be set for updates of a notification that is
23+
// still on screen (transfer progress). A new notification always has to
24+
// start with replaces_id 0, otherwise some notification daemons keep
25+
// updating an already dismissed bubble and never show the new one.
26+
void notifyMessage(const QString &title, const QString &body, const QStringList &actions,
27+
QVariantMap hitMap, int expireTimeout, bool replaceExisting = false);
2328
void closeNotification();
2429
void resetNotifyId();
2530

31+
protected:
32+
// Seams for tests, so the id lifecycle can be verified without a running
33+
// notification daemon.
34+
virtual uint sendNotify(const QString &title, const QString &body, const QStringList &actions,
35+
const QVariantMap &hints, int expireTimeout, uint replacesId);
36+
virtual void sendClose(uint notificationId);
37+
2638
Q_SIGNALS:
2739
void ActionInvoked(const QString &action);
2840
void onConfirmTimeout();

‎src/lib/cooperation/core/net/networkutil.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,8 @@ NetworkUtilPrivate::NetworkUtilPrivate(NetworkUtil *qq)
106106
DLOG << "Handling APPLY_TRANS message";
107107
ApplyMessage req, res;
108108
req.from_json(json_value);
109+
WLOG << "Received transfer request from:" << req.host << "nick:" << req.nick
110+
<< "tunnel:" << req.tunnel;
109111
res.flag = DO_WAIT;
110112
*res_msg = res.as_json().serialize();
111113
confirmTargetAddress = QString::fromStdString(req.host);

‎tests/coop/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ add_executable(coop_tests ${COOP_SRC}
5656
${CMAKE_CURRENT_SOURCE_DIR}/cooperationutil_test.cpp
5757
${CMAKE_CURRENT_SOURCE_DIR}/sharehelper_test.cpp
5858
${CMAKE_CURRENT_SOURCE_DIR}/transferhelper_core_test.cpp
59+
${CMAKE_CURRENT_SOURCE_DIR}/noticeutil_test.cpp
5960
${CMAKE_CURRENT_SOURCE_DIR}/compatwrapper_test.cpp
6061
${CMAKE_CURRENT_SOURCE_DIR}/transferwrapper_test.cpp
6162
${CMAKE_CURRENT_SOURCE_DIR}/sharecooperationservice_test.cpp

‎tests/coop/noticeutil_test.cpp‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
// SPDX-FileCopyrightText: 2026 UnionTech Software Technology Co., Ltd.
2+
// SPDX-License-Identifier: GPL-3.0-or-later
3+
4+
#include <gtest/gtest.h>
5+
6+
#include "net/linux/noticeutil.h"
7+
8+
#include <QSignalSpy>
9+
#include <QStringList>
10+
#include <QVariantMap>
11+
#include <QVector>
12+
13+
using namespace cooperation_core;
14+
15+
namespace {
16+
17+
class FakeNoticeUtil : public NoticeUtil
18+
{
19+
public:
20+
struct NotifyCall
21+
{
22+
uint replacesId { 0 };
23+
QString body;
24+
int timeout { 0 };
25+
};
26+
27+
QVector<NotifyCall> calls;
28+
QVector<uint> closed;
29+
uint nextId { 1 };
30+
31+
protected:
32+
uint sendNotify(const QString &title, const QString &body, const QStringList &actions,
33+
const QVariantMap &hints, int expireTimeout, uint replacesId) override
34+
{
35+
Q_UNUSED(title)
36+
Q_UNUSED(actions)
37+
Q_UNUSED(hints)
38+
calls.append(NotifyCall { replacesId, body, expireTimeout });
39+
// Mimics org.freedesktop.Notifications: replacing an existing id
40+
// returns that same id, a new notification gets a fresh one.
41+
return replacesId == 0 ? nextId++ : replacesId;
42+
}
43+
44+
void sendClose(uint notificationId) override
45+
{
46+
closed.append(notificationId);
47+
}
48+
};
49+
50+
const QVariantMap kHiddenHints { { "x-deepin-ShowInNotifyCenter", false } };
51+
52+
} // namespace
53+
54+
TEST(NoticeUtilTest, RequestAfterDismissalStartsNewBubble)
55+
{
56+
FakeNoticeUtil util;
57+
58+
util.notifyMessage("File transfer", "request 1", { "accept" }, kHiddenHints, 10000);
59+
ASSERT_EQ(1, util.calls.size());
60+
EXPECT_EQ(0u, util.calls[0].replacesId);
61+
62+
// Progress updates are the only notifications allowed to replace a bubble.
63+
util.notifyMessage("File transfer", "progress", { "cancel" }, kHiddenHints, 15000, true);
64+
ASSERT_EQ(2, util.calls.size());
65+
EXPECT_EQ(1u, util.calls[1].replacesId);
66+
67+
// Acting on the request dismisses the notification.
68+
ASSERT_TRUE(QMetaObject::invokeMethod(&util, "onActionTriggered",
69+
Q_ARG(uint, 1), Q_ARG(QString, QStringLiteral("accept"))));
70+
71+
// The next request is a new notification, not an update of the dismissed
72+
// one. Some daemons never present an update for an id they already closed.
73+
util.notifyMessage("File transfer", "request 2", { "accept" }, kHiddenHints, 10000);
74+
ASSERT_EQ(3, util.calls.size());
75+
EXPECT_EQ(0u, util.calls[2].replacesId);
76+
}
77+
78+
TEST(NoticeUtilTest, CloseReleasesNotificationId)
79+
{
80+
FakeNoticeUtil util;
81+
82+
util.notifyMessage("File transfer", "request", { "accept" }, kHiddenHints, 10000);
83+
util.closeNotification();
84+
ASSERT_EQ(1, util.closed.size());
85+
EXPECT_EQ(1u, util.closed[0]);
86+
87+
util.notifyMessage("File transfer", "another request", { "accept" }, kHiddenHints, 10000);
88+
ASSERT_EQ(2, util.calls.size());
89+
EXPECT_EQ(0u, util.calls[1].replacesId);
90+
}
91+
92+
TEST(NoticeUtilTest, ActionFromAnotherNotificationIsIgnored)
93+
{
94+
FakeNoticeUtil util;
95+
QSignalSpy spy(&util, &NoticeUtil::ActionInvoked);
96+
97+
util.notifyMessage("File transfer", "request", { "accept" }, kHiddenHints, 10000);
98+
ASSERT_TRUE(QMetaObject::invokeMethod(&util, "onActionTriggered",
99+
Q_ARG(uint, 99), Q_ARG(QString, QStringLiteral("reject"))));
100+
EXPECT_EQ(0, spy.count());
101+
102+
// The notification is still on screen, so progress may still replace it.
103+
util.notifyMessage("File transfer", "progress", { "cancel" }, kHiddenHints, 15000, true);
104+
ASSERT_EQ(2, util.calls.size());
105+
EXPECT_EQ(1u, util.calls[1].replacesId);
106+
}

0 commit comments

Comments
 (0)