timeline: refactor reaction handling

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

@@ -269,7 +269,6 @@ impl EncryptedMessage {
 #[derive(Clone, uniffi::Record)]
 pub struct Reaction {
     pub key: String,
-    pub count: u64,
     pub senders: Vec<ReactionSenderData>,
 }
 

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

@@ -978,12 +978,11 @@ impl EventTimelineItem {
             .iter()
             .map(|(k, v)| Reaction {
                 key: k.to_owned(),
-                count: v.len().try_into().unwrap(),
                 senders: v
-                    .senders()
-                    .map(|v| ReactionSenderData {
-                        sender_id: v.sender_id.to_string(),
-                        timestamp: v.timestamp.0.into(),
+                    .iter()
+                    .map(|(sender_id, info)| ReactionSenderData {
+                        sender_id: sender_id.to_string(),
+                        timestamp: info.timestamp.0.into(),
                     })
                     .collect(),
             })

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

@@ -33,7 +33,6 @@ use tracing::warn;
 
 mod content;
 mod local;
-mod reactions;
 mod remote;
 
 pub use self::{
@@ -43,7 +42,6 @@ pub use self::{
         TimelineItemContent,
     },
     local::EventSendState,
-    reactions::{BundledReactions, ReactionGroup},
 };
 pub(super) use self::{
     local::LocalEventTimelineItem,
@@ -269,9 +267,9 @@ impl EventTimelineItem {
     }
 
     /// Get the reactions of this item.
-    pub fn reactions(&self) -> &BundledReactions {
+    pub fn reactions(&self) -> &ReactionsByKeyBySender {
         // There's not much of a point in allowing reactions to local echoes.
-        static EMPTY_REACTIONS: Lazy<BundledReactions> = Lazy::new(Default::default);
+        static EMPTY_REACTIONS: Lazy<ReactionsByKeyBySender> = Lazy::new(Default::default);
         match &self.kind {
             EventTimelineItemKind::Local(_) => &EMPTY_REACTIONS,
             EventTimelineItemKind::Remote(remote_event) => &remote_event.reactions,
@@ -580,6 +578,20 @@ pub enum EventItemOrigin {
     Pagination,
 }
 
+/// Information about a single reaction stored in [`ReactionsByKeyBySender`].
+#[derive(Clone, Debug)]
+pub struct ReactionInfo {
+    pub timestamp: MilliSecondsSinceUnixEpoch,
+    /// Id of the reaction (not the reacted-to event).
+    pub id: TimelineEventItemId,
+}
+
+/// Reactions grouped by key first, then by sender.
+///
+/// This representation makes sure that a given sender has sent at most one
+/// reaction for an event.
+pub type ReactionsByKeyBySender = IndexMap<String, IndexMap<OwnedUserId, ReactionInfo>>;
+
 #[cfg(test)]
 mod tests {
     use assert_matches::assert_matches;

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

@@ -22,7 +22,7 @@ use ruma::{
     OwnedEventId, OwnedTransactionId, OwnedUserId,
 };
 
-use super::BundledReactions;
+use super::ReactionsByKeyBySender;
 
 /// An item for an event that was received from the homeserver.
 #[derive(Clone)]
@@ -34,7 +34,7 @@ pub(in crate::timeline) struct RemoteEventTimelineItem {
     pub transaction_id: Option<OwnedTransactionId>,
 
     /// All bundled reactions about the event.
-    pub reactions: BundledReactions,
+    pub reactions: ReactionsByKeyBySender,
 
     /// All read receipts for the event.
     ///
@@ -74,15 +74,15 @@ pub(in crate::timeline) struct RemoteEventTimelineItem {
 
 impl RemoteEventTimelineItem {
     /// Clone the current event item, and update its `reactions`.
-    pub fn with_reactions(&self, reactions: BundledReactions) -> Self {
+    pub fn with_reactions(&self, reactions: ReactionsByKeyBySender) -> Self {
         Self { reactions, ..self.clone() }
     }
 
     /// Clone the current event item, and clear its `reactions` as well as the
     /// JSON representation fields.
     pub fn redact(&self) -> Self {
         Self {
-            reactions: BundledReactions::default(),
+            reactions: ReactionsByKeyBySender::default(),
             original_json: None,
             latest_edit_json: None,
             ..self.clone()

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

@@ -21,7 +21,6 @@ use eyeball_im::{ObservableVectorEntry, VectorDiff};
 use eyeball_im_util::vector::VectorObserverExt;
 use futures_core::Stream;
 use imbl::Vector;
-use itertools::Itertools;
 #[cfg(all(test, feature = "e2e-encryption"))]
 use matrix_sdk::crypto::OlmMachine;
 use matrix_sdk::{
@@ -58,11 +57,11 @@ use super::traits::Decryptor;
 use super::{
     event_handler::TimelineEventKind,
     event_item::RemoteEventOrigin,
-    reactions::ReactionToggleResult,
+    reactions::{AnnotationKey, ReactionAction, ReactionState, ReactionToggleResult},
     traits::RoomDataProvider,
     util::{rfind_event_by_id, rfind_event_item, RelativePosition},
-    AnnotationKey, Error, EventSendState, EventTimelineItem, InReplyToDetails, Message,
-    PaginationError, Profile, RepliedToEvent, TimelineDetails, TimelineFocus, TimelineItem,
+    Error, EventSendState, EventTimelineItem, InReplyToDetails, Message, PaginationError, Profile,
+    RepliedToEvent, TimelineDetails, TimelineEventItemId, TimelineFocus, TimelineItem,
     TimelineItemContent, TimelineItemKind,
 };
 use crate::{
@@ -112,30 +111,6 @@ pub(super) struct TimelineInner<P: RoomDataProvider = Room> {
     settings: TimelineInnerSettings,
 }
 
-#[derive(Debug, Clone)]
-pub(super) enum ReactionAction {
-    /// Request already in progress so allow that one to resolve
-    None,
-
-    /// Send this reaction to the server
-    SendRemote(OwnedTransactionId),
-
-    /// Redact this reaction from the server
-    RedactRemote(OwnedEventId),
-}
-
-#[derive(Debug, Clone)]
-pub(super) enum ReactionState {
-    /// We're redacting a reaction.
-    ///
-    /// The optional event id is defined if, and only if, there already was a
-    /// remote echo for this reaction.
-    Redacting(Option<OwnedEventId>),
-    /// We're sending the reaction with the given transaction id, which we'll
-    /// use to match against the response in the sync event.
-    Sending(OwnedTransactionId),
-}
-
 #[derive(Clone)]
 pub(super) struct TimelineInnerSettings {
     /// Should the read receipts and read markers be handled?
@@ -433,17 +408,13 @@ impl<P: RoomDataProvider> TimelineInner<P> {
         };
 
         let (local_echo_txn_id, remote_echo_event_id) = {
-            let reactions = related_event.reactions();
-
-            let user_reactions =
-                reactions.get(&annotation.key).map(|group| group.by_sender(user_id));
-
-            user_reactions
-                .map(|reactions| {
-                    let reactions = reactions.collect_vec();
-                    let local = reactions.iter().find_map(|(txid, _event_id)| *txid);
-                    let remote = reactions.iter().find_map(|(_txid, event_id)| *event_id);
-                    (local, remote)
+            related_event
+                .reactions()
+                .get(&annotation.key)
+                .and_then(|group| group.get(user_id))
+                .map(|reaction_info| match &reaction_info.id {
+                    TimelineEventItemId::TransactionId(txn_id) => (Some(txn_id), None),
+                    TimelineEventItemId::EventId(event_id) => (None, Some(event_id)),
                 })
                 .unwrap_or((None, None))
         };
@@ -453,8 +424,11 @@ impl<P: RoomDataProvider> TimelineInner<P> {
         let reaction_state = match (local_echo_txn_id, remote_echo_event_id) {
             (None, None) => {
                 // No previous record of the reaction, create a local echo.
-                let in_flight =
-                    state.meta.in_flight_reaction.get::<AnnotationKey>(&annotation.into());
+                let in_flight = state
+                    .meta
+                    .reactions
+                    .in_flight_reaction
+                    .get::<AnnotationKey>(&annotation.into());
                 let txn_id = match in_flight {
                     Some(ReactionState::Sending(txn_id)) => {
                         // Use the transaction ID as the in flight request
@@ -501,10 +475,11 @@ impl<P: RoomDataProvider> TimelineInner<P> {
             }
         };
 
-        state.meta.reaction_state.insert(annotation.into(), reaction_state.clone());
+        state.meta.reactions.reaction_state.insert(annotation.into(), reaction_state.clone());
 
         // Check the action to perform depending on any in flight request
-        let in_flight = state.meta.in_flight_reaction.get::<AnnotationKey>(&annotation.into());
+        let in_flight =
+            state.meta.reactions.in_flight_reaction.get::<AnnotationKey>(&annotation.into());
         let result = match in_flight {
             Some(_) => {
                 // There is an in-flight request
@@ -529,7 +504,7 @@ impl<P: RoomDataProvider> TimelineInner<P> {
             ReactionAction::None => {}
             ReactionAction::SendRemote(_) | ReactionAction::RedactRemote(_) => {
                 // Remember the new in flight request
-                state.meta.in_flight_reaction.insert(annotation.into(), reaction_state);
+                state.meta.reactions.in_flight_reaction.insert(annotation.into(), reaction_state);
             }
         };
 
@@ -734,6 +709,7 @@ impl<P: RoomDataProvider> TimelineInner<P> {
 
         let reaction_state = state
             .meta
+            .reactions
             .reaction_state
             .get(&AnnotationKey::from(annotation))
             .expect("Reaction state should be set before sending the reaction");
@@ -743,6 +719,7 @@ impl<P: RoomDataProvider> TimelineInner<P> {
                 // A reaction was added successfully but we've been requested to undo it
                 state
                     .meta
+                    .reactions
                     .in_flight_reaction
                     .insert(annotation_key, ReactionState::Redacting(Some(event_id.to_owned())));
                 ReactionAction::RedactRemote(event_id.to_owned())
@@ -752,14 +729,15 @@ impl<P: RoomDataProvider> TimelineInner<P> {
                 let txn_id = txn_id.to_owned();
                 state
                     .meta
+                    .reactions
                     .in_flight_reaction
                     .insert(annotation_key, ReactionState::Sending(txn_id.clone()));
                 ReactionAction::SendRemote(txn_id)
             }
             _ => {
                 // We're done, so also update the timeline
-                state.meta.in_flight_reaction.swap_remove(&annotation_key);
-                state.meta.reaction_state.swap_remove(&annotation_key);
+                state.meta.reactions.in_flight_reaction.swap_remove(&annotation_key);
+                state.meta.reactions.reaction_state.swap_remove(&annotation_key);
                 state.update_timeline_reaction(user_id, annotation, result)?;
 
                 ReactionAction::None