From 356e8eefe0b5e6502402ee3d6e8937b3b03a72f8 Mon Sep 17 00:00:00 2001 From: James Graham Date: Tue, 2 Jan 2024 21:22:08 +0000 Subject: [PATCH] Refactor PollHandler Refactor PollHandler to make it more reliable. This ended up with much more code than I expected as the original intent was just to stop a crash when switching rooms. - Using a event string was flaky, changing to using an event reference is more reliable. - Since we're only creating them from NeoChatRoom there is no need to to be able to set properties from QML so only read properties. - Pass from the MessageEventModel rather than an invokable method. - Create a basic test suite - Create properties in PollHandler to remove the need to use content in PollDelegate, this means content is no longer a required role. --- autotests/CMakeLists.txt | 6 + .../data/test-pollhandlerstart-sync.json | 38 +++++ autotests/pollhandlertest.cpp | 70 ++++++++++ src/models/messageeventmodel.cpp | 41 +++--- src/models/messageeventmodel.h | 3 +- src/neochatroom.cpp | 21 ++- src/neochatroom.h | 10 +- src/pollhandler.cpp | 130 ++++++++++-------- src/pollhandler.h | 41 +++--- src/qml/PollDelegate.qml | 15 +- 10 files changed, 259 insertions(+), 116 deletions(-) create mode 100644 autotests/data/test-pollhandlerstart-sync.json create mode 100644 autotests/pollhandlertest.cpp diff --git a/autotests/CMakeLists.txt b/autotests/CMakeLists.txt index 1ae30108f..c2efaf2e7 100644 --- a/autotests/CMakeLists.txt +++ b/autotests/CMakeLists.txt @@ -64,3 +64,9 @@ ecm_add_test( LINK_LIBRARIES neochat Qt::Test TEST_NAME windowcontrollertest ) + +ecm_add_test( + pollhandlertest.cpp + LINK_LIBRARIES neochat Qt::Test + TEST_NAME pollhandlertest +) diff --git a/autotests/data/test-pollhandlerstart-sync.json b/autotests/data/test-pollhandlerstart-sync.json new file mode 100644 index 000000000..eb8b52912 --- /dev/null +++ b/autotests/data/test-pollhandlerstart-sync.json @@ -0,0 +1,38 @@ +{ + "timeline": { + "events": [ + { + "content": { + "org.matrix.msc1767.text": "test\n1. option1\n2. option 2", + "org.matrix.msc3381.poll.start": { + "answers": [ + { + "id": "option1", + "org.matrix.msc1767.text": "option1" + }, + { + "id": "option2", + "org.matrix.msc1767.text": "option2" + } + ], + "kind": "org.matrix.msc3381.poll.disclosed", + "max_selections": 1, + "question": { + "body": "test", + "msgtype": "m.text", + "org.matrix.msc1767.text": "test" + } + } + }, + "event_id": "$153456789:example.org", + "origin_server_ts": 1432735824654, + "room_id": "!jEsUZKDJdhlrceRyVU:example.org", + "sender": "@example:example.org", + "type": "org.matrix.msc3381.poll.start", + "unsigned": { + "age": 1232 + } + } + ] + } +} diff --git a/autotests/pollhandlertest.cpp b/autotests/pollhandlertest.cpp new file mode 100644 index 000000000..5c8e06f01 --- /dev/null +++ b/autotests/pollhandlertest.cpp @@ -0,0 +1,70 @@ +// SPDX-FileCopyrightText: 2023 James Graham +// SPDX-License-Identifier: GPL-2.0-only OR GPL-3.0-only OR LicenseRef-KDE-Accepted-GPL + +#include +#include +#include + +#include +#include +#include + +#include "events/pollevent.h" +#include "pollhandler.h" +#include "testutils.h" + +using namespace Quotient; + +class PollHandlerTest : public QObject +{ + Q_OBJECT + +private: + Connection *connection = nullptr; + TestUtils::TestRoom *room = nullptr; + +private Q_SLOTS: + void initTestCase(); + void nullObject(); + void poll(); +}; + +void PollHandlerTest::initTestCase() +{ + connection = Connection::makeMockConnection(QStringLiteral("@bob:kde.org")); + room = new TestUtils::TestRoom(connection, QStringLiteral("#myroom:kde.org"), "test-pollhandlerstart-sync.json"_ls); +} + +// Basically don't crash. +void PollHandlerTest::nullObject() +{ + auto pollHandler = PollHandler(); + + QCOMPARE(pollHandler.hasEnded(), false); + QCOMPARE(pollHandler.answerCount(), 0); + QCOMPARE(pollHandler.question(), QString()); + QCOMPARE(pollHandler.options(), QJsonArray()); + QCOMPARE(pollHandler.answers(), QJsonObject()); + QCOMPARE(pollHandler.counts(), QJsonObject()); + QCOMPARE(pollHandler.kind(), QString()); +} + +void PollHandlerTest::poll() +{ + auto startEvent = eventCast(room->messageEvents().at(0).get()); + auto pollHandler = PollHandler(room, startEvent); + + auto options = QJsonArray{QJsonObject{{"id"_ls, "option1"_ls}, {"org.matrix.msc1767.text"_ls, "option1"_ls}}, + QJsonObject{{"id"_ls, "option2"_ls}, {"org.matrix.msc1767.text"_ls, "option2"_ls}}}; + + QCOMPARE(pollHandler.hasEnded(), false); + QCOMPARE(pollHandler.answerCount(), 0); + QCOMPARE(pollHandler.question(), QStringLiteral("test")); + QCOMPARE(pollHandler.options(), options); + QCOMPARE(pollHandler.answers(), QJsonObject()); + QCOMPARE(pollHandler.counts(), QJsonObject()); + QCOMPARE(pollHandler.kind(), QStringLiteral("org.matrix.msc3381.poll.disclosed")); +} + +QTEST_GUILESS_MAIN(PollHandlerTest) +#include "pollhandlertest.moc" diff --git a/src/models/messageeventmodel.cpp b/src/models/messageeventmodel.cpp index 69f2a5e49..2dbe85b56 100644 --- a/src/models/messageeventmodel.cpp +++ b/src/models/messageeventmodel.cpp @@ -20,6 +20,7 @@ #include "enums/delegatetype.h" #include "eventhandler.h" +#include "events/pollevent.h" #include "models/reactionmodel.h" using namespace Quotient; @@ -34,7 +35,6 @@ QHash MessageEventModel::roleNames() const roles[TimeStringRole] = "timeString"; roles[SectionRole] = "section"; roles[AuthorRole] = "author"; - roles[ContentRole] = "content"; roles[HighlightRole] = "isHighlighted"; roles[SpecialMarksRole] = "marks"; roles[ProgressInfoRole] = "progressInfo"; @@ -65,6 +65,7 @@ QHash MessageEventModel::roleNames() const roles[LatitudeRole] = "latitude"; roles[LongitudeRole] = "longitude"; roles[AssetRole] = "asset"; + roles[PollHandlerRole] = "pollHandler"; return roles; } @@ -101,6 +102,9 @@ void MessageEventModel::setRoom(NeoChatRoom *room) if (const auto &roomMessageEvent = &*event->viewAs()) { createEventObjects(roomMessageEvent); } + if (event->event()->is()) { + m_currentRoom->createPollHandler(eventCast(event->event())); + } } if (m_currentRoom->timelineSize() < 10 && !room->allHistoryLoaded()) { @@ -151,6 +155,9 @@ void MessageEventModel::setRoom(NeoChatRoom *room) } } } + if (event->is()) { + m_currentRoom->createPollHandler(eventCast(event.get())); + } } m_initialized = true; beginInsertRows({}, timelineBaseIndex(), timelineBaseIndex() + int(events.size()) - 1); @@ -160,6 +167,9 @@ void MessageEventModel::setRoom(NeoChatRoom *room) if (const auto &roomMessageEvent = dynamic_cast(event.get())) { createEventObjects(roomMessageEvent); } + if (event->is()) { + m_currentRoom->createPollHandler(eventCast(event.get())); + } } if (rowCount() > 0) { rowBelowInserted = rowCount() - 1; // See #312 @@ -231,6 +241,9 @@ void MessageEventModel::setRoom(NeoChatRoom *room) if (const auto &event = dynamic_cast(&**eventIt)) { createEventObjects(event); } + if (eventIt->event()->is()) { + m_currentRoom->createPollHandler(eventCast(eventIt->event())); + } } refreshEventRoles(eventId, {ReactionRole, ShowReactionsRole, Qt::DisplayRole}); }); @@ -485,28 +498,6 @@ QVariant MessageEventModel::data(const QModelIndex &idx, int role) const return eventHandler.getAuthor(isPending); } - if (role == ContentRole) { - if (evt.isRedacted()) { - auto reason = evt.redactedBecause()->reason(); - return (reason.isEmpty()) ? i18n("[REDACTED]") : i18n("[REDACTED: %1]").arg(evt.redactedBecause()->reason()); - } - - if (auto e = eventCast(&evt)) { - if (e->msgtype() == Quotient::MessageEventType::Location) { - return e->contentJson(); - } - // Cannot use e.contentJson() here because some - // EventContent classes inject values into the copy of the - // content JSON stored in EventContent::Base - return e->hasFileContent() ? QVariant::fromValue(e->content()->originalJson) : QVariant(); - }; - - if (auto e = eventCast(&evt)) { - return QVariant::fromValue(e->image().originalJson); - } - return evt.contentJson(); - } - if (role == HighlightRole) { return eventHandler.isHighlighted(); } @@ -702,6 +693,10 @@ QVariant MessageEventModel::data(const QModelIndex &idx, int role) const return row < static_cast(m_currentRoom->pendingEvents().size()); } + if (role == PollHandlerRole) { + return QVariant::fromValue(m_currentRoom->poll(evt.id())); + } + return {}; } diff --git a/src/models/messageeventmodel.h b/src/models/messageeventmodel.h index 9f4d2e7ae..2a0ef9808 100644 --- a/src/models/messageeventmodel.h +++ b/src/models/messageeventmodel.h @@ -9,6 +9,7 @@ #include "linkpreviewer.h" #include "neochatroom.h" +#include "pollhandler.h" class ReactionModel; @@ -45,7 +46,6 @@ public: TimeStringRole, /**< The timestamp for when the event was sent as a string (in QLocale::ShortFormat). */ SectionRole, /**< The date of the event as a string. */ AuthorRole, /**< The author of the event. */ - ContentRole, /**< The full message content. */ HighlightRole, /**< Whether the event should be highlighted. */ SpecialMarksRole, /**< Whether the event is hidden or not. */ ProgressInfoRole, /**< Progress info when downloading files. */ @@ -83,6 +83,7 @@ public: LatitudeRole, /**< Latitude for a location event. */ LongitudeRole, /**< Longitude for a location event. */ AssetRole, /**< Type of location event, e.g. self pin of the user location. */ + PollHandlerRole, /**< The PollHandler for the event, if any. */ LastRole, // Keep this last }; Q_ENUM(EventRoles) diff --git a/src/neochatroom.cpp b/src/neochatroom.cpp index 762bbbe24..68a17f0f5 100644 --- a/src/neochatroom.cpp +++ b/src/neochatroom.cpp @@ -1725,15 +1725,26 @@ bool NeoChatRoom::canEncryptRoom() const return !usesEncryption() && canSendState("m.room.encryption"_ls); } -PollHandler *NeoChatRoom::poll(const QString &eventId) +static PollHandler *emptyPollHandler = new PollHandler; + +PollHandler *NeoChatRoom::poll(const QString &eventId) const { + if (auto pollHandler = m_polls[eventId]) { + return pollHandler; + } + return emptyPollHandler; +} + +void NeoChatRoom::createPollHandler(const Quotient::PollStartEvent *event) +{ + if (event == nullptr) { + return; + } + auto eventId = event->id(); if (!m_polls.contains(eventId)) { - auto handler = new PollHandler(this); - handler->setRoom(this); - handler->setPollStartEventId(eventId); + auto handler = new PollHandler(this, event); m_polls.insert(eventId, handler); } - return m_polls[eventId]; } bool NeoChatRoom::downloadTempFile(const QString &eventId) diff --git a/src/neochatroom.h b/src/neochatroom.h index 82e3d2c3e..dfb2a505e 100644 --- a/src/neochatroom.h +++ b/src/neochatroom.h @@ -14,6 +14,7 @@ #include #include "enums/pushrule.h" +#include "events/pollevent.h" #include "pollhandler.h" namespace Quotient @@ -733,7 +734,14 @@ public: * * @sa PollHandler */ - Q_INVOKABLE PollHandler *poll(const QString &eventId); + PollHandler *poll(const QString &eventId) const; + + /** + * @brief Create a PollHandler object for the given event. + * + * @sa PollHandler + */ + void createPollHandler(const Quotient::PollStartEvent *event); /** * @brief Get the full Json data for a given room account data event. diff --git a/src/pollhandler.cpp b/src/pollhandler.cpp index 978ce893b..463cbc5fb 100644 --- a/src/pollhandler.cpp +++ b/src/pollhandler.cpp @@ -3,90 +3,71 @@ #include "pollhandler.h" -#include "events/pollevent.h" #include "neochatroom.h" #include #include -#include #include using namespace Quotient; -PollHandler::PollHandler(QObject *parent) - : QObject(parent) +PollHandler::PollHandler(NeoChatRoom *room, const Quotient::PollStartEvent *pollStartEvent) + : QObject(room) + , m_pollStartEvent(pollStartEvent) { - connect(this, &PollHandler::roomChanged, this, &PollHandler::checkLoadRelations); - connect(this, &PollHandler::pollStartEventIdChanged, this, &PollHandler::checkLoadRelations); + if (room != nullptr && m_pollStartEvent != nullptr) { + connect(room, &NeoChatRoom::aboutToAddNewMessages, this, &PollHandler::updatePoll); + checkLoadRelations(); + } } -NeoChatRoom *PollHandler::room() const +void PollHandler::updatePoll(Quotient::RoomEventsRange events) { - return m_room; -} - -void PollHandler::setRoom(NeoChatRoom *room) -{ - if (m_room == room) { - return; - } - if (m_room) { - disconnect(m_room, nullptr, this, nullptr); - } - connect(room, &NeoChatRoom::aboutToAddNewMessages, this, [this](Quotient::RoomEventsRange events) { - for (const auto &event : events) { - if (event->is()) { - auto plEvent = m_room->currentState().get(); - if (!plEvent) { - continue; - } - auto userPl = plEvent->powerLevelForUser(event->senderId()); - if (event->senderId() == (*m_room->findInTimeline(m_pollStartEventId))->senderId() || userPl >= plEvent->redact()) { - m_hasEnded = true; - m_endedTimestamp = event->originTimestamp(); - Q_EMIT hasEndedChanged(); - } + // This function will never be called if the PollHandler was not initialized with + // a NeoChatRoom as parent and a PollStartEvent so no need to null check. + auto room = dynamic_cast(parent()); + for (const auto &event : events) { + if (event->is()) { + auto plEvent = room->currentState().get(); + if (!plEvent) { + continue; } - if (event->is()) { - handleAnswer(event->contentJson(), event->senderId(), event->originTimestamp()); + auto userPl = plEvent->powerLevelForUser(event->senderId()); + if (event->senderId() == m_pollStartEvent->senderId() || userPl >= plEvent->redact()) { + m_hasEnded = true; + m_endedTimestamp = event->originTimestamp(); + Q_EMIT hasEndedChanged(); } } - }); - m_room = room; - Q_EMIT roomChanged(); -} - -QString PollHandler::pollStartEventId() const -{ - return m_pollStartEventId; -} - -void PollHandler::setPollStartEventId(const QString &eventId) -{ - if (eventId == m_pollStartEventId) { - return; + if (event->is()) { + handleAnswer(event->contentJson(), event->senderId(), event->originTimestamp()); + } + if (event->contentPart("m.relates_to"_ls).contains("rel_type"_ls) + && event->contentPart("m.relates_to"_ls)["rel_type"_ls].toString() == "m.replace"_ls + && event->contentPart("m.relates_to"_ls)["event_id"_ls].toString() == m_pollStartEvent->id()) { + Q_EMIT questionChanged(); + Q_EMIT optionsChanged(); + } } - m_pollStartEventId = eventId; - Q_EMIT pollStartEventIdChanged(); } void PollHandler::checkLoadRelations() { - if (!m_room || m_pollStartEventId.isEmpty()) { - return; - } - m_maxVotes = eventCast(&**m_room->findInTimeline(m_pollStartEventId))->maxSelections(); - auto job = m_room->connection()->callApi(m_room->id(), m_pollStartEventId); - connect(job, &BaseJob::success, this, [this, job]() { + // This function will never be called if the PollHandler was not initialized with + // a NeoChatRoom as parent and a PollStartEvent so no need to null check. + auto room = dynamic_cast(parent()); + m_maxVotes = m_pollStartEvent->maxSelections(); + auto job = room->connection()->callApi(room->id(), m_pollStartEvent->id()); + connect(job, &BaseJob::success, this, [this, job, room]() { for (const auto &event : job->chunk()) { if (event->is()) { - auto plEvent = m_room->currentState().get(); + auto plEvent = room->currentState().get(); if (!plEvent) { continue; } auto userPl = plEvent->powerLevelForUser(event->senderId()); - if (event->senderId() == (*m_room->findInTimeline(m_pollStartEventId))->senderId() || userPl >= plEvent->redact()) { + if (event->senderId() == m_pollStartEvent->senderId() || userPl >= plEvent->redact()) { m_hasEnded = true; m_endedTimestamp = event->originTimestamp(); Q_EMIT hasEndedChanged(); @@ -123,6 +104,22 @@ void PollHandler::handleAnswer(const QJsonObject &content, const QString &sender Q_EMIT answersChanged(); } +QString PollHandler::question() const +{ + if (m_pollStartEvent == nullptr) { + return {}; + } + return m_pollStartEvent->contentPart("org.matrix.msc3381.poll.start"_ls)["question"_ls].toObject()["body"_ls].toString(); +} + +QJsonArray PollHandler::options() const +{ + if (m_pollStartEvent == nullptr) { + return {}; + } + return m_pollStartEvent->contentPart("org.matrix.msc3381.poll.start"_ls)["answers"_ls].toArray(); +} + QJsonObject PollHandler::answers() const { return m_answers; @@ -139,12 +136,25 @@ QJsonObject PollHandler::counts() const return counts; } +QString PollHandler::kind() const +{ + if (m_pollStartEvent == nullptr) { + return {}; + } + return m_pollStartEvent->contentPart("org.matrix.msc3381.poll.start"_ls)["kind"_ls].toString(); +} + void PollHandler::sendPollAnswer(const QString &eventId, const QString &answerId) { Q_ASSERT(eventId.length() > 0); Q_ASSERT(answerId.length() > 0); + auto room = dynamic_cast(parent()); + if (room == nullptr) { + qWarning() << "PollHandler is empty, cannot send an answer."; + return; + } QStringList ownAnswers; - for (const auto &answer : m_answers[m_room->localUser()->id()].toArray()) { + for (const auto &answer : m_answers[room->localUser()->id()].toArray()) { ownAnswers += answer.toString(); } if (ownAnswers.contains(answerId)) { @@ -159,8 +169,8 @@ void PollHandler::sendPollAnswer(const QString &eventId, const QString &answerId } auto response = new PollResponseEvent(eventId, ownAnswers); - handleAnswer(response->contentJson(), m_room->localUser()->id(), QDateTime::currentDateTime()); - m_room->postEvent(response); + handleAnswer(response->contentJson(), room->localUser()->id(), QDateTime::currentDateTime()); + room->postEvent(response); } bool PollHandler::hasEnded() const diff --git a/src/pollhandler.h b/src/pollhandler.h index af545ad03..d50c7b9e8 100644 --- a/src/pollhandler.h +++ b/src/pollhandler.h @@ -3,11 +3,16 @@ #pragma once +#include #include #include #include #include +#include + +#include "events/pollevent.h" + class NeoChatRoom; /** @@ -27,17 +32,17 @@ class PollHandler : public QObject QML_ELEMENT /** - * @brief The current room for the poll. + * @brief The question for the poll. */ - Q_PROPERTY(NeoChatRoom *room READ room WRITE setRoom NOTIFY roomChanged) + Q_PROPERTY(QString question READ question NOTIFY questionChanged) /** - * @brief The Matrix event ID for the event that started the poll. + * @brief The list of possible answers to the poll. */ - Q_PROPERTY(QString pollStartEventId READ pollStartEventId WRITE setPollStartEventId NOTIFY pollStartEventIdChanged) + Q_PROPERTY(QJsonArray options READ options NOTIFY optionsChanged) /** - * @brief The list of answers to the poll from users in the room. + * @brief The list of answer responses to the poll from users in the room. */ Q_PROPERTY(QJsonObject answers READ answers NOTIFY answersChanged) @@ -56,20 +61,23 @@ class PollHandler : public QObject */ Q_PROPERTY(int answerCount READ answerCount NOTIFY answersChanged) + /** + * @brief The kind of the poll. + */ + Q_PROPERTY(QString kind READ kind CONSTANT) + public: - PollHandler(QObject *parent = nullptr); - - NeoChatRoom *room() const; - void setRoom(NeoChatRoom *room); - - QString pollStartEventId() const; - void setPollStartEventId(const QString &eventId); + PollHandler() = default; + PollHandler(NeoChatRoom *room, const Quotient::PollStartEvent *pollStartEvent); bool hasEnded() const; int answerCount() const; + QString question() const; + QJsonArray options() const; QJsonObject answers() const; QJsonObject counts() const; + QString kind() const; /** * @brief Send an answer to the poll. @@ -77,14 +85,15 @@ public: Q_INVOKABLE void sendPollAnswer(const QString &eventId, const QString &answerId); Q_SIGNALS: - void roomChanged(); - void pollStartEventIdChanged(); + void questionChanged(); + void optionsChanged(); void answersChanged(); void hasEndedChanged(); private: - NeoChatRoom *m_room = nullptr; - QString m_pollStartEventId; + const Quotient::PollStartEvent *m_pollStartEvent; + + void updatePoll(Quotient::RoomEventsRange events); void checkLoadRelations(); void handleAnswer(const QJsonObject &object, const QString &sender, QDateTime timestamp); diff --git a/src/qml/PollDelegate.qml b/src/qml/PollDelegate.qml index d410d62c3..9b92c944b 100644 --- a/src/qml/PollDelegate.qml +++ b/src/qml/PollDelegate.qml @@ -16,28 +16,23 @@ import org.kde.neochat MessageDelegate { id: root - /** - * @brief The matrix message content. - */ - required property var content - /** * @brief The poll handler for this poll. * * This contains the required information like what the question, answers and * current number of votes for each is. */ - property var pollHandler: currentRoom.poll(root.eventId) + required property var pollHandler bubbleContent: ColumnLayout { Label { id: questionLabel - text: root.content["org.matrix.msc3381.poll.start"]["question"]["body"] + text: root.pollHandler.question wrapMode: Text.Wrap Layout.fillWidth: true } Repeater { - model: root.content["org.matrix.msc3381.poll.start"]["answers"] + model: root.pollHandler.options delegate: RowLayout { Layout.fillWidth: true CheckBox { @@ -51,14 +46,14 @@ MessageDelegate { wrapMode: Text.Wrap } Label { - visible: root.content["org.matrix.msc3381.poll.start"]["kind"] == "org.matrix.msc3381.poll.disclosed" || pollHandler.hasEnded + visible: root.pollHandler.kind == "org.matrix.msc3381.poll.disclosed" || pollHandler.hasEnded Layout.preferredWidth: contentWidth text: root.pollHandler.counts[modelData["id"]] ?? "0" } } } Label { - visible: root.content["org.matrix.msc3381.poll.start"]["kind"] == "org.matrix.msc3381.poll.disclosed" || root.pollHandler.hasEnded + visible: root.pollHandler.kind == "org.matrix.msc3381.poll.disclosed" || root.pollHandler.hasEnded text: i18np("Based on votes by %1 user", "Based on votes by %1 users", root.pollHandler.answerCount) + (root.pollHandler.hasEnded ? (" " + i18nc("as in 'this vote has ended'", "(Ended)")) : "") font.pointSize: questionLabel.font.pointSize * 0.8 }