refactor(timeline): get rid of two uses of meta.reactions.map

Author: bnjbvr

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

@@ -16,6 +16,7 @@ use std::collections::HashMap;
 
 use matrix_sdk::send_queue::SendHandle;
 use ruma::{MilliSecondsSinceUnixEpoch, OwnedEventId, OwnedUserId};
+use tracing::warn;
 
 use crate::timeline::{
     PollState, ReactionInfo, ReactionStatus, TimelineEventItemId, TimelineItemContent,
@@ -45,7 +46,7 @@ pub(crate) enum AggregationKind {
 
 #[derive(Clone, Debug)]
 pub(crate) struct Aggregation {
-    kind: AggregationKind,
+    pub kind: AggregationKind,
     pub own_id: TimelineEventItemId,
 }
 
@@ -131,22 +132,52 @@ impl Aggregation {
 pub(crate) struct Aggregations {
     /// Mapping of a target event to its list of aggregations.
     related_events: HashMap<TimelineEventItemId, Vec<Aggregation>>,
+
+    /// Mapping of a related event identifier to its target.
+    inverted_map: HashMap<TimelineEventItemId, TimelineEventItemId>,
 }
 
 impl Aggregations {
     pub fn clear(&mut self) {
         self.related_events.clear();
+        self.inverted_map.clear();
     }
 
     pub fn add(&mut self, related_to: OwnedEventId, aggregation: Aggregation) {
+        self.inverted_map
+            .insert(aggregation.own_id.clone(), TimelineEventItemId::EventId(related_to.clone()));
         self.related_events
             .entry(TimelineEventItemId::EventId(related_to))
             .or_default()
             .push(aggregation);
     }
 
-    pub fn get_mut(&mut self, related_to: &TimelineEventItemId) -> Option<&mut Vec<Aggregation>> {
-        self.related_events.get_mut(related_to)
+    /// Is the given id one for a known aggregation to another event?
+    ///
+    /// If so, returns the target event identifier as well as the aggregation.
+    pub fn try_remove_aggregation(
+        &mut self,
+        aggregation_id: &TimelineEventItemId,
+    ) -> Option<(&TimelineEventItemId, Aggregation)> {
+        if let Some(found) = self.inverted_map.get(aggregation_id) {
+            // Find and remove the aggregation in the other mapping.
+            let aggregation = self.related_events.get_mut(found).and_then(|aggregations| {
+                if let Some(idx) = aggregations.iter().position(|agg| agg.own_id == *aggregation_id)
+                {
+                    Some(aggregations.remove(idx))
+                } else {
+                    None
+                }
+            });
+
+            if aggregation.is_none() {
+                warn!("unexpected missing aggregation {aggregation_id:?}: was present in the inverted map, not in the actual map");
+            }
+
+            Some((found, aggregation?))
+        } else {
+            None
+        }
     }
 
     pub fn apply(

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

@@ -51,7 +51,7 @@ use ruma::{
 use tracing::{debug, error, field::debug, info, instrument, trace, warn};
 
 use super::{
-    algorithms::rfind_event_by_id,
+    algorithms::{rfind_event_by_id, rfind_event_by_item_id},
     controller::{
         Aggregation, AggregationKind, ObservableItemsTransaction, ObservableItemsTransactionEntry,
         PendingEdit, PendingEditKind, TimelineMetadata, TimelineStateTransaction,
@@ -1007,41 +1007,31 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
     fn handle_reaction_redaction(&mut self, reaction_id: OwnedEventId) -> bool {
         let reaction_id = TimelineEventItemId::EventId(reaction_id);
 
-        if let Some(FullReactionKey {
-            item: TimelineEventItemId::EventId(reacted_to_event_id),
-            key,
-            sender,
-        }) = self.meta.reactions.map.remove(&reaction_id)
+        if let Some((target, aggregation)) =
+            self.meta.aggregations.try_remove_aggregation(&reaction_id)
         {
-            let Some((item_pos, item)) = rfind_event_by_id(self.items, &reacted_to_event_id) else {
-                // The remote event wasn't in the timeline.
-
-                // Remove any possibly pending reactions to that event, as this redaction would
-                // affect them.
-                if let Some(aggregations) = self
-                    .meta
-                    .aggregations
-                    .get_mut(&TimelineEventItemId::EventId(reacted_to_event_id))
-                {
-                    if let Some(found) = aggregations
-                        .iter()
-                        .enumerate()
-                        .find_map(|(idx, agg)| (agg.own_id == reaction_id).then_some(idx))
-                    {
-                        aggregations.remove(found);
+            match &aggregation.kind {
+                AggregationKind::Reaction { sender, key, .. } => {
+                    let Some((item_pos, item)) = rfind_event_by_item_id(&self.items, &target)
+                    else {
+                        warn!("missing reacted-to item {target:?}");
+                        return false;
+                    };
+                    let mut reactions = item.content().reactions().clone();
+                    if reactions.remove_reaction(&sender, &key).is_some() {
+                        trace!("Removing reaction");
+                        self.items.replace(item_pos, item.with_reactions(reactions));
+                        self.result.items_updated += 1;
+                        return true;
                     }
                 }
 
-                // We haven't redacted the reaction.
-                return false;
-            };
-
-            let mut reactions = item.content().reactions().clone();
-            if reactions.remove_reaction(&sender, &key).is_some() {
-                trace!("Removing reaction");
-                self.items.replace(item_pos, item.with_reactions(reactions));
-                self.result.items_updated += 1;
-                return true;
+                _ => {
+                    warn!(
+                        "unexpected aggregation kind in `handle_reaction_redaction`: {:?}",
+                        aggregation.kind
+                    );
+                }
             }
         }