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

event graph: add an initial implementation #3061

Merged
merged 5 commits into from Jan 30, 2024
Merged

event graph: add an initial implementation #3061

merged 5 commits into from Jan 30, 2024

Conversation

bnjbvr
Copy link
Member

@bnjbvr bnjbvr commented Jan 26, 2024

This is mostly a demonstration of how to plug the EventGraph with the timeline, add some initial types and documentation, and how little it changes the timeline as a result.

One thing I'm not satisfied with is that the TimelineBuilder::build() function should really be fallible eventually, because the subscribe() would read events from the storage, which is fallible. I've tried to make it so, but in the sliding sync code we're lazily initializing a Timeline using TimelineBuilder, and that can't fail, or we'd need the caller to check the result each time. Needs to be properly solved now, or as the immediate next PR.

I've also started a WIP to move the support for read receipts (not unread counters yet) from the Timeline into the Event graph, as a further proof of concept, and I might wait for it before requesting review here.

I'll move back to draft as soon as I get some test results :-)

Part of #3058.

@bnjbvr bnjbvr requested a review from a team as a code owner January 26, 2024 16:45
@bnjbvr bnjbvr requested review from andybalaam and removed request for a team January 26, 2024 16:45
@bnjbvr bnjbvr marked this pull request as draft January 26, 2024 17:08
Copy link
Contributor

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

I like the direction, and so far as I understand it, this looks good. Thanks!

@bnjbvr
Copy link
Member Author

bnjbvr commented Jan 30, 2024

(Trying a rebase with #3070, to see if I can reproduce the slow test run 🤔)

@bnjbvr bnjbvr marked this pull request as ready for review January 30, 2024 10:11
@bnjbvr
Copy link
Member Author

bnjbvr commented Jan 30, 2024

Above push was another rebase, now that the integration test failure has been fixed in main.

This is mostly a demonstration of how to plug this with the timeline, and how little it changes as a result.

Remove read receipts
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (f5f8f47) 83.72% compared to head (0b7be11) 83.77%.

Files Patch % Lines
crates/matrix-sdk-ui/src/event_graph.rs 82.95% 15 Missing ⚠️
crates/matrix-sdk-ui/src/room_list_service/room.rs 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3061      +/-   ##
==========================================
+ Coverage   83.72%   83.77%   +0.05%     
==========================================
  Files         223      224       +1     
  Lines       23367    23454      +87     
==========================================
+ Hits        19564    19649      +85     
- Misses       3803     3805       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

It's super slow (as it recreates and backpaginates from the start again) and unused in both EX apps, which are our main FFI embedders.
It's only used in test code, so it's not worth exposing to the SlidingSyncRoom object (and the Room already has such a timeline function, if needs be).
@bnjbvr
Copy link
Member Author

bnjbvr commented Jan 30, 2024

@andybalaam I consider there are enough changes that this justifies a new review, if you have some time. The new commits introduce the following changes:

  • make the TimelineBuilder::build() function fallible (to avoid using expect() in that function), which meant sparkling Result in a few places, and making a few FFI functions fallible too.

And two bonus commits, that could be moved to another PR if judged controversial

  • get rid of Room::poll_history as it's untested, unused in both EX apps, and likely too slow (it creates an independent timeline which will re-run its own backpagination and so on)
  • get rid of SlidingSyncRoomExt::timeline(), as it was only used in a test function, and that avoids a few hoops in the one caller (and permits extra refactorings)

Copy link
Contributor

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Still looks good. Note that something like poll_history is definitely going to be needed sometime, but happy for it to be removed if the implementation is no good. Any plan for how to do it better?

@bnjbvr bnjbvr merged commit a356a20 into main Jan 30, 2024
34 checks passed
@bnjbvr bnjbvr deleted the event-graph branch January 30, 2024 22:27
@bnjbvr
Copy link
Member Author

bnjbvr commented Jan 30, 2024

Any plan for how to do it better?

Yeah, if the timeline becomes a pure view over an existing set of events, it could be resurrected as such and be much more performant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants