From 76caf7ed05f70e31c6c9923020e07503eb103f89 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 3 Jul 2024 15:25:44 +0200 Subject: [PATCH 1/2] feat(ui): Trigger room list update only when necessary. This patch revisits the need to trigger a room list update for all changes of `RoomInfo`. For the moment, it reduces the scope to `recency_timestamp` update. This patch comes with a test to ensure things work as expected. --- crates/matrix-sdk-base/src/sliding_sync.rs | 20 +- .../src/room_list_service/room.rs | 10 +- .../tests/integration/room_list_service.rs | 271 ++++++++++++++++++ .../src/tests/sliding_sync/room.rs | 9 +- 4 files changed, 292 insertions(+), 18 deletions(-) diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 3bca1564b4b..497fae21197 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -153,6 +153,8 @@ impl BaseClient { let mut notifications = Default::default(); let mut rooms_account_data = account_data.rooms.clone(); + let mut trigger_room_list_update = false; + for (room_id, response_room_data) in rooms { let (room_info, joined_room, left_room, invited_room) = self .process_sliding_sync_room( @@ -163,6 +165,7 @@ impl BaseClient { &mut changes, &mut notifications, &mut ambiguity_cache, + &mut trigger_room_list_update, ) .await?; @@ -292,12 +295,7 @@ impl BaseClient { trace!("ready to submit changes to store"); store.save_changes(&changes).await?; - self.apply_changes( - &changes, - // The changes may result in a latest event update, which should trigger a room list - // re-ordering. This the room list should be notified by these changes. - true, - ); + self.apply_changes(&changes, trigger_room_list_update); trace!("applied changes"); // Now that all the rooms information have been saved, update the display name @@ -327,6 +325,7 @@ impl BaseClient { changes: &mut StateChanges, notifications: &mut BTreeMap>, ambiguity_cache: &mut AmbiguityCache, + trigger_room_list_update: &mut bool, ) -> Result<(RoomInfo, Option, Option, Option)> { let (raw_state_events, state_events): (Vec<_>, Vec<_>) = { @@ -375,7 +374,7 @@ impl BaseClient { .await?; } - process_room_properties(room_data, &mut room_info); + process_room_properties(room_data, &mut room_info, trigger_room_list_update); let timeline = self .handle_timeline( @@ -690,7 +689,11 @@ async fn cache_latest_events( room.latest_encrypted_events.write().unwrap().extend(encrypted_events.into_iter().rev()); } -fn process_room_properties(room_data: &v4::SlidingSyncRoom, room_info: &mut RoomInfo) { +fn process_room_properties( + room_data: &v4::SlidingSyncRoom, + room_info: &mut RoomInfo, + trigger_room_list_update: &mut bool, +) { // Handle the room's avatar. // // It can be updated via the state events, or via the @@ -736,6 +739,7 @@ fn process_room_properties(room_data: &v4::SlidingSyncRoom, room_info: &mut Room if let Some(recency_timestamp) = &room_data.timestamp { room_info.update_recency_timestamp(*recency_timestamp); + *trigger_room_list_update = true; } } diff --git a/crates/matrix-sdk-ui/src/room_list_service/room.rs b/crates/matrix-sdk-ui/src/room_list_service/room.rs index 58b6da3a5cf..99651d52b60 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/room.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/room.rs @@ -14,6 +14,7 @@ //! The `Room` type. +use core::fmt; use std::{ops::Deref, sync::Arc}; use async_once_cell::OnceCell as AsyncOnceCell; @@ -29,12 +30,17 @@ use crate::{ /// A room in the room list. /// /// It's cheap to clone this type. -#[derive(Clone, Debug)] +#[derive(Clone)] pub struct Room { inner: Arc, } -#[derive(Debug)] +impl fmt::Debug for Room { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.debug_tuple("Room").field(&self.id().to_owned()).finish() + } +} + struct RoomInner { /// The Sliding Sync where everything comes from. sliding_sync: Arc, 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 47fddfad322..276a77d9ae6 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs @@ -1653,6 +1653,277 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> { Ok(()) } +#[async_test] +async fn test_room_sorting() -> Result<(), Error> { + let (client, server, room_list) = new_room_list_service().await?; + + let sync = room_list.sync(); + pin_mut!(sync); + + let all_rooms = room_list.all_rooms().await?; + + let (stream, dynamic_entries) = + all_rooms.entries_with_dynamic_adapters(10, client.roominfo_update_receiver()); + pin_mut!(stream); + + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Init => SettingUp, + assert request >= { + "lists": { + ALL_ROOMS: { + "ranges": [[0, 19]], + }, + }, + }, + respond with = { + "pos": "0", + "lists": { + ALL_ROOMS: { + "count": 5, + }, + }, + "rooms": { + "!r0:bar.org": { + "initial": true, + "timestamp": 3, + "required_state": [ + { + "content": { + "name": "Bbb" + }, + "sender": "@example:bar.org", + "state_key": "", + "type": "m.room.name", + "event_id": "$s0", + "origin_server_ts": 3, + }, + ], + }, + "!r1:bar.org": { + "initial": true, + "timestamp": 3, + "required_state": [ + { + "content": { + "name": "Aaa" + }, + "sender": "@example:bar.org", + "state_key": "", + "type": "m.room.name", + "event_id": "$s1", + "origin_server_ts": 3, + }, + ], + }, + "!r2:bar.org": { + "initial": true, + "timestamp": 1, + }, + "!r3:bar.org": { + "initial": true, + "timestamp": 4, + }, + "!r4:bar.org": { + "initial": true, + "timestamp": 5, + }, + }, + }, + }; + + // Ensure the dynamic entries' stream is pending because there is no filter set + // yet. + assert_pending!(stream); + + // Now, let's define a filter. + dynamic_entries.set_filter(Box::new(new_filter_non_left())); + + // Assert rooms are sorted by recency and by name!. + assert_entries_batch! { + [stream] + reset [ + "!r4:bar.org", // recency of 5 + "!r3:bar.org", // recency of 4 + "!r1:bar.org", // recency of 3, but name comes before `!r0` + "!r0:bar.org", // recency of 3, but name comes after `!r1` + "!r2:bar.org", // recency of 1 + ]; + end; + }; + + // Now we have: + // + // | index | room ID | recency | name | + // |-------|---------|---------|------| + // | 0 | !r4 | 5 | | + // | 1 | !r3 | 4 | | + // | 2 | !r1 | 3 | Aaa | + // | 3 | !r0 | 3 | Bbb | + // | 4 | !r2 | 1 | | + + assert_pending!(stream); + + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = SettingUp => Running, + assert request >= { + "lists": { + ALL_ROOMS: { + "ranges": [[0, 4]], + }, + }, + }, + respond with = { + "pos": "1", + "lists": { + ALL_ROOMS: { + "count": 5, + }, + }, + "rooms": { + "!r0:bar.org": { + "timestamp": 7, + }, + "!r1:bar.org": { + "timestamp": 6, + }, + "!r2:bar.org": { + "timestamp": 9, + }, + }, + }, + }; + + // Assert rooms are moving. + assert_entries_batch! { + [stream] + remove [ 3 ]; + insert [ 0 ] [ "!r0:bar.org" ]; + end; + }; + + // Now we have: + // + // | index | room ID | recency | name | + // |-------|---------|---------|------| + // | 0 | !r0 | 7 | Bbb | + // | 1 | !r4 | 5 | | + // | 2 | !r3 | 4 | | + // | 3 | !r1 | 3 | Aaa | + // | 4 | !r2 | 1 | | + + assert_entries_batch! { + [stream] + remove [ 3 ]; + insert [ 1 ] [ "!r1:bar.org" ]; + end; + }; + + // Now we have: + // + // | index | room ID | recency | name | + // |-------|---------|---------|------| + // | 0 | !r0 | 7 | Bbb | + // | 1 | !r1 | 6 | Aaa | + // | 2 | !r4 | 5 | | + // | 3 | !r3 | 4 | | + // | 4 | !r2 | 1 | | + + assert_entries_batch! { + [stream] + remove [ 4 ]; + insert [ 0 ] [ "!r2:bar.org" ]; + end; + }; + + // Now we have: + // + // | index | room ID | recency | name | + // |-------|---------|---------|------| + // | 0 | !r2 | 9 | | + // | 1 | !r0 | 7 | Bbb | + // | 2 | !r1 | 6 | Aaa | + // | 3 | !r4 | 5 | | + // | 4 | !r3 | 4 | | + + assert_pending!(stream); + + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Running => Running, + assert request >= { + "lists": { + ALL_ROOMS: { + "ranges": [[0, 4]], + }, + }, + }, + respond with = { + "pos": "2", + "lists": { + ALL_ROOMS: { + "count": 6, + }, + }, + "rooms": { + "!r6:bar.org": { + "initial": true, + "timestamp": 8, + }, + "!r3:bar.org": { + "timestamp": 10, + }, + }, + }, + }; + + assert_entries_batch! { + [stream] + insert [ 1 ] [ "!r6:bar.org" ]; + end; + }; + + // Now we have: + // + // | index | room ID | recency | name | + // |-------|---------|---------|------| + // | 0 | !r2 | 9 | | + // | 1 | !r6 | 8 | | + // | 2 | !r0 | 7 | Bbb | + // | 3 | !r1 | 6 | Aaa | + // | 4 | !r4 | 5 | | + // | 5 | !r3 | 4 | | + + assert_entries_batch! { + [stream] + remove [ 5 ]; + insert [ 0 ] [ "!r3:bar.org" ]; + end; + }; + + // Now we have: + // + // | index | room ID | recency | name | + // |-------|---------|---------|------| + // | 0 | !r3 | 10 | | + // | 1 | !r2 | 9 | | + // | 2 | !r6 | 8 | | + // | 3 | !r0 | 7 | Bbb | + // | 4 | !r1 | 6 | Aaa | + // | 5 | !r4 | 5 | | + + assert_entries_batch! { + [stream] + set [ 2 ] [ "!r6:bar.org" ]; + end; + }; + + assert_pending!(stream); + + Ok(()) +} + #[async_test] async fn test_room() -> Result<(), Error> { let (_, server, room_list) = new_room_list_service().await?; 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 63890a5b771..faf5c36b22f 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 @@ -954,14 +954,6 @@ async fn test_roominfo_update_deduplication() -> Result<()> { 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); // Send a message, it should arrive @@ -982,6 +974,7 @@ async fn test_roominfo_update_deduplication() -> Result<()> { } ); } + assert_pending!(stream); Ok(()) From 77feed244770c5a074c19872720968a28f4ac29f Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 3 Jul 2024 16:43:44 +0200 Subject: [PATCH 2/2] test: Fix flakyness. --- .../src/tests/sliding_sync/room.rs | 64 +++++++++++++++---- 1 file changed, 50 insertions(+), 14 deletions(-) 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 faf5c36b22f..8b601928502 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 @@ -7,7 +7,7 @@ use anyhow::Result; use assert_matches::assert_matches; use assert_matches2::assert_let; use eyeball_im::VectorDiff; -use futures_util::{pin_mut, FutureExt, StreamExt as _}; +use futures_util::{pin_mut, StreamExt as _}; use matrix_sdk::{ bytes::Bytes, config::SyncSettings, @@ -847,6 +847,19 @@ async fn test_delayed_decryption_latest_event() -> Result<()> { assert_eq!(room.room_id(), alice_room.room_id()); } ); + + // 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(()) @@ -920,10 +933,6 @@ async fn test_roominfo_update_deduplication() -> Result<()> { pin_mut!(stream); - // Wait shortly so the manual roominfo update is triggered before we load the - // stream. - sleep(Duration::from_secs(1)).await; - // Stream only has the initial Reset entry. assert_let!(Some(diffs) = stream.next().await); assert_eq!(diffs.len(), 1); @@ -936,15 +945,17 @@ async fn test_roominfo_update_deduplication() -> Result<()> { ); assert_pending!(stream); - sleep(Duration::from_secs(1)).await; let alice_room = alice.get_room(alice_room.room_id()).unwrap(); + assert_eq!(alice_room.state(), RoomState::Joined); + assert!(alice_room.is_encrypted().await.unwrap()); + let bob_room = bob.get_room(alice_room.room_id()).unwrap(); bob_room.join().await.unwrap(); - sleep(Duration::from_secs(1)).await; - assert_eq!(alice_room.state(), RoomState::Joined); - assert!(alice_room.is_encrypted().await.unwrap()); + // Wait Bob to be in the room. + sleep(Duration::from_secs(2)).await; assert_eq!(bob_room.state(), RoomState::Joined); + // Room update for join assert_let!(Some(diffs) = stream.next().await); assert_eq!(diffs.len(), 1); @@ -954,21 +965,46 @@ async fn test_roominfo_update_deduplication() -> Result<()> { assert_eq!(room.room_id(), alice_room.room_id()); } ); + + // 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); // Send a message, it should arrive let event = bob_room.send(RoomMessageEventContent::text_plain("hello world")).await?; - sleep(Duration::from_secs(1)).await; + // Wait the message from Bob to be sent. + sleep(Duration::from_secs(2)).await; // Latest event is set now assert_eq!(alice_room.latest_event().unwrap().event_id(), Some(event.event_id)); - // Stream has the room again, but no second event - while let Some(Some(updated_rooms)) = stream.next().now_or_never() { - assert!(!updated_rooms.is_empty()); + // Room has been updated. + 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()); + } + ); + + // 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!( - &updated_rooms[0], + &diffs[0], VectorDiff::Set { index: 0, value: room } => { assert_eq!(room.room_id(), alice_room.room_id()); }