Merge pull request #8782 from nextcloud/bugfix/6624/duplicate-notifications-in-activity-list

fix: try to avoid displaying duplicate server notifications in activity list

Author: mgallien

src/gui/systray.h

@@ -187,7 +187,7 @@ private slots:
 
     AccessManagerFactory _accessManagerFactory;
 
-    QSet<qlonglong> _callsAlreadyNotified;
+    QSet<qint64> _callsAlreadyNotified;
     QPointer<QObject> _editFileLocallyLoadingDialog;
     QPointer<QObject> _encryptionTokenDiscoveryDialog;
     QVector<QQuickWindow*> _fileDetailDialogs;

src/gui/tray/activitydata.cpp

@@ -77,7 +77,7 @@ OCC::Activity Activity::fromActivityJson(const QJsonObject &json, const AccountP
     activity._objectType = json.value(QStringLiteral("object_type")).toString();
     activity._objectId = json.value(QStringLiteral("object_id")).toInt();
     activity._objectName = json.value(QStringLiteral("object_name")).toString();
-    activity._id = json.value(QStringLiteral("activity_id")).toInt();
+    activity._id = json.value(QStringLiteral("activity_id")).toInteger();
     activity._fileAction = json.value(QStringLiteral("type")).toString();
     activity._accName = account->displayName();
     activity._subject = json.value(QStringLiteral("subject")).toString();

src/gui/tray/activitydata.h

@@ -136,7 +136,7 @@ class Activity
     Q_PROPERTY(QVariantMap subjectRichParameters MEMBER _subjectRichParameters)
 
 public:
-    using Identifier = QPair<qlonglong, QString>;
+    using Identifier = QPair<qint64, QString>;
 
     // Note that these are in the order we want to present them in the model!
     enum Type {
@@ -160,7 +160,7 @@ class Activity
     using TalkNotificationData = OCC::TalkNotificationData;
 
     Type _type;
-    qlonglong _id = 0LL;
+    qint64 _id = 0LL;
     QString _fileAction;
     int _objectId = 0;
     TalkNotificationData _talkNotificationData;

src/gui/tray/activitylistmodel.cpp

@@ -523,18 +523,37 @@ void ActivityListModel::activitiesReceived(const QJsonDocument &json, int status
 
 void ActivityListModel::addEntriesToActivityList(const ActivityList &activityList)
 {
-    if(activityList.isEmpty()) {
+    if (activityList.isEmpty()) {
+        return;
+    }
+
+    // filter activities that do not need to be added again
+    ActivityList filteredList;
+    std::copy_if(activityList.constBegin(), activityList.constEnd(), std::back_inserter(filteredList), [this](const auto &activity) -> bool {
+        if (activity._type == Activity::NotificationType && _activeNotificationIds.contains(activity._id)) {
+            return false;
+        }
+        return true;
+    });
+
+    if (filteredList.isEmpty()) {
         return;
     }
 
     const auto startRow = _finalList.count();
 
-    beginInsertRows({}, startRow, startRow + activityList.count() - 1);
-    for(const auto &activity : activityList) {
+    beginInsertRows({}, startRow, startRow + filteredList.count() - 1);
+    for(const auto &activity : std::as_const(filteredList)) {
         _finalList.append(activity);
+
         if (activity._syncFileItemStatus == SyncFileItem::Conflict) {
             _conflictsList.push_back(activity);
         }
+
+        if (activity._type == Activity::NotificationType) {
+            // new unseen notification, to avoid duplicate activities to appear add its id to the filter list
+            _activeNotificationIds.insert(activity._id);
+        }
     }
     endInsertRows();
 
@@ -591,7 +610,6 @@ void ActivityListModel::addNotificationToActivityList(const Activity &activity)
 {
     qCDebug(lcActivity) << "Notification successfully added to the notification list: " << activity._subject;
     addEntriesToActivityList({activity});
-    _notificationLists.prepend(activity);
     for (const auto &link : activity._links) {
         if (link._verb == QByteArrayLiteral("POST")
             || link._verb == QByteArrayLiteral("REPLY")
@@ -605,7 +623,6 @@ void ActivityListModel::addSyncFileItemToActivityList(const Activity &activity)
 {
     qCDebug(lcActivity) << "Successfully added to the activity list: " << activity._subject;
     addEntriesToActivityList({activity});
-    _syncFileItemLists.prepend(activity);
 }
 
 void ActivityListModel::removeActivityFromActivityList(int row)
@@ -627,27 +644,37 @@ void ActivityListModel::removeActivityFromActivityList(const Activity &activity)
         _conflictsList.removeOne(activity);
     }
 
+    if (activity._type == Activity::NotificationType) {
+        _activeNotificationIds.remove(activity._id);
+    }
+
     if (activity._type != Activity::ActivityType &&
         activity._type != Activity::DummyFetchingActivityType &&
         activity._type != Activity::DummyMoreActivitiesAvailableType &&
         activity._type != Activity::NotificationType &&
         activity._type != Activity::OpenSettingsNotificationType) {
 
         const auto notificationErrorsListIndex = _notificationErrorsLists.indexOf(activity);
-        if (notificationErrorsListIndex != -1)
+        if (notificationErrorsListIndex != -1) {
             _notificationErrorsLists.removeAt(notificationErrorsListIndex);
+        }
     }
 }
 
-void ActivityListModel::checkAndRemoveSeenActivities(const OCC::ActivityList &newActivities)
+void ActivityListModel::removeOutdatedNotifications(const OCC::ActivityList &receivedNotifications)
 {
     ActivityList activitiesToRemove;
     for (const auto &activity : _finalList) {
-        const auto isTalkActiity = activity._objectType == QStringLiteral("chat") ||
-            activity._objectType == QStringLiteral("call");
-        if (isTalkActiity && !newActivities.contains(activity)) {
-            activitiesToRemove.push_back(activity);
+        if (activity._type != Activity::NotificationType || receivedNotifications.contains(activity)) {
+            continue;
         }
+
+        qCDebug(lcActivity).nospace() << "marking notification activity for removal"
+            << " activity.type=" << activity._type
+            << " activity.id=" << activity._id
+            << " activity.objectType=" << activity._objectType
+            << " activity.accName=" << activity._accName;
+        activitiesToRemove.push_back(activity);
     }
 
     for (const auto &toRemove : activitiesToRemove) {
@@ -930,6 +957,7 @@ void ActivityListModel::slotRemoveAccount()
     _conflictsList.clear();
     _activityLists.clear();
     _presentedActivities.clear();
+    _activeNotificationIds.clear();
     setAndRefreshCurrentlyFetching(false);
     _doneFetching = false;
     _currentItem = 0;

src/gui/tray/activitylistmodel.h

@@ -127,7 +127,7 @@ public slots:
     void removeActivityFromActivityList(int row);
     void removeActivityFromActivityList(const OCC::Activity &activity);
 
-    void checkAndRemoveSeenActivities(const OCC::ActivityList &newActivities);
+    void removeOutdatedNotifications(const OCC::ActivityList &receivedNotifications);
 
     void setAccountState(OCC::AccountState *state);
     void setReplyMessageSent(const int activityIndex, const QString &message);
@@ -179,18 +179,16 @@ private slots:
     Activity _dummyFetchingActivities;
 
     ActivityList _activityLists;
-    ActivityList _syncFileItemLists;
-    ActivityList _notificationLists;
-    ActivityList _listOfIgnoredFiles;
     ActivityList _notificationErrorsLists;
     ActivityList _conflictsList;
     ActivityList _finalList;
 
     QSet<qint64> _presentedActivities;
+    QSet<qint64> _activeNotificationIds;
 
     bool _displayActions = true;
 
-    int _currentItem = 0;
+    qint64 _currentItem = 0;
     static constexpr int _maxActivities = 100;
     static constexpr int _maxActivitiesDays = 30;
     bool _showMoreActivitiesAvailableEntry = false;

src/gui/tray/notificationhandler.cpp

@@ -106,7 +106,7 @@ void ServerNotificationHandler::slotNotificationsReceived(const QJsonDocument &j
         auto a = Activity::fromActivityJson(json, ai->account());
 
         a._type = Activity::NotificationType;
-        a._id = json.value("notification_id").toInt();
+        a._id = json.value("notification_id").toInteger();
 
         if(json.contains("subjectRichParameters")) {
             const auto richParams = json.value("subjectRichParameters").toObject();

src/gui/tray/usermodel.cpp

@@ -125,38 +125,30 @@ User::User(AccountStatePtr &account, const bool &isCurrent, QObject *parent)
 
 void User::checkNotifiedNotifications()
 {
-    // after one hour, clear the gui log notification store
+    // clear the gui log notification store after one hour has passed since the last received notification
     constexpr qint64 clearGuiLogInterval = 60 * 60 * 1000;
     if (_guiLogTimer.elapsed() > clearGuiLogInterval) {
         _notifiedNotifications.clear();
     }
 }
 
-bool User::notificationAlreadyShown(const long notificationId)
+bool User::notificationAlreadyShown(const qint64 notificationId)
 {
     checkNotifiedNotifications();
     return _notifiedNotifications.contains(notificationId);
 }
 
-bool User::canShowNotification(const long notificationId)
+bool User::canShowNotification(const qint64 notificationId)
 {
     ConfigFile cfg;
     return cfg.optionalServerNotifications() &&
             isDesktopNotificationsAllowed() &&
             !notificationAlreadyShown(notificationId);
 }
 
-void User::checkAndRemoveSeenActivities(const ActivityList &list, const int numTalkNotificationsReceived)
+void User::showDesktopNotification(const QString &title, const QString &message, const qint64 notificationId)
 {
-    if (numTalkNotificationsReceived < _lastTalkNotificationsReceivedCount) {
-        _activityModel->checkAndRemoveSeenActivities(list);
-    }
-    _lastTalkNotificationsReceivedCount = numTalkNotificationsReceived;
-}
-
-void User::showDesktopNotification(const QString &title, const QString &message, const long notificationId)
-{
-    if(!canShowNotification(notificationId)) {
+    if (!canShowNotification(notificationId)) {
         return;
     }
 
@@ -200,7 +192,7 @@ void User::showDesktopNotification(const ActivityList &activityList)
 
     Logger::instance()->postGuiLog(subject, message);
 
-    for(const auto &activity : activityList) {
+    for (const auto &activity : activityList) {
         _notifiedNotifications.insert(activity._id);
         _activityModel->addNotificationToActivityList(activity);
     }
@@ -232,43 +224,41 @@ void User::showDesktopTalkNotification(const Activity &activity)
 
 void User::slotBuildNotificationDisplay(const ActivityList &list)
 {
-    const auto talkNotificationsReceivedCount = std::count_if(std::cbegin(list), std::cend(list), [](const auto &activity) {
-        return activity._objectType == QStringLiteral("chat") ||
-            activity._objectType == QStringLiteral("call");
-    });
-    checkAndRemoveSeenActivities(list, talkNotificationsReceivedCount);
-
     ActivityList toNotifyList;
 
-    std::copy_if(list.constBegin(), list.constEnd(), std::back_inserter(toNotifyList), [&](const Activity &activity) {
+    _activityModel->removeOutdatedNotifications(list);
 
-        if (_blacklistedNotifications.contains(activity)) {
-            qCInfo(lcActivity) << "Activity in blacklist, skip";
-            return false;
-        } else if(_notifiedNotifications.contains(activity._id)) {
-            qCInfo(lcActivity) << "Activity already notified, skip";
+    std::copy_if(list.constBegin(), list.constEnd(), std::back_inserter(toNotifyList), [&](const Activity &activity) -> bool {
+        if (!activity._shouldNotify) {
+            qCDebug(lcActivity).nospace() << "No notification should be sent for activity with id=" << activity._id << " objectType=" << activity._objectType;
             return false;
         }
-        if (!activity._shouldNotify) {
-            qCDebug(lcActivity) << "Activity should not be notified";
+
+        if (_notifiedNotifications.contains(activity._id)) {
+            qCInfo(lcActivity).nospace() << "Ignoring already notified activity with id=" << activity._id << " objectType=" << activity._objectType;
             return false;
         }
 
         return true;
     });
 
-    if(toNotifyList.count() > 2) {
-        showDesktopNotification(toNotifyList);
+    if (toNotifyList.isEmpty()) {
         return;
     }
 
-    for (const auto &activity : std::as_const(toNotifyList)) {
+    if (toNotifyList.size() == 1) {
+        const auto &activity = toNotifyList.constFirst();
         if (activity._objectType == QStringLiteral("chat")) {
+            // Talk's "call" type is handled in slotBuildIncomingCallDialogs
             showDesktopTalkNotification(activity);
-        } else {
-            showDesktopNotification(activity);
+            return;
         }
+
+        showDesktopNotification(activity);
+        return;
     }
+
+    showDesktopNotification(toNotifyList);
 }
 
 void User::slotNotificationFetchFinished()
@@ -288,16 +278,18 @@ void User::slotBuildIncomingCallDialogs(const ActivityList &list)
     }
 
     const auto systray = Systray::instance();
+    if (!systray) {
+        qCWarning(lcActivity) << "No systray instance available, can not notify about new calls";
+        return;
+    }
 
-    if(systray) {
-        for(const auto &activity : list) {
-            if (!activity._shouldNotify) {
-                qCDebug(lcActivity) << "Activity should not be notified";
-                continue;
-            }
-
-            systray->createCallDialog(activity, _account);
+    for (const auto &activity : list) {
+        if (!activity._shouldNotify) {
+            qCDebug(lcActivity).nospace() << "No notification should be sent for activity with id=" << activity._id << " objectType=" << activity._objectType;
+            continue;
         }
+
+        systray->createCallDialog(activity, _account);
     }
 }
 

src/gui/tray/usermodel.h

@@ -164,7 +164,7 @@ private slots:
     void slotGroupFoldersFetched(QNetworkReply *reply);
     void slotQuotaChanged(const int64_t &usedBytes, const int64_t &availableBytes);
     void checkNotifiedNotifications();
-    void showDesktopNotification(const QString &title, const QString &message, const long notificationId);
+    void showDesktopNotification(const QString &title, const QString &message, const qint64 notificationId);
     void showDesktopNotification(const OCC::Activity &activity);
     void showDesktopNotification(const OCC::ActivityList &activityList);
     void showDesktopTalkNotification(const OCC::Activity &activity);
@@ -178,18 +178,15 @@ private slots:
     bool isActivityOfCurrentAccount(const Folder *folder) const;
     [[nodiscard]] bool isUnsolvableConflict(const SyncFileItemPtr &item) const;
 
-    bool notificationAlreadyShown(const long notificationId);
-    bool canShowNotification(const long notificationId);
-
-    void checkAndRemoveSeenActivities(const ActivityList &list, const int numTalkNotificationsReceived);
+    bool notificationAlreadyShown(const qint64 notificationId);
+    bool canShowNotification(const qint64 notificationId);
 
     [[nodiscard]] bool serverHasTalk() const;
 
     AccountStatePtr _account;
     bool _isCurrentUser;
     ActivityListModel *_activityModel;
     UnifiedSearchResultsListModel *_unifiedSearchResultsModel;
-    ActivityList _blacklistedNotifications;
 
     QVariantList _trayFolderInfos;
 
@@ -198,7 +195,8 @@ private slots:
     QHash<AccountState *, QElapsedTimer> _timeSinceLastCheck;
 
     QElapsedTimer _guiLogTimer;
-    QSet<long> _notifiedNotifications;
+    QSet<qint64> _notifiedNotifications;
+    QSet<qint64> _activeNotifications;
     QMimeDatabase _mimeDb;
 
     // number of currently running notification requests. If non zero,