From e00532f5d21f7068d68afc7dffdc20392b828ef8 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 1 Feb 2024 08:56:21 +0100 Subject: [PATCH 1/3] feat(base): Update room's avatar from `SlidingSyncRoom::avatar`. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To update the avatar of a room, one has to look up in the state event. That's the canonical way to do. For Sliding Sync, it means looking inside the `required_state` field of the `v4::SlidingSyncRoom`. This case already works and was tested. However, a `v4::SlidingSyncRoom` comes with a direct `avatar` field. It's another way to know the avatar URL of the room. This case was handled and tested in `matrix_sdk::sliding_sync::SlidingSyncRoom`, but it was never propagated into the proper sync mechanism. So when the `avatar` field was set up, `matrix_sdk::sliding_sync::SlidingSyncRoom` was holding this information, and the `avatar` wasn't defined in the proper `Room`: `SlidingSyncRoom` has to look up inside the `Room` as a fallback. This patch is the first one to fix this “fallback” mechanism. The `avatar` field of a `v4::SlidingSyncRoom` now triggers an update to the new `RoomInfo::update_avatar` method (à la `update_name`), via `process_room_properties`. --- crates/matrix-sdk-base/src/client.rs | 1 + crates/matrix-sdk-base/src/rooms/normal.rs | 11 ++ crates/matrix-sdk-base/src/sliding_sync.rs | 112 ++++++++++++++++++++- 3 files changed, 119 insertions(+), 5 deletions(-) diff --git a/crates/matrix-sdk-base/src/client.rs b/crates/matrix-sdk-base/src/client.rs index 558a0d11a..024611ac0 100644 --- a/crates/matrix-sdk-base/src/client.rs +++ b/crates/matrix-sdk-base/src/client.rs @@ -496,6 +496,7 @@ impl BaseClient { let mut profiles = BTreeMap::new(); assert_eq!(raw_events.len(), events.len()); + for (raw_event, event) in iter::zip(raw_events, events) { room_info.handle_state_event(event); diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index eca133583..212cd3fa3 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -34,6 +34,7 @@ use ruma::{ ignored_user_list::IgnoredUserListEventContent, receipt::{Receipt, ReceiptThread, ReceiptType}, room::{ + avatar::RoomAvatarEventContent, encryption::RoomEncryptionEventContent, guest_access::GuestAccess, history_visibility::HistoryVisibility, @@ -1007,6 +1008,16 @@ impl RoomInfo { })); } + /// Update the room avatar + pub fn update_avatar(&mut self, url: Option) { + self.base_info.avatar = url.map(|url| { + let mut content = RoomAvatarEventContent::new(); + content.url = Some(url); + + MinimalStateEvent::Original(OriginalMinimalStateEvent { content, event_id: None }) + }); + } + /// Update the notifications count pub fn update_notification_count(&mut self, notification_counts: UnreadNotificationsCount) { self.notification_counts = notification_counts; diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 9a6bb0b5d..28f7e9dd2 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -26,7 +26,7 @@ use ruma::{ }, events::{AnyRoomAccountDataEvent, AnySyncStateEvent, AnySyncTimelineEvent}, serde::Raw, - OwnedRoomId, RoomId, + JsOption, OwnedRoomId, RoomId, }; use tracing::{instrument, trace, warn}; @@ -319,10 +319,17 @@ impl BaseClient { notifications: &mut BTreeMap>, ambiguity_cache: &mut AmbiguityCache, ) -> Result<(RoomInfo, Option, Option, Option)> { - let mut state_events = Self::deserialize_state_events(&room_data.required_state); - state_events.extend(Self::deserialize_state_events_from_timeline(&room_data.timeline)); + let (raw_state_events, state_events): (Vec<_>, Vec<_>) = { + let mut state_events = Vec::new(); - let (raw_state_events, state_events): (Vec<_>, Vec<_>) = state_events.into_iter().unzip(); + // Read state events from the `required_state` field. + state_events.extend(Self::deserialize_state_events(&room_data.required_state)); + + // Read state events from the `timeline` field. + state_events.extend(Self::deserialize_state_events_from_timeline(&room_data.timeline)); + + state_events.into_iter().unzip() + }; // Find or create the room in the store #[allow(unused_mut)] // Required for some feature flag combinations @@ -669,10 +676,25 @@ async fn cache_latest_events( } fn process_room_properties(room_data: &v4::SlidingSyncRoom, room_info: &mut RoomInfo) { + // Handle the room's name. if let Some(name) = &room_data.name { room_info.update_name(name.to_owned()); } + // Handle the room's avatar. + // + // It can be updated via the state events, or via the `SlidingSyncRoom::avatar` + // field. This part of the code handles the latter case. The former case is + // handled by [`BaseClient::handle_state`]. + match &room_data.avatar { + // A new avatar! + JsOption::Some(avatar_uri) => room_info.update_avatar(Some(avatar_uri.to_owned())), + // Avatar must be removed. + JsOption::Null => room_info.update_avatar(None), + // Nothing to do. + JsOption::Undefined => {} + } + // Sliding sync doesn't have a room summary, nevertheless it contains the joined // and invited member counts. It likely will never have a heroes concept since // it calculates the room display name for us. @@ -716,7 +738,7 @@ mod tests { }, mxc_uri, room_alias_id, room_id, serde::Raw, - uint, user_id, MxcUri, OwnedRoomId, OwnedUserId, RoomAliasId, RoomId, UserId, + uint, user_id, JsOption, MxcUri, OwnedRoomId, OwnedUserId, RoomAliasId, RoomId, UserId, }; use serde_json::json; @@ -1027,6 +1049,86 @@ mod tests { // Given a logged-in client let client = logged_in_client().await; let room_id = room_id!("!r:e.uk"); + + // When I send sliding sync response containing a room with an avatar + let room = { + let mut room = v4::SlidingSyncRoom::new(); + room.avatar = JsOption::from_option(Some(mxc_uri!("mxc://e.uk/med1").to_owned())); + + room + }; + let response = response_with_room(room_id, room).await; + client.process_sliding_sync(&response, &()).await.expect("Failed to process sync"); + + // Then the room in the client has the avatar + let client_room = client.get_room(room_id).expect("No room found"); + assert_eq!( + client_room.avatar_url().expect("No avatar URL").media_id().expect("No media ID"), + "med1" + ); + } + + #[async_test] + async fn avatar_can_be_unset_when_processing_sliding_sync_response() { + // Given a logged-in client + let client = logged_in_client().await; + let room_id = room_id!("!r:e.uk"); + + // Set the avatar. + + // When I send sliding sync response containing a room with an avatar + let room = { + let mut room = v4::SlidingSyncRoom::new(); + room.avatar = JsOption::from_option(Some(mxc_uri!("mxc://e.uk/med1").to_owned())); + + room + }; + let response = response_with_room(room_id, room).await; + client.process_sliding_sync(&response, &()).await.expect("Failed to process sync"); + + // Then the room in the client has the avatar + let client_room = client.get_room(room_id).expect("No room found"); + assert_eq!( + client_room.avatar_url().expect("No avatar URL").media_id().expect("No media ID"), + "med1" + ); + + // No avatar. Still here. + + // When I send sliding sync response containing no avatar. + let room = v4::SlidingSyncRoom::new(); + let response = response_with_room(room_id, room).await; + client.process_sliding_sync(&response, &()).await.expect("Failed to process sync"); + + // Then the room in the client still has the avatar + let client_room = client.get_room(room_id).expect("No room found"); + assert_eq!( + client_room.avatar_url().expect("No avatar URL").media_id().expect("No media ID"), + "med1" + ); + + // Avatar is unset. + + // When I send sliding sync response containing an avatar set to `null` (!). + let room = { + let mut room = v4::SlidingSyncRoom::new(); + room.avatar = JsOption::Null; + + room + }; + let response = response_with_room(room_id, room).await; + client.process_sliding_sync(&response, &()).await.expect("Failed to process sync"); + + // Then the room in the client has no more avatar + let client_room = client.get_room(room_id).expect("No room found"); + assert!(client_room.avatar_url().is_none()); + } + + #[async_test] + async fn avatar_is_found_from_required_state_when_processing_sliding_sync_response() { + // Given a logged-in client + let client = logged_in_client().await; + let room_id = room_id!("!r:e.uk"); let user_id = user_id!("@u:e.uk"); // When I send sliding sync response containing a room with an avatar From 90f1a34855e5319a6841ce06334c66feef1176c1 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 1 Feb 2024 09:09:33 +0100 Subject: [PATCH 2/3] feat(sdk): Remove the `avatar_url` logic in `SlidingSyncRoom`. With the previous commit, the avatar is properly synchronized with the `Room`. The result is that `SlidingSyncRoom` no longer needs to hold the `avatar_url`. --- crates/matrix-sdk/src/sliding_sync/room.rs | 42 +------------------ .../src/tests/sliding_sync/room.rs | 8 ---- 2 files changed, 1 insertion(+), 49 deletions(-) diff --git a/crates/matrix-sdk/src/sliding_sync/room.rs b/crates/matrix-sdk/src/sliding_sync/room.rs index abbfbb668..df4c3330b 100644 --- a/crates/matrix-sdk/src/sliding_sync/room.rs +++ b/crates/matrix-sdk/src/sliding_sync/room.rs @@ -10,7 +10,7 @@ use ruma::{ api::client::sync::sync_events::{v4, UnreadNotificationsCount}, events::AnySyncStateEvent, serde::Raw, - OwnedMxcUri, OwnedRoomId, RoomId, + OwnedRoomId, RoomId, }; use serde::{Deserialize, Serialize}; @@ -74,13 +74,6 @@ impl SlidingSyncRoom { inner.name.to_owned() } - /// Get the room avatar URL. - pub fn avatar_url(&self) -> Option { - let inner = self.inner.inner.read().unwrap(); - - inner.avatar.clone().into_option() - } - /// Is this a direct message? pub fn is_dm(&self) -> Option { let inner = self.inner.inner.read().unwrap(); @@ -459,22 +452,6 @@ mod tests { _ = Some("gordon".to_owned()); } - test_avatar { - avatar_url() = None; - receives room_response!({"avatar": "mxc://homeserver/media"}); - _ = Some(mxc_uri!("mxc://homeserver/media").to_owned()); - receives nothing; - _ = Some(mxc_uri!("mxc://homeserver/media").to_owned()); - } - - test_avatar_unset { - avatar_url() = None; - receives room_response!({ "avatar": null }); - _ = None; - receives nothing; - _ = None; - } - test_room_is_dm { is_dm() = None; receives room_response!({"is_dm": true}); @@ -1059,21 +1036,4 @@ mod tests { ); } } - - #[async_test] - async fn test_avatar_set_then_unset() { - let mut room = new_room(room_id!("!foo:bar.org"), room_response!({})).await; - assert_eq!(room.avatar_url(), None); - - room.update(room_response!({ "avatar": "mxc://homeserver/media" }), vec![]); - assert_eq!(room.avatar_url().as_deref(), Some(mxc_uri!("mxc://homeserver/media"))); - - // avatar is undefined. - room.update(room_response!({}), vec![]); - assert_eq!(room.avatar_url().as_deref(), Some(mxc_uri!("mxc://homeserver/media"))); - - // avatar is null => reset it to None. - room.update(room_response!({ "avatar": null }), vec![]); - assert_eq!(room.avatar_url().as_deref(), None); - } } 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 feffd8565..567f13697 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 @@ -168,17 +168,11 @@ async fn test_room_avatar_group_conversation() -> Result<()> { let alice_room = alice.get_room(alice_room.room_id()).unwrap(); assert_eq!(alice_room.state(), RoomState::Joined); - let sliding_room = sliding_alice - .get_room(alice_room.room_id()) - .await - .expect("sliding sync finds alice's own room"); - // Here, there should be no avatar (group conversation and no avatar has been // set in the room). for _ in 0..3 { sleep(Duration::from_secs(1)).await; assert_eq!(alice_room.avatar_url(), None); - assert_eq!(sliding_room.avatar_url(), None); // Force a new server response. alice_room.send(RoomMessageEventContent::text_plain("hello world")).await?; @@ -191,7 +185,6 @@ async fn test_room_avatar_group_conversation() -> Result<()> { for _ in 0..3 { sleep(Duration::from_secs(1)).await; assert_eq!(alice_room.avatar_url().as_deref(), Some(group_avatar_uri)); - assert_eq!(sliding_room.avatar_url().as_deref(), Some(group_avatar_uri)); // Force a new server response. alice_room.send(RoomMessageEventContent::text_plain("hello world")).await?; @@ -203,7 +196,6 @@ async fn test_room_avatar_group_conversation() -> Result<()> { for _ in 0..3 { sleep(Duration::from_secs(1)).await; assert_eq!(alice_room.avatar_url(), None); - assert_eq!(sliding_room.avatar_url(), None); // Force a new server response. alice_room.send(RoomMessageEventContent::text_plain("hello world")).await?; From 9ef9251936655e6f8b8cdd1bbe5485724139c6d0 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 1 Feb 2024 09:12:42 +0100 Subject: [PATCH 3/3] feat(ui): Remove the fallback mechanism for avatar in `room_list_service::Room`. `SlidingSyncRoom` no longer has an `avatar_url` method. `room_list::Room` no longer needs to check first in sliding sync then in `Room` as a fallback. This patch removes the `room_list::Room::avatar_url` method. This patch also implements `Deref` for `room_list::Room` to `matrix_sdk::Room`, to make our lifes easier. --- .../src/room_list_service/room.rs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) 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 83e9e16d6..9b5bdf32b 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/room.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/room.rs @@ -14,13 +14,13 @@ //! The `Room` type. -use std::sync::Arc; +use std::{ops::Deref, sync::Arc}; use async_once_cell::OnceCell as AsyncOnceCell; use matrix_sdk::{SlidingSync, SlidingSyncRoom}; use ruma::{ api::client::sync::sync_events::{v4::RoomSubscription, UnreadNotificationsCount}, - OwnedMxcUri, RoomId, + RoomId, }; use super::Error; @@ -52,6 +52,14 @@ struct RoomInner { timeline: AsyncOnceCell>, } +impl Deref for Room { + type Target = matrix_sdk::Room; + + fn deref(&self) -> &Self::Target { + &self.inner.room + } +} + impl Room { /// Create a new `Room`. pub(super) fn new( @@ -89,14 +97,6 @@ impl Room { }) } - /// Get the best possible avatar for the room. - /// - /// If the sliding sync room has received an avatar from the server, then - /// use it, otherwise, let's try to find one from `Room`. - pub fn avatar_url(&self) -> Option { - self.inner.sliding_sync_room.avatar_url().or_else(|| self.inner.room.avatar_url()) - } - /// Get the underlying [`matrix_sdk::Room`]. pub fn inner_room(&self) -> &matrix_sdk::Room { &self.inner.room