From 15ed91e04799801e4447fe8d2187bd40b32eb321 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 13 Jun 2024 14:05:22 +0200 Subject: [PATCH 1/2] fix(ui): Change the behaviour when a duplicated event is received by `Timeline`. This patch changes the behaviour of the `Timeline` when a duplicated event is received. Prior to this patch, the old event was removed, and the one was added. With this patch, the old event is kept, and the new one is skipped. This is important for https://github.com/matrix-org/matrix-rust-sdk/pull/3512 where events are mapped to the timeline item indices. When an event is removed, it becomes difficult to keep the mapping correct. This patch also adds duplication detection for pagination (with `TimelineItemPosition::Start`). --- .../matrix-sdk-ui/src/timeline/inner/state.rs | 43 +++++++++++++++---- .../integration/timeline/sliding_sync.rs | 4 -- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/inner/state.rs b/crates/matrix-sdk-ui/src/timeline/inner/state.rs index 736d766cdcc..d7fc726679b 100644 --- a/crates/matrix-sdk-ui/src/timeline/inner/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/inner/state.rs @@ -501,7 +501,8 @@ impl TimelineInnerStateTransaction<'_> { timestamp: Some(event.origin_server_ts()), visible: false, }; - self.add_event(event_meta, position, room_data_provider, settings).await; + let _event_added = + self.add_event(event_meta, position, room_data_provider, settings).await; return HandleEventResult::default(); } @@ -526,7 +527,9 @@ impl TimelineInnerStateTransaction<'_> { timestamp, visible: false, }; - self.add_event(event_meta, position, room_data_provider, settings).await; + let _event_added = self + .add_event(event_meta, position, room_data_provider, settings) + .await; } return HandleEventResult::default(); @@ -543,7 +546,14 @@ impl TimelineInnerStateTransaction<'_> { timestamp: Some(timestamp), visible: should_add, }; - self.add_event(event_meta, position, room_data_provider, settings).await; + + let event_added = self.add_event(event_meta, position, room_data_provider, settings).await; + + // If the event has not been added, it's because it's a duplicated event. Let's + // return early. + if !event_added { + return HandleEventResult::default(); + } let sender_profile = room_data_provider.profile_from_user_id(&sender).await; let ctx = TimelineEventContext { @@ -637,23 +647,36 @@ impl TimelineInnerStateTransaction<'_> { items.commit(); } + /// Add an event in the [`TimelineInnerMeta::all_events`] collection. + /// + /// This method also adjusts read receipt if needed. + /// + /// It returns `true` if the event has been added, `false` otherwise. The + /// latter happens if the event already exists, i.e. if an existing event is + /// requested to be added. async fn add_event( &mut self, event_meta: FullEventMeta<'_>, position: TimelineItemPosition, room_data_provider: &P, settings: &TimelineInnerSettings, - ) { + ) -> bool { + // Detect if an event already exists in [`TimelineInnerMeta::all_events`] + fn event_already_exists(new_event_id: &EventId, all_events: &VecDeque) -> bool { + all_events.iter().any(|EventMeta { event_id, .. }| event_id == new_event_id) + } + match position { TimelineItemPosition::Start { .. } => { + if event_already_exists(event_meta.event_id, &self.meta.all_events) { + return false; + } + self.meta.all_events.push_front(event_meta.base_meta()) } TimelineItemPosition::End { .. } => { - // Handle duplicated event. - if let Some(pos) = - self.meta.all_events.iter().position(|ev| ev.event_id == event_meta.event_id) - { - self.meta.all_events.remove(pos); + if event_already_exists(event_meta.event_id, &self.meta.all_events) { + return false; } self.meta.all_events.push_back(event_meta.base_meta()); @@ -686,6 +709,8 @@ impl TimelineInnerStateTransaction<'_> { self.maybe_add_implicit_read_receipt(event_meta); } + + true } fn adjust_day_dividers(&mut self, mut adjuster: DayDividerAdjuster) { diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/sliding_sync.rs b/crates/matrix-sdk-ui/tests/integration/timeline/sliding_sync.rs index db56d215db2..d7877bdf8df 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/sliding_sync.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/sliding_sync.rs @@ -416,10 +416,6 @@ async fn test_timeline_duplicated_events() -> Result<()> { assert_timeline_stream! { [timeline_stream] update[3] "$x3:bar.org"; - update[1] "$x1:bar.org"; - remove[1]; - append "$x1:bar.org"; - update[3] "$x1:bar.org"; append "$x4:bar.org"; }; } From d46e65805af1ada641723fe85259b452dd087a70 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 13 Jun 2024 14:21:17 +0200 Subject: [PATCH 2/2] chore(ui): Rename `TimelineInnerState::add_event` to `add_or_update_event`. This patch renames `add_event` to `add_or_update_event` because it is what it does. --- .../matrix-sdk-ui/src/timeline/inner/state.rs | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/inner/state.rs b/crates/matrix-sdk-ui/src/timeline/inner/state.rs index d7fc726679b..c97b84c7252 100644 --- a/crates/matrix-sdk-ui/src/timeline/inner/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/inner/state.rs @@ -501,8 +501,9 @@ impl TimelineInnerStateTransaction<'_> { timestamp: Some(event.origin_server_ts()), visible: false, }; - let _event_added = - self.add_event(event_meta, position, room_data_provider, settings).await; + let _event_added_or_updated = self + .add_or_update_event(event_meta, position, room_data_provider, settings) + .await; return HandleEventResult::default(); } @@ -527,8 +528,8 @@ impl TimelineInnerStateTransaction<'_> { timestamp, visible: false, }; - let _event_added = self - .add_event(event_meta, position, room_data_provider, settings) + let _event_added_or_updated = self + .add_or_update_event(event_meta, position, room_data_provider, settings) .await; } @@ -547,11 +548,12 @@ impl TimelineInnerStateTransaction<'_> { visible: should_add, }; - let event_added = self.add_event(event_meta, position, room_data_provider, settings).await; + let event_added_or_updated = + self.add_or_update_event(event_meta, position, room_data_provider, settings).await; - // If the event has not been added, it's because it's a duplicated event. Let's - // return early. - if !event_added { + // If the event has not been added or updated, it's because it's a duplicated + // event. Let's return early. + if !event_added_or_updated { return HandleEventResult::default(); } @@ -647,14 +649,15 @@ impl TimelineInnerStateTransaction<'_> { items.commit(); } - /// Add an event in the [`TimelineInnerMeta::all_events`] collection. + /// Add or update an event in the [`TimelineInnerMeta::all_events`] + /// collection. /// /// This method also adjusts read receipt if needed. /// - /// It returns `true` if the event has been added, `false` otherwise. The - /// latter happens if the event already exists, i.e. if an existing event is - /// requested to be added. - async fn add_event( + /// It returns `true` if the event has been added or updated, `false` + /// otherwise. The latter happens if the event already exists, i.e. if + /// an existing event is requested to be added. + async fn add_or_update_event( &mut self, event_meta: FullEventMeta<'_>, position: TimelineItemPosition,