Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create ThreadView to track threads in a room #825

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ target_sources(${QUOTIENT_LIB_NAME} PUBLIC FILE_SET HEADERS BASE_DIRS .
Quotient/events/keyverificationevent.h
Quotient/keyimport.h
Quotient/qt_connection_util.h
Quotient/threadview.h
PRIVATE
Quotient/function_traits.cpp
Quotient/networkaccessmanager.cpp
Expand Down Expand Up @@ -247,6 +248,7 @@ target_sources(${QUOTIENT_LIB_NAME} PUBLIC FILE_SET HEADERS BASE_DIRS .
Quotient/e2ee/cryptoutils.cpp
Quotient/e2ee/sssshandler.cpp
Quotient/keyimport.cpp
Quotient/threadview.cpp
libquotientemojis.qrc
)

Expand Down
5 changes: 3 additions & 2 deletions Quotient/events/roommessageevent.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -318,9 +318,10 @@ QString RoomMessageEvent::threadRootEventId() const
const auto relation = relatesTo();
if (relation && relation.value().type == EventRelation::ThreadType) {
return relation.value().eventId;
} else {
return unsignedPart<QJsonObject>("m.relations"_ls)[EventRelation::ThreadType].toString();
} else if (unsignedPart<QJsonObject>("m.relations"_L1).contains(EventRelation::ThreadType)) {
return id();
}
return {};
}

namespace {
Expand Down
4 changes: 4 additions & 0 deletions Quotient/events/roommessageevent.h
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,10 @@ class QUOTIENT_API RoomMessageEvent : public RoomEvent {
//!
//! \note This will return the ID of the event if it is the thread root.
//!
//! \note If the event is the thread root event and has not been updated with the server-side
//! the function will return and empty string as we can tell if the message
nvrWhere marked this conversation as resolved.
Show resolved Hide resolved
//! is threaded.
//!
//! \return The event ID of the thread root if threaded, an empty string otherwise.
QString threadRootEventId()const;

Expand Down
44 changes: 44 additions & 0 deletions Quotient/room.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "roommember.h"
#include "roomstateview.h"
#include "syncdata.h"
#include "threadview.h"
#include "user.h"

#include "csapi/account-data.h"
Expand Down Expand Up @@ -123,6 +124,8 @@ class Q_DECL_HIDDEN Room::Private {
// about the timeline.
EventStats partiallyReadStats {}, unreadStats {};

ThreadView threads;

// For storing a list of current member names for the purpose of disambiguation.
QMultiHash<QString, QString> memberNameMap;
QStringList membersInvited;
Expand Down Expand Up @@ -270,6 +273,8 @@ class Q_DECL_HIDDEN Room::Private {
Timeline::size_type moveEventsToTimeline(RoomEventsRange events,
EventsPlacement placement);

void updateThread(const RoomEvent* event);

/**
* Remove events from the passed container that are already in the timeline
*/
Expand Down Expand Up @@ -555,6 +560,8 @@ const Room::PendingEvents& Room::pendingEvents() const
return d->unsyncedEvents;
}

const ThreadView& Room::threads() const { return d->threads; }

int Room::requestedHistorySize() const
{
return eventsHistoryJob() != nullptr ? d->lastRequestedHistorySize : 0;
Expand Down Expand Up @@ -1752,12 +1759,49 @@ Room::Private::moveEventsToTimeline(RoomEventsRange events,
if (auto n = q->checkForNotifications(ti); n.type != Notification::None)
notifications.insert(eId, n);
Q_ASSERT(q->findInTimeline(eId)->event()->id() == eId);
updateThread(ti.event());
}
const auto insertedSize = (index - baseIndex) * placement;
Q_ASSERT(insertedSize == int(events.size()));
return Timeline::size_type(insertedSize);
}

void Room::Private::updateThread(const RoomEvent* event)
{
const auto rme = eventCast<const RoomMessageEvent>(event);
if (rme == nullptr) {
return;
}
if (!rme->isThreaded()) {
return;
}

if (threads.exisits(rme->threadRootEventId())) {
auto thread = threads.getThread(rme->threadRootEventId());
const auto threadLatestIndex = eventsIndex.constFind(thread->latestEventId());
if (threadLatestIndex == eventsIndex.cend()) {
// Assume this is the latest event. This shouldn't happen but we can work around it.
thread->addEvent(rme, true, rme->senderId() == connection->userId());
return;
}

const auto eventIndexIt = eventsIndex.constFind(rme->id());
if (eventIndexIt == eventsIndex.cend()) {
qCCritical(EVENTS) << rme->id() << "not in the timeline. Update a thread after moving the event to timeline.";
}

thread->addEvent(rme, *eventIndexIt > *threadLatestIndex, rme->senderId() == connection->userId());
} else {
if (!event->id().isEmpty()) {
threads.add(std::make_unique<Thread>(rme->threadRootEventId(),
// For pending events we can get the full correct details when the remote echo comes in.
rme->id().isEmpty() ? rme->threadRootEventId() : rme->id(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rme->id() is exactly the same as event->id() in the enclosing if. You either have to remove the enclosing if (which is perfectly fine, I think?), or you don't need to special-case pending events here because they won't reach this code.

rme->id().isEmpty() ? 1 : 2,
rme->senderId() == connection->userId()));
}
}
}

const Avatar& Room::memberAvatarObject(const QString& memberId) const
{
return connection()->userAvatar(member(memberId).avatarUrl());
Expand Down
4 changes: 4 additions & 0 deletions Quotient/room.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ struct MemberSorter;
class LeaveRoomJob;
class SetRoomStateWithKeyJob;
class RedactEventJob;
class ThreadView;

/** The data structure used to expose file transfer information to views
*
Expand Down Expand Up @@ -363,6 +364,9 @@ class QUOTIENT_API Room : public QObject {
//!
//! Same as messageEvents().crend()
rev_iter_t historyEdge() const;

const ThreadView& threads() const;

//! \brief Get an iterator for the position beyond the latest arrived event
//!
//! Same as messageEvents().cend()
Expand Down
77 changes: 77 additions & 0 deletions Quotient/threadview.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
// SPDX-FileCopyrightText: 2024 James Graham <james.h.graham@protonmail.com>
// SPDX-License-Identifier: LGPL-2.1-or-later

#include "threadview.h"

#include "events/roommessageevent.h"

using namespace Quotient;

void ThreadView::add(std::unique_ptr<Thread> thread)
{
_threads.push_back(std::move(thread));
}

bool ThreadView::erase(const QString& threadRootId)
{
auto threadIt = std::find_if(_threads.begin(), _threads.end(), [threadRootId](const auto& thread) { return thread->threadRootId() == threadRootId; });
if (threadIt == _threads.end()) {
return false;
}
_threads.erase(threadIt);
return true;
nvrWhere marked this conversation as resolved.
Show resolved Hide resolved
}

bool ThreadView::exisits(const QString& threadRootId) const
nvrWhere marked this conversation as resolved.
Show resolved Hide resolved
{
return std::find_if(_threads.begin(), _threads.end(), [threadRootId](const auto& thread) { return thread->threadRootId() == threadRootId; }) != _threads.end();
nvrWhere marked this conversation as resolved.
Show resolved Hide resolved
}

Thread* ThreadView::getThread(const QString& threadRootId) const
{
auto threadIt = std::find_if(_threads.begin(), _threads.end(), [threadRootId](const auto& thread) { return thread->threadRootId() == threadRootId; });
nvrWhere marked this conversation as resolved.
Show resolved Hide resolved
if (threadIt == _threads.end()) {
return nullptr;
}
return threadIt->get();
}

Thread::Thread(QString threadRootId, QString latestEventId, int size, bool localUserParticipated)
nvrWhere marked this conversation as resolved.
Show resolved Hide resolved
: _threadRootId(threadRootId), _latestEventId(threadRootId), _size(size), _localUserParticipated(localUserParticipated)
{}

QString Thread::threadRootId() const
{
return _threadRootId;
}

QString Thread::latestEventId() const
{
return _latestEventId;
}

int Thread::size() const
{
return _size;
}

bool Thread::localUserParticipated() const
{
return _localUserParticipated;
}

bool Thread::addEvent(const RoomMessageEvent* event, bool isLatest, bool isLocalUser)
{
if (event->threadRootEventId() != _threadRootId) {
return false;
}
if (isLatest) {
_latestEventId = event->id();
}
_size++;
nvrWhere marked this conversation as resolved.
Show resolved Hide resolved
if (!_localUserParticipated) {
_localUserParticipated = isLocalUser;
}

return true;
}
49 changes: 49 additions & 0 deletions Quotient/threadview.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// SPDX-FileCopyrightText: 2024 James Graham <james.h.graham@protonmail.com>
// SPDX-License-Identifier: LGPL-2.1-or-later

#pragma once

#include <QString>
#include <QJsonObject>

#include "quotient_export.h"

namespace Quotient {

class RoomMessageEvent;

class QUOTIENT_API Thread {
public:
explicit Thread(QString threadRootId, QString latestEventId, int size, bool localUserParticipated);

QString threadRootId() const;
QString latestEventId() const;
int size() const;
bool localUserParticipated() const;

bool addEvent(const RoomMessageEvent* event, bool isLatest, bool isLocalUser);

private:
const QString _threadRootId;
QString _latestEventId;
int _size;
bool _localUserParticipated;
};

class QUOTIENT_API ThreadView {
public:
ThreadView() = default;

void add(std::unique_ptr<Thread> thread);

bool erase(const QString& threadRootId);

bool exisits(const QString& threadRootId) const;

Thread* getThread(const QString& threadRootId) const;

private:
std::vector<std::unique_ptr<Thread>> _threads;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why std::unique_ptr and not Thread itself? I don't see polymorphism being involved, and I'm not sure what other reasons might require dynamic allocation of a structure the size of 4 pointers.

Copy link
Collaborator Author

@nvrWhere nvrWhere Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So currently the Thread object is not copy assignable by design so this makes erasing impossible. Maybe however this is just a sign that std::vector is not the right container

Copy link
Member

@KitsuneRal KitsuneRal Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No copy assignability should not be a problem for std::vector, it can deal with move-only objects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see it now: there's a const field there, which prevents it from being move assignable, and that's a problem for std::vector::erase(). I might look at pulling that field out of Thread and making _threads a hash-map.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After quickly playing around with code - so far it looks like you can make ThreadView simply a QHash<QString, Thread> and implement all the logic you have in ThreadView inside Room::Private::updateThread(). Thread can be a simple struct (Q_GADGET?). That won't look quite nice when you expose threads outside of Room; but if you really need to add the thread root id you can have a separate plain structure for that (possibly but not necessarily derived from Thread). In fact, the library already has a similar case with FileTransferInfo and FileTransferPrivateInfo structures.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I ended up with after playing with the code, for what it's worth. This is the meat of Room::Private::updateThread(), assuming that threads is now QHash<QString, Thread> (potentially aliased as ThreadView for API convenience), Thread is a plain struct with public fields, and that pending events are admitted in the thread (which is not happening yet, as doSendEvent() doesn't update threads).

I can see that most of this code can be pulled away to Thread::addEvent() again, with necessary datapoints from Room passed as parameters but I'll leave it at that. My main point is that you don't really need ThreadView as a separate class (so far, at least) - once you have found a thread by its root id, the rest is internal to the thread class.

    auto& thread = threads[rme->threadRootEventId()];
    thread._localUserParticipated |= rme->senderId() == connection->userId();
    if (++thread._size == 1) { // The thread has just been created
        // For pending events we can get the full correct details when the remote echo comes in.
        thread._latestEventId = rme->id().isEmpty() ? rme->threadRootEventId() : rme->id();
        // When we can't find the root we assume its a historical event that will load later if
        // we can find it we assume a new thread was just created.
        if (!rme->id().isEmpty() && eventsIndex.contains(rme->threadRootEventId()))
            ++thread._size; // Add the thread root event to the size number
    } else {
        const auto threadLatestIndex = eventsIndex.constFind(thread._latestEventId);
        if (threadLatestIndex == eventsIndex.cend()) {
            // Assume this is the latest event. This shouldn't happen but we can work around it.
            thread._latestEventId = rme->id().isEmpty() ? rme->threadRootEventId() : rme->id();
            return;
        }

        const auto eventIndexIt = eventsIndex.constFind(rme->id());
        if (QUO_ALARM_X(
                eventIndexIt == eventsIndex.cend(),
                rme->id()
                    + u"not in the timeline. Update a thread after moving the event to timeline."_s))
            return;

        if (*eventIndexIt > *threadLatestIndex)
            thread._latestEventId = rme->id();
    }

};

} // namespace Quotient
Loading