diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index e4a2e63a3..16fb67049 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -682,12 +682,12 @@ impl<'a> TimelineEventHandler<'a> { if let Some(day_divider_item) = maybe_create_day_divider_from_timestamps(old_ts, timestamp) { - trace!("Adding day divider"); + trace!("Adding day divider (local)"); self.items.push_back(Arc::new(day_divider_item)); } } else { // If there is no event item, there is no day divider yet. - trace!("Adding first day divider"); + trace!("Adding first day divider (local)"); self.items.push_back(Arc::new(TimelineItem::day_divider(timestamp))); } @@ -799,36 +799,35 @@ impl<'a> TimelineEventHandler<'a> { trace!(idx, "Replacing existing event"); self.items.set(idx, Arc::new(item.into())); return; - } else { - // In more complex cases, remove the item and day - // divider (if necessary) before re-adding the item. - trace!("Removing local echo or duplicate timeline item"); - self.items.remove(idx); - - assert_ne!( - idx, 0, - "there is never an event item at index 0 because \ - the first event item is preceded by a day divider" - ); - - // Pre-requisites for removing the day divider: - // 1. there is one preceding the old item at all - if self.items[idx - 1].is_day_divider() - // 2. the item after the old one that was removed - // is virtual (it should be impossible for this - // to be a read marker) - && self - .items - .get(idx) - .map_or(true, |item| item.is_virtual()) - { - trace!("Removing day divider"); - self.items.remove(idx - 1); - } - - // no return here, below code for adding a new event - // will run to re-add the removed item } + + // In more complex cases, remove the item and day + // divider (if necessary) before re-adding the item. + trace!("Removing local echo or duplicate timeline item"); + self.items.remove(idx); + + assert_ne!( + idx, 0, + "there is never an event item at index 0 because \ + the first event item is preceded by a day divider" + ); + + // Pre-requisites for removing the day divider: + // 1. there is one preceding the old item at all + if self.items[idx - 1].is_day_divider() + // 2. the item after the old one that was removed is virtual (it should be + // impossible for this to be a read marker) + && self + .items + .get(idx) + .map_or(true, |item| item.is_virtual()) + { + trace!("Removing day divider"); + self.items.remove(idx - 1); + } + + // no return here, below code for adding a new event + // will run to re-add the removed item } else if txn_id.is_some() { warn!( "Received event with transaction ID, but didn't \ @@ -836,26 +835,66 @@ impl<'a> TimelineEventHandler<'a> { ); } - // Check if the latest event has the same date as this event. - if let Some(latest_event) = self.items.iter().rev().find_map(|item| item.as_event()) - { + // Local echoes that are pending should stick to the bottom, + // find the latest event that isn't that. + let mut latest_event_stream = self + .items + .iter() + .enumerate() + .rev() + .filter_map(|(idx, item)| Some((idx, item.as_event()?))); + + // Find the latest event, independently of success or failure status. + let latest_event = latest_event_stream.clone().next().unzip().1; + + // Find the index of the latest non-failure event. + let latest_nonfailed_event_idx = latest_event_stream + .find(|(_, evt)| { + !matches!( + evt.send_state(), + Some(EventSendState::NotSentYet | EventSendState::Sent { .. }) + ) + }) + .unzip() + .0; + + // Insert the next item after the latest non-failed event item, + // or at the start if there is no such item. + let mut insert_idx = latest_nonfailed_event_idx.map_or(0, |idx| idx + 1); + + // Keep push semantics, if we're inserting at the end. + let should_push = insert_idx == self.items.len(); + + if let Some(latest_event) = latest_event { + // Check if that event has the same date as the new one. let old_ts = latest_event.timestamp(); if let Some(day_divider_item) = maybe_create_day_divider_from_timestamps(old_ts, timestamp) { - trace!("Adding day divider"); - self.items.push_back(Arc::new(day_divider_item)); + trace!("Adding day divider (remote)"); + if should_push { + self.items.push_back(Arc::new(day_divider_item)); + } else { + self.items.insert(insert_idx, Arc::new(day_divider_item)); + insert_idx += 1; + } } } else { // If there is no event item, there is no day divider yet. - trace!("Adding first day divider"); - self.items.push_back(Arc::new(TimelineItem::day_divider(timestamp))); + trace!("Adding first day divider (remote)"); + if should_push { + self.items.push_back(Arc::new(TimelineItem::day_divider(timestamp))); + } else { + self.items + .insert(insert_idx, Arc::new(TimelineItem::day_divider(timestamp))); + insert_idx += 1; + } } if self.track_read_receipts { maybe_add_implicit_read_receipt( - self.items.len(), + insert_idx, &mut item, self.meta.is_own_event, self.items, @@ -863,8 +902,12 @@ impl<'a> TimelineEventHandler<'a> { ); } - trace!("Adding new remote timeline item at the end"); - self.items.push_back(Arc::new(item.into())); + trace!("Adding new remote timeline item after all non-pending events"); + if should_push { + self.items.push_back(Arc::new(item.into())); + } else { + self.items.insert(insert_idx, Arc::new(item.into())); + } } #[cfg(feature = "e2e-encryption")] diff --git a/crates/matrix-sdk-ui/src/timeline/event_item/local.rs b/crates/matrix-sdk-ui/src/timeline/event_item/local.rs index 727c39752..7c526464d 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_item/local.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_item/local.rs @@ -12,9 +12,10 @@ // See the License for the specific language governing permissions and // limitations under the License. -use ruma::{EventId, OwnedTransactionId}; +use std::sync::Arc; -use super::EventSendState; +use matrix_sdk::Error; +use ruma::{EventId, OwnedEventId, OwnedTransactionId}; /// An item for an event that was created locally and not yet echoed back by /// the homeserver. @@ -43,3 +44,24 @@ impl LocalEventTimelineItem { Self { send_state, ..self.clone() } } } + +/// This type represents the "send state" of a local event timeline item. +#[derive(Clone, Debug)] +pub enum EventSendState { + /// The local event has not been sent yet. + NotSentYet, + /// The local event has been sent to the server, but unsuccessfully: The + /// sending has failed. + SendingFailed { + /// Details about how sending the event failed. + error: Arc, + }, + /// Sending has been cancelled because an earlier event in the + /// message-sending queue failed. + Cancelled, + /// The local event has been sent successfully to the server. + Sent { + /// The event ID assigned by the server. + event_id: OwnedEventId, + }, +} diff --git a/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs b/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs index f670e7102..85ddf41b9 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs @@ -20,18 +20,20 @@ use once_cell::sync::Lazy; use ruma::{ events::{receipt::Receipt, room::message::MessageType, AnySyncTimelineEvent}, serde::Raw, - EventId, MilliSecondsSinceUnixEpoch, OwnedEventId, OwnedMxcUri, OwnedUserId, TransactionId, - UserId, + EventId, MilliSecondsSinceUnixEpoch, OwnedMxcUri, OwnedUserId, TransactionId, UserId, }; mod content; mod local; mod remote; -pub use self::content::{ - AnyOtherFullStateEventContent, BundledReactions, EncryptedMessage, InReplyToDetails, - MemberProfileChange, MembershipChange, Message, OtherState, ReactionGroup, RepliedToEvent, - RoomMembershipChange, Sticker, TimelineItemContent, +pub use self::{ + content::{ + AnyOtherFullStateEventContent, BundledReactions, EncryptedMessage, InReplyToDetails, + MemberProfileChange, MembershipChange, Message, OtherState, ReactionGroup, RepliedToEvent, + RoomMembershipChange, Sticker, TimelineItemContent, + }, + local::EventSendState, }; pub(super) use self::{ local::LocalEventTimelineItem, @@ -290,27 +292,6 @@ impl EventTimelineItem { } } -/// This type represents the "send state" of a local event timeline item. -#[derive(Clone, Debug)] -pub enum EventSendState { - /// The local event has not been sent yet. - NotSentYet, - /// The local event has been sent to the server, but unsuccessfully: The - /// sending has failed. - SendingFailed { - /// Details about how sending the event failed. - error: Arc, - }, - /// Sending has been cancelled because an earlier event in the - /// message-sending queue failed. - Cancelled, - /// The local event has been sent successfully to the server. - Sent { - /// The event ID assigned by the server. - event_id: OwnedEventId, - }, -} - impl From for EventTimelineItemKind { fn from(value: LocalEventTimelineItem) -> Self { EventTimelineItemKind::Local(value) diff --git a/crates/matrix-sdk-ui/src/timeline/inner.rs b/crates/matrix-sdk-ui/src/timeline/inner.rs index 1ba52080b..8f2bba1cf 100644 --- a/crates/matrix-sdk-ui/src/timeline/inner.rs +++ b/crates/matrix-sdk-ui/src/timeline/inner.rs @@ -496,25 +496,19 @@ impl TimelineInner

{ _ => None, }; - // Look for the local event by the transaction ID or event ID. - let result = rfind_event_item(&state.items, |it| { - it.transaction_id() == Some(txn_id) - || new_event_id.is_some() && it.event_id() == new_event_id - }); - - let Some((idx, item)) = result else { - // Event isn't found at all. - warn!("Timeline item not found, can't add event ID"); - return; - }; - - let Some(local_item) = item.as_local() else { + // The local echoes are always at the end of the timeline, we must first make + // sure the remote echo hasn't showed up yet. + if rfind_event_item(&state.items, |it| { + new_event_id.is_some() && it.event_id() == new_event_id && it.as_remote().is_some() + }) + .is_some() + { // Remote echo already received. This is very unlikely. trace!("Remote echo received before send-event response"); - let local_echo = rfind_event_item(&state.items, |it| { - it.transaction_id() == Some(txn_id) - }); + let local_echo = + rfind_event_item(&state.items, |it| it.transaction_id() == Some(txn_id)); + // If there's both the remote echo and a local echo, that means the // remote echo was received before the response *and* contained no // transaction ID (and thus duplicated the local echo). @@ -526,17 +520,33 @@ impl TimelineInner

{ error!("Inconsistent state: Local echo was not preceded by day divider"); return; } - if idx == state.items.len() { - error!("Inconsistent state: Echo was duplicated but local echo was last"); - return; - } - if state.items[idx - 1].is_day_divider() && state.items[idx].is_day_divider() { - // If local echo was the only event from that day, remove day divider. + if idx == state.items.len() && state.items[idx - 1].is_day_divider() { + // The day divider may have been added for this local echo, remove it and let + // the next message decide whether it's required or not. state.items.remove(idx - 1); } } + return; + } + + // Look for the local event by the transaction ID or event ID. + let result = rfind_event_item(&state.items, |it| { + it.transaction_id() == Some(txn_id) + || new_event_id.is_some() + && it.event_id() == new_event_id + && it.as_local().is_some() + }); + + let Some((idx, item)) = result else { + // Event isn't found at all. + warn!("Timeline item not found, can't add event ID"); + return; + }; + + let Some(local_item) = item.as_local() else { + warn!("We looked for a local item, but it transitioned to remote??"); return; }; diff --git a/crates/matrix-sdk-ui/src/timeline/tests/echo.rs b/crates/matrix-sdk-ui/src/timeline/tests/echo.rs index 149613856..1d5e76e40 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/echo.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/echo.rs @@ -125,8 +125,11 @@ async fn remote_echo_new_position() { // … and another event that comes back before the remote echo timeline.handle_live_message_event(&BOB, RoomMessageEventContent::text_plain("test")).await; - let _day_divider = assert_next_matches!(stream, VectorDiff::PushBack { value } => value); - let _bob_message = assert_next_matches!(stream, VectorDiff::PushBack { value } => value); + // … and is inserted before the local echo item + let _day_divider = + assert_next_matches!(stream, VectorDiff::Insert { index: 0, value } => value); + let _bob_message = + assert_next_matches!(stream, VectorDiff::Insert { index: 1, value } => value); // When the remote echo comes in… timeline @@ -146,9 +149,9 @@ async fn remote_echo_new_position() { .await; // … the local echo should be removed - assert_next_matches!(stream, VectorDiff::Remove { index: 1 }); + assert_next_matches!(stream, VectorDiff::Remove { index: 3 }); // … along with its day divider - assert_next_matches!(stream, VectorDiff::Remove { index: 0 }); + assert_next_matches!(stream, VectorDiff::Remove { index: 2 }); // … and the remote echo added (no new day divider because both bob's and // alice's message are from the same day according to server timestamps) diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/echo.rs b/crates/matrix-sdk-ui/tests/integration/timeline/echo.rs index 046a1fd5c..570cbe493 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/echo.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/echo.rs @@ -246,15 +246,15 @@ async fn dedup_by_event_id_late() { mock_sync(&server, ev_builder.build_json_sync_response(), None).await; let _response = client.sync_once(sync_settings.clone()).await.unwrap(); - assert_next_matches!(timeline_stream, VectorDiff::PushBack { .. }); // day divider + assert_next_matches!(timeline_stream, VectorDiff::Insert { index: 0, .. }); // day divider let remote_echo = - assert_next_matches!(timeline_stream, VectorDiff::PushBack { value } => value); + assert_next_matches!(timeline_stream, VectorDiff::Insert { index: 1, value } => value); let item = remote_echo.as_event().unwrap(); assert_eq!(item.event_id(), Some(event_id)); // Local echo and its day divider are removed. - assert_matches!(timeline_stream.next().await, Some(VectorDiff::Remove { index: 1 })); - assert_matches!(timeline_stream.next().await, Some(VectorDiff::Remove { index: 0 })); + assert_matches!(timeline_stream.next().await, Some(VectorDiff::Remove { index: 3 })); + assert_matches!(timeline_stream.next().await, Some(VectorDiff::Remove { index: 2 })); } #[async_test] 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 f2cff0301..7b2a2bbdb 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/sliding_sync.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/sliding_sync.rs @@ -117,6 +117,31 @@ macro_rules! assert_timeline_stream { ) }; + // `insert [$nth] "$event_id"` + ( @_ [ $stream:ident ] [ insert [$index:literal] $event_id:literal ; $( $rest:tt )* ] [ $( $accumulator:tt )* ] ) => { + assert_timeline_stream!( + @_ + [ $stream ] + [ $( $rest )* ] + [ + $( $accumulator )* + { + assert_matches!( + $stream.next().now_or_never(), + Some(Some(VectorDiff::Insert { index: $index, value })) => { + assert_matches!( + value.as_ref(), + TimelineItem::Event(event_timeline_item) => { + assert_eq!(event_timeline_item.event_id().unwrap().as_str(), $event_id); + } + ); + } + ); + } + ] + ) + }; + // `update [$nth] "$event_id"` ( @_ [ $stream:ident ] [ update [$index:literal] $event_id:literal ; $( $rest:tt )* ] [ $( $accumulator:tt )* ] ) => { assert_timeline_stream!( diff --git a/testing/matrix-sdk-integration-testing/src/tests/reactions.rs b/testing/matrix-sdk-integration-testing/src/tests/reactions.rs index d974fbea6..e496d7ccc 100644 --- a/testing/matrix-sdk-integration-testing/src/tests/reactions.rs +++ b/testing/matrix-sdk-integration-testing/src/tests/reactions.rs @@ -76,13 +76,11 @@ async fn test_toggling_reaction() -> Result<()> { let _join_rules = stream.next().await; let _history_visibility = stream.next().await; let _guest_access = stream.next().await; - let _remove_local_event = stream.next().await; } // Check we have the remote echo { - let event = - assert_matches!(stream.next().await, Some(VectorDiff::PushBack { value }) => value); + let event = assert_matches!(stream.next().await, Some(VectorDiff::Set { index: 7, value }) => value); let event = event.as_event().unwrap(); assert_eq!(event.event_id().unwrap(), &event_id); assert!(!event.is_local_echo());