From 9bae0396294042573cec90cd72a1c4e3a9cae77c Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 29 Jun 2023 11:21:31 +0200 Subject: [PATCH] fix(sdk): Change how `SlidingSyncList::update_room_list` handles updates for out of list rooms. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../matrix-sdk/src/sliding_sync/list/mod.rs | 47 +++++++------------ .../sliding_sync/list/request_generator.rs | 2 + 2 files changed, 19 insertions(+), 30 deletions(-) diff --git a/crates/matrix-sdk/src/sliding_sync/list/mod.rs b/crates/matrix-sdk/src/sliding_sync/list/mod.rs index 62f65b03f..128cd9c84 100644 --- a/crates/matrix-sdk/src/sliding_sync/list/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/list/mod.rs @@ -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; } } diff --git a/crates/matrix-sdk/src/sliding_sync/list/request_generator.rs b/crates/matrix-sdk/src/sliding_sync/list/request_generator.rs index 1108b2627..80e6aa0f1 100644 --- a/crates/matrix-sdk/src/sliding_sync/list/request_generator.rs +++ b/crates/matrix-sdk/src/sliding_sync/list/request_generator.rs @@ -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 }