fix(sdk): Change how SlidingSyncList::update_room_list handles updates for out of list rooms.

So. `SlidingSyncList::update_room_list()` does the following:

1. It adjust room list entries,
2. It updates the `maximum_number_of_rooms`,
3. It applies the sync operations on the `room_list` entries,
4. It updates the `room_list` entries for rooms outside the sync
   operations.

SlidingSync answers with a `lists` and `rooms`. The `room_list` entries
is updated as follows: either a sync operation from `lists` has been
applied and the entries are updated, or `rooms` contains rooms that
are not updated by `lists` but that receive new events, and thus must
trigger an update in the `room_list` entries.

This patch changes a little bit the last part of this. Initially,
we were updating rooms that are part of `rooms` but absent of
`lists` sync operations, only **for the current list's ranges**.
But that's wrong! First, we have noticed a bug here: the
correct code wasn't `skip(start).take(end.saturating_add(1))` but
`skip(start).take(end.saturating_add(1) - start)`. Note a big deal, we
were iterating on more entries like it was necessary, but everything
was filtered later, so no bug, just useless computations. Second,
this `skip` and `take` is actually… useless. A room to which we have
subscribed can be out of any range, but we still want `room_list`
entries to receive an update for that particular room.

This patch fixes that once and for all.

In practise, the bug wasn't happening because if someone subscribes
to a room, its timeline is likely to be fetched, and updates will be
received, but still, this is better now from a SlidingSync strict point
of view.
This commit is contained in:
Ivan Enderlin
2023-06-29 11:21:31 +02:00
parent 5d9cc6be15
commit 9bae039629
2 changed files with 19 additions and 30 deletions

View File

@@ -455,41 +455,28 @@ impl SlidingSyncListInner {
// user to know if a room has received an update: be either a
// position modification or an update in general (like a new room
// message). Let's trigger those.
let request_generator = self.request_generator.read().unwrap();
// Note: this is fine to call `request_generator` here because we've handled the
// response in the generator first.
let ranges = request_generator.requested_ranges();
let mut rooms_to_update = Vec::with_capacity(rooms_that_have_received_an_update.len());
for (start, end) in ranges
.iter()
.map(|r| (usize::try_from(*r.start()).unwrap(), usize::try_from(*r.end()).unwrap()))
{
let mut rooms_to_update =
Vec::with_capacity(rooms_that_have_received_an_update.len());
for (position, room_list_entry) in
room_list.iter().enumerate().skip(start).take(end.saturating_add(1))
{
// Invalidated rooms must be considered as empty rooms, so let's just filter by
// filled rooms.
if let RoomListEntry::Filled(room_id) = room_list_entry {
// If room has received an update but that has not been handled by a
// sync operation.
if rooms_that_have_received_an_update.contains(room_id) {
rooms_to_update.push((position, room_list_entry.clone()));
}
for (position, room_list_entry) in room_list.iter().enumerate() {
// Invalidated rooms must be considered as empty rooms, so let's just filter by
// filled rooms.
if let RoomListEntry::Filled(room_id) = room_list_entry {
// If room has received an update but that has not been handled by a
// sync operation.
if rooms_that_have_received_an_update.contains(room_id) {
rooms_to_update.push((position, room_list_entry.clone()));
}
}
}
if !rooms_to_update.is_empty() {
for (position, room_list_entry) in rooms_to_update {
// Setting to `room_list`'s item to the same value, just
// to generate an “diff update”.
room_list.set(position, room_list_entry);
}
new_changes = true;
if !rooms_to_update.is_empty() {
for (position, room_list_entry) in rooms_to_update {
// Setting to `room_list`'s item to the same value, just
// to generate an “diff update”.
room_list.set(position, room_list_entry);
}
new_changes = true;
}
}

View File

@@ -78,6 +78,7 @@ pub(in super::super) struct SlidingSyncListRequestGenerator {
///
/// Note there's only one range in the `Growing` and `Paging` mode.
ranges: Ranges,
/// The kind of request generator.
pub(super) kind: SlidingSyncListRequestGeneratorKind,
}
@@ -119,6 +120,7 @@ impl SlidingSyncListRequestGenerator {
/// For generators in the selective mode, this is the initial set of ranges.
/// For growing and paginated generators, this is the range committed in the
/// latest response received from the server.
#[cfg(test)]
pub(super) fn requested_ranges(&self) -> &[Range] {
&self.ranges
}