Merge pull request #4546 from nextcloud/backport/4508/stable-3.5

[stable-3.5] Simplify and remove the notification "cache"

src/gui/CMakeLists.txt

@@ -204,8 +204,6 @@ set(client_SRCS
     tray/usermodel.cpp
     tray/notificationhandler.h
     tray/notificationhandler.cpp
-    tray/notificationcache.h
-    tray/notificationcache.cpp
     creds/credentialsfactory.h
     tray/talkreply.cpp
     creds/credentialsfactory.cpp

src/gui/owncloudgui.cpp

@@ -52,6 +52,8 @@
 
 namespace OCC {
 
+Q_LOGGING_CATEGORY(lcOwnCloudGui, "com.nextcloud.owncloudgui")
+
 const char propertyAccountC[] = "oc_account";
 
 ownCloudGui::ownCloudGui(Application *parent)
@@ -373,10 +375,12 @@ void ownCloudGui::hideAndShowTray()
 
 void ownCloudGui::slotShowTrayMessage(const QString &title, const QString &msg)
 {
-    if (_tray)
+    qCDebug(lcOwnCloudGui) << "Going to show notification with title: '" << title << "' and message: '" << msg << "'";
+    if (_tray) {
         _tray->showMessage(title, msg);
-    else
+    } else {
         qCWarning(lcApplication) << "Tray not ready: " << msg;
+    }
 }
 
 void ownCloudGui::slotShowTrayUpdateMessage(const QString &title, const QString &msg, const QUrl &webUrl)

src/gui/tray/usermodel.cpp

@@ -14,7 +14,6 @@
 #include "syncfileitem.h"
 #include "systray.h"
 #include "tray/activitylistmodel.h"
-#include "tray/notificationcache.h"
 #include "tray/unifiedsearchresultslistmodel.h"
 #include "tray/talkreply.h"
 #include "userstatusconnector.h"
@@ -70,8 +69,6 @@ User::User(AccountStatePtr &account, const bool &isCurrent, QObject *parent)
 
     connect(FolderMan::instance(), &FolderMan::folderListChanged, this, &User::hasLocalFolderChanged);
 
-    connect(this, &User::guiLog, Logger::instance(), &Logger::guiLog);
-
     connect(_account->account().data(), &Account::accountChangedAvatar, this, &User::avatarChanged);
     connect(_account->account().data(), &Account::userStatusChanged, this, &User::statusChanged);
     connect(_account.data(), &AccountState::desktopNotificationsAllowedChanged, this, &User::desktopNotificationsAllowedChanged);
@@ -85,8 +82,15 @@ User::User(AccountStatePtr &account, const bool &isCurrent, QObject *parent)
     connect(this, &User::sendReplyMessage, this, &User::slotSendReplyMessage);
 }
 
-void User::showDesktopNotification(const QString &title, const QString &message)
+void User::showDesktopNotification(const QString &title, const QString &message, const long notificationId)
 {
+    // Notification ids are uints, which are 4 bytes. Error activities don't have ids, however, so we generate one.
+    // To avoid possible collisions between the activity ids which are actually the notification ids received from
+    // the server (which are always positive) and our "fake" error activity ids, we assign a negative id to the
+    // error notification.
+    //
+    // To ensure that we can still treat an unsigned int as normal, we use a long, which is 8 bytes.
+
     ConfigFile cfg;
     if (!cfg.optionalServerNotifications() || !isDesktopNotificationsAllowed()) {
         return;
@@ -95,16 +99,15 @@ void User::showDesktopNotification(const QString &title, const QString &message)
     // after one hour, clear the gui log notification store
     constexpr qint64 clearGuiLogInterval = 60 * 60 * 1000;
     if (_guiLogTimer.elapsed() > clearGuiLogInterval) {
-        _notificationCache.clear();
+        _notifiedNotifications.clear();
     }
 
-    const NotificationCache::Notification notification { title, message };
-    if (_notificationCache.contains(notification)) {
+    if (_notifiedNotifications.contains(notificationId)) {
         return;
     }
 
-    _notificationCache.insert(notification);
-    emit guiLog(notification.title, notification.message);
+    _notifiedNotifications.insert(notificationId);
+    Logger::instance()->postGuiLog(title, message);
     // restart the gui log timer now that we show a new notification
     _guiLogTimer.start();
 }
@@ -119,7 +122,7 @@ void User::slotBuildNotificationDisplay(const ActivityList &list)
             continue;
         }
         const auto message = AccountManager::instance()->accounts().count() == 1 ? "" : activity._accName;
-        showDesktopNotification(activity._subject, message);
+        showDesktopNotification(activity._subject, message, activity._id); // We assigned the notif. id to the activity id
         _activityModel->addNotificationToActivityList(activity);
     }
 }
@@ -502,10 +505,13 @@ void User::slotAddErrorToGui(const QString &folderAlias, SyncFileItem::Status st
         activity._accName = folderInstance->accountState()->account()->displayName();
         activity._folder = folderAlias;
 
+        // Error notifications don't have ids by themselves so we will create one for it
+        activity._id = -static_cast<int>(qHash(activity._subject + activity._message));
+
         // add 'other errors' to activity list
         _activityModel->addErrorToActivityList(activity);
 
-        showDesktopNotification(activity._subject, activity._message);
+        showDesktopNotification(activity._subject, activity._message, activity._id);
 
         if (!_expiredActivitiesCheckTimer.isActive()) {
             _expiredActivitiesCheckTimer.start(expiredActivitiesCheckIntervalMsecs);
@@ -607,13 +613,14 @@ void User::processCompletedSyncItem(const Folder *folder, const SyncFileItemPtr
         qCWarning(lcActivity) << "Item " << item->_file << " retrieved resulted in error " << item->_errorString;
 
         activity._subject = item->_errorString;
+        activity._id = -static_cast<int>(qHash(activity._subject + activity._message));
 
         if (item->_status == SyncFileItem::Status::FileIgnored) {
             _activityModel->addIgnoredFileToList(activity);
         } else {
             // add 'protocol error' to activity list
             if (item->_status == SyncFileItem::Status::FileNameInvalid) {
-                showDesktopNotification(item->_file, activity._subject);
+                showDesktopNotification(item->_file, activity._subject, activity._id);
             }
             _activityModel->addErrorToActivityList(activity);
         }

src/gui/tray/usermodel.h

@@ -12,7 +12,6 @@
 #include "accountfwd.h"
 #include "accountmanager.h"
 #include "folderman.h"
-#include "notificationcache.h"
 #include "userstatusselectormodel.h"
 #include "userstatusconnector.h"
 #include <chrono>
@@ -76,7 +75,6 @@ class User : public QObject
     void processCompletedSyncItem(const Folder *folder, const SyncFileItemPtr &item);
 
 signals:
-    void guiLog(const QString &, const QString &);
     void nameChanged();
     void hasLocalFolderChanged();
     void serverHasTalkChanged();
@@ -124,7 +122,7 @@ public slots:
     bool isActivityOfCurrentAccount(const Folder *folder) const;
     bool isUnsolvableConflict(const SyncFileItemPtr &item) const;
 
-    void showDesktopNotification(const QString &title, const QString &message);
+    void showDesktopNotification(const QString &title, const QString &message, const long notificationId);
 
 private:
     AccountStatePtr _account;
@@ -138,7 +136,7 @@ public slots:
     QHash<AccountState *, QElapsedTimer> _timeSinceLastCheck;
 
     QElapsedTimer _guiLogTimer;
-    NotificationCache _notificationCache;
+    QSet<long> _notifiedNotifications;
     QMimeDatabase _mimeDb;
 
     // number of currently running notification requests. If non zero,

test/CMakeLists.txt

@@ -58,7 +58,6 @@ nextcloud_add_test(Capabilities)
 nextcloud_add_test(PushNotifications)
 nextcloud_add_test(Theme)
 nextcloud_add_test(IconUtils)
-nextcloud_add_test(NotificationCache)
 nextcloud_add_test(SetUserStatusDialog)
 nextcloud_add_test(UnifiedSearchListmodel)
 nextcloud_add_test(ActivityListModel)