Merge pull request #5879 from nextcloud/bugfix/fix-incorrect-seen-chat-notifications-removal

Fix crash and incorrect implementation of seen chat notofications removal

.github/workflows/windows-build-and-test.yml

@@ -19,6 +19,9 @@ jobs:
         with:
           fetch-depth: 0 # Shallow clones should be disabled for a better relevancy of analysis
 
+      - uses: actions/setup-python@v1
+        with:
+          python-version: '3.8'
       - name: Install Craft Master with Nextcloud Client Deps
         shell: pwsh
         run: |

src/gui/tray/activitylistmodel.cpp

@@ -691,11 +691,16 @@ void ActivityListModel::removeActivityFromActivityList(const Activity &activity)
 
 void ActivityListModel::checkAndRemoveSeenActivities(const OCC::ActivityList &newActivities)
 {
+    ActivityList activitiesToRemove;
     for (const auto &activity : _finalList) {
         if (activity._objectType == QStringLiteral("chat") && !newActivities.contains(activity)) {
-            removeActivityFromActivityList(activity);
+            activitiesToRemove.push_back(activity);
         }
     }
+
+    for (const auto &toRemove : activitiesToRemove) {
+        removeActivityFromActivityList(toRemove);
+    }
 }
 
 void ActivityListModel::slotTriggerDefaultAction(const int activityIndex)

src/gui/tray/notificationhandler.cpp

@@ -24,12 +24,12 @@ ServerNotificationHandler::ServerNotificationHandler(AccountState *accountState,
 {
 }
 
-void ServerNotificationHandler::slotFetchNotifications()
+bool ServerNotificationHandler::startFetchNotifications()
 {
     // check connectivity and credentials
     if (!(_accountState && _accountState->isConnected() && _accountState->account() && _accountState->account()->credentials() && _accountState->account()->credentials()->ready())) {
         deleteLater();
-        return;
+        return false;
     }
     // check if the account has notifications enabled. If the capabilities are
     // not yet valid, its assumed that notifications are available.
@@ -37,7 +37,7 @@ void ServerNotificationHandler::slotFetchNotifications()
         if (!_accountState->account()->capabilities().notificationsAvailable()) {
             qCInfo(lcServerNotification) << "Account" << _accountState->account()->displayName() << "does not have notifications enabled.";
             deleteLater();
-            return;
+            return false;
         }
     }
 
@@ -50,6 +50,7 @@ void ServerNotificationHandler::slotFetchNotifications()
     _notificationJob->setProperty(propertyAccountStateC, QVariant::fromValue<AccountState *>(_accountState));
     _notificationJob->addRawHeader("If-None-Match", _accountState->notificationsEtagResponseHeader());
     _notificationJob->start();
+    return true;
 }
 
 void ServerNotificationHandler::slotEtagResponseHeaderReceived(const QByteArray &value, int statusCode)
@@ -66,12 +67,14 @@ void ServerNotificationHandler::slotNotificationsReceived(const QJsonDocument &j
     if (statusCode != successStatusCode && statusCode != notModifiedStatusCode) {
         qCWarning(lcServerNotification) << "Notifications failed with status code " << statusCode;
         deleteLater();
+        emit jobFinished();
         return;
     }
 
     if (statusCode == notModifiedStatusCode) {
         qCWarning(lcServerNotification) << "Status code " << statusCode << " Not Modified - No new notifications.";
         deleteLater();
+        emit jobFinished();
         return;
     }
 
@@ -170,6 +173,7 @@ void ServerNotificationHandler::slotNotificationsReceived(const QJsonDocument &j
     }
     emit newNotificationList(list);
     emit newIncomingCallsList(callList);
+    emit jobFinished();
 
     deleteLater();
 }

src/gui/tray/notificationhandler.h

@@ -18,9 +18,10 @@ public:
 signals:
     void newNotificationList(OCC::ActivityList);
     void newIncomingCallsList(OCC::ActivityList);
+    void jobFinished();
 
-public slots:
-    void slotFetchNotifications();
+public:
+    bool startFetchNotifications();
 
 private slots:
     void slotNotificationsReceived(const QJsonDocument &json, int statusCode);

src/gui/tray/usermodel.cpp

@@ -119,6 +119,14 @@ bool User::canShowNotification(const long notificationId)
             !notificationAlreadyShown(notificationId);
 }
 
+void User::checkAndRemoveSeenActivities(const ActivityList &list, const int numChatNotificationsReceived)
+{
+    if (numChatNotificationsReceived < _lastChatNotificationsReceivedCount) {
+        _activityModel->checkAndRemoveSeenActivities(list);
+    }
+    _lastChatNotificationsReceivedCount = numChatNotificationsReceived;
+}
+
 void User::showDesktopNotification(const QString &title, const QString &message, const long notificationId)
 {
     if(!canShowNotification(notificationId)) {
@@ -197,6 +205,11 @@ void User::showDesktopTalkNotification(const Activity &activity)
 
 void User::slotBuildNotificationDisplay(const ActivityList &list)
 {
+    const auto chatNotificationsReceivedCount = std::count_if(std::cbegin(list), std::cend(list), [](const auto &activity) {
+        return activity._objectType == QStringLiteral("chat");
+    });
+    checkAndRemoveSeenActivities(list, chatNotificationsReceivedCount);
+
     ActivityList toNotifyList;
 
     std::copy_if(list.constBegin(), list.constEnd(), std::back_inserter(toNotifyList), [&](const Activity &activity) {
@@ -221,21 +234,18 @@ void User::slotBuildNotificationDisplay(const ActivityList &list)
         return;
     }
 
-    auto chatNotificationsReceivedCount = 0;
-
-    for(const auto &activity : qAsConst(toNotifyList)) {
+    for (const auto &activity : qAsConst(toNotifyList)) {
         if (activity._objectType == QStringLiteral("chat")) {
-            ++chatNotificationsReceivedCount;
             showDesktopTalkNotification(activity);
         } else {
             showDesktopNotification(activity);
         }
     }
+}
 
-    if (chatNotificationsReceivedCount < _lastChatNotificationsReceivedCount) {
-        _activityModel->checkAndRemoveSeenActivities(toNotifyList);
-    }
-    _lastChatNotificationsReceivedCount = chatNotificationsReceivedCount;
+void User::slotNotificationFetchFinished()
+{
+    _isNotificationFetchRunning = false;
 }
 
 void User::slotBuildIncomingCallDialogs(const ActivityList &list)
@@ -440,13 +450,18 @@ void User::slotRefreshNotifications()
     // start a server notification handler if no notification requests
     // are running
     if (_notificationRequestsRunning == 0) {
+        if (_isNotificationFetchRunning) {
+            qCDebug(lcActivity) << "Notification fetch is already running.";
+            return;
+        }
         auto *snh = new ServerNotificationHandler(_account.data());
         connect(snh, &ServerNotificationHandler::newNotificationList,
             this, &User::slotBuildNotificationDisplay);
         connect(snh, &ServerNotificationHandler::newIncomingCallsList,
             this, &User::slotBuildIncomingCallDialogs);
-
-        snh->slotFetchNotifications();
+        connect(snh, &ServerNotificationHandler::jobFinished,
+            this, &User::slotNotificationFetchFinished);
+        _isNotificationFetchRunning = snh->startFetchNotifications();
     } else {
         qCWarning(lcActivity) << "Notification request counter not zero.";
     }

src/gui/tray/usermodel.h

@@ -124,6 +124,7 @@ public slots:
     void slotNotifyServerFinished(const QString &reply, int replyCode);
     void slotSendNotificationRequest(const QString &accountName, const QString &link, const QByteArray &verb, int row);
     void slotBuildNotificationDisplay(const OCC::ActivityList &list);
+    void slotNotificationFetchFinished();
     void slotBuildIncomingCallDialogs(const OCC::ActivityList &list);
     void slotRefreshNotifications();
     void slotRefreshActivitiesInitial();
@@ -163,6 +164,8 @@ private:
     bool notificationAlreadyShown(const long notificationId);
     bool canShowNotification(const long notificationId);
 
+    void checkAndRemoveSeenActivities(const ActivityList &list, const int numChatNotificationsReceived);
+
     AccountStatePtr _account;
     bool _isCurrentUser;
     ActivityListModel *_activityModel;
@@ -184,6 +187,8 @@ private:
     int _notificationRequestsRunning = 0;
 
     int _lastChatNotificationsReceivedCount = 0;
+
+    bool _isNotificationFetchRunning = false;
 };
 
 class UserModel : public QAbstractListModel