From bc811cc1dc939acbb7af8a750553399bb54f2b41 Mon Sep 17 00:00:00 2001 From: Mauro Romito Date: Mon, 3 Jul 2023 18:31:51 +0200 Subject: [PATCH] ui: Add a unique ID for all timeline items Co-authored-by: Jonas Platte --- bindings/matrix-sdk-ffi/src/timeline.rs | 25 ++-- .../src/timeline/event_handler.rs | 95 ++++++++------ .../src/timeline/event_item/mod.rs | 13 -- crates/matrix-sdk-ui/src/timeline/inner.rs | 49 ++++---- crates/matrix-sdk-ui/src/timeline/mod.rs | 117 ++++++++++++++---- .../src/timeline/read_receipts.rs | 12 +- .../matrix-sdk-ui/src/timeline/tests/basic.rs | 13 +- .../matrix-sdk-ui/src/timeline/tests/virt.rs | 6 +- .../tests/integration/room_list_service.rs | 10 +- .../tests/integration/timeline/echo.rs | 4 +- .../integration/timeline/sliding_sync.rs | 20 +-- 11 files changed, 226 insertions(+), 138 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/timeline.rs b/bindings/matrix-sdk-ffi/src/timeline.rs index d8c27f99f..a82800783 100644 --- a/bindings/matrix-sdk-ffi/src/timeline.rs +++ b/bindings/matrix-sdk-ffi/src/timeline.rs @@ -218,23 +218,22 @@ impl TimelineItem { #[uniffi::export] impl TimelineItem { pub fn as_event(self: Arc) -> Option> { - use matrix_sdk_ui::timeline::TimelineItem as Item; - unwrap_or_clone_arc_into_variant!(self, .0, Item::Event(evt) => { - Arc::new(EventTimelineItem(evt)) - }) + let event_item = self.0.as_event()?; + Some(Arc::new(EventTimelineItem(event_item.clone()))) } pub fn as_virtual(self: Arc) -> Option { - use matrix_sdk_ui::timeline::{TimelineItem as Item, VirtualTimelineItem as VItem}; - match &self.0 { - Item::Virtual(VItem::DayDivider(ts)) => { - Some(VirtualTimelineItem::DayDivider { ts: ts.0.into() }) - } - Item::Virtual(VItem::ReadMarker) => Some(VirtualTimelineItem::ReadMarker), - Item::Event(_) => None, + use matrix_sdk_ui::timeline::VirtualTimelineItem as VItem; + match self.0.as_virtual()? { + VItem::DayDivider(ts) => Some(VirtualTimelineItem::DayDivider { ts: ts.0.into() }), + VItem::ReadMarker => Some(VirtualTimelineItem::ReadMarker), } } + pub fn unique_id(&self) -> u64 { + self.0.unique_id() + } + pub fn fmt_debug(&self) -> String { format!("{:#?}", self.0) } @@ -281,10 +280,6 @@ impl EventTimelineItem { !self.0.is_local_echo() } - pub fn unique_identifier(&self) -> String { - self.0.unique_identifier() - } - pub fn transaction_id(&self) -> Option { self.0.transaction_id().map(ToString::to_string) } diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index 989ddd630..3538c83bb 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -46,10 +46,10 @@ use super::{ EventTimelineItemKind, LocalEventTimelineItem, Profile, RemoteEventOrigin, RemoteEventTimelineItem, }, - find_read_marker, + find_read_marker, new_timeline_item, read_receipts::maybe_add_implicit_read_receipt, - rfind_event_by_id, rfind_event_item, EventTimelineItem, Message, OtherState, ReactionGroup, - Sticker, TimelineDetails, TimelineInnerState, TimelineItem, TimelineItemContent, + rfind_event_by_id, rfind_event_item, timeline_item, EventTimelineItem, Message, OtherState, + ReactionGroup, Sticker, TimelineDetails, TimelineInnerState, TimelineItem, TimelineItemContent, VirtualTimelineItem, DEFAULT_SANITIZER_MODE, }; use crate::{events::SyncTimelineEventWithoutContent, timeline::event_item::ReactionSenderData}; @@ -209,6 +209,7 @@ pub(super) struct TimelineEventHandler<'a> { meta: TimelineEventMetadata, flow: Flow, items: &'a mut ObservableVector>, + next_internal_id: &'a mut u64, reaction_map: &'a mut HashMap, pending_reactions: &'a mut HashMap>, fully_read_event: &'a mut Option, @@ -245,6 +246,7 @@ impl<'a> TimelineEventHandler<'a> { meta: event_meta, flow, items: &mut state.items, + next_internal_id: &mut state.next_internal_id, reaction_map: &mut state.reaction_map, pending_reactions: &mut state.pending_reactions, fully_read_event: &mut state.fully_read_event, @@ -474,9 +476,7 @@ impl<'a> TimelineEventHandler<'a> { trace!("Adding reaction"); self.items.set( idx, - Arc::new(TimelineItem::Event( - event_item.with_kind(remote_event_item.with_reactions(reactions)), - )), + event_item.with_inner_kind(remote_event_item.with_reactions(reactions)), ); self.result.items_updated += 1; } @@ -707,19 +707,24 @@ impl<'a> TimelineEventHandler<'a> { { let old_ts = latest_event.timestamp(); - if let Some(day_divider_item) = - maybe_create_day_divider_from_timestamps(old_ts, timestamp) - { + if let Some(day_divider_item) = maybe_create_day_divider_from_timestamps( + old_ts, + timestamp, + self.next_internal_id, + ) { trace!("Adding day divider (local)"); - self.items.push_back(Arc::new(day_divider_item)); + self.items.push_back(day_divider_item); } } else { // If there is no event item, there is no day divider yet. trace!("Adding first day divider (local)"); - self.items.push_back(Arc::new(TimelineItem::day_divider(timestamp))); + self.items.push_back(new_timeline_item( + VirtualTimelineItem::DayDivider(timestamp), + self.next_internal_id, + )); } - self.items.push_back(Arc::new(item.into())); + self.items.push_back(new_timeline_item(item, self.next_internal_id)); } Flow::Remote { position: TimelineItemPosition::Start, event_id, .. } => { @@ -739,14 +744,19 @@ impl<'a> TimelineEventHandler<'a> { if let Some(VirtualTimelineItem::DayDivider(divider_ts)) = self.items.front().and_then(|item| item.as_virtual()) { - if let Some(day_divider_item) = - maybe_create_day_divider_from_timestamps(*divider_ts, timestamp) - { - self.items.push_front(Arc::new(day_divider_item)); + if let Some(day_divider_item) = maybe_create_day_divider_from_timestamps( + *divider_ts, + timestamp, + self.next_internal_id, + ) { + self.items.push_front(day_divider_item); } } else { // The list must always start with a day divider. - self.items.push_front(Arc::new(TimelineItem::day_divider(timestamp))); + self.items.push_front(new_timeline_item( + VirtualTimelineItem::DayDivider(timestamp), + self.next_internal_id, + )); } if self.track_read_receipts { @@ -759,7 +769,7 @@ impl<'a> TimelineEventHandler<'a> { ); } - self.items.insert(1, Arc::new(item.into())); + self.items.insert(1, new_timeline_item(item, self.next_internal_id)); } Flow::Remote { @@ -775,7 +785,7 @@ impl<'a> TimelineEventHandler<'a> { // Item was previously received from the server. This // should be very rare normally, but with the sliding- // sync proxy, it is actually very common. - trace!(?item, ?old_item, "Received duplicate event"); + trace!(?item, old_item = ?*old_item, "Received duplicate event"); if old_item.content.is_redacted() && !item.content.is_redacted() { warn!("Got original form of an event that was previously redacted"); @@ -799,6 +809,8 @@ impl<'a> TimelineEventHandler<'a> { // TODO: Check whether anything is different about the // old and new item? + let old_item_id = old_item.internal_id; + if idx == self.items.len() - 1 && timestamp_to_date(old_item.timestamp()) == timestamp_to_date(timestamp) { @@ -816,7 +828,7 @@ impl<'a> TimelineEventHandler<'a> { } trace!(idx, "Replacing existing event"); - self.items.set(idx, Arc::new(item.into())); + self.items.set(idx, timeline_item(item, old_item_id)); return; } @@ -888,14 +900,16 @@ impl<'a> TimelineEventHandler<'a> { // 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) - { + if let Some(day_divider_item) = maybe_create_day_divider_from_timestamps( + old_ts, + timestamp, + self.next_internal_id, + ) { trace!("Adding day divider (remote)"); if should_push { - self.items.push_back(Arc::new(day_divider_item)); + self.items.push_back(day_divider_item); } else { - self.items.insert(insert_idx, Arc::new(day_divider_item)); + self.items.insert(insert_idx, day_divider_item); insert_idx += 1; } } @@ -903,10 +917,18 @@ impl<'a> TimelineEventHandler<'a> { // If there is no event item, there is no day divider yet. trace!("Adding first day divider (remote)"); if should_push { - self.items.push_back(Arc::new(TimelineItem::day_divider(timestamp))); + self.items.push_back(new_timeline_item( + VirtualTimelineItem::DayDivider(timestamp), + self.next_internal_id, + )); } else { - self.items - .insert(insert_idx, Arc::new(TimelineItem::day_divider(timestamp))); + self.items.insert( + insert_idx, + new_timeline_item( + VirtualTimelineItem::DayDivider(timestamp), + self.next_internal_id, + ), + ); insert_idx += 1; } } @@ -923,16 +945,16 @@ impl<'a> TimelineEventHandler<'a> { trace!("Adding new remote timeline item after all non-pending events"); if should_push { - self.items.push_back(Arc::new(item.into())); + self.items.push_back(new_timeline_item(item, self.next_internal_id)); } else { - self.items.insert(insert_idx, Arc::new(item.into())); + self.items.insert(insert_idx, new_timeline_item(item, self.next_internal_id)); } } #[cfg(feature = "e2e-encryption")] Flow::Remote { position: TimelineItemPosition::Update(idx), .. } => { trace!("Updating timeline item at position {idx}"); - self.items.set(*idx, Arc::new(item.into())); + self.items.set(*idx, new_timeline_item(item, self.next_internal_id)); } } @@ -994,7 +1016,7 @@ pub(crate) fn update_read_marker( // We don't want to insert the read marker if it is at the end of the timeline. if idx + 1 < items.len() { *event_should_update_fully_read_marker = false; - items.insert(idx + 1, Arc::new(TimelineItem::read_marker())); + items.insert(idx + 1, TimelineItem::read_marker()); } else { *event_should_update_fully_read_marker = true; } @@ -1035,9 +1057,9 @@ fn _update_timeline_item( ) { if let Some((idx, item)) = rfind_event_by_id(items, event_id) { trace!("Found timeline item to update"); - if let Some(new_item) = update(item) { + if let Some(new_item) = update(item.inner) { trace!("Updating item"); - items.set(idx, Arc::new(TimelineItem::Event(new_item))); + items.set(idx, timeline_item(new_item, item.internal_id)); *items_updated += 1; } } else { @@ -1070,7 +1092,8 @@ fn timestamp_to_date(ts: MilliSecondsSinceUnixEpoch) -> Date { fn maybe_create_day_divider_from_timestamps( old_ts: MilliSecondsSinceUnixEpoch, new_ts: MilliSecondsSinceUnixEpoch, -) -> Option { + next_internal_id: &mut u64, +) -> Option> { (timestamp_to_date(old_ts) != timestamp_to_date(new_ts)) - .then(|| TimelineItem::day_divider(new_ts)) + .then(|| new_timeline_item(VirtualTimelineItem::DayDivider(new_ts), next_internal_id)) } 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 f4e6210df..221d7b3dc 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs @@ -200,19 +200,6 @@ impl EventTimelineItem { } } - /// Get a unique identifier to identify the event item, either by using - /// transaction ID or event ID in case of a local event, or by event ID in - /// case of a remote event. - pub fn unique_identifier(&self) -> String { - match &self.kind { - EventTimelineItemKind::Local(item) => match &item.send_state { - EventSendState::Sent { event_id } => event_id.to_string(), - _ => item.transaction_id.to_string(), - }, - EventTimelineItemKind::Remote(item) => item.event_id.to_string(), - } - } - /// Get the event's send state, if it is a local echo. pub fn send_state(&self) -> Option<&EventSendState> { match &self.kind { diff --git a/crates/matrix-sdk-ui/src/timeline/inner.rs b/crates/matrix-sdk-ui/src/timeline/inner.rs index 9af52b42b..cde358d20 100644 --- a/crates/matrix-sdk-ui/src/timeline/inner.rs +++ b/crates/matrix-sdk-ui/src/timeline/inner.rs @@ -64,12 +64,13 @@ use super::{ }, event_item::{EventItemIdentifier, ReactionSenderData}, reactions::ReactionToggleResult, - rfind_event_by_id, rfind_event_item, + rfind_event_by_id, rfind_event_item, timeline_item, traits::RoomDataProvider, AnnotationKey, EventSendState, EventTimelineItem, InReplyToDetails, Message, Profile, RelativePosition, RepliedToEvent, TimelineDetails, TimelineItem, TimelineItemContent, + TimelineItemKind, }; -use crate::events::SyncTimelineEventWithoutContent; +use crate::{events::SyncTimelineEventWithoutContent, timeline::new_timeline_item}; #[derive(Clone, Debug)] pub(super) struct TimelineInner { @@ -81,6 +82,7 @@ pub(super) struct TimelineInner { #[derive(Debug, Default)] pub(super) struct TimelineInnerState { pub(super) items: ObservableVector>, + pub(super) next_internal_id: u64, /// Reaction event / txn ID => sender and reaction data. pub(super) reaction_map: HashMap, /// ID of event that is not in the timeline yet => List of reaction event @@ -602,8 +604,8 @@ impl TimelineInner

{ let is_error = matches!(send_state, EventSendState::SendingFailed { .. }); - let new_item = TimelineItem::Event(item.with_kind(local_item.with_send_state(send_state))); - state.items.set(idx, Arc::new(new_item)); + let new_item = item.with_inner_kind(local_item.with_send_state(send_state)); + state.items.set(idx, new_item); if is_error { // When there is an error, sending further messages is paused. This @@ -611,12 +613,13 @@ impl TimelineInner

{ // events to cancelled. let num_items = state.items.len(); for idx in 0..num_items { - let Some(item) = state.items[idx].as_event() else { continue }; - let Some(local_item) = item.as_local() else { continue }; + let item = state.items[idx].clone(); + let Some(event_item) = item.as_event() else { continue }; + let Some(local_item) = event_item.as_local() else { continue }; if matches!(&local_item.send_state, EventSendState::NotSentYet) { - let new_item = - item.with_kind(local_item.with_send_state(EventSendState::Cancelled)); - state.items.set(idx, Arc::new(TimelineItem::Event(new_item))); + let new_event_item = + event_item.with_kind(local_item.with_send_state(EventSendState::Cancelled)); + state.items.set(idx, item.with_kind(new_event_item)); } } } @@ -691,12 +694,9 @@ impl TimelineInner

{ EventSendState::SendingFailed { .. } | EventSendState::Cancelled => {} } - let new_item = TimelineItem::Event( - item.with_kind(local_item.with_send_state(EventSendState::NotSentYet)), - ); - + let new_item = item.with_inner_kind(local_item.with_send_state(EventSendState::NotSentYet)); let content = item.content.clone(); - state.items.set(idx, Arc::new(new_item)); + state.items.set(idx, new_item); Some(content) } @@ -894,9 +894,10 @@ impl TimelineInner

{ async fn set_non_ready_sender_profiles(&self, profile_state: TimelineDetails) { let mut state = self.state.lock().await; for idx in 0..state.items.len() { - let Some(event_item) = state.items[idx].as_event() else { continue }; + let item = state.items[idx].clone(); + let Some(event_item) = item.as_event() else { continue }; if !matches!(event_item.sender_profile(), TimelineDetails::Ready(_)) { - let item = Arc::new(TimelineItem::Event( + let item = item.with_kind(TimelineItemKind::Event( event_item.with_sender_profile(profile_state.clone()), )); state.items.set(idx, item); @@ -919,20 +920,21 @@ impl TimelineInner

{ assert_eq!(state.items.len(), num_items); - let event_item = state.items[idx].as_event().unwrap(); + let item = state.items[idx].clone(); + let event_item = item.as_event().unwrap(); match maybe_profile { Some(profile) => { if !event_item.sender_profile().contains(&profile) { let updated_item = event_item.with_sender_profile(TimelineDetails::Ready(profile)); - state.items.set(idx, Arc::new(TimelineItem::Event(updated_item))); + state.items.set(idx, item.with_kind(updated_item)); } } None => { if !event_item.sender_profile().is_unavailable() { let updated_item = event_item.with_sender_profile(TimelineDetails::Unavailable); - state.items.set(idx, Arc::new(TimelineItem::Event(updated_item))); + state.items.set(idx, item.with_kind(updated_item)); } } } @@ -1028,6 +1030,7 @@ impl TimelineInner { }; trace!("Updating in-reply-to details"); + let internal_id = item.internal_id; let mut item = item.clone(); item.set_content(TimelineItemContent::Message( message.with_in_reply_to(InReplyToDetails { @@ -1035,7 +1038,7 @@ impl TimelineInner { event, }), )); - state.items.set(index, Arc::new(item.into())); + state.items.set(index, timeline_item(item, internal_id)); Ok(()) } @@ -1279,7 +1282,9 @@ async fn fetch_replied_to_event( event: TimelineDetails::Pending, }); let event_item = item.with_content(TimelineItemContent::Message(reply), None); - state.items.set(index, Arc::new(event_item.into())); + + let state_ref = &mut *state; + state_ref.items.set(index, new_timeline_item(event_item, &mut state_ref.next_internal_id)); // Don't hold the state lock while the network request is made drop(state); @@ -1384,7 +1389,7 @@ fn update_timeline_reaction( } } - state.items.set(idx, Arc::new(TimelineItem::Event(new_related))); + state.items.set(idx, timeline_item(new_related, related.internal_id)); Ok(()) } diff --git a/crates/matrix-sdk-ui/src/timeline/mod.rs b/crates/matrix-sdk-ui/src/timeline/mod.rs index e269078ce..2618f1f16 100644 --- a/crates/matrix-sdk-ui/src/timeline/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/mod.rs @@ -16,7 +16,7 @@ //! //! See [`Timeline`] for details. -use std::{pin::Pin, sync::Arc, task::Poll, time::Duration}; +use std::{ops::Deref, pin::Pin, sync::Arc, task::Poll, time::Duration}; use async_std::sync::{Condvar, Mutex}; use eyeball::{SharedObservable, Subscriber}; @@ -42,7 +42,7 @@ use ruma::{ room::{message::sanitize::HtmlSanitizerMode, redaction::RoomRedactionEventContent}, AnyMessageLikeEventContent, }, - EventId, MilliSecondsSinceUnixEpoch, OwnedEventId, OwnedTransactionId, TransactionId, UserId, + EventId, OwnedEventId, OwnedTransactionId, TransactionId, UserId, }; use thiserror::Error; use tokio::sync::mpsc::Sender; @@ -82,6 +82,7 @@ pub use self::{ virtual_item::VirtualTimelineItem, }; use self::{ + event_item::EventTimelineItemKind, inner::{ReactionAction, TimelineInner, TimelineInnerState}, queue::LocalMessage, reactions::ReactionToggleResult, @@ -708,10 +709,9 @@ impl Stream for TimelineStream { } } -/// A single entry in timeline. #[derive(Clone, Debug)] #[allow(clippy::large_enum_variant)] -pub enum TimelineItem { +pub enum TimelineItemKind { /// An event or aggregation of multiple events. Event(EventTimelineItem), /// An item that doesn't correspond to an event, for example the user's own @@ -719,75 +719,144 @@ pub enum TimelineItem { Virtual(VirtualTimelineItem), } +/// A single entry in timeline. +#[derive(Clone, Debug)] +pub struct TimelineItem { + kind: TimelineItemKind, + internal_id: u64, +} + impl TimelineItem { - /// Get the inner `EventTimelineItem`, if this is a `TimelineItem::Event`. + pub(crate) fn with_kind(&self, kind: impl Into) -> Arc { + Arc::new(Self { kind: kind.into(), internal_id: self.internal_id }) + } + + /// Get the inner `EventTimelineItem`, if this is a + /// `TimelineItemKind::Event`. pub fn as_event(&self) -> Option<&EventTimelineItem> { - match self { - Self::Event(v) => Some(v), + match &self.kind { + TimelineItemKind::Event(v) => Some(v), _ => None, } } /// Get the inner `VirtualTimelineItem`, if this is a - /// `TimelineItem::Virtual`. + /// `TimelineItemKind::Virtual`. pub fn as_virtual(&self) -> Option<&VirtualTimelineItem> { - match self { - Self::Virtual(v) => Some(v), + match &self.kind { + TimelineItemKind::Virtual(v) => Some(v), _ => None, } } - /// Creates a new day divider from the given timestamp. - fn day_divider(ts: MilliSecondsSinceUnixEpoch) -> Self { - Self::Virtual(VirtualTimelineItem::DayDivider(ts)) + /// Get a unique ID for this timeline item. + /// + /// It identifies the item on a best-effort basis. For instance, edits to an + /// [`EventTimelineItem`] will not change the ID of the enclosing + /// `TimelineItem`. For some virtual items like day dividers, identity isn't + /// easy to define though and you might see a new ID getting generated for a + /// day divider that you perceive to be "the same" as a previous one. + pub fn unique_id(&self) -> u64 { + self.internal_id } - fn read_marker() -> Self { - Self::Virtual(VirtualTimelineItem::ReadMarker) + fn read_marker() -> Arc { + Arc::new(Self { + kind: TimelineItemKind::Virtual(VirtualTimelineItem::ReadMarker), + internal_id: u64::MAX, + }) } fn is_virtual(&self) -> bool { - matches!(self, Self::Virtual(_)) + matches!(self.kind, TimelineItemKind::Virtual(_)) } fn is_day_divider(&self) -> bool { - matches!(self, Self::Virtual(VirtualTimelineItem::DayDivider(_))) + matches!(self.kind, TimelineItemKind::Virtual(VirtualTimelineItem::DayDivider(_))) } fn is_read_marker(&self) -> bool { - matches!(self, Self::Virtual(VirtualTimelineItem::ReadMarker)) + matches!(self.kind, TimelineItemKind::Virtual(VirtualTimelineItem::ReadMarker)) } } -impl From for TimelineItem { +impl Deref for TimelineItem { + type Target = TimelineItemKind; + + fn deref(&self) -> &Self::Target { + &self.kind + } +} + +impl From for TimelineItemKind { fn from(item: EventTimelineItem) -> Self { Self::Event(item) } } -impl From for TimelineItem { +impl From for TimelineItemKind { fn from(item: VirtualTimelineItem) -> Self { Self::Virtual(item) } } +fn timeline_item(kind: impl Into, internal_id: u64) -> Arc { + Arc::new(TimelineItem { kind: kind.into(), internal_id }) +} + +fn new_timeline_item( + kind: impl Into, + next_internal_id: &mut u64, +) -> Arc { + let internal_id = *next_internal_id; + *next_internal_id += 1; + timeline_item(kind, internal_id) +} + +struct EventTimelineItemWithId<'a> { + inner: &'a EventTimelineItem, + internal_id: u64, +} + +impl<'a> EventTimelineItemWithId<'a> { + fn with_inner_kind(&self, kind: impl Into) -> Arc { + Arc::new(TimelineItem { + kind: self.inner.with_kind(kind).into(), + internal_id: self.internal_id, + }) + } +} + +impl Deref for EventTimelineItemWithId<'_> { + type Target = EventTimelineItem; + + fn deref(&self) -> &Self::Target { + self.inner + } +} + // FIXME: Put an upper bound on timeline size or add a separate map to look up // the index of a timeline item by its key, to avoid large linear scans. fn rfind_event_item( items: &Vector>, mut f: impl FnMut(&EventTimelineItem) -> bool, -) -> Option<(usize, &EventTimelineItem)> { +) -> Option<(usize, EventTimelineItemWithId<'_>)> { items .iter() .enumerate() - .filter_map(|(idx, item)| Some((idx, item.as_event()?))) - .rfind(|(_, it)| f(it)) + .filter_map(|(idx, item)| { + Some(( + idx, + EventTimelineItemWithId { inner: item.as_event()?, internal_id: item.internal_id }, + )) + }) + .rfind(|(_, it)| f(it.inner)) } fn rfind_event_by_id<'a>( items: &'a Vector>, event_id: &EventId, -) -> Option<(usize, &'a EventTimelineItem)> { +) -> Option<(usize, EventTimelineItemWithId<'a>)> { rfind_event_item(items, |it| it.event_id() == Some(event_id)) } diff --git a/crates/matrix-sdk-ui/src/timeline/read_receipts.rs b/crates/matrix-sdk-ui/src/timeline/read_receipts.rs index 202317024..48db373c9 100644 --- a/crates/matrix-sdk-ui/src/timeline/read_receipts.rs +++ b/crates/matrix-sdk-ui/src/timeline/read_receipts.rs @@ -25,7 +25,8 @@ use tracing::{error, warn}; use super::{ compare_events_positions, event_item::EventTimelineItemKind, inner::TimelineInnerState, - rfind_event_by_id, traits::RoomDataProvider, EventTimelineItem, RelativePosition, TimelineItem, + rfind_event_by_id, timeline_item, traits::RoomDataProvider, EventTimelineItem, + RelativePosition, TimelineItem, }; struct FullReceipt<'a> { @@ -44,10 +45,11 @@ impl TimelineInnerState { receipt: Receipt, ) { let Some(pos) = receipt_item_pos else { return }; - let Some(mut event_item) = self.items[pos].as_event().cloned() else { return }; + let timeline_item = self.items[pos].clone(); + let Some(mut event_item) = timeline_item.as_event().cloned() else { return }; event_item.as_remote_mut().unwrap().add_read_receipt(user_id, receipt); - self.items.set(pos, Arc::new(event_item.into())); + self.items.set(pos, timeline_item.with_kind(event_item)); } pub(super) fn handle_explicit_read_receipts( @@ -273,6 +275,7 @@ fn maybe_update_read_receipt( if !is_own_user_id { // Remove the read receipt for this user from the old event. + let old_event_item_id = old_event_item.internal_id; let mut old_event_item = old_event_item.clone(); if let Some(old_remote_event_item) = old_event_item.as_remote_mut() { if !old_remote_event_item.remove_read_receipt(receipt.user_id) { @@ -281,7 +284,8 @@ fn maybe_update_read_receipt( receipt doesn't have a receipt for the user" ); } - timeline_items.set(old_receipt_pos, Arc::new(old_event_item.into())); + timeline_items + .set(old_receipt_pos, timeline_item(old_event_item, old_event_item_id)); } else { warn!("received a read receipt for a local item, this should not be possible"); } diff --git a/crates/matrix-sdk-ui/src/timeline/tests/basic.rs b/crates/matrix-sdk-ui/src/timeline/tests/basic.rs index 152f98920..affef7533 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/basic.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/basic.rs @@ -34,8 +34,8 @@ use stream_assert::assert_next_matches; use super::{sync_timeline_event, TestTimeline, ALICE, BOB}; use crate::timeline::{ - event_item::AnyOtherFullStateEventContent, MembershipChange, TimelineDetails, TimelineItem, - TimelineItemContent, VirtualTimelineItem, + event_item::AnyOtherFullStateEventContent, MembershipChange, TimelineDetails, + TimelineItemContent, TimelineItemKind, VirtualTimelineItem, }; #[async_test] @@ -56,7 +56,7 @@ async fn initial_events() { .await; let item = assert_next_matches!(stream, VectorDiff::PushBack { value } => value); - assert_matches!(&*item, TimelineItem::Virtual(VirtualTimelineItem::DayDivider(_))); + assert_matches!(&item.kind, TimelineItemKind::Virtual(VirtualTimelineItem::DayDivider(_))); let item = assert_next_matches!(stream, VectorDiff::PushBack { value } => value); assert_eq!(item.as_event().unwrap().sender(), *ALICE); let item = assert_next_matches!(stream, VectorDiff::PushBack { value } => value); @@ -203,8 +203,11 @@ async fn dedup_pagination() { let timeline_items = timeline.inner.items().await; assert_eq!(timeline_items.len(), 2); - assert_matches!(*timeline_items[0], TimelineItem::Virtual(VirtualTimelineItem::DayDivider(_))); - assert_matches!(*timeline_items[1], TimelineItem::Event(_)); + assert_matches!( + timeline_items[0].kind, + TimelineItemKind::Virtual(VirtualTimelineItem::DayDivider(_)) + ); + assert_matches!(timeline_items[1].kind, TimelineItemKind::Event(_)); } #[async_test] diff --git a/crates/matrix-sdk-ui/src/timeline/tests/virt.rs b/crates/matrix-sdk-ui/src/timeline/tests/virt.rs index 210c4630c..9b0e56cd5 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/virt.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/virt.rs @@ -23,7 +23,7 @@ use ruma::{ use stream_assert::assert_next_matches; use super::{TestTimeline, ALICE, BOB}; -use crate::timeline::{TimelineItem, VirtualTimelineItem}; +use crate::timeline::{TimelineItemKind, VirtualTimelineItem}; #[async_test] async fn day_divider() { @@ -132,7 +132,7 @@ async fn update_read_marker() { // Now the read marker is reinserted after the second event. let marker = assert_next_matches!(stream, VectorDiff::Insert { index: 3, value } => value); - assert_matches!(*marker, TimelineItem::Virtual(VirtualTimelineItem::ReadMarker)); + assert_matches!(marker.kind, TimelineItemKind::Virtual(VirtualTimelineItem::ReadMarker)); // Nothing should happen if the fully read event is set back to an older event. timeline.inner.set_fully_read_event(first_event_id).await; @@ -153,5 +153,5 @@ async fn update_read_marker() { // The read marker is moved after the third event. assert_next_matches!(stream, VectorDiff::Remove { index: 3 }); let marker = assert_next_matches!(stream, VectorDiff::Insert { index: 4, value } => value); - assert_matches!(*marker, TimelineItem::Virtual(VirtualTimelineItem::ReadMarker)); + assert_matches!(marker.kind, TimelineItemKind::Virtual(VirtualTimelineItem::ReadMarker)); } diff --git a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs index 98397981d..bf94c04fb 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs @@ -11,7 +11,7 @@ use matrix_sdk_ui::{ ALL_ROOMS_LIST_NAME as ALL_ROOMS, INVITES_LIST_NAME as INVITES, VISIBLE_ROOMS_LIST_NAME as VISIBLE_ROOMS, }, - timeline::{TimelineItem, VirtualTimelineItem}, + timeline::{TimelineItemKind, VirtualTimelineItem}, RoomListService, }; use ruma::{ @@ -2027,12 +2027,12 @@ async fn test_room_timeline() -> Result<(), Error> { // Previous timeline items. assert_matches!( - previous_timeline_items[0].as_ref(), - TimelineItem::Virtual(VirtualTimelineItem::DayDivider(_)) + **previous_timeline_items[0], + TimelineItemKind::Virtual(VirtualTimelineItem::DayDivider(_)) ); assert_matches!( - previous_timeline_items[1].as_ref(), - TimelineItem::Event(item) => { + &**previous_timeline_items[1], + TimelineItemKind::Event(item) => { assert_eq!(item.event_id().unwrap().as_str(), "$x0:bar.org"); } ); diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/echo.rs b/crates/matrix-sdk-ui/tests/integration/timeline/echo.rs index 570cbe493..4d5ea66a3 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/echo.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/echo.rs @@ -20,7 +20,7 @@ use futures_util::StreamExt; use matrix_sdk::{config::SyncSettings, executor::spawn, ruma::MilliSecondsSinceUnixEpoch}; use matrix_sdk_test::{async_test, EventBuilder, JoinedRoomBuilder, TimelineTestEvent}; use matrix_sdk_ui::timeline::{ - EventSendState, RoomExt, TimelineItem, TimelineItemContent, VirtualTimelineItem, + EventSendState, RoomExt, TimelineItemContent, TimelineItemKind, VirtualTimelineItem, }; use ruma::{ event_id, @@ -121,7 +121,7 @@ async fn echo() { timeline_stream.next().await, Some(VectorDiff::PushBack { value }) => value ); - assert_matches!(&*new_item, TimelineItem::Virtual(VirtualTimelineItem::DayDivider(_))); + assert_matches!(**new_item, TimelineItemKind::Virtual(VirtualTimelineItem::DayDivider(_))); // Remote echo is added let remote_echo = assert_matches!( 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 7b2a2bbdb..dd985cc39 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/sliding_sync.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/sliding_sync.rs @@ -22,7 +22,9 @@ use matrix_sdk::{ SlidingSync, SlidingSyncList, SlidingSyncListBuilder, SlidingSyncMode, UpdateSummary, }; use matrix_sdk_test::async_test; -use matrix_sdk_ui::timeline::{SlidingSyncRoomExt, TimelineItem, VirtualTimelineItem}; +use matrix_sdk_ui::timeline::{ + SlidingSyncRoomExt, TimelineItem, TimelineItemKind, VirtualTimelineItem, +}; use ruma::{room_id, RoomId}; use serde_json::json; use wiremock::{http::Method, Match, Mock, MockServer, Request, ResponseTemplate}; @@ -80,8 +82,8 @@ macro_rules! assert_timeline_stream { $stream.next().now_or_never(), Some(Some(VectorDiff::PushBack { value })) => { assert_matches!( - value.as_ref(), - TimelineItem::Virtual( + **value, + TimelineItemKind::Virtual( VirtualTimelineItem::DayDivider(_) ) ); @@ -105,8 +107,8 @@ macro_rules! assert_timeline_stream { $stream.next().now_or_never(), Some(Some(VectorDiff::PushBack { value })) => { assert_matches!( - value.as_ref(), - TimelineItem::Event(event_timeline_item) => { + &**value, + TimelineItemKind::Event(event_timeline_item) => { assert_eq!(event_timeline_item.event_id().unwrap().as_str(), $event_id); } ); @@ -130,8 +132,8 @@ macro_rules! assert_timeline_stream { $stream.next().now_or_never(), Some(Some(VectorDiff::Insert { index: $index, value })) => { assert_matches!( - value.as_ref(), - TimelineItem::Event(event_timeline_item) => { + &**value, + TimelineItemKind::Event(event_timeline_item) => { assert_eq!(event_timeline_item.event_id().unwrap().as_str(), $event_id); } ); @@ -155,8 +157,8 @@ macro_rules! assert_timeline_stream { $stream.next().now_or_never(), Some(Some(VectorDiff::Set { index: $index, value })) => { assert_matches!( - value.as_ref(), - TimelineItem::Event(event_timeline_item) => { + &**value, + TimelineItemKind::Event(event_timeline_item) => { assert_eq!(event_timeline_item.event_id().unwrap().as_str(), $event_id); } );