From ebf3522e1cec525cf6ce1879ef2cc26ef52b8fd1 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 3 Jul 2024 22:52:35 +0200 Subject: [PATCH] fix(ui): Emit a `RoomInfoNotableUpdateReasons::RECENCY_TIMESTAMP` ony for non-new rooms. This patch avoids to emit a `RoomInfoNotableUpdateReasons::RECENCY_TIMESTAMP` for rooms that are new. Otherwise the entries in `matrix_sdk_ui::room_list_service::RoomList` receive a `VectorDiff` because a new room is inserted, and then a `VectorDiff` because the recency timestamp is updated. The second `VectorDiff` is useless in this case. --- crates/matrix-sdk-base/src/sliding_sync.rs | 22 ++++--- crates/matrix-sdk-base/src/store/mod.rs | 5 ++ .../tests/integration/room_list_service.rs | 63 ++++++++++++------- .../src/tests/sliding_sync/room.rs | 39 ++++-------- 4 files changed, 72 insertions(+), 57 deletions(-) diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 015f37cdebc..c7dbe277326 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -12,9 +12,9 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::BTreeMap; #[cfg(feature = "e2e-encryption")] use std::ops::Deref; +use std::{collections::BTreeMap, ops::Not}; #[cfg(feature = "e2e-encryption")] use matrix_sdk_common::deserialized_responses::SyncTimelineEvent; @@ -341,6 +341,8 @@ impl BaseClient { }; // Find or create the room in the store + let is_a_new_room = store.room_exists(room_id).not(); + #[allow(unused_mut)] // Required for some feature flag combinations let (mut room, mut room_info, invited_room) = self.process_sliding_sync_room_membership(room_data, &state_events, store, room_id); @@ -374,7 +376,7 @@ impl BaseClient { .await?; } - process_room_properties(room_id, room_data, &mut room_info, changes); + process_room_properties(room_id, room_data, &mut room_info, is_a_new_room, changes); let timeline = self .handle_timeline( @@ -693,6 +695,7 @@ fn process_room_properties( room_id: &RoomId, room_data: &v4::SlidingSyncRoom, room_info: &mut RoomInfo, + is_a_new_room: bool, changes: &mut StateChanges, ) { // Handle the room's avatar. @@ -741,11 +744,16 @@ fn process_room_properties( if let Some(recency_timestamp) = &room_data.timestamp { room_info.update_recency_timestamp(*recency_timestamp); - changes - .room_info_notable_updates - .entry(room_id.to_owned()) - .or_default() - .insert(RoomInfoNotableUpdateReasons::RECENCY_TIMESTAMP); + // If it's not a new room, let's emit a `RECENCY_TIMESTAMP` update. + // For a new room, the room will appear as new, so we don't care about this + // update. + if is_a_new_room.not() { + changes + .room_info_notable_updates + .entry(room_id.to_owned()) + .or_default() + .insert(RoomInfoNotableUpdateReasons::RECENCY_TIMESTAMP); + } } } diff --git a/crates/matrix-sdk-base/src/store/mod.rs b/crates/matrix-sdk-base/src/store/mod.rs index b52b79c5502..6fa9ed83e84 100644 --- a/crates/matrix-sdk-base/src/store/mod.rs +++ b/crates/matrix-sdk-base/src/store/mod.rs @@ -240,6 +240,11 @@ impl Store { self.rooms.read().unwrap().get(room_id).cloned() } + /// Check if a room exists. + pub(crate) fn room_exists(&self, room_id: &RoomId) -> bool { + self.rooms.read().unwrap().get(room_id).is_some() + } + /// Lookup the `Room` for the given `RoomId`, or create one, if it didn't /// exist yet in the store pub fn get_or_create_room( diff --git a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs index 000c55d1b83..15638a26e05 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs @@ -1413,24 +1413,13 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> { }; // Assert the dynamic entries. - // It's pushed on the front because rooms are sorted by recency (based on their - // `origin_server_ts` of their latest event candidate). + // It's pushed on the front because rooms are sorted by recency. assert_entries_batch! { [dynamic_entries_stream] push front [ "!r1:bar.org" ]; push front [ "!r4:bar.org" ]; end; }; - assert_entries_batch! { - [dynamic_entries_stream] - set [ 1 ] [ "!r1:bar.org" ]; - end; - }; - assert_entries_batch! { - [dynamic_entries_stream] - set [ 0 ] [ "!r4:bar.org" ]; - end; - }; assert_pending!(dynamic_entries_stream); sync_then_assert_request_and_fake_response! { @@ -1510,16 +1499,6 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> { push front [ "!r7:bar.org" ]; end; }; - assert_entries_batch! { - [dynamic_entries_stream] - set [ 1 ] [ "!r5:bar.org" ]; - end; - }; - assert_entries_batch! { - [dynamic_entries_stream] - set [ 0 ] [ "!r7:bar.org" ]; - end; - }; assert_pending!(dynamic_entries_stream); // Now, let's change the dynamic entries! @@ -1913,12 +1892,50 @@ async fn test_room_sorting() -> Result<(), Error> { // | 4 | !r1 | 6 | Aaa | // | 5 | !r4 | 5 | | + assert_pending!(stream); + + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Running => Running, + assert request >= { + "lists": { + ALL_ROOMS: { + "ranges": [[0, 5]], + }, + }, + }, + respond with = { + "pos": "3", + "lists": { + ALL_ROOMS: { + "count": 6, + }, + }, + "rooms": { + "!r3:bar.org": { + "timestamp": 11, + }, + }, + }, + }; + assert_entries_batch! { [stream] - set [ 2 ] [ "!r6:bar.org" ]; + set [ 0 ] [ "!r3:bar.org" ]; end; }; + // Now we have: + // + // | index | room ID | recency | name | + // |-------|---------|---------|------| + // | 0 | !r3 | 11 | | + // | 1 | !r2 | 9 | | + // | 2 | !r6 | 8 | | + // | 3 | !r0 | 7 | Bbb | + // | 4 | !r1 | 6 | Aaa | + // | 5 | !r4 | 5 | | + assert_pending!(stream); Ok(()) diff --git a/testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs b/testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs index dc5922bf846..691eb24f2b8 100644 --- a/testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs +++ b/testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs @@ -801,8 +801,8 @@ async fn test_delayed_decryption_latest_event() -> Result<()> { assert_eq!(bob_room.state(), RoomState::Joined); let alice_all_rooms = alice_sync_service.room_list_service().all_rooms().await.unwrap(); - let (stream, entries) = - alice_all_rooms.entries_with_dynamic_adapters(10, alice.room_info_notable_update_receiver()); + let (stream, entries) = alice_all_rooms + .entries_with_dynamic_adapters(10, alice.room_info_notable_update_receiver()); entries.set_filter(Box::new(new_filter_all(vec![]))); pin_mut!(stream); @@ -848,18 +848,6 @@ async fn test_delayed_decryption_latest_event() -> Result<()> { } ); - // Sometimes Synapse sends the same message twice. Let's consume useless `Set`… - // if they arrived before 3s. - if let Ok(Some(diffs)) = timeout(Duration::from_secs(3), stream.next()).await { - assert_eq!(diffs.len(), 1); - assert_matches!( - &diffs[0], - VectorDiff::Set { index: 0, value: room } => { - assert_eq!(room.room_id(), alice_room.room_id()); - } - ); - } - assert_pending!(stream); Ok(()) @@ -927,8 +915,8 @@ async fn test_room_info_notable_update_deduplication() -> Result<()> { alice_room.enable_encryption().await.unwrap(); let alice_all_rooms = alice_sync_service.room_list_service().all_rooms().await.unwrap(); - let (stream, entries) = - alice_all_rooms.entries_with_dynamic_adapters(10, alice.room_info_notable_update_receiver()); + let (stream, entries) = alice_all_rooms + .entries_with_dynamic_adapters(10, alice.room_info_notable_update_receiver()); entries.set_filter(Box::new(new_filter_all(vec![]))); pin_mut!(stream); @@ -999,17 +987,14 @@ async fn test_room_info_notable_update_deduplication() -> Result<()> { } ); - // Sometimes Synapse sends the same message twice. Let's consume useless `Set`… - // if they arrived before 3s. - if let Ok(Some(diffs)) = timeout(Duration::from_secs(3), stream.next()).await { - assert_eq!(diffs.len(), 1); - assert_matches!( - &diffs[0], - VectorDiff::Set { index: 0, value: room } => { - assert_eq!(room.room_id(), alice_room.room_id()); - } - ); - } + assert_let!(Some(diffs) = stream.next().await); + assert_eq!(diffs.len(), 1); + assert_matches!( + &diffs[0], + VectorDiff::Set { index: 0, value: room } => { + assert_eq!(room.room_id(), alice_room.room_id()); + } + ); assert_pending!(stream);