Merge #826(kitsune): More cleanup and code modernization (still 0.9 c…

…ompatible)

Quotient/avatar.h

@@ -28,7 +28,7 @@ class QUOTIENT_API Avatar {
 
 
     QImage get(int dimension, get_callback_t callback) const;
-    QImage get(int w, int h, get_callback_t callback) const;
+    QImage get(int width, int height, get_callback_t callback) const;
 
     [[deprecated("Use the QFuture-returning overload instead")]]
     bool upload(const QString& fileName, upload_callback_t callback) const;

Quotient/connection.cpp

@@ -50,13 +50,15 @@
 
 using namespace Quotient;
 
+namespace {
 // This is very much Qt-specific; STL iterators don't have key() and value()
-template <typename HashT, typename Pred>
-HashT remove_if(HashT& hashMap, Pred pred)
+template <typename HashT>
+HashT remove_if(HashT& hashMap,
+                std::invocable<typename HashT::key_type, typename HashT::value_type> auto pred)
 {
     HashT removals;
     for (auto it = hashMap.begin(); it != hashMap.end();) {
-        if (pred(it)) {
+        if (pred(it.key(), it.value())) {
             removals.insert(it.key(), it.value());
             it = hashMap.erase(it);
         } else
@@ -65,6 +67,12 @@ HashT remove_if(HashT& hashMap, Pred pred)
     return removals;
 }
 
+inline void map_subtract(auto& lhs, const auto& rhs)
+{
+    remove_if(lhs, [&rhs](const auto& k, const auto& v) { return rhs.contains(k, v); });
+}
+}
+
 Connection::Connection(const QUrl& server, QObject* parent)
     : QObject(parent)
     , d(makeImpl<Private>(std::make_unique<ConnectionData>(server)))
@@ -556,47 +564,36 @@ void Connection::Private::consumeAccountData(Events&& accountDataEvents)
             [this](const DirectChatEvent& dce) {
                 // https://github.com/quotient-im/libQuotient/wiki/Handling-direct-chat-events
                 const auto& usersToDCs = dce.usersToDirectChats();
-                DirectChatsMap remoteRemovals =
-                    remove_if(directChats, [&usersToDCs, this](auto it) {
-                        return !(
-                            usersToDCs.contains(it.key()->id(), it.value())
-                            || dcLocalAdditions.contains(it.key(), it.value()));
+                const DirectChatsMap remoteRemovals =
+                    remove_if(directChats, [&usersToDCs, this](const User* u, const QString& rId) {
+                        const auto removed = !(usersToDCs.contains(u->id(), rId)
+                                               || dcLocalAdditions.contains(u, rId));
+                        if (removed)
+                            qCDebug(MAIN) << rId << "is no more a direct chat with" << u->id();
+                        return removed;
                     });
-                remove_if(directChatMemberIds, [&remoteRemovals, this](auto it) {
-                    return remoteRemovals.contains(q->user(it.value()), it.key());
-                });
+                remove_if(directChatMemberIds,
+                          [&remoteRemovals, this](const QString& rId, const QString& mId) {
+                              return remoteRemovals.contains(q->user(mId), rId);
+                          });
                 // Remove from dcLocalRemovals what the server already has.
-                remove_if(dcLocalRemovals, [&remoteRemovals](auto it) {
-                    return remoteRemovals.contains(it.key(), it.value());
-                });
-                if (MAIN().isDebugEnabled())
-                    for (auto it = remoteRemovals.begin();
-                         it != remoteRemovals.end(); ++it) {
-                        qCDebug(MAIN)
-                            << it.value() << "is no more a direct chat with"
-                            << it.key()->id();
-                    }
+                map_subtract(dcLocalRemovals, remoteRemovals);
 
                 DirectChatsMap remoteAdditions;
-                for (auto it = usersToDCs.begin(); it != usersToDCs.end(); ++it) {
-                    if (auto* u = q->user(it.key())) {
-                        if (!directChats.contains(u, it.value())
-                            && !dcLocalRemovals.contains(u, it.value())) {
-                            Q_ASSERT(!directChatMemberIds.contains(it.value(), it.key()));
-                            remoteAdditions.insert(u, it.value());
-                            directChats.insert(u, it.value());
-                            directChatMemberIds.insert(it.value(), it.key());
-                            qCDebug(MAIN) << "Marked room" << it.value()
-                                          << "as a direct chat with" << u->id();
+                for (const auto& [uId, rId] : usersToDCs.asKeyValueRange()) {
+                    if (const auto* const u = q->user(uId)) {
+                        if (!directChats.contains(u, rId) && !dcLocalRemovals.contains(u, rId)) {
+                            Q_ASSERT(!directChatMemberIds.contains(rId, uId));
+                            remoteAdditions.insert(u, rId);
+                            directChats.insert(u, rId);
+                            directChatMemberIds.insert(rId, uId);
+                            qCDebug(MAIN) << "Marked room" << rId << "as a direct chat with" << uId;
                         }
                     } else
-                        qCWarning(MAIN)
-                            << "Couldn't get a user object for" << it.key();
+                        qCWarning(MAIN) << "Couldn't get a user object for" << uId;
                 }
                 // Remove from dcLocalAdditions what the server already has.
-                remove_if(dcLocalAdditions, [&remoteAdditions](auto it) {
-                    return remoteAdditions.contains(it.key(), it.value());
-                });
+                map_subtract(dcLocalAdditions, remoteAdditions);
                 if (!remoteAdditions.isEmpty() || !remoteRemovals.isEmpty())
                     emit q->directChatsListChanged(remoteAdditions,
                                                    remoteRemovals);
@@ -1139,16 +1136,14 @@ QVector<Room*> Connection::rooms(JoinStates joinStates) const
 int Connection::roomsCount(JoinStates joinStates) const
 {
     // Using int to maintain compatibility with QML
-    // (consider also that QHash<>::size() returns int anyway).
-    return int(std::count_if(d->roomMap.cbegin(), d->roomMap.cend(),
-                             [joinStates](Room* r) {
-                                 return joinStates.testFlag(r->joinState());
-                             }));
+    return static_cast<int>(std::ranges::count_if(d->roomMap, [joinStates](const Room* r) {
+        return joinStates.testFlag(r->joinState());
+    }));
 }
 
 bool Connection::hasAccountData(const QString& type) const
 {
-    return d->accountData.find(type) != d->accountData.cend();
+    return d->accountData.contains(type);
 }
 
 const EventPtr& Connection::accountData(const QString& type) const
@@ -1182,10 +1177,10 @@ QHash<QString, QVector<Room*>> Connection::tagsToRooms() const
         for (const auto& tagName : tagNames)
             result[tagName].push_back(r);
     }
-    for (auto it = result.begin(); it != result.end(); ++it)
-        std::sort(it->begin(), it->end(), [t = it.key()](Room* r1, Room* r2) {
-            return r1->tags().value(t) < r2->tags().value(t);
-        });
+    // TODO: use a structured binding once https://github.com/llvm/llvm-project/issues/115137 is done
+    for (auto&& p : result.asKeyValueRange()) {
+        std::ranges::sort(p.second, {}, [tag=p.first](const Room* r) { return r->tag(tag); });
+    }
     return result;
 }
 
@@ -1256,8 +1251,7 @@ void Connection::removeFromDirectChats(const QString& roomId, const QString& use
         removals.insert(u, roomId);
         d->dcLocalRemovals.insert(u, roomId);
     } else {
-        removals = remove_if(d->directChats,
-                            [&roomId](auto it) { return it.value() == roomId; });
+        removals = remove_if(d->directChats, [&roomId](auto, auto rId) { return rId == roomId; });
         d->dcLocalRemovals += removals;
     }
     emit directChatsListChanged({}, removals);

Quotient/connectiondata.cpp

@@ -124,9 +124,9 @@ void ConnectionData::setBaseUrl(QUrl baseUrl)
     }
 }
 
-void ConnectionData::setAccessToken(QByteArray token)
+void ConnectionData::setAccessToken(QByteArray accessToken)
 {
-    d->accessToken = std::move(token);
+    d->accessToken = std::move(accessToken);
     NetworkAccessManager::setAccessToken(d->userId, d->accessToken);
 }
 

Quotient/converters.h

@@ -12,6 +12,9 @@
 #include <QtCore/QSet>
 #include <QtCore/QUrlQuery>
 #include <QtCore/QVector>
+#if QT_VERSION >= QT_VERSION_CHECK(6, 5, 0)
+    #include <QtCore/QTimeZone>
+#endif
 
 #include <type_traits>
 #include <vector>
@@ -263,7 +266,12 @@ inline QJsonValue toJson(const QDateTime& val)
 template <>
 inline QDateTime fromJson(const QJsonValue& jv)
 {
-    return QDateTime::fromMSecsSinceEpoch(fromJson<qint64>(jv), Qt::UTC);
+    return QDateTime::fromMSecsSinceEpoch(fromJson<qint64>(jv),
+#if QT_VERSION >= QT_VERSION_CHECK(6, 5, 0)
+                                          QTimeZone::UTC);
+#else
+                                          Qt::UTC);
+#endif
 }
 
 inline QJsonValue toJson(const QDate& val) { return toJson(val.startOfDay()); }

Quotient/events/accountdataevents.h

@@ -14,7 +14,7 @@ constexpr inline auto ServerNoticeTag = "m.server_notice"_L1;
 
 using TagRecord [[deprecated("Use Tag from csapi/definitions/tag.h instead")]] = Tag;
 
-inline std::partial_ordering operator<=>( //
+inline std::partial_ordering operator<=>(
     const Tag& lhs, const Tag& rhs) // clazy:exclude=function-args-by-value
 {
     // Per The Spec, rooms with no order should be after those with order,
@@ -24,6 +24,11 @@ inline std::partial_ordering operator<=>( //
                                        : *lhs.order <=> *rhs.order;
 }
 
+inline bool operator==(const Tag& lhs, const Tag& rhs) // clazy:exclude=function-args-by-value
+{
+    return std::is_eq(lhs <=> rhs);
+}
+
 using TagsMap = QHash<QString, Tag>;
 
 DEFINE_SIMPLE_EVENT(TagEvent, Event, "m.tag", TagsMap, tags, "tags")

Quotient/events/event.h

@@ -548,11 +548,8 @@ inline auto eventCast(event_ptr_tt<BaseEventT>&& eptr)
 
 namespace _impl {
     template <typename FnT, typename BaseT>
-    concept Invocable_With_Downcast = requires
-    {
-        requires EventClass<BaseT>;
-        std::is_base_of_v<BaseT, std::remove_cvref_t<fn_arg_t<FnT>>>;
-    };
+    concept Invocable_With_Downcast =
+        EventClass<BaseT> && std::derived_from<std::remove_cvref_t<fn_arg_t<FnT>>, BaseT>;
 }
 
 template <EventClass BaseT, typename TailT>

Quotient/events/eventcontent.h

@@ -151,7 +151,7 @@ struct QUOTIENT_API Thumbnail : public ImageInfo {
 //! The base for all file-based content classes
 class QUOTIENT_API FileContentBase : public Base {
 public:
-    FileContentBase(const QJsonObject& contentJson = {})
+    explicit FileContentBase(const QJsonObject& contentJson = {})
         : Base(contentJson), thumbnail(contentJson[InfoKey].toObject())
     {}
     virtual QUrl url() const = 0;

Quotient/events/stateevent.h

@@ -124,6 +124,10 @@ class KeylessStateEventBase
 
 public:
     template <typename... ContentParamTs>
+        // Ideally, we want to check std::constructible_from<ContentT, ContentParamTs...> -
+        // unfortunately, Xcode 15.4 still thinks that, e.g., AliasEventContent is not constructible
+        // from QString and QStringList, so we have to make the check slightly indirect
+        requires std::constructible_from<base_type, QString, ContentParamTs...>
     explicit KeylessStateEventBase(ContentParamTs&&... contentParams)
         : base_type(QString(), std::forward<ContentParamTs>(contentParams)...)
     {}

Quotient/mxcreply.cpp

@@ -51,10 +51,10 @@ MxcReply::MxcReply()
         }, Qt::QueuedConnection);
 }
 
-qint64 MxcReply::readData(char *data, qint64 maxSize)
+qint64 MxcReply::readData(char *data, qint64 maxlen)
 {
     if(d != nullptr && d->m_device != nullptr) {
-        return d->m_device->read(data, maxSize);
+        return d->m_device->read(data, maxlen);
     }
     return -1;
 }

Quotient/mxcreply.h

@@ -24,7 +24,7 @@ public Q_SLOTS:
     void abort() override;
 
 protected:
-    qint64 readData(char* data, qint64 maxSize) override;
+    qint64 readData(char* data, qint64 maxlen) override;
 
 private:
     class Private;

Quotient/room.cpp

@@ -2454,12 +2454,9 @@ void Room::downloadFile(const QString& eventId, const QUrl& localFilename)
     Q_ASSERT_X(localFilename.isEmpty() || localFilename.isLocalFile(),
                __FUNCTION__, "localFilename should point at a local file");
     const auto* event = d->getEventWithFile(eventId);
-    if (!event) {
-        qCCritical(MAIN)
-            << eventId << "is not in the local timeline or has no file content";
-        Q_ASSERT(false);
+    if (QUO_ALARM_X(!event, eventId + " is not in the local timeline or has no file content"_L1))
         return;
-    }
+
     const auto fileInfo = event->get<EventContent::FileContentBase>()->commonInfo();
     if (!fileInfo.isValid()) {
         qCWarning(MAIN) << "Event" << eventId
@@ -2477,31 +2474,35 @@ void Room::downloadFile(const QString& eventId, const QUrl& localFilename)
         filePath = QDir::tempPath() % u'/' % filePath;
         qDebug(MAIN) << "File path:" << filePath;
     }
-    DownloadFileJob *job = nullptr;
-    if (auto* fileMetadata = std::get_if<EncryptedFileMetadata>(&fileInfo.source)) {
-        job = connection()->downloadFile(fileUrl, *fileMetadata, filePath);
-    } else {
-        job = connection()->downloadFile(fileUrl, filePath);
-    }
-    if (isJobPending(job)) {
-        // If there was a previous transfer (completed or failed), overwrite it.
-        d->fileTransfers[eventId] = { job, job->targetFileName() };
-        connect(job, &BaseJob::downloadProgress, this,
-                [this, eventId](qint64 received, qint64 total) {
-                    d->fileTransfers[eventId].update(received, total);
-                    emit fileTransferProgress(eventId, received, total);
-                });
-        connect(job, &BaseJob::success, this, [this, eventId, fileUrl, job] {
-            d->fileTransfers[eventId].status = FileTransferInfo::Completed;
-            emit fileTransferCompleted(
-                eventId, fileUrl, QUrl::fromLocalFile(job->targetFileName()));
-        });
-        connect(job, &BaseJob::failure, this,
-                std::bind(&Private::failedTransfer, d, eventId,
-                          job->errorString()));
-        emit newFileTransfer(eventId, localFilename);
-    } else
+    const auto job =
+        std::visit(Overloads{ [this, &fileUrl, &filePath](const EncryptedFileMetadata& fileMetadata) {
+                                 return connection()->downloadFile(fileUrl, fileMetadata, filePath);
+                             },
+                              [this, &fileUrl, &filePath](auto) {
+                                  return connection()->downloadFile(fileUrl, filePath);
+                              } },
+                   fileInfo.source);
+    if (!isJobPending(job)) {
         d->failedTransfer(eventId);
+        return;
+    }
+
+    // If there was a previous transfer (completed or failed), overwrite it.
+    d->fileTransfers[eventId] = { job, job->targetFileName() };
+    connect(job, &BaseJob::downloadProgress, this,
+            [this, eventId](qint64 received, qint64 total) {
+                d->fileTransfers[eventId].update(received, total);
+                emit fileTransferProgress(eventId, received, total);
+            });
+    connect(job, &BaseJob::success, this, [this, eventId, fileUrl, job] {
+        d->fileTransfers[eventId].status = FileTransferInfo::Completed;
+        emit fileTransferCompleted(
+            eventId, fileUrl, QUrl::fromLocalFile(job->targetFileName()));
+    });
+    connect(job, &BaseJob::failure, this,
+            std::bind(&Private::failedTransfer, d, eventId,
+                      job->errorString()));
+    emit newFileTransfer(eventId, localFilename);
 }
 
 void Room::cancelFileTransfer(const QString& id)

Quotient/util.h

@@ -140,10 +140,8 @@ class [[deprecated("Use std::ranges::subrange instead")]] Range {
 
 namespace _impl {
     template <typename T>
-    concept Holds_NonConst_LValue_Ref = requires {
-        std::is_lvalue_reference_v<T>;
-        !std::is_const_v<std::remove_reference<T>>;
-    };
+    concept Holds_NonConst_LValue_Ref =
+        std::is_lvalue_reference_v<T> && !std::is_const_v<std::remove_reference<T>>;
 }
 
 //! \brief An adaptor for Qt (hash-)maps to make them iterable in STL style

quotest/quotest.cpp

@@ -677,6 +677,11 @@ TEST_IMPL(addAndRemoveTag)
         clog << "Tag adding failed" << endl;
         FAIL_TEST();
     }
+    const auto& tagsToRooms = connection()->tagsToRooms();
+    if (!tagsToRooms.contains(TestTag) || !tagsToRooms[TestTag].contains(targetRoom)) {
+        clog << "Tag adding succeeded but the connection doesn't know about it\n";
+        FAIL_TEST();
+    }
     clog << "Test tag set, removing it now" << endl;
     targetRoom->removeTag(TestTag);
     FINISH_TEST(spy.size() == 2 && !targetRoom->tags().contains(TestTag));