From 56be9d546112428bab094fee0726643fa225d605 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Thu, 23 May 2024 19:33:01 +0200 Subject: [PATCH] sdk-base: store heroes as `OwnedUserId`s in our own `RoomSummary` This is the right goal, and Ruma will be updated to reflect that the `heroes` field should contain `OwnedUserId` too in [1]. This simplifies the code a bit, and avoids a round-trip encoding user-ids into a string then decoding them from a string later, in the case of sliding sync and room name computation. [1] https://github.com/ruma/ruma/pull/1822 --- crates/matrix-sdk-base/src/client.rs | 2 +- crates/matrix-sdk-base/src/rooms/normal.rs | 134 +++++++++++++-------- crates/matrix-sdk-base/src/sliding_sync.rs | 30 ++--- 3 files changed, 99 insertions(+), 67 deletions(-) diff --git a/crates/matrix-sdk-base/src/client.rs b/crates/matrix-sdk-base/src/client.rs index 73e063533..8ecc0ff4a 100644 --- a/crates/matrix-sdk-base/src/client.rs +++ b/crates/matrix-sdk-base/src/client.rs @@ -835,7 +835,7 @@ impl BaseClient { let mut room_info = room.clone_info(); room_info.mark_as_joined(); - room_info.update_summary(&new_info.summary); + room_info.update_from_ruma_summary(&new_info.summary); room_info.set_prev_batch(new_info.timeline.prev_batch.as_deref()); 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 2245b8056..124e922ba 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -113,17 +113,21 @@ pub struct Room { pub struct RoomSummary { /// The heroes of the room, members that should be used for the room display /// name. - heroes: Vec, + /// + /// This was called `heroes` and contained raw `String`s of the `UserId` + /// before; changing the field's name helped with avoiding a migration. + #[serde(default)] + pub(crate) heroes_user_ids: Vec, /// The number of members that are considered to be joined to the room. - joined_member_count: u64, + pub(crate) joined_member_count: u64, /// The number of members that are considered to be invited to the room. - invited_member_count: u64, + pub(crate) invited_member_count: u64, } #[cfg(test)] impl RoomSummary { - pub(crate) fn heroes(&self) -> &[String] { - &self.heroes + pub(crate) fn heroes(&self) -> &[OwnedUserId] { + &self.heroes_user_ids } } @@ -621,45 +625,46 @@ impl Room { let own_user_id = self.own_user_id().as_str(); - let (heroes, guessed_num_members): (Vec, _) = if summary.heroes.is_empty() { - let mut members = self.members(RoomMemberships::ACTIVE).await?; + let (heroes, guessed_num_members): (Vec, _) = + if summary.heroes_user_ids.is_empty() { + let mut members = self.members(RoomMemberships::ACTIVE).await?; - // Make the ordering deterministic. - members.sort_unstable_by(|lhs, rhs| lhs.name().cmp(rhs.name())); + // Make the ordering deterministic. + 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()); + // 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()); - ( - members - .into_iter() - .take(NUM_HEROES) - .filter(|u| u.user_id() != own_user_id) - .collect(), - guessed_num_members, - ) - } else { - let mut heroes = summary.heroes; + ( + members + .into_iter() + .take(NUM_HEROES) + .filter(|u| u.user_id() != own_user_id) + .collect(), + guessed_num_members, + ) + } else { + let mut heroes = summary.heroes_user_ids; - // Make the ordering deterministic. - heroes.sort_unstable(); + // Make the ordering deterministic. + heroes.sort_unstable(); - let members: Vec<_> = stream::iter(heroes.iter()) - .filter_map(|u| async move { - let user_id = UserId::parse(u.as_str()).ok()?; - if user_id == own_user_id { - return None; - } - self.get_member(&user_id).await.transpose() - }) - .collect() - .await; + let members: Vec<_> = stream::iter(heroes.iter()) + .filter_map(|user_id| async move { + // Filter out the current user. + if user_id == own_user_id { + return None; + } + self.get_member(&user_id).await.transpose() + }) + .collect() + .await; - let members: StoreResult> = members.into_iter().collect(); + let members: StoreResult> = members.into_iter().collect(); - (members?, None) - }; + (members?, None) + }; let (num_joined, num_invited) = match self.state() { RoomState::Invited => { @@ -1081,15 +1086,27 @@ impl RoomInfo { self.notification_counts = notification_counts; } - /// Update the RoomSummary. + /// Update the RoomSummary from a Ruma `RoomSummary`. /// - /// Returns true if the Summary modified the info, false otherwise. - pub fn update_summary(&mut self, summary: &RumaSummary) -> bool { + /// Returns true if any field has been updated, false otherwise. + pub fn update_from_ruma_summary(&mut self, summary: &RumaSummary) -> bool { let mut changed = false; if !summary.is_empty() { if !summary.heroes.is_empty() { - self.summary.heroes = summary.heroes.clone(); + // Parse the user IDs from Ruma. + self.summary.heroes_user_ids = summary + .heroes + .iter() + .filter_map(|hero| match UserId::parse(hero.as_str()) { + Ok(user_id) => Some(user_id), + Err(err) => { + warn!("error when parsing user id from hero '{hero}': {err}"); + None + } + }) + .collect(); + changed = true; } @@ -1107,6 +1124,21 @@ impl RoomInfo { changed } + /// Updates the joined member count. + pub(crate) fn update_joined_member_count(&mut self, count: u64) { + self.summary.joined_member_count = count; + } + + /// Updates the invited member count. + pub(crate) fn update_invited_member_count(&mut self, count: u64) { + self.summary.invited_member_count = count; + } + + /// Updates the heroes user ids. + pub(crate) fn update_heroes(&mut self, heroes: Vec) { + self.summary.heroes_user_ids = heroes; + } + /// The number of active members (invited + joined) in the room. /// /// The return value is saturated at `u64::MAX`. @@ -1425,6 +1457,8 @@ mod tests { // This test exists to make sure we don't accidentally change the // serialized format for `RoomInfo`. + use ruma::owned_user_id; + use super::RoomSummary; use crate::{rooms::BaseRoomInfo, sync::UnreadNotificationsCount}; @@ -1436,7 +1470,7 @@ mod tests { notification_count: 2, }, summary: RoomSummary { - heroes: vec!["Somebody".to_owned()], + heroes_user_ids: vec![owned_user_id!("@somebody:example.org")], joined_member_count: 5, invited_member_count: 0, }, @@ -1460,7 +1494,7 @@ mod tests { "notification_count": 2, }, "summary": { - "heroes": ["Somebody"], + "heroes_user_ids": ["@somebody:example.org"], "joined_member_count": 5, "invited_member_count": 0, }, @@ -1511,6 +1545,8 @@ mod tests { // The following JSON should never change if we want to be able to read in old // cached state + + use ruma::owned_user_id; let info_json = json!({ "room_id": "!gda78o:server.tld", "room_state": "Invited", @@ -1519,7 +1555,7 @@ mod tests { "notification_count": 2, }, "summary": { - "heroes": ["Somebody"], + "heroes_user_ids": ["@somebody:example.org"], "joined_member_count": 5, "invited_member_count": 0, }, @@ -1549,7 +1585,7 @@ mod tests { assert_eq!(info.room_state, RoomState::Invited); assert_eq!(info.notification_counts.highlight_count, 1); assert_eq!(info.notification_counts.notification_count, 2); - assert_eq!(info.summary.heroes, vec!["Somebody".to_owned()]); + assert_eq!(info.summary.heroes_user_ids, vec![owned_user_id!("@somebody:example.org")]); assert_eq!(info.summary.joined_member_count, 5); assert_eq!(info.summary.invited_member_count, 0); assert!(info.members_synced); @@ -1864,7 +1900,7 @@ mod tests { changes.add_stripped_member(room_id, me, make_stripped_member_event(me, "Me")); store.save_changes(&changes).await.unwrap(); - room.inner.update_if(|info| info.update_summary(&summary)); + room.inner.update_if(|info| info.update_from_ruma_summary(&summary)); assert_eq!( room.computed_display_name().await.unwrap(), DisplayName::Calculated("Matthew".to_owned()) @@ -1916,7 +1952,7 @@ mod tests { store.save_changes(&changes).await.unwrap(); - room.inner.update_if(|info| info.update_summary(&summary)); + room.inner.update_if(|info| info.update_from_ruma_summary(&summary)); assert_eq!( room.computed_display_name().await.unwrap(), DisplayName::Calculated("Matthew".to_owned()) @@ -1995,7 +2031,7 @@ mod tests { joined_member_count: Some(7u32.into()), heroes: vec![denis.to_string(), carol.to_string(), bob.to_string(), erica.to_string()], }); - room.inner.update_if(|info| info.update_summary(&summary)); + room.inner.update_if(|info| info.update_from_ruma_summary(&summary)); assert_eq!( room.computed_display_name().await.unwrap(), @@ -2075,7 +2111,7 @@ mod tests { store.save_changes(&changes).await.unwrap(); - room.inner.update_if(|info| info.update_summary(&summary)); + room.inner.update_if(|info| info.update_from_ruma_summary(&summary)); assert_eq!( room.computed_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 2b9c6e12f..91d4dd7ba 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -21,7 +21,7 @@ use matrix_sdk_common::deserialized_responses::SyncTimelineEvent; use ruma::events::AnyToDeviceEvent; use ruma::{ api::client::sync::sync_events::{ - v3::{self, InvitedRoom, RoomSummary}, + v3::{self, InvitedRoom}, v4, }, events::{AnyRoomAccountDataEvent, AnySyncStateEvent, AnySyncTimelineEvent}, @@ -38,7 +38,7 @@ use crate::RoomMemberships; use crate::{ error::Result, read_receipts::{compute_unread_counts, PreviousEventsProvider}, - rooms::RoomState, + rooms::{normal::RoomSummary, RoomState}, store::{ambiguity_map::AmbiguityCache, StateChanges, Store}, sync::{JoinedRoomUpdate, LeftRoomUpdate, Notification, RoomUpdates, SyncResponse}, Room, RoomInfo, @@ -695,23 +695,19 @@ fn process_room_properties(room_data: &v4::SlidingSyncRoom, room_info: &mut Room // Sliding sync doesn't have a room summary, nevertheless it contains the joined // and invited member counts, in addition to the heroes if it's been configured // to return them (see the [`v4::RoomSubscription::include_heroes`]). - // - // Let's at least fetch the member counts, since they might be useful. - let mut room_summary = RoomSummary::new(); - - room_summary.invited_member_count = room_data.invited_count; - room_summary.joined_member_count = room_data.joined_count; - - if let Some(heroes) = &room_data.heroes { - // It's not clear for Ruma, but the `heroes` field should be a collection of - // [`UserId`]. - room_summary.heroes = heroes - .iter() - .filter_map(|hero| hero.user_id.as_ref().map(ToString::to_string)) - .collect(); + if let Some(count) = room_data.joined_count { + room_info.update_joined_member_count(count.into()); + } + if let Some(count) = room_data.invited_count { + room_info.update_invited_member_count(count.into()); } - room_info.update_summary(&room_summary); + if let Some(heroes) = &room_data.heroes { + // Filter out all the heroes which don't have a user id. + room_info.update_heroes( + heroes.iter().filter_map(|hero| hero.user_id.as_ref()).cloned().collect(), + ); + } room_info.set_prev_batch(room_data.prev_batch.as_deref());