Fixed a wrong use of std::move when std::forward should have been use…

…d; improved handling of messages originating from other threads

src/filesystem/fileaccessnextcloud.cpp

@@ -3,6 +3,7 @@
 #include <QLoggingCategory>
 #include <QQueue>
 #include <QXmlStreamReader>
+#include <utility>
 
 using namespace Files;
 using namespace Services;
@@ -34,8 +35,8 @@ auto FileAccessNextcloud::getDataAsync(const QString &path, bool allowCache) ->
     return future.then([future, path](QNetworkReply *reply) {
         if (replyHasError(reply))
         {
-            auto errorMessage = makeAndPrintError(u"Could not get data from %1"_s.arg(path), reply);
-            return deleteReplyAndReturn(FileDataResult(std::move(errorMessage)), reply);
+            return deleteReplyAndReturn(
+                FileDataResult(makeAndPrintError(u"Could not get data from %1"_s.arg(path), reply)), reply);
         }
 
         qCDebug(gmFileAccessNextCloud()) << "Received data from file" << path;
@@ -81,8 +82,9 @@ auto FileAccessNextcloud::saveAsync(const QString &path, const QByteArray &data)
                         reply);
                 }
 
-                auto errorMessage = makeAndPrintError(u"Could not save file %1"_s.arg(path), reply);
-                return deleteReplyAndReturn(QtFuture::makeReadyFuture(FileResult(std::move(errorMessage))), reply);
+                return deleteReplyAndReturn(QtFuture::makeReadyFuture(FileResult(
+                                                makeAndPrintError(u"Could not save file %1"_s.arg(path), reply))),
+                                            reply);
             }
 
             if (!m_cache.createOrUpdateEntry(path, data))
@@ -118,8 +120,9 @@ auto FileAccessNextcloud::moveAsync(const QString &oldPath, const QString &newPa
                         reply);
                 }
 
-                auto errorMessage = makeAndPrintError(u"Could not move file %1 to %2"_s.arg(oldPath, newPath), reply);
-                return deleteReplyAndReturn(QtFuture::makeReadyFuture(FileResult(std::move(errorMessage))), reply);
+                return deleteReplyAndReturn(QtFuture::makeReadyFuture(FileResult(makeAndPrintError(
+                                                u"Could not move file %1 to %2"_s.arg(oldPath, newPath), reply))),
+                                            reply);
             }
 
             if (!m_cache.moveEntry(oldPath, newPath))
@@ -143,8 +146,9 @@ auto FileAccessNextcloud::deleteAsync(const QString &path) -> QFuture<FileResult
         .then([this, future, path](QNetworkReply *reply) {
             if (replyHasError(reply))
             {
-                auto errorMessage = makeAndPrintError(u"Could not delete file/folder %1"_s.arg(path), reply);
-                return deleteReplyAndReturn(QtFuture::makeReadyFuture(FileResult(std::move(errorMessage))), reply);
+                return deleteReplyAndReturn(QtFuture::makeReadyFuture(FileResult(makeAndPrintError(
+                                                u"Could not delete file/folder %1"_s.arg(path), reply))),
+                                            reply);
             }
 
             m_cache.removeEntry(path);
@@ -177,8 +181,9 @@ auto FileAccessNextcloud::copyAsync(const QString &path, const QString &copy) ->
                         reply);
                 }
 
-                auto errorMessage = makeAndPrintError(u"Could not copy %1 to %2: %3 %4"_s.arg(path), reply);
-                return deleteReplyAndReturn(QtFuture::makeReadyFuture(FileResult(std::move(errorMessage))), reply);
+                return deleteReplyAndReturn(QtFuture::makeReadyFuture(FileResult(makeAndPrintError(
+                                                u"Could not copy %1 to %2: %3 %4"_s.arg(path), reply))),
+                                            reply);
             }
 
             if (!m_cache.copyEntry(path, copy))
@@ -204,8 +209,9 @@ auto FileAccessNextcloud::listAsync(const QString &path, bool files, bool folder
     return future.then([future, path, files, folders](QNetworkReply *reply) {
         if (replyHasError(reply))
         {
-            auto errorMessage = makeAndPrintError(u"Could not list content of folder %1"_s.arg(path), reply);
-            return deleteReplyAndReturn(FileListResult(path, std::move(errorMessage)), reply);
+            return deleteReplyAndReturn(
+                FileListResult(path, makeAndPrintError(u"Could not list content of folder %1"_s.arg(path), reply)),
+                reply);
         }
 
         qCDebug(gmFileAccessNextCloud()) << "Successfully received content of" << path;
@@ -277,8 +283,8 @@ auto FileAccessNextcloud::createDirAsync(const QString &path) -> QFuture<FileRes
     return future.then([future, path](QNetworkReply *reply) {
         if (replyHasError(reply))
         {
-            auto errorMessage = makeAndPrintError(u"Could not create directory %1"_s.arg(path), reply);
-            return deleteReplyAndReturn(FileResult(std::move(errorMessage)), reply);
+            return deleteReplyAndReturn(
+                FileResult(makeAndPrintError(u"Could not create directory %1"_s.arg(path), reply)), reply);
         }
 
         qCDebug(gmFileAccessNextCloud()) << "Successfully created directory" << path;
@@ -304,8 +310,9 @@ auto FileAccessNextcloud::checkAsync(const QString &path, bool allowCache) -> QF
 
         if (const auto hasError = replyHasError(reply) && doesExist; hasError)
         {
-            auto errorMessage = makeAndPrintError(u"Could not check if file %1 exists"_s.arg(path), reply);
-            return deleteReplyAndReturn(FileCheckResult(path, std::move(errorMessage)), reply);
+            return deleteReplyAndReturn(
+                FileCheckResult(path, makeAndPrintError(u"Could not check if file %1 exists"_s.arg(path), reply)),
+                reply);
         }
 
         qCDebug(gmFileAccessNextCloud()) << "PROPFIND:" << reply->readAll();
@@ -352,5 +359,5 @@ auto FileAccessNextcloud::makeMoveHeaders(const QString &newPath) -> QList<std::
 template <typename T> auto FileAccessNextcloud::deleteReplyAndReturn(T &&value, QNetworkReply *reply) -> T
 {
     QScopedPointer scopedReply(reply);
-    return std::move(value);
+    return std::forward<T>(value);
 }

src/messages/message.h

@@ -10,7 +10,7 @@ struct Message
     {
     }
 
-    explicit Message(QDateTime &timestamp, QtMsgType type, QString &category, QString &body)
+    explicit Message(QDateTime &&timestamp, QtMsgType type, QString &&category, QString &&body)
         : timestamp(std::move(timestamp)), type(type), category(std::move(category)), body(std::move(body))
     {
     }

src/messages/messagemanager.cpp

@@ -1,5 +1,4 @@
 #include "messagemanager.h"
-#include <QMutexLocker>
 
 auto MessageManager::instance() -> MessageManager *
 {
@@ -11,23 +10,20 @@ auto MessageManager::instance() -> MessageManager *
     return single;
 }
 
+/// Thread safe
 void MessageManager::addMessage(const QDateTime &timestamp, QtMsgType type, const QString &category,
                                 const QString &body)
 {
-    addMessage(std::make_shared<Message>(timestamp, type, category, body));
+    auto message = std::make_shared<Message>(timestamp, type, category, body);
+
+    QMetaObject::invokeMethod(
+        instance(), [message = std::move(message)]() { instance()->addMessage(std::move(message)); },
+        Qt::ConnectionType::AutoConnection);
 }
 
+/// Not thread safe
 void MessageManager::addMessage(std::shared_ptr<Message> message)
 {
-    if (QThread::currentThread() != this->thread())
-    {
-        QTimer::singleShot(std::chrono::milliseconds::zero(), this,
-                           [this, message = std::move(message)]() { addMessage(std::move(message)); });
-        return;
-    }
-
-    QMutexLocker const lock(&m_mutex); // lock for messages from different threads
-
     // filter errors
     if (message->type > QtMsgType::QtDebugMsg && message->type < QtMsgType::QtInfoMsg)
     {

src/messages/messagemanager.h

@@ -5,7 +5,6 @@
 #include "qmlsingletonfactory.h"
 #include "thirdparty/propertyhelper/PropertyHelper.h"
 #include <QJSEngine>
-#include <QMutex>
 #include <QObject>
 #include <QQmlEngine>
 #include <QtQml/qqmlregistration.h>
@@ -27,17 +26,17 @@ class MessageManager : public QObject
     Q_INVOKABLE void markAllAsRead();
     Q_INVOKABLE void clearMessages();
 
+    void addMessage(const QDateTime &timestamp, QtMsgType type, const QString &category, const QString &body);
+
     AUTO_PROPERTY_VAL2(bool, hasNewErrors, false)
 
 public slots:
-    void addMessage(const QDateTime &timestamp, QtMsgType type, const QString &category, const QString &body);
     void addMessage(std::shared_ptr<Message> message);
 
 private:
     using QObject::QObject;
 
     inline static MessageManager *single = nullptr;
 
-    QMutex m_mutex;
     MessageModel m_model = MessageModel(nullptr);
 };

tests/messages/testmessagemanager.cpp

@@ -47,10 +47,15 @@ TEST(MessageManagerTest, CanAddMessagesFromDifferentThreads)
     EXPECT_EQ(MessageManager::instance()->messages()->rowCount(), 1);
 
     QtConcurrent::run(QThreadPool::globalInstance(), []() {
+        // via implicit move
         MessageManager::instance()->addMessage(QDateTime(), QtMsgType::QtWarningMsg, u"category1"_s, u"message1"_s);
 
-        auto message2 = std::make_shared<Message>(QDateTime(), QtMsgType::QtWarningMsg, u"category2"_s, u"message2"_s);
-        MessageManager::instance()->addMessage(std::move(message2));
+        // via const ref
+        auto ts = QDateTime();
+        auto type = QtMsgType::QtWarningMsg;
+        auto category = u"category2"_s;
+        auto message = u"message2"_s;
+        MessageManager::instance()->addMessage(ts, type, category, message);
     }).waitForFinished();
 
     QTest::qWait(50);