From ee252437d1af6842bbbaa560c1ed3ffcf1dbbd12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Mon, 4 Nov 2024 13:26:25 +0100 Subject: [PATCH] fix(pinned_events): get pinned event ids from the HS if the sync doesn'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 | 10 ++++--- bindings/matrix-sdk-ffi/src/room_info.rs | 3 +- crates/matrix-sdk-base/src/rooms/normal.rs | 6 ++-- .../matrix-sdk-base/src/sliding_sync/mod.rs | 6 ++-- crates/matrix-sdk-ui/src/timeline/mod.rs | 28 +++++++++++++++---- .../src/timeline/pinned_events_loader.rs | 5 ++-- .../matrix-sdk-ui/src/timeline/tests/mod.rs | 2 +- .../tests/integration/timeline/mod.rs | 27 ++++++++++++++---- crates/matrix-sdk/src/room/mod.rs | 28 +++++++++++++++++++ crates/matrix-sdk/tests/integration/client.rs | 2 +- 10 files changed, 90 insertions(+), 27 deletions(-) diff --git a/benchmarks/benches/room_bench.rs b/benchmarks/benches/room_bench.rs index 5b010b6ee31..2283cc955ab 100644 --- a/benchmarks/benches/room_bench.rs +++ b/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; diff --git a/bindings/matrix-sdk-ffi/src/room_info.rs b/bindings/matrix-sdk-ffi/src/room_info.rs index 3f04eed962b..96de331fd10 100644 --- a/bindings/matrix-sdk-ffi/src/room_info.rs +++ b/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(), diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index cfe1625b7af..7181d4d4824 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/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 { + pub fn pinned_event_ids(&self) -> Option> { 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 { - self.base_info.pinned_events.clone().map(|c| c.pinned).unwrap_or_default() + pub fn pinned_event_ids(&self) -> Option> { + self.base_info.pinned_events.clone().map(|c| c.pinned) } /// Checks if an `EventId` is currently pinned. diff --git a/crates/matrix-sdk-base/src/sliding_sync/mod.rs b/crates/matrix-sdk-base/src/sliding_sync/mod.rs index f613d177b6e..9350492a098 100644 --- a/crates/matrix-sdk-base/src/sliding_sync/mod.rs +++ b/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()); } diff --git a/crates/matrix-sdk-ui/src/timeline/mod.rs b/crates/matrix-sdk-ui/src/timeline/mod.rs index 93168e1732a..b5bd98b10c2 100644 --- a/crates/matrix-sdk-ui/src/timeline/mod.rs +++ b/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 { - 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 { - 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); diff --git a/crates/matrix-sdk-ui/src/timeline/pinned_events_loader.rs b/crates/matrix-sdk-ui/src/timeline/pinned_events_loader.rs index 450ccae0e94..1deb853607d 100644 --- a/crates/matrix-sdk-ui/src/timeline/pinned_events_loader.rs +++ b/crates/matrix-sdk-ui/src/timeline/pinned_events_loader.rs @@ -61,6 +61,7 @@ impl PinnedEventsLoader { let pinned_event_ids: Vec = 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), PaginatorError>>; /// Get the pinned event ids for a room. - fn pinned_event_ids(&self) -> Vec; + fn pinned_event_ids(&self) -> Option>; /// 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 { + fn pinned_event_ids(&self) -> Option> { self.clone_info().pinned_event_ids() } diff --git a/crates/matrix-sdk-ui/src/timeline/tests/mod.rs b/crates/matrix-sdk-ui/src/timeline/tests/mod.rs index 26cadd3b6dc..356a2ad2228 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/mod.rs @@ -351,7 +351,7 @@ impl PinnedEventsRoom for TestRoomDataProvider { unimplemented!(); } - fn pinned_event_ids(&self) -> Vec { + fn pinned_event_ids(&self) -> Option> { unimplemented!(); } diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs b/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs index e9d28a84e3a..b6bc4211140 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs +++ b/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) diff --git a/crates/matrix-sdk/src/room/mod.rs b/crates/matrix-sdk/src/room/mod.rs index 0be442f9167..f974b2ec769 100644 --- a/crates/matrix-sdk/src/room/mod.rs +++ b/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>> { + 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::()?.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")))] diff --git a/crates/matrix-sdk/tests/integration/client.rs b/crates/matrix-sdk/tests/integration/client.rs index 6e044c28567..589087296a5 100644 --- a/crates/matrix-sdk/tests/integration/client.rs +++ b/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()); }