fixup! contains several fixes for review comments, should be split into individual commits later

jmartinesp · jmartinesp · commit d9880bd75d32 · 2024-08-14T17:22:53.000+02:00
diff --git a/benchmarks/benches/room_bench.rs b/benchmarks/benches/room_bench.rs
@@ -1,4 +1,4 @@
-use std::time::Duration;
+use std::{sync::Arc, time::Duration};
 
 use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion, Throughput};
 use matrix_sdk::{
@@ -185,10 +185,17 @@ pub fn load_pinned_events_benchmark(c: &mut Criterion) {
     group.throughput(Throughput::Elements(count as u64));
     group.sample_size(10);
 
-    group.bench_function(BenchmarkId::new("load_pinned_events", name), |b| {
-        b.to_async(&runtime).iter(|| async {
+    let client = Arc::new(client);
+
+    {
+        let client = client.clone();
+        runtime.spawn_blocking(move || {
             client.event_cache().subscribe().unwrap();
+        });
+    }
 
+    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);
 
@@ -210,7 +217,7 @@ pub fn load_pinned_events_benchmark(c: &mut Criterion) {
     {
         let _guard = runtime.enter();
         runtime.block_on(server.reset());
-        drop(client);
+        // drop(client);
         drop(server);
     }
 
@@ -249,6 +256,7 @@ fn criterion() -> Criterion {
 criterion_group! {
     name = room;
     config = criterion();
-    targets = receive_all_members_benchmark, load_pinned_events_benchmark,
+    targets = load_pinned_events_benchmark,
+    // targets = receive_all_members_benchmark, load_pinned_events_benchmark,
 }
 criterion_main!(room);
diff --git a/crates/matrix-sdk-ui/src/timeline/pinned_events_loader.rs b/crates/matrix-sdk-ui/src/timeline/pinned_events_loader.rs
@@ -16,7 +16,7 @@ use std::{fmt::Formatter, sync::Arc};
 
 use futures_util::future::join_all;
 use matrix_sdk::{
-    config::RequestConfig, event_cache::paginator::PaginatorError, Room, SendOutsideWasm,
+    config::RequestConfig, event_cache::paginator::PaginatorError, Error, Room, SendOutsideWasm,
     SyncOutsideWasm,
 };
 use matrix_sdk_base::deserialized_responses::SyncTimelineEvent;
@@ -25,7 +25,9 @@ use thiserror::Error;
 use tokio::sync::Semaphore;
 use tracing::{debug, warn};
 
-use crate::timeline::event_handler::TimelineEventKind;
+use crate::timeline::{
+    event_handler::TimelineEventKind, pinned_events_loader::PinnedEventsLoaderError::EventNotFound,
+};
 
 const MAX_CONCURRENT_REQUESTS: usize = 10;
 
@@ -82,7 +84,11 @@ impl PinnedEventsLoader {
                     return None;
                 }
                 match provider.load_event_with_relations(&event_id, request_config).await {
-                    Ok(event) => Some(event),
+                    Ok((event, related_events)) => {
+                        let mut events = vec![event];
+                        events.extend(related_events);
+                        Some(events)
+                    }
                     Err(err) => {
                         warn!("error when loading pinned event: {err}");
                         None
@@ -114,12 +120,12 @@ impl PinnedEventsLoader {
 #[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)]
 pub trait PinnedEventsRoom: SendOutsideWasm + SyncOutsideWasm {
     /// Load a single room event from either the cache or network and any events
-    /// related to it, if cached.
+    /// related to it, if they are cached.
     async fn load_event_with_relations(
         &self,
         event_id: &EventId,
         request_config: Option<RequestConfig>,
-    ) -> Result<Vec<SyncTimelineEvent>, PaginatorError>;
+    ) -> Result<(SyncTimelineEvent, Vec<SyncTimelineEvent>), PaginatorError>;
 
     /// Get the pinned event ids for a room.
     fn pinned_event_ids(&self) -> Vec<OwnedEventId>;
@@ -154,19 +160,24 @@ impl PinnedEventsRoom for Room {
         &self,
         event_id: &EventId,
         request_config: Option<RequestConfig>,
-    ) -> Result<Vec<SyncTimelineEvent>, PaginatorError> {
+    ) -> Result<(SyncTimelineEvent, Vec<SyncTimelineEvent>), PaginatorError> {
         if let Ok((cache, _handles)) = self.event_cache().await {
-            let cached_event_with_relations = cache.event_with_relations(event_id).await;
-            if !cached_event_with_relations.is_empty() {
+            if let Some(ret) = cache.event_with_relations(event_id).await {
                 debug!("Loaded pinned event {event_id} and related events from cache");
-                return Ok(cached_event_with_relations);
+                return Ok(ret);
             }
         }
 
         debug!("Loading pinned event {event_id} from HS");
         self.event_with_context(event_id, true, uint!(0), request_config)
             .await
-            .map(|e| if let Some(ev) = e.event { vec![ev.into()] } else { Vec::new() })
+            .and_then(|e| {
+                if let Some(ev) = e.event {
+                    Ok((ev.into(), Vec::new()))
+                } else {
+                    Err(Error::UnknownError(Box::new(EventNotFound(event_id.to_owned()))))
+                }
+            })
             .map_err(|err| PaginatorError::SdkError(Box::new(err)))
     }
 
diff --git a/crates/matrix-sdk-ui/src/timeline/tests/mod.rs b/crates/matrix-sdk-ui/src/timeline/tests/mod.rs
@@ -326,7 +326,7 @@ impl PinnedEventsRoom for TestRoomDataProvider {
         &self,
         _event_id: &EventId,
         _request_config: Option<RequestConfig>,
-    ) -> Result<Vec<SyncTimelineEvent>, PaginatorError> {
+    ) -> Result<(SyncTimelineEvent, Vec<SyncTimelineEvent>), PaginatorError> {
         unimplemented!();
     }
 
diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/pinned_event.rs b/crates/matrix-sdk-ui/tests/integration/timeline/pinned_event.rs
@@ -367,10 +367,9 @@ async fn test_edited_events_are_reflected_in_sync() {
     test_helper.server.reset().await;
 
     // Load new pinned event contents from sync, where $2 is and edit on $1
-    let _ = Box::pin(
-        test_helper.setup_sync_response(vec![(edit("$2", "$1", "edited message!"), true)], None),
-    )
-    .await;
+    let _ = test_helper
+        .setup_sync_response(vec![(edit("$2", "$1", "edited message!"), true)], None)
+        .await;
 
     // The list is reloaded, so it's reset
     assert_matches!(timeline_stream.next().await.unwrap(), VectorDiff::Clear);
@@ -434,7 +433,7 @@ async fn test_redacted_events_are_reflected_in_sync() {
     test_helper.server.reset().await;
 
     // Load new pinned event contents from sync, where $1 is now redacted
-    let _ = Box::pin(test_helper.setup_sync_response(vec![(redact("$2", "$1"), true)], None)).await;
+    let _ = test_helper.setup_sync_response(vec![(redact("$2", "$1"), true)], None).await;
 
     // The list is reloaded, so it's reset
     assert_matches!(timeline_stream.next().await.unwrap(), VectorDiff::Clear);
@@ -494,10 +493,9 @@ async fn test_edited_events_survive_pinned_event_ids_change() {
 
     // Load new pinned event contents from sync, $2 was pinned but wasn't available
     // before
-    let _ = Box::pin(
-        test_helper.setup_sync_response(vec![(edit("$2", "$1", "edited message!"), true)], None),
-    )
-    .await;
+    let _ = test_helper
+        .setup_sync_response(vec![(edit("$2", "$1", "edited message!"), true)], None)
+        .await;
     test_helper.server.reset().await;
 
     // The list is reloaded, so it's reset
@@ -523,11 +521,9 @@ async fn test_edited_events_survive_pinned_event_ids_change() {
     assert_pending!(timeline_stream);
 
     // Load new pinned event contents from sync: $3
-    let _ = Box::pin(
-        test_helper
-            .setup_sync_response(vec![(msg("$3", "new message"), true)], Some(vec!["$1", "$3"])),
-    )
-    .await;
+    let _ = test_helper
+        .setup_sync_response(vec![(msg("$3", "new message"), true)], Some(vec!["$1", "$3"]))
+        .await;
     test_helper.server.reset().await;
 
     // New item gets added
@@ -604,18 +600,14 @@ impl TestHelper {
     ) -> Result<SyncResponse, matrix_sdk::Error> {
         let mut joined_room_builder = JoinedRoomBuilder::new(&self.room_id);
         for (message_type, add_to_timeline) in text_messages {
-            let (event_id, timeline_item) = match message_type {
+            let f = EventFactory::new().room(&self.room_id).sender(*BOB);
+            let (event_id, timeline_event) = match message_type {
                 MessageType::New { id, txt } => {
-                    let f = EventFactory::new().room(&self.room_id);
-                    let event_builder = f
-                        .text_msg(txt)
-                        .event_id(&id)
-                        .sender(*BOB)
-                        .server_ts(MilliSecondsSinceUnixEpoch::now());
+                    let event_builder =
+                        f.text_msg(txt).event_id(&id).server_ts(MilliSecondsSinceUnixEpoch::now());
                     (id, event_builder.into_timeline())
                 }
                 MessageType::Edit { id, old_id, txt } => {
-                    let f = EventFactory::new().room(&self.room_id);
                     let event_builder = f
                         .replacement_msg(txt, &old_id)
                         .event_id(&id)
@@ -624,7 +616,6 @@ impl TestHelper {
                     (id, event_builder.into_timeline())
                 }
                 MessageType::Redaction { id, old_id } => {
-                    let f = EventFactory::new().room(&self.room_id);
                     let event_builder = f
                         .redaction(&old_id)
                         .event_id(&id)
@@ -639,7 +630,7 @@ impl TestHelper {
                 &event_id,
                 None,
                 Vec::new(),
-                timeline_item.clone(),
+                timeline_event.clone(),
                 Vec::new(),
                 None,
                 Vec::new(),
@@ -648,7 +639,7 @@ impl TestHelper {
 
             if add_to_timeline {
                 joined_room_builder =
-                    joined_room_builder.add_timeline_event(timeline_item.event.cast());
+                    joined_room_builder.add_timeline_event(timeline_event.event.cast());
             }
         }
 
diff --git a/crates/matrix-sdk/src/event_cache/mod.rs b/crates/matrix-sdk/src/event_cache/mod.rs
diff --git a/crates/matrix-sdk/src/test_utils/events.rs b/crates/matrix-sdk/src/test_utils/events.rs

Original file line number	Diff line number	Diff line change
`@@ -326,7 +326,7 @@ impl PinnedEventsRoom for TestRoomDataProvider {`
`326`	`326`	`&self,`
`327`	`327`	`_event_id: &EventId,`
`328`	`328`	`_request_config: Option<RequestConfig>,`
`329`		`- ) -> Result<Vec<SyncTimelineEvent>, PaginatorError> {`
	`329`	`+ ) -> Result<(SyncTimelineEvent, Vec<SyncTimelineEvent>), PaginatorError> {`
`330`	`330`	`unimplemented!();`
`331`	`331`	`}`
`332`	`332`