timeline: use a TimelineEventItemId for reacting to something

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

@@ -495,7 +495,7 @@ impl Timeline {
         new_content: EditedContent,
     ) -> Result<bool, ClientError> {
         self.inner
-            .edit_by_id(&(event_or_transaction_id.try_into()?), new_content.try_into()?)
+            .edit_by_id(&event_or_transaction_id.try_into()?, new_content.try_into()?)
             .await
             .map_err(Into::into)
     }
@@ -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
@@ -735,7 +735,7 @@ impl<P: RoomDataProvider> TimelineController<P> {
     ///
     /// If the corresponding local timeline item is missing, a warning is
     /// raised.
-    #[instrument(skip_all, fields(txn_id))]
+    #[instrument(skip(self))]
     pub(super) async fn update_event_send_state(
         &self,
         txn_id: &TransactionId,

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

@@ -103,22 +103,6 @@ pub enum TimelineEventItemId {
     EventId(OwnedEventId),
 }
 
-impl From<String> for TimelineEventItemId {
-    fn from(value: String) -> Self {
-        value.as_str().into()
-    }
-}
-
-impl From<&str> for TimelineEventItemId {
-    fn from(value: &str) -> Self {
-        if let Ok(event_id) = EventId::parse(value) {
-            TimelineEventItemId::EventId(event_id)
-        } else {
-            TimelineEventItemId::TransactionId(value.into())
-        }
-    }
-}
-
 /// An handle that usually allows to perform an action on a timeline event.
 ///
 /// If the item represents a remote item, then the event id is usually
@@ -749,7 +733,7 @@ mod tests {
     };
 
     use super::{EventTimelineItem, Profile};
-    use crate::timeline::{TimelineDetails, TimelineEventItemId};
+    use crate::timeline::TimelineDetails;
 
     #[async_test]
     async fn test_latest_message_event_can_be_wrapped_as_a_timeline_item() {
@@ -974,20 +958,6 @@ mod tests {
         );
     }
 
-    #[test]
-    fn test_raw_event_id_into_timeline_event_item_id_gets_event_id() {
-        let raw_id = "$123:example.com";
-        let id: TimelineEventItemId = raw_id.into();
-        assert_matches!(id, TimelineEventItemId::EventId(_));
-    }
-
-    #[test]
-    fn test_raw_str_into_timeline_event_item_id_gets_transaction_id() {
-        let raw_id = "something something";
-        let id: TimelineEventItemId = raw_id.into();
-        assert_matches!(id, TimelineEventItemId::TransactionId(_));
-    }
-
     fn member_event(
         room_id: &RoomId,
         user_id: &UserId,

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

@@ -29,7 +29,7 @@ use tokio::time::timeout;
 
 use crate::timeline::{
     controller::TimelineEnd, event_item::RemoteEventOrigin, tests::TestTimeline, ReactionStatus,
-    TimelineItem,
+    TimelineEventItemId, TimelineItem,
 };
 
 const REACTION_KEY: &str = "👍";
@@ -83,7 +83,11 @@ 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(&TimelineEventItemId::EventId(event_id), REACTION_KEY)
+        .await
+        .unwrap_err();
 
     assert!(stream.next().now_or_never().is_none());
 }
@@ -92,10 +96,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 (item_id, 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(&item_id, 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 +127,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 (item_id, 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 +139,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(&item_id, REACTION_KEY).await.unwrap();
 
     // Will immediately redact it on the item.
     let event = assert_item_update!(stream, &event_id, item_pos);
@@ -166,12 +170,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 (item_id, 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(&item_id, 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,15 +220,17 @@ 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) {
+) -> (TimelineEventItemId, 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);
-    let event_id = item.as_event().unwrap().as_remote().unwrap().event_id.clone();
+    let event_item = item.as_event().unwrap();
+    let item_id = event_item.identifier();
+    let event_id = event_item.event_id().unwrap().to_owned();
     let position = timeline.len().await - 1;
 
     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)
+    (item_id, 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(&event_item.identifier(), "✨").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);