From 37d6033df4247bf18c038041bab9b74e71447697 Mon Sep 17 00:00:00 2001 From: James Graham Date: Wed, 31 Jul 2024 17:57:11 +0000 Subject: [PATCH] Manage MessageContentModels properly so we don't leak memory. - Manage MessageContentModels properly so we don't leak memory creating new ones every time the role is refreshed. - Parent and reply MessageContentModels to their message to make sure they get cleaned up when the parent is deleted. - Make sure ReactionModels are cleaned up on room change to stop that list just growing. --- src/models/messagecontentmodel.cpp | 28 +++++++++++++++++----------- src/models/messagecontentmodel.h | 8 ++++++-- src/models/messageeventmodel.cpp | 19 +++++++++++-------- src/models/messageeventmodel.h | 2 ++ src/neochatroom.cpp | 1 + 5 files changed, 37 insertions(+), 21 deletions(-) diff --git a/src/models/messagecontentmodel.cpp b/src/models/messagecontentmodel.cpp index 67e9b1f83..bdb8347b4 100644 --- a/src/models/messagecontentmodel.cpp +++ b/src/models/messagecontentmodel.cpp @@ -31,8 +31,8 @@ using namespace Quotient; -MessageContentModel::MessageContentModel(NeoChatRoom *room, const Quotient::RoomEvent *event, bool isReply, bool isPending) - : QAbstractListModel(nullptr) +MessageContentModel::MessageContentModel(NeoChatRoom *room, const Quotient::RoomEvent *event, bool isReply, bool isPending, MessageContentModel *parent) + : QAbstractListModel(parent) , m_room(room) , m_eventId(event != nullptr ? event->id() : QString()) , m_eventSenderId(event != nullptr ? event->senderId() : QString()) @@ -43,8 +43,8 @@ MessageContentModel::MessageContentModel(NeoChatRoom *room, const Quotient::Room initializeModel(); } -MessageContentModel::MessageContentModel(NeoChatRoom *room, const QString &eventId, bool isReply, bool isPending) - : QAbstractListModel(nullptr) +MessageContentModel::MessageContentModel(NeoChatRoom *room, const QString &eventId, bool isReply, bool isPending, MessageContentModel *parent) + : QAbstractListModel(parent) , m_room(room) , m_eventId(eventId) , m_isPending(isPending) @@ -58,15 +58,16 @@ void MessageContentModel::initializeModel() Q_ASSERT(m_room != nullptr); // Allow making a model for an event that is being downloaded but will appear later // e.g. a reply, but we need an ID to know when it has arrived. - Q_ASSERT(!m_eventId.isEmpty()); + // Also note that a pending event may not have an event ID yet but as long as we have an event + // pointer we can pass out the transaction ID until it is set. + Q_ASSERT(!m_eventId.isEmpty() || m_event != nullptr); Quotient::connectUntil(m_room.get(), &NeoChatRoom::extraEventLoaded, this, [this](const QString &eventId) { if (m_room != nullptr) { if (eventId == m_eventId) { - m_event = loadEvent(m_room->getEvent(eventId)->fullJson()); - Q_EMIT eventUpdated(); + intiializeEvent(m_room->getEvent(eventId)); updateReplyModel(); - resetContent(); + resetModel(); return true; } } @@ -164,7 +165,12 @@ void MessageContentModel::intiializeEvent(const QString &eventId) void MessageContentModel::intiializeEvent(const Quotient::RoomEvent *event) { m_event = loadEvent(event->fullJson()); - auto senderId = event->senderId(); + // a pending event may not previously have had an event ID so update. + if (m_eventId.isEmpty()) { + m_eventId = m_event->id(); + } + + auto senderId = m_event->senderId(); // A pending event might not have a sender ID set yet but in that case it must // be the local member. if (senderId.isEmpty()) { @@ -422,9 +428,9 @@ void MessageContentModel::updateReplyModel() const auto replyEvent = m_room->findInTimeline(eventHandler.getReplyId()); if (replyEvent == m_room->historyEdge()) { - m_replyModel = new MessageContentModel(m_room, eventHandler.getReplyId(), true); + m_replyModel = new MessageContentModel(m_room, eventHandler.getReplyId(), true, false, this); } else { - m_replyModel = new MessageContentModel(m_room, replyEvent->get(), true); + m_replyModel = new MessageContentModel(m_room, replyEvent->get(), true, false, this); } connect(m_replyModel, &MessageContentModel::eventUpdated, this, [this]() { diff --git a/src/models/messagecontentmodel.h b/src/models/messagecontentmodel.h index 1e086a72b..857317c95 100644 --- a/src/models/messagecontentmodel.h +++ b/src/models/messagecontentmodel.h @@ -75,8 +75,12 @@ public: }; Q_ENUM(Roles) - explicit MessageContentModel(NeoChatRoom *room, const Quotient::RoomEvent *event, bool isReply = false, bool isPending = false); - MessageContentModel(NeoChatRoom *room, const QString &eventId, bool isReply = false, bool isPending = false); + explicit MessageContentModel(NeoChatRoom *room, + const Quotient::RoomEvent *event, + bool isReply = false, + bool isPending = false, + MessageContentModel *parent = nullptr); + MessageContentModel(NeoChatRoom *room, const QString &eventId, bool isReply = false, bool isPending = false, MessageContentModel *parent = nullptr); bool showAuthor() const; void setShowAuthor(bool showAuthor); diff --git a/src/models/messageeventmodel.cpp b/src/models/messageeventmodel.cpp index 2ed17187a..ed98d426c 100644 --- a/src/models/messageeventmodel.cpp +++ b/src/models/messageeventmodel.cpp @@ -88,7 +88,6 @@ void MessageEventModel::setRoom(NeoChatRoom *room) // HACK: Reset the model to a null room first to make sure QML dismantles // last room's objects before the room is actually changed beginResetModel(); - m_readMarkerModels.clear(); m_currentRoom->disconnect(this); m_currentRoom = nullptr; endResetModel(); @@ -96,6 +95,9 @@ void MessageEventModel::setRoom(NeoChatRoom *room) // Don't clear the member objects until the model has been fully reset and all // refs cleared. m_memberObjects.clear(); + m_contentModels.clear(); + m_reactionModels.clear(); + m_readMarkerModels.clear(); } beginResetModel(); @@ -442,13 +444,8 @@ QVariant MessageEventModel::data(const QModelIndex &idx, int role) const } if (role == ContentModelRole) { - if (!evt.isStateEvent() && !evt.id().isEmpty()) { - return QVariant::fromValue(new MessageContentModel(m_currentRoom, &evt)); - } - if (evt.isStateEvent()) { - if (evt.matrixType() == QStringLiteral("org.matrix.msc3672.beacon_info")) { - return QVariant::fromValue(new MessageContentModel(m_currentRoom, &evt)); - } + if (m_contentModels.contains(evt.id())) { + return QVariant::fromValue(m_contentModels.at(evt.id()).get()); } return {}; } @@ -634,6 +631,12 @@ void MessageEventModel::createEventObjects(const Quotient::RoomEvent *event) m_memberObjects[senderId] = std::unique_ptr(new NeochatRoomMember(m_currentRoom, senderId)); } + if (!m_contentModels.contains(eventId)) { + if (!event->isStateEvent() || event->matrixType() == QStringLiteral("org.matrix.msc3672.beacon_info")) { + m_contentModels[eventId] = std::unique_ptr(new MessageContentModel(m_currentRoom, event)); + } + } + // ReadMarkerModel handles updates to add and remove markers, we only need to // handle adding and removing whole models here. if (m_readMarkerModels.contains(eventId)) { diff --git a/src/models/messageeventmodel.h b/src/models/messageeventmodel.h index ef895f4ee..6fa197c0e 100644 --- a/src/models/messageeventmodel.h +++ b/src/models/messageeventmodel.h @@ -8,6 +8,7 @@ #include #include "linkpreviewer.h" +#include "messagecontentmodel.h" #include "neochatroom.h" #include "neochatroommember.h" #include "pollhandler.h" @@ -117,6 +118,7 @@ private: KFormat m_format; std::map> m_memberObjects; + std::map> m_contentModels; QMap> m_readMarkerModels; QMap> m_reactionModels; diff --git a/src/neochatroom.cpp b/src/neochatroom.cpp index 378a2222b..748802a12 100644 --- a/src/neochatroom.cpp +++ b/src/neochatroom.cpp @@ -1748,6 +1748,7 @@ void NeoChatRoom::downloadEventFromServer(const QString &eventId) connect(job, &BaseJob::success, this, [this, job, eventId] { // The event may have arrived in the meantime so check it's not in the timeline. if (findInTimeline(eventId) != historyEdge()) { + Q_EMIT extraEventLoaded(eventId); return; }