From fd17bce3009f0f62fd9cf96ffcbbd9a45bbacb1f Mon Sep 17 00:00:00 2001 From: Jonas Platte Date: Thu, 14 Sep 2023 14:14:44 +0200 Subject: [PATCH] ui: Fix day divider logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit … for when a remote event is re-received while a local echo is pending. Also simplify test_togglling_reaction integration test so it still passes. --- .../src/timeline/event_handler.rs | 23 ++++------ .../matrix-sdk-ui/src/timeline/tests/echo.rs | 46 +++++++++++++++++++ .../src/tests/reactions.rs | 44 +++++------------- 3 files changed, 65 insertions(+), 48 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index 9a6ac62c1..68c49e24d 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -999,30 +999,23 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { // Local echoes that are pending should stick to the bottom, // find the latest event that isn't that. - let mut latest_event_stream = self + let (latest_event_idx, latest_event) = 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)| { + .filter_map(|(idx, item)| Some((idx, item.as_event()?))) + .find(|(_, item)| { !matches!( - evt.send_state(), + item.send_state(), Some(EventSendState::NotSentYet | EventSendState::Sent { .. }) ) }) - .unzip() - .0; + .unzip(); - // 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); + // Insert the next item after the latest event item that's not a + // pending local echo, or at the start if there is no such item. + let mut insert_idx = latest_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(); diff --git a/crates/matrix-sdk-ui/src/timeline/tests/echo.rs b/crates/matrix-sdk-ui/src/timeline/tests/echo.rs index 3b315d929..c089cff4b 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/echo.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/echo.rs @@ -165,3 +165,49 @@ async fn remote_echo_new_position() { let item = assert_next_matches!(stream, VectorDiff::PushBack { value } => value); assert!(!item.as_event().unwrap().is_local_echo()); } + +#[async_test] +async fn day_divider_duplication() { + let timeline = TestTimeline::new(); + + // Given two remote events from one day, and a local event from another day… + timeline.handle_live_message_event(&BOB, RoomMessageEventContent::text_plain("A")).await; + timeline.handle_live_message_event(&BOB, RoomMessageEventContent::text_plain("B")).await; + timeline + .handle_local_event(AnyMessageLikeEventContent::RoomMessage( + RoomMessageEventContent::text_plain("C"), + )) + .await; + + let items = timeline.inner.items().await; + assert_eq!(items.len(), 5); + assert!(items[0].is_day_divider()); + assert!(items[1].is_remote_event()); + assert!(items[2].is_remote_event()); + assert!(items[3].is_day_divider()); + assert!(items[4].is_local_echo()); + + // … when the second remote event is re-received (day still the same) + let event_id = items[2].as_event().unwrap().event_id().unwrap(); + timeline + .handle_live_custom_event(json!({ + "content": { + "body": "B", + "msgtype": "m.text", + }, + "sender": &*BOB, + "event_id": event_id, + "origin_server_ts": 1, + "type": "m.room.message", + })) + .await; + + // … it should not impact the day dividers. + let items = timeline.inner.items().await; + assert_eq!(items.len(), 5); + assert!(items[0].is_day_divider()); + assert!(items[1].is_remote_event()); + assert!(items[2].is_remote_event()); + assert!(items[3].is_day_divider()); + assert!(items[4].is_local_echo()); +} diff --git a/testing/matrix-sdk-integration-testing/src/tests/reactions.rs b/testing/matrix-sdk-integration-testing/src/tests/reactions.rs index bdb346b72..10ac4cb39 100644 --- a/testing/matrix-sdk-integration-testing/src/tests/reactions.rs +++ b/testing/matrix-sdk-integration-testing/src/tests/reactions.rs @@ -19,7 +19,7 @@ use assert_matches::assert_matches; use assign::assign; use eyeball_im::VectorDiff; use futures_core::Stream; -use futures_util::{future::join_all, StreamExt}; +use futures_util::{future::join_all, FutureExt, StreamExt}; use matrix_sdk::{ config::SyncSettings, ruma::{ @@ -50,41 +50,19 @@ async fn test_toggling_reaction() -> Result<()> { // Send message timeline.send(RoomMessageEventContent::text_plain("hi!").into(), None).await; - { - let _day_divider = stream.next().await; - let event = - assert_matches!(stream.next().await, Some(VectorDiff::PushBack { value }) => value); - let event = event.as_event().unwrap(); - assert_matches!(event.content().as_message(), Some(_)); - assert!(event.is_local_echo()); - assert!(event.reactions().is_empty()); - } - // Receive updated local echo with event ID - let event_id = { - let event = assert_matches!(stream.next().await, Some(VectorDiff::Set { index: 1, value }) => value); - let event = event.as_event().unwrap(); - event.event_id().unwrap().to_owned() + // Sync until the remote echo arrives + let event_id = loop { + sync_room(&alice, room_id).await?; + let items = timeline.items().await; + let last_item = items.last().unwrap().as_event().unwrap(); + if !last_item.is_local_echo() { + break last_item.event_id().unwrap().to_owned(); + } }; - // Sync remaining remote events - sync_room(&alice, room_id).await?; - { - let _room_create = stream.next().await; - let _membership_change = stream.next().await; - let _power_levels = stream.next().await; - let _join_rules = stream.next().await; - let _history_visibility = stream.next().await; - let _guest_access = stream.next().await; - } - - // Check we have the remote echo - { - 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()); - }; + // Skip all stream updates that have happened so far + while stream.next().now_or_never().is_some() {} let message_position = timeline.items().await.len() - 1; let reaction = Annotation::new(event_id.clone(), reaction_key.into());