diff --git a/bindings/matrix-sdk-ffi/src/timeline/mod.rs b/bindings/matrix-sdk-ffi/src/timeline/mod.rs index 66d686fcb49..0dd994da43d 100644 --- a/bindings/matrix-sdk-ffi/src/timeline/mod.rs +++ b/bindings/matrix-sdk-ffi/src/timeline/mod.rs @@ -32,6 +32,7 @@ use matrix_sdk::{ }; use matrix_sdk_ui::timeline::{ EventItemOrigin, LiveBackPaginationStatus, Profile, RepliedToEvent, TimelineDetails, + TimelineUniqueId as SdkTimelineUniqueId, }; use mime::Mime; use ruma::{ @@ -868,6 +869,23 @@ pub enum TimelineChange { Reset, } +#[derive(Clone, uniffi::Record)] +pub struct TimelineUniqueId { + id: String, +} + +impl From<&SdkTimelineUniqueId> for TimelineUniqueId { + fn from(value: &SdkTimelineUniqueId) -> Self { + Self { id: value.0.clone() } + } +} + +impl From<&TimelineUniqueId> for SdkTimelineUniqueId { + fn from(value: &TimelineUniqueId) -> Self { + Self(value.id.clone()) + } +} + #[repr(transparent)] #[derive(Clone, uniffi::Object)] pub struct TimelineItem(pub(crate) matrix_sdk_ui::timeline::TimelineItem); @@ -895,8 +913,9 @@ impl TimelineItem { } } - pub fn unique_id(&self) -> String { - self.0.unique_id().to_owned() + /// An opaque unique identifier for this timeline item. + pub fn unique_id(&self) -> TimelineUniqueId { + self.0.unique_id().into() } pub fn fmt_debug(&self) -> String { diff --git a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs index 930facfa69b..6dbab04a5db 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs @@ -58,6 +58,7 @@ pub(super) use self::state::{ use super::{ event_handler::TimelineEventKind, event_item::{ReactionStatus, RemoteEventOrigin}, + item::TimelineUniqueId, traits::{Decryptor, RoomDataProvider}, util::{rfind_event_by_id, rfind_event_item, RelativePosition}, Error, EventSendState, EventTimelineItem, InReplyToDetails, Message, PaginationError, Profile, @@ -1541,7 +1542,7 @@ async fn fetch_replied_to_event( mut state: RwLockWriteGuard<'_, TimelineState>, index: usize, item: &EventTimelineItem, - internal_id: String, + internal_id: TimelineUniqueId, message: &Message, in_reply_to: &EventId, room: &Room, diff --git a/crates/matrix-sdk-ui/src/timeline/controller/state.rs b/crates/matrix-sdk-ui/src/timeline/controller/state.rs index 41f4f163b16..4f23f58a426 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/state.rs @@ -54,6 +54,7 @@ use crate::{ TimelineItemPosition, }, event_item::{PollState, RemoteEventOrigin, ResponseData}, + item::TimelineUniqueId, reactions::Reactions, read_receipts::ReadReceipts, traits::RoomDataProvider, @@ -623,11 +624,7 @@ impl TimelineStateTransaction<'_> { self.items.clear(); } - // Only clear the internal counter if there are no local echoes. Otherwise, we - // might end up reusing the same internal id for a local echo and - // another item. - let reset_internal_id = !has_local_echoes; - self.meta.clear(reset_internal_id); + self.meta.clear(); debug!(remaining_items = self.items.len(), "Timeline cleared"); } @@ -864,6 +861,13 @@ pub(in crate::timeline) struct TimelineMetadata { // **** DYNAMIC FIELDS **** /// The next internal identifier for timeline items, used for both local and /// remote echoes. + /// + /// This is never cleared, but always incremented, to avoid issues with + /// reusing a stale internal id across timeline clears. We don't expect + /// we can hit `u64::max_value()` realistically, but if this would + /// happen, we do a wrapping addition when incrementing this + /// id; the previous 0 value would have disappeared a long time ago, unless + /// the device has terabytes of RAM. next_internal_id: u64, /// List of all the events as received in the timeline, even the ones that @@ -929,10 +933,9 @@ impl TimelineMetadata { } } - pub(crate) fn clear(&mut self, reset_internal_id: bool) { - if reset_internal_id { - self.next_internal_id = 0; - } + pub(crate) fn clear(&mut self) { + // Note: we don't clear the next internal id to avoid bad cases of stale unique + // ids across timeline clears. self.all_events.clear(); self.reactions.clear(); self.pending_poll_events.clear(); @@ -975,11 +978,11 @@ impl TimelineMetadata { /// Returns the next internal id for a timeline item (and increment our /// internal counter). - fn next_internal_id(&mut self) -> String { + fn next_internal_id(&mut self) -> TimelineUniqueId { let val = self.next_internal_id; - self.next_internal_id += 1; + self.next_internal_id = self.next_internal_id.wrapping_add(1); let prefix = self.internal_id_prefix.as_deref().unwrap_or(""); - format!("{prefix}{val}") + TimelineUniqueId(format!("{prefix}{val}")) } /// Returns a new timeline item with a fresh internal id. diff --git a/crates/matrix-sdk-ui/src/timeline/item.rs b/crates/matrix-sdk-ui/src/timeline/item.rs index ba85a99efe4..8096da48c1a 100644 --- a/crates/matrix-sdk-ui/src/timeline/item.rs +++ b/crates/matrix-sdk-ui/src/timeline/item.rs @@ -18,6 +18,14 @@ use as_variant::as_variant; use super::{EventTimelineItem, VirtualTimelineItem}; +/// Opaque unique identifier for a timeline item. +/// +/// It is transferred whenever a timeline item is updated. This can be used as a +/// stable identifier for UI purposes, as well as operations on the event +/// represented by the item. +#[derive(Clone, Debug, PartialEq, Eq, Hash)] +pub struct TimelineUniqueId(pub String); + #[derive(Clone, Debug)] #[allow(clippy::large_enum_variant)] pub enum TimelineItemKind { @@ -32,12 +40,15 @@ pub enum TimelineItemKind { #[derive(Clone, Debug)] pub struct TimelineItem { pub(crate) kind: TimelineItemKind, - pub(crate) internal_id: String, + pub(crate) internal_id: TimelineUniqueId, } impl TimelineItem { /// Create a new `TimelineItem` with the given kind and internal id. - pub(crate) fn new(kind: impl Into, internal_id: String) -> Arc { + pub(crate) fn new( + kind: impl Into, + internal_id: TimelineUniqueId, + ) -> Arc { Arc::new(TimelineItem { kind: kind.into(), internal_id }) } @@ -71,14 +82,14 @@ impl TimelineItem { /// 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) -> &str { + pub fn unique_id(&self) -> &TimelineUniqueId { &self.internal_id } pub(crate) fn read_marker() -> Arc { Arc::new(Self { kind: TimelineItemKind::Virtual(VirtualTimelineItem::ReadMarker), - internal_id: "__read_marker".to_owned(), + internal_id: TimelineUniqueId("__read_marker".to_owned()), }) } diff --git a/crates/matrix-sdk-ui/src/timeline/mod.rs b/crates/matrix-sdk-ui/src/timeline/mod.rs index c364fe7cf86..35adf7e875a 100644 --- a/crates/matrix-sdk-ui/src/timeline/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/mod.rs @@ -89,7 +89,7 @@ pub use self::{ Sticker, TimelineDetails, TimelineEventItemId, TimelineItemContent, }, event_type_filter::TimelineEventTypeFilter, - item::{TimelineItem, TimelineItemKind}, + item::{TimelineItem, TimelineItemKind, TimelineUniqueId}, pagination::LiveBackPaginationStatus, traits::RoomExt, virtual_item::VirtualTimelineItem, diff --git a/crates/matrix-sdk-ui/src/timeline/tests/basic.rs b/crates/matrix-sdk-ui/src/timeline/tests/basic.rs index d0149b7b3a3..bf613932fdd 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/basic.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/basic.rs @@ -330,16 +330,16 @@ async fn test_dedup_initial() { let event2 = &timeline_items[2]; let event3 = &timeline_items[3]; - // Make sure the order is right + // Make sure the order is right. assert_eq!(event1.as_event().unwrap().sender(), *ALICE); assert_eq!(event2.as_event().unwrap().sender(), *BOB); assert_eq!(event3.as_event().unwrap().sender(), *CAROL); - // Make sure we reused IDs when deduplicating events - assert_eq!(event1.unique_id(), "0"); - assert_eq!(event2.unique_id(), "1"); - assert_eq!(event3.unique_id(), "2"); - assert_eq!(timeline_items[0].unique_id(), "3"); + // Make sure we reused IDs when deduplicating events. + assert_eq!(event1.unique_id().0, "0"); + assert_eq!(event2.unique_id().0, "1"); + assert_eq!(event3.unique_id().0, "2"); + assert_eq!(timeline_items[0].unique_id().0, "3"); } #[async_test] @@ -360,19 +360,19 @@ async fn test_internal_id_prefix() { assert_eq!(timeline_items.len(), 4); assert!(timeline_items[0].is_day_divider()); - assert_eq!(timeline_items[0].unique_id(), "le_prefix_3"); + assert_eq!(timeline_items[0].unique_id().0, "le_prefix_3"); let event1 = &timeline_items[1]; assert_eq!(event1.as_event().unwrap().sender(), *ALICE); - assert_eq!(event1.unique_id(), "le_prefix_0"); + assert_eq!(event1.unique_id().0, "le_prefix_0"); let event2 = &timeline_items[2]; assert_eq!(event2.as_event().unwrap().sender(), *BOB); - assert_eq!(event2.unique_id(), "le_prefix_1"); + assert_eq!(event2.unique_id().0, "le_prefix_1"); let event3 = &timeline_items[3]; assert_eq!(event3.as_event().unwrap().sender(), *CAROL); - assert_eq!(event3.unique_id(), "le_prefix_2"); + assert_eq!(event3.unique_id().0, "le_prefix_2"); } #[async_test] diff --git a/crates/matrix-sdk-ui/src/timeline/tests/echo.rs b/crates/matrix-sdk-ui/src/timeline/tests/echo.rs index d8d8b56f6d8..fea31476c78 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/echo.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/echo.rs @@ -85,7 +85,7 @@ async fn test_remote_echo_full_trip() { event_item.send_state(), Some(EventSendState::SendingFailed { is_recoverable: true, .. }) ); - assert_eq!(item.unique_id(), id); + assert_eq!(*item.unique_id(), id); } // Scenario 3: The local event has been sent successfully to the server and an @@ -104,7 +104,7 @@ async fn test_remote_echo_full_trip() { let event_item = item.as_event().unwrap(); assert!(event_item.is_local_echo()); assert_matches!(event_item.send_state(), Some(EventSendState::Sent { .. })); - assert_eq!(item.unique_id(), id); + assert_eq!(*item.unique_id(), id); event_item.timestamp() }; @@ -125,7 +125,7 @@ async fn test_remote_echo_full_trip() { // The local echo is replaced with the remote echo. let item = assert_next_matches!(stream, VectorDiff::Set { index: 1, value } => value); assert!(!item.as_event().unwrap().is_local_echo()); - assert_eq!(item.unique_id(), id); + assert_eq!(*item.unique_id(), id); } #[async_test] diff --git a/crates/matrix-sdk-ui/src/timeline/util.rs b/crates/matrix-sdk-ui/src/timeline/util.rs index 91159f8654c..3dd1302fa43 100644 --- a/crates/matrix-sdk-ui/src/timeline/util.rs +++ b/crates/matrix-sdk-ui/src/timeline/util.rs @@ -21,27 +21,27 @@ use ruma::{EventId, MilliSecondsSinceUnixEpoch}; #[cfg(doc)] use super::controller::TimelineMetadata; use super::{ - event_item::EventTimelineItemKind, EventTimelineItem, ReactionsByKeyBySender, - TimelineEventItemId, TimelineItem, + event_item::EventTimelineItemKind, item::TimelineUniqueId, EventTimelineItem, + ReactionsByKeyBySender, TimelineEventItemId, TimelineItem, }; pub(super) struct EventTimelineItemWithId<'a> { pub inner: &'a EventTimelineItem, /// Internal identifier generated by [`TimelineMetadata`]. - pub internal_id: &'a str, + pub internal_id: &'a TimelineUniqueId, } impl<'a> EventTimelineItemWithId<'a> { /// Create a clone of the underlying [`TimelineItem`] with the given kind. pub fn with_inner_kind(&self, kind: impl Into) -> Arc { - TimelineItem::new(self.inner.with_kind(kind), self.internal_id.to_owned()) + TimelineItem::new(self.inner.with_kind(kind), self.internal_id.clone()) } /// Create a clone of the underlying [`TimelineItem`] with the given /// reactions. pub fn with_reactions(&self, reactions: ReactionsByKeyBySender) -> Arc { let event_item = self.inner.with_reactions(reactions); - TimelineItem::new(event_item, self.internal_id.to_owned()) + TimelineItem::new(event_item, self.internal_id.clone()) } } diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/replies.rs b/crates/matrix-sdk-ui/tests/integration/timeline/replies.rs index f53218d854c..08b9cd2a638 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/replies.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/replies.rs @@ -133,12 +133,12 @@ async fn test_in_reply_to_details() { assert_let!(Some(VectorDiff::Set { index: 3, value: third }) = timeline_stream.next().await); assert_let!(TimelineItemContent::Message(message) = third.as_event().unwrap().content()); assert_matches!(message.in_reply_to().unwrap().event, TimelineDetails::Pending); - assert_eq!(third.unique_id(), unique_id); + assert_eq!(*third.unique_id(), unique_id); assert_let!(Some(VectorDiff::Set { index: 3, value: third }) = timeline_stream.next().await); assert_let!(TimelineItemContent::Message(message) = third.as_event().unwrap().content()); assert_matches!(message.in_reply_to().unwrap().event, TimelineDetails::Error(_)); - assert_eq!(third.unique_id(), unique_id); + assert_eq!(*third.unique_id(), unique_id); // Set up fetching the replied-to event to succeed Mock::given(method("GET")) @@ -162,12 +162,12 @@ async fn test_in_reply_to_details() { assert_let!(Some(VectorDiff::Set { index: 3, value: third }) = timeline_stream.next().await); assert_let!(TimelineItemContent::Message(message) = third.as_event().unwrap().content()); assert_matches!(message.in_reply_to().unwrap().event, TimelineDetails::Pending); - assert_eq!(third.unique_id(), unique_id); + assert_eq!(*third.unique_id(), unique_id); assert_let!(Some(VectorDiff::Set { index: 3, value: third }) = timeline_stream.next().await); assert_let!(TimelineItemContent::Message(message) = third.as_event().unwrap().content()); assert_matches!(message.in_reply_to().unwrap().event, TimelineDetails::Ready(_)); - assert_eq!(third.unique_id(), unique_id); + assert_eq!(*third.unique_id(), unique_id); } #[async_test]