Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(pinned_events): get pinned event ids from the HS as fallback #4205

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions benchmarks/benches/room_bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion bindings/matrix-sdk-ffi/src/room_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
6 changes: 3 additions & 3 deletions crates/matrix-sdk-base/src/rooms/normal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}
Expand Down Expand Up @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions crates/matrix-sdk-base/src/sliding_sync/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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);

Expand All @@ -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());
}

Expand Down
28 changes: 22 additions & 6 deletions crates/matrix-sdk-ui/src/timeline/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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);
Expand Down
5 changes: 3 additions & 2 deletions crates/matrix-sdk-ui/src/timeline/pinned_events_loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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.
///
Expand Down Expand Up @@ -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()
}

Expand Down
2 changes: 1 addition & 1 deletion crates/matrix-sdk-ui/src/timeline/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!();
}

Expand Down
27 changes: 21 additions & 6 deletions crates/matrix-sdk-ui/tests/integration/timeline/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand All @@ -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;
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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 {
Expand All @@ -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"))
Expand All @@ -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)
Expand Down
28 changes: 28 additions & 0 deletions crates/matrix-sdk/src/room/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down Expand Up @@ -91,6 +92,7 @@ use ruma::{
VideoMessageEventContent,
},
name::RoomNameEventContent,
pinned_events::RoomPinnedEventsEventContent,
power_levels::{RoomPowerLevels, RoomPowerLevelsEventContent},
server_acl::RoomServerAclEventContent,
topic::RoomTopicEventContent,
Expand Down Expand Up @@ -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")))]
Expand Down
2 changes: 1 addition & 1 deletion crates/matrix-sdk/tests/integration/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Loading