From 751ddd68eb30b67a8f07d78dc294471313dc35e9 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Mon, 3 Jun 2024 12:50:56 +0200 Subject: [PATCH 01/14] base: directly call `calculate_room_name` since it's a free-function Instead of calling it through `BaseRoomInfo::calculate_room_name`. --- crates/matrix-sdk-base/src/rooms/mod.rs | 11 +---------- crates/matrix-sdk-base/src/rooms/normal.rs | 3 ++- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/crates/matrix-sdk-base/src/rooms/mod.rs b/crates/matrix-sdk-base/src/rooms/mod.rs index 48ef48c12b5..22db7a40c39 100644 --- a/crates/matrix-sdk-base/src/rooms/mod.rs +++ b/crates/matrix-sdk-base/src/rooms/mod.rs @@ -122,15 +122,6 @@ impl BaseRoomInfo { Self::default() } - pub(crate) fn calculate_room_name( - &self, - joined_member_count: u64, - invited_member_count: u64, - heroes: Vec<&str>, - ) -> DisplayName { - calculate_room_name(joined_member_count, invited_member_count, heroes) - } - /// Get the room version of this room. /// /// For room versions earlier than room version 11, if the event is @@ -359,7 +350,7 @@ impl Default for BaseRoomInfo { } } -/// Calculate room name according to step 3 of the [naming algorithm.] +/// Calculate room name according to step 3 of the [naming algorithm]. /// /// [naming algorithm]: https://spec.matrix.org/latest/client-server-api/#calculating-the-display-name-for-a-room fn calculate_room_name( diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index 28c6ccff035..0d81f262f30 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -64,6 +64,7 @@ use crate::latest_event::LatestEvent; use crate::{ deserialized_responses::MemberEvent, read_receipts::RoomReadReceipts, + rooms::calculate_room_name, store::{DynStateStore, Result as StoreResult, StateStoreExt}, sync::UnreadNotificationsCount, MinimalStateEvent, OriginalMinimalStateEvent, RoomMemberships, @@ -695,7 +696,7 @@ impl Room { "Calculating name for a room", ); - Ok(self.inner.read().base_info.calculate_room_name( + Ok(calculate_room_name( num_joined, num_invited, heroes.iter().map(|hero| hero.as_str()).collect(), From 8efcf4e971b0d091d0ff70218fc7efb99a7e560c Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Mon, 3 Jun 2024 12:56:14 +0200 Subject: [PATCH 02/14] base: fix guessed number of members as it should relate to joined users, not joined+invited The previous code would use `ACTIVE` which means "either joined or invited" members, but the code thereafter considered this to be the number of joined members. Also replaced a call to `self.members()` (which does a lot of work under the hood) with a call to `self.joined_user_ids()`, since we only retrieve the `.len()` of the resulting vector. --- crates/matrix-sdk-base/src/rooms/normal.rs | 23 +++++++++++----------- crates/matrix-sdk-base/src/sliding_sync.rs | 17 ++++++++-------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index 0d81f262f30..1c73790f68a 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -628,7 +628,7 @@ impl Room { let own_user_id = self.own_user_id().as_str(); - let (heroes, guessed_num_members): (Vec, _) = if !summary.heroes_names.is_empty() { + let (heroes, num_joined_guess): (Vec, _) = if !summary.heroes_names.is_empty() { // Straightforward path: pass through the heroes names, don't give a guess of // the number of members. (summary.heroes_names, None) @@ -648,22 +648,22 @@ impl Room { (names, None) } else { - let mut members = self.members(RoomMemberships::ACTIVE).await?; + let mut joined_members = self.members(RoomMemberships::JOIN).await?; // Make the ordering deterministic. - members.sort_unstable_by(|lhs, rhs| lhs.name().cmp(rhs.name())); + joined_members.sort_unstable_by(|lhs, rhs| lhs.name().cmp(rhs.name())); // We can make a good prediction of the total number of members here. This might // be incorrect if the database info is outdated. - let guessed_num_members = Some(members.len()); + let num_joined = Some(joined_members.len()); ( - members + joined_members .into_iter() .take(NUM_HEROES) .filter_map(|u| (u.user_id() != own_user_id).then(|| u.name().to_owned())) .collect(), - guessed_num_members, + num_joined, ) }; @@ -675,14 +675,13 @@ impl Room { } RoomState::Joined if summary.joined_member_count == 0 => { - let num_members = if let Some(guessed_num_members) = guessed_num_members { - guessed_num_members + let num_joined = if let Some(num_joined) = num_joined_guess { + num_joined } else { - self.members(RoomMemberships::ACTIVE).await?.len() + self.joined_user_ids().await?.len() }; - // joined but the summary is not completed yet - (num_members as u64, summary.invited_member_count) + (num_joined as u64, summary.invited_member_count) } _ => (summary.joined_member_count, summary.invited_member_count), @@ -693,7 +692,7 @@ impl Room { own_user = ?self.own_user_id, num_joined, num_invited, heroes = ?heroes, - "Calculating name for a room", + "Calculating name for a room based on heroes", ); Ok(calculate_room_name( diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 5df2068e48f..62ff321e190 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -817,19 +817,20 @@ mod tests { } #[async_test] - async fn test_room_name_is_found_when_processing_sliding_sync_response() { + async fn test_missing_room_name_event() { // Given a logged-in client let client = logged_in_base_client(None).await; let room_id = room_id!("!r:e.uk"); - // When I send sliding sync response containing a room with a name + // When I send sliding sync response containing a room with a name set in the + // sliding sync response, let mut room = v4::SlidingSyncRoom::new(); room.name = Some("little room".to_owned()); let response = response_with_room(room_id, room); let sync_resp = client.process_sliding_sync(&response, &()).await.expect("Failed to process sync"); - // Then the room doesn't have the name in the client. + // No m.room.name event, no heroes, no members => considered an empty room! let client_room = client.get_room(room_id).expect("No room found"); assert!(client_room.name().is_none()); assert_eq!(client_room.computed_display_name().await.unwrap().to_string(), "Empty Room"); @@ -842,14 +843,15 @@ mod tests { } #[async_test] - async fn test_invited_room_name_is_found_when_processing_sliding_sync_response() { + async fn test_missing_invited_room_name_event() { // Given a logged-in client, let client = logged_in_base_client(None).await; let room_id = room_id!("!r:e.uk"); let user_id = user_id!("@w:e.uk"); let inviter = user_id!("@john:mastodon.org"); - // When I send sliding sync response containing a room with a name, + // When I send sliding sync response containing a room with a name set in the + // sliding sync response, let mut room = v4::SlidingSyncRoom::new(); set_room_invited(&mut room, inviter, user_id); room.name = Some("name from sliding sync response".to_owned()); @@ -861,9 +863,8 @@ mod tests { let client_room = client.get_room(room_id).expect("No room found"); assert!(client_room.name().is_none()); - // The computed display name will show heroes name, but for an invited room… - // it's only self, the invitee, who will appear. - assert_eq!(client_room.computed_display_name().await.unwrap().to_string(), "w"); + // No m.room.name event, no heroes, no members => considered an empty room! + assert_eq!(client_room.computed_display_name().await.unwrap().to_string(), "Empty Room"); assert_eq!(client_room.state(), RoomState::Invited); From cddc2b157dedbe222d14d30803660c700dd2644c Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 5 Jun 2024 16:56:14 +0200 Subject: [PATCH 03/14] base: get rid of intermediate `calculate_name()` function --- crates/matrix-sdk-base/src/rooms/normal.rs | 184 ++++++++++----------- 1 file changed, 90 insertions(+), 94 deletions(-) diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index 1c73790f68a..e00b76c00ce 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -481,7 +481,96 @@ impl Room { /// /// [spec]: pub async fn computed_display_name(&self) -> StoreResult { - self.calculate_name().await + let summary = { + let inner = self.inner.read(); + + if let Some(name) = inner.name() { + let name = name.trim(); + return Ok(DisplayName::Named(name.to_owned())); + } + + if let Some(alias) = inner.canonical_alias() { + let alias = alias.alias().trim(); + return Ok(DisplayName::Aliased(alias.to_owned())); + } + + inner.summary.clone() + }; + + let own_user_id = self.own_user_id().as_str(); + + let (heroes, num_joined_guess): (Vec, _) = if !summary.heroes_names.is_empty() { + // Straightforward path: pass through the heroes names, don't give a guess of + // the number of members. + (summary.heroes_names, None) + } else if !summary.heroes_user_ids.is_empty() { + // Use the heroes, if available. + let heroes = summary.heroes_user_ids; + + let mut names = Vec::with_capacity(heroes.len()); + for user_id in heroes { + if user_id == own_user_id { + continue; + } + if let Some(member) = self.get_member(&user_id).await? { + names.push(member.name().to_owned()); + } + } + + (names, None) + } else { + let mut joined_members = self.members(RoomMemberships::JOIN).await?; + + // Make the ordering deterministic. + joined_members.sort_unstable_by(|lhs, rhs| lhs.name().cmp(rhs.name())); + + // We can make a good prediction of the total number of members here. This might + // be incorrect if the database info is outdated. + let num_joined = Some(joined_members.len()); + + ( + joined_members + .into_iter() + .take(NUM_HEROES) + .filter_map(|u| (u.user_id() != own_user_id).then(|| u.name().to_owned())) + .collect(), + num_joined, + ) + }; + + let (num_joined, num_invited) = match self.state() { + RoomState::Invited => { + // when we were invited we don't have a proper summary, we have to do best + // guessing + (heroes.len() as u64, 1u64) + } + + RoomState::Joined if summary.joined_member_count == 0 => { + let num_joined = if let Some(num_joined) = num_joined_guess { + num_joined + } else { + self.joined_user_ids().await?.len() + }; + + (num_joined as u64, summary.invited_member_count) + } + + _ => (summary.joined_member_count, summary.invited_member_count), + }; + + debug!( + room_id = ?self.room_id(), + own_user = ?self.own_user_id, + num_joined, num_invited, + heroes = ?heroes, + "Calculating name for a room based on heroes", + ); + + Ok(calculate_room_name( + num_joined, + num_invited, + heroes.iter().map(|hero| hero.as_str()).collect(), + )) } /// Return the last event in this room, if one has been cached during @@ -609,99 +698,6 @@ impl Room { self.inner.read().joined_members_count() } - async fn calculate_name(&self) -> StoreResult { - let summary = { - let inner = self.inner.read(); - - if let Some(name) = inner.name() { - let name = name.trim(); - return Ok(DisplayName::Named(name.to_owned())); - } - - if let Some(alias) = inner.canonical_alias() { - let alias = alias.alias().trim(); - return Ok(DisplayName::Aliased(alias.to_owned())); - } - - inner.summary.clone() - }; - - let own_user_id = self.own_user_id().as_str(); - - let (heroes, num_joined_guess): (Vec, _) = if !summary.heroes_names.is_empty() { - // Straightforward path: pass through the heroes names, don't give a guess of - // the number of members. - (summary.heroes_names, None) - } else if !summary.heroes_user_ids.is_empty() { - // Use the heroes, if available. - let heroes = summary.heroes_user_ids; - - let mut names = Vec::with_capacity(heroes.len()); - for user_id in heroes { - if user_id == own_user_id { - continue; - } - if let Some(member) = self.get_member(&user_id).await? { - names.push(member.name().to_owned()); - } - } - - (names, None) - } else { - let mut joined_members = self.members(RoomMemberships::JOIN).await?; - - // Make the ordering deterministic. - joined_members.sort_unstable_by(|lhs, rhs| lhs.name().cmp(rhs.name())); - - // We can make a good prediction of the total number of members here. This might - // be incorrect if the database info is outdated. - let num_joined = Some(joined_members.len()); - - ( - joined_members - .into_iter() - .take(NUM_HEROES) - .filter_map(|u| (u.user_id() != own_user_id).then(|| u.name().to_owned())) - .collect(), - num_joined, - ) - }; - - let (num_joined, num_invited) = match self.state() { - RoomState::Invited => { - // when we were invited we don't have a proper summary, we have to do best - // guessing - (heroes.len() as u64, 1u64) - } - - RoomState::Joined if summary.joined_member_count == 0 => { - let num_joined = if let Some(num_joined) = num_joined_guess { - num_joined - } else { - self.joined_user_ids().await?.len() - }; - - (num_joined as u64, summary.invited_member_count) - } - - _ => (summary.joined_member_count, summary.invited_member_count), - }; - - debug!( - room_id = ?self.room_id(), - own_user = ?self.own_user_id, - num_joined, num_invited, - heroes = ?heroes, - "Calculating name for a room based on heroes", - ); - - Ok(calculate_room_name( - num_joined, - num_invited, - heroes.iter().map(|hero| hero.as_str()).collect(), - )) - } - /// Subscribe to the inner `RoomInfo`. pub fn subscribe_info(&self) -> Subscriber { self.inner.subscribe() From 322170339f1f9e1f88db8d0c3016e80c874d00f3 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 5 Jun 2024 16:58:08 +0200 Subject: [PATCH 04/14] base(code motion): move `calculate_room_name` into the one file where it's used --- crates/matrix-sdk-base/src/rooms/mod.rs | 77 +-------------------- crates/matrix-sdk-base/src/rooms/normal.rs | 78 +++++++++++++++++++++- 2 files changed, 77 insertions(+), 78 deletions(-) diff --git a/crates/matrix-sdk-base/src/rooms/mod.rs b/crates/matrix-sdk-base/src/rooms/mod.rs index 22db7a40c39..a5fca578bfe 100644 --- a/crates/matrix-sdk-base/src/rooms/mod.rs +++ b/crates/matrix-sdk-base/src/rooms/mod.rs @@ -350,45 +350,6 @@ impl Default for BaseRoomInfo { } } -/// Calculate room name according to step 3 of the [naming algorithm]. -/// -/// [naming algorithm]: https://spec.matrix.org/latest/client-server-api/#calculating-the-display-name-for-a-room -fn calculate_room_name( - joined_member_count: u64, - invited_member_count: u64, - mut heroes: Vec<&str>, -) -> DisplayName { - let num_heroes = heroes.len() as u64; - let invited_joined = invited_member_count + joined_member_count; - let invited_joined_minus_one = invited_joined.saturating_sub(1); - - // Stabilize ordering. - heroes.sort_unstable(); - - let names = if num_heroes == 0 && invited_joined > 1 { - format!("{} people", invited_joined) - } else if num_heroes >= invited_joined_minus_one { - heroes.join(", ") - } else if num_heroes < invited_joined_minus_one && invited_joined > 1 { - // TODO: What length does the spec want us to use here and in - // the `else`? - format!("{}, and {} others", heroes.join(", "), (invited_joined - num_heroes)) - } else { - "".to_owned() - }; - - // User is alone. - if invited_joined <= 1 { - if names.is_empty() { - DisplayName::Empty - } else { - DisplayName::EmptyWas(names) - } - } else { - DisplayName::Calculated(names) - } -} - /// The content of an `m.room.create` event, with a required `creator` field. /// /// Starting with room version 11, the `creator` field should be removed and the @@ -554,43 +515,7 @@ mod tests { use ruma::events::tag::{TagInfo, TagName, Tags}; - use super::{calculate_room_name, BaseRoomInfo, DisplayName, RoomNotableTags}; - - #[test] - fn test_calculate_room_name() { - let mut actual = calculate_room_name(2, 0, vec!["a"]); - assert_eq!(DisplayName::Calculated("a".to_owned()), actual); - - actual = calculate_room_name(3, 0, vec!["a", "b"]); - assert_eq!(DisplayName::Calculated("a, b".to_owned()), actual); - - actual = calculate_room_name(4, 0, vec!["a", "b", "c"]); - assert_eq!(DisplayName::Calculated("a, b, c".to_owned()), actual); - - actual = calculate_room_name(5, 0, vec!["a", "b", "c"]); - assert_eq!(DisplayName::Calculated("a, b, c, and 2 others".to_owned()), actual); - - actual = calculate_room_name(5, 0, vec![]); - assert_eq!(DisplayName::Calculated("5 people".to_owned()), actual); - - actual = calculate_room_name(0, 0, vec![]); - assert_eq!(DisplayName::Empty, actual); - - actual = calculate_room_name(1, 0, vec![]); - assert_eq!(DisplayName::Empty, actual); - - actual = calculate_room_name(0, 1, vec![]); - assert_eq!(DisplayName::Empty, actual); - - actual = calculate_room_name(1, 0, vec!["a"]); - assert_eq!(DisplayName::EmptyWas("a".to_owned()), actual); - - actual = calculate_room_name(1, 0, vec!["a", "b"]); - assert_eq!(DisplayName::EmptyWas("a, b".to_owned()), actual); - - actual = calculate_room_name(1, 0, vec!["a", "b", "c"]); - assert_eq!(DisplayName::EmptyWas("a, b, c".to_owned()), actual); - } + use super::{BaseRoomInfo, RoomNotableTags}; #[test] fn test_handle_notable_tags_favourite() { diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index e00b76c00ce..73f2283cc0a 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -64,7 +64,6 @@ use crate::latest_event::LatestEvent; use crate::{ deserialized_responses::MemberEvent, read_receipts::RoomReadReceipts, - rooms::calculate_room_name, store::{DynStateStore, Result as StoreResult, StateStoreExt}, sync::UnreadNotificationsCount, MinimalStateEvent, OriginalMinimalStateEvent, RoomMemberships, @@ -1396,6 +1395,45 @@ impl RoomStateFilter { } } +/// Calculate room name according to step 3 of the [naming algorithm]. +/// +/// [naming algorithm]: https://spec.matrix.org/latest/client-server-api/#calculating-the-display-name-for-a-room +fn calculate_room_name( + joined_member_count: u64, + invited_member_count: u64, + mut heroes: Vec<&str>, +) -> DisplayName { + let num_heroes = heroes.len() as u64; + let invited_joined = invited_member_count + joined_member_count; + let invited_joined_minus_one = invited_joined.saturating_sub(1); + + // Stabilize ordering. + heroes.sort_unstable(); + + let names = if num_heroes == 0 && invited_joined > 1 { + format!("{} people", invited_joined) + } else if num_heroes >= invited_joined_minus_one { + heroes.join(", ") + } else if num_heroes < invited_joined_minus_one && invited_joined > 1 { + // TODO: What length does the spec want us to use here and in + // the `else`? + format!("{}, and {} others", heroes.join(", "), (invited_joined - num_heroes)) + } else { + "".to_owned() + }; + + // User is alone. + if invited_joined <= 1 { + if names.is_empty() { + DisplayName::Empty + } else { + DisplayName::EmptyWas(names) + } + } else { + DisplayName::Calculated(names) + } +} + #[cfg(test)] mod tests { use std::{ @@ -1435,7 +1473,7 @@ mod tests { #[cfg(feature = "experimental-sliding-sync")] use super::SyncInfo; - use super::{Room, RoomInfo, RoomState}; + use super::{calculate_room_name, Room, RoomInfo, RoomState}; #[cfg(any(feature = "experimental-sliding-sync", feature = "e2e-encryption"))] use crate::latest_event::LatestEvent; use crate::{ @@ -2366,4 +2404,40 @@ mod tests { assert_eq!(Vec::::new(), room.active_room_call_participants()); assert!(!room.has_active_room_call()); } + + #[test] + fn test_calculate_room_name() { + let mut actual = calculate_room_name(2, 0, vec!["a"]); + assert_eq!(DisplayName::Calculated("a".to_owned()), actual); + + actual = calculate_room_name(3, 0, vec!["a", "b"]); + assert_eq!(DisplayName::Calculated("a, b".to_owned()), actual); + + actual = calculate_room_name(4, 0, vec!["a", "b", "c"]); + assert_eq!(DisplayName::Calculated("a, b, c".to_owned()), actual); + + actual = calculate_room_name(5, 0, vec!["a", "b", "c"]); + assert_eq!(DisplayName::Calculated("a, b, c, and 2 others".to_owned()), actual); + + actual = calculate_room_name(5, 0, vec![]); + assert_eq!(DisplayName::Calculated("5 people".to_owned()), actual); + + actual = calculate_room_name(0, 0, vec![]); + assert_eq!(DisplayName::Empty, actual); + + actual = calculate_room_name(1, 0, vec![]); + assert_eq!(DisplayName::Empty, actual); + + actual = calculate_room_name(0, 1, vec![]); + assert_eq!(DisplayName::Empty, actual); + + actual = calculate_room_name(1, 0, vec!["a"]); + assert_eq!(DisplayName::EmptyWas("a".to_owned()), actual); + + actual = calculate_room_name(1, 0, vec!["a", "b"]); + assert_eq!(DisplayName::EmptyWas("a, b".to_owned()), actual); + + actual = calculate_room_name(1, 0, vec!["a", "b", "c"]); + assert_eq!(DisplayName::EmptyWas("a, b, c".to_owned()), actual); + } } From 0e075d3a20d829dfd98c9934ce3a0f2ff53a6147 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 5 Jun 2024 17:14:58 +0200 Subject: [PATCH 05/14] sliding sync: add some simple positive tests for room names too --- crates/matrix-sdk-base/src/sliding_sync.rs | 58 ++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 62ff321e190..0e89bf3d3c4 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -738,6 +738,7 @@ mod tests { canonical_alias::RoomCanonicalAliasEventContent, member::{MembershipState, RoomMemberEventContent}, message::SyncRoomMessageEvent, + name::RoomNameEventContent, }, AnySyncMessageLikeEvent, AnySyncTimelineEvent, GlobalAccountDataEventContent, StateEventContent, @@ -842,6 +843,28 @@ mod tests { assert!(!sync_resp.rooms.invite.contains_key(room_id)); } + #[async_test] + async fn test_room_name_event() { + // Given a logged-in client + let client = logged_in_base_client(None).await; + let room_id = room_id!("!r:e.uk"); + + // When I send sliding sync response containing a room with a name set in the + // sliding sync response, and a m.room.name event, + let mut room = v4::SlidingSyncRoom::new(); + + room.name = Some("little room".to_owned()); + set_room_name(&mut room, user_id!("@a:b.c"), "The Name".to_owned()); + + let response = response_with_room(room_id, room); + client.process_sliding_sync(&response, &()).await.expect("Failed to process sync"); + + // The name is known. + let client_room = client.get_room(room_id).expect("No room found"); + assert_eq!(client_room.name().as_deref(), Some("The Name")); + assert_eq!(client_room.computed_display_name().await.unwrap().to_string(), "The Name"); + } + #[async_test] async fn test_missing_invited_room_name_event() { // Given a logged-in client, @@ -874,6 +897,32 @@ mod tests { assert!(sync_resp.rooms.invite.contains_key(room_id)); } + #[async_test] + async fn test_invited_room_name_event() { + // Given a logged-in client, + let client = logged_in_base_client(None).await; + let room_id = room_id!("!r:e.uk"); + let user_id = user_id!("@w:e.uk"); + let inviter = user_id!("@john:mastodon.org"); + + // When I send sliding sync response containing a room with a name set in the + // sliding sync response, and a m.room.name event, + let mut room = v4::SlidingSyncRoom::new(); + + set_room_invited(&mut room, inviter, user_id); + + room.name = Some("name from sliding sync response".to_owned()); + set_room_name(&mut room, user_id!("@a:b.c"), "The Name".to_owned()); + + let response = response_with_room(room_id, room); + client.process_sliding_sync(&response, &()).await.expect("Failed to process sync"); + + // The name is known. + let client_room = client.get_room(room_id).expect("No room found"); + assert_eq!(client_room.name().as_deref(), Some("The Name")); + assert_eq!(client_room.computed_display_name().await.unwrap().to_string(), "The Name"); + } + #[async_test] async fn test_left_a_room_from_required_state_event() { // Given a logged-in client @@ -1858,6 +1907,15 @@ mod tests { room } + fn set_room_name(room: &mut v4::SlidingSyncRoom, sender: &UserId, name: String) { + room.required_state.push(make_state_event( + sender, + "", + RoomNameEventContent::new(name), + None, + )); + } + fn set_room_invited(room: &mut v4::SlidingSyncRoom, inviter: &UserId, invitee: &UserId) { // MSC3575 shows an almost-empty event to indicate that we are invited to a // room. Just the type is supplied. From 60f5d8dd6d750f000afa3127bee6f8716449aa02 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Thu, 6 Jun 2024 11:21:12 +0200 Subject: [PATCH 06/14] base: make `get_or_create_room` async Since this is a good place where to recompute the cached display name at start. --- benchmarks/benches/room_bench.rs | 4 +- crates/matrix-sdk-base/src/client.rs | 59 ++++++++++--------- crates/matrix-sdk-base/src/rooms/normal.rs | 6 +- crates/matrix-sdk-base/src/sliding_sync.rs | 27 +++++---- crates/matrix-sdk-base/src/store/mod.rs | 4 +- crates/matrix-sdk/src/client/mod.rs | 3 +- crates/matrix-sdk/src/event_cache/mod.rs | 2 +- .../matrix-sdk/src/event_cache/pagination.rs | 6 +- crates/matrix-sdk/src/sliding_sync/mod.rs | 4 +- 9 files changed, 62 insertions(+), 53 deletions(-) diff --git a/benchmarks/benches/room_bench.rs b/benchmarks/benches/room_bench.rs index e9decee401f..73e173d5d26 100644 --- a/benchmarks/benches/room_bench.rs +++ b/benchmarks/benches/room_bench.rs @@ -63,6 +63,7 @@ pub fn receive_all_members_benchmark(c: &mut Criterion) { .expect("initial filling of sqlite failed"); let base_client = BaseClient::with_store_config(StoreConfig::new().state_store(sqlite_store)); + runtime .block_on(base_client.set_session_meta( SessionMeta { @@ -72,7 +73,8 @@ pub fn receive_all_members_benchmark(c: &mut Criterion) { None, )) .expect("Could not set session meta"); - base_client.get_or_create_room(&room_id, RoomState::Joined); + + runtime.block_on(base_client.get_or_create_room(&room_id, RoomState::Joined)); let request = get_member_events::v3::Request::new(room_id.clone()); let response = get_member_events::v3::Response::new(member_events); diff --git a/crates/matrix-sdk-base/src/client.rs b/crates/matrix-sdk-base/src/client.rs index 478153325fb..7202afcc1e3 100644 --- a/crates/matrix-sdk-base/src/client.rs +++ b/crates/matrix-sdk-base/src/client.rs @@ -171,8 +171,10 @@ impl BaseClient { /// 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(&self, room_id: &RoomId, room_state: RoomState) -> Room { - self.store.get_or_create_room(room_id, room_state, self.roominfo_update_sender.clone()) + pub async fn get_or_create_room(&self, room_id: &RoomId, room_state: RoomState) -> Room { + self.store + .get_or_create_room(room_id, room_state, self.roominfo_update_sender.clone()) + .await } /// Get all the rooms this client knows about. @@ -746,11 +748,10 @@ impl BaseClient { /// /// Update the internal and cached state accordingly. Return the final Room. pub async fn room_joined(&self, room_id: &RoomId) -> Result { - let room = self.store.get_or_create_room( - room_id, - RoomState::Joined, - self.roominfo_update_sender.clone(), - ); + let room = self + .store + .get_or_create_room(room_id, RoomState::Joined, self.roominfo_update_sender.clone()) + .await; if room.state() != RoomState::Joined { let _sync_lock = self.sync_lock().lock().await; @@ -772,11 +773,10 @@ impl BaseClient { /// /// Update the internal and cached state accordingly. pub async fn room_left(&self, room_id: &RoomId) -> Result<()> { - let room = self.store.get_or_create_room( - room_id, - RoomState::Left, - self.roominfo_update_sender.clone(), - ); + let room = self + .store + .get_or_create_room(room_id, RoomState::Left, self.roominfo_update_sender.clone()) + .await; if room.state() != RoomState::Left { let _sync_lock = self.sync_lock().lock().await; @@ -847,11 +847,14 @@ impl BaseClient { let mut notifications = Default::default(); for (room_id, new_info) in response.rooms.join { - let room = self.store.get_or_create_room( - &room_id, - RoomState::Joined, - self.roominfo_update_sender.clone(), - ); + let room = self + .store + .get_or_create_room( + &room_id, + RoomState::Joined, + self.roominfo_update_sender.clone(), + ) + .await; let mut room_info = room.clone_info(); room_info.mark_as_joined(); @@ -959,11 +962,10 @@ impl BaseClient { } for (room_id, new_info) in response.rooms.leave { - let room = self.store.get_or_create_room( - &room_id, - RoomState::Left, - self.roominfo_update_sender.clone(), - ); + let room = self + .store + .get_or_create_room(&room_id, RoomState::Left, self.roominfo_update_sender.clone()) + .await; let mut room_info = room.clone_info(); room_info.mark_as_left(); room_info.mark_state_partially_synced(); @@ -1017,11 +1019,14 @@ impl BaseClient { } for (room_id, new_info) in response.rooms.invite { - let room = self.store.get_or_create_room( - &room_id, - RoomState::Invited, - self.roominfo_update_sender.clone(), - ); + let room = self + .store + .get_or_create_room( + &room_id, + RoomState::Invited, + self.roominfo_update_sender.clone(), + ) + .await; let mut room_info = room.clone_info(); room_info.mark_as_invited(); room_info.mark_state_fully_synced(); diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index 73f2283cc0a..068e3ab77c1 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -1658,7 +1658,7 @@ mod tests { .unwrap(); let room_id = room_id!("!test:localhost"); - let room = client.get_or_create_room(room_id, RoomState::Joined); + let room = client.get_or_create_room(room_id, RoomState::Joined).await; // Sanity checks to ensure the room isn't marked as favourite. assert!(room.is_favourite().not()); @@ -1732,7 +1732,7 @@ mod tests { .unwrap(); let room_id = room_id!("!test:localhost"); - let room = client.get_or_create_room(room_id, RoomState::Joined); + let room = client.get_or_create_room(room_id, RoomState::Joined).await; // Sanity checks to ensure the room isn't marked as low priority. assert!(!room.is_low_priority()); @@ -2179,7 +2179,7 @@ mod tests { .unwrap(); let room_id = room_id!("!test:localhost"); - let room = client.get_or_create_room(room_id, RoomState::Joined); + let room = client.get_or_create_room(room_id, RoomState::Joined).await; // That has an encrypted event, add_encrypted_event(&room, "$A"); diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 0e89bf3d3c4..17c7d617527 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -331,8 +331,9 @@ impl BaseClient { // Find or create the room in the store #[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); + let (mut room, mut room_info, invited_room) = self + .process_sliding_sync_room_membership(room_data, &state_events, store, room_id) + .await; room_info.mark_state_partially_synced(); @@ -452,7 +453,7 @@ impl BaseClient { /// If any invite_state exists, we take it to mean that we are invited to /// this room, unless that state contains membership events that specify /// otherwise. https://github.com/matrix-org/matrix-spec-proposals/blob/kegan/sync-v3/proposals/3575-sync.md#room-list-parameters - fn process_sliding_sync_room_membership( + async fn process_sliding_sync_room_membership( &self, room_data: &v4::SlidingSyncRoom, state_events: &[AnySyncStateEvent], @@ -460,11 +461,13 @@ impl BaseClient { room_id: &RoomId, ) -> (Room, RoomInfo, Option) { if let Some(invite_state) = &room_data.invite_state { - let room = store.get_or_create_room( - room_id, - RoomState::Invited, - self.roominfo_update_sender.clone(), - ); + let room = store + .get_or_create_room( + room_id, + RoomState::Invited, + self.roominfo_update_sender.clone(), + ) + .await; let mut room_info = room.clone_info(); // We don't actually know what events are inside invite_state. In theory, they @@ -482,11 +485,9 @@ impl BaseClient { (room, room_info, Some(InvitedRoom::from(v3::InviteState::from(invite_state.clone())))) } else { - let room = store.get_or_create_room( - room_id, - RoomState::Joined, - self.roominfo_update_sender.clone(), - ); + let room = store + .get_or_create_room(room_id, RoomState::Joined, self.roominfo_update_sender.clone()) + .await; let mut room_info = room.clone_info(); // We default to considering this room joined if it's not an invite. If it's diff --git a/crates/matrix-sdk-base/src/store/mod.rs b/crates/matrix-sdk-base/src/store/mod.rs index c812770aaed..a2109553333 100644 --- a/crates/matrix-sdk-base/src/store/mod.rs +++ b/crates/matrix-sdk-base/src/store/mod.rs @@ -222,8 +222,8 @@ impl Store { } /// 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( + /// yet in the store. + pub async fn get_or_create_room( &self, room_id: &RoomId, room_type: RoomState, diff --git a/crates/matrix-sdk/src/client/mod.rs b/crates/matrix-sdk/src/client/mod.rs index 652162c201c..09b3a6af36c 100644 --- a/crates/matrix-sdk/src/client/mod.rs +++ b/crates/matrix-sdk/src/client/mod.rs @@ -1244,7 +1244,8 @@ impl Client { let is_direct_room = request.is_direct; let response = self.send(request, None).await?; - let base_room = self.base_client().get_or_create_room(&response.room_id, RoomState::Joined); + let base_room = + self.base_client().get_or_create_room(&response.room_id, RoomState::Joined).await; let joined_room = Room::new(self.clone(), base_room); diff --git a/crates/matrix-sdk/src/event_cache/mod.rs b/crates/matrix-sdk/src/event_cache/mod.rs index 47e032a90b6..f0482b0ef6e 100644 --- a/crates/matrix-sdk/src/event_cache/mod.rs +++ b/crates/matrix-sdk/src/event_cache/mod.rs @@ -787,7 +787,7 @@ mod tests { async fn test_uniq_read_marker() { let client = logged_in_client(None).await; let room_id = room_id!("!galette:saucisse.bzh"); - client.base_client().get_or_create_room(room_id, matrix_sdk_base::RoomState::Joined); + client.base_client().get_or_create_room(room_id, matrix_sdk_base::RoomState::Joined).await; let event_cache = client.event_cache(); diff --git a/crates/matrix-sdk/src/event_cache/pagination.rs b/crates/matrix-sdk/src/event_cache/pagination.rs index a24af594094..0ba7fd89f6c 100644 --- a/crates/matrix-sdk/src/event_cache/pagination.rs +++ b/crates/matrix-sdk/src/event_cache/pagination.rs @@ -345,7 +345,7 @@ mod tests { async fn test_wait_no_pagination_token() { let client = logged_in_client(None).await; let room_id = room_id!("!galette:saucisse.bzh"); - client.base_client().get_or_create_room(room_id, RoomState::Joined); + client.base_client().get_or_create_room(room_id, RoomState::Joined).await; let event_cache = client.event_cache(); @@ -398,7 +398,7 @@ mod tests { async fn test_wait_for_pagination_token_already_present() { let client = logged_in_client(None).await; let room_id = room_id!("!galette:saucisse.bzh"); - client.base_client().get_or_create_room(room_id, RoomState::Joined); + client.base_client().get_or_create_room(room_id, RoomState::Joined).await; let event_cache = client.event_cache(); @@ -453,7 +453,7 @@ mod tests { async fn test_wait_for_late_pagination_token() { let client = logged_in_client(None).await; let room_id = room_id!("!galette:saucisse.bzh"); - client.base_client().get_or_create_room(room_id, RoomState::Joined); + client.base_client().get_or_create_room(room_id, RoomState::Joined).await; let event_cache = client.event_cache(); diff --git a/crates/matrix-sdk/src/sliding_sync/mod.rs b/crates/matrix-sdk/src/sliding_sync/mod.rs index dff893e3a71..7e9eba88fa5 100644 --- a/crates/matrix-sdk/src/sliding_sync/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/mod.rs @@ -2187,7 +2187,7 @@ mod tests { let server = MockServer::start().await; let client = logged_in_client(Some(server.uri())).await; - client.base_client().get_or_create_room(&room_id, RoomState::Joined); + client.base_client().get_or_create_room(&room_id, RoomState::Joined).await; // Setup sliding sync with with one room and one list @@ -2310,7 +2310,7 @@ mod tests { let server = MockServer::start().await; let client = logged_in_client(Some(server.uri())).await; - client.base_client().get_or_create_room(&room, RoomState::Joined); + client.base_client().get_or_create_room(&room, RoomState::Joined).await; let sliding_sync = client .sliding_sync("test")? From 4c790bdbdaf4bf2057bfc45ed8fa8ecce444194f Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Thu, 6 Jun 2024 11:14:38 +0200 Subject: [PATCH 07/14] base: add a cache to the computed display name --- crates/matrix-sdk-base/src/rooms/normal.rs | 122 ++++++++++++--------- crates/matrix-sdk-base/src/sliding_sync.rs | 17 +-- crates/matrix-sdk-base/src/store/mod.rs | 31 ++++-- crates/matrix-sdk/src/sync.rs | 13 +++ 4 files changed, 115 insertions(+), 68 deletions(-) diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index 068e3ab77c1..88549a35a36 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -12,12 +12,10 @@ // See the License for the specific language governing permissions and // limitations under the License. -#[cfg(all(feature = "e2e-encryption", feature = "experimental-sliding-sync"))] -use std::sync::RwLock as SyncRwLock; use std::{ collections::{BTreeMap, HashSet}, mem, - sync::{atomic::AtomicBool, Arc}, + sync::{atomic::AtomicBool, Arc, RwLock as SyncRwLock}, }; use bitflags::bitflags; @@ -104,6 +102,13 @@ pub struct Room { /// to disk but held in memory. #[cfg(all(feature = "e2e-encryption", feature = "experimental-sliding-sync"))] pub latest_encrypted_events: Arc>>>, + + /// Cached value of the last call to `computed_display_name()`, useful for + /// sync access. + /// + /// Filled by calling `computed_display_name`. It's automatically filled at + /// start, when creating a room, or on every successful sync. + pub(crate) cached_computed_display_name: Arc>>, } /// The room summary containing member counts and members that should be used to @@ -175,7 +180,7 @@ impl Room { const MAX_ENCRYPTED_EVENTS: std::num::NonZeroUsize = unsafe { std::num::NonZeroUsize::new_unchecked(10) }; - pub(crate) fn new( + pub(crate) async fn new( own_user_id: &UserId, store: Arc, room_id: &RoomId, @@ -183,16 +188,16 @@ impl Room { roominfo_update_sender: broadcast::Sender, ) -> Self { let room_info = RoomInfo::new(room_id, room_state); - Self::restore(own_user_id, store, room_info, roominfo_update_sender) + Self::restore(own_user_id, store, room_info, roominfo_update_sender).await } - pub(crate) fn restore( + pub(crate) async fn restore( own_user_id: &UserId, store: Arc, room_info: RoomInfo, roominfo_update_sender: broadcast::Sender, ) -> Self { - Self { + let room = Self { own_user_id: own_user_id.into(), room_id: room_info.room_id.clone(), store, @@ -202,7 +207,13 @@ impl Room { Self::MAX_ENCRYPTED_EVENTS, ))), roominfo_update_sender, - } + cached_computed_display_name: Arc::new(SyncRwLock::new(None)), + }; + + // Refill the computed_display_name cache. + let _ = room.computed_display_name().await; + + room } /// Get the unique room id of the room. @@ -478,24 +489,32 @@ impl Room { /// /// The display name is calculated according to [this algorithm][spec]. /// + /// This is automatically recomputed on every successful sync, and the + /// cached result can be retrieved in + /// [`Self::cached_computed_display_name`]. + /// /// [spec]: pub async fn computed_display_name(&self) -> StoreResult { + let update_cache = |new_val: DisplayName| { + *self.cached_computed_display_name.write().unwrap() = Some(new_val.clone()); + new_val + }; + let summary = { let inner = self.inner.read(); if let Some(name) = inner.name() { - let name = name.trim(); - return Ok(DisplayName::Named(name.to_owned())); + return Ok(update_cache(DisplayName::Named(name.trim().to_owned()))); } if let Some(alias) = inner.canonical_alias() { - let alias = alias.alias().trim(); - return Ok(DisplayName::Aliased(alias.to_owned())); + return Ok(update_cache(DisplayName::Aliased(alias.alias().trim().to_owned()))); } inner.summary.clone() }; + // From here, use some heroes to compute the room's name. let own_user_id = self.own_user_id().as_str(); let (heroes, num_joined_guess): (Vec, _) = if !summary.heroes_names.is_empty() { @@ -565,11 +584,16 @@ impl Room { "Calculating name for a room based on heroes", ); - Ok(calculate_room_name( + Ok(update_cache(calculate_room_name( num_joined, num_invited, heroes.iter().map(|hero| hero.as_str()).collect(), - )) + ))) + } + + /// Returns the cached computed display name, if available. + pub fn cached_computed_display_name(&self) -> Option { + self.cached_computed_display_name.read().unwrap().clone() } /// Return the last event in this room, if one has been cached during @@ -1562,7 +1586,7 @@ mod tests { "num_notifications": 0, "latest_active": null, "pending": [] - } + }, }); assert_eq!(serde_json::to_value(info).unwrap(), info_json); @@ -1608,7 +1632,7 @@ mod tests { "name": null, "tombstone": null, "topic": null, - } + }, }); let info: RoomInfo = serde_json::from_value(info_json).unwrap(); @@ -1788,13 +1812,13 @@ mod tests { assert!(room.is_low_priority().not()); } - fn make_room(room_type: RoomState) -> (Arc, Room) { + async fn make_room_test_helper(room_type: RoomState) -> (Arc, Room) { let store = Arc::new(MemoryStore::new()); let user_id = user_id!("@me:example.org"); let room_id = room_id!("!test:localhost"); let (sender, _receiver) = tokio::sync::broadcast::channel(1); - (store.clone(), Room::new(user_id, store, room_id, room_type, sender)) + (store.clone(), Room::new(user_id, store, room_id, room_type, sender).await) } fn make_stripped_member_event(user_id: &UserId, name: &str) -> Raw { @@ -1827,13 +1851,13 @@ mod tests { #[async_test] async fn test_display_name_for_joined_room_is_empty_if_no_info() { - let (_, room) = make_room(RoomState::Joined); + let (_, room) = make_room_test_helper(RoomState::Joined).await; assert_eq!(room.computed_display_name().await.unwrap(), DisplayName::Empty); } #[async_test] async fn test_display_name_for_joined_room_uses_canonical_alias_if_available() { - let (_, room) = make_room(RoomState::Joined); + let (_, room) = make_room_test_helper(RoomState::Joined).await; room.inner .update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event())); assert_eq!( @@ -1844,7 +1868,7 @@ mod tests { #[async_test] async fn test_display_name_for_joined_room_prefers_name_over_alias() { - let (_, room) = make_room(RoomState::Joined); + let (_, room) = make_room_test_helper(RoomState::Joined).await; room.inner .update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event())); assert_eq!( @@ -1861,13 +1885,13 @@ mod tests { #[async_test] async fn test_display_name_for_invited_room_is_empty_if_no_info() { - let (_, room) = make_room(RoomState::Invited); + let (_, room) = make_room_test_helper(RoomState::Invited).await; assert_eq!(room.computed_display_name().await.unwrap(), DisplayName::Empty); } #[async_test] async fn test_display_name_for_invited_room_is_empty_if_room_name_empty() { - let (_, room) = make_room(RoomState::Invited); + let (_, room) = make_room_test_helper(RoomState::Invited).await; let room_name = MinimalStateEvent::Original(OriginalMinimalStateEvent { content: RoomNameEventContent::new(String::new()), @@ -1880,7 +1904,7 @@ mod tests { #[async_test] async fn test_display_name_for_invited_room_uses_canonical_alias_if_available() { - let (_, room) = make_room(RoomState::Invited); + let (_, room) = make_room_test_helper(RoomState::Invited).await; room.inner .update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event())); assert_eq!( @@ -1891,7 +1915,7 @@ mod tests { #[async_test] async fn test_display_name_for_invited_room_prefers_name_over_alias() { - let (_, room) = make_room(RoomState::Invited); + let (_, room) = make_room_test_helper(RoomState::Invited).await; room.inner .update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event())); assert_eq!( @@ -1924,7 +1948,7 @@ mod tests { #[async_test] async fn test_display_name_dm_invited() { - let (store, room) = make_room(RoomState::Invited); + let (store, room) = make_room_test_helper(RoomState::Invited).await; let room_id = room_id!("!test:localhost"); let matthew = user_id!("@matthew:example.org"); let me = user_id!("@me:example.org"); @@ -1950,7 +1974,7 @@ mod tests { #[async_test] async fn test_display_name_dm_invited_no_heroes() { - let (store, room) = make_room(RoomState::Invited); + let (store, room) = make_room_test_helper(RoomState::Invited).await; let room_id = room_id!("!test:localhost"); let matthew = user_id!("@matthew:example.org"); let me = user_id!("@me:example.org"); @@ -1972,7 +1996,7 @@ mod tests { #[async_test] async fn test_display_name_dm_joined() { - let (store, room) = make_room(RoomState::Joined); + let (store, room) = make_room_test_helper(RoomState::Joined).await; let room_id = room_id!("!test:localhost"); let matthew = user_id!("@matthew:example.org"); let me = user_id!("@me:example.org"); @@ -2002,7 +2026,7 @@ mod tests { #[async_test] async fn test_display_name_dm_joined_no_heroes() { - let (store, room) = make_room(RoomState::Joined); + let (store, room) = make_room_test_helper(RoomState::Joined).await; let room_id = room_id!("!test:localhost"); let matthew = user_id!("@matthew:example.org"); let me = user_id!("@me:example.org"); @@ -2027,7 +2051,7 @@ mod tests { #[async_test] async fn test_display_name_deterministic() { - let (store, room) = make_room(RoomState::Joined); + let (store, room) = make_room_test_helper(RoomState::Joined).await; let alice = user_id!("@alice:example.org"); let bob = user_id!("@bob:example.org"); @@ -2082,7 +2106,7 @@ mod tests { #[async_test] async fn test_display_name_deterministic_no_heroes() { - let (store, room) = make_room(RoomState::Joined); + let (store, room) = make_room_test_helper(RoomState::Joined).await; let alice = user_id!("@alice:example.org"); let bob = user_id!("@bob:example.org"); @@ -2131,7 +2155,7 @@ mod tests { #[async_test] async fn test_display_name_dm_alone() { - let (store, room) = make_room(RoomState::Joined); + let (store, room) = make_room_test_helper(RoomState::Joined).await; let room_id = room_id!("!test:localhost"); let matthew = user_id!("@matthew:example.org"); let me = user_id!("@me:example.org"); @@ -2207,11 +2231,11 @@ mod tests { assert_pending!(room_info_subscriber); } - #[test] + #[async_test] #[cfg(feature = "experimental-sliding-sync")] - fn test_when_we_provide_a_newly_decrypted_event_it_replaces_latest_event() { + async fn test_when_we_provide_a_newly_decrypted_event_it_replaces_latest_event() { // Given a room with an encrypted event - let (_store, room) = make_room(RoomState::Joined); + let (_store, room) = make_room_test_helper(RoomState::Joined).await; add_encrypted_event(&room, "$A"); // Sanity: it has no latest_event assert!(room.latest_event().is_none()); @@ -2226,11 +2250,11 @@ mod tests { assert_eq!(room.latest_event().unwrap().event_id(), event.event_id()); } - #[test] + #[async_test] #[cfg(feature = "experimental-sliding-sync")] - fn test_when_a_newly_decrypted_event_appears_we_delete_all_older_encrypted_events() { + async fn test_when_a_newly_decrypted_event_appears_we_delete_all_older_encrypted_events() { // Given a room with some encrypted events and a latest event - let (_store, room) = make_room(RoomState::Joined); + let (_store, room) = make_room_test_helper(RoomState::Joined).await; room.inner.update(|info| info.latest_event = Some(make_latest_event("$A"))); add_encrypted_event(&room, "$0"); add_encrypted_event(&room, "$1"); @@ -2254,11 +2278,11 @@ mod tests { assert_eq!(room.latest_event().unwrap().event_id(), new_event.event_id()); } - #[test] + #[async_test] #[cfg(feature = "experimental-sliding-sync")] - fn test_replacing_the_newest_event_leaves_none_left() { + async fn test_replacing_the_newest_event_leaves_none_left() { // Given a room with some encrypted events - let (_store, room) = make_room(RoomState::Joined); + let (_store, room) = make_room_test_helper(RoomState::Joined).await; add_encrypted_event(&room, "$0"); add_encrypted_event(&room, "$1"); add_encrypted_event(&room, "$2"); @@ -2356,8 +2380,8 @@ mod tests { /// `user_a`: empty memberships /// `user_b`: one membership /// `user_c`: two memberships (two devices) - fn create_call_with_member_events_for_user(a: &UserId, b: &UserId, c: &UserId) -> Room { - let (_, room) = make_room(RoomState::Joined); + async fn create_call_with_member_events_for_user(a: &UserId, b: &UserId, c: &UserId) -> Room { + let (_, room) = make_room_test_helper(RoomState::Joined).await; let a_empty = call_member_state_event(Vec::new(), "$1234", a); @@ -2377,9 +2401,9 @@ mod tests { room } - #[test] - fn test_show_correct_active_call_state() { - let room = create_call_with_member_events_for_user(&ALICE, &BOB, &CAROL); + #[async_test] + async fn test_show_correct_active_call_state() { + let room = create_call_with_member_events_for_user(&ALICE, &BOB, &CAROL).await; // This check also tests the ordering. // We want older events to be in the front. @@ -2391,9 +2415,9 @@ mod tests { assert!(room.has_active_room_call()); } - #[test] - fn test_active_call_is_false_when_everyone_left() { - let room = create_call_with_member_events_for_user(&ALICE, &BOB, &CAROL); + #[async_test] + async fn test_active_call_is_false_when_everyone_left() { + let room = create_call_with_member_events_for_user(&ALICE, &BOB, &CAROL).await; let b_empty_membership = call_member_state_event(Vec::new(), "$1234_1", &BOB); let c_empty_membership = call_member_state_event(Vec::new(), "$12345_1", &CAROL); diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 17c7d617527..7ce1b94b620 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -1510,7 +1510,7 @@ mod tests { let events = &[event1, event2.clone(), event3.clone(), event4.clone()]; // When I ask to cache events - let room = make_room(); + let room = make_room().await; let mut room_info = room.clone_info(); cache_latest_events(&room, &mut room_info, events, None, None).await; @@ -1539,7 +1539,7 @@ mod tests { let events = &[event1, event2.clone(), event3.clone()]; // When I ask to cache events - let room = make_room(); + let room = make_room().await; let mut room_info = room.clone_info(); cache_latest_events(&room, &mut room_info, events, None, None).await; room.set_room_info(room_info, false); @@ -1566,7 +1566,7 @@ mod tests { let events = &[event1, event2.clone(), event3.clone(), event4, event5.clone()]; // When I ask to cache events - let room = make_room(); + let room = make_room().await; let mut room_info = room.clone_info(); cache_latest_events(&room, &mut room_info, events, None, None).await; room.set_room_info(room_info, false); @@ -1619,7 +1619,7 @@ mod tests { ]; // When I ask to cache events - let room = make_room(); + let room = make_room().await; let mut room_info = room.clone_info(); cache_latest_events(&room, &mut room_info, events, None, None).await; room.set_room_info(room_info, false); @@ -1642,7 +1642,7 @@ mod tests { #[async_test] async fn test_dont_overflow_capacity_if_previous_encrypted_events_exist() { // Given a RoomInfo with lots of encrypted events already inside it - let room = make_room(); + let room = make_room().await; let mut room_info = room.clone_info(); cache_latest_events( &room, @@ -1684,7 +1684,7 @@ mod tests { #[async_test] async fn test_existing_encrypted_events_are_deleted_if_we_receive_unencrypted() { // Given a RoomInfo with some encrypted events already inside it - let room = make_room(); + let room = make_room().await; let mut room_info = room.clone_info(); cache_latest_events( &room, @@ -1710,7 +1710,7 @@ mod tests { } async fn choose_event_to_cache(events: &[SyncTimelineEvent]) -> Option { - let room = make_room(); + let room = make_room().await; let mut room_info = room.clone_info(); cache_latest_events(&room, &mut room_info, events, None, None).await; room.set_room_info(room_info, false); @@ -1733,7 +1733,7 @@ mod tests { events.iter().map(|e| e.event_id().unwrap().to_string()).collect() } - fn make_room() -> Room { + async fn make_room() -> Room { let (sender, _receiver) = tokio::sync::broadcast::channel(1); Room::new( @@ -1743,6 +1743,7 @@ mod tests { RoomState::Joined, sender, ) + .await } fn make_event(typ: &str, id: &str) -> SyncTimelineEvent { diff --git a/crates/matrix-sdk-base/src/store/mod.rs b/crates/matrix-sdk-base/src/store/mod.rs index a2109553333..90a533ba833 100644 --- a/crates/matrix-sdk-base/src/store/mod.rs +++ b/crates/matrix-sdk-base/src/store/mod.rs @@ -182,7 +182,9 @@ impl Store { self.inner.clone(), info, roominfo_update_sender.clone(), - ); + ) + .await; + self.rooms.write().unwrap().insert(room.room_id().to_owned(), room); } @@ -229,17 +231,24 @@ impl Store { room_type: RoomState, roominfo_update_sender: broadcast::Sender, ) -> Room { - let user_id = - &self.session_meta.get().expect("Creating room while not being logged in").user_id; + let user_id = self + .session_meta + .get() + .expect("Creating room while not being logged in") + .user_id + .clone(); + + if let Some(room) = self.rooms.read().unwrap().get(room_id).cloned() { + return room; + } - self.rooms - .write() - .unwrap() - .entry(room_id.to_owned()) - .or_insert_with(|| { - Room::new(user_id, self.inner.clone(), room_id, room_type, roominfo_update_sender) - }) - .clone() + let room = + Room::new(&user_id, self.inner.clone(), room_id, room_type, roominfo_update_sender) + .await; + + // Use entry() because another caller could have racily written the room in + // the meanwhile. + self.rooms.write().unwrap().entry(room_id.to_owned()).or_insert(room).clone() } } diff --git a/crates/matrix-sdk/src/sync.rs b/crates/matrix-sdk/src/sync.rs index 1aa3792c2c3..9ca4d760427 100644 --- a/crates/matrix-sdk/src/sync.rs +++ b/crates/matrix-sdk/src/sync.rs @@ -154,6 +154,19 @@ impl Client { ) -> Result<()> { let BaseSyncResponse { rooms, presence, account_data, to_device, notifications } = response; + { + // Recompute the computed display name for all the rooms which had an update. + for room in rooms + .leave + .keys() + .chain(rooms.join.keys()) + .chain(rooms.invite.keys()) + .filter_map(|room_id| self.get_room(room_id)) + { + let _ = room.computed_display_name().await; + } + } + let now = Instant::now(); self.handle_sync_events(HandlerKind::GlobalAccountData, None, account_data).await?; self.handle_sync_events(HandlerKind::Presence, None, presence).await?; From 256d7b3d1ce653db71dc363fbc35b78439c2d619 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Thu, 6 Jun 2024 11:41:16 +0200 Subject: [PATCH 08/14] ui: make use of the cached computed display name in the room name matchers --- .../src/room_list_service/filters/fuzzy_match_room_name.rs | 4 ++-- .../room_list_service/filters/normalized_match_room_name.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/matrix-sdk-ui/src/room_list_service/filters/fuzzy_match_room_name.rs b/crates/matrix-sdk-ui/src/room_list_service/filters/fuzzy_match_room_name.rs index 4b476035fc6..196f0bbc06e 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/filters/fuzzy_match_room_name.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/filters/fuzzy_match_room_name.rs @@ -53,9 +53,9 @@ pub fn new_filter(client: &Client, pattern: &str) -> impl Filter { move |room_list_entry| -> bool { let Some(room_id) = room_list_entry.as_room_id() else { return false }; let Some(room) = client.get_room(room_id) else { return false }; - let Some(room_name) = room.name() else { return false }; + let Some(room_name) = room.cached_computed_display_name() else { return false }; - searcher.matches(&room_name) + searcher.matches(&room_name.to_string()) } } diff --git a/crates/matrix-sdk-ui/src/room_list_service/filters/normalized_match_room_name.rs b/crates/matrix-sdk-ui/src/room_list_service/filters/normalized_match_room_name.rs index 184404bd78e..05a3ec6a163 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/filters/normalized_match_room_name.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/filters/normalized_match_room_name.rs @@ -53,9 +53,9 @@ pub fn new_filter(client: &Client, pattern: &str) -> impl Filter { move |room_list_entry| -> bool { let Some(room_id) = room_list_entry.as_room_id() else { return false }; let Some(room) = client.get_room(room_id) else { return false }; - let Some(room_name) = room.name() else { return false }; + let Some(room_name) = room.cached_computed_display_name() else { return false }; - searcher.matches(&room_name) + searcher.matches(&room_name.to_string()) } } From 22911dde8e83ba4cff3a0bc784c90a7348ca1edd Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Thu, 6 Jun 2024 12:10:20 +0200 Subject: [PATCH 09/14] base: rename `calculate_room_name` to `compute_display_name_from_heroes` --- crates/matrix-sdk-base/src/rooms/normal.rs | 28 +++++++++++----------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index 88549a35a36..c75c325f163 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -584,7 +584,7 @@ impl Room { "Calculating name for a room based on heroes", ); - Ok(update_cache(calculate_room_name( + Ok(update_cache(compute_display_name_from_heroes( num_joined, num_invited, heroes.iter().map(|hero| hero.as_str()).collect(), @@ -1422,7 +1422,7 @@ impl RoomStateFilter { /// Calculate room name according to step 3 of the [naming algorithm]. /// /// [naming algorithm]: https://spec.matrix.org/latest/client-server-api/#calculating-the-display-name-for-a-room -fn calculate_room_name( +fn compute_display_name_from_heroes( joined_member_count: u64, invited_member_count: u64, mut heroes: Vec<&str>, @@ -1497,7 +1497,7 @@ mod tests { #[cfg(feature = "experimental-sliding-sync")] use super::SyncInfo; - use super::{calculate_room_name, Room, RoomInfo, RoomState}; + use super::{compute_display_name_from_heroes, Room, RoomInfo, RoomState}; #[cfg(any(feature = "experimental-sliding-sync", feature = "e2e-encryption"))] use crate::latest_event::LatestEvent; use crate::{ @@ -2431,37 +2431,37 @@ mod tests { #[test] fn test_calculate_room_name() { - let mut actual = calculate_room_name(2, 0, vec!["a"]); + let mut actual = compute_display_name_from_heroes(2, 0, vec!["a"]); assert_eq!(DisplayName::Calculated("a".to_owned()), actual); - actual = calculate_room_name(3, 0, vec!["a", "b"]); + actual = compute_display_name_from_heroes(3, 0, vec!["a", "b"]); assert_eq!(DisplayName::Calculated("a, b".to_owned()), actual); - actual = calculate_room_name(4, 0, vec!["a", "b", "c"]); + actual = compute_display_name_from_heroes(4, 0, vec!["a", "b", "c"]); assert_eq!(DisplayName::Calculated("a, b, c".to_owned()), actual); - actual = calculate_room_name(5, 0, vec!["a", "b", "c"]); + actual = compute_display_name_from_heroes(5, 0, vec!["a", "b", "c"]); assert_eq!(DisplayName::Calculated("a, b, c, and 2 others".to_owned()), actual); - actual = calculate_room_name(5, 0, vec![]); + actual = compute_display_name_from_heroes(5, 0, vec![]); assert_eq!(DisplayName::Calculated("5 people".to_owned()), actual); - actual = calculate_room_name(0, 0, vec![]); + actual = compute_display_name_from_heroes(0, 0, vec![]); assert_eq!(DisplayName::Empty, actual); - actual = calculate_room_name(1, 0, vec![]); + actual = compute_display_name_from_heroes(1, 0, vec![]); assert_eq!(DisplayName::Empty, actual); - actual = calculate_room_name(0, 1, vec![]); + actual = compute_display_name_from_heroes(0, 1, vec![]); assert_eq!(DisplayName::Empty, actual); - actual = calculate_room_name(1, 0, vec!["a"]); + actual = compute_display_name_from_heroes(1, 0, vec!["a"]); assert_eq!(DisplayName::EmptyWas("a".to_owned()), actual); - actual = calculate_room_name(1, 0, vec!["a", "b"]); + actual = compute_display_name_from_heroes(1, 0, vec!["a", "b"]); assert_eq!(DisplayName::EmptyWas("a, b".to_owned()), actual); - actual = calculate_room_name(1, 0, vec!["a", "b", "c"]); + actual = compute_display_name_from_heroes(1, 0, vec!["a", "b", "c"]); assert_eq!(DisplayName::EmptyWas("a, b, c".to_owned()), actual); } } From 9021c189be3c7620e0ba09d3f0c291c46c90bd89 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Thu, 6 Jun 2024 12:03:04 +0200 Subject: [PATCH 10/14] sdk: use the cached computed display name in more places --- bindings/matrix-sdk-ffi/src/room.rs | 4 ++-- bindings/matrix-sdk-ffi/src/room_info.rs | 2 +- bindings/matrix-sdk-ffi/src/room_list.rs | 2 +- crates/matrix-sdk-ui/src/room_list_service/room.rs | 4 ++-- crates/matrix-sdk-ui/tests/integration/room_list_service.rs | 6 +++--- labs/multiverse/src/main.rs | 2 +- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/room.rs b/bindings/matrix-sdk-ffi/src/room.rs index bf90d8f0fa2..2d3ae4c4cb1 100644 --- a/bindings/matrix-sdk-ffi/src/room.rs +++ b/bindings/matrix-sdk-ffi/src/room.rs @@ -81,8 +81,8 @@ impl Room { /// Returns the room's name from the state event if available, otherwise /// compute a room name based on the room's nature (DM or not) and number of /// members. - pub fn display_name(&self) -> Result { - Ok(RUNTIME.block_on(self.inner.computed_display_name())?.to_string()) + pub fn display_name(&self) -> Option { + Some(self.inner.cached_computed_display_name()?.to_string()) } /// The raw name as present in the room state event. diff --git a/bindings/matrix-sdk-ffi/src/room_info.rs b/bindings/matrix-sdk-ffi/src/room_info.rs index 9abaeecac74..64cbedbc347 100644 --- a/bindings/matrix-sdk-ffi/src/room_info.rs +++ b/bindings/matrix-sdk-ffi/src/room_info.rs @@ -69,7 +69,7 @@ impl RoomInfo { Ok(Self { id: room.room_id().to_string(), - display_name: room.computed_display_name().await.ok().map(|name| name.to_string()), + display_name: room.cached_computed_display_name().map(|name| name.to_string()), raw_name: room.name(), topic: room.topic(), avatar_url: room.avatar_url().map(Into::into), diff --git a/bindings/matrix-sdk-ffi/src/room_list.rs b/bindings/matrix-sdk-ffi/src/room_list.rs index 4e76cb075dc..8a3571a064a 100644 --- a/bindings/matrix-sdk-ffi/src/room_list.rs +++ b/bindings/matrix-sdk-ffi/src/room_list.rs @@ -489,7 +489,7 @@ impl RoomListItem { /// compute a room name based on the room's nature (DM or not) and number of /// members. fn display_name(&self) -> Option { - RUNTIME.block_on(self.inner.computed_display_name()) + self.inner.cached_display_name() } fn avatar_url(&self) -> Option { 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 667ed1fde9d..a2acec1075f 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/room.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/room.rs @@ -84,8 +84,8 @@ impl Room { } /// Get a computed room name for the room. - pub async fn computed_display_name(&self) -> Option { - Some(self.inner.room.computed_display_name().await.ok()?.to_string()) + pub fn cached_display_name(&self) -> Option { + Some(self.inner.room.cached_computed_display_name()?.to_string()) } /// Get the underlying [`matrix_sdk::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 0821b0f5bae..4237257615a 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs @@ -2075,7 +2075,7 @@ async fn test_room() -> Result<(), Error> { let room0 = room_list.room(room_id_0).await?; // Room has received a name from sliding sync. - assert_eq!(room0.computed_display_name().await, Some("Room #0".to_owned())); + assert_eq!(room0.cached_display_name(), Some("Room #0".to_owned())); // Room has received an avatar from sliding sync. assert_eq!(room0.avatar_url(), Some(mxc_uri!("mxc://homeserver/media").to_owned())); @@ -2083,7 +2083,7 @@ async fn test_room() -> Result<(), Error> { let room1 = room_list.room(room_id_1).await?; // Room has not received a name from sliding sync, then it's calculated. - assert_eq!(room1.computed_display_name().await, Some("Empty Room".to_owned())); + assert_eq!(room1.cached_display_name(), Some("Empty Room".to_owned())); // Room has not received an avatar from sliding sync, then it's calculated, but // there is nothing to calculate from, so there is no URL. @@ -2129,7 +2129,7 @@ async fn test_room() -> Result<(), Error> { }; // Room has _now_ received a name from sliding sync! - assert_eq!(room1.computed_display_name().await, Some("Room #1".to_owned())); + assert_eq!(room1.cached_display_name(), Some("Room #1".to_owned())); // Room has _now_ received an avatar URL from sliding sync! assert_eq!(room1.avatar_url(), Some(mxc_uri!("mxc://homeserver/other-media").to_owned())); diff --git a/labs/multiverse/src/main.rs b/labs/multiverse/src/main.rs index d3b56442db4..775b97651c8 100644 --- a/labs/multiverse/src/main.rs +++ b/labs/multiverse/src/main.rs @@ -272,7 +272,7 @@ impl App { for (room_id, room) in &new_ui_rooms { let raw_name = room.name(); - let display_name = room.computed_display_name().await; + let display_name = room.cached_display_name(); room_infos .lock() .unwrap() From 823226a3cdbe2f4c83151ecc99fcc9e4f0521c22 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Thu, 6 Jun 2024 12:23:28 +0200 Subject: [PATCH 11/14] base: rename computed_display_name to compute_display_name and remove computed_ in the cached one --- bindings/matrix-sdk-ffi/src/room.rs | 2 +- bindings/matrix-sdk-ffi/src/room_info.rs | 2 +- crates/matrix-sdk-base/src/client.rs | 2 +- crates/matrix-sdk-base/src/rooms/normal.rs | 60 ++++++++++--------- crates/matrix-sdk-base/src/sliding_sync.rs | 10 ++-- .../matrix-sdk-ui/src/notification_client.rs | 2 +- .../filters/fuzzy_match_room_name.rs | 2 +- .../filters/normalized_match_room_name.rs | 2 +- .../src/room_list_service/room.rs | 2 +- crates/matrix-sdk/src/sync.rs | 2 +- .../tests/integration/room/common.rs | 6 +- examples/oidc_cli/src/main.rs | 2 +- examples/persist_session/src/main.rs | 2 +- 13 files changed, 49 insertions(+), 47 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/room.rs b/bindings/matrix-sdk-ffi/src/room.rs index 2d3ae4c4cb1..a31d35062b7 100644 --- a/bindings/matrix-sdk-ffi/src/room.rs +++ b/bindings/matrix-sdk-ffi/src/room.rs @@ -82,7 +82,7 @@ impl Room { /// compute a room name based on the room's nature (DM or not) and number of /// members. pub fn display_name(&self) -> Option { - Some(self.inner.cached_computed_display_name()?.to_string()) + Some(self.inner.cached_display_name()?.to_string()) } /// The raw name as present in the room state event. diff --git a/bindings/matrix-sdk-ffi/src/room_info.rs b/bindings/matrix-sdk-ffi/src/room_info.rs index 64cbedbc347..6e379728956 100644 --- a/bindings/matrix-sdk-ffi/src/room_info.rs +++ b/bindings/matrix-sdk-ffi/src/room_info.rs @@ -69,7 +69,7 @@ impl RoomInfo { Ok(Self { id: room.room_id().to_string(), - display_name: room.cached_computed_display_name().map(|name| name.to_string()), + display_name: room.cached_display_name().map(|name| name.to_string()), raw_name: room.name(), topic: room.topic(), avatar_url: room.avatar_url().map(Into::into), diff --git a/crates/matrix-sdk-base/src/client.rs b/crates/matrix-sdk-base/src/client.rs index 7202afcc1e3..77b9b4f9b79 100644 --- a/crates/matrix-sdk-base/src/client.rs +++ b/crates/matrix-sdk-base/src/client.rs @@ -1660,7 +1660,7 @@ mod tests { let room = client.get_room(room_id).expect("Room not found"); assert_eq!(room.state(), RoomState::Invited); assert_eq!( - room.computed_display_name().await.expect("fetching display name failed"), + room.compute_display_name().await.expect("fetching display name failed"), DisplayName::Calculated("Kyra".to_owned()) ); } diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index c75c325f163..3b37d2d6772 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -103,12 +103,11 @@ pub struct Room { #[cfg(all(feature = "e2e-encryption", feature = "experimental-sliding-sync"))] pub latest_encrypted_events: Arc>>>, - /// Cached value of the last call to `computed_display_name()`, useful for - /// sync access. + /// Cached display name, useful for sync access. /// - /// Filled by calling `computed_display_name`. It's automatically filled at - /// start, when creating a room, or on every successful sync. - pub(crate) cached_computed_display_name: Arc>>, + /// Filled by calling [`Self::compute_display_name`]. It's automatically + /// filled at start when creating a room, or on every successful sync. + pub(crate) cached_display_name: Arc>>, } /// The room summary containing member counts and members that should be used to @@ -207,11 +206,11 @@ impl Room { Self::MAX_ENCRYPTED_EVENTS, ))), roominfo_update_sender, - cached_computed_display_name: Arc::new(SyncRwLock::new(None)), + cached_display_name: Arc::new(SyncRwLock::new(None)), }; - // Refill the computed_display_name cache. - let _ = room.computed_display_name().await; + // Refill the display name cache. + let _ = room.compute_display_name().await; room } @@ -491,12 +490,12 @@ impl Room { /// /// This is automatically recomputed on every successful sync, and the /// cached result can be retrieved in - /// [`Self::cached_computed_display_name`]. + /// [`Self::cached_display_name`]. /// /// [spec]: - pub async fn computed_display_name(&self) -> StoreResult { + pub async fn compute_display_name(&self) -> StoreResult { let update_cache = |new_val: DisplayName| { - *self.cached_computed_display_name.write().unwrap() = Some(new_val.clone()); + *self.cached_display_name.write().unwrap() = Some(new_val.clone()); new_val }; @@ -592,8 +591,11 @@ impl Room { } /// Returns the cached computed display name, if available. - pub fn cached_computed_display_name(&self) -> Option { - self.cached_computed_display_name.read().unwrap().clone() + /// + /// This cache is refilled every time we call + /// [`Self::compute_display_name`]. + pub fn cached_display_name(&self) -> Option { + self.cached_display_name.read().unwrap().clone() } /// Return the last event in this room, if one has been cached during @@ -1852,7 +1854,7 @@ mod tests { #[async_test] async fn test_display_name_for_joined_room_is_empty_if_no_info() { let (_, room) = make_room_test_helper(RoomState::Joined).await; - assert_eq!(room.computed_display_name().await.unwrap(), DisplayName::Empty); + assert_eq!(room.compute_display_name().await.unwrap(), DisplayName::Empty); } #[async_test] @@ -1861,7 +1863,7 @@ mod tests { room.inner .update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event())); assert_eq!( - room.computed_display_name().await.unwrap(), + room.compute_display_name().await.unwrap(), DisplayName::Aliased("test".to_owned()) ); } @@ -1872,13 +1874,13 @@ mod tests { room.inner .update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event())); assert_eq!( - room.computed_display_name().await.unwrap(), + room.compute_display_name().await.unwrap(), DisplayName::Aliased("test".to_owned()) ); room.inner.update(|info| info.base_info.name = Some(make_name_event())); // Display name wasn't cached when we asked for it above, and name overrides assert_eq!( - room.computed_display_name().await.unwrap(), + room.compute_display_name().await.unwrap(), DisplayName::Named("Test Room".to_owned()) ); } @@ -1886,7 +1888,7 @@ mod tests { #[async_test] async fn test_display_name_for_invited_room_is_empty_if_no_info() { let (_, room) = make_room_test_helper(RoomState::Invited).await; - assert_eq!(room.computed_display_name().await.unwrap(), DisplayName::Empty); + assert_eq!(room.compute_display_name().await.unwrap(), DisplayName::Empty); } #[async_test] @@ -1899,7 +1901,7 @@ mod tests { }); room.inner.update(|info| info.base_info.name = Some(room_name)); - assert_eq!(room.computed_display_name().await.unwrap(), DisplayName::Empty); + assert_eq!(room.compute_display_name().await.unwrap(), DisplayName::Empty); } #[async_test] @@ -1908,7 +1910,7 @@ mod tests { room.inner .update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event())); assert_eq!( - room.computed_display_name().await.unwrap(), + room.compute_display_name().await.unwrap(), DisplayName::Aliased("test".to_owned()) ); } @@ -1919,13 +1921,13 @@ mod tests { room.inner .update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event())); assert_eq!( - room.computed_display_name().await.unwrap(), + room.compute_display_name().await.unwrap(), DisplayName::Aliased("test".to_owned()) ); room.inner.update(|info| info.base_info.name = Some(make_name_event())); // Display name wasn't cached when we asked for it above, and name overrides assert_eq!( - room.computed_display_name().await.unwrap(), + room.compute_display_name().await.unwrap(), DisplayName::Named("Test Room".to_owned()) ); } @@ -1967,7 +1969,7 @@ mod tests { room.inner.update_if(|info| info.update_from_ruma_summary(&summary)); assert_eq!( - room.computed_display_name().await.unwrap(), + room.compute_display_name().await.unwrap(), DisplayName::Calculated("Matthew".to_owned()) ); } @@ -1989,7 +1991,7 @@ mod tests { store.save_changes(&changes).await.unwrap(); assert_eq!( - room.computed_display_name().await.unwrap(), + room.compute_display_name().await.unwrap(), DisplayName::Calculated("Matthew".to_owned()) ); } @@ -2019,7 +2021,7 @@ mod tests { room.inner.update_if(|info| info.update_from_ruma_summary(&summary)); assert_eq!( - room.computed_display_name().await.unwrap(), + room.compute_display_name().await.unwrap(), DisplayName::Calculated("Matthew".to_owned()) ); } @@ -2044,7 +2046,7 @@ mod tests { store.save_changes(&changes).await.unwrap(); assert_eq!( - room.computed_display_name().await.unwrap(), + room.compute_display_name().await.unwrap(), DisplayName::Calculated("Matthew".to_owned()) ); } @@ -2099,7 +2101,7 @@ mod tests { room.inner.update_if(|info| info.update_from_ruma_summary(&summary)); assert_eq!( - room.computed_display_name().await.unwrap(), + room.compute_display_name().await.unwrap(), DisplayName::Calculated("Bob, Carol, Denis, Erica, and 3 others".to_owned()) ); } @@ -2148,7 +2150,7 @@ mod tests { } assert_eq!( - room.computed_display_name().await.unwrap(), + room.compute_display_name().await.unwrap(), DisplayName::Calculated("Alice, Bob, Carol, Denis, Erica, and 2 others".to_owned()) ); } @@ -2178,7 +2180,7 @@ mod tests { room.inner.update_if(|info| info.update_from_ruma_summary(&summary)); assert_eq!( - room.computed_display_name().await.unwrap(), + room.compute_display_name().await.unwrap(), DisplayName::EmptyWas("Matthew".to_owned()) ); } diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 7ce1b94b620..b4eea2d8b44 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -835,7 +835,7 @@ mod tests { // No m.room.name event, no heroes, no members => considered an empty room! let client_room = client.get_room(room_id).expect("No room found"); assert!(client_room.name().is_none()); - assert_eq!(client_room.computed_display_name().await.unwrap().to_string(), "Empty Room"); + assert_eq!(client_room.compute_display_name().await.unwrap().to_string(), "Empty Room"); assert_eq!(client_room.state(), RoomState::Joined); // And it is added to the list of joined rooms only. @@ -863,7 +863,7 @@ mod tests { // The name is known. let client_room = client.get_room(room_id).expect("No room found"); assert_eq!(client_room.name().as_deref(), Some("The Name")); - assert_eq!(client_room.computed_display_name().await.unwrap().to_string(), "The Name"); + assert_eq!(client_room.compute_display_name().await.unwrap().to_string(), "The Name"); } #[async_test] @@ -888,7 +888,7 @@ mod tests { assert!(client_room.name().is_none()); // No m.room.name event, no heroes, no members => considered an empty room! - assert_eq!(client_room.computed_display_name().await.unwrap().to_string(), "Empty Room"); + assert_eq!(client_room.compute_display_name().await.unwrap().to_string(), "Empty Room"); assert_eq!(client_room.state(), RoomState::Invited); @@ -921,7 +921,7 @@ mod tests { // The name is known. let client_room = client.get_room(room_id).expect("No room found"); assert_eq!(client_room.name().as_deref(), Some("The Name")); - assert_eq!(client_room.computed_display_name().await.unwrap().to_string(), "The Name"); + assert_eq!(client_room.compute_display_name().await.unwrap().to_string(), "The Name"); } #[async_test] @@ -1329,7 +1329,7 @@ mod tests { // Then the room's name is NOT overridden by the server-computed display name. let client_room = client.get_room(room_id).expect("No room found"); - assert_eq!(client_room.computed_display_name().await.unwrap().to_string(), "myroom"); + assert_eq!(client_room.compute_display_name().await.unwrap().to_string(), "myroom"); assert!(client_room.name().is_none()); } diff --git a/crates/matrix-sdk-ui/src/notification_client.rs b/crates/matrix-sdk-ui/src/notification_client.rs index 6a110439c08..64f475106e4 100644 --- a/crates/matrix-sdk-ui/src/notification_client.rs +++ b/crates/matrix-sdk-ui/src/notification_client.rs @@ -668,7 +668,7 @@ impl NotificationItem { sender_display_name, sender_avatar_url, is_sender_name_ambiguous, - room_computed_display_name: room.computed_display_name().await?.to_string(), + room_computed_display_name: room.compute_display_name().await?.to_string(), room_avatar_url: room.avatar_url().map(|s| s.to_string()), room_canonical_alias: room.canonical_alias().map(|c| c.to_string()), is_direct_message_room: room.is_direct().await?, diff --git a/crates/matrix-sdk-ui/src/room_list_service/filters/fuzzy_match_room_name.rs b/crates/matrix-sdk-ui/src/room_list_service/filters/fuzzy_match_room_name.rs index 196f0bbc06e..bd127f9f5c9 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/filters/fuzzy_match_room_name.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/filters/fuzzy_match_room_name.rs @@ -53,7 +53,7 @@ pub fn new_filter(client: &Client, pattern: &str) -> impl Filter { move |room_list_entry| -> bool { let Some(room_id) = room_list_entry.as_room_id() else { return false }; let Some(room) = client.get_room(room_id) else { return false }; - let Some(room_name) = room.cached_computed_display_name() else { return false }; + let Some(room_name) = room.cached_display_name() else { return false }; searcher.matches(&room_name.to_string()) } diff --git a/crates/matrix-sdk-ui/src/room_list_service/filters/normalized_match_room_name.rs b/crates/matrix-sdk-ui/src/room_list_service/filters/normalized_match_room_name.rs index 05a3ec6a163..55c510b5564 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/filters/normalized_match_room_name.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/filters/normalized_match_room_name.rs @@ -53,7 +53,7 @@ pub fn new_filter(client: &Client, pattern: &str) -> impl Filter { move |room_list_entry| -> bool { let Some(room_id) = room_list_entry.as_room_id() else { return false }; let Some(room) = client.get_room(room_id) else { return false }; - let Some(room_name) = room.cached_computed_display_name() else { return false }; + let Some(room_name) = room.cached_display_name() else { return false }; searcher.matches(&room_name.to_string()) } 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 a2acec1075f..94b7159fea2 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/room.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/room.rs @@ -85,7 +85,7 @@ impl Room { /// Get a computed room name for the room. pub fn cached_display_name(&self) -> Option { - Some(self.inner.room.cached_computed_display_name()?.to_string()) + Some(self.inner.room.cached_display_name()?.to_string()) } /// Get the underlying [`matrix_sdk::Room`]. diff --git a/crates/matrix-sdk/src/sync.rs b/crates/matrix-sdk/src/sync.rs index 9ca4d760427..8c3ecec3473 100644 --- a/crates/matrix-sdk/src/sync.rs +++ b/crates/matrix-sdk/src/sync.rs @@ -163,7 +163,7 @@ impl Client { .chain(rooms.invite.keys()) .filter_map(|room_id| self.get_room(room_id)) { - let _ = room.computed_display_name().await; + let _ = room.compute_display_name().await; } } diff --git a/crates/matrix-sdk/tests/integration/room/common.rs b/crates/matrix-sdk/tests/integration/room/common.rs index a3728130736..c01c0b7a17e 100644 --- a/crates/matrix-sdk/tests/integration/room/common.rs +++ b/crates/matrix-sdk/tests/integration/room/common.rs @@ -58,7 +58,7 @@ async fn test_calculate_room_names_from_summary() { assert_eq!( DisplayName::Calculated("example2".to_owned()), - room.computed_display_name().await.unwrap() + room.compute_display_name().await.unwrap() ); } @@ -77,7 +77,7 @@ async fn test_room_names() { assert_eq!( DisplayName::Aliased("tutorial".to_owned()), - room.computed_display_name().await.unwrap() + room.compute_display_name().await.unwrap() ); mock_sync(&server, &*test_json::INVITE_SYNC, Some(sync_token.clone())).await; @@ -89,7 +89,7 @@ async fn test_room_names() { assert_eq!( DisplayName::Named("My Room Name".to_owned()), - invited_room.computed_display_name().await.unwrap() + invited_room.compute_display_name().await.unwrap() ); } diff --git a/examples/oidc_cli/src/main.rs b/examples/oidc_cli/src/main.rs index 20794444b1d..55d0d16c0c5 100644 --- a/examples/oidc_cli/src/main.rs +++ b/examples/oidc_cli/src/main.rs @@ -881,7 +881,7 @@ async fn on_room_message(event: OriginalSyncRoomMessageEvent, room: Room) { } let MessageType::Text(text_content) = &event.content.msgtype else { return }; - let room_name = match room.computed_display_name().await { + let room_name = match room.compute_display_name().await { Ok(room_name) => room_name.to_string(), Err(error) => { println!("Error getting room display name: {error}"); diff --git a/examples/persist_session/src/main.rs b/examples/persist_session/src/main.rs index 6b880f36502..3ce375dd8b2 100644 --- a/examples/persist_session/src/main.rs +++ b/examples/persist_session/src/main.rs @@ -297,7 +297,7 @@ async fn on_room_message(event: OriginalSyncRoomMessageEvent, room: Room) { } let MessageType::Text(text_content) = &event.content.msgtype else { return }; - let room_name = match room.computed_display_name().await { + let room_name = match room.compute_display_name().await { Ok(room_name) => room_name.to_string(), Err(error) => { println!("Error getting room display name: {error}"); From cd65997c568693d40cb97cfbf76fb3056dfa1f76 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Thu, 6 Jun 2024 14:45:53 +0200 Subject: [PATCH 12/14] ci: fix flakeyness of `test_room_notification_count` in an innovative way The test looks at updates of `RoomInfo`s, but these depends on external factors like the server sending them in one part or multiple ones. Instead of trying to figure out which partial updates the server sent, wait for the stream of `RoomInfo`s to stabilize by trying to get the latest item from the stream, then wait up to N seconds for another item to show up, and continue as long as items come in. This should allow us to get rid of some code that was required to prevent flakey updates. --- .../src/tests/sliding_sync/room.rs | 30 +++++-------------- 1 file changed, 8 insertions(+), 22 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 d5d537d0ef1..5886a0db184 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 @@ -4,7 +4,6 @@ use std::{ }; use anyhow::Result; -use assert_matches2::assert_let; use eyeball_im::VectorDiff; use futures_util::{pin_mut, FutureExt, StreamExt as _}; use matrix_sdk::{ @@ -355,9 +354,12 @@ impl UpdateObserver { /// /// Returns `None` when the diff was empty, aka it was a spurious update. async fn next(&mut self) -> Option { - assert_let!( - Ok(Some(update)) = timeout(Duration::from_secs(3), self.subscriber.next()).await - ); + // Wait for the room info updates to stabilize. + let mut update = None; + while let Ok(Some(up)) = timeout(Duration::from_secs(2), self.subscriber.next()).await { + update = Some(up); + } + let update = update.expect("there should have been an update"); let update_json = serde_json::to_value(&update).unwrap(); let update_diff = json_structural_diff::JsonDiff::diff_string( @@ -570,12 +572,7 @@ async fn test_room_notification_count() -> Result<()> { { debug!("Remote echo of marking the room as read"); - let update = update_observer.next().await; - - if update.is_none() { - debug!("Previous update was spurious, actual update now"); - update_observer.next().await.expect("there should be a non-empty update at some point"); - } + update_observer.next().await; assert!(!alice_room.are_members_synced()); @@ -600,7 +597,7 @@ async fn test_room_notification_count() -> Result<()> { alice_room.send(RoomMessageEventContent::text_plain("hello bob")).await?; { - debug!("Room members got synced."); + debug!("Room members got synced + remote echo for hello bob."); update_observer.next().await.expect("syncing room members should update room info"); assert!(alice_room.are_members_synced()); @@ -612,17 +609,6 @@ async fn test_room_notification_count() -> Result<()> { update_observer.assert_is_pending(); } - { - debug!("Remote echo for hello bob"); - update_observer.next().await.expect("we should receive a remote echo for our own message"); - - assert_eq!(alice_room.num_unread_messages(), 0); - assert_eq!(alice_room.num_unread_notifications(), 0); - assert_eq!(alice_room.num_unread_mentions(), 0); - - update_observer.assert_is_pending(); - } - // Now Alice is only interesting in mentions of their name. let settings = alice.notification_settings().await; From e96d932f007efbb2719ff97f1e26c52ad00f1b95 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Mon, 10 Jun 2024 15:23:06 +0200 Subject: [PATCH 13/14] base: don't regenerate the display name when creating a room, store the cached display name in RoomInfo So revert a few changes to make some functions async, etc. --- benchmarks/benches/room_bench.rs | 2 +- crates/matrix-sdk-base/src/client.rs | 64 +++++----- crates/matrix-sdk-base/src/rooms/normal.rs | 120 ++++++++++-------- crates/matrix-sdk-base/src/sliding_sync.rs | 44 +++---- .../src/store/migration_helpers.rs | 1 + crates/matrix-sdk-base/src/store/mod.rs | 32 ++--- crates/matrix-sdk/src/client/mod.rs | 3 +- crates/matrix-sdk/src/event_cache/mod.rs | 2 +- .../matrix-sdk/src/event_cache/pagination.rs | 6 +- crates/matrix-sdk/src/sliding_sync/mod.rs | 4 +- 10 files changed, 142 insertions(+), 136 deletions(-) diff --git a/benchmarks/benches/room_bench.rs b/benchmarks/benches/room_bench.rs index 73e173d5d26..fae41ef7536 100644 --- a/benchmarks/benches/room_bench.rs +++ b/benchmarks/benches/room_bench.rs @@ -74,7 +74,7 @@ pub fn receive_all_members_benchmark(c: &mut Criterion) { )) .expect("Could not set session meta"); - runtime.block_on(base_client.get_or_create_room(&room_id, RoomState::Joined)); + base_client.get_or_create_room(&room_id, RoomState::Joined); let request = get_member_events::v3::Request::new(room_id.clone()); let response = get_member_events::v3::Response::new(member_events); diff --git a/crates/matrix-sdk-base/src/client.rs b/crates/matrix-sdk-base/src/client.rs index 77b9b4f9b79..33f5d215f03 100644 --- a/crates/matrix-sdk-base/src/client.rs +++ b/crates/matrix-sdk-base/src/client.rs @@ -171,10 +171,8 @@ impl BaseClient { /// Lookup the Room for the given RoomId, or create one, if it didn't exist /// yet in the store - pub async fn get_or_create_room(&self, room_id: &RoomId, room_state: RoomState) -> Room { - self.store - .get_or_create_room(room_id, room_state, self.roominfo_update_sender.clone()) - .await + pub fn get_or_create_room(&self, room_id: &RoomId, room_state: RoomState) -> Room { + self.store.get_or_create_room(room_id, room_state, self.roominfo_update_sender.clone()) } /// Get all the rooms this client knows about. @@ -748,10 +746,12 @@ impl BaseClient { /// /// Update the internal and cached state accordingly. Return the final Room. pub async fn room_joined(&self, room_id: &RoomId) -> Result { - let room = self - .store - .get_or_create_room(room_id, RoomState::Joined, self.roominfo_update_sender.clone()) - .await; + let room = self.store.get_or_create_room( + room_id, + RoomState::Joined, + self.roominfo_update_sender.clone(), + ); + if room.state() != RoomState::Joined { let _sync_lock = self.sync_lock().lock().await; @@ -773,10 +773,12 @@ impl BaseClient { /// /// Update the internal and cached state accordingly. pub async fn room_left(&self, room_id: &RoomId) -> Result<()> { - let room = self - .store - .get_or_create_room(room_id, RoomState::Left, self.roominfo_update_sender.clone()) - .await; + let room = self.store.get_or_create_room( + room_id, + RoomState::Left, + self.roominfo_update_sender.clone(), + ); + if room.state() != RoomState::Left { let _sync_lock = self.sync_lock().lock().await; @@ -847,14 +849,12 @@ impl BaseClient { let mut notifications = Default::default(); for (room_id, new_info) in response.rooms.join { - let room = self - .store - .get_or_create_room( - &room_id, - RoomState::Joined, - self.roominfo_update_sender.clone(), - ) - .await; + let room = self.store.get_or_create_room( + &room_id, + RoomState::Joined, + self.roominfo_update_sender.clone(), + ); + let mut room_info = room.clone_info(); room_info.mark_as_joined(); @@ -962,10 +962,12 @@ impl BaseClient { } for (room_id, new_info) in response.rooms.leave { - let room = self - .store - .get_or_create_room(&room_id, RoomState::Left, self.roominfo_update_sender.clone()) - .await; + let room = self.store.get_or_create_room( + &room_id, + RoomState::Left, + self.roominfo_update_sender.clone(), + ); + let mut room_info = room.clone_info(); room_info.mark_as_left(); room_info.mark_state_partially_synced(); @@ -1019,14 +1021,12 @@ impl BaseClient { } for (room_id, new_info) in response.rooms.invite { - let room = self - .store - .get_or_create_room( - &room_id, - RoomState::Invited, - self.roominfo_update_sender.clone(), - ) - .await; + let room = self.store.get_or_create_room( + &room_id, + RoomState::Invited, + self.roominfo_update_sender.clone(), + ); + let mut room_info = room.clone_info(); room_info.mark_as_invited(); room_info.mark_state_fully_synced(); diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index 3b37d2d6772..0c0b5d996ca 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -12,10 +12,12 @@ // See the License for the specific language governing permissions and // limitations under the License. +#[cfg(all(feature = "e2e-encryption", feature = "experimental-sliding-sync"))] +use std::sync::RwLock as SyncRwLock; use std::{ collections::{BTreeMap, HashSet}, mem, - sync::{atomic::AtomicBool, Arc, RwLock as SyncRwLock}, + sync::{atomic::AtomicBool, Arc}, }; use bitflags::bitflags; @@ -102,12 +104,6 @@ pub struct Room { /// to disk but held in memory. #[cfg(all(feature = "e2e-encryption", feature = "experimental-sliding-sync"))] pub latest_encrypted_events: Arc>>>, - - /// Cached display name, useful for sync access. - /// - /// Filled by calling [`Self::compute_display_name`]. It's automatically - /// filled at start when creating a room, or on every successful sync. - pub(crate) cached_display_name: Arc>>, } /// The room summary containing member counts and members that should be used to @@ -179,7 +175,7 @@ impl Room { const MAX_ENCRYPTED_EVENTS: std::num::NonZeroUsize = unsafe { std::num::NonZeroUsize::new_unchecked(10) }; - pub(crate) async fn new( + pub(crate) fn new( own_user_id: &UserId, store: Arc, room_id: &RoomId, @@ -187,16 +183,16 @@ impl Room { roominfo_update_sender: broadcast::Sender, ) -> Self { let room_info = RoomInfo::new(room_id, room_state); - Self::restore(own_user_id, store, room_info, roominfo_update_sender).await + Self::restore(own_user_id, store, room_info, roominfo_update_sender) } - pub(crate) async fn restore( + pub(crate) fn restore( own_user_id: &UserId, store: Arc, room_info: RoomInfo, roominfo_update_sender: broadcast::Sender, ) -> Self { - let room = Self { + Self { own_user_id: own_user_id.into(), room_id: room_info.room_id.clone(), store, @@ -206,13 +202,7 @@ impl Room { Self::MAX_ENCRYPTED_EVENTS, ))), roominfo_update_sender, - cached_display_name: Arc::new(SyncRwLock::new(None)), - }; - - // Refill the display name cache. - let _ = room.compute_display_name().await; - - room + } } /// Get the unique room id of the room. @@ -495,7 +485,14 @@ impl Room { /// [spec]: pub async fn compute_display_name(&self) -> StoreResult { let update_cache = |new_val: DisplayName| { - *self.cached_display_name.write().unwrap() = Some(new_val.clone()); + self.inner.update_if(|info| { + if info.cached_display_name.as_ref() != Some(&new_val) { + info.cached_display_name = Some(new_val.clone()); + true + } else { + false + } + }); new_val }; @@ -503,11 +500,15 @@ impl Room { let inner = self.inner.read(); if let Some(name) = inner.name() { - return Ok(update_cache(DisplayName::Named(name.trim().to_owned()))); + let name = name.trim().to_owned(); + drop(inner); // drop the lock on `self.inner` to avoid deadlocking in `update_cache`. + return Ok(update_cache(DisplayName::Named(name))); } if let Some(alias) = inner.canonical_alias() { - return Ok(update_cache(DisplayName::Aliased(alias.alias().trim().to_owned()))); + let alias = alias.alias().trim().to_owned(); + drop(inner); // See above comment. + return Ok(update_cache(DisplayName::Aliased(alias))); } inner.summary.clone() @@ -595,7 +596,7 @@ impl Room { /// This cache is refilled every time we call /// [`Self::compute_display_name`]. pub fn cached_display_name(&self) -> Option { - self.cached_display_name.read().unwrap().clone() + self.inner.read().cached_display_name.clone() } /// Return the last event in this room, if one has been cached during @@ -918,6 +919,13 @@ pub struct RoomInfo { /// spamming about unknown room versions in the log for the same room. #[serde(skip)] pub(crate) warned_about_unknown_room_version: Arc, + + /// Cached display name, useful for sync access. + /// + /// Filled by calling [`Self::compute_display_name`]. It's automatically + /// filled at start when creating a room, or on every successful sync. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub(crate) cached_display_name: Option, } #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] @@ -954,6 +962,7 @@ impl RoomInfo { read_receipts: Default::default(), base_info: Box::new(BaseRoomInfo::new()), warned_about_unknown_room_version: Arc::new(false.into()), + cached_display_name: None, } } @@ -1541,6 +1550,7 @@ mod tests { base_info: Box::new(BaseRoomInfo::new()), read_receipts: Default::default(), warned_about_unknown_room_version: Arc::new(false.into()), + cached_display_name: None, }; let info_json = json!({ @@ -1635,6 +1645,7 @@ mod tests { "tombstone": null, "topic": null, }, + "cached_display_name": { "Calculated": "lol" } }); let info: RoomInfo = serde_json::from_value(info_json).unwrap(); @@ -1664,6 +1675,11 @@ mod tests { assert!(info.base_info.name.is_none()); assert!(info.base_info.tombstone.is_none()); assert!(info.base_info.topic.is_none()); + + assert_eq!( + info.cached_display_name.as_ref(), + Some(&DisplayName::Calculated("lol".to_owned())) + ) } #[async_test] @@ -1684,7 +1700,7 @@ mod tests { .unwrap(); let room_id = room_id!("!test:localhost"); - let room = client.get_or_create_room(room_id, RoomState::Joined).await; + let room = client.get_or_create_room(room_id, RoomState::Joined); // Sanity checks to ensure the room isn't marked as favourite. assert!(room.is_favourite().not()); @@ -1758,7 +1774,7 @@ mod tests { .unwrap(); let room_id = room_id!("!test:localhost"); - let room = client.get_or_create_room(room_id, RoomState::Joined).await; + let room = client.get_or_create_room(room_id, RoomState::Joined); // Sanity checks to ensure the room isn't marked as low priority. assert!(!room.is_low_priority()); @@ -1814,13 +1830,13 @@ mod tests { assert!(room.is_low_priority().not()); } - async fn make_room_test_helper(room_type: RoomState) -> (Arc, Room) { + fn make_room_test_helper(room_type: RoomState) -> (Arc, Room) { let store = Arc::new(MemoryStore::new()); let user_id = user_id!("@me:example.org"); let room_id = room_id!("!test:localhost"); let (sender, _receiver) = tokio::sync::broadcast::channel(1); - (store.clone(), Room::new(user_id, store, room_id, room_type, sender).await) + (store.clone(), Room::new(user_id, store, room_id, room_type, sender)) } fn make_stripped_member_event(user_id: &UserId, name: &str) -> Raw { @@ -1853,13 +1869,13 @@ mod tests { #[async_test] async fn test_display_name_for_joined_room_is_empty_if_no_info() { - let (_, room) = make_room_test_helper(RoomState::Joined).await; + let (_, room) = make_room_test_helper(RoomState::Joined); assert_eq!(room.compute_display_name().await.unwrap(), DisplayName::Empty); } #[async_test] async fn test_display_name_for_joined_room_uses_canonical_alias_if_available() { - let (_, room) = make_room_test_helper(RoomState::Joined).await; + let (_, room) = make_room_test_helper(RoomState::Joined); room.inner .update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event())); assert_eq!( @@ -1870,7 +1886,7 @@ mod tests { #[async_test] async fn test_display_name_for_joined_room_prefers_name_over_alias() { - let (_, room) = make_room_test_helper(RoomState::Joined).await; + let (_, room) = make_room_test_helper(RoomState::Joined); room.inner .update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event())); assert_eq!( @@ -1887,13 +1903,13 @@ mod tests { #[async_test] async fn test_display_name_for_invited_room_is_empty_if_no_info() { - let (_, room) = make_room_test_helper(RoomState::Invited).await; + let (_, room) = make_room_test_helper(RoomState::Invited); assert_eq!(room.compute_display_name().await.unwrap(), DisplayName::Empty); } #[async_test] async fn test_display_name_for_invited_room_is_empty_if_room_name_empty() { - let (_, room) = make_room_test_helper(RoomState::Invited).await; + let (_, room) = make_room_test_helper(RoomState::Invited); let room_name = MinimalStateEvent::Original(OriginalMinimalStateEvent { content: RoomNameEventContent::new(String::new()), @@ -1906,7 +1922,7 @@ mod tests { #[async_test] async fn test_display_name_for_invited_room_uses_canonical_alias_if_available() { - let (_, room) = make_room_test_helper(RoomState::Invited).await; + let (_, room) = make_room_test_helper(RoomState::Invited); room.inner .update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event())); assert_eq!( @@ -1917,7 +1933,7 @@ mod tests { #[async_test] async fn test_display_name_for_invited_room_prefers_name_over_alias() { - let (_, room) = make_room_test_helper(RoomState::Invited).await; + let (_, room) = make_room_test_helper(RoomState::Invited); room.inner .update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event())); assert_eq!( @@ -1950,7 +1966,7 @@ mod tests { #[async_test] async fn test_display_name_dm_invited() { - let (store, room) = make_room_test_helper(RoomState::Invited).await; + let (store, room) = make_room_test_helper(RoomState::Invited); let room_id = room_id!("!test:localhost"); let matthew = user_id!("@matthew:example.org"); let me = user_id!("@me:example.org"); @@ -1976,7 +1992,7 @@ mod tests { #[async_test] async fn test_display_name_dm_invited_no_heroes() { - let (store, room) = make_room_test_helper(RoomState::Invited).await; + let (store, room) = make_room_test_helper(RoomState::Invited); let room_id = room_id!("!test:localhost"); let matthew = user_id!("@matthew:example.org"); let me = user_id!("@me:example.org"); @@ -1998,7 +2014,7 @@ mod tests { #[async_test] async fn test_display_name_dm_joined() { - let (store, room) = make_room_test_helper(RoomState::Joined).await; + let (store, room) = make_room_test_helper(RoomState::Joined); let room_id = room_id!("!test:localhost"); let matthew = user_id!("@matthew:example.org"); let me = user_id!("@me:example.org"); @@ -2028,7 +2044,7 @@ mod tests { #[async_test] async fn test_display_name_dm_joined_no_heroes() { - let (store, room) = make_room_test_helper(RoomState::Joined).await; + let (store, room) = make_room_test_helper(RoomState::Joined); let room_id = room_id!("!test:localhost"); let matthew = user_id!("@matthew:example.org"); let me = user_id!("@me:example.org"); @@ -2053,7 +2069,7 @@ mod tests { #[async_test] async fn test_display_name_deterministic() { - let (store, room) = make_room_test_helper(RoomState::Joined).await; + let (store, room) = make_room_test_helper(RoomState::Joined); let alice = user_id!("@alice:example.org"); let bob = user_id!("@bob:example.org"); @@ -2108,7 +2124,7 @@ mod tests { #[async_test] async fn test_display_name_deterministic_no_heroes() { - let (store, room) = make_room_test_helper(RoomState::Joined).await; + let (store, room) = make_room_test_helper(RoomState::Joined); let alice = user_id!("@alice:example.org"); let bob = user_id!("@bob:example.org"); @@ -2157,7 +2173,7 @@ mod tests { #[async_test] async fn test_display_name_dm_alone() { - let (store, room) = make_room_test_helper(RoomState::Joined).await; + let (store, room) = make_room_test_helper(RoomState::Joined); let room_id = room_id!("!test:localhost"); let matthew = user_id!("@matthew:example.org"); let me = user_id!("@me:example.org"); @@ -2205,7 +2221,7 @@ mod tests { .unwrap(); let room_id = room_id!("!test:localhost"); - let room = client.get_or_create_room(room_id, RoomState::Joined).await; + let room = client.get_or_create_room(room_id, RoomState::Joined); // That has an encrypted event, add_encrypted_event(&room, "$A"); @@ -2237,7 +2253,7 @@ mod tests { #[cfg(feature = "experimental-sliding-sync")] async fn test_when_we_provide_a_newly_decrypted_event_it_replaces_latest_event() { // Given a room with an encrypted event - let (_store, room) = make_room_test_helper(RoomState::Joined).await; + let (_store, room) = make_room_test_helper(RoomState::Joined); add_encrypted_event(&room, "$A"); // Sanity: it has no latest_event assert!(room.latest_event().is_none()); @@ -2256,7 +2272,7 @@ mod tests { #[cfg(feature = "experimental-sliding-sync")] async fn test_when_a_newly_decrypted_event_appears_we_delete_all_older_encrypted_events() { // Given a room with some encrypted events and a latest event - let (_store, room) = make_room_test_helper(RoomState::Joined).await; + let (_store, room) = make_room_test_helper(RoomState::Joined); room.inner.update(|info| info.latest_event = Some(make_latest_event("$A"))); add_encrypted_event(&room, "$0"); add_encrypted_event(&room, "$1"); @@ -2284,7 +2300,7 @@ mod tests { #[cfg(feature = "experimental-sliding-sync")] async fn test_replacing_the_newest_event_leaves_none_left() { // Given a room with some encrypted events - let (_store, room) = make_room_test_helper(RoomState::Joined).await; + let (_store, room) = make_room_test_helper(RoomState::Joined); add_encrypted_event(&room, "$0"); add_encrypted_event(&room, "$1"); add_encrypted_event(&room, "$2"); @@ -2382,8 +2398,8 @@ mod tests { /// `user_a`: empty memberships /// `user_b`: one membership /// `user_c`: two memberships (two devices) - async fn create_call_with_member_events_for_user(a: &UserId, b: &UserId, c: &UserId) -> Room { - let (_, room) = make_room_test_helper(RoomState::Joined).await; + fn create_call_with_member_events_for_user(a: &UserId, b: &UserId, c: &UserId) -> Room { + let (_, room) = make_room_test_helper(RoomState::Joined); let a_empty = call_member_state_event(Vec::new(), "$1234", a); @@ -2403,9 +2419,9 @@ mod tests { room } - #[async_test] - async fn test_show_correct_active_call_state() { - let room = create_call_with_member_events_for_user(&ALICE, &BOB, &CAROL).await; + #[test] + fn test_show_correct_active_call_state() { + let room = create_call_with_member_events_for_user(&ALICE, &BOB, &CAROL); // This check also tests the ordering. // We want older events to be in the front. @@ -2417,9 +2433,9 @@ mod tests { assert!(room.has_active_room_call()); } - #[async_test] - async fn test_active_call_is_false_when_everyone_left() { - let room = create_call_with_member_events_for_user(&ALICE, &BOB, &CAROL).await; + #[test] + fn test_active_call_is_false_when_everyone_left() { + let room = create_call_with_member_events_for_user(&ALICE, &BOB, &CAROL); let b_empty_membership = call_member_state_event(Vec::new(), "$1234_1", &BOB); let c_empty_membership = call_member_state_event(Vec::new(), "$12345_1", &CAROL); diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index b4eea2d8b44..6263a9459c2 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -331,9 +331,8 @@ impl BaseClient { // Find or create the room in the store #[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) - .await; + let (mut room, mut room_info, invited_room) = + self.process_sliding_sync_room_membership(room_data, &state_events, store, room_id); room_info.mark_state_partially_synced(); @@ -453,7 +452,7 @@ impl BaseClient { /// If any invite_state exists, we take it to mean that we are invited to /// this room, unless that state contains membership events that specify /// otherwise. https://github.com/matrix-org/matrix-spec-proposals/blob/kegan/sync-v3/proposals/3575-sync.md#room-list-parameters - async fn process_sliding_sync_room_membership( + fn process_sliding_sync_room_membership( &self, room_data: &v4::SlidingSyncRoom, state_events: &[AnySyncStateEvent], @@ -461,13 +460,11 @@ impl BaseClient { room_id: &RoomId, ) -> (Room, RoomInfo, Option) { if let Some(invite_state) = &room_data.invite_state { - let room = store - .get_or_create_room( - room_id, - RoomState::Invited, - self.roominfo_update_sender.clone(), - ) - .await; + let room = store.get_or_create_room( + room_id, + RoomState::Invited, + self.roominfo_update_sender.clone(), + ); let mut room_info = room.clone_info(); // We don't actually know what events are inside invite_state. In theory, they @@ -485,9 +482,11 @@ impl BaseClient { (room, room_info, Some(InvitedRoom::from(v3::InviteState::from(invite_state.clone())))) } else { - let room = store - .get_or_create_room(room_id, RoomState::Joined, self.roominfo_update_sender.clone()) - .await; + let room = store.get_or_create_room( + room_id, + RoomState::Joined, + self.roominfo_update_sender.clone(), + ); let mut room_info = room.clone_info(); // We default to considering this room joined if it's not an invite. If it's @@ -1510,7 +1509,7 @@ mod tests { let events = &[event1, event2.clone(), event3.clone(), event4.clone()]; // When I ask to cache events - let room = make_room().await; + let room = make_room(); let mut room_info = room.clone_info(); cache_latest_events(&room, &mut room_info, events, None, None).await; @@ -1539,7 +1538,7 @@ mod tests { let events = &[event1, event2.clone(), event3.clone()]; // When I ask to cache events - let room = make_room().await; + let room = make_room(); let mut room_info = room.clone_info(); cache_latest_events(&room, &mut room_info, events, None, None).await; room.set_room_info(room_info, false); @@ -1566,7 +1565,7 @@ mod tests { let events = &[event1, event2.clone(), event3.clone(), event4, event5.clone()]; // When I ask to cache events - let room = make_room().await; + let room = make_room(); let mut room_info = room.clone_info(); cache_latest_events(&room, &mut room_info, events, None, None).await; room.set_room_info(room_info, false); @@ -1619,7 +1618,7 @@ mod tests { ]; // When I ask to cache events - let room = make_room().await; + let room = make_room(); let mut room_info = room.clone_info(); cache_latest_events(&room, &mut room_info, events, None, None).await; room.set_room_info(room_info, false); @@ -1642,7 +1641,7 @@ mod tests { #[async_test] async fn test_dont_overflow_capacity_if_previous_encrypted_events_exist() { // Given a RoomInfo with lots of encrypted events already inside it - let room = make_room().await; + let room = make_room(); let mut room_info = room.clone_info(); cache_latest_events( &room, @@ -1684,7 +1683,7 @@ mod tests { #[async_test] async fn test_existing_encrypted_events_are_deleted_if_we_receive_unencrypted() { // Given a RoomInfo with some encrypted events already inside it - let room = make_room().await; + let room = make_room(); let mut room_info = room.clone_info(); cache_latest_events( &room, @@ -1710,7 +1709,7 @@ mod tests { } async fn choose_event_to_cache(events: &[SyncTimelineEvent]) -> Option { - let room = make_room().await; + let room = make_room(); let mut room_info = room.clone_info(); cache_latest_events(&room, &mut room_info, events, None, None).await; room.set_room_info(room_info, false); @@ -1733,7 +1732,7 @@ mod tests { events.iter().map(|e| e.event_id().unwrap().to_string()).collect() } - async fn make_room() -> Room { + fn make_room() -> Room { let (sender, _receiver) = tokio::sync::broadcast::channel(1); Room::new( @@ -1743,7 +1742,6 @@ mod tests { RoomState::Joined, sender, ) - .await } fn make_event(typ: &str, id: &str) -> SyncTimelineEvent { diff --git a/crates/matrix-sdk-base/src/store/migration_helpers.rs b/crates/matrix-sdk-base/src/store/migration_helpers.rs index 79c991f8828..9621afce82d 100644 --- a/crates/matrix-sdk-base/src/store/migration_helpers.rs +++ b/crates/matrix-sdk-base/src/store/migration_helpers.rs @@ -124,6 +124,7 @@ impl RoomInfoV1 { read_receipts: Default::default(), base_info: base_info.migrate(create), warned_about_unknown_room_version: Arc::new(false.into()), + cached_display_name: None, } } } diff --git a/crates/matrix-sdk-base/src/store/mod.rs b/crates/matrix-sdk-base/src/store/mod.rs index 90a533ba833..81e2eef8a3d 100644 --- a/crates/matrix-sdk-base/src/store/mod.rs +++ b/crates/matrix-sdk-base/src/store/mod.rs @@ -182,8 +182,7 @@ impl Store { self.inner.clone(), info, roominfo_update_sender.clone(), - ) - .await; + ); self.rooms.write().unwrap().insert(room.room_id().to_owned(), room); } @@ -225,30 +224,23 @@ impl Store { /// Lookup the Room for the given RoomId, or create one, if it didn't exist /// yet in the store. - pub async fn get_or_create_room( + pub fn get_or_create_room( &self, room_id: &RoomId, room_type: RoomState, roominfo_update_sender: broadcast::Sender, ) -> Room { - let user_id = self - .session_meta - .get() - .expect("Creating room while not being logged in") - .user_id - .clone(); - - if let Some(room) = self.rooms.read().unwrap().get(room_id).cloned() { - return room; - } - - let room = - Room::new(&user_id, self.inner.clone(), room_id, room_type, roominfo_update_sender) - .await; + let user_id = + &self.session_meta.get().expect("Creating room while not being logged in").user_id; - // Use entry() because another caller could have racily written the room in - // the meanwhile. - self.rooms.write().unwrap().entry(room_id.to_owned()).or_insert(room).clone() + self.rooms + .write() + .unwrap() + .entry(room_id.to_owned()) + .or_insert_with(|| { + Room::new(user_id, self.inner.clone(), room_id, room_type, roominfo_update_sender) + }) + .clone() } } diff --git a/crates/matrix-sdk/src/client/mod.rs b/crates/matrix-sdk/src/client/mod.rs index 09b3a6af36c..652162c201c 100644 --- a/crates/matrix-sdk/src/client/mod.rs +++ b/crates/matrix-sdk/src/client/mod.rs @@ -1244,8 +1244,7 @@ impl Client { let is_direct_room = request.is_direct; let response = self.send(request, None).await?; - let base_room = - self.base_client().get_or_create_room(&response.room_id, RoomState::Joined).await; + let base_room = self.base_client().get_or_create_room(&response.room_id, RoomState::Joined); let joined_room = Room::new(self.clone(), base_room); diff --git a/crates/matrix-sdk/src/event_cache/mod.rs b/crates/matrix-sdk/src/event_cache/mod.rs index f0482b0ef6e..47e032a90b6 100644 --- a/crates/matrix-sdk/src/event_cache/mod.rs +++ b/crates/matrix-sdk/src/event_cache/mod.rs @@ -787,7 +787,7 @@ mod tests { async fn test_uniq_read_marker() { let client = logged_in_client(None).await; let room_id = room_id!("!galette:saucisse.bzh"); - client.base_client().get_or_create_room(room_id, matrix_sdk_base::RoomState::Joined).await; + client.base_client().get_or_create_room(room_id, matrix_sdk_base::RoomState::Joined); let event_cache = client.event_cache(); diff --git a/crates/matrix-sdk/src/event_cache/pagination.rs b/crates/matrix-sdk/src/event_cache/pagination.rs index 0ba7fd89f6c..a24af594094 100644 --- a/crates/matrix-sdk/src/event_cache/pagination.rs +++ b/crates/matrix-sdk/src/event_cache/pagination.rs @@ -345,7 +345,7 @@ mod tests { async fn test_wait_no_pagination_token() { let client = logged_in_client(None).await; let room_id = room_id!("!galette:saucisse.bzh"); - client.base_client().get_or_create_room(room_id, RoomState::Joined).await; + client.base_client().get_or_create_room(room_id, RoomState::Joined); let event_cache = client.event_cache(); @@ -398,7 +398,7 @@ mod tests { async fn test_wait_for_pagination_token_already_present() { let client = logged_in_client(None).await; let room_id = room_id!("!galette:saucisse.bzh"); - client.base_client().get_or_create_room(room_id, RoomState::Joined).await; + client.base_client().get_or_create_room(room_id, RoomState::Joined); let event_cache = client.event_cache(); @@ -453,7 +453,7 @@ mod tests { async fn test_wait_for_late_pagination_token() { let client = logged_in_client(None).await; let room_id = room_id!("!galette:saucisse.bzh"); - client.base_client().get_or_create_room(room_id, RoomState::Joined).await; + client.base_client().get_or_create_room(room_id, RoomState::Joined); let event_cache = client.event_cache(); diff --git a/crates/matrix-sdk/src/sliding_sync/mod.rs b/crates/matrix-sdk/src/sliding_sync/mod.rs index 7e9eba88fa5..dff893e3a71 100644 --- a/crates/matrix-sdk/src/sliding_sync/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/mod.rs @@ -2187,7 +2187,7 @@ mod tests { let server = MockServer::start().await; let client = logged_in_client(Some(server.uri())).await; - client.base_client().get_or_create_room(&room_id, RoomState::Joined).await; + client.base_client().get_or_create_room(&room_id, RoomState::Joined); // Setup sliding sync with with one room and one list @@ -2310,7 +2310,7 @@ mod tests { let server = MockServer::start().await; let client = logged_in_client(Some(server.uri())).await; - client.base_client().get_or_create_room(&room, RoomState::Joined).await; + client.base_client().get_or_create_room(&room, RoomState::Joined); let sliding_sync = client .sliding_sync("test")? From 5001b27cfb8d5ba94fd00ebfc8c622a2ed5fef1b Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Mon, 10 Jun 2024 16:32:17 +0200 Subject: [PATCH 14/14] base: move the initial filling of the display name cache into the sync methods It's not perfect, but it's honest work. --- crates/matrix-sdk-base/src/client.rs | 7 +++++++ crates/matrix-sdk-base/src/sliding_sync.rs | 7 +++++++ crates/matrix-sdk-base/src/sync.rs | 18 ++++++++++++++++++ crates/matrix-sdk/src/sync.rs | 13 ------------- 4 files changed, 32 insertions(+), 13 deletions(-) diff --git a/crates/matrix-sdk-base/src/client.rs b/crates/matrix-sdk-base/src/client.rs index 33f5d215f03..26adfb424b5 100644 --- a/crates/matrix-sdk-base/src/client.rs +++ b/crates/matrix-sdk-base/src/client.rs @@ -1071,6 +1071,13 @@ impl BaseClient { self.apply_changes(&changes, false); } + // Now that all the rooms information have been saved, update the display name + // cache (which relies on information stored in the database). This will + // live in memory, until the next sync which will saves the room info to + // disk; we do this to avoid saving that would be redundant with the + // above. Oh well. + new_rooms.update_in_memory_caches(&self.store).await; + info!("Processed a sync response in {:?}", now.elapsed()); let response = SyncResponse { diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 6263a9459c2..939d3cbddb3 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -295,6 +295,13 @@ impl BaseClient { self.apply_changes(&changes, false); trace!("applied changes"); + // Now that all the rooms information have been saved, update the display name + // cache (which relies on information stored in the database). This will + // live in memory, until the next sync which will saves the room info to + // disk; we do this to avoid saving that would be redundant with the + // above. Oh well. + new_rooms.update_in_memory_caches(&self.store).await; + Ok(SyncResponse { rooms: new_rooms, notifications, diff --git a/crates/matrix-sdk-base/src/sync.rs b/crates/matrix-sdk-base/src/sync.rs index 23f1bff59a4..50cc6210b19 100644 --- a/crates/matrix-sdk-base/src/sync.rs +++ b/crates/matrix-sdk-base/src/sync.rs @@ -35,6 +35,7 @@ use serde::{Deserialize, Serialize}; use crate::{ debug::{DebugInvitedRoom, DebugListOfRawEvents, DebugListOfRawEventsNoId}, deserialized_responses::{AmbiguityChange, RawAnySyncOrStrippedTimelineEvent}, + store::Store, }; /// Generalized representation of a `/sync` response. @@ -78,6 +79,23 @@ pub struct RoomUpdates { pub invite: BTreeMap, } +impl RoomUpdates { + /// Update the caches for the rooms that received updates. + /// + /// This will only fill the in-memory caches, not save the info on disk. + pub(crate) async fn update_in_memory_caches(&self, store: &Store) { + for room in self + .leave + .keys() + .chain(self.join.keys()) + .chain(self.invite.keys()) + .filter_map(|room_id| store.get_room(room_id)) + { + let _ = room.compute_display_name().await; + } + } +} + #[cfg(not(tarpaulin_include))] impl fmt::Debug for RoomUpdates { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { diff --git a/crates/matrix-sdk/src/sync.rs b/crates/matrix-sdk/src/sync.rs index 8c3ecec3473..1aa3792c2c3 100644 --- a/crates/matrix-sdk/src/sync.rs +++ b/crates/matrix-sdk/src/sync.rs @@ -154,19 +154,6 @@ impl Client { ) -> Result<()> { let BaseSyncResponse { rooms, presence, account_data, to_device, notifications } = response; - { - // Recompute the computed display name for all the rooms which had an update. - for room in rooms - .leave - .keys() - .chain(rooms.join.keys()) - .chain(rooms.invite.keys()) - .filter_map(|room_id| self.get_room(room_id)) - { - let _ = room.compute_display_name().await; - } - } - let now = Instant::now(); self.handle_sync_events(HandlerKind::GlobalAccountData, None, account_data).await?; self.handle_sync_events(HandlerKind::Presence, None, presence).await?;