feat(timeline): introduce the TimelineUniqueId

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 {

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,

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.

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<TimelineItemKind>, internal_id: String) -> Arc<Self> {
+    pub(crate) fn new(
+        kind: impl Into<TimelineItemKind>,
+        internal_id: TimelineUniqueId,
+    ) -> Arc<Self> {
         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<TimelineItem> {
         Arc::new(Self {
             kind: TimelineItemKind::Virtual(VirtualTimelineItem::ReadMarker),
-            internal_id: "__read_marker".to_owned(),
+            internal_id: TimelineUniqueId("__read_marker".to_owned()),
         })
     }
 

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,

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]

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]

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<EventTimelineItemKind>) -> Arc<TimelineItem> {
-        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<TimelineItem> {
         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())
     }
 }
 

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]