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.

Sign up for GitHub

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

Jump to bottom

event cache/timeline: reuse the Paginator when running back-paginations #3373

Merged
merged 8 commits into from
May 16, 2024
Merged

event cache/timeline: reuse the Paginator when running back-paginations #3373

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
48bc7fe
event cache: reuse the paginator internally
bnjbvr Apr 30, 2024
8862d00
event cache: move the `pagination_token_notifier` into the `RoomPagin…
bnjbvr May 1, 2024
3ca4c98
event cache: introduce a `RoomPagination` API object and move code ar…
bnjbvr May 1, 2024
2300253
event cache: remove "paginate" (et al.) in `RoomPagination` method names
bnjbvr May 1, 2024
bbd20b0
event_cache/timeline: have the event cache handle restarting a back-p…
bnjbvr May 1, 2024
b2070b0
Merge branch 'main' into bnjbvr/reuse-paginator-in-event-cache
bnjbvr May 13, 2024
cf01cb1
Address review comments
bnjbvr May 13, 2024
3002389
Merge branch 'main' into bnjbvr/reuse-paginator-in-event-cache
bnjbvr May 13, 2024
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
6 changes: 4 additions & 2 deletions crates/matrix-sdk-ui/src/timeline/pagination.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,11 @@ impl super::Timeline {
let initial_options = options.clone();
let mut outcome = PaginationOutcome::default();

let pagination = self.event_cache.pagination();

while let Some(batch_size) = options.next_event_limit(outcome) {
loop {
let result = self.event_cache.paginate_backwards(batch_size).await;
let result = pagination.paginate_backwards(batch_size).await;

let event_cache_outcome = match result {
Ok(outcome) => outcome,
Expand Down Expand Up @@ -145,7 +147,7 @@ impl super::Timeline {
/// Note: this may send multiple Paginating/Idle sequences during a single
Copy link
Member

Choose a reason for hiding this comment

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

Why? Do we want to dedup them? Maybe with async_rx::StreamExt::dedup if we can provide a Stream.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an implementation detail of eyeball that we're running into. If you have an emitter E and an observer O. Say E emits very quickly e1, e2, e3; then if O doesn't react fast enough, it may only see e1, handle it, and then miss e2, and only see e3.

Before this PR, the exposed pagination status was controlled at the timeline level, and the timeline would make sure to reset it only when a full pagination loop is over, after we obtained e.g. N events.

After this PR, the pagination status is that of the underlying paginator. The timeline may continue to paginate after the paginator has run one pagination query. In that case, the underlying status of the paginator will be:

  • running (because it's busy doing the network request)
  • idle (because it's done processing the response)
  • (the timeline sees it hasn't received as many events as it wanted, so it retriggers the paginator) the paginator quickly re-runs into the running state

So the quick running -> idle -> running transition might make it so that an observer could see running -> running, because they didn't react fast enough to see the idle state in the middle.

I've added a dedup() call to address this, thanks.

Now, if the caller is actually fast enough, they may see a few quick transitions from running -> idle -> running; I wonder if that may cause some UI flickering (e.g. the spinner quickly disappears and reappears). At this point, both EX apps doesn't use the back-pagination status anymore, so it's a theoretical issue.

/// call to [`Self::paginate_backwards()`].
pub fn back_pagination_status(&self) -> Subscriber<PaginatorState> {
self.event_cache.pagination_status()
self.event_cache.pagination().pagination_status()
}
}

Expand Down
Loading