From 90bed18415f3cbc5c8222c96caabbab9f15dbf11 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 1 May 2024 11:56:00 +0200 Subject: [PATCH] ffi: make the `name` method sync again Also: - rename `display_name` to `computed_display_name` in several places, and reflect that change into a few callers - simplify slightly the `computed_display_name()` method --- bindings/matrix-sdk-ffi/src/room.rs | 7 +- bindings/matrix-sdk-ffi/src/room_list.rs | 7 +- crates/matrix-sdk-base/src/client.rs | 2 +- crates/matrix-sdk-base/src/rooms/normal.rs | 74 ++++++++++++------- crates/matrix-sdk-base/src/sliding_sync.rs | 2 +- .../matrix-sdk-ui/src/notification_client.rs | 2 +- .../src/room_list_service/room.rs | 6 +- .../tests/integration/room_list_service.rs | 6 +- .../tests/integration/room/common.rs | 12 ++- examples/oidc_cli/src/main.rs | 2 +- examples/persist_session/src/main.rs | 2 +- 11 files changed, 78 insertions(+), 44 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/room.rs b/bindings/matrix-sdk-ffi/src/room.rs index c53e65657e2..dd755aaab54 100644 --- a/bindings/matrix-sdk-ffi/src/room.rs +++ b/bindings/matrix-sdk-ffi/src/room.rs @@ -210,8 +210,11 @@ impl Room { Ok(Timeline::new(timeline)) } - pub async fn display_name(&self) -> Result { - Ok(self.inner.display_name().await?.to_string()) + /// 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 computed_display_name(&self) -> Result { + Ok(RUNTIME.block_on(self.inner.computed_display_name())?.to_string()) } pub async fn is_encrypted(&self) -> Result { diff --git a/bindings/matrix-sdk-ffi/src/room_list.rs b/bindings/matrix-sdk-ffi/src/room_list.rs index 19d439addd5..82594ca2e1c 100644 --- a/bindings/matrix-sdk-ffi/src/room_list.rs +++ b/bindings/matrix-sdk-ffi/src/room_list.rs @@ -490,8 +490,11 @@ impl RoomListItem { self.inner.id().to_string() } - async fn name(&self) -> Option { - self.inner.name().await + /// 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. + fn computed_display_name(&self) -> Option { + RUNTIME.block_on(self.inner.computed_display_name()) } fn avatar_url(&self) -> Option { diff --git a/crates/matrix-sdk-base/src/client.rs b/crates/matrix-sdk-base/src/client.rs index babd5d38edb..1cdf1841edc 100644 --- a/crates/matrix-sdk-base/src/client.rs +++ b/crates/matrix-sdk-base/src/client.rs @@ -1635,7 +1635,7 @@ mod tests { let room = client.get_room(room_id).expect("Room not found"); assert_eq!(room.state(), RoomState::Invited); assert_eq!( - room.display_name().await.expect("fetching display name failed"), + room.computed_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 4b4b465c187..95088787c61 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -458,7 +458,7 @@ impl Room { /// The display name is calculated according to [this algorithm][spec]. /// /// [spec]: - pub async fn display_name(&self) -> StoreResult { + pub async fn computed_display_name(&self) -> StoreResult { self.calculate_name().await } @@ -602,25 +602,26 @@ impl Room { inner.summary.clone() }; - let is_own_member = |m: &RoomMember| m.user_id() == &*self.own_user_id; - let is_own_user_id = |u: &str| u == self.own_user_id().as_str(); + let own_user_id = self.own_user_id().as_str(); let members: Vec = if summary.heroes.is_empty() { self.members(RoomMemberships::ACTIVE) .await? .into_iter() - .filter(|u| !is_own_member(u)) + .filter(|u| u.user_id() != own_user_id) .take(5) .collect() } else { - let members: Vec<_> = - stream::iter(summary.heroes.iter().filter(|u| !is_own_user_id(u))) - .filter_map(|u| async move { - let user_id = UserId::parse(u.as_str()).ok()?; - self.get_member(&user_id).await.transpose() - }) - .collect() - .await; + let members: Vec<_> = stream::iter(summary.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: StoreResult> = members.into_iter().collect(); @@ -1720,7 +1721,7 @@ mod tests { #[async_test] async fn test_display_name_for_joined_room_is_empty_if_no_info() { let (_, room) = make_room(RoomState::Joined); - assert_eq!(room.display_name().await.unwrap(), DisplayName::Empty); + assert_eq!(room.computed_display_name().await.unwrap(), DisplayName::Empty); } #[async_test] @@ -1728,7 +1729,10 @@ mod tests { let (_, room) = make_room(RoomState::Joined); room.inner .update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event())); - assert_eq!(room.display_name().await.unwrap(), DisplayName::Aliased("test".to_owned())); + assert_eq!( + room.computed_display_name().await.unwrap(), + DisplayName::Aliased("test".to_owned()) + ); } #[async_test] @@ -1736,16 +1740,22 @@ mod tests { let (_, room) = make_room(RoomState::Joined); room.inner .update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event())); - assert_eq!(room.display_name().await.unwrap(), DisplayName::Aliased("test".to_owned())); + assert_eq!( + room.computed_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.display_name().await.unwrap(), DisplayName::Named("Test Room".to_owned())); + assert_eq!( + room.computed_display_name().await.unwrap(), + DisplayName::Named("Test Room".to_owned()) + ); } #[async_test] async fn test_display_name_for_invited_room_is_empty_if_no_info() { let (_, room) = make_room(RoomState::Invited); - assert_eq!(room.display_name().await.unwrap(), DisplayName::Empty); + assert_eq!(room.computed_display_name().await.unwrap(), DisplayName::Empty); } #[async_test] @@ -1758,7 +1768,7 @@ mod tests { }); room.inner.update(|info| info.base_info.name = Some(room_name)); - assert_eq!(room.display_name().await.unwrap(), DisplayName::Empty); + assert_eq!(room.computed_display_name().await.unwrap(), DisplayName::Empty); } #[async_test] @@ -1766,7 +1776,10 @@ mod tests { let (_, room) = make_room(RoomState::Invited); room.inner .update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event())); - assert_eq!(room.display_name().await.unwrap(), DisplayName::Aliased("test".to_owned())); + assert_eq!( + room.computed_display_name().await.unwrap(), + DisplayName::Aliased("test".to_owned()) + ); } #[async_test] @@ -1774,10 +1787,16 @@ mod tests { let (_, room) = make_room(RoomState::Invited); room.inner .update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event())); - assert_eq!(room.display_name().await.unwrap(), DisplayName::Aliased("test".to_owned())); + assert_eq!( + room.computed_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.display_name().await.unwrap(), DisplayName::Named("Test Room".to_owned())); + assert_eq!( + room.computed_display_name().await.unwrap(), + DisplayName::Named("Test Room".to_owned()) + ); } fn make_canonical_alias_event() -> MinimalStateEvent { @@ -1817,7 +1836,7 @@ mod tests { room.inner.update_if(|info| info.update_summary(&summary)); assert_eq!( - room.display_name().await.unwrap(), + room.computed_display_name().await.unwrap(), DisplayName::Calculated("Matthew".to_owned()) ); } @@ -1839,7 +1858,7 @@ mod tests { store.save_changes(&changes).await.unwrap(); assert_eq!( - room.display_name().await.unwrap(), + room.computed_display_name().await.unwrap(), DisplayName::Calculated("Matthew".to_owned()) ); } @@ -1869,7 +1888,7 @@ mod tests { room.inner.update_if(|info| info.update_summary(&summary)); assert_eq!( - room.display_name().await.unwrap(), + room.computed_display_name().await.unwrap(), DisplayName::Calculated("Matthew".to_owned()) ); } @@ -1894,7 +1913,7 @@ mod tests { store.save_changes(&changes).await.unwrap(); assert_eq!( - room.display_name().await.unwrap(), + room.computed_display_name().await.unwrap(), DisplayName::Calculated("Matthew".to_owned()) ); } @@ -1923,7 +1942,10 @@ mod tests { store.save_changes(&changes).await.unwrap(); room.inner.update_if(|info| info.update_summary(&summary)); - assert_eq!(room.display_name().await.unwrap(), DisplayName::EmptyWas("Matthew".to_owned())); + assert_eq!( + room.computed_display_name().await.unwrap(), + DisplayName::EmptyWas("Matthew".to_owned()) + ); } #[test] diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 4eff738921d..a15c826cf37 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -1272,7 +1272,7 @@ mod tests { // Then the room's name is just exactly what the server supplied let client_room = client.get_room(room_id).expect("No room found"); assert_eq!( - client_room.display_name().await.unwrap().to_string(), + client_room.computed_display_name().await.unwrap().to_string(), "This came from the server" ); } diff --git a/crates/matrix-sdk-ui/src/notification_client.rs b/crates/matrix-sdk-ui/src/notification_client.rs index 3ece08deefe..341c8301391 100644 --- a/crates/matrix-sdk-ui/src/notification_client.rs +++ b/crates/matrix-sdk-ui/src/notification_client.rs @@ -715,7 +715,7 @@ impl NotificationItem { sender_display_name, sender_avatar_url, is_sender_name_ambiguous, - room_display_name: room.display_name().await?.to_string(), + room_display_name: room.computed_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/room.rs b/crates/matrix-sdk-ui/src/room_list_service/room.rs index 4a4162bdc1f..667ed1fde9d 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/room.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/room.rs @@ -83,9 +83,9 @@ impl Room { self.inner.room.room_id() } - /// Get the name of the room if it exists. - pub async fn name(&self) -> Option { - Some(self.inner.room.display_name().await.ok()?.to_string()) + /// 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()) } /// 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 c9ea8610752..80ef25c913c 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs @@ -2260,7 +2260,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.name().await, Some("Room #0".to_owned())); + assert_eq!(room0.computed_display_name().await, 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())); @@ -2268,7 +2268,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.name().await, Some("Empty Room".to_owned())); + assert_eq!(room1.computed_display_name().await, 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. @@ -2303,7 +2303,7 @@ async fn test_room() -> Result<(), Error> { }; // Room has _now_ received a name from sliding sync! - assert_eq!(room1.name().await, Some("Room #1".to_owned())); + assert_eq!(room1.computed_display_name().await, 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/crates/matrix-sdk/tests/integration/room/common.rs b/crates/matrix-sdk/tests/integration/room/common.rs index 31e33e4455c..cde18aed29d 100644 --- a/crates/matrix-sdk/tests/integration/room/common.rs +++ b/crates/matrix-sdk/tests/integration/room/common.rs @@ -56,7 +56,10 @@ async fn calculate_room_names_from_summary() { let _response = client.sync_once(sync_settings).await.unwrap(); let room = client.get_room(&DEFAULT_TEST_ROOM_ID).unwrap(); - assert_eq!(DisplayName::Calculated("example2".to_owned()), room.display_name().await.unwrap()); + assert_eq!( + DisplayName::Calculated("example2".to_owned()), + room.computed_display_name().await.unwrap() + ); } #[async_test] @@ -72,7 +75,10 @@ async fn room_names() { assert_eq!(client.rooms().len(), 1); let room = client.get_room(&DEFAULT_TEST_ROOM_ID).unwrap(); - assert_eq!(DisplayName::Aliased("tutorial".to_owned()), room.display_name().await.unwrap()); + assert_eq!( + DisplayName::Aliased("tutorial".to_owned()), + room.computed_display_name().await.unwrap() + ); mock_sync(&server, &*test_json::INVITE_SYNC, Some(sync_token.clone())).await; @@ -83,7 +89,7 @@ async fn room_names() { assert_eq!( DisplayName::Named("My Room Name".to_owned()), - invited_room.display_name().await.unwrap() + invited_room.computed_display_name().await.unwrap() ); } diff --git a/examples/oidc_cli/src/main.rs b/examples/oidc_cli/src/main.rs index 34366de9db3..830d6b446ab 100644 --- a/examples/oidc_cli/src/main.rs +++ b/examples/oidc_cli/src/main.rs @@ -880,7 +880,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.display_name().await { + let room_name = match room.computed_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 81b40545da1..6b880f36502 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.display_name().await { + let room_name = match room.computed_display_name().await { Ok(room_name) => room_name.to_string(), Err(error) => { println!("Error getting room display name: {error}");