From 79f412790fe0931e931287f3b06cdce1aa82d6eb Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 4 Sep 2024 18:36:12 +0200 Subject: [PATCH] timeline: stash edits around in case they arrive before the related event --- .../src/timeline/controller/state.rs | 22 +++++- .../src/timeline/event_handler.rs | 40 +++++++++- .../tests/integration/timeline/edit.rs | 74 +++++++++++++++++++ 3 files changed, 131 insertions(+), 5 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/controller/state.rs b/crates/matrix-sdk-ui/src/timeline/controller/state.rs index 84ae7c567..14f2a88a2 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/state.rs @@ -12,7 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::{collections::VecDeque, future::Future, sync::Arc}; +use std::{ + collections::{HashMap, VecDeque}, + future::Future, + sync::Arc, +}; use eyeball_im::{ObservableVector, ObservableVectorTransaction, ObservableVectorTransactionEntry}; use itertools::Itertools as _; @@ -21,9 +25,14 @@ use matrix_sdk_base::deserialized_responses::TimelineEvent; #[cfg(test)] use ruma::events::receipt::ReceiptEventContent; use ruma::{ - events::AnySyncEphemeralRoomEvent, push::Action, serde::Raw, EventId, - MilliSecondsSinceUnixEpoch, OwnedEventId, OwnedTransactionId, OwnedUserId, RoomVersionId, - UserId, + events::{ + relation::Replacement, room::message::RoomMessageEventContentWithoutRelation, + AnySyncEphemeralRoomEvent, + }, + push::Action, + serde::Raw, + EventId, MilliSecondsSinceUnixEpoch, OwnedEventId, OwnedTransactionId, OwnedUserId, + RoomVersionId, UserId, }; use tracing::{debug, instrument, trace, warn}; @@ -727,7 +736,10 @@ pub(in crate::timeline) struct TimelineMetadata { pub all_events: VecDeque, pub reactions: Reactions, + pub pending_poll_events: PendingPollEvents, + pub pending_edits: HashMap>, + pub fully_read_event: Option, /// Whether we have a fully read-marker item in the timeline, that's up to @@ -755,6 +767,7 @@ impl TimelineMetadata { next_internal_id: Default::default(), reactions: Default::default(), pending_poll_events: Default::default(), + pending_edits: Default::default(), fully_read_event: Default::default(), // It doesn't make sense to set this to false until we fill the `fully_read_event` // field, otherwise we'll keep on exiting early in `Self::update_read_marker`. @@ -774,6 +787,7 @@ impl TimelineMetadata { self.all_events.clear(); self.reactions.clear(); self.pending_poll_events.clear(); + self.pending_edits.clear(); self.fully_read_event = None; // We forgot about the fully read marker right above, so wait for a new one // before attempting to update it for each new timeline item. diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index 24caf5530..01be9f76d 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -453,7 +453,39 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { replacement: Replacement, ) { let Some((item_pos, item)) = rfind_event_by_id(self.items, &replacement.event_id) else { - debug!("Timeline item not found, discarding edit"); + if let Flow::Remote { position, .. } = &self.ctx.flow { + match position { + TimelineItemPosition::Start { .. } => { + // Only insert the edit if there wasn't any other edit + // before. + if self.meta.pending_edits.get(&replacement.event_id).is_none() { + self.meta + .pending_edits + .insert(replacement.event_id.clone(), replacement); + debug!("Timeline item not found, stashing edit"); + } else { + debug!("Timeline item not found, but there was a previous edit for the event: discarding"); + } + } + + TimelineItemPosition::End { .. } => { + // This is a more recent edit: it's fine to overwrite the previous one, if + // available. + self.meta.pending_edits.insert(replacement.event_id.clone(), replacement); + debug!("Timeline item not found, stashing edit"); + } + + TimelineItemPosition::Update(_) => { + // This is not trivial: we don't really have any recency information about + // the edit. Maybe there was another edit that's more recent and could be + // decrypted, or maybe it's the opposite. Discard. + debug!("Timeline item not found, but discarding as we don't know the relative position of this edit event"); + } + } + } else { + debug!("Local edit for a timeline item not found, discarding"); + } + return; }; @@ -969,6 +1001,12 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { } } + if let Flow::Remote { event_id, .. } = &self.ctx.flow { + if let Some(edit) = self.meta.pending_edits.remove(event_id) { + self.handle_room_message_edit(edit); + } + } + // If we don't have a read marker item, look if we need to add one now. if !self.meta.has_up_to_date_read_marker_item { self.meta.update_read_marker(self.items); diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/edit.rs b/crates/matrix-sdk-ui/tests/integration/timeline/edit.rs index dd3992e44..f24f303f3 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/edit.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/edit.rs @@ -601,3 +601,77 @@ async fn test_send_edit_when_timeline_is_clear() { server.verify().await; } + +#[async_test] +async fn test_pending_edit() { + let room_id = room_id!("!a98sd12bjh:example.org"); + let (client, server) = logged_in_client_with_server().await; + let sync_settings = SyncSettings::new().timeout(Duration::from_millis(3000)); + + let f = EventFactory::new(); + + let mut sync_builder = SyncResponseBuilder::new(); + sync_builder.add_joined_room(JoinedRoomBuilder::new(room_id)); + + mock_sync(&server, sync_builder.build_json_sync_response(), None).await; + let _response = client.sync_once(sync_settings.clone()).await.unwrap(); + server.reset().await; + + mock_encryption_state(&server, false).await; + + let room = client.get_room(room_id).unwrap(); + let timeline = room.timeline().await.unwrap(); + let (_, mut timeline_stream) = timeline.subscribe().await; + + // When I receive an edit event for an event I don't know about… + let original_event_id = event_id!("$edited"); + let edit_event_id = event_id!("$original"); + sync_builder.add_joined_room( + JoinedRoomBuilder::new(room_id).add_timeline_event( + f.text_msg("* hello") + .sender(&ALICE) + .event_id(edit_event_id) + .edit(original_event_id, RoomMessageEventContent::text_plain("[edit]").into()), + ), + ); + + mock_sync(&server, sync_builder.build_json_sync_response(), None).await; + let _response = client.sync_once(sync_settings.clone()).await.unwrap(); + server.reset().await; + + // Nothing happens. + assert!(timeline_stream.next().now_or_never().is_none()); + + // But when I receive the original event after a bit… + sync_builder.add_joined_room( + JoinedRoomBuilder::new(room_id) + .add_timeline_event(f.text_msg("hi").sender(&ALICE).event_id(&original_event_id)), + ); + + mock_sync(&server, sync_builder.build_json_sync_response(), None).await; + let _response = client.sync_once(sync_settings.clone()).await.unwrap(); + server.reset().await; + + // Then I get the content as original first… + assert_let!(Some(VectorDiff::PushBack { value }) = timeline_stream.next().await); + let item = value.as_event().unwrap(); + assert!(item.event_id().is_some()); + assert!(!item.is_own()); + assert_eq!(item.content().as_message().unwrap().body(), "hi"); + + // And then the edit. + assert_next_matches!(timeline_stream, VectorDiff::Set { index: 0, value } => { + let item = value.as_event().unwrap(); + assert!(item.event_id().is_some()); + assert!(!item.is_own()); + assert_eq!(item.content().as_message().unwrap().body(), "[edit]"); + }); + + // The day divider. + assert_next_matches!(timeline_stream, VectorDiff::PushFront { value } => { + assert!(value.is_day_divider()); + }); + + // And nothing else. + assert!(timeline_stream.next().now_or_never().is_none()); +}