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 cache: introduce the Paginator API #3309

Merged
merged 8 commits into from
Apr 12, 2024
Merged

Conversation

bnjbvr
Copy link
Member

@bnjbvr bnjbvr commented Apr 8, 2024

This introduces a new helper object to run arbitrary pagination requests, backwards- or forward-. At the moment they're disconnected from the event cache, although I've put the files there for future convenience, since at some point we'll want to merge the retrieved events with the cache (? maybe).

This little state machine makes it possible to retrieve the initial data, given an initial event id, using the /context endpoint; then allow stateful pagination using a paginator kind of API. Paginating in the timeline indicates whether we've reached the start/end of the timeline.

The test for the state subscription is quite extensive and makes sure the basic functionality works as intended.

Some testing helpers have been (re)introduced in the SDK crate, simplifying the code, and introducing a better EventFactory / EventBuilder pattern than the existing one in the matrix-sdk-test crate. In particular, this can make use of some types in matrix-sdk, notably SyncTimelineEvent and TimelineEvent, and I've found the API to be simpler to use as well.

Commit history is messy, so we might want to squash when merging.

Part of #3234.

@bnjbvr bnjbvr marked this pull request as ready for review April 9, 2024 15:44
@bnjbvr bnjbvr requested a review from a team as a code owner April 9, 2024 15:44
@bnjbvr bnjbvr requested review from andybalaam and removed request for a team April 9, 2024 15:44
Copy link

codecov bot commented Apr 9, 2024

Codecov Report

Attention: Patch coverage is 72.25434% with 48 lines in your changes are missing coverage. Please review.

Project coverage is 83.58%. Comparing base (fcfdaad) to head (f4b63cc).
Report is 15 commits behind head on main.

Files Patch % Lines
crates/matrix-sdk/src/test_utils/events.rs 48.14% 28 Missing ⚠️
crates/matrix-sdk/src/event_cache/paginator.rs 81.98% 20 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3309      +/-   ##
==========================================
- Coverage   83.64%   83.58%   -0.06%     
==========================================
  Files         238      240       +2     
  Lines       24662    24835     +173     
==========================================
+ Hits        20629    20759     +130     
- Misses       4033     4076      +43     

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

@bnjbvr bnjbvr force-pushed the bnjbvr/event_with_context branch from 63df8fa to 001e86a Compare April 9, 2024 16:00
@bnjbvr bnjbvr force-pushed the bnjbvr/event_with_context branch from f7e053e to d90b37a Compare April 9, 2024 16:14
Copy link
Member

@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.

This looks great! Lots of questions and suggestions, and an ask for more tests. Thanks!

@bnjbvr bnjbvr requested a review from andybalaam April 12, 2024 14:45
@bnjbvr
Copy link
Member Author

bnjbvr commented Apr 12, 2024

I've now added isolated tests for getting events from start_from / paginate_backwards / paginate_forwards, and I think it covers all the things you've asked — PTAL!

Copy link
Member

@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.

Brilliant, thank you!

async fn paginate(
&self,
dir: Direction,
token_lock: &Mutex<Option<String>>,
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 great!

@bnjbvr bnjbvr merged commit fc4cd53 into main Apr 12, 2024
35 checks passed
@bnjbvr bnjbvr deleted the bnjbvr/event_with_context branch April 12, 2024 15:57
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.

2 participants