Review comments. Part I.

Signed-off-by: alex-z <[email protected]>
nextcloud · Dec 6, 2023 · 07e0504 · 07e0504
1 parent ac03970
commit 07e0504
Show file tree

Hide file tree

Showing 9 changed files with 44 additions and 66 deletions.
diff --git a/src/gui/tray/NCBusyIndicator.qml b/src/gui/tray/NCBusyIndicator.qml
@@ -42,7 +42,7 @@ BusyIndicator {
 
         RotationAnimator {
             target: contentImage
-            running: false
+            running: root.running
             onRunningChanged: contentImage.rotation = 0
             from: 0
             to: 360

diff --git a/src/libsync/CMakeLists.txt b/src/libsync/CMakeLists.txt
@@ -27,7 +27,6 @@ set(libsync_SRCS
     clientstatusreporting.h
     clientstatusreporting.cpp
     clientstatusreportingrecord.h
-    clientstatusreportingrecord.cpp
     cookiejar.h
     cookiejar.cpp
     discovery.h

diff --git a/src/libsync/account.cpp b/src/libsync/account.cpp
@@ -286,19 +286,17 @@ void Account::setPushNotificationsReconnectInterval(int interval)
 
 void Account::trySetupClientStatusReporting()
 {
-    if (_capabilities.isClientStatusReportingEnabled()) {
-        if (!_clientStatusReporting) {
-            _clientStatusReporting.reset(new ClientStatusReporting(this));
-        }
+    if (!_capabilities.isClientStatusReportingEnabled()) {
+        _clientStatusReporting.reset();
         return;
     }
 
     if (!_clientStatusReporting) {
-        _clientStatusReporting.reset();
+        _clientStatusReporting = std::make_unique<ClientStatusReporting>(this);
     }
 }
 
-void Account::reportClientStatus(const ClientStatusReporting::Status status)
+void Account::reportClientStatus(const ClientStatusReporting::Status status) const
 {
     if (_clientStatusReporting) {
         _clientStatusReporting->reportClientStatus(status);

diff --git a/src/libsync/account.h b/src/libsync/account.h
@@ -308,7 +308,7 @@ class OWNCLOUDSYNC_EXPORT Account : public QObject
 
     void trySetupClientStatusReporting();
 
-    void reportClientStatus(const ClientStatusReporting::Status status);
+    void reportClientStatus(const ClientStatusReporting::Status status) const;
 
     [[nodiscard]] std::shared_ptr<UserStatusConnector> userStatusConnector() const;
 
@@ -444,7 +444,7 @@ private slots:
 
     PushNotifications *_pushNotifications = nullptr;
 
-    QScopedPointer<ClientStatusReporting> _clientStatusReporting;
+    std::unique_ptr<ClientStatusReporting> _clientStatusReporting;
 
     std::shared_ptr<UserStatusConnector> _userStatusConnector;
 

diff --git a/src/libsync/clientstatusreporting.cpp b/src/libsync/clientstatusreporting.cpp
@@ -52,7 +52,6 @@ void ClientStatusReporting::init()
 {
     Q_ASSERT(!_isInitialized);
     if (_isInitialized) {
-        qCDebug(lcClientStatusReporting) << "Double call to init";
         return;
     }
 
@@ -82,30 +81,12 @@ void ClientStatusReporting::init()
         return;
     }
 
-    if (!query.prepare(QStringLiteral("CREATE INDEX IF NOT EXISTS name ON clientstatusreporting(name);")) || !query.exec()) {
-        qCDebug(lcClientStatusReporting) << "Could not create index on clientstatusreporting table:" << query.lastError().text();
-        return;
-    }
-
     if (!query.prepare(QStringLiteral("CREATE TABLE IF NOT EXISTS keyvalue(key VARCHAR(4096), value VARCHAR(4096), PRIMARY KEY(key))")) || !query.exec()) {
         qCDebug(lcClientStatusReporting) << "Could not setup client keyvalue table:" << query.lastError().text();
         return;
     }
 
-    // prevent issues in case enum gets changed in future, hash its value and clean the db in case there was a change
-    QByteArray statusNamesContatenated;
-    for (int i = 0; i < ClientStatusReporting::Status::Count; ++i) {
-        statusNamesContatenated += statusStringFromNumber(static_cast<Status>(i));
-    }
-    statusNamesContatenated += QByteArray::number(ClientStatusReporting::Status::Count);
-    const auto statusNamesHashCurrent = QCryptographicHash::hash(statusNamesContatenated, QCryptographicHash::Md5).toHex();
-    const auto statusNamesHashFromDb = getStatusNamesHash();
-
-    if (statusNamesHashCurrent != statusNamesHashFromDb) {
-        deleteClientStatusReportingRecords();
-        setStatusNamesHash(statusNamesHashCurrent);
-    }
-    //
+    updateStatusNamesHash();
 
     _clientStatusReportingSendTimer.setInterval(clientStatusReportingTrySendTimerInterval);
     connect(&_clientStatusReportingSendTimer, &QTimer::timeout, this, &ClientStatusReporting::sendReportToServer);
@@ -212,7 +193,6 @@ void ClientStatusReporting::sendReportToServer()
 
     const auto report = prepareReport();
     if (report.isEmpty()) {
-        qCDebug(lcClientStatusReporting) << "Failed to generate report. Report is empty.";
         return;
     }
 
@@ -250,6 +230,22 @@ QString ClientStatusReporting::makeDbPath() const
     return ConfigFile().configPath() + QStringLiteral(".userdata_%1.db").arg(QString::fromLatin1(databaseIdHash.left(6).toHex()));
 }
 
+void ClientStatusReporting::updateStatusNamesHash()
+{
+    QByteArray statusNamesContatenated;
+    for (int i = 0; i < ClientStatusReporting::Status::Count; ++i) {
+        statusNamesContatenated += statusStringFromNumber(static_cast<Status>(i));
+    }
+    statusNamesContatenated += QByteArray::number(ClientStatusReporting::Status::Count);
+    const auto statusNamesHashCurrent = QCryptographicHash::hash(statusNamesContatenated, QCryptographicHash::Md5).toHex();
+    const auto statusNamesHashFromDb = getStatusNamesHash();
+
+    if (statusNamesHashCurrent != statusNamesHashFromDb) {
+        deleteClientStatusReportingRecords();
+        setStatusNamesHash(statusNamesHashCurrent);
+    }
+}
+
 quint64 ClientStatusReporting::getLastSentReportTimestamp() const
 {
     QMutexLocker locker(&_mutex);

diff --git a/src/libsync/clientstatusreporting.h b/src/libsync/clientstatusreporting.h
@@ -16,15 +16,18 @@
 #include "owncloudlib.h"
 #include <common/result.h>
 
-#include <QtGlobal>
-#include <QByteArray>
-#include <QHash>
-#include <QObject>
-#include <QPair>
-#include <QRecursiveMutex>
-#include <QString>
-#include <QTimer>
-#include <QtSql>
+#include <QtCore/qglobal.h>
+#include <QtCore/qbytearray.h>
+#include <QtCore/qhash.h>
+#include <QtCore/qobject.h>
+#include <QtCore/qmutex.h>
+#include <QtCore/qpair.h>
+#include <QtCore/qstring.h>
+#include <QtCore/qtimer.h>
+#include <QtSql/qsqldatabase.h>
+#include <QtSql/qsqlerror.h>
+#include <QtSql/qsqlrecord.h>
+#include <QtSql/qsqlquery.h>
 
 namespace OCC {
 
@@ -52,6 +55,7 @@ class OWNCLOUDSYNC_EXPORT ClientStatusReporting : public QObject
         UploadError_Virus_Detected,
         Count,
     };
+    Q_ENUM(Status);
 
     explicit ClientStatusReporting(Account *account, QObject *parent = nullptr);
     ~ClientStatusReporting() override;
@@ -78,6 +82,8 @@ class OWNCLOUDSYNC_EXPORT ClientStatusReporting : public QObject
 
     [[nodiscard]] QString makeDbPath() const;
 
+    void updateStatusNamesHash();
+
 private slots:
     void sendReportToServer();
 

diff --git a/src/libsync/clientstatusreportingrecord.cpp b/src/libsync/clientstatusreportingrecord.cpp
diff --git a/src/libsync/clientstatusreportingrecord.h b/src/libsync/clientstatusreportingrecord.h
@@ -30,6 +30,9 @@ struct OWNCLOUDSYNC_EXPORT ClientStatusReportingRecord {
     quint64 _numOccurences = 1;
     quint64 _lastOccurence = 0;
 
-    [[nodiscard]] bool isValid() const;
+    [[nodiscard]] inline bool isValid() const
+    {
+        return _status >= 0 && !_name.isEmpty() && _lastOccurence > 0;
+    }
 };
 }
diff --git a/src/libsync/owncloudpropagator_p.h b/src/libsync/owncloudpropagator_p.h
@@ -63,7 +63,7 @@ inline QByteArray getEtagFromReply(QNetworkReply *reply)
     return ret;
 }
 
-inline QPair<QByteArray, QByteArray> getExceptionFromReply(QNetworkReply *reply)
+inline QPair<QByteArray, QByteArray> getExceptionFromReply(QNetworkReply * const reply)
 {
     Q_ASSERT(reply);
     if (!reply) {