Merge #796(kitsune): RoomMember::avatar()

Quotient/avatar.cpp

@@ -15,133 +15,128 @@
 
 using namespace Quotient;
 
-class Q_DECL_HIDDEN Avatar::Private : public QObject {
+class Q_DECL_HIDDEN Avatar::Private {
 public:
-    explicit Private(QUrl url = {}) : _url(std::move(url)) {}
-    ~Private() override
+    explicit Private(Connection* c) : connection(c) {}
+    ~Private()
     {
-        _thumbnailRequest.abandon();
-        _uploadRequest.abandon();
+        thumbnailRequest.abandon();
+        uploadRequest.abandon();
     }
     Q_DISABLE_COPY_MOVE(Private)
 
-    QImage get(Connection* connection, QSize size,
-               get_callback_t callback) const;
-    void thumbnailRequestFinished();
+    QImage get(QSize size, get_callback_t callback) const;
+    void thumbnailRequestFinished() const;
 
-    bool checkUrl(const QUrl& url) const;
     QString localFile() const;
 
+    Connection* connection;
     QUrl _url;
 
     // The below are related to image caching, hence mutable
-    mutable QImage _originalImage;
-    mutable std::vector<std::pair<QSize, QImage>> _scaledImages;
-    mutable QSize _largestRequestedSize{};
+    mutable QImage originalImage;
+    mutable std::vector<std::pair<QSize, QImage>> scaledImages;
+    mutable QSize largestRequestedSize{};
     enum ImageSource : quint8 { Unknown, Cache, Network, Invalid };
-    mutable ImageSource _imageSource = Unknown;
-    mutable JobHandle<MediaThumbnailJob> _thumbnailRequest = nullptr;
-    mutable JobHandle<UploadContentJob> _uploadRequest = nullptr;
+    mutable ImageSource imageSource = Invalid;
+    mutable JobHandle<MediaThumbnailJob> thumbnailRequest = nullptr;
+    mutable JobHandle<UploadContentJob> uploadRequest = nullptr;
     mutable std::vector<get_callback_t> callbacks{};
 };
 
-Avatar::Avatar() : d(makeImpl<Private>()) {}
-
-Avatar::Avatar(QUrl url) : d(makeImpl<Private>(std::move(url))) {}
+Avatar::Avatar(Connection* parent, const QUrl& url) : d(makeImpl<Private>(parent))
+{
+    if (!url.isEmpty())
+        updateUrl(url);
+}
 
-QImage Avatar::get(Connection* connection, int dimension,
-                   get_callback_t callback) const
+QImage Avatar::get(int dimension, get_callback_t callback) const
 {
-    return d->get(connection, { dimension, dimension }, std::move(callback));
+    return d->get({ dimension, dimension }, std::move(callback));
 }
 
-QImage Avatar::get(Connection* connection, int width, int height,
-                   get_callback_t callback) const
+QImage Avatar::get(int width, int height, get_callback_t callback) const
 {
-    return d->get(connection, { width, height }, std::move(callback));
+    return d->get({ width, height }, std::move(callback));
 }
 
-bool Avatar::upload(Connection* connection, const QString& fileName,
-                    upload_callback_t callback) const
+bool Avatar::upload(const QString& fileName, upload_callback_t callback) const
 {
-    if (isJobPending(d->_uploadRequest))
+    if (isJobPending(d->uploadRequest))
         return false;
-    upload(connection, fileName).then(std::move(callback));
+    upload(fileName).then(std::move(callback));
     return true;
 }
 
-bool Avatar::upload(Connection* connection, QIODevice* source,
-                    upload_callback_t callback) const
+bool Avatar::upload(QIODevice* source, upload_callback_t callback) const
 {
-    if (isJobPending(d->_uploadRequest) || !source->isReadable())
+    if (isJobPending(d->uploadRequest) || !source->isReadable())
         return false;
-    upload(connection, source).then(std::move(callback));
+    upload(source).then(std::move(callback));
     return true;
 }
 
-QFuture<QUrl> Avatar::upload(Connection* connection, const QString& fileName) const
+QFuture<QUrl> Avatar::upload(const QString& fileName) const
 {
-    d->_uploadRequest = connection->uploadFile(fileName);
-    return d->_uploadRequest.responseFuture();
+    d->uploadRequest = d->connection->uploadFile(fileName);
+    return d->uploadRequest.responseFuture();
 }
 
-QFuture<QUrl> Avatar::upload(Connection* connection, QIODevice* source) const
+QFuture<QUrl> Avatar::upload(QIODevice* source) const
 {
-    d->_uploadRequest = connection->uploadContent(source);
-    return d->_uploadRequest.responseFuture();
+    d->uploadRequest = d->connection->uploadContent(source);
+    return d->uploadRequest.responseFuture();
 }
 
+bool Avatar::isEmpty() const { return d->_url.isEmpty(); }
+
 QString Avatar::mediaId() const { return d->_url.authority() + d->_url.path(); }
 
-QImage Avatar::Private::get(Connection* connection, QSize size,
-                            get_callback_t callback) const
+QImage Avatar::Private::get(QSize size, get_callback_t callback) const
 {
-    if (_imageSource == Unknown && _originalImage.load(localFile())) {
-        _imageSource = Cache;
-        _largestRequestedSize = _originalImage.size();
+    if (imageSource == Unknown && originalImage.load(localFile())) {
+        imageSource = Cache;
+        largestRequestedSize = originalImage.size();
     }
 
     // Assuming that all thumbnails for this avatar have the same aspect ratio,
     // it's enough for the image requested before to be large enough in at least
     // one dimension to be suitable for scaling down to the requested size;
     // therefore the new size has to be larger in both dimensions to warrant a
     // new request to the server
-    if (((_imageSource == Unknown && !_thumbnailRequest)
-         || (size.width() > _largestRequestedSize.width()
-             && size.height() > _largestRequestedSize.height()))
-        && checkUrl(_url)) {
+    if ((imageSource == Unknown && !thumbnailRequest)
+        || (imageSource != Invalid && size.width() > largestRequestedSize.width()
+            && size.height() > largestRequestedSize.height())) {
         qCDebug(MAIN) << "Getting avatar from" << _url.toString();
-        _largestRequestedSize = size;
-        if (isJobPending(_thumbnailRequest))
-            _thumbnailRequest->abandon();
+        largestRequestedSize = size;
+        thumbnailRequest.abandon();
         if (callback)
             callbacks.emplace_back(std::move(callback));
-        _thumbnailRequest = connection->getThumbnail(_url, size);
-        connect(_thumbnailRequest, &MediaThumbnailJob::finished, this,
-                &Private::thumbnailRequestFinished);
+        thumbnailRequest = connection->getThumbnail(_url, size);
+        thumbnailRequest.onResult([this] { thumbnailRequestFinished(); });
         // The result of this request will only be returned when get() is
         // called next time afterwards
     }
-    if (_imageSource == Invalid || _originalImage.isNull())
+    if (imageSource == Invalid || originalImage.isNull())
         return {};
 
     // NB: because of KeepAspectRatio, scaledImage.size() might not be equal to
     // requestedSize - this is why requestedSize is stored separately
-    for (const auto& [requestedSize, scaledImage] : _scaledImages)
+    for (const auto& [requestedSize, scaledImage] : scaledImages)
         if (requestedSize == size)
             return scaledImage;
 
-    const auto& result = _originalImage.scaled(size, Qt::KeepAspectRatio,
+    const auto& result = originalImage.scaled(size, Qt::KeepAspectRatio,
                                                Qt::SmoothTransformation);
-    _scaledImages.emplace_back(size, result);
+    scaledImages.emplace_back(size, result);
     return result;
 }
 
-void Avatar::Private::thumbnailRequestFinished()
+void Avatar::Private::thumbnailRequestFinished() const
 {
     // NB: The following code preserves _originalImage in case of
     // most errors
-    switch (_thumbnailRequest->error()) {
+    switch (thumbnailRequest->error()) {
     case BaseJob::NoError: break;
     case BaseJob::NetworkError:
     case BaseJob::NetworkAuthRequired:
@@ -152,41 +147,26 @@ void Avatar::Private::thumbnailRequestFinished()
         // Other errors are likely unrecoverable but just in case,
         // check if there's a previous image to fall back to; if
         // there is, assume that the error is temporary
-        if (_originalImage.isNull())
-            _imageSource = Invalid; // Can't do much with the rest
+        if (originalImage.isNull())
+            imageSource = Invalid; // Can't do much with the rest
         return;
     }
-    auto&& img = _thumbnailRequest->thumbnail();
+    auto&& img = thumbnailRequest->thumbnail();
     if (img.format() == QImage::Format_Invalid) {
         qCWarning(MAIN) << "The request for" << _url
                         << "was successful but the received image "
                            "is invalid or unsupported";
         return;
     }
-    _imageSource = Network;
-    _originalImage = std::move(img);
-    _originalImage.save(localFile());
-    _scaledImages.clear();
-    for (const auto& n : callbacks)
+    imageSource = Network;
+    originalImage = std::move(img);
+    originalImage.save(localFile());
+    scaledImages.clear();
+    for (auto&& n : callbacks)
         n();
     callbacks.clear();
 }
 
-bool Avatar::Private::checkUrl(const QUrl& url) const
-{
-    if (_imageSource == Invalid || url.isEmpty())
-        return false;
-
-    // FIXME: Make "mxc" a library-wide constant and maybe even make
-    // the URL checker a Connection(?) method.
-    if (!url.isValid() || url.scheme() != "mxc"_L1 || url.path().count(u'/') != 1) {
-        qCWarning(MAIN) << "Avatar URL is invalid or not mxc-based:"
-                        << url.toDisplayString();
-        _imageSource = Invalid;
-    }
-    return _imageSource != Invalid;
-}
-
 QString Avatar::Private::localFile() const
 {
     static const auto cachePath = cacheLocation(u"avatars");
@@ -200,11 +180,21 @@ bool Avatar::updateUrl(const QUrl& newUrl)
     if (newUrl == d->_url)
         return false;
 
-    d->_url = newUrl;
-    d->_imageSource = Private::Unknown;
-    d->_originalImage = {};
-    d->_scaledImages.clear();
-    if (isJobPending(d->_thumbnailRequest))
-        d->_thumbnailRequest->abandon();
+    if (isUrlValid(newUrl)) {
+        d->_url = d->connection->makeMediaUrl(newUrl);
+        d->imageSource = Private::Unknown;
+    } else {
+        qCWarning(MAIN) << "Avatar URL is invalid or not mxc-based:" << newUrl.toDisplayString();
+        d->_url.clear();
+        d->imageSource = Private::Invalid;
+    }
+    d->originalImage = {};
+    d->scaledImages.clear();
+    d->thumbnailRequest.abandon();
     return true;
 }
+
+bool Avatar::isUrlValid(const QUrl& u)
+{
+    return u.isValid() && u.scheme() == u"mxc" && u.path().count(u'/') == 1;
+}

Quotient/avatar.h

@@ -16,31 +16,34 @@ class Connection;
 
 class QUOTIENT_API Avatar {
 public:
-    explicit Avatar();
-    explicit Avatar(QUrl url);
+    explicit Avatar(Connection* parent, const QUrl& url = {});
 
-    // TODO: use std::move_only_function once C++23 is here
+#ifdef __cpp_lib_move_only_function // AppleClang 15 doesn't have it
+    using get_callback_t = std::move_only_function<void()>;
+    using upload_callback_t = std::move_only_function<void(QUrl)>;
+#else
     using get_callback_t = std::function<void()>;
     using upload_callback_t = std::function<void(QUrl)>;
+#endif
 
-    QImage get(Connection* connection, int dimension,
-               get_callback_t callback) const;
-    QImage get(Connection* connection, int w, int h,
-               get_callback_t callback) const;
+
+    QImage get(int dimension, get_callback_t callback) const;
+    QImage get(int w, int h, get_callback_t callback) const;
 
     [[deprecated("Use the QFuture-returning overload instead")]]
-    bool upload(Connection* connection, const QString& fileName,
-                upload_callback_t callback) const;
+    bool upload(const QString& fileName, upload_callback_t callback) const;
     [[deprecated("Use the QFuture-returning overload instead")]]
-    bool upload(Connection* connection, QIODevice* source,
-                upload_callback_t callback) const;
-    QFuture<QUrl> upload(Connection* connection, const QString& fileName) const;
-    QFuture<QUrl> upload(Connection* connection, QIODevice* source) const;
+    bool upload(QIODevice* source, upload_callback_t callback) const;
+    QFuture<QUrl> upload(const QString& fileName) const;
+    QFuture<QUrl> upload(QIODevice* source) const;
 
+    bool isEmpty() const;
     QString mediaId() const;
     QUrl url() const;
     bool updateUrl(const QUrl& newUrl);
 
+    static bool isUrlValid(const QUrl& u);
+
 private:
     class Private;
     ImplPtr<Private> d;

Quotient/connection.cpp

@@ -1082,7 +1082,7 @@ Avatar& Connection::userAvatar(const QString& avatarMediaId)
 Avatar& Connection::userAvatar(const QUrl& avatarUrl)
 {
     const auto mediaId = avatarUrl.authority() + avatarUrl.path();
-    return d->userAvatarMap.try_emplace(mediaId, avatarUrl).first->second;
+    return d->userAvatarMap.try_emplace(mediaId, this, avatarUrl).first->second;
 }
 
 QString Connection::deviceId() const { return d->data->deviceId(); }

Quotient/room.cpp

@@ -85,6 +85,7 @@ class Q_DECL_HIDDEN Room::Private {
         : connection(c)
         , id(std::move(id_))
         , joinState(initialJoinState)
+        , avatar(c)
     {}
 
     Room* q = nullptr;
@@ -617,23 +618,24 @@ QString Room::avatarMediaId() const { return d->avatar.mediaId(); }
 
 QUrl Room::avatarUrl() const { return d->avatar.url(); }
 
-const Avatar& Room::avatarObject() const { return d->avatar; }
+const Avatar& Room::avatarObject() const
+{
+    // If the avatar is not empty use it; otherwise, try to use the first (excluding self) user's
+    // avatar for direct chats, or just return the empty room avatar if that doesn't work out
+    if (d->avatar.isEmpty())
+        for (const auto dcMembers = directChatMembers(); const auto& m : dcMembers)
+            if (m != localMember())
+                return m.avatarObject();
+
+    return d->avatar;
+}
 
 QImage Room::avatar(int dimension) { return avatar(dimension, dimension); }
 
 QImage Room::avatar(int width, int height)
 {
-    if (!d->avatar.url().isEmpty())
-        return d->avatar.get(connection(), width, height,
-                             [this] { emit avatarChanged(); });
-
-    // Use the first (excluding self) user's avatar for direct chats
-    for (const auto dcMembers = directChatMembers(); const auto& m : dcMembers)
-        if (m != localMember())
-            return memberAvatar(m.id()).get(connection(), width, height,
-                                            [this] { emit avatarChanged(); });
-
-    return {};
+    return d->avatar.isEmpty() ? QImage()
+                               : d->avatar.get(width, height, [this] { emit avatarChanged(); });
 }
 
 RoomMember Room::localMember() const { return member(connection()->userId()); }
@@ -1754,13 +1756,24 @@ Room::Private::moveEventsToTimeline(RoomEventsRange events,
     return Timeline::size_type(insertedSize);
 }
 
-const Avatar& Room::memberAvatar(const QString& memberId) const
+const Avatar& Room::memberAvatarObject(const QString& memberId) const
 {
     return connection()->userAvatar(member(memberId).avatarUrl());
 }
 
-Room::Changes Room::Private::updateStatsFromSyncData(const SyncRoomData& data,
-                                                     bool fromCache)
+QImage Room::memberAvatar(const QString& memberId, int width, int height)
+{
+    return member(memberId).avatar(width, height, [this, memberId] {
+        emit memberAvatarUpdated(member(memberId));
+    });
+}
+
+QImage Room::memberAvatar(const QString& memberId, int dimension)
+{
+    return memberAvatar(memberId, dimension, dimension);
+}
+
+Room::Changes Room::Private::updateStatsFromSyncData(const SyncRoomData& data, bool fromCache)
 {
     Changes changes {};
     if (fromCache) {