timeline: use a TimelineItemId to react to a timeline item

Changelog: `Timeline::toggle_reaction` now identifies the item that's
reacted to with a `TimelineEventItemId`. There are drop-in conversions
from `OwnedEventId` and `OwnedTransactionId` to `TimelineEventItemId`
too.

bindings/matrix-sdk-ffi/src/timeline/mod.rs

@@ -530,19 +530,21 @@ impl Timeline {
 
     /// Toggle a reaction on an event.
     ///
-    /// The `unique_id` parameter is a string returned by
-    /// the `TimelineItem::unique_id()` method. As such, this method works both
-    /// on local echoes and remote items.
-    ///
     /// Adds or redacts a reaction based on the state of the reaction at the
     /// time it is called.
     ///
+    /// This method works both on local echoes and remote items.
+    ///
     /// When redacting a previous reaction, the redaction reason is not set.
     ///
     /// Ensures that only one reaction is sent at a time to avoid race
     /// conditions and spamming the homeserver with requests.
-    pub async fn toggle_reaction(&self, unique_id: String, key: String) -> Result<(), ClientError> {
-        self.inner.toggle_reaction(&unique_id, &key).await?;
+    pub async fn toggle_reaction(
+        &self,
+        item_id: EventOrTransactionId,
+        key: String,
+    ) -> Result<(), ClientError> {
+        self.inner.toggle_reaction(&(item_id.try_into()?), &key).await?;
         Ok(())
     }
 

crates/matrix-sdk-ui/src/timeline/controller/mod.rs

@@ -70,7 +70,7 @@ use crate::{
         event_item::EventTimelineItemKind,
         pinned_events_loader::{PinnedEventsLoader, PinnedEventsLoaderError},
         reactions::FullReactionKey,
-        util::rfind_event_by_uid,
+        util::rfind_event_by_item_id,
         TimelineEventFilterFn,
     },
     unable_to_decrypt_hook::UtdHookManager,
@@ -484,12 +484,12 @@ impl<P: RoomDataProvider> TimelineController<P> {
     #[instrument(skip_all)]
     pub(super) async fn toggle_reaction_local(
         &self,
-        unique_id: &str,
+        item_id: &TimelineEventItemId,
         key: &str,
     ) -> Result<bool, Error> {
         let mut state = self.state.write().await;
 
-        let Some((item_pos, item)) = rfind_event_by_uid(&state.items, unique_id) else {
+        let Some((item_pos, item)) = rfind_event_by_item_id(&state.items, item_id) else {
             warn!("Timeline item not found, can't add reaction");
             return Err(Error::FailedToToggleReaction);
         };
@@ -502,7 +502,7 @@ impl<P: RoomDataProvider> TimelineController<P> {
             .map(|reaction_info| reaction_info.status.clone());
 
         let Some(prev_status) = prev_status else {
-            match &item.inner.kind {
+            match &item.kind {
                 EventTimelineItemKind::Local(local) => {
                     if let Some(send_handle) = local.send_handle.clone() {
                         if send_handle

crates/matrix-sdk-ui/src/timeline/event_item/mod.rs

@@ -103,6 +103,19 @@ pub enum TimelineEventItemId {
     EventId(OwnedEventId),
 }
 
+impl From<OwnedEventId> for TimelineEventItemId {
+    fn from(value: OwnedEventId) -> Self {
+        Self::EventId(value)
+    }
+}
+
+impl From<OwnedTransactionId> for TimelineEventItemId {
+    fn from(value: OwnedTransactionId) -> Self {
+        Self::TransactionId(value)
+    }
+}
+
+// TODO remove those two
 impl From<String> for TimelineEventItemId {
     fn from(value: String) -> Self {
         value.as_str().into()

crates/matrix-sdk-ui/src/timeline/mod.rs

@@ -547,18 +547,19 @@ impl Timeline {
 
     /// Toggle a reaction on an event.
     ///
-    /// The `unique_id` parameter is a string returned by
-    /// [`TimelineItem::unique_id()`].
-    ///
     /// Adds or redacts a reaction based on the state of the reaction at the
     /// time it is called.
     ///
     /// When redacting a previous reaction, the redaction reason is not set.
     ///
     /// Ensures that only one reaction is sent at a time to avoid race
     /// conditions and spamming the homeserver with requests.
-    pub async fn toggle_reaction(&self, unique_id: &str, reaction_key: &str) -> Result<(), Error> {
-        self.controller.toggle_reaction_local(unique_id, reaction_key).await?;
+    pub async fn toggle_reaction(
+        &self,
+        item_id: &TimelineEventItemId,
+        reaction_key: &str,
+    ) -> Result<(), Error> {
+        self.controller.toggle_reaction_local(item_id, reaction_key).await?;
         Ok(())
     }
 

crates/matrix-sdk-ui/src/timeline/tests/mod.rs

@@ -60,7 +60,9 @@ use super::{
     event_handler::TimelineEventKind,
     event_item::RemoteEventOrigin,
     traits::RoomDataProvider,
-    EventTimelineItem, Profile, TimelineController, TimelineFocus, TimelineItem,
+    util::rfind_event_by_item_id,
+    EventTimelineItem, Profile, TimelineController, TimelineEventItemId, TimelineFocus,
+    TimelineItem,
 };
 use crate::{
     timeline::pinned_events_loader::PinnedEventsRoom, unable_to_decrypt_hook::UtdHookManager,
@@ -265,17 +267,16 @@ impl TestTimeline {
         self.controller.handle_read_receipts(ev_content).await;
     }
 
-    async fn toggle_reaction_local(&self, unique_id: &str, key: &str) -> Result<(), super::Error> {
-        if self.controller.toggle_reaction_local(unique_id, key).await? {
+    async fn toggle_reaction_local(
+        &self,
+        item_id: &TimelineEventItemId,
+        key: &str,
+    ) -> Result<(), super::Error> {
+        if self.controller.toggle_reaction_local(item_id, key).await? {
             // TODO(bnjbvr): hacky?
-            if let Some(event_id) = self
-                .controller
-                .items()
-                .await
-                .iter()
-                .rfind(|item| item.unique_id() == unique_id)
-                .and_then(|item| item.as_event()?.as_remote())
-                .map(|event_item| event_item.event_id.clone())
+            let items = self.controller.items().await;
+            if let Some(event_id) = rfind_event_by_item_id(&items, item_id)
+                .and_then(|(_pos, item)| item.event_id().map(ToOwned::to_owned))
             {
                 // Fake a local echo, for new reactions.
                 self.handle_local_event(

crates/matrix-sdk-ui/src/timeline/tests/reactions.rs

@@ -83,7 +83,8 @@ async fn test_add_reaction_on_non_existent_event() {
     let timeline = TestTimeline::new();
     let mut stream = timeline.subscribe().await;
 
-    timeline.toggle_reaction_local("nonexisting_unique_id", REACTION_KEY).await.unwrap_err();
+    let event_id = EventId::parse("$nonexisting_unique_id").unwrap();
+    timeline.toggle_reaction_local(&event_id.into(), REACTION_KEY).await.unwrap_err();
 
     assert!(stream.next().now_or_never().is_none());
 }
@@ -92,10 +93,10 @@ async fn test_add_reaction_on_non_existent_event() {
 async fn test_add_reaction_success() {
     let timeline = TestTimeline::new();
     let mut stream = timeline.subscribe().await;
-    let (msg_uid, event_id, item_pos) = send_first_message(&timeline, &mut stream).await;
+    let (event_id, item_pos) = send_first_message(&timeline, &mut stream).await;
 
     // If I toggle a reaction on an event which didn't have any…
-    timeline.toggle_reaction_local(&msg_uid, REACTION_KEY).await.unwrap();
+    timeline.toggle_reaction_local(&event_id.clone().into(), REACTION_KEY).await.unwrap();
 
     // The timeline item is updated, with a local echo for the reaction.
     assert_reaction_is_updated!(stream, &event_id, item_pos, false);
@@ -123,7 +124,7 @@ async fn test_redact_reaction_success() {
     let f = &timeline.factory;
 
     let mut stream = timeline.subscribe().await;
-    let (msg_uid, event_id, item_pos) = send_first_message(&timeline, &mut stream).await;
+    let (event_id, item_pos) = send_first_message(&timeline, &mut stream).await;
 
     // A reaction is added by sync.
     let reaction_id = event_id!("$reaction_id");
@@ -135,7 +136,7 @@ async fn test_redact_reaction_success() {
     assert_reaction_is_updated!(stream, &event_id, item_pos, true);
 
     // Toggling the reaction locally…
-    timeline.toggle_reaction_local(&msg_uid, REACTION_KEY).await.unwrap();
+    timeline.toggle_reaction_local(&event_id.clone().into(), REACTION_KEY).await.unwrap();
 
     // Will immediately redact it on the item.
     let event = assert_item_update!(stream, &event_id, item_pos);
@@ -166,12 +167,12 @@ async fn test_redact_reaction_success() {
 async fn test_reactions_store_timestamp() {
     let timeline = TestTimeline::new();
     let mut stream = timeline.subscribe().await;
-    let (msg_uid, event_id, msg_pos) = send_first_message(&timeline, &mut stream).await;
+    let (event_id, msg_pos) = send_first_message(&timeline, &mut stream).await;
 
     // Creating a reaction adds a valid timestamp.
     let timestamp_before = MilliSecondsSinceUnixEpoch::now();
 
-    timeline.toggle_reaction_local(&msg_uid, REACTION_KEY).await.unwrap();
+    timeline.toggle_reaction_local(&event_id.clone().into(), REACTION_KEY).await.unwrap();
 
     let event = assert_reaction_is_updated!(stream, &event_id, msg_pos, false);
     let reactions = event.reactions().get(&REACTION_KEY.to_owned()).unwrap();
@@ -216,7 +217,7 @@ async fn test_initial_reaction_timestamp_is_stored() {
 async fn send_first_message(
     timeline: &TestTimeline,
     stream: &mut (impl Stream<Item = VectorDiff<Arc<TimelineItem>>> + Unpin),
-) -> (String, OwnedEventId, usize) {
+) -> (OwnedEventId, usize) {
     timeline.handle_live_event(timeline.factory.text_msg("I want you to react").sender(&BOB)).await;
 
     let item = assert_next_matches!(*stream, VectorDiff::PushBack { value } => value);
@@ -226,5 +227,5 @@ async fn send_first_message(
     let day_divider = assert_next_matches!(*stream, VectorDiff::PushFront { value } => value);
     assert!(day_divider.is_day_divider());
 
-    (item.unique_id().to_owned(), event_id, position)
+    (event_id, position)
 }

crates/matrix-sdk-ui/src/timeline/util.rs

@@ -21,7 +21,8 @@ use ruma::{EventId, MilliSecondsSinceUnixEpoch};
 #[cfg(doc)]
 use super::controller::TimelineMetadata;
 use super::{
-    event_item::EventTimelineItemKind, EventTimelineItem, ReactionsByKeyBySender, TimelineItem,
+    event_item::EventTimelineItemKind, EventTimelineItem, ReactionsByKeyBySender,
+    TimelineEventItemId, TimelineItem,
 };
 
 pub(super) struct EventTimelineItemWithId<'a> {
@@ -80,26 +81,37 @@ pub(super) fn rfind_event_item(
     rfind_event_item_internal(items, |item_with_id| f(item_with_id.inner))
 }
 
-/// Find the timeline item that matches the given internal id, if any.
+/// Find the timeline item that matches the given event id, if any.
 ///
 /// WARNING: Linear scan of the items, see documentation of
 /// [`rfind_event_item`].
-pub(super) fn rfind_event_by_uid<'a>(
+pub(super) fn rfind_event_by_id<'a>(
     items: &'a Vector<Arc<TimelineItem>>,
-    internal_id: &'a str,
+    event_id: &EventId,
 ) -> Option<(usize, EventTimelineItemWithId<'a>)> {
-    rfind_event_item_internal(items, |item_with_id| item_with_id.internal_id == internal_id)
+    rfind_event_item(items, |it| it.event_id() == Some(event_id))
 }
 
-/// Find the timeline item that matches the given event id, if any.
+/// Find the timeline item that matches the given item (event or transaction)
+/// id, if any.
 ///
 /// WARNING: Linear scan of the items, see documentation of
 /// [`rfind_event_item`].
-pub(super) fn rfind_event_by_id<'a>(
+pub(super) fn rfind_event_by_item_id<'a>(
     items: &'a Vector<Arc<TimelineItem>>,
-    event_id: &EventId,
+    item_id: &TimelineEventItemId,
 ) -> Option<(usize, EventTimelineItemWithId<'a>)> {
-    rfind_event_item(items, |it| it.event_id() == Some(event_id))
+    match item_id {
+        TimelineEventItemId::TransactionId(txn_id) => {
+            rfind_event_item(items, |item| match &item.kind {
+                EventTimelineItemKind::Local(local) => local.transaction_id == *txn_id,
+                EventTimelineItemKind::Remote(remote) => {
+                    remote.transaction_id.as_deref() == Some(txn_id)
+                }
+            })
+        }
+        TimelineEventItemId::EventId(event_id) => rfind_event_by_id(items, event_id),
+    }
 }
 
 /// Result of comparing events position in the timeline.

crates/matrix-sdk-ui/tests/integration/timeline/focus_event.rs

@@ -320,7 +320,7 @@ async fn test_focused_timeline_local_echoes() {
     assert_pending!(timeline_stream);
 
     // Add a reaction to the focused event, which will cause a local echo to happen.
-    timeline.toggle_reaction(items[1].unique_id(), "✨").await.unwrap();
+    timeline.toggle_reaction(&target_event.to_owned().into(), "✨").await.unwrap();
 
     // We immediately get the local echo for the reaction.
     let item = assert_next_matches_with_timeout!(timeline_stream, VectorDiff::Set { index: 1, value: item } => item);

crates/matrix-sdk-ui/tests/integration/timeline/reactions.rs

@@ -75,7 +75,7 @@ async fn test_abort_before_being_sent() {
 
     assert_let!(Some(VectorDiff::PushBack { value: first }) = stream.next().await);
     let item = first.as_event().unwrap();
-    let unique_id = first.unique_id();
+    let item_id = item.event_id().unwrap().to_owned().into();
     assert_eq!(item.content().as_message().unwrap().body(), "hello");
 
     assert_let!(Some(VectorDiff::PushFront { value: day_divider }) = stream.next().await);
@@ -102,7 +102,7 @@ async fn test_abort_before_being_sent() {
     mock_redaction(event_id!("$3")).mount(&server).await;
 
     // We add the reaction…
-    timeline.toggle_reaction(unique_id, "👍").await.unwrap();
+    timeline.toggle_reaction(&item_id, "👍").await.unwrap();
 
     // First toggle (local echo).
     {
@@ -119,7 +119,7 @@ async fn test_abort_before_being_sent() {
     }
 
     // We toggle another reaction at the same time…
-    timeline.toggle_reaction(unique_id, "🥰").await.unwrap();
+    timeline.toggle_reaction(&item_id, "🥰").await.unwrap();
 
     {
         assert_let!(Some(VectorDiff::Set { index: 1, value: item }) = stream.next().await);
@@ -140,7 +140,7 @@ async fn test_abort_before_being_sent() {
 
     // Then we remove the first one; because it was being sent, it should lead to a
     // redaction event.
-    timeline.toggle_reaction(unique_id, "👍").await.unwrap();
+    timeline.toggle_reaction(&item_id, "👍").await.unwrap();
 
     {
         assert_let!(Some(VectorDiff::Set { index: 1, value: item }) = stream.next().await);
@@ -157,7 +157,7 @@ async fn test_abort_before_being_sent() {
 
     // But because the first one was being sent, this one won't and the local echo
     // could be discarded.
-    timeline.toggle_reaction(unique_id, "🥰").await.unwrap();
+    timeline.toggle_reaction(&item_id, "🥰").await.unwrap();
 
     {
         assert_let!(Some(VectorDiff::Set { index: 1, value: item }) = stream.next().await);
@@ -214,12 +214,11 @@ async fn test_redact_failed() {
     let _response = client.sync_once(Default::default()).await.unwrap();
     server.reset().await;
 
-    let unique_id = assert_next_matches_with_timeout!(stream, VectorDiff::PushBack { value: item } => {
-        let unique_id = item.unique_id().to_owned();
+    let event_id = assert_next_matches_with_timeout!(stream, VectorDiff::PushBack { value: item } => {
         let item = item.as_event().unwrap();
         assert_eq!(item.content().as_message().unwrap().body(), "hello");
         assert!(item.reactions().is_empty());
-        unique_id
+        item.event_id().unwrap().to_owned()
     });
 
     assert_next_matches_with_timeout!(stream, VectorDiff::Set { index: 0, value: item } => {
@@ -242,7 +241,7 @@ async fn test_redact_failed() {
         .await;
 
     // We toggle the reaction, which fails with an error.
-    timeline.toggle_reaction(&unique_id, "😆").await.unwrap_err();
+    timeline.toggle_reaction(&event_id.into(), "😆").await.unwrap_err();
 
     // The local echo is removed (assuming the redaction works)…
     assert_next_matches_with_timeout!(stream, VectorDiff::Set { index: 1, value: item } => {
@@ -310,13 +309,12 @@ async fn test_local_reaction_to_local_echo() {
     let _ = timeline.send(RoomMessageEventContent::text_plain("lol").into()).await.unwrap();
 
     // Receive a local echo.
-    let unique_id = assert_next_matches_with_timeout!(stream, VectorDiff::PushBack { value: item } => {
-        let unique_id = item.unique_id().to_owned();
+    let item_id = assert_next_matches_with_timeout!(stream, VectorDiff::PushBack { value: item } => {
         let item = item.as_event().unwrap();
         assert!(item.is_local_echo());
         assert_eq!(item.content().as_message().unwrap().body(), "lol");
         assert!(item.reactions().is_empty());
-        unique_id
+        item.transaction_id().unwrap().to_owned().into()
     });
 
     // Good ol' day divider.
@@ -326,7 +324,7 @@ async fn test_local_reaction_to_local_echo() {
 
     // Add a reaction before the remote echo comes back.
     let key1 = "🤣";
-    timeline.toggle_reaction(&unique_id, key1).await.unwrap();
+    timeline.toggle_reaction(&item_id, key1).await.unwrap();
 
     // The reaction is added to the local echo.
     assert_next_matches_with_timeout!(stream, VectorDiff::Set { index: 1, value: item } => {
@@ -338,7 +336,7 @@ async fn test_local_reaction_to_local_echo() {
 
     // Add another reaction.
     let key2 = "😈";
-    timeline.toggle_reaction(&unique_id, key2).await.unwrap();
+    timeline.toggle_reaction(&item_id, key2).await.unwrap();
 
     // Also comes as a local echo.
     assert_next_matches_with_timeout!(stream, VectorDiff::Set { index: 1, value: item } => {
@@ -350,7 +348,7 @@ async fn test_local_reaction_to_local_echo() {
 
     // Remove second reaction. It's immediately removed, since it was a local echo,
     // and it wasn't being sent.
-    timeline.toggle_reaction(&unique_id, key2).await.unwrap();
+    timeline.toggle_reaction(&item_id, key2).await.unwrap();
 
     assert_next_matches_with_timeout!(stream, VectorDiff::Set { index: 1, value: item } => {
         let reactions = item.as_event().unwrap().reactions();