Fix crash in _finalList removal loop. Fix incorrect implementation of…

… seen chat notifications removal. Improved notifications fetch code to avoid multiple calls from different threads. Signed-off-by: alex-z <[email protected]>
nextcloud · Jul 10, 2023 · 1a9f4a7 · 1a9f4a7
1 parent 0bb0491
commit 1a9f4a7
Show file tree

Hide file tree

Showing 6 changed files with 70 additions and 27 deletions.
diff --git a/src/gui/tray/activitylistmodel.cpp b/src/gui/tray/activitylistmodel.cpp
@@ -440,6 +440,7 @@ void ActivityListModel::startFetchJob()
 
 void ActivityListModel::setFinalList(const ActivityList &finalList)
 {
+    QMutexLocker locker(&_finalListMutex);
     _finalList = finalList;
 
     emit allConflictsChanged();
@@ -668,14 +669,17 @@ void ActivityListModel::removeActivityFromActivityList(const Activity &activity)
     qCInfo(lcActivity) << "Activity/Notification/Error successfully dismissed: " << activity._subject;
     qCInfo(lcActivity) << "Trying to remove Activity/Notification/Error from view... ";
 
-    const auto index = _finalList.indexOf(activity);
-    if (index != -1) {
-        qCInfo(lcActivity) << "Activity/Notification/Error successfully removed from the list.";
-        qCInfo(lcActivity) << "Updating Activity/Notification/Error view.";
-
-        beginRemoveRows({}, index, index);
-        _finalList.removeAt(index);
-        endRemoveRows();
+    {
+        QMutexLocker locker(&_finalListMutex);
+        const auto index = _finalList.indexOf(activity);
+        if (index != -1) {
+            qCInfo(lcActivity) << "Activity/Notification/Error successfully removed from the list.";
+            qCInfo(lcActivity) << "Updating Activity/Notification/Error view.";
+
+            beginRemoveRows({}, index, index);
+            _finalList.removeAt(index);
+            endRemoveRows();
+        }
     }
 
     if (activity._type != Activity::ActivityType &&
@@ -691,11 +695,18 @@ void ActivityListModel::removeActivityFromActivityList(const Activity &activity)
 
 void ActivityListModel::checkAndRemoveSeenActivities(const OCC::ActivityList &newActivities)
 {
-    for (const auto &activity : _finalList) {
-        if (activity._objectType == QStringLiteral("chat") && !newActivities.contains(activity)) {
-            removeActivityFromActivityList(activity);
+    ActivityList activitiesToRemove;
+    {
+        QMutexLocker locker(&_finalListMutex);
+        for (const auto &activity : _finalList) {
+            if (activity._objectType == QStringLiteral("chat") && !newActivities.contains(activity)) {
+                activitiesToRemove.push_back(activity);
+            }
         }
     }
+    for (const auto &toRemove : activitiesToRemove) {
+        removeActivityFromActivityList(toRemove);
+    }
 }
 
 void ActivityListModel::slotTriggerDefaultAction(const int activityIndex)
@@ -949,7 +960,10 @@ void ActivityListModel::slotRefreshActivityInitial()
 
 void ActivityListModel::slotRemoveAccount()
 {
-    _finalList.clear();
+    {
+        QMutexLocker locker(&_finalListMutex);
+        _finalList.clear();
+    }
     _activityLists.clear();
     _presentedActivities.clear();
     setAndRefreshCurrentlyFetching(false);

diff --git a/src/gui/tray/activitylistmodel.h b/src/gui/tray/activitylistmodel.h
@@ -191,6 +191,7 @@ private slots:
     ActivityList _listOfIgnoredFiles;
     ActivityList _notificationErrorsLists;
     ActivityList _finalList;
+    QMutex _finalListMutex;
 
     QSet<qint64> _presentedActivities;
 

diff --git a/src/gui/tray/notificationhandler.cpp b/src/gui/tray/notificationhandler.cpp
@@ -24,20 +24,20 @@ 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.
     if (_accountState->account()->capabilities().isValid()) {
         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();
 }

diff --git a/src/gui/tray/notificationhandler.h b/src/gui/tray/notificationhandler.h
@@ -18,9 +18,10 @@ class ServerNotificationHandler : public QObject
 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);

diff --git a/src/gui/tray/usermodel.cpp b/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,19 @@ 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()
+{
+    QMutexLocker locker(&_isNotificationFetchRunningMutex);
+    _isNotificationFetchRunning = false;
 }
 
 void User::slotBuildIncomingCallDialogs(const ActivityList &list)
@@ -440,13 +451,19 @@ void User::slotRefreshNotifications()
     // start a server notification handler if no notification requests
     // are running
     if (_notificationRequestsRunning == 0) {
+        QMutexLocker locker(&_isNotificationFetchRunningMutex);
+        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.";
     }

diff --git a/src/gui/tray/usermodel.h b/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 slots:
     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,9 @@ private slots:
     int _notificationRequestsRunning = 0;
 
     int _lastChatNotificationsReceivedCount = 0;
+
+    QMutex _isNotificationFetchRunningMutex;
+    bool _isNotificationFetchRunning = false;
 };
 
 class UserModel : public QAbstractListModel