diff --git a/crates/matrix-sdk-ui/src/timeline/inner/mod.rs b/crates/matrix-sdk-ui/src/timeline/inner/mod.rs index 1d35db415d6..e34f7db3b13 100644 --- a/crates/matrix-sdk-ui/src/timeline/inner/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/inner/mod.rs @@ -82,7 +82,7 @@ mod state; /// Data associated to the current timeline focus. #[derive(Debug)] -enum TimelineFocusData { +enum TimelineFocusData { /// The timeline receives live events from the sync. Live, @@ -92,7 +92,7 @@ enum TimelineFocusData { /// The event id we've started to focus on. event_id: OwnedEventId, /// The paginator instance. - paginator: Paginator, + paginator: Paginator

, /// Number of context events to request for the first request. num_context_events: u16, }, @@ -108,7 +108,7 @@ pub(super) struct TimelineInner { state: Arc>, /// Inner mutable focus state. - focus: Arc>, + focus: Arc>>, /// A [`RoomDataProvider`] implementation, providing data. /// @@ -240,7 +240,7 @@ impl TimelineInner

{ let (focus_data, is_live) = match focus { TimelineFocus::Live => (TimelineFocusData::Live, LiveTimelineUpdatesAllowed::All), TimelineFocus::Event { target, num_context_events } => { - let paginator = Paginator::new(Box::new(room_data_provider.clone())); + let paginator = Paginator::new(room_data_provider.clone()); ( TimelineFocusData::Event { paginator, event_id: target, num_context_events }, LiveTimelineUpdatesAllowed::None, diff --git a/crates/matrix-sdk-ui/src/timeline/tests/mod.rs b/crates/matrix-sdk-ui/src/timeline/tests/mod.rs index 28f7512381f..2d587c21191 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/mod.rs @@ -16,6 +16,7 @@ use std::{ collections::{BTreeMap, HashMap}, + future::Future, sync::Arc, }; @@ -303,20 +304,21 @@ impl TestRoomDataProvider { } } -#[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))] -#[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)] impl PaginableRoom for TestRoomDataProvider { - async fn event_with_context( + fn event_with_context( &self, _event_id: &EventId, _lazy_load_members: bool, _num_events: UInt, - ) -> Result { - unimplemented!(); + ) -> impl Future> { + async { unimplemented!() } } - async fn messages(&self, _opts: MessagesOptions) -> Result { - unimplemented!(); + fn messages( + &self, + _opts: MessagesOptions, + ) -> impl Future> { + async { unimplemented!() } } } diff --git a/crates/matrix-sdk/src/event_cache/mod.rs b/crates/matrix-sdk/src/event_cache/mod.rs index 1641d943187..b85b3b2c30c 100644 --- a/crates/matrix-sdk/src/event_cache/mod.rs +++ b/crates/matrix-sdk/src/event_cache/mod.rs @@ -543,7 +543,7 @@ struct RoomEventCacheInner { /// /// It's protected behind a lock to avoid multiple accesses to the paginator /// at the same time. - pagination: RoomPaginationData, + pagination: RoomPaginationData, } impl RoomEventCacheInner { @@ -560,7 +560,7 @@ impl RoomEventCacheInner { all_events, sender, pagination: RoomPaginationData { - paginator: Paginator::new(Box::new(weak_room)), + paginator: Paginator::new(weak_room), waited_for_initial_prev_token: Mutex::new(false), token_notifier: Default::default(), }, diff --git a/crates/matrix-sdk/src/event_cache/pagination.rs b/crates/matrix-sdk/src/event_cache/pagination.rs index a24af594094..e81fdad8879 100644 --- a/crates/matrix-sdk/src/event_cache/pagination.rs +++ b/crates/matrix-sdk/src/event_cache/pagination.rs @@ -25,19 +25,19 @@ use tokio::{ use tracing::{debug, instrument, trace}; use super::{ - paginator::{PaginationResult, Paginator, PaginatorState}, + paginator::{PaginableRoom, PaginationResult, Paginator, PaginatorState}, store::Gap, BackPaginationOutcome, Result, RoomEventCacheInner, }; use crate::event_cache::{linked_chunk::ChunkContent, store::RoomEvents}; #[derive(Debug)] -pub(super) struct RoomPaginationData { +pub(super) struct RoomPaginationData { /// A notifier that we received a new pagination token. pub token_notifier: Notify, /// The stateful paginator instance used for the integrated pagination. - pub paginator: Paginator, + pub paginator: Paginator, /// Have we ever waited for a previous-batch-token to come from sync? We do /// this at most once per room, the first time we try to run backward diff --git a/crates/matrix-sdk/src/event_cache/paginator.rs b/crates/matrix-sdk/src/event_cache/paginator.rs index 42d82fb2a2f..d84fc25f0e0 100644 --- a/crates/matrix-sdk/src/event_cache/paginator.rs +++ b/crates/matrix-sdk/src/event_cache/paginator.rs @@ -17,7 +17,7 @@ //! makes it possible to paginate forward or backward, from that event, until //! one end of the timeline (front or back) is reached. -use std::sync::Mutex; +use std::{future::Future, sync::Mutex}; use eyeball::{SharedObservable, Subscriber}; use matrix_sdk_base::{deserialized_responses::TimelineEvent, SendOutsideWasm, SyncOutsideWasm}; @@ -94,9 +94,9 @@ impl From> for PaginationToken { /// forward from it. /// /// See also the module-level documentation. -pub struct Paginator { +pub struct Paginator { /// The room in which we're going to run the pagination. - room: Box, + room: PR, /// Current state of the paginator. state: SharedObservable, @@ -113,7 +113,7 @@ pub struct Paginator { } #[cfg(not(tarpaulin_include))] -impl std::fmt::Debug for Paginator { +impl std::fmt::Debug for Paginator { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { // Don't include the room in the debug output. f.debug_struct("Paginator") @@ -186,9 +186,9 @@ impl Drop for ResetStateGuard { } } -impl Paginator { +impl Paginator { /// Create a new [`Paginator`], given a room implementation. - pub fn new(room: Box) -> Self { + pub fn new(room: PR) -> Self { Self { room, state: SharedObservable::new(PaginatorState::Initial), @@ -431,8 +431,6 @@ impl Paginator { /// /// Not [`crate::Room`] because we may want to paginate rooms we don't belong /// to. -#[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))] -#[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)] pub trait PaginableRoom: SendOutsideWasm + SyncOutsideWasm { /// Runs a /context query for the given room. /// @@ -440,87 +438,113 @@ pub trait PaginableRoom: SendOutsideWasm + SyncOutsideWasm { /// /// - `event_id` is the identifier of the target event. /// - `lazy_load_members` controls whether room membership events are lazily - /// loaded as context - /// state events. + /// loaded as context state events. /// - `num_events` is the number of events (including the fetched event) to - /// return as context. + /// return as context. /// /// ## Returns /// /// Must return [`PaginatorError::EventNotFound`] whenever the target event /// could not be found, instead of causing an http `Err` result. - async fn event_with_context( + fn event_with_context( &self, event_id: &EventId, lazy_load_members: bool, num_events: UInt, - ) -> Result; + ) -> impl Future>; /// Runs a /messages query for the given room. - async fn messages(&self, opts: MessagesOptions) -> Result; + fn messages( + &self, + opts: MessagesOptions, + ) -> impl Future>; } -#[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))] -#[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)] impl PaginableRoom for Room { - async fn event_with_context( + // This is written as a function returning a Future to avoid a performance + // pitfall of rustc when using async_trait. See https://github.com/matrix-org/matrix-rust-sdk/pull/3880 for + // details. + #[allow(clippy::manual_async_fn)] + fn event_with_context( &self, event_id: &EventId, lazy_load_members: bool, num_events: UInt, - ) -> Result { - let response = match self.event_with_context(event_id, lazy_load_members, num_events).await - { - Ok(result) => result, - - Err(err) => { - // If the error was a 404, then the event wasn't found on the server; special - // case this to make it easy to react to such an error. - if let Some(error) = err.as_client_api_error() { - if error.status_code == 404 { - // Event not found - return Err(PaginatorError::EventNotFound(event_id.to_owned())); + ) -> impl Future> { + async move { + let response = + match self.event_with_context(event_id, lazy_load_members, num_events).await { + Ok(result) => result, + + Err(err) => { + // If the error was a 404, then the event wasn't found on the server; + // special case this to make it easy to react to + // such an error. + if let Some(error) = err.as_client_api_error() { + if error.status_code == 404 { + // Event not found + return Err(PaginatorError::EventNotFound(event_id.to_owned())); + } + } + + // Otherwise, just return a wrapped error. + return Err(PaginatorError::SdkError(Box::new(err))); } - } - - // Otherwise, just return a wrapped error. - return Err(PaginatorError::SdkError(Box::new(err))); - } - }; + }; - Ok(response) + Ok(response) + } } - async fn messages(&self, opts: MessagesOptions) -> Result { - self.messages(opts).await.map_err(|err| PaginatorError::SdkError(Box::new(err))) + // This is written as a function returning a Future to avoid a performance + // pitfall of rustc when using async_trait. See https://github.com/matrix-org/matrix-rust-sdk/pull/3880 for + // details. + #[allow(clippy::manual_async_fn)] + fn messages( + &self, + opts: MessagesOptions, + ) -> impl Future> { + async move { self.messages(opts).await.map_err(|err| PaginatorError::SdkError(Box::new(err))) } } } -#[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))] -#[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)] impl PaginableRoom for WeakRoom { - async fn event_with_context( + // This is written as a function returning a Future to avoid a performance + // pitfall of rustc when using async_trait. See https://github.com/matrix-org/matrix-rust-sdk/pull/3880 for + // details. + #[allow(clippy::manual_async_fn)] + fn event_with_context( &self, event_id: &EventId, lazy_load_members: bool, num_events: UInt, - ) -> Result { - let Some(room) = self.get() else { - // Client is shutting down, return a default response. - return Ok(EventWithContextResponse::default()); - }; + ) -> impl Future> { + async move { + let Some(room) = self.get() else { + // Client is shutting down, return a default response. + return Ok(EventWithContextResponse::default()); + }; - PaginableRoom::event_with_context(&room, event_id, lazy_load_members, num_events).await + PaginableRoom::event_with_context(&room, event_id, lazy_load_members, num_events).await + } } - /// Runs a /messages query for the given room. - async fn messages(&self, opts: MessagesOptions) -> Result { - let Some(room) = self.get() else { - // Client is shutting down, return a default response. - return Ok(Messages::default()); - }; + // This is written as a function returning a Future to avoid a performance + // pitfall of rustc when using async_trait. See https://github.com/matrix-org/matrix-rust-sdk/pull/3880 for + // details. + #[allow(clippy::manual_async_fn)] + fn messages( + &self, + opts: MessagesOptions, + ) -> impl Future> { + async move { + let Some(room) = self.get() else { + // Client is shutting down, return a default response. + return Ok(Messages::default()); + }; - PaginableRoom::messages(&room, opts).await + PaginableRoom::messages(&room, opts).await + } } } @@ -529,7 +553,6 @@ mod tests { use std::sync::Arc; use assert_matches2::assert_let; - use async_trait::async_trait; use futures_core::Future; use futures_util::FutureExt as _; use matrix_sdk_base::deserialized_responses::TimelineEvent; @@ -589,97 +612,103 @@ mod tests { static ROOM_ID: Lazy<&RoomId> = Lazy::new(|| room_id!("!dune:herbert.org")); static USER_ID: Lazy<&UserId> = Lazy::new(|| user_id!("@paul:atreid.es")); - #[async_trait] impl PaginableRoom for TestRoom { - async fn event_with_context( + fn event_with_context( &self, event_id: &EventId, _lazy_load_members: bool, num_events: UInt, - ) -> Result { - // Wait for the room to be marked as ready first. - if self.wait_for_ready { - self.room_ready.notified().await; - } - - let event = self - .event_factory - .text_msg(self.target_event_text.lock().await.clone()) - .event_id(event_id) - .into_timeline(); - - // Properly simulate `num_events`: take either the closest num_events events - // before, or use all of the before events and then consume after events. - let mut num_events = u64::from(num_events) as usize; - - let prev_events = self.prev_events.lock().await; - - let events_before = if prev_events.is_empty() { - Vec::new() - } else { - let len = prev_events.len(); - let take_before = num_events.min(len); - // Subtract is safe because take_before <= num_events. - num_events -= take_before; - // Subtract is safe because take_before <= len - prev_events[len - take_before..len].to_vec() - }; - - let events_after = self.next_events.lock().await; - let events_after = if events_after.is_empty() { - Vec::new() - } else { - events_after[0..num_events.min(events_after.len())].to_vec() - }; - - return Ok(EventWithContextResponse { - event: Some(event), - events_before, - events_after, - prev_batch_token: self.prev_batch_token.lock().await.clone(), - next_batch_token: self.next_batch_token.lock().await.clone(), - state: Vec::new(), - }); - } + ) -> impl Future> { + async move { + // Wait for the room to be marked as ready first. + if self.wait_for_ready { + self.room_ready.notified().await; + } - async fn messages(&self, opts: MessagesOptions) -> Result { - if self.wait_for_ready { - self.room_ready.notified().await; + let event = self + .event_factory + .text_msg(self.target_event_text.lock().await.clone()) + .event_id(event_id) + .into_timeline(); + + // Properly simulate `num_events`: take either the closest num_events events + // before, or use all of the before events and then consume after events. + let mut num_events = u64::from(num_events) as usize; + + let prev_events = self.prev_events.lock().await; + + let events_before = if prev_events.is_empty() { + Vec::new() + } else { + let len = prev_events.len(); + let take_before = num_events.min(len); + // Subtract is safe because take_before <= num_events. + num_events -= take_before; + // Subtract is safe because take_before <= len + prev_events[len - take_before..len].to_vec() + }; + + let events_after = self.next_events.lock().await; + let events_after = if events_after.is_empty() { + Vec::new() + } else { + events_after[0..num_events.min(events_after.len())].to_vec() + }; + + return Ok(EventWithContextResponse { + event: Some(event), + events_before, + events_after, + prev_batch_token: self.prev_batch_token.lock().await.clone(), + next_batch_token: self.next_batch_token.lock().await.clone(), + state: Vec::new(), + }); } + } - let limit = u64::from(opts.limit) as usize; - - let (end, events) = match opts.dir { - Direction::Backward => { - let events = self.prev_events.lock().await; - let events = if events.is_empty() { - Vec::new() - } else { - let len = events.len(); - let take_before = limit.min(len); - // Subtract is safe because take_before <= len - events[len - take_before..len].to_vec() - }; - (self.prev_batch_token.lock().await.clone(), events) + fn messages( + &self, + opts: MessagesOptions, + ) -> impl Future> { + async move { + if self.wait_for_ready { + self.room_ready.notified().await; } - Direction::Forward => { - let events = self.next_events.lock().await; - let events = if events.is_empty() { - Vec::new() - } else { - events[0..limit.min(events.len())].to_vec() - }; - (self.next_batch_token.lock().await.clone(), events) - } - }; + let limit = u64::from(opts.limit) as usize; + + let (end, events) = match opts.dir { + Direction::Backward => { + let events = self.prev_events.lock().await; + let events = if events.is_empty() { + Vec::new() + } else { + let len = events.len(); + let take_before = limit.min(len); + // Subtract is safe because take_before <= len + events[len - take_before..len].to_vec() + }; + (self.prev_batch_token.lock().await.clone(), events) + } - return Ok(Messages { - start: opts.from.unwrap(), - end, - chunk: events, - state: Vec::new(), - }); + Direction::Forward => { + let events = self.next_events.lock().await; + let events = if events.is_empty() { + Vec::new() + } else { + events[0..limit.min(events.len())].to_vec() + }; + (self.next_batch_token.lock().await.clone(), events) + } + }; + + return Ok(Messages { + start: opts.from.unwrap(), + end, + chunk: events, + state: Vec::new(), + }); + } } } @@ -701,7 +730,7 @@ mod tests { #[async_test] async fn test_start_from() { // Prepare test data. - let room = Box::new(TestRoom::new(false, *ROOM_ID, *USER_ID)); + let room = TestRoom::new(false, *ROOM_ID, *USER_ID); let event_id = event_id!("$yoyoyo"); let event_factory = &room.event_factory; @@ -749,7 +778,7 @@ mod tests { #[async_test] async fn test_start_from_with_num_events() { // Prepare test data. - let room = Box::new(TestRoom::new(false, *ROOM_ID, *USER_ID)); + let room = TestRoom::new(false, *ROOM_ID, *USER_ID); let event_id = event_id!("$yoyoyo"); let event_factory = &room.event_factory; @@ -780,7 +809,7 @@ mod tests { #[async_test] async fn test_paginate_backward() { // Prepare test data. - let room = Box::new(TestRoom::new(false, *ROOM_ID, *USER_ID)); + let room = TestRoom::new(false, *ROOM_ID, *USER_ID); let event_id = event_id!("$yoyoyo"); let event_factory = &room.event_factory; @@ -852,7 +881,7 @@ mod tests { #[async_test] async fn test_paginate_backward_with_limit() { // Prepare test data. - let room = Box::new(TestRoom::new(false, *ROOM_ID, *USER_ID)); + let room = TestRoom::new(false, *ROOM_ID, *USER_ID); let event_id = event_id!("$yoyoyo"); let event_factory = &room.event_factory; @@ -896,7 +925,7 @@ mod tests { #[async_test] async fn test_paginate_forward() { // Prepare test data. - let room = Box::new(TestRoom::new(false, *ROOM_ID, *USER_ID)); + let room = TestRoom::new(false, *ROOM_ID, *USER_ID); let event_id = event_id!("$yoyoyo"); let event_factory = &room.event_factory; @@ -967,7 +996,7 @@ mod tests { #[async_test] async fn test_state() { - let room = Box::new(TestRoom::new(true, *ROOM_ID, *USER_ID)); + let room = TestRoom::new(true, *ROOM_ID, *USER_ID); *room.prev_batch_token.lock().await = Some("prev".to_owned()); *room.next_batch_token.lock().await = Some("next".to_owned()); @@ -1089,25 +1118,28 @@ mod tests { } } - #[async_trait] impl PaginableRoom for AbortingRoom { - async fn event_with_context( + fn event_with_context( &self, _event_id: &EventId, _lazy_load_members: bool, _num_events: UInt, - ) -> Result { - self.wait_abort_and_yield().await + ) -> impl Future> + { + async { self.wait_abort_and_yield().await } } - async fn messages(&self, _opts: MessagesOptions) -> Result { - self.wait_abort_and_yield().await + fn messages( + &self, + _opts: MessagesOptions, + ) -> impl Future> { + async { self.wait_abort_and_yield().await } } } #[async_test] async fn test_abort_while_starting_from() { - let room = Box::new(AbortingRoom::default()); + let room = AbortingRoom::default(); let paginator = Arc::new(Paginator::new(room.clone())); @@ -1140,7 +1172,7 @@ mod tests { #[async_test] async fn test_abort_while_paginating() { - let room = Box::new(AbortingRoom::default()); + let room = AbortingRoom::default(); // Assuming a paginator ready to back- or forward- paginate, let paginator = Paginator::new(room.clone()); diff --git a/crates/matrix-sdk/src/room/edit.rs b/crates/matrix-sdk/src/room/edit.rs index 9718b1e80f0..ff8cc1de313 100644 --- a/crates/matrix-sdk/src/room/edit.rs +++ b/crates/matrix-sdk/src/room/edit.rs @@ -14,6 +14,8 @@ //! Facilities to edit existing events. +use std::future::Future; + use matrix_sdk_base::deserialized_responses::SyncTimelineEvent; use ruma::{ events::{ @@ -88,34 +90,42 @@ impl Room { } } -#[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))] -#[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)] trait EventSource { - async fn get_event(&self, event_id: &EventId) -> Result; + fn get_event( + &self, + event_id: &EventId, + ) -> impl Future>; } -#[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))] -#[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)] impl<'a> EventSource for &'a Room { - async fn get_event(&self, event_id: &EventId) -> Result { - match self.event_cache().await { - Ok((event_cache, _drop_handles)) => { - if let Some(event) = event_cache.event(event_id).await { - return Ok(event); + // This is written as a function returning a Future to avoid a performance + // pitfall of rustc when using async_trait. See https://github.com/matrix-org/matrix-rust-sdk/pull/3880 for + // details. + #[allow(clippy::manual_async_fn)] + fn get_event( + &self, + event_id: &EventId, + ) -> impl Future> { + async { + match self.event_cache().await { + Ok((event_cache, _drop_handles)) => { + if let Some(event) = event_cache.event(event_id).await { + return Ok(event); + } + // Fallthrough: try with /event. } - // Fallthrough: try with /event. - } - Err(err) => { - debug!("error when getting the event cache: {err}"); + Err(err) => { + debug!("error when getting the event cache: {err}"); + } } - } - trace!("trying with /event now"); - self.event(event_id, None) - .await - .map(Into::into) - .map_err(|err| EditError::Fetch(Box::new(err))) + trace!("trying with /event now"); + self.event(event_id, None) + .await + .map(Into::into) + .map_err(|err| EditError::Fetch(Box::new(err))) + } } } @@ -200,7 +210,10 @@ async fn make_edit_event( #[cfg(test)] mod tests { - use std::collections::BTreeMap; + use std::{ + collections::BTreeMap, + future::{ready, Future}, + }; use assert_matches2::{assert_let, assert_matches}; use matrix_sdk_base::deserialized_responses::SyncTimelineEvent; @@ -225,11 +238,12 @@ mod tests { events: BTreeMap, } - #[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))] - #[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)] impl EventSource for TestEventCache { - async fn get_event(&self, event_id: &EventId) -> Result { - Ok(self.events.get(event_id).unwrap().clone()) + fn get_event( + &self, + event_id: &EventId, + ) -> impl Future> { + ready(Ok(self.events.get(event_id).unwrap().clone())) } }