refactor(timeline): introduce TimelineUniqueId as an opaque type fo…

…r the unique identifier

We can now use this type instead of passing a string, which means
there's no way to confuse oneself in methods like
`toggle_reaction_local`.

Changelog: Introduced `TimelineUniqueId`, returned by
`TimelineItem::unique_id()` and serving as an opaque identifier to use
in other methods modifying the timeline item (e.g. `toggle_reaction`).

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,
@@ -977,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 = 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]