Fix notifications not being shown when they should be

Signed-off-by: Claudio Cambra <claudio.cambra@gmail.com>

6 files changed, 27 insertions, 72 deletions

diff --git a/src/gui/CMakeLists.txt b/src/gui/CMakeLists.txt
index c7fb8bc59..920996836 100644
--- a/src/gui/CMakeLists.txt
+++ b/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
diff --git a/src/gui/owncloudgui.cpp b/src/gui/owncloudgui.cpp
index 402e3801d..aa5066d60 100644
--- a/src/gui/owncloudgui.cpp
+++ b/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)
diff --git a/src/gui/tray/notificationcache.cpp b/src/gui/tray/notificationcache.cpp
deleted file mode 100644
index f60c2457d..000000000
--- a/src/gui/tray/notificationcache.cpp
+++ /dev/null
@@ -1,24 +0,0 @@
-#include "notificationcache.h"
-
-namespace OCC {
-
-bool NotificationCache::contains(const Notification &notification) const
-{
-    return _notifications.find(calculateKey(notification)) != _notifications.end();
-}
-
-void NotificationCache::insert(const Notification &notification)
-{
-    _notifications.insert(calculateKey(notification));
-}
-
-void NotificationCache::clear()
-{
-    _notifications.clear();
-}
-
-uint NotificationCache::calculateKey(const Notification &notification) const
-{
-    return qHash(notification.title + notification.message);
-}
-}
diff --git a/src/gui/tray/notificationcache.h b/src/gui/tray/notificationcache.h
deleted file mode 100644
index 65caff611..000000000
--- a/src/gui/tray/notificationcache.h
+++ /dev/null
@@ -1,28 +0,0 @@
-#pragma once
-
-#include <QSet>
-
-namespace OCC {
-
-class NotificationCache
-{
-public:
-    struct Notification
-    {
-        QString title;
-        QString message;
-    };
-
-    bool contains(const Notification &notification) const;
-
-    void insert(const Notification &notification);
-
-    void clear();
-
-private:
-    uint calculateKey(const Notification &notification) const;
-
-
-    QSet<uint> _notifications;
-};
-}
diff --git a/src/gui/tray/usermodel.cpp b/src/gui/tray/usermodel.cpp
index d7a817748..d3117b07c 100644
--- a/src/gui/tray/usermodel.cpp
+++ b/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);
         }
diff --git a/src/gui/tray/usermodel.h b/src/gui/tray/usermodel.h
index 10c9cd1a9..afe7fe6d4 100644
--- a/src/gui/tray/usermodel.h
+++ b/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 @@ public:
     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 @@ private:
     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 @@ private:
     QHash<AccountState *, QElapsedTimer> _timeSinceLastCheck;
 
     QElapsedTimer _guiLogTimer;
-    NotificationCache _notificationCache;
+    QSet<long> _notifiedNotifications;
     QMimeDatabase _mimeDb;
 
     // number of currently running notification requests. If non zero,