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

Merged
merged 12 commits into from
Dec 10, 2024
Merged

Conversation

nvrWhere
Copy link
Collaborator

@nvrWhere nvrWhere commented Nov 2, 2024

This is some initial work to make supporting threads easier. As part of the thread spec the root event gets some aggregation in the unsigned of the root event. However this is not automatically sync when the thread is started or a new message is added so we can't just rely on it's presence.

This leaves us with 2 options:

  1. Update the root event from the server every time a thread is started or updated - this will have a delay and hit the server
  2. Track this info ourselves

This PR does number 2.

A new ThreadView is created which holds a list of Thread objects each representing the info for a single thread. As new events are added to the timeline each one is now checked to see if it is threaded and if so a Thread is either updated with the info as required or created if it is a previously unknown thread.

This is intentionally a very simple start, I intend at some point for it to contain it's own timeline but before we can do that we would have to decide how we want to manage having the same event in multiple timelines.

@KitsuneRal
Copy link
Member

I happen to have the answer to the question about having the same event in several timelines. We really should separate the concerns of storing events and referencing them. Each room should have its own storage of events (could be unique pointers because we rarely delete events atm, but we could also use shared pointers or Qt's shared data paradigm); any event container (state or timeline) would then simply refer to the event. Event ids are obvious keys for the hash map storage, to find them whenever they are mentioned in sync/history batches. That really is it. It cannot be implemented without breaking the API because Room::Timeline is exposed in the API; but that's certainly something achievable for 0.10 and shouldn't disrupt client code too much.

@nvrWhere
Copy link
Collaborator Author

nvrWhere commented Nov 2, 2024

I happen to have the answer to the question about having the same event in several timelines. We really should separate the concerns of storing events and referencing them. Each room should have its own storage of events (could be unique pointers because we rarely delete events atm, but we could also use shared pointers or Qt's shared data paradigm); any event container (state or timeline) would then simply refer to the event. Event ids are obvious keys for the hash map storage, to find them whenever they are mentioned in sync/history batches. That really is it. It cannot be implemented without breaking the API because Room::Timeline is exposed in the API; but that's certainly something achievable for 0.10 and shouldn't disrupt client code too much.

As it so happens this is exactly what I was thinking.

Copy link

sonarqubecloud bot commented Nov 2, 2024

Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

A few comments on the current code.

Quotient/threadview.cpp Outdated Show resolved Hide resolved
Quotient/threadview.cpp Outdated Show resolved Hide resolved
Quotient/threadview.cpp Outdated Show resolved Hide resolved
Quotient/threadview.cpp Outdated Show resolved Hide resolved
Quotient/threadview.cpp Outdated Show resolved Hide resolved
Quotient/threadview.cpp Outdated Show resolved Hide resolved
Quotient/threadview.h Outdated Show resolved Hide resolved
@KitsuneRal KitsuneRal added the enhancement A feature or change request for the library label Nov 18, 2024
@nvrWhere nvrWhere requested a review from KitsuneRal November 21, 2024 18:24
gtad/gtad Outdated Show resolved Hide resolved
Quotient/room.cpp Outdated Show resolved Hide resolved
Quotient/room.cpp Outdated Show resolved Hide resolved
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Aside from all what I wrote above - once you are through with most of functionality, please add a test (or several), either as a unit test (I doubt that it's even possible though) or to quotest.cpp. Happy to discuss the details.

@nvrWhere nvrWhere force-pushed the nvrwhere/threadview branch from f7b965d to f059e78 Compare November 30, 2024 14:36
@nvrWhere
Copy link
Collaborator Author

Aside from all what I wrote above - once you are through with most of functionality, please add a test (or several), either as a unit test (I doubt that it's even possible though) or to quotest.cpp. Happy to discuss the details.

So I added some unit tests for Thread and a quotest for a basic thread, tell me if you want more.

@nvrWhere nvrWhere force-pushed the nvrwhere/threadview branch 3 times, most recently from 1cecf8d to acf7b60 Compare November 30, 2024 18:03
@nvrWhere nvrWhere requested a review from KitsuneRal December 1, 2024 12:58
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

I really love where this is going. A few more things though.

Quotient/room.cpp Outdated Show resolved Hide resolved
Quotient/room.cpp Outdated Show resolved Hide resolved
autotests/testthread.h Outdated Show resolved Hide resolved
autotests/testutils.h Outdated Show resolved Hide resolved
autotests/testutils.h Show resolved Hide resolved
quotest/quotest.cpp Outdated Show resolved Hide resolved
quotest/quotest.cpp Outdated Show resolved Hide resolved
@nvrWhere nvrWhere requested a review from KitsuneRal December 4, 2024 18:29
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

A couple brush-ups, and I think we're there.

quotest/quotest.cpp Outdated Show resolved Hide resolved
quotest/quotest.cpp Outdated Show resolved Hide resolved
@nvrWhere nvrWhere force-pushed the nvrwhere/threadview branch from e89799d to 3084338 Compare December 10, 2024 16:42
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Thanks a ton!

@KitsuneRal KitsuneRal merged commit 8d9f044 into dev Dec 10, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or change request for the library
Projects
Status: 0.9 - Done
Development

Successfully merging this pull request may close these issues.

3 participants