Room::postFile(): adjust to the changed RoomMessageEvent API

9a5fa62 dropped one of RoomMessageEvent constructors for Qt 6 in order to address #483 - breaking the build with Qt 6 along the way, as Room::postFile() relied on that constructor. This commit changes Room::postFile() in turn, deprecating the current signature and adding a new one that accepts an EventContent object rather than a path to a file. In order to achieve that, FileInfo and ImageInfo classes have gained new constructors that accept QFileInfo instead of the legacy series of parameters, streamlining usage of EventContent structures.
quotient-im · Jul 18, 2021 · 004ebf8 · 004ebf8
1 parent e3bdbc8
commit 004ebf8
Show file tree

Hide file tree

Showing 6 changed files with 128 additions and 69 deletions.
diff --git a/lib/events/eventcontent.cpp b/lib/events/eventcontent.cpp
@@ -5,10 +5,13 @@
 
 #include "converters.h"
 #include "util.h"
+#include "logging.h"
 
 #include <QtCore/QMimeDatabase>
+#include <QtCore/QFileInfo>
 
 using namespace Quotient::EventContent;
+using std::move;
 
 QJsonObject Base::toJson() const
 {
@@ -17,22 +20,37 @@ QJsonObject Base::toJson() const
     return o;
 }
 
-FileInfo::FileInfo(const QUrl& u, qint64 payloadSize, const QMimeType& mimeType,
-                   const QString& originalFilename)
+FileInfo::FileInfo(const QFileInfo &fi)
+    : mimeType(QMimeDatabase().mimeTypeForFile(fi))
+    , url(QUrl::fromLocalFile(fi.filePath()))
+    , payloadSize(fi.size())
+    , originalName(fi.fileName())
+{
+    Q_ASSERT(fi.isFile());
+}
+
+FileInfo::FileInfo(QUrl u, qint64 payloadSize, const QMimeType& mimeType,
+                   QString originalFilename)
     : mimeType(mimeType)
-    , url(u)
+    , url(move(u))
     , payloadSize(payloadSize)
-    , originalName(originalFilename)
-{}
+    , originalName(move(originalFilename))
+{
+    if (!isValid())
+        qCWarning(MESSAGES)
+            << "To client developers: using FileInfo(QUrl, qint64, ...) "
+               "constructor for non-mxc resources is deprecated since Quotient "
+               "0.7; for local resources, use FileInfo(QFileInfo) instead";
+}
 
-FileInfo::FileInfo(const QUrl& u, const QJsonObject& infoJson,
-                   const QString& originalFilename)
+FileInfo::FileInfo(QUrl mxcUrl, const QJsonObject& infoJson,
+                   QString originalFilename)
     : originalInfoJson(infoJson)
     , mimeType(
           QMimeDatabase().mimeTypeForName(infoJson["mimetype"_ls].toString()))
-    , url(u)
+    , url(move(mxcUrl))
     , payloadSize(fromJson<qint64>(infoJson["size"_ls]))
-    , originalName(originalFilename)
+    , originalName(move(originalFilename))
 {
     if (!mimeType.isValid())
         mimeType = QMimeDatabase().mimeTypeForData(QByteArray());
@@ -53,14 +71,19 @@ void FileInfo::fillInfoJson(QJsonObject* infoJson) const
         infoJson->insert(QStringLiteral("mimetype"), mimeType.name());
 }
 
-ImageInfo::ImageInfo(const QUrl& u, qint64 fileSize, QMimeType mimeType,
-                     const QSize& imageSize, const QString& originalFilename)
-    : FileInfo(u, fileSize, mimeType, originalFilename), imageSize(imageSize)
+ImageInfo::ImageInfo(const QFileInfo& fi, QSize imageSize)
+    : FileInfo(fi), imageSize(imageSize)
+{}
+
+ImageInfo::ImageInfo(const QUrl& mxcUrl, qint64 fileSize, const QMimeType& type,
+                     QSize imageSize, const QString& originalFilename)
+    : FileInfo(mxcUrl, fileSize, type, originalFilename)
+    , imageSize(imageSize)
 {}
 
-ImageInfo::ImageInfo(const QUrl& u, const QJsonObject& infoJson,
+ImageInfo::ImageInfo(const QUrl& mxcUrl, const QJsonObject& infoJson,
                      const QString& originalFilename)
-    : FileInfo(u, infoJson, originalFilename)
+    : FileInfo(mxcUrl, infoJson, originalFilename)
     , imageSize(infoJson["w"_ls].toInt(), infoJson["h"_ls].toInt())
 {}
 

diff --git a/lib/events/eventcontent.h b/lib/events/eventcontent.h
@@ -10,7 +10,8 @@
 #include <QtCore/QMimeType>
 #include <QtCore/QSize>
 #include <QtCore/QUrl>
-#include <QtCore/QMetaType>
+
+class QFileInfo;
 
 namespace Quotient {
 namespace EventContent {
@@ -73,11 +74,13 @@ namespace EventContent {
      */
     class FileInfo {
     public:
-        explicit FileInfo(const QUrl& u, qint64 payloadSize = -1,
+        FileInfo() = default;
+        explicit FileInfo(const QFileInfo& fi);
+        explicit FileInfo(QUrl mxcUrl, qint64 payloadSize = -1,
                           const QMimeType& mimeType = {},
-                          const QString& originalFilename = {});
-        FileInfo(const QUrl& u, const QJsonObject& infoJson,
-                 const QString& originalFilename = {});
+                          QString originalFilename = {});
+        FileInfo(QUrl mxcUrl, const QJsonObject& infoJson,
+                 QString originalFilename = {});
 
         bool isValid() const;
 
@@ -113,10 +116,12 @@ namespace EventContent {
      */
     class ImageInfo : public FileInfo {
     public:
-        explicit ImageInfo(const QUrl& u, qint64 fileSize = -1,
-                           QMimeType mimeType = {}, const QSize& imageSize = {},
+        ImageInfo() = default;
+        explicit ImageInfo(const QFileInfo& fi, QSize imageSize = {});
+        explicit ImageInfo(const QUrl& mxcUrl, qint64 fileSize = -1,
+                           const QMimeType& type = {}, QSize imageSize = {},
                            const QString& originalFilename = {});
-        ImageInfo(const QUrl& u, const QJsonObject& infoJson,
+        ImageInfo(const QUrl& mxcUrl, const QJsonObject& infoJson,
                   const QString& originalFilename = {});
 
         void fillInfoJson(QJsonObject* infoJson) const;
@@ -134,7 +139,7 @@ namespace EventContent {
      */
     class Thumbnail : public ImageInfo {
     public:
-        Thumbnail() : ImageInfo(QUrl()) {} // To allow empty thumbnails
+        Thumbnail() = default; // Allow empty thumbnails
         Thumbnail(const QJsonObject& infoJson);
         Thumbnail(const ImageInfo& info) : ImageInfo(info) {}
         using ImageInfo::ImageInfo;

diff --git a/lib/events/roomavatarevent.h b/lib/events/roomavatarevent.h
@@ -20,12 +20,12 @@ class RoomAvatarEvent : public StateEvent<EventContent::ImageContent> {
         : StateEvent(typeId(), matrixTypeId(), QString(), avatar)
     {}
     // A replica of EventContent::ImageInfo constructor
-    explicit RoomAvatarEvent(const QUrl& u, qint64 fileSize = -1,
+    explicit RoomAvatarEvent(const QUrl& mxcUrl, qint64 fileSize = -1,
                              QMimeType mimeType = {},
                              const QSize& imageSize = {},
                              const QString& originalFilename = {})
         : RoomAvatarEvent(EventContent::ImageContent {
-            u, fileSize, mimeType, imageSize, originalFilename })
+            mxcUrl, fileSize, mimeType, imageSize, originalFilename })
     {}
 
     QUrl url() const { return content().url; }

diff --git a/lib/room.cpp b/lib/room.cpp
@@ -306,6 +306,8 @@ class Room::Private {
         return sendEvent(makeEvent<EventT>(std::forward<ArgTs>(eventArgs)...));
     }
 
+    QString doPostFile(RoomEventPtr &&msgEvent, const QUrl &localUrl);
+
     RoomEvent* addAsPending(RoomEventPtr&& event);
 
     QString doSendEvent(const RoomEvent* pEvent);
@@ -1756,57 +1758,80 @@ QString Room::postReaction(const QString& eventId, const QString& key)
     return d->sendEvent<ReactionEvent>(EventRelation::annotate(eventId, key));
 }
 
-QString Room::postFile(const QString& plainText, const QUrl& localPath,
-                       bool asGenericFile)
+QString Room::Private::doPostFile(RoomEventPtr&& msgEvent, const QUrl& localUrl)
 {
-    QFileInfo localFile { localPath.toLocalFile() };
-    Q_ASSERT(localFile.isFile());
-
-    const auto txnId =
-        d->addAsPending(
-             makeEvent<RoomMessageEvent>(plainText, localFile, asGenericFile))
-            ->transactionId();
+    const auto txnId = addAsPending(move(msgEvent))->transactionId();
     // Remote URL will only be known after upload; fill in the local path
     // to enable the preview while the event is pending.
-    uploadFile(txnId, localPath);
+    q->uploadFile(txnId, localUrl);
     // Below, the upload job is used as a context object to clean up connections
-    const auto& transferJob = d->fileTransfers.value(txnId).job;
-    connect(this, &Room::fileTransferCompleted, transferJob,
-            [this, txnId](const QString& id, const QUrl&, const QUrl& mxcUri) {
-                if (id == txnId) {
-                    auto it = findPendingEvent(txnId);
-                    if (it != d->unsyncedEvents.end()) {
-                        it->setFileUploaded(mxcUri);
-                        emit pendingEventChanged(
-                            int(it - d->unsyncedEvents.begin()));
-                        d->doSendEvent(it->get());
-                    } else {
-                        // Normally in this situation we should instruct
-                        // the media server to delete the file; alas, there's no
-                        // API specced for that.
-                        qCWarning(MAIN) << "File uploaded to" << mxcUri
-                                        << "but the event referring to it was "
-                                           "cancelled";
-                    }
+    const auto& transferJob = fileTransfers.value(txnId).job;
+    connect(q, &Room::fileTransferCompleted, transferJob,
+            [this, txnId](const QString& tId, const QUrl&, const QUrl& mxcUri) {
+                if (tId != txnId)
+                    return;
+
+                const auto it = q->findPendingEvent(txnId);
+                if (it != unsyncedEvents.end()) {
+                    it->setFileUploaded(mxcUri);
+                    emit q->pendingEventChanged(
+                        int(it - unsyncedEvents.begin()));
+                    doSendEvent(it->get());
+                } else {
+                    // Normally in this situation we should instruct
+                    // the media server to delete the file; alas, there's no
+                    // API specced for that.
+                    qCWarning(MAIN) << "File uploaded to" << mxcUri
+                                    << "but the event referring to it was "
+                                       "cancelled";
                 }
             });
-    connect(this, &Room::fileTransferCancelled, transferJob,
-            [this, txnId](const QString& id) {
-                if (id == txnId) {
-                    auto it = findPendingEvent(txnId);
-                    if (it != d->unsyncedEvents.end()) {
-                        const auto idx = int(it - d->unsyncedEvents.begin());
-                        emit pendingEventAboutToDiscard(idx);
-                        // See #286 on why iterator may not be valid here.
-                        d->unsyncedEvents.erase(d->unsyncedEvents.begin() + idx);
-                        emit pendingEventDiscarded();
-                    }
-                }
+    connect(q, &Room::fileTransferCancelled, transferJob,
+            [this, txnId](const QString& tId) {
+                if (tId != txnId)
+                    return;
+
+                const auto it = q->findPendingEvent(txnId);
+                if (it == unsyncedEvents.end())
+                    return;
+
+                const auto idx = int(it - unsyncedEvents.begin());
+                emit q->pendingEventAboutToDiscard(idx);
+                // See #286 on why `it` may not be valid here.
+                unsyncedEvents.erase(unsyncedEvents.begin() + idx);
+                emit q->pendingEventDiscarded();
             });
 
     return txnId;
 }
 
+QString Room::postFile(const QString& plainText,
+                       EventContent::TypedBase* content)
+{
+    Q_ASSERT(content != nullptr && content->fileInfo() != nullptr);
+    const auto* const fileInfo = content->fileInfo();
+    Q_ASSERT(fileInfo != nullptr);
+    QFileInfo localFile { fileInfo->url.toLocalFile() };
+    Q_ASSERT(localFile.isFile());
+
+    return d->doPostFile(
+        makeEvent<RoomMessageEvent>(
+            plainText, RoomMessageEvent::rawMsgTypeForFile(localFile), content),
+        fileInfo->url);
+}
+
+#if QT_VERSION_MAJOR < 6
+QString Room::postFile(const QString& plainText, const QUrl& localPath,
+                       bool asGenericFile)
+{
+    QFileInfo localFile { localPath.toLocalFile() };
+    Q_ASSERT(localFile.isFile());
+    return d->doPostFile(makeEvent<RoomMessageEvent>(plainText, localFile,
+                                                     asGenericFile),
+                         localPath);
+}
+#endif
+
 QString Room::postEvent(RoomEvent* event)
 {
     return d->sendEvent(RoomEventPtr(event));

diff --git a/lib/room.h b/lib/room.h
@@ -549,8 +549,13 @@ public Q_SLOTS:
     QString postHtmlText(const QString& plainText, const QString& html);
     /// Send a reaction on a given event with a given key
     QString postReaction(const QString& eventId, const QString& key);
+
+    QString postFile(const QString& plainText, EventContent::TypedBase* content);
+#if QT_VERSION_MAJOR < 6
+    /// \deprecated Use postFile(QString, MessageEventType, EventContent) instead
     QString postFile(const QString& plainText, const QUrl& localPath,
                      bool asGenericFile = false);
+#endif
     /** Post a pre-created room message event
      *
      * Takes ownership of the event, deleting it once the matching one

diff --git a/quotest/quotest.cpp b/quotest/quotest.cpp
@@ -397,15 +397,16 @@ TEST_IMPL(sendFile)
     }
     tf->write("Test");
     tf->close();
+    QFileInfo tfi { *tf };
     // QFileInfo::fileName brings only the file name; QFile::fileName brings
     // the full path
-    const auto tfName = QFileInfo(*tf).fileName();
+    const auto tfName = tfi.fileName();
     clog << "Sending file " << tfName.toStdString() << endl;
-    const auto txnId =
-        targetRoom->postFile("Test file", QUrl::fromLocalFile(tf->fileName()));
+    const auto txnId = targetRoom->postFile(
+        "Test file", new EventContent::FileContent(tfi));
     if (!validatePendingEvent(txnId)) {
         clog << "Invalid pending event right after submitting" << endl;
-        delete tf;
+        tf->deleteLater();
         FAIL_TEST();
     }