fix(pinned_events): get pinned event ids from the HS if the sync does…

…n't contain it

This should take care of a bug that caused pinned events to be incorrectly removed when the new pinned event ids list was based on an empty one if the required state of the room didn't contain any pinned events info

benchmarks/benches/room_bench.rs

@@ -171,8 +171,9 @@ pub fn load_pinned_events_benchmark(c: &mut Criterion) {
     );
 
     let room = client.get_room(&room_id).expect("Room not found");
-    assert!(!room.pinned_event_ids().is_empty());
-    assert_eq!(room.pinned_event_ids().len(), PINNED_EVENTS_COUNT);
+    let pinned_event_ids = room.pinned_event_ids().unwrap_or_default();
+    assert!(!pinned_event_ids.is_empty());
+    assert_eq!(pinned_event_ids.len(), PINNED_EVENTS_COUNT);
 
     let count = PINNED_EVENTS_COUNT;
     let name = format!("{count} pinned events");
@@ -191,8 +192,9 @@ pub fn load_pinned_events_benchmark(c: &mut Criterion) {
 
     group.bench_function(BenchmarkId::new("load_pinned_events", name), |b| {
         b.to_async(&runtime).iter(|| async {
-            assert!(!room.pinned_event_ids().is_empty());
-            assert_eq!(room.pinned_event_ids().len(), PINNED_EVENTS_COUNT);
+            let pinned_event_ids = room.pinned_event_ids().unwrap_or_default();
+            assert!(!pinned_event_ids.is_empty());
+            assert_eq!(pinned_event_ids.len(), PINNED_EVENTS_COUNT);
 
             // Reset cache so it always loads the events from the mocked endpoint
             client.event_cache().empty_immutable_cache().await;

bindings/matrix-sdk-ffi/src/room_info.rs

@@ -67,7 +67,8 @@ impl RoomInfo {
         for (id, level) in power_levels_map.iter() {
             user_power_levels.insert(id.to_string(), *level);
         }
-        let pinned_event_ids = room.pinned_event_ids().iter().map(|id| id.to_string()).collect();
+        let pinned_event_ids =
+            room.pinned_event_ids().unwrap_or_default().iter().map(|id| id.to_string()).collect();
 
         Ok(Self {
             id: room.room_id().to_string(),

crates/matrix-sdk-base/src/rooms/normal.rs

@@ -1019,7 +1019,7 @@ impl Room {
     }
 
     /// Returns the current pinned event ids for this room.
-    pub fn pinned_event_ids(&self) -> Vec<OwnedEventId> {
+    pub fn pinned_event_ids(&self) -> Option<Vec<OwnedEventId>> {
         self.inner.read().pinned_event_ids()
     }
 }
@@ -1596,8 +1596,8 @@ impl RoomInfo {
     }
 
     /// Returns the current pinned event ids for this room.
-    pub fn pinned_event_ids(&self) -> Vec<OwnedEventId> {
-        self.base_info.pinned_events.clone().map(|c| c.pinned).unwrap_or_default()
+    pub fn pinned_event_ids(&self) -> Option<Vec<OwnedEventId>> {
+        self.base_info.pinned_events.clone().map(|c| c.pinned)
     }
 
     /// Checks if an `EventId` is currently pinned.

crates/matrix-sdk-base/src/sliding_sync/mod.rs

@@ -2509,7 +2509,7 @@ mod tests {
         // The newly created room has no pinned event ids
         let room = client.get_room(room_id).unwrap();
         let pinned_event_ids = room.pinned_event_ids();
-        assert!(pinned_event_ids.is_empty());
+        assert_matches!(pinned_event_ids, None);
 
         // Load new pinned event id
         let mut room_response = http::response::Room::new();
@@ -2522,7 +2522,7 @@ mod tests {
         let response = response_with_room(room_id, room_response);
         client.process_sliding_sync(&response, &(), true).await.expect("Failed to process sync");
 
-        let pinned_event_ids = room.pinned_event_ids();
+        let pinned_event_ids = room.pinned_event_ids().unwrap_or_default();
         assert_eq!(pinned_event_ids.len(), 1);
         assert_eq!(pinned_event_ids[0], pinned_event_id);
 
@@ -2536,7 +2536,7 @@ mod tests {
         ));
         let response = response_with_room(room_id, room_response);
         client.process_sliding_sync(&response, &(), true).await.expect("Failed to process sync");
-        let pinned_event_ids = room.pinned_event_ids();
+        let pinned_event_ids = room.pinned_event_ids().unwrap();
         assert!(pinned_event_ids.is_empty());
     }
 

crates/matrix-sdk-ui/src/timeline/mod.rs

@@ -729,10 +729,18 @@ impl Timeline {
     /// Adds a new pinned event by sending an updated `m.room.pinned_events`
     /// event containing the new event id.
     ///
-    /// Returns `true` if we sent the request, `false` if the event was already
+    /// This method will first try to get the pinned events from the current
+    /// room's state and if it fails to do so it'll try to load them from the
+    /// homeserver.
+    ///
+    /// Returns `true` if we pinned the event, `false` if the event was already
     /// pinned.
     pub async fn pin_event(&self, event_id: &EventId) -> Result<bool> {
-        let mut pinned_event_ids = self.room().pinned_event_ids();
+        let mut pinned_event_ids = if let Some(event_ids) = self.room().pinned_event_ids() {
+            event_ids
+        } else {
+            self.room().load_pinned_events().await?.unwrap_or_default()
+        };
         let event_id = event_id.to_owned();
         if pinned_event_ids.contains(&event_id) {
             Ok(false)
@@ -744,13 +752,21 @@ impl Timeline {
         }
     }
 
-    /// Adds a new pinned event by sending an updated `m.room.pinned_events`
+    /// Removes a pinned event by sending an updated `m.room.pinned_events`
     /// event without the event id we want to remove.
     ///
-    /// Returns `true` if we sent the request, `false` if the event wasn't
-    /// pinned.
+    /// This method will first try to get the pinned events from the current
+    /// room's state and if it fails to do so it'll try to load them from the
+    /// homeserver.
+    ///
+    /// Returns `true` if we unpinned the event, `false` if the event wasn't
+    /// pinned before.
     pub async fn unpin_event(&self, event_id: &EventId) -> Result<bool> {
-        let mut pinned_event_ids = self.room().pinned_event_ids();
+        let mut pinned_event_ids = if let Some(event_ids) = self.room().pinned_event_ids() {
+            event_ids
+        } else {
+            self.room().load_pinned_events().await?.unwrap_or_default()
+        };
         let event_id = event_id.to_owned();
         if let Some(idx) = pinned_event_ids.iter().position(|e| *e == *event_id) {
             pinned_event_ids.remove(idx);

crates/matrix-sdk-ui/src/timeline/pinned_events_loader.rs

@@ -61,6 +61,7 @@ impl PinnedEventsLoader {
         let pinned_event_ids: Vec<OwnedEventId> = self
             .room
             .pinned_event_ids()
+            .unwrap_or_default()
             .into_iter()
             .rev()
             .take(self.max_events_to_load)
@@ -134,7 +135,7 @@ pub trait PinnedEventsRoom: SendOutsideWasm + SyncOutsideWasm {
     ) -> BoxFuture<'a, Result<(SyncTimelineEvent, Vec<SyncTimelineEvent>), PaginatorError>>;
 
     /// Get the pinned event ids for a room.
-    fn pinned_event_ids(&self) -> Vec<OwnedEventId>;
+    fn pinned_event_ids(&self) -> Option<Vec<OwnedEventId>>;
 
     /// Checks whether an event id is pinned in this room.
     ///
@@ -168,7 +169,7 @@ impl PinnedEventsRoom for Room {
         .boxed()
     }
 
-    fn pinned_event_ids(&self) -> Vec<OwnedEventId> {
+    fn pinned_event_ids(&self) -> Option<Vec<OwnedEventId>> {
         self.clone_info().pinned_event_ids()
     }
 

crates/matrix-sdk-ui/src/timeline/tests/mod.rs

@@ -351,7 +351,7 @@ impl PinnedEventsRoom for TestRoomDataProvider {
         unimplemented!();
     }
 
-    fn pinned_event_ids(&self) -> Vec<OwnedEventId> {
+    fn pinned_event_ids(&self) -> Option<Vec<OwnedEventId>> {
         unimplemented!();
     }
 

crates/matrix-sdk-ui/tests/integration/timeline/mod.rs

@@ -628,7 +628,7 @@ async fn test_pin_event_is_sent_successfully() {
 
     // Pinning a remote event succeeds.
     setup
-        .mock_response(ResponseTemplate::new(200).set_body_json(json!({
+        .mock_pin_unpin_response(ResponseTemplate::new(200).set_body_json(json!({
             "event_id": "$42"
         })))
         .await;
@@ -662,7 +662,7 @@ async fn test_pin_event_is_returning_an_error() {
     assert!(!timeline.items().await.is_empty());
 
     // Pinning a remote event fails.
-    setup.mock_response(ResponseTemplate::new(400)).await;
+    setup.mock_pin_unpin_response(ResponseTemplate::new(400)).await;
 
     let event_id = setup.event_id();
     assert!(timeline.pin_event(event_id).await.is_err());
@@ -680,7 +680,7 @@ async fn test_unpin_event_is_sent_successfully() {
 
     // Unpinning a remote event succeeds.
     setup
-        .mock_response(ResponseTemplate::new(200).set_body_json(json!({
+        .mock_pin_unpin_response(ResponseTemplate::new(200).set_body_json(json!({
             "event_id": "$42"
         })))
         .await;
@@ -714,7 +714,7 @@ async fn test_unpin_event_is_returning_an_error() {
     assert!(!timeline.items().await.is_empty());
 
     // Unpinning a remote event fails.
-    setup.mock_response(ResponseTemplate::new(400)).await;
+    setup.mock_pin_unpin_response(ResponseTemplate::new(400)).await;
 
     let event_id = setup.event_id();
     assert!(timeline.unpin_event(event_id).await.is_err());
@@ -834,7 +834,13 @@ impl PinningTestSetup<'_> {
         let _response = client.sync_once(sync_settings.clone()).await.unwrap();
         server.reset().await;
 
-        Self { event_id, room_id, client, server, sync_settings, sync_builder }
+        let setup = Self { event_id, room_id, client, server, sync_settings, sync_builder };
+
+        // This is necessary to get an empty list of pinned events when there are no
+        // pinned events state event in the required state
+        setup.mock_get_empty_pinned_events_state_response().await;
+
+        setup
     }
 
     async fn timeline(&self) -> Timeline {
@@ -847,7 +853,7 @@ impl PinningTestSetup<'_> {
         self.server.reset().await;
     }
 
-    async fn mock_response(&self, response: ResponseTemplate) {
+    async fn mock_pin_unpin_response(&self, response: ResponseTemplate) {
         Mock::given(method("PUT"))
             .and(path_regex(r"^/_matrix/client/r0/rooms/.*/state/m.room.pinned_events/.*?"))
             .and(header("authorization", "Bearer 1234"))
@@ -856,6 +862,15 @@ impl PinningTestSetup<'_> {
             .await;
     }
 
+    async fn mock_get_empty_pinned_events_state_response(&self) {
+        Mock::given(method("GET"))
+            .and(path_regex(r"^/_matrix/client/r0/rooms/.*/state/m.room.pinned_events/.*"))
+            .and(header("authorization", "Bearer 1234"))
+            .respond_with(ResponseTemplate::new(404).set_body_json(json!({})))
+            .mount(&self.server)
+            .await;
+    }
+
     async fn mock_sync(&mut self, is_using_pinned_state_event: bool) {
         let f = EventFactory::new().sender(user_id!("@a:b.c"));
         let mut joined_room_builder = JoinedRoomBuilder::new(self.room_id)

crates/matrix-sdk/src/room/mod.rs

@@ -30,6 +30,7 @@ use futures_util::{
     future::{try_join, try_join_all},
     stream::FuturesUnordered,
 };
+use http::StatusCode;
 #[cfg(all(feature = "e2e-encryption", not(target_arch = "wasm32")))]
 pub use identity_status_changes::IdentityStatusChanges;
 #[cfg(feature = "e2e-encryption")]
@@ -91,6 +92,7 @@ use ruma::{
                 VideoMessageEventContent,
             },
             name::RoomNameEventContent,
+            pinned_events::RoomPinnedEventsEventContent,
             power_levels::{RoomPowerLevels, RoomPowerLevelsEventContent},
             server_acl::RoomServerAclEventContent,
             topic::RoomTopicEventContent,
@@ -3130,6 +3132,32 @@ impl Room {
             .await?;
         Ok(())
     }
+
+    /// Load pinned state events for a room from the `/state` endpoint in the
+    /// home server.
+    pub async fn load_pinned_events(&self) -> Result<Option<Vec<OwnedEventId>>> {
+        let response = self
+            .client
+            .send(
+                get_state_events_for_key::v3::Request::new(
+                    self.room_id().to_owned(),
+                    StateEventType::RoomPinnedEvents,
+                    "".to_owned(),
+                ),
+                None,
+            )
+            .await;
+
+        match response {
+            Ok(response) => {
+                Ok(Some(response.content.deserialize_as::<RoomPinnedEventsEventContent>()?.pinned))
+            }
+            Err(http_error) => match http_error.as_client_api_error() {
+                Some(error) if error.status_code == StatusCode::NOT_FOUND => Ok(None),
+                _ => Err(http_error.into()),
+            },
+        }
+    }
 }
 
 #[cfg(all(feature = "e2e-encryption", not(target_arch = "wasm32")))]

crates/matrix-sdk/tests/integration/client.rs

@@ -1388,5 +1388,5 @@ async fn test_restore_room() {
 
     let room = client.get_room(room_id).unwrap();
     assert!(room.is_favourite());
-    assert!(!room.pinned_event_ids().is_empty());
+    assert!(!room.pinned_event_ids().unwrap().is_empty());
 }