refactor(ui): Use an iterator instead of Vec to represent events.

This patch changes `TimelineStateTransaction::add_remote_events_at` to take an `IntoIterator<Item = Into<SyncTimelineEvent>>` for `events`. In the current code, it saves one `iter().map(Into::into).collect::<Vec<_>>()`, but it will save another one when we will support `VectorDiff`s coming from the `EventCache`. It also avoids to allocate a vector to pass new events (this mostly happens in the test, but it can happen in real life).
matrix-org · Dec 2, 2024 · ad3d1fb · ad3d1fb
1 parent d2fecb6
commit ad3d1fb
Show file tree

Hide file tree

Showing 9 changed files with 88 additions and 47 deletions.
diff --git a/crates/matrix-sdk-ui/src/timeline/builder.rs b/crates/matrix-sdk-ui/src/timeline/builder.rs
@@ -183,7 +183,7 @@ impl TimelineBuilder {
                         if let Ok(events) = inner.reload_pinned_events().await {
                             inner
                                 .replace_with_initial_remote_events(
-                                    events,
+                                    events.into_iter(),
                                     RemoteEventOrigin::Pagination,
                                 )
                                 .await;
@@ -238,7 +238,7 @@ impl TimelineBuilder {
                             // current timeline.
                             match room_event_cache.subscribe().await {
                                 Ok((events, _)) => {
-                                    inner.replace_with_initial_remote_events(events, RemoteEventOrigin::Sync).await;
+                                    inner.replace_with_initial_remote_events(events.into_iter(), RemoteEventOrigin::Sync).await;
                                 }
                                 Err(err) => {
                                     warn!("Error when re-inserting initial events into the timeline: {err}");
@@ -272,7 +272,7 @@ impl TimelineBuilder {
                             trace!("Received new timeline events.");
 
                             inner.add_events_at(
-                                events,
+                                events.into_iter(),
                                 TimelineNewItemPosition::End {                                    origin: match origin {
                                         EventsOrigin::Sync => RemoteEventOrigin::Sync,
                                     }

diff --git a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs
@@ -303,7 +303,11 @@ impl<P: RoomDataProvider> TimelineController<P> {
 
                 let has_events = !events.is_empty();
 
-                self.replace_with_initial_remote_events(events, RemoteEventOrigin::Cache).await;
+                self.replace_with_initial_remote_events(
+                    events.into_iter(),
+                    RemoteEventOrigin::Cache,
+                )
+                .await;
 
                 Ok(has_events)
             }
@@ -320,7 +324,7 @@ impl<P: RoomDataProvider> TimelineController<P> {
                 let has_events = !start_from_result.events.is_empty();
 
                 self.replace_with_initial_remote_events(
-                    start_from_result.events.into_iter().map(Into::into).collect(),
+                    start_from_result.events.into_iter(),
                     RemoteEventOrigin::Pagination,
                 )
                 .await;
@@ -336,7 +340,7 @@ impl<P: RoomDataProvider> TimelineController<P> {
                 let has_events = !loaded_events.is_empty();
 
                 self.replace_with_initial_remote_events(
-                    loaded_events,
+                    loaded_events.into_iter(),
                     RemoteEventOrigin::Pagination,
                 )
                 .await;
@@ -405,7 +409,7 @@ impl<P: RoomDataProvider> TimelineController<P> {
         };
 
         self.add_events_at(
-            pagination.events,
+            pagination.events.into_iter(),
             TimelineNewItemPosition::Start { origin: RemoteEventOrigin::Pagination },
         )
         .await;
@@ -432,7 +436,7 @@ impl<P: RoomDataProvider> TimelineController<P> {
         };
 
         self.add_events_at(
-            pagination.events,
+            pagination.events.into_iter(),
             TimelineNewItemPosition::End { origin: RemoteEventOrigin::Pagination },
         )
         .await;
@@ -632,12 +636,16 @@ impl<P: RoomDataProvider> TimelineController<P> {
     /// is the most recent.
     ///
     /// Returns the number of timeline updates that were made.
-    pub(super) async fn add_events_at(
+    pub(super) async fn add_events_at<Events>(
         &self,
-        events: Vec<impl Into<SyncTimelineEvent>>,
+        events: Events,
         position: TimelineNewItemPosition,
-    ) -> HandleManyEventsResult {
-        if events.is_empty() {
+    ) -> HandleManyEventsResult
+    where
+        Events: IntoIterator + ExactSizeIterator,
+        <Events as IntoIterator>::Item: Into<SyncTimelineEvent>,
+    {
+        if events.len() == 0 {
             return Default::default();
         }
 
@@ -656,11 +664,14 @@ impl<P: RoomDataProvider> TimelineController<P> {
     ///
     /// This is all done with a single lock guard, since we don't want the state
     /// to be modified between the clear and re-insertion of new events.
-    pub(super) async fn replace_with_initial_remote_events(
+    pub(super) async fn replace_with_initial_remote_events<Events>(
         &self,
-        events: Vec<SyncTimelineEvent>,
+        events: Events,
         origin: RemoteEventOrigin,
-    ) {
+    ) where
+        Events: IntoIterator + ExactSizeIterator,
+        <Events as IntoIterator>::Item: Into<SyncTimelineEvent>,
+    {
         let mut state = self.state.write().await;
 
         let track_read_markers = self.settings.track_read_receipts;
@@ -676,7 +687,7 @@ impl<P: RoomDataProvider> TimelineController<P> {
         // Previously we just had to check the new one wasn't empty because
         // we did a clear operation before so the current one would always be empty, but
         // now we may want to replace a populated timeline with an empty one.
-        if !state.items.is_empty() || !events.is_empty() {
+        if !state.items.is_empty() || events.len() > 0 {
             state
                 .replace_with_remote_events(
                     events,

diff --git a/crates/matrix-sdk-ui/src/timeline/controller/state.rs b/crates/matrix-sdk-ui/src/timeline/controller/state.rs
@@ -127,14 +127,19 @@ impl TimelineState {
     /// should be ordered in *reverse* topological order, that is, `events[0]`
     /// is the most recent.
     #[tracing::instrument(skip(self, events, room_data_provider, settings))]
-    pub(super) async fn add_remote_events_at<P: RoomDataProvider>(
+    pub(super) async fn add_remote_events_at<Events, RoomData>(
         &mut self,
-        events: Vec<impl Into<SyncTimelineEvent>>,
+        events: Events,
         position: TimelineNewItemPosition,
-        room_data_provider: &P,
+        room_data_provider: &RoomData,
         settings: &TimelineSettings,
-    ) -> HandleManyEventsResult {
-        if events.is_empty() {
+    ) -> HandleManyEventsResult
+    where
+        Events: IntoIterator + ExactSizeIterator,
+        <Events as IntoIterator>::Item: Into<SyncTimelineEvent>,
+        RoomData: RoomDataProvider,
+    {
+        if events.len() == 0 {
             return Default::default();
         }
 
@@ -292,13 +297,18 @@ impl TimelineState {
     /// Note: when the `position` is [`TimelineEnd::Front`], prepended events
     /// should be ordered in *reverse* topological order, that is, `events[0]`
     /// is the most recent.
-    pub(super) async fn replace_with_remote_events<P: RoomDataProvider>(
+    pub(super) async fn replace_with_remote_events<Events, RoomData>(
         &mut self,
-        events: Vec<SyncTimelineEvent>,
+        events: Events,
         position: TimelineNewItemPosition,
-        room_data_provider: &P,
+        room_data_provider: &RoomData,
         settings: &TimelineSettings,
-    ) -> HandleManyEventsResult {
+    ) -> HandleManyEventsResult
+    where
+        Events: IntoIterator,
+        Events::Item: Into<SyncTimelineEvent>,
+        RoomData: RoomDataProvider,
+    {
         let mut txn = self.transaction();
         txn.clear();
         let result = txn.add_remote_events_at(events, position, room_data_provider, settings).await;
@@ -352,13 +362,18 @@ impl TimelineStateTransaction<'_> {
     /// should be ordered in *reverse* topological order, that is, `events[0]`
     /// is the most recent.
     #[tracing::instrument(skip(self, events, room_data_provider, settings))]
-    pub(super) async fn add_remote_events_at<P: RoomDataProvider>(
+    pub(super) async fn add_remote_events_at<Events, RoomData>(
         &mut self,
-        events: Vec<impl Into<SyncTimelineEvent>>,
+        events: Events,
         position: TimelineNewItemPosition,
-        room_data_provider: &P,
+        room_data_provider: &RoomData,
         settings: &TimelineSettings,
-    ) -> HandleManyEventsResult {
+    ) -> HandleManyEventsResult
+    where
+        Events: IntoIterator,
+        Events::Item: Into<SyncTimelineEvent>,
+        RoomData: RoomDataProvider,
+    {
         let mut total = HandleManyEventsResult::default();
 
         let position = position.into();

diff --git a/crates/matrix-sdk-ui/src/timeline/pagination.rs b/crates/matrix-sdk-ui/src/timeline/pagination.rs
@@ -81,7 +81,7 @@ impl super::Timeline {
                     // `matrix_sdk::event_cache::RoomEventCacheUpdate` from
                     // `matrix_sdk::event_cache::RoomPagination::run_backwards`.
                     self.controller
-                        .add_events_at(events, TimelineNewItemPosition::Start { origin: RemoteEventOrigin::Pagination })
+                        .add_events_at(events.into_iter(), TimelineNewItemPosition::Start { origin: RemoteEventOrigin::Pagination })
                         .await;
 
                     if num_events == 0 && !reached_start {

diff --git a/crates/matrix-sdk-ui/src/timeline/tests/basic.rs b/crates/matrix-sdk-ui/src/timeline/tests/basic.rs
@@ -50,7 +50,7 @@ async fn test_initial_events() {
     timeline
         .controller
         .add_events_at(
-            vec![f.text_msg("A").sender(*ALICE), f.text_msg("B").sender(*BOB)],
+            [f.text_msg("A").sender(*ALICE), f.text_msg("B").sender(*BOB)].into_iter(),
             TimelineNewItemPosition::End { origin: RemoteEventOrigin::Sync },
         )
         .await;
@@ -92,7 +92,10 @@ async fn test_replace_with_initial_events_and_read_marker() {
 
     timeline
         .controller
-        .add_events_at(vec![ev], TimelineNewItemPosition::End { origin: RemoteEventOrigin::Sync })
+        .add_events_at(
+            [ev].into_iter(),
+            TimelineNewItemPosition::End { origin: RemoteEventOrigin::Sync },
+        )
         .await;
 
     let items = timeline.controller.items().await;
@@ -101,7 +104,10 @@ async fn test_replace_with_initial_events_and_read_marker() {
     assert_eq!(items[1].as_event().unwrap().content().as_message().unwrap().body(), "hey");
 
     let ev = f.text_msg("yo").sender(*BOB).into_sync();
-    timeline.controller.replace_with_initial_remote_events(vec![ev], RemoteEventOrigin::Sync).await;
+    timeline
+        .controller
+        .replace_with_initial_remote_events([ev].into_iter(), RemoteEventOrigin::Sync)
+        .await;
 
     let items = timeline.controller.items().await;
     assert_eq!(items.len(), 2);
@@ -309,7 +315,7 @@ async fn test_dedup_initial() {
     timeline
         .controller
         .add_events_at(
-            vec![
+            [
                 // two events
                 event_a.clone(),
                 event_b.clone(),
@@ -318,7 +324,8 @@ async fn test_dedup_initial() {
                 event_b,
                 // … and a new event also came in
                 event_c,
-            ],
+            ]
+            .into_iter(),
             TimelineNewItemPosition::End { origin: RemoteEventOrigin::Sync },
         )
         .await;
@@ -356,7 +363,7 @@ async fn test_internal_id_prefix() {
     timeline
         .controller
         .add_events_at(
-            vec![ev_a, ev_b, ev_c],
+            [ev_a, ev_b, ev_c].into_iter(),
             TimelineNewItemPosition::End { origin: RemoteEventOrigin::Sync },
         )
         .await;
@@ -522,7 +529,10 @@ async fn test_replace_with_initial_events_when_batched() {
 
     timeline
         .controller
-        .add_events_at(vec![ev], TimelineNewItemPosition::End { origin: RemoteEventOrigin::Sync })
+        .add_events_at(
+            [ev].into_iter(),
+            TimelineNewItemPosition::End { origin: RemoteEventOrigin::Sync },
+        )
         .await;
 
     let (items, mut stream) = timeline.controller.subscribe_batched().await;
@@ -531,7 +541,10 @@ async fn test_replace_with_initial_events_when_batched() {
     assert_eq!(items[1].as_event().unwrap().content().as_message().unwrap().body(), "hey");
 
     let ev = f.text_msg("yo").sender(*BOB).into_sync();
-    timeline.controller.replace_with_initial_remote_events(vec![ev], RemoteEventOrigin::Sync).await;
+    timeline
+        .controller
+        .replace_with_initial_remote_events([ev].into_iter(), RemoteEventOrigin::Sync)
+        .await;
 
     // Assert there are more than a single Clear diff in the next batch:
     // Clear + PushBack (event) + PushFront (day divider)

diff --git a/crates/matrix-sdk-ui/src/timeline/tests/echo.rs b/crates/matrix-sdk-ui/src/timeline/tests/echo.rs
@@ -272,12 +272,12 @@ async fn test_no_read_marker_with_local_echo() {
     timeline
         .controller
         .replace_with_initial_remote_events(
-            vec![f
-                .text_msg("msg1")
+            [f.text_msg("msg1")
                 .sender(user_id!("@a:b.c"))
                 .event_id(event_id)
                 .server_ts(MilliSecondsSinceUnixEpoch::now())
-                .into_sync()],
+                .into_sync()]
+            .into_iter(),
             RemoteEventOrigin::Sync,
         )
         .await;

diff --git a/crates/matrix-sdk-ui/src/timeline/tests/mod.rs b/crates/matrix-sdk-ui/src/timeline/tests/mod.rs
@@ -242,7 +242,7 @@ impl TestTimeline {
         let event = event.into();
         self.controller
             .add_events_at(
-                vec![event],
+                [event].into_iter(),
                 TimelineNewItemPosition::End { origin: RemoteEventOrigin::Sync },
             )
             .await;
@@ -264,7 +264,7 @@ impl TestTimeline {
         let timeline_event = TimelineEvent::new(event.cast());
         self.controller
             .add_events_at(
-                vec![timeline_event],
+                [timeline_event].into_iter(),
                 TimelineNewItemPosition::Start { origin: RemoteEventOrigin::Pagination },
             )
             .await;

diff --git a/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs b/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs
@@ -196,14 +196,15 @@ async fn test_initial_reaction_timestamp_is_stored() {
     timeline
         .controller
         .add_events_at(
-            vec![
+            [
                 // Reaction comes first.
                 f.reaction(&message_event_id, REACTION_KEY.to_owned())
                     .server_ts(reaction_timestamp)
                     .into_sync(),
                 // Event comes next.
                 f.text_msg("A").event_id(&message_event_id).into_sync(),
-            ],
+            ]
+            .into_iter(),
             TimelineNewItemPosition::End { origin: RemoteEventOrigin::Sync },
         )
         .await;

diff --git a/crates/matrix-sdk-ui/src/timeline/tests/redaction.rs b/crates/matrix-sdk-ui/src/timeline/tests/redaction.rs
@@ -141,11 +141,12 @@ async fn test_reaction_redaction_timeline_filter() {
     timeline
         .controller
         .add_events_at(
-            vec![SyncTimelineEvent::new(
+            [SyncTimelineEvent::new(
                 timeline
                     .event_builder
                     .make_sync_redacted_message_event(*ALICE, RedactedReactionEventContent::new()),
-            )],
+            )]
+            .into_iter(),
             TimelineNewItemPosition::End { origin: RemoteEventOrigin::Sync },
         )
         .await;