From 9dff5490103b98aec948c88a31b914b6c6b0538a Mon Sep 17 00:00:00 2001 From: Kitsune Ral <> Date: Mon, 25 Dec 2023 19:45:12 +0100 Subject: [PATCH] Streamline ThumbnailProvider 1. Split ThumbnailProvider into two image providers, one for avatars and another for thumbnails; the common code is now almost entirely in AbstractThumbnailResponse; and the provider type declaration is templated because it's exactly the same across the two instances. 2. No more atomic pointer to Connection - instead, ThumbnailProviders now rely on TimelineWidget as a singular holder of the necessary object, which is now the current room, not the connection (see next). Because the pointer to TimelineWidget doesn't change, there's no need in atomics; and the just-in-time resolution of the current room and connection, right in the main thread, avoids potential synchronization problems. 3. AvatarProvider no more accepts a two-part media id - it's either the room id or the user id, the latter resolved within the room context. --- client/qml/Avatar.qml | 5 +- client/thumbnailprovider.cpp | 116 +++++++++++++++++++---------------- client/thumbnailprovider.h | 24 +++----- client/timelinewidget.cpp | 7 +-- client/timelinewidget.h | 2 - 5 files changed, 77 insertions(+), 77 deletions(-) diff --git a/client/qml/Avatar.qml b/client/qml/Avatar.qml index 0a632fa3..f31ee875 100644 --- a/client/qml/Avatar.qml +++ b/client/qml/Avatar.qml @@ -5,9 +5,8 @@ Image { readonly property var forRoom: root.room /* readonly */ property var forMember - property string sourceId: forRoom ? "image://thumbnail/" + forRoom.id - + (forMember ? '/' + forMember.id : "") - : "" + property string sourceId: + forRoom ? "image://avatar/" + (forMember ? forMember : forRoom).id : "" source: sourceId cache: false // Quotient::Avatar takes care of caching fillMode: Image.PreserveAspectFit diff --git a/client/thumbnailprovider.cpp b/client/thumbnailprovider.cpp index 3a5800cd..61de8555 100644 --- a/client/thumbnailprovider.cpp +++ b/client/thumbnailprovider.cpp @@ -8,20 +8,26 @@ #include "thumbnailprovider.h" +#include "timelinewidget.h" +#include "quaternionroom.h" #include "logging_categories.h" -#include -#include #include #include +#include // for qApp #include #include -#include -using Quotient::Connection; using Quotient::BaseJob; +inline int checkDimension(int d) +{ + // Emulate ushort overflow if the value is -1 - may cause issues when + // screen resolution becomes 100K+ each dimension :-D + return d >= 0 ? d : std::numeric_limits::max(); +} + inline QDebug operator<<(QDebug dbg, const auto&& size) requires std::is_same_v, QSize> { @@ -32,31 +38,28 @@ inline QDebug operator<<(QDebug dbg, const auto&& size) class AbstractThumbnailResponse : public QQuickImageResponse { Q_OBJECT public: - AbstractThumbnailResponse(Connection* c, QString id, QSize size) - : connection(c), mediaId(std::move(id)), requestedSize(size) + AbstractThumbnailResponse(const TimelineWidget* timeline, QString id, + QSize size) + : timeline(timeline) + , mediaId(std::move(id)) + , requestedSize( + { checkDimension(size.width()), checkDimension(size.height()) }) { qCDebug(THUMBNAILS).noquote() << mediaId << '@' << requestedSize << "requested"; - if (!c) - errorStr = tr("No connection to perform image request"); - else if (mediaId.isEmpty() || size.isEmpty()) { + if (mediaId.isEmpty() || requestedSize.isEmpty()) { qCDebug(THUMBNAILS) << "Returning an empty thumbnail"; image = { requestedSize, QImage::Format_Invalid }; - } else if (!mediaId.startsWith('!') && mediaId.count('/') != 1) - errorStr = - tr("Media id '%1' doesn't follow server/mediaId pattern") - .arg(mediaId); - else { - errorStr = tr("Image request is pending"); - // Start a request on the main thread, concluding the initialisation - moveToThread(connection->thread()); - QMetaObject::invokeMethod(this, - &AbstractThumbnailResponse::startRequest); - // From this point, access to `image` and `errorStr` must be guarded - // by `lock` + emit finished(); return; } - emit finished(); + errorStr = tr("Image request is pending"); + // Start a request on the main thread, concluding the initialisation + moveToThread(qApp->thread()); + QMetaObject::invokeMethod(this, + &AbstractThumbnailResponse::startRequest); + // From this point, access to `image` and `errorStr` must be guarded + // by `lock` } protected: @@ -74,7 +77,7 @@ class AbstractThumbnailResponse : public QQuickImageResponse { emit finished(); } - Connection* const connection = nullptr; + const TimelineWidget* const timeline; const QString mediaId{}; const QSize requestedSize{}; @@ -104,19 +107,30 @@ class AbstractThumbnailResponse : public QQuickImageResponse { } }; +namespace { +const auto NoConnectionError = + AbstractThumbnailResponse::tr("No connection to perform image request"); +} + class ThumbnailResponse : public AbstractThumbnailResponse { Q_OBJECT public: using AbstractThumbnailResponse::AbstractThumbnailResponse; - ~ThumbnailResponse() override = default; private slots: void startRequest() override { - Q_ASSERT(QThread::currentThread() == connection->thread()); + Q_ASSERT(QThread::currentThread() == qApp->thread()); + + const auto* currentRoom = timeline->currentRoom(); + if (!currentRoom) { + finish({}, NoConnectionError); + return; + } + + job = currentRoom->connection()->getThumbnail(mediaId, requestedSize); - job = connection->getThumbnail(mediaId, requestedSize); // Connect to any possible outcome including abandonment // to make sure the QML thread is not left stuck forever. connect(job, &BaseJob::finished, this, [this] { @@ -157,19 +171,15 @@ class AvatarResponse : public AbstractThumbnailResponse { using AbstractThumbnailResponse::AbstractThumbnailResponse; private: - Quotient::Room* room = nullptr; - void startRequest() override { - Q_ASSERT(QThread::currentThread() == connection->thread()); - const auto parts = mediaId.split(u'/'); - if (parts.size() > 2) { // Not tr() because it's internal error - qCCritical(THUMBNAILS) << "Avatar reference '" << mediaId - << "' must look like 'roomid[/userid]'"; - Q_ASSERT(parts.size() <= 2); + Q_ASSERT(QThread::currentThread() == qApp->thread()); + + Quotient::Room* currentRoom = timeline->currentRoom(); + if (!currentRoom) { + finish({}, NoConnectionError); + return; } - room = connection->room(parts.front()); - Q_ASSERT(room != nullptr); // NB: both Room:avatar() and User::avatar() invocations return an image // available right now and, if needed, request one with the better @@ -179,17 +189,21 @@ class AvatarResponse : public AbstractThumbnailResponse { // respectively. const auto& w = requestedSize.width(); const auto& h = requestedSize.height(); - if (parts.size() == 1) { + if (mediaId.startsWith(u'!')) { + if (mediaId != currentRoom->id()) { + currentRoom = currentRoom->connection()->room(mediaId); + Q_ASSERT(currentRoom != nullptr); + } // As of libQuotient 0.8, Room::avatar() is the only call in the // Room::avatar*() family that substitutes the counterpart's // avatar for a direct chat avatar. - prepareResult(room->avatar(w, h)); + prepareResult(currentRoom->avatar(w, h)); return; } - auto* user = room->user(parts.back()); + auto* user = currentRoom->user(mediaId); Q_ASSERT(user != nullptr); - prepareResult(user->avatar(w, h, room)); + prepareResult(user->avatar(w, h, currentRoom)); } void prepareResult(const QImage& avatar) @@ -202,18 +216,16 @@ class AvatarResponse : public AbstractThumbnailResponse { #include "thumbnailprovider.moc" // Because we define a Q_OBJECT in the cpp file +template <> +QQuickImageResponse* AvatarProvider::requestImageResponse( + const QString& id, const QSize& requestedSize) +{ + return new AvatarResponse(timeline, id, requestedSize); +} + +template <> QQuickImageResponse* ThumbnailProvider::requestImageResponse( - const QString& id, const QSize& requestedSize) + const QString& id, const QSize& requestedSize) { - auto size = requestedSize; - // Force integer overflow if the value is -1 - may cause issues when - // screens resolution becomes 100K+ each dimension :-D - if (size.width() == -1) - size.setWidth(ushort(-1)); - if (size.height() == -1) - size.setHeight(ushort(-1)); - auto* const c = m_connection.loadRelaxed(); - if (id.startsWith(u'!')) - return new AvatarResponse(c, id, size); - return new ThumbnailResponse(c, id, size); + return new ThumbnailResponse(timeline, id, requestedSize); } diff --git a/client/thumbnailprovider.h b/client/thumbnailprovider.h index d0b8bdab..8964d520 100644 --- a/client/thumbnailprovider.h +++ b/client/thumbnailprovider.h @@ -9,27 +9,21 @@ #pragma once #include -#include -namespace Quotient { - class Connection; -} +class TimelineWidget; -class ThumbnailProvider : public QQuickAsyncImageProvider { +template +class ImageProviderTemplate : public QQuickAsyncImageProvider { public: - explicit ThumbnailProvider(Quotient::Connection* connection = nullptr) - : m_connection(connection) - { } + explicit ImageProviderTemplate(TimelineWidget* parent) : timeline(parent) {} QQuickImageResponse* requestImageResponse( const QString& id, const QSize& requestedSize) override; - void setConnection(Quotient::Connection* connection) - { - m_connection.storeRelaxed(connection); - } - private: - QAtomicPointer m_connection; - Q_DISABLE_COPY(ThumbnailProvider) + const TimelineWidget* const timeline; + Q_DISABLE_COPY(ImageProviderTemplate) }; + +using AvatarProvider = ImageProviderTemplate; +using ThumbnailProvider = ImageProviderTemplate; diff --git a/client/timelinewidget.cpp b/client/timelinewidget.cpp index af49d556..c7c8af4f 100644 --- a/client/timelinewidget.cpp +++ b/client/timelinewidget.cpp @@ -25,7 +25,6 @@ using Quotient::operator""_ls; TimelineWidget::TimelineWidget(ChatRoomWidget* chatRoomWidget) : QQuickWidget(chatRoomWidget) , m_messageModel(new MessageEventModel(this)) - , m_thumbnailProvider(new ThumbnailProvider()) , indexToMaybeRead(-1) , readMarkerOnScreen(false) , roomWidget(chatRoomWidget) @@ -45,7 +44,8 @@ TimelineWidget::TimelineWidget(ChatRoomWidget* chatRoomWidget) setResizeMode(SizeRootObjectToView); - engine()->addImageProvider("thumbnail"_ls, m_thumbnailProvider); + engine()->addImageProvider("avatar"_ls, new AvatarProvider(this)); + engine()->addImageProvider("thumbnail"_ls, new ThumbnailProvider(this)); auto* ctxt = rootContext(); ctxt->setContextProperty("messageModel"_ls, m_messageModel); @@ -84,9 +84,6 @@ void TimelineWidget::setRoom(QuaternionRoom* newRoom) indicesOnScreen.clear(); indexToMaybeRead = -1; - // Update the image provider upfront to allow image requests from - // QML bindings to MessageEventModel::roomChanged - m_thumbnailProvider->setConnection(newRoom ? newRoom->connection() : nullptr); m_messageModel->changeRoom(newRoom); if (newRoom) { connect(newRoom, &Quotient::Room::fullyReadMarkerMoved, this, [this] { diff --git a/client/timelinewidget.h b/client/timelinewidget.h index c412b038..e9cc7078 100644 --- a/client/timelinewidget.h +++ b/client/timelinewidget.h @@ -9,7 +9,6 @@ class ChatRoomWidget; class MessageEventModel; -class ThumbnailProvider; class QuaternionRoom; class TimelineWidget : public QQuickWidget { @@ -46,7 +45,6 @@ public slots: private: MessageEventModel* m_messageModel; - ThumbnailProvider* m_thumbnailProvider; QString m_selectedText; using timeline_index_t = Quotient::TimelineItem::index_t;