From b6def519457239bff6d5dcfcf7ccf7d99d2771d0 Mon Sep 17 00:00:00 2001 From: Claudio Cambra Date: Mon, 9 May 2022 18:07:01 +0200 Subject: [PATCH] Fix notifications not being shown when they should be Signed-off-by: Claudio Cambra --- src/gui/CMakeLists.txt | 2 -- src/gui/owncloudgui.cpp | 8 ++++-- src/gui/tray/notificationcache.cpp | 24 ------------------ src/gui/tray/notificationcache.h | 28 --------------------- src/gui/tray/usermodel.cpp | 31 ++++++++++++++--------- src/gui/tray/usermodel.h | 6 ++--- test/CMakeLists.txt | 1 - test/testnotificationcache.cpp | 40 ------------------------------ 8 files changed, 27 insertions(+), 113 deletions(-) delete mode 100644 src/gui/tray/notificationcache.cpp delete mode 100644 src/gui/tray/notificationcache.h delete mode 100644 test/testnotificationcache.cpp diff --git a/src/gui/CMakeLists.txt b/src/gui/CMakeLists.txt index c7fb8bc597d4c..9209968367385 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 402e3801d35bf..aa5066d608e76 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 f60c2457d6683..0000000000000 --- a/src/gui/tray/notificationcache.cpp +++ /dev/null @@ -1,24 +0,0 @@ -#include "notificationcache.h" - -namespace OCC { - -bool NotificationCache::contains(const Notification ¬ification) const -{ - return _notifications.find(calculateKey(notification)) != _notifications.end(); -} - -void NotificationCache::insert(const Notification ¬ification) -{ - _notifications.insert(calculateKey(notification)); -} - -void NotificationCache::clear() -{ - _notifications.clear(); -} - -uint NotificationCache::calculateKey(const Notification ¬ification) 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 65caff61136ad..0000000000000 --- a/src/gui/tray/notificationcache.h +++ /dev/null @@ -1,28 +0,0 @@ -#pragma once - -#include - -namespace OCC { - -class NotificationCache -{ -public: - struct Notification - { - QString title; - QString message; - }; - - bool contains(const Notification ¬ification) const; - - void insert(const Notification ¬ification); - - void clear(); - -private: - uint calculateKey(const Notification ¬ification) const; - - - QSet _notifications; -}; -} diff --git a/src/gui/tray/usermodel.cpp b/src/gui/tray/usermodel.cpp index d7a817748e17a..d3117b07c0608 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(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(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 10c9cd1a9b493..afe7fe6d4b4d7 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 @@ -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 _timeSinceLastCheck; QElapsedTimer _guiLogTimer; - NotificationCache _notificationCache; + QSet _notifiedNotifications; QMimeDatabase _mimeDb; // number of currently running notification requests. If non zero, diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index b119c384d3cec..a6e6304c905cc 100644 --- a/test/CMakeLists.txt +++ b/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) diff --git a/test/testnotificationcache.cpp b/test/testnotificationcache.cpp deleted file mode 100644 index eba7e3f4d5269..0000000000000 --- a/test/testnotificationcache.cpp +++ /dev/null @@ -1,40 +0,0 @@ -#include - -#include "tray/notificationcache.h" - -class TestNotificationCache : public QObject -{ - Q_OBJECT - -private slots: - void testContains_doesNotContainNotification_returnsFalse() - { - OCC::NotificationCache notificationCache; - - QVERIFY(!notificationCache.contains({ "Title", { "Message" } })); - } - - void testContains_doesContainNotification_returnTrue() - { - OCC::NotificationCache notificationCache; - const OCC::NotificationCache::Notification notification { "Title", "message" }; - - notificationCache.insert(notification); - - QVERIFY(notificationCache.contains(notification)); - } - - void testClear_doesContainNotification_clearNotifications() - { - OCC::NotificationCache notificationCache; - const OCC::NotificationCache::Notification notification { "Title", "message" }; - - notificationCache.insert(notification); - notificationCache.clear(); - - QVERIFY(!notificationCache.contains(notification)); - } -}; - -QTEST_GUILESS_MAIN(TestNotificationCache) -#include "testnotificationcache.moc"