fix(base,sdk,ui): Synchronize sliding sync room's avatar with regular Room

fix(base,sdk,ui): Synchronize sliding sync room's avatar with regular `Room`

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);
 

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<OwnedMxcUri>) {
+        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;

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<OwnedRoomId, Vec<Notification>>,
         ambiguity_cache: &mut AmbiguityCache,
     ) -> Result<(RoomInfo, Option<JoinedRoom>, Option<LeftRoom>, Option<InvitedRoom>)> {
-        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

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<Arc<Timeline>>,
 }
 
+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<OwnedMxcUri> {
-        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

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<OwnedMxcUri> {
-        let inner = self.inner.inner.read().unwrap();
-
-        inner.avatar.clone().into_option()
-    }
-
     /// Is this a direct message?
     pub fn is_dm(&self) -> Option<bool> {
         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);
-    }
 }

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?;