From 4e95212283392d4aac3efe7b7c735fb3f49f526e Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 24 Jul 2024 20:55:00 +0200 Subject: [PATCH 01/11] timeline: move the removal of reactions to `pending_reactions` --- .../matrix-sdk-ui/src/timeline/event_handler.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index bf3b17e4481..60388e4f38a 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -874,7 +874,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { let sender = self.ctx.sender.to_owned(); let sender_profile = TimelineDetails::from_initial_value(self.ctx.sender_profile.clone()); let timestamp = self.ctx.timestamp; - let mut reactions = self.pending_reactions().unwrap_or_default(); + let reactions = self.pending_reactions(&content).unwrap_or_default(); let kind: EventTimelineItemKind = match &self.ctx.flow { Flow::Local { txn_id, send_handle } => LocalEventTimelineItem { @@ -885,13 +885,6 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { .into(), Flow::Remote { event_id, raw_event, position, txn_id, encryption_info, .. } => { - // Drop pending reactions if the message is redacted. - if let TimelineItemContent::RedactedMessage = content { - if !reactions.is_empty() { - reactions = BundledReactions::default(); - } - } - let origin = match *position { TimelineItemPosition::Start { origin } | TimelineItemPosition::End { origin } => origin, @@ -1050,7 +1043,12 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { } } - fn pending_reactions(&mut self) -> Option { + fn pending_reactions(&mut self, content: &TimelineItemContent) -> Option { + // Drop pending reactions if the message is redacted. + if let TimelineItemContent::RedactedMessage = content { + return None; + } + match &self.ctx.flow { Flow::Local { .. } => None, Flow::Remote { event_id, .. } => { From 5c1eb2eb08c3cac748b83206cc759d9e98183d98 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 24 Jul 2024 20:58:21 +0200 Subject: [PATCH 02/11] timeline: move the clearing of `TimelineInnerMetadata` to its own function --- .../matrix-sdk-ui/src/timeline/inner/state.rs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/inner/state.rs b/crates/matrix-sdk-ui/src/timeline/inner/state.rs index d899a0099b7..5d6f04c97f3 100644 --- a/crates/matrix-sdk-ui/src/timeline/inner/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/inner/state.rs @@ -610,13 +610,7 @@ impl TimelineInnerStateTransaction<'_> { self.items.clear(); } - self.meta.all_events.clear(); - self.meta.read_receipts.clear(); - self.meta.reactions.clear(); - self.meta.fully_read_event = None; - // We forgot about the fully read marker right above, so wait for a new one - // before attempting to update it for each new timeline item. - self.meta.has_up_to_date_read_marker_item = true; + self.meta.clear(); debug!(remaining_items = self.items.len(), "Timeline cleared"); } @@ -786,6 +780,16 @@ impl TimelineInnerMetadata { } } + pub(crate) fn clear(&mut self) { + self.all_events.clear(); + self.read_receipts.clear(); + self.reactions.clear(); + self.fully_read_event = None; + // We forgot about the fully read marker right above, so wait for a new one + // before attempting to update it for each new timeline item. + self.has_up_to_date_read_marker_item = true; + } + /// Get the relative positions of two events in the timeline. /// /// This method assumes that all events since the end of the timeline are From 90c78dcd1233347dac66fab7bb59e7cb535dd25c Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 24 Jul 2024 21:01:15 +0200 Subject: [PATCH 03/11] timeline: clear more things in `TimelineInnerMetadata` when clearing the timeline --- .../matrix-sdk-ui/src/timeline/inner/state.rs | 38 ++++++++++++------- crates/matrix-sdk-ui/src/timeline/polls.rs | 5 +++ 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/inner/state.rs b/crates/matrix-sdk-ui/src/timeline/inner/state.rs index 5d6f04c97f3..116a0bbb896 100644 --- a/crates/matrix-sdk-ui/src/timeline/inner/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/inner/state.rs @@ -718,17 +718,31 @@ impl TimelineInnerStateTransaction<'_> { #[derive(Clone, Debug)] pub(in crate::timeline) struct TimelineInnerMetadata { - /// List of all the events as received in the timeline, even the ones that - /// are discarded in the timeline items. - pub all_events: VecDeque, + // **** CONSTANT FIELDS **** + /// An optional prefix for internal IDs, defined during construction of the + /// timeline. + /// + /// This value is constant over the lifetime of the metadata. + internal_id_prefix: Option, + + /// The hook to call whenever we run into a unable-to-decrypt event. + /// + /// This value is constant over the lifetime of the metadata. + pub(crate) unable_to_decrypt_hook: Option>, + + /// Matrix room version of the timeline's room, or a sensible default. + /// + /// This value is constant over the lifetime of the metadata. + pub room_version: RoomVersionId, + // **** DYNAMIC FIELDS **** /// The next internal identifier for timeline items, used for both local and /// remote echoes. next_internal_id: u64, - /// An optional prefix for internal IDs, defined during construction of the - /// timeline. - internal_id_prefix: Option, + /// List of all the events as received in the timeline, even the ones that + /// are discarded in the timeline items. + pub all_events: VecDeque, pub reactions: Reactions, pub poll_pending_events: PollPendingEvents, @@ -748,12 +762,6 @@ pub(in crate::timeline) struct TimelineInnerMetadata { pub reaction_state: IndexMap, /// The in-flight reaction request state that is ongoing. pub in_flight_reaction: IndexMap, - - /// The hook to call whenever we run into a unable-to-decrypt event. - pub(crate) unable_to_decrypt_hook: Option>, - - /// Matrix room version of the timeline's room, or a sensible default. - pub room_version: RoomVersionId, } impl TimelineInnerMetadata { @@ -781,13 +789,17 @@ impl TimelineInnerMetadata { } pub(crate) fn clear(&mut self) { + self.next_internal_id = 0; self.all_events.clear(); - self.read_receipts.clear(); self.reactions.clear(); + self.poll_pending_events.clear(); self.fully_read_event = None; // We forgot about the fully read marker right above, so wait for a new one // before attempting to update it for each new timeline item. self.has_up_to_date_read_marker_item = true; + self.read_receipts.clear(); + self.reaction_state.clear(); + self.in_flight_reaction.clear(); } /// Get the relative positions of two events in the timeline. diff --git a/crates/matrix-sdk-ui/src/timeline/polls.rs b/crates/matrix-sdk-ui/src/timeline/polls.rs index 23647c6b819..df95133de6d 100644 --- a/crates/matrix-sdk-ui/src/timeline/polls.rs +++ b/crates/matrix-sdk-ui/src/timeline/polls.rs @@ -162,6 +162,11 @@ impl PollPendingEvents { }); } + pub(super) fn clear(&mut self) { + self.pending_poll_ends.clear(); + self.pending_poll_responses.clear(); + } + pub(super) fn add_end(&mut self, start_id: &EventId, timestamp: MilliSecondsSinceUnixEpoch) { self.pending_poll_ends.insert(start_id.to_owned(), timestamp); } From 7447d54a5ece08e7306e148523b6f679441b50a8 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 24 Jul 2024 21:11:15 +0200 Subject: [PATCH 04/11] timeline: move reaction-related structs to the reaction module, move meta's reaction fields to the Reactions object No changes in functionality, pure code motion. --- .../matrix-sdk-ui/src/timeline/inner/mod.rs | 53 +++++++------------ .../matrix-sdk-ui/src/timeline/inner/state.rs | 15 +----- crates/matrix-sdk-ui/src/timeline/mod.rs | 16 +----- .../matrix-sdk-ui/src/timeline/reactions.rs | 49 +++++++++++++++-- .../matrix-sdk-ui/src/timeline/tests/mod.rs | 4 +- .../src/timeline/tests/reactions.rs | 4 +- 6 files changed, 72 insertions(+), 69 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/inner/mod.rs b/crates/matrix-sdk-ui/src/timeline/inner/mod.rs index 120a5fcb676..ba5d0b180f5 100644 --- a/crates/matrix-sdk-ui/src/timeline/inner/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/inner/mod.rs @@ -58,12 +58,12 @@ 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, - TimelineItemContent, TimelineItemKind, + Error, EventSendState, EventTimelineItem, InReplyToDetails, Message, PaginationError, Profile, + RepliedToEvent, TimelineDetails, TimelineFocus, TimelineItem, TimelineItemContent, + TimelineItemKind, }; use crate::{ timeline::{day_dividers::DayDividerAdjuster, TimelineEventFilterFn}, @@ -112,30 +112,6 @@ pub(super) struct TimelineInner { 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), - /// 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? @@ -453,8 +429,11 @@ impl TimelineInner

{ 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::(&annotation.into()); + let in_flight = state + .meta + .reactions + .in_flight_reaction + .get::(&annotation.into()); let txn_id = match in_flight { Some(ReactionState::Sending(txn_id)) => { // Use the transaction ID as the in flight request @@ -501,10 +480,11 @@ impl TimelineInner

{ } }; - 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::(&annotation.into()); + let in_flight = + state.meta.reactions.in_flight_reaction.get::(&annotation.into()); let result = match in_flight { Some(_) => { // There is an in-flight request @@ -529,7 +509,7 @@ impl TimelineInner

{ 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 +714,7 @@ impl TimelineInner

{ let reaction_state = state .meta + .reactions .reaction_state .get(&AnnotationKey::from(annotation)) .expect("Reaction state should be set before sending the reaction"); @@ -743,6 +724,7 @@ impl TimelineInner

{ // 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 +734,15 @@ impl TimelineInner

{ 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 diff --git a/crates/matrix-sdk-ui/src/timeline/inner/state.rs b/crates/matrix-sdk-ui/src/timeline/inner/state.rs index 116a0bbb896..86b1c7baeed 100644 --- a/crates/matrix-sdk-ui/src/timeline/inner/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/inner/state.rs @@ -15,7 +15,6 @@ use std::{collections::VecDeque, future::Future, sync::Arc}; use eyeball_im::{ObservableVector, ObservableVectorTransaction, ObservableVectorTransactionEntry}; -use indexmap::IndexMap; use matrix_sdk::{deserialized_responses::SyncTimelineEvent, send_queue::SendHandle}; use matrix_sdk_base::deserialized_responses::TimelineEvent; #[cfg(test)] @@ -29,7 +28,7 @@ use ruma::{ }; use tracing::{debug, error, instrument, trace, warn}; -use super::{HandleManyEventsResult, ReactionState, TimelineInnerSettings}; +use super::{HandleManyEventsResult, TimelineInnerSettings}; use crate::{ events::SyncTimelineEventWithoutContent, timeline::{ @@ -44,8 +43,7 @@ use crate::{ read_receipts::ReadReceipts, traits::RoomDataProvider, util::{rfind_event_by_id, rfind_event_item, RelativePosition}, - AnnotationKey, Error as TimelineError, Profile, ReactionSenderData, TimelineItem, - TimelineItemKind, + Error as TimelineError, Profile, ReactionSenderData, TimelineItem, TimelineItemKind, }, unable_to_decrypt_hook::UtdHookManager, }; @@ -757,11 +755,6 @@ pub(in crate::timeline) struct TimelineInnerMetadata { pub has_up_to_date_read_marker_item: bool, pub read_receipts: ReadReceipts, - - /// The local reaction request state that is queued next. - pub reaction_state: IndexMap, - /// The in-flight reaction request state that is ongoing. - pub in_flight_reaction: IndexMap, } impl TimelineInnerMetadata { @@ -780,8 +773,6 @@ impl TimelineInnerMetadata { // field, otherwise we'll keep on exiting early in `Self::update_read_marker`. has_up_to_date_read_marker_item: true, read_receipts: Default::default(), - reaction_state: Default::default(), - in_flight_reaction: Default::default(), room_version, unable_to_decrypt_hook, internal_id_prefix, @@ -798,8 +789,6 @@ impl TimelineInnerMetadata { // before attempting to update it for each new timeline item. self.has_up_to_date_read_marker_item = true; self.read_receipts.clear(); - self.reaction_state.clear(); - self.in_flight_reaction.clear(); } /// Get the relative positions of two events in the timeline. diff --git a/crates/matrix-sdk-ui/src/timeline/mod.rs b/crates/matrix-sdk-ui/src/timeline/mod.rs index 86cd72533f3..5a929f034ba 100644 --- a/crates/matrix-sdk-ui/src/timeline/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/mod.rs @@ -34,6 +34,7 @@ use matrix_sdk::{ use matrix_sdk_base::RoomState; use mime::Mime; use pin_project_lite::pin_project; +use reactions::ReactionAction; use ruma::{ api::client::receipt::create_receipt::v3::ReceiptType, events::{ @@ -102,7 +103,7 @@ pub use self::{ }; use self::{ futures::SendAttachment, - inner::{ReactionAction, TimelineInner}, + inner::TimelineInner, reactions::ReactionToggleResult, util::{rfind_event_by_id, rfind_event_item}, }; @@ -164,19 +165,6 @@ pub struct Timeline { drop_handle: Arc, } -// Implements hash etc -#[derive(Clone, Hash, PartialEq, Eq, Debug)] -struct AnnotationKey { - event_id: OwnedEventId, - key: String, -} - -impl From<&Annotation> for AnnotationKey { - fn from(annotation: &Annotation) -> Self { - Self { event_id: annotation.event_id.clone(), key: annotation.key.clone() } - } -} - /// What should the timeline focus on? #[derive(Clone, Debug, PartialEq)] pub enum TimelineFocus { diff --git a/crates/matrix-sdk-ui/src/timeline/reactions.rs b/crates/matrix-sdk-ui/src/timeline/reactions.rs index 028eb6bc441..4ecebd75ce1 100644 --- a/crates/matrix-sdk-ui/src/timeline/reactions.rs +++ b/crates/matrix-sdk-ui/src/timeline/reactions.rs @@ -14,7 +14,7 @@ use std::collections::HashMap; -use indexmap::IndexSet; +use indexmap::{IndexMap, IndexSet}; use ruma::{ events::relation::Annotation, MilliSecondsSinceUnixEpoch, OwnedEventId, OwnedTransactionId, OwnedUserId, @@ -22,6 +22,43 @@ use ruma::{ use super::event_item::TimelineEventItemId; +// Implements hash etc +#[derive(Clone, Hash, PartialEq, Eq, Debug)] +pub(super) struct AnnotationKey { + event_id: OwnedEventId, + key: String, +} + +impl From<&Annotation> for AnnotationKey { + fn from(annotation: &Annotation) -> Self { + Self { event_id: annotation.event_id.clone(), key: annotation.key.clone() } + } +} + +#[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), + /// 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), +} + /// Data associated with a reaction sender. It can be used to display /// a details UI component for a reaction with both sender /// names and the date at which they sent a reaction. @@ -36,16 +73,22 @@ pub struct ReactionSenderData { #[derive(Clone, Debug, Default)] pub(super) struct Reactions { /// Reaction event / txn ID => sender and reaction data. - pub(super) map: HashMap, + pub map: HashMap, /// ID of event that is not in the timeline yet => List of reaction event /// IDs. - pub(super) pending: HashMap>, + pub pending: HashMap>, + /// The local reaction request state that is queued next. + pub reaction_state: IndexMap, + /// The in-flight reaction request state that is ongoing. + pub in_flight_reaction: IndexMap, } impl Reactions { pub(super) fn clear(&mut self) { self.map.clear(); self.pending.clear(); + self.reaction_state.clear(); + self.in_flight_reaction.clear(); } } diff --git a/crates/matrix-sdk-ui/src/timeline/tests/mod.rs b/crates/matrix-sdk-ui/src/timeline/tests/mod.rs index bc27dbe7114..cb09ebeb888 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/mod.rs @@ -53,8 +53,8 @@ use ruma::{ use super::{ event_handler::TimelineEventKind, event_item::RemoteEventOrigin, - inner::{ReactionAction, TimelineEnd, TimelineInnerSettings}, - reactions::ReactionToggleResult, + inner::{TimelineEnd, TimelineInnerSettings}, + reactions::{ReactionAction, ReactionToggleResult}, traits::RoomDataProvider, EventTimelineItem, Profile, TimelineFocus, TimelineInner, TimelineItem, }; diff --git a/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs b/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs index 7e1b993f0f1..d5b0a38e47b 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs @@ -30,8 +30,8 @@ use stream_assert::assert_next_matches; use crate::timeline::{ event_item::RemoteEventOrigin, - inner::{ReactionAction, TimelineEnd}, - reactions::ReactionToggleResult, + inner::TimelineEnd, + reactions::{ReactionAction, ReactionToggleResult}, tests::{assert_event_is_updated, assert_no_more_updates, TestTimeline}, TimelineItem, }; From 4f99f4ca924f1fe29173a453be2f207af310a841 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 24 Jul 2024 21:13:09 +0200 Subject: [PATCH 05/11] ffi: remove the `Reaction::count` field It's exactly the same as `Reaction::senders`'s length. --- bindings/matrix-sdk-ffi/src/timeline/content.rs | 1 - bindings/matrix-sdk-ffi/src/timeline/mod.rs | 1 - 2 files changed, 2 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/timeline/content.rs b/bindings/matrix-sdk-ffi/src/timeline/content.rs index b9c92051c94..dad31e94742 100644 --- a/bindings/matrix-sdk-ffi/src/timeline/content.rs +++ b/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, } diff --git a/bindings/matrix-sdk-ffi/src/timeline/mod.rs b/bindings/matrix-sdk-ffi/src/timeline/mod.rs index 8ad1df9585c..4393df5d379 100644 --- a/bindings/matrix-sdk-ffi/src/timeline/mod.rs +++ b/bindings/matrix-sdk-ffi/src/timeline/mod.rs @@ -978,7 +978,6 @@ impl EventTimelineItem { .iter() .map(|(k, v)| Reaction { key: k.to_owned(), - count: v.len().try_into().unwrap(), senders: v .senders() .map(|v| ReactionSenderData { From 18a5dd085bee1801cbc709f170417cbbce6cb3d1 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 24 Jul 2024 21:31:01 +0200 Subject: [PATCH 06/11] timeline: remove Deref for ReactionGroup --- .../src/timeline/event_item/reactions.rs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/event_item/reactions.rs b/crates/matrix-sdk-ui/src/timeline/event_item/reactions.rs index d117fb94d83..114047eec45 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_item/reactions.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_item/reactions.rs @@ -12,8 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::ops::Deref; - use indexmap::IndexMap; use itertools::Itertools as _; use ruma::{OwnedEventId, OwnedTransactionId, UserId}; @@ -37,7 +35,12 @@ pub struct ReactionGroup(pub(in crate::timeline) IndexMap impl Iterator { - self.values().unique_by(|v| &v.sender_id) + self.0.values().unique_by(|v| &v.sender_id) + } + + /// Returns the number of reactions in this group. + pub fn len(&self) -> usize { + self.0.len() } /// All reactions within this reaction group that were sent by the given @@ -49,7 +52,7 @@ impl ReactionGroup { &'a self, user_id: &'a UserId, ) -> impl Iterator, Option<&OwnedEventId>)> + 'a { - self.iter().filter_map(move |(k, v)| { + self.0.iter().filter_map(move |(k, v)| { (v.sender_id == user_id).then_some(match k { TimelineEventItemId::TransactionId(txn_id) => (Some(txn_id), None), TimelineEventItemId::EventId(event_id) => (None, Some(event_id)), @@ -57,11 +60,3 @@ impl ReactionGroup { }) } } - -impl Deref for ReactionGroup { - type Target = IndexMap; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} From b9e00de3a86ca5e939355464110471684532b8fc Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 24 Jul 2024 21:49:11 +0200 Subject: [PATCH 07/11] timeline: reorganize `handle_reaction` --- .../src/timeline/event_handler.rs | 57 ++++++++++--------- 1 file changed, 30 insertions(+), 27 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index 60388e4f38a..a3394bcbea6 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -533,7 +533,8 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { // Redacted reaction events are no-ops so don't need to be handled #[instrument(skip_all, fields(relates_to_event_id = ?c.relates_to.event_id))] fn handle_reaction(&mut self, c: ReactionEventContent) { - let event_id: &EventId = &c.relates_to.event_id; + let reacted_to_event_id = &c.relates_to.event_id; + let (reaction_id, old_txn_id) = match &self.ctx.flow { Flow::Local { txn_id, .. } => { (TimelineEventItemId::TransactionId(txn_id.clone()), None) @@ -543,29 +544,22 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { } }; - if let Some((idx, event_item)) = rfind_event_by_id(self.items, event_id) { + if let Some((idx, event_item)) = rfind_event_by_id(self.items, reacted_to_event_id) { let Some(remote_event_item) = event_item.as_remote() else { - error!("inconsistent state: reaction received on a non-remote event item"); + error!("received reaction to a local echo"); return; }; - // Handling of reactions on redacted events is an open question. - // For now, ignore reactions on redacted events like Element does. + // Ignore reactions on redacted events. if let TimelineItemContent::RedactedMessage = event_item.content() { debug!("Ignoring reaction on redacted event"); return; } + // Add the reaction to event item's bundled reactions. let mut reactions = remote_event_item.reactions.clone(); - let reaction_group = reactions.entry(c.relates_to.key.clone()).or_default(); - - if let Some(txn_id) = old_txn_id { - let id = TimelineEventItemId::TransactionId(txn_id.clone()); - // Remove the local echo from the related event. - reaction_group.0.swap_remove(&id); - } - reaction_group.0.insert( + reactions.entry(c.relates_to.key.clone()).or_default().0.insert( reaction_id.clone(), ReactionSenderData { sender_id: self.ctx.sender.clone(), @@ -576,32 +570,41 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { trace!("Adding reaction"); self.items .set(idx, event_item.with_inner_kind(remote_event_item.with_reactions(reactions))); + self.result.items_updated += 1; } else { trace!("Timeline item not found, adding reaction to the pending list"); + let TimelineEventItemId::EventId(reaction_event_id) = reaction_id.clone() else { error!("Adding local reaction echo to event absent from the timeline"); return; }; - let pending = self.meta.reactions.pending.entry(event_id.to_owned()).or_default(); - - pending.insert(reaction_event_id); + self.meta + .reactions + .pending + .entry(reacted_to_event_id.to_owned()) + .or_default() + .insert(reaction_event_id); } - if let Flow::Remote { txn_id: Some(txn_id), .. } = &self.ctx.flow { - let id = TimelineEventItemId::TransactionId(txn_id.clone()); - // Remove the local echo from the reaction map. It could be missing, if the - // transaction id refers to a reaction sent times ago, so no need to check its - // return value to know if the value was missing or not. - self.meta.reactions.map.remove(&id); + if let Some(txn_id) = old_txn_id { + // Try to remove a local echo of that reaction. It might be missing if the + // reaction wasn't sent by this device, or was sent in a previous + // session. + self.meta.reactions.map.remove(&TimelineEventItemId::TransactionId(txn_id.clone())); } - let reaction_sender_data = ReactionSenderData { - sender_id: self.ctx.sender.clone(), - timestamp: self.ctx.timestamp, - }; - self.meta.reactions.map.insert(reaction_id, (reaction_sender_data, c.relates_to)); + self.meta.reactions.map.insert( + reaction_id, + ( + ReactionSenderData { + sender_id: self.ctx.sender.clone(), + timestamp: self.ctx.timestamp, + }, + c.relates_to, + ), + ); } #[instrument(skip_all, fields(replacement_event_id = ?replacement.event_id))] From 74919fe070dab2f73dd3bf4401ad01a0ea5c0cc5 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 24 Jul 2024 22:51:01 +0200 Subject: [PATCH 08/11] timeline: store reaction by key by sender, instead of by key by local or remote id This makes it impossible to represent states like "there's a local *and* a remote echo for the same sender for a given reaction", or multiple reactions from the same sender to the same event, and so on. --- bindings/matrix-sdk-ffi/src/timeline/mod.rs | 8 +- .../src/timeline/event_handler.rs | 167 +++++++----------- .../src/timeline/event_item/mod.rs | 19 +- .../src/timeline/event_item/reactions.rs | 62 ------- .../src/timeline/event_item/remote.rs | 8 +- .../matrix-sdk-ui/src/timeline/inner/mod.rs | 23 +-- .../matrix-sdk-ui/src/timeline/inner/state.rs | 48 +++-- crates/matrix-sdk-ui/src/timeline/mod.rs | 8 +- .../matrix-sdk-ui/src/timeline/tests/mod.rs | 1 - .../src/timeline/tests/reaction_group.rs | 131 -------------- .../src/timeline/tests/reactions.rs | 41 +++-- .../tests/integration/timeline/mod.rs | 2 +- .../src/tests/timeline.rs | 22 +-- 13 files changed, 154 insertions(+), 386 deletions(-) delete mode 100644 crates/matrix-sdk-ui/src/timeline/event_item/reactions.rs delete mode 100644 crates/matrix-sdk-ui/src/timeline/tests/reaction_group.rs diff --git a/bindings/matrix-sdk-ffi/src/timeline/mod.rs b/bindings/matrix-sdk-ffi/src/timeline/mod.rs index 4393df5d379..b67ec1e21c5 100644 --- a/bindings/matrix-sdk-ffi/src/timeline/mod.rs +++ b/bindings/matrix-sdk-ffi/src/timeline/mod.rs @@ -979,10 +979,10 @@ impl EventTimelineItem { .map(|(k, v)| Reaction { key: k.to_owned(), 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(), }) diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index a3394bcbea6..13bf6bf40d1 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -16,7 +16,7 @@ use std::sync::Arc; use as_variant::as_variant; use eyeball_im::{ObservableVectorTransaction, ObservableVectorTransactionEntry}; -use indexmap::{map::Entry, IndexMap}; +use indexmap::IndexMap; use matrix_sdk::{ crypto::types::events::UtdCause, deserialized_responses::EncryptionInfo, send_queue::SendHandle, }; @@ -51,17 +51,20 @@ use tracing::{debug, error, field::debug, info, instrument, trace, warn}; use super::{ day_dividers::DayDividerAdjuster, event_item::{ - AnyOtherFullStateEventContent, BundledReactions, EventSendState, EventTimelineItemKind, - LocalEventTimelineItem, Profile, RemoteEventOrigin, RemoteEventTimelineItem, - TimelineEventItemId, + AnyOtherFullStateEventContent, EventSendState, EventTimelineItemKind, + LocalEventTimelineItem, Profile, ReactionsByKeyBySender, RemoteEventOrigin, + RemoteEventTimelineItem, TimelineEventItemId, }, inner::{TimelineInnerMetadata, TimelineInnerStateTransaction}, polls::PollState, util::{rfind_event_by_id, rfind_event_item}, - EventTimelineItem, InReplyToDetails, Message, OtherState, ReactionGroup, ReactionSenderData, - Sticker, TimelineDetails, TimelineItem, TimelineItemContent, + EventTimelineItem, InReplyToDetails, Message, OtherState, ReactionSenderData, Sticker, + TimelineDetails, TimelineItem, TimelineItemContent, +}; +use crate::{ + events::SyncTimelineEventWithoutContent, timeline::event_item::ReactionInfo, + DEFAULT_SANITIZER_MODE, }; -use crate::{events::SyncTimelineEventWithoutContent, DEFAULT_SANITIZER_MODE}; /// When adding an event, useful information related to the source of the event. #[derive(Clone)] @@ -424,7 +427,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { self.handle_redaction(redacts); } TimelineEventKind::LocalRedaction { redacts } => { - self.handle_local_redaction(redacts); + self.handle_reaction_redaction(TimelineEventItemId::TransactionId(redacts)); } TimelineEventKind::RoomMember { user_id, content, sender } => { @@ -559,12 +562,9 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { // Add the reaction to event item's bundled reactions. let mut reactions = remote_event_item.reactions.clone(); - reactions.entry(c.relates_to.key.clone()).or_default().0.insert( - reaction_id.clone(), - ReactionSenderData { - sender_id: self.ctx.sender.clone(), - timestamp: self.ctx.timestamp, - }, + reactions.entry(c.relates_to.key.clone()).or_default().insert( + self.ctx.sender.clone(), + ReactionInfo { timestamp: self.ctx.timestamp, id: reaction_id.clone() }, ); trace!("Adding reaction"); @@ -710,77 +710,24 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { /// redacts it. /// /// This only applies to *remote* events; for local items being redacted, - /// use [`Self::handle_local_redaction`]. + /// use [`Self::handle_reaction_redaction`]. /// /// This assumes the redacted event was present in the timeline in the first /// place; it will warn if the redacted event has not been found. - #[instrument(skip_all, fields(redacts_event_id = ?redacts))] - fn handle_redaction(&mut self, redacts: OwnedEventId) { + #[instrument(skip_all, fields(redacts_event_id = ?redacted))] + fn handle_redaction(&mut self, redacted: OwnedEventId) { // TODO: Apply local redaction of PollResponse and PollEnd events. // https://github.com/matrix-org/matrix-rust-sdk/pull/2381#issuecomment-1689647825 - let id = TimelineEventItemId::EventId(redacts.clone()); - // If it's a reaction that's being redacted, handle it here. - if let Some((_, rel)) = self.meta.reactions.map.remove(&id) { - let found_reacted_to = self.update_timeline_item(&rel.event_id, |_this, event_item| { - let Some(remote_event_item) = event_item.as_remote() else { - error!("inconsistent state: redaction received on a non-remote event item"); - return None; - }; - - let mut reactions = remote_event_item.reactions.clone(); - - let count = { - let Entry::Occupied(mut group_entry) = reactions.entry(rel.key.clone()) else { - return None; - }; - let group = group_entry.get_mut(); - - if group.0.swap_remove(&id).is_none() { - error!( - "inconsistent state: reaction from reaction_map not in reaction list \ - of timeline item" - ); - return None; - } - - group.len() - }; - - if count == 0 { - reactions.swap_remove(&rel.key); - } - - trace!("Removing reaction"); - Some(event_item.with_kind(remote_event_item.with_reactions(reactions))) - }); - - if !found_reacted_to { - debug!("Timeline item not found, discarding reaction redaction"); - } - - if self.result.items_updated == 0 { - if let Some(reactions) = self.meta.reactions.pending.get_mut(&rel.event_id) { - if !reactions.swap_remove(&redacts.clone()) { - error!( - "inconsistent state: reaction from reaction_map not in reaction list \ - of pending_reactions" - ); - } - } else { - warn!("reaction_map out of sync with timeline items"); - } - } - - // Even if the event being redacted is a reaction (found in - // `reaction_map`), it can still be present in the timeline items - // directly with the raw event timeline feature (not yet - // implemented) => no early return here. + if self.handle_reaction_redaction(TimelineEventItemId::EventId(redacted.clone())) { + // When we have raw timeline items, we should not return here anymore, as we + // might need to redact the raw item as well. + return; } // General path: redact another kind of (non-reaction) event. - let found_redacted_event = self.update_timeline_item(&redacts, |this, event_item| { + let found_redacted_event = self.update_timeline_item(&redacted, |this, event_item| { if event_item.as_remote().is_none() { error!("inconsistent state: redaction received on a non-remote event item"); return None; @@ -810,7 +757,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { let Some(message) = event_item.content.as_message() else { return }; let Some(in_reply_to) = message.in_reply_to() else { return }; let TimelineDetails::Ready(replied_to_event) = &in_reply_to.event else { return }; - if redacts == in_reply_to.event_id { + if redacted == in_reply_to.event_id { let replied_to_event = replied_to_event.redact(&self.meta.room_version); let in_reply_to = InReplyToDetails { event_id: in_reply_to.event_id.clone(), @@ -824,50 +771,57 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { }); } - // Redacted redactions are no-ops (unfortunately) - #[instrument(skip_all, fields(redacts_event_id = ?redacts))] - fn handle_local_redaction(&mut self, redacts: OwnedTransactionId) { - let id = TimelineEventItemId::TransactionId(redacts); - - // Redact the reaction, if any. - if let Some((_, rel)) = self.meta.reactions.map.remove(&id) { - let found = self.update_timeline_item(&rel.event_id, |_this, event_item| { + /// Attempts to redact a reaction, local or remote. + /// + /// Returns true if it's succeeded. + #[instrument(skip_all, fields(redacts = ?reaction_id))] + fn handle_reaction_redaction(&mut self, reaction_id: TimelineEventItemId) -> bool { + if let Some((_, rel)) = self.meta.reactions.map.remove(&reaction_id) { + let updated_event = self.update_timeline_item(&rel.event_id, |this, event_item| { let Some(remote_event_item) = event_item.as_remote() else { error!("inconsistent state: redaction received on a non-remote event item"); return None; }; let mut reactions = remote_event_item.reactions.clone(); - let Entry::Occupied(mut group_entry) = reactions.entry(rel.key.clone()) else { - return None; - }; - let group = group_entry.get_mut(); - if group.0.swap_remove(&id).is_none() { - error!( - "inconsistent state: reaction from reaction_map not in reaction list \ - of timeline item" - ); - return None; - } - - if group.len() == 0 { - group_entry.swap_remove(); + if let Some(by_sender) = reactions.get_mut(&rel.key) { + if let Some(reaction) = by_sender.get(&this.ctx.sender) { + if reaction.id == reaction_id { + by_sender.swap_remove(&this.ctx.sender); + // Remove the reaction group if this was the last reaction. + if by_sender.is_empty() { + reactions.swap_remove(&rel.key); + } + } else { + warn!("Tried to remove a reaction which didn't match the known one."); + } + } } trace!("Removing reaction"); Some(event_item.with_kind(remote_event_item.with_reactions(reactions))) }); - if !found { - debug!("Timeline item not found, discarding local redaction"); + if !updated_event { + if let TimelineEventItemId::EventId(event_id) = reaction_id { + // If 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(reactions) = self.meta.reactions.pending.get_mut(&rel.event_id) { + reactions.swap_remove(&event_id); + } + } } + + return updated_event; } if self.result.items_updated == 0 { // We will want to know this when debugging redaction issues. error!("redaction affected no event"); } + + false } /// Add a new event item in the timeline. @@ -1046,7 +1000,10 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { } } - fn pending_reactions(&mut self, content: &TimelineItemContent) -> Option { + fn pending_reactions( + &mut self, + content: &TimelineItemContent, + ) -> Option { // Drop pending reactions if the message is redacted. if let TimelineItemContent::RedactedMessage = content { return None; @@ -1069,9 +1026,13 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { continue; }; - let group: &mut ReactionGroup = + let group: &mut IndexMap = bundled.entry(annotation.key.clone()).or_default(); - group.0.insert(reaction_id, reaction_sender_data.clone()); + + group.insert( + reaction_sender_data.sender_id.clone(), + ReactionInfo { timestamp: reaction_sender_data.timestamp, id: reaction_id }, + ); } Some(bundled) diff --git a/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs b/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs index 53021494c48..34a8aae9fa8 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs +++ b/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 = Lazy::new(Default::default); + static EMPTY_REACTIONS: Lazy = Lazy::new(Default::default); match &self.kind { EventTimelineItemKind::Local(_) => &EMPTY_REACTIONS, EventTimelineItemKind::Remote(remote_event) => &remote_event.reactions, @@ -580,6 +578,19 @@ pub enum EventItemOrigin { Pagination, } +/// Information about a single reaction stored in [`ReactionsByKeyBySender`]. +#[derive(Clone, Debug)] +pub struct ReactionInfo { + pub timestamp: MilliSecondsSinceUnixEpoch, + 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>; + #[cfg(test)] mod tests { use assert_matches::assert_matches; diff --git a/crates/matrix-sdk-ui/src/timeline/event_item/reactions.rs b/crates/matrix-sdk-ui/src/timeline/event_item/reactions.rs deleted file mode 100644 index 114047eec45..00000000000 --- a/crates/matrix-sdk-ui/src/timeline/event_item/reactions.rs +++ /dev/null @@ -1,62 +0,0 @@ -// Copyright 2023 The Matrix.org Foundation C.I.C. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -use indexmap::IndexMap; -use itertools::Itertools as _; -use ruma::{OwnedEventId, OwnedTransactionId, UserId}; - -use super::TimelineEventItemId; -use crate::timeline::ReactionSenderData; - -/// The reactions grouped by key. -/// -/// Key: The reaction, usually an emoji. -/// Value: The group of reactions. -pub type BundledReactions = IndexMap; - -/// A group of reaction events on the same event with the same key. -/// -/// This is a map of the event ID or transaction ID of the reactions to the ID -/// of the sender of the reaction. -#[derive(Clone, Debug, Default)] -pub struct ReactionGroup(pub(in crate::timeline) IndexMap); - -impl ReactionGroup { - /// The (deduplicated) senders of the reactions in this group. - pub fn senders(&self) -> impl Iterator { - self.0.values().unique_by(|v| &v.sender_id) - } - - /// Returns the number of reactions in this group. - pub fn len(&self) -> usize { - self.0.len() - } - - /// All reactions within this reaction group that were sent by the given - /// user. - /// - /// Note that it is possible for multiple reactions by the same user to - /// have arrived over federation. - pub fn by_sender<'a>( - &'a self, - user_id: &'a UserId, - ) -> impl Iterator, Option<&OwnedEventId>)> + 'a { - self.0.iter().filter_map(move |(k, v)| { - (v.sender_id == user_id).then_some(match k { - TimelineEventItemId::TransactionId(txn_id) => (Some(txn_id), None), - TimelineEventItemId::EventId(event_id) => (None, Some(event_id)), - }) - }) - } -} diff --git a/crates/matrix-sdk-ui/src/timeline/event_item/remote.rs b/crates/matrix-sdk-ui/src/timeline/event_item/remote.rs index add84ba0cf3..a17979b4a64 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_item/remote.rs +++ b/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, /// All bundled reactions about the event. - pub reactions: BundledReactions, + pub reactions: ReactionsByKeyBySender, /// All read receipts for the event. /// @@ -74,7 +74,7 @@ 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() } } @@ -82,7 +82,7 @@ impl RemoteEventTimelineItem { /// JSON representation fields. pub fn redact(&self) -> Self { Self { - reactions: BundledReactions::default(), + reactions: ReactionsByKeyBySender::default(), original_json: None, latest_edit_json: None, ..self.clone() diff --git a/crates/matrix-sdk-ui/src/timeline/inner/mod.rs b/crates/matrix-sdk-ui/src/timeline/inner/mod.rs index ba5d0b180f5..37ed108e99b 100644 --- a/crates/matrix-sdk-ui/src/timeline/inner/mod.rs +++ b/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::{ @@ -62,8 +61,8 @@ use super::{ traits::RoomDataProvider, util::{rfind_event_by_id, rfind_event_item, RelativePosition}, Error, EventSendState, EventTimelineItem, InReplyToDetails, Message, PaginationError, Profile, - RepliedToEvent, TimelineDetails, TimelineFocus, TimelineItem, TimelineItemContent, - TimelineItemKind, + RepliedToEvent, TimelineDetails, TimelineEventItemId, TimelineFocus, TimelineItem, + TimelineItemContent, TimelineItemKind, }; use crate::{ timeline::{day_dividers::DayDividerAdjuster, TimelineEventFilterFn}, @@ -409,17 +408,13 @@ impl TimelineInner

{ }; 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)) }; diff --git a/crates/matrix-sdk-ui/src/timeline/inner/state.rs b/crates/matrix-sdk-ui/src/timeline/inner/state.rs index 86b1c7baeed..015742de86f 100644 --- a/crates/matrix-sdk-ui/src/timeline/inner/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/inner/state.rs @@ -37,7 +37,7 @@ use crate::{ Flow, HandleEventResult, TimelineEventContext, TimelineEventHandler, TimelineEventKind, TimelineItemPosition, }, - event_item::{RemoteEventOrigin, TimelineEventItemId}, + event_item::{ReactionInfo, RemoteEventOrigin, TimelineEventItemId}, polls::PollPendingEvents, reactions::{ReactionToggleResult, Reactions}, read_receipts::ReadReceipts, @@ -277,42 +277,36 @@ impl TimelineInnerState { error!("inconsistent state: reaction received on a non-remote event item"); return Err(TimelineError::FailedToToggleReaction); }; - // Note: remote event is not synced yet, so we're adding an item - // with the local timestamp. - let reaction_sender_data = ReactionSenderData { - sender_id: own_user_id.to_owned(), - timestamp: MilliSecondsSinceUnixEpoch::now(), - }; + + let now = MilliSecondsSinceUnixEpoch::now(); + let reaction_sender_data = + ReactionSenderData { sender_id: own_user_id.to_owned(), timestamp: now }; let new_reactions = { let mut reactions = remote_related.reactions.clone(); - let reaction_group = reactions.entry(annotation.key.clone()).or_default(); - - // Remove the local echo from the related event. - if let Some(txn_id) = local_echo_to_remove { - let id = TimelineEventItemId::TransactionId(txn_id.clone()); - if reaction_group.0.swap_remove(&id).is_none() { - warn!( - "Tried to remove reaction by transaction ID, but didn't \ - find matching reaction in the related event's reactions" - ); - } - } + let reaction_by_sender = reactions.entry(annotation.key.clone()).or_default(); - // Add the remote echo to the related event if let Some(event_id) = remote_echo_to_add { - reaction_group.0.insert( - TimelineEventItemId::EventId(event_id.clone()), - reaction_sender_data.clone(), + reaction_by_sender.insert( + own_user_id.to_owned(), + ReactionInfo { + // Note: remote event is not synced yet, so we're adding an item + // with the local timestamp. + timestamp: now, + id: TimelineEventItemId::EventId(event_id.clone()), + }, ); - }; - - if reaction_group.0.is_empty() { - reactions.swap_remove(&annotation.key); + } else if local_echo_to_remove.is_some() { + reaction_by_sender.swap_remove(own_user_id); + // Remove the group if we were the last reaction. + if reaction_by_sender.is_empty() { + reactions.swap_remove(&annotation.key); + } } reactions }; + let new_related = related.with_kind(remote_related.with_reactions(new_reactions)); // Update the reactions stored in the timeline state diff --git a/crates/matrix-sdk-ui/src/timeline/mod.rs b/crates/matrix-sdk-ui/src/timeline/mod.rs index 5a929f034ba..a7c4d7b7562 100644 --- a/crates/matrix-sdk-ui/src/timeline/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/mod.rs @@ -87,10 +87,10 @@ pub use self::{ builder::TimelineBuilder, error::*, event_item::{ - AnyOtherFullStateEventContent, BundledReactions, EncryptedMessage, EventItemOrigin, - EventSendState, EventTimelineItem, InReplyToDetails, MemberProfileChange, MembershipChange, - Message, OtherState, Profile, ReactionGroup, RepliedToEvent, RoomMembershipChange, Sticker, - TimelineDetails, TimelineEventItemId, TimelineItemContent, + AnyOtherFullStateEventContent, EncryptedMessage, EventItemOrigin, EventSendState, + EventTimelineItem, InReplyToDetails, MemberProfileChange, MembershipChange, Message, + OtherState, Profile, RepliedToEvent, RoomMembershipChange, Sticker, TimelineDetails, + TimelineEventItemId, TimelineItemContent, }, event_type_filter::TimelineEventTypeFilter, inner::default_event_filter, diff --git a/crates/matrix-sdk-ui/src/timeline/tests/mod.rs b/crates/matrix-sdk-ui/src/timeline/tests/mod.rs index cb09ebeb888..90ff38618f6 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/mod.rs @@ -68,7 +68,6 @@ mod encryption; mod event_filter; mod invalid; mod polls; -mod reaction_group; mod reactions; mod read_receipts; mod redaction; diff --git a/crates/matrix-sdk-ui/src/timeline/tests/reaction_group.rs b/crates/matrix-sdk-ui/src/timeline/tests/reaction_group.rs deleted file mode 100644 index 5a684f9886a..00000000000 --- a/crates/matrix-sdk-ui/src/timeline/tests/reaction_group.rs +++ /dev/null @@ -1,131 +0,0 @@ -// Copyright 2023 The Matrix.org Foundation C.I.C. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -use assert_matches2::assert_let; -use itertools::Itertools; -use matrix_sdk_test::{ALICE, BOB}; -use ruma::{server_name, uint, user_id, EventId, MilliSecondsSinceUnixEpoch, OwnedUserId, UserId}; - -use crate::timeline::{event_item::TimelineEventItemId, ReactionGroup, ReactionSenderData}; - -#[test] -fn test_by_sender() { - let alice = ALICE.to_owned(); - let bob = BOB.to_owned(); - - let reaction_1 = new_reaction(); - let reaction_2 = new_reaction(); - - let mut reaction_group = ReactionGroup::default(); - reaction_group.0.insert(reaction_1.clone(), new_sender_data(alice.clone())); - reaction_group.0.insert(reaction_2, new_sender_data(bob)); - - let alice_reactions = reaction_group.by_sender(&alice).collect::>(); - - let reaction = alice_reactions[0]; - - assert_let!(TimelineEventItemId::EventId(event_id) = reaction_1); - assert_eq!(reaction.1.unwrap(), &event_id); -} - -#[test] -fn test_by_sender_with_empty_group() { - let reaction_group = ReactionGroup::default(); - - let reactions = reaction_group.by_sender(&ALICE).collect::>(); - - assert!(reactions.is_empty()); -} - -#[test] -fn test_by_sender_with_multiple_users() { - let alice = ALICE.to_owned(); - let bob = BOB.to_owned(); - let carol = user_id!("@carol:other.server"); - - let reaction_1 = new_reaction(); - let reaction_2 = new_reaction(); - let reaction_3 = new_reaction(); - - let mut reaction_group = ReactionGroup::default(); - reaction_group.0.insert(reaction_1, new_sender_data(alice.clone())); - reaction_group.0.insert(reaction_2, new_sender_data(alice.clone())); - reaction_group.0.insert(reaction_3, new_sender_data(bob.clone())); - - let alice_reactions = reaction_group.by_sender(&alice).collect::>(); - let bob_reactions = reaction_group.by_sender(&bob).collect::>(); - let carol_reactions = reaction_group.by_sender(carol).collect::>(); - - assert_eq!(alice_reactions.len(), 2); - assert_eq!(bob_reactions.len(), 1); - assert!(carol_reactions.is_empty()); -} - -/// The Matrix spec does not allow duplicate annotations to be created but it -/// is still possible for duplicates to be received over federation. And in -/// that case, clients are expected to treat duplicates as a single annotation. -#[test] -fn test_senders_are_deduplicated() { - let group = { - let mut group = ReactionGroup::default(); - insert(&mut group, &ALICE, 3); - insert(&mut group, &BOB, 2); - group - }; - - let senders = group.senders().map(|v| &v.sender_id).collect::>(); - assert_eq!(senders, vec![&ALICE.to_owned(), &BOB.to_owned()]); -} - -#[test] -fn test_timestamps_are_stored() { - let reaction = new_reaction(); - let reaction_2 = new_reaction(); - let timestamp = MilliSecondsSinceUnixEpoch(uint!(0)); - let timestamp_2 = MilliSecondsSinceUnixEpoch::now(); - let mut reaction_group = ReactionGroup::default(); - reaction_group - .0 - .insert(reaction, ReactionSenderData { sender_id: ALICE.to_owned(), timestamp }); - reaction_group.0.insert( - reaction_2, - ReactionSenderData { sender_id: BOB.to_owned(), timestamp: timestamp_2 }, - ); - - assert_eq!( - reaction_group.senders().map(|v| v.timestamp).collect_vec(), - vec![timestamp, timestamp_2] - ); -} - -fn insert(group: &mut ReactionGroup, sender: &UserId, count: u64) { - for _ in 0..count { - group.0.insert( - new_reaction(), - ReactionSenderData { - sender_id: sender.to_owned(), - timestamp: MilliSecondsSinceUnixEpoch::now(), - }, - ); - } -} - -fn new_reaction() -> TimelineEventItemId { - let event_id = EventId::new(server_name!("example.org")); - TimelineEventItemId::EventId(event_id) -} - -fn new_sender_data(sender: OwnedUserId) -> ReactionSenderData { - ReactionSenderData { sender_id: sender, timestamp: MilliSecondsSinceUnixEpoch::now() } -} diff --git a/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs b/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs index d5b0a38e47b..7332b697501 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs @@ -19,12 +19,11 @@ use assert_matches2::assert_let; use eyeball_im::VectorDiff; use futures_core::Stream; use matrix_sdk::test_utils::events::EventFactory; -use matrix_sdk_base::deserialized_responses::SyncTimelineEvent; use matrix_sdk_test::{async_test, ALICE, BOB}; use ruma::{ event_id, events::{relation::Annotation, room::message::RoomMessageEventContent}, - server_name, uint, EventId, MilliSecondsSinceUnixEpoch, OwnedEventId, TransactionId, + server_name, uint, EventId, MilliSecondsSinceUnixEpoch, OwnedEventId, TransactionId, UserId, }; use stream_assert::assert_next_matches; @@ -33,7 +32,7 @@ use crate::timeline::{ inner::TimelineEnd, reactions::{ReactionAction, ReactionToggleResult}, tests::{assert_event_is_updated, assert_no_more_updates, TestTimeline}, - TimelineItem, + TimelineEventItemId, TimelineItem, }; const REACTION_KEY: &str = "👍"; @@ -230,7 +229,7 @@ async fn test_reactions_store_timestamp() { let _ = timeline.toggle_reaction_local(&reaction).await.unwrap(); let event = assert_event_is_updated(&mut stream, &msg_id, msg_pos).await; let reactions = event.reactions().get(&REACTION_KEY.to_owned()).unwrap(); - let timestamp = reactions.senders().next().unwrap().timestamp; + let timestamp = reactions.values().next().unwrap().timestamp; assert!(timestamp_range_until_now_from(timestamp_before).contains(×tamp)); // Failing a redaction. @@ -247,7 +246,7 @@ async fn test_reactions_store_timestamp() { // Restores an event with a valid timestamp. let event = assert_event_is_updated(&mut stream, &msg_id, msg_pos).await; let reactions = event.reactions().get(&REACTION_KEY.to_owned()).unwrap(); - let new_timestamp = reactions.senders().next().unwrap().timestamp; + let new_timestamp = reactions.values().next().unwrap().timestamp; assert!(timestamp_range_until_now_from(timestamp_before).contains(&new_timestamp)); } @@ -255,6 +254,7 @@ async fn test_reactions_store_timestamp() { async fn test_initial_reaction_timestamp_is_stored() { let timeline = TestTimeline::new(); + let f = EventFactory::new().sender(*ALICE); let message_event_id = EventId::new(server_name!("dummy.server")); let reaction_timestamp = MilliSecondsSinceUnixEpoch(uint!(39845)); @@ -262,16 +262,12 @@ async fn test_initial_reaction_timestamp_is_stored() { .inner .add_events_at( vec![ - SyncTimelineEvent::new(timeline.event_builder.make_sync_reaction( - *ALICE, - &Annotation::new(message_event_id.clone(), REACTION_KEY.to_owned()), - reaction_timestamp, - )), - SyncTimelineEvent::new(timeline.event_builder.make_sync_message_event_with_id( - *ALICE, - &message_event_id, - RoomMessageEventContent::text_plain("A"), - )), + // Reaction comes first. + f.reaction(&message_event_id, REACTION_KEY.to_owned()) + .server_ts(reaction_timestamp) + .into_sync(), + // Event comes next. + f.text_msg("A").event_id(&message_event_id).into_sync(), ], TimelineEnd::Back, RemoteEventOrigin::Sync, @@ -282,7 +278,7 @@ async fn test_initial_reaction_timestamp_is_stored() { let reactions = items.last().unwrap().as_event().unwrap().reactions(); let entry = reactions.get(&REACTION_KEY.to_owned()).unwrap(); - assert_eq!(reaction_timestamp, entry.senders().next().unwrap().timestamp); + assert_eq!(reaction_timestamp, entry.values().next().unwrap().timestamp); } fn create_reaction(related_message_id: &EventId) -> Annotation { @@ -317,12 +313,15 @@ async fn assert_reaction_is_updated( expected_event_id: Option<&EventId>, expected_txn_id: Option<&TransactionId>, ) { - let own_user_id = &ALICE; + let own_user_id: &UserId = &ALICE; let event = assert_event_is_updated(stream, related_to, message_position).await; let (reaction_tx_id, reaction_event_id) = { let reactions = event.reactions().get(&REACTION_KEY.to_owned()).unwrap(); - let reaction = reactions.by_sender(own_user_id).next().unwrap(); - reaction.to_owned() + let reaction = reactions.get(own_user_id).unwrap(); + match &reaction.id { + TimelineEventItemId::TransactionId(txn_id) => (Some(txn_id), None), + TimelineEventItemId::EventId(event_id) => (None, Some(event_id)), + } }; assert_eq!(reaction_tx_id, expected_txn_id.map(|it| it.to_owned()).as_ref()); assert_eq!(reaction_event_id, expected_event_id.map(|it| it.to_owned()).as_ref()); @@ -333,10 +332,10 @@ async fn assert_reaction_is_added( related_to: &EventId, message_position: usize, ) { - let own_user_id = &ALICE; + let own_user_id: &UserId = &ALICE; let event = assert_event_is_updated(stream, related_to, message_position).await; let reactions = event.reactions().get(&REACTION_KEY.to_owned()).unwrap(); - assert!(reactions.by_sender(own_user_id).next().is_some()); + assert!(reactions.get(own_user_id).is_some()); } async fn assert_reactions_are_removed( diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs b/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs index ab23c28b392..e419f7dd1ba 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs @@ -127,7 +127,7 @@ async fn test_reaction() { assert_eq!(event_item.reactions().len(), 1); let group = &event_item.reactions()["👍"]; assert_eq!(group.len(), 1); - let senders: Vec<_> = group.senders().map(|v| &v.sender_id).collect(); + let senders: Vec<_> = group.keys().collect(); assert_eq!(senders.as_slice(), [user_id!("@bob:example.org")]); // The day divider. diff --git a/testing/matrix-sdk-integration-testing/src/tests/timeline.rs b/testing/matrix-sdk-integration-testing/src/tests/timeline.rs index 95f20145e86..cb8267c6b85 100644 --- a/testing/matrix-sdk-integration-testing/src/tests/timeline.rs +++ b/testing/matrix-sdk-integration-testing/src/tests/timeline.rs @@ -26,7 +26,9 @@ use matrix_sdk::ruma::{ events::{relation::Annotation, room::message::RoomMessageEventContent}, EventId, MilliSecondsSinceUnixEpoch, UserId, }; -use matrix_sdk_ui::timeline::{EventSendState, EventTimelineItem, RoomExt, TimelineItem}; +use matrix_sdk_ui::timeline::{ + EventSendState, EventTimelineItem, RoomExt, TimelineEventItemId, TimelineItem, +}; use tokio::{ spawn, task::JoinHandle, @@ -195,8 +197,11 @@ async fn assert_local_added( let (reaction_tx_id, reaction_event_id) = { let reactions = event.reactions().get(&reaction.key).unwrap(); - let reaction = reactions.by_sender(user_id).next().unwrap(); - reaction.to_owned() + let reaction = reactions.get(user_id).unwrap(); + match &reaction.id { + TimelineEventItemId::TransactionId(txn_id) => (Some(txn_id), None), + TimelineEventItemId::EventId(event_id) => (None, Some(event_id)), + } }; assert_matches!(reaction_tx_id, Some(_)); // Event ID hasn't been received from homeserver yet @@ -222,18 +227,15 @@ async fn assert_remote_added( let event = assert_event_is_updated(stream, event_id, message_position).await; let reactions = event.reactions().get(&reaction.key).unwrap(); - assert_eq!(reactions.senders().count(), 1); + assert_eq!(reactions.keys().count(), 1); - let reaction = reactions.by_sender(user_id).next().unwrap(); - let (reaction_tx_id, reaction_event_id) = reaction; - assert_matches!(reaction_tx_id, None); - assert_matches!(reaction_event_id, Some(value) => value); + let reaction = reactions.get(user_id).unwrap(); + assert_matches!(reaction.id, TimelineEventItemId::EventId(..)); // Remote event should have a timestamp <= than now. // Note: this can actually be equal because if the timestamp from // server is not available, it might be created with a local call to `now()` - let reaction = reactions.senders().next(); - assert!(reaction.unwrap().timestamp <= MilliSecondsSinceUnixEpoch::now()); + assert!(reaction.timestamp <= MilliSecondsSinceUnixEpoch::now()); } async fn assert_event_is_updated( From 79416271f10a70316ebd7cf12a58faf0f6f0b8ec Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 24 Jul 2024 23:10:46 +0200 Subject: [PATCH 09/11] timeline: store pending reactions without the indirection to Reactions::map --- .../src/timeline/event_handler.rs | 53 ++++++++----------- .../matrix-sdk-ui/src/timeline/reactions.rs | 16 ++++-- 2 files changed, 33 insertions(+), 36 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index 13bf6bf40d1..02b6777b009 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -62,7 +62,8 @@ use super::{ TimelineDetails, TimelineItem, TimelineItemContent, }; use crate::{ - events::SyncTimelineEventWithoutContent, timeline::event_item::ReactionInfo, + events::SyncTimelineEventWithoutContent, + timeline::{event_item::ReactionInfo, reactions::PendingReaction}, DEFAULT_SANITIZER_MODE, }; @@ -547,6 +548,11 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { } }; + let reaction_sender_data = ReactionSenderData { + sender_id: self.ctx.sender.clone(), + timestamp: self.ctx.timestamp, + }; + if let Some((idx, event_item)) = rfind_event_by_id(self.items, reacted_to_event_id) { let Some(remote_event_item) = event_item.as_remote() else { error!("received reaction to a local echo"); @@ -580,12 +586,13 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { return; }; - self.meta - .reactions - .pending - .entry(reacted_to_event_id.to_owned()) - .or_default() - .insert(reaction_event_id); + self.meta.reactions.pending.entry(reacted_to_event_id.to_owned()).or_default().insert( + reaction_event_id, + PendingReaction { + key: c.relates_to.key.clone(), + sender_data: reaction_sender_data.clone(), + }, + ); } if let Some(txn_id) = old_txn_id { @@ -595,16 +602,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { self.meta.reactions.map.remove(&TimelineEventItemId::TransactionId(txn_id.clone())); } - self.meta.reactions.map.insert( - reaction_id, - ( - ReactionSenderData { - sender_id: self.ctx.sender.clone(), - timestamp: self.ctx.timestamp, - }, - c.relates_to, - ), - ); + self.meta.reactions.map.insert(reaction_id, (reaction_sender_data, c.relates_to)); } #[instrument(skip_all, fields(replacement_event_id = ?replacement.event_id))] @@ -1015,23 +1013,16 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { let reactions = self.meta.reactions.pending.remove(event_id)?; let mut bundled = IndexMap::new(); - for reaction_event_id in reactions { - let reaction_id = TimelineEventItemId::EventId(reaction_event_id); - let Some((reaction_sender_data, annotation)) = - self.meta.reactions.map.get(&reaction_id) - else { - error!( - "inconsistent state: reaction from pending_reactions not in reaction_map" - ); - continue; - }; - + for (reaction_event_id, reaction) in reactions { let group: &mut IndexMap = - bundled.entry(annotation.key.clone()).or_default(); + bundled.entry(reaction.key).or_default(); group.insert( - reaction_sender_data.sender_id.clone(), - ReactionInfo { timestamp: reaction_sender_data.timestamp, id: reaction_id }, + reaction.sender_data.sender_id, + ReactionInfo { + timestamp: reaction.sender_data.timestamp, + id: TimelineEventItemId::EventId(reaction_event_id), + }, ); } diff --git a/crates/matrix-sdk-ui/src/timeline/reactions.rs b/crates/matrix-sdk-ui/src/timeline/reactions.rs index 4ecebd75ce1..c58b29e39df 100644 --- a/crates/matrix-sdk-ui/src/timeline/reactions.rs +++ b/crates/matrix-sdk-ui/src/timeline/reactions.rs @@ -14,7 +14,7 @@ use std::collections::HashMap; -use indexmap::{IndexMap, IndexSet}; +use indexmap::IndexMap; use ruma::{ events::relation::Annotation, MilliSecondsSinceUnixEpoch, OwnedEventId, OwnedTransactionId, OwnedUserId, @@ -62,7 +62,7 @@ pub(super) enum ReactionState { /// Data associated with a reaction sender. It can be used to display /// a details UI component for a reaction with both sender /// names and the date at which they sent a reaction. -#[derive(Clone, Debug, Eq, PartialEq)] +#[derive(Clone, Debug, Eq, Hash, PartialEq)] pub struct ReactionSenderData { /// Sender identifier. pub sender_id: OwnedUserId, @@ -70,13 +70,19 @@ pub struct ReactionSenderData { pub timestamp: MilliSecondsSinceUnixEpoch, } +#[derive(Clone, Debug)] +pub(crate) struct PendingReaction { + pub key: String, + pub sender_data: ReactionSenderData, +} + #[derive(Clone, Debug, Default)] pub(super) struct Reactions { /// Reaction event / txn ID => sender and reaction data. pub map: HashMap, - /// ID of event that is not in the timeline yet => List of reaction event - /// IDs. - pub pending: HashMap>, + /// Mapping of events that are not in the timeline => reaction event id => + /// pending reaction. + pub pending: HashMap>, /// The local reaction request state that is queued next. pub reaction_state: IndexMap, /// The in-flight reaction request state that is ongoing. From f61cc92c74e360e0a7e14be2358e0be452b62e38 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 24 Jul 2024 23:40:44 +0200 Subject: [PATCH 10/11] timeline: map from the reaction local-or-remote id to the item it's reacting to --- .../src/timeline/event_handler.rs | 65 ++++++++++++------- .../src/timeline/event_item/mod.rs | 1 + .../matrix-sdk-ui/src/timeline/inner/state.rs | 25 +++---- .../matrix-sdk-ui/src/timeline/reactions.rs | 11 +++- 4 files changed, 64 insertions(+), 38 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index 02b6777b009..6cb22e84383 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -57,6 +57,7 @@ use super::{ }, inner::{TimelineInnerMetadata, TimelineInnerStateTransaction}, polls::PollState, + reactions::FullReactionKey, util::{rfind_event_by_id, rfind_event_item}, EventTimelineItem, InReplyToDetails, Message, OtherState, ReactionSenderData, Sticker, TimelineDetails, TimelineItem, TimelineItemContent, @@ -602,7 +603,14 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { self.meta.reactions.map.remove(&TimelineEventItemId::TransactionId(txn_id.clone())); } - self.meta.reactions.map.insert(reaction_id, (reaction_sender_data, c.relates_to)); + self.meta.reactions.map.insert( + reaction_id, + FullReactionKey { + item: TimelineEventItemId::EventId(c.relates_to.event_id), + sender: self.ctx.sender.clone(), + key: c.relates_to.key, + }, + ); } #[instrument(skip_all, fields(replacement_event_id = ?replacement.event_id))] @@ -774,38 +782,47 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { /// Returns true if it's succeeded. #[instrument(skip_all, fields(redacts = ?reaction_id))] fn handle_reaction_redaction(&mut self, reaction_id: TimelineEventItemId) -> bool { - if let Some((_, rel)) = self.meta.reactions.map.remove(&reaction_id) { - let updated_event = self.update_timeline_item(&rel.event_id, |this, event_item| { - let Some(remote_event_item) = event_item.as_remote() else { - error!("inconsistent state: redaction received on a non-remote event item"); - return None; - }; + if let Some(FullReactionKey { + item: TimelineEventItemId::EventId(reacted_to_event_id), + key, + sender, + }) = self.meta.reactions.map.remove(&reaction_id) + { + let updated_event = + self.update_timeline_item(&reacted_to_event_id, |_this, event_item| { + let Some(remote_event_item) = event_item.as_remote() else { + error!("inconsistent state: redaction received on a non-remote event item"); + return None; + }; + + let mut reactions = remote_event_item.reactions.clone(); + + if let Some(by_sender) = reactions.get_mut(&key) { + if by_sender.swap_remove(&sender).is_none() { + warn!("Tried to redact a reaction that wasn't sent by this sender"); + } - let mut reactions = remote_event_item.reactions.clone(); - - if let Some(by_sender) = reactions.get_mut(&rel.key) { - if let Some(reaction) = by_sender.get(&this.ctx.sender) { - if reaction.id == reaction_id { - by_sender.swap_remove(&this.ctx.sender); - // Remove the reaction group if this was the last reaction. - if by_sender.is_empty() { - reactions.swap_remove(&rel.key); - } - } else { - warn!("Tried to remove a reaction which didn't match the known one."); + // Remove the reaction group if this was the last reaction. + if by_sender.is_empty() { + reactions.swap_remove(&key); } + } else { + warn!( + "Tried to redact a reaction for a key not present in the reaction list" + ); } - } - trace!("Removing reaction"); - Some(event_item.with_kind(remote_event_item.with_reactions(reactions))) - }); + trace!("Removing reaction"); + Some(event_item.with_kind(remote_event_item.with_reactions(reactions))) + }); if !updated_event { if let TimelineEventItemId::EventId(event_id) = reaction_id { // If 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(reactions) = self.meta.reactions.pending.get_mut(&rel.event_id) { + if let Some(reactions) = + self.meta.reactions.pending.get_mut(&reacted_to_event_id) + { reactions.swap_remove(&event_id); } } diff --git a/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs b/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs index 34a8aae9fa8..0475cc98016 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs @@ -582,6 +582,7 @@ pub enum EventItemOrigin { #[derive(Clone, Debug)] pub struct ReactionInfo { pub timestamp: MilliSecondsSinceUnixEpoch, + /// Id of the reaction (not the reacted-to event). pub id: TimelineEventItemId, } diff --git a/crates/matrix-sdk-ui/src/timeline/inner/state.rs b/crates/matrix-sdk-ui/src/timeline/inner/state.rs index 015742de86f..c273dcd90c0 100644 --- a/crates/matrix-sdk-ui/src/timeline/inner/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/inner/state.rs @@ -39,11 +39,11 @@ use crate::{ }, event_item::{ReactionInfo, RemoteEventOrigin, TimelineEventItemId}, polls::PollPendingEvents, - reactions::{ReactionToggleResult, Reactions}, + reactions::{FullReactionKey, ReactionToggleResult, Reactions}, read_receipts::ReadReceipts, traits::RoomDataProvider, util::{rfind_event_by_id, rfind_event_item, RelativePosition}, - Error as TimelineError, Profile, ReactionSenderData, TimelineItem, TimelineItemKind, + Error as TimelineError, Profile, TimelineItem, TimelineItemKind, }, unable_to_decrypt_hook::UtdHookManager, }; @@ -278,10 +278,6 @@ impl TimelineInnerState { return Err(TimelineError::FailedToToggleReaction); }; - let now = MilliSecondsSinceUnixEpoch::now(); - let reaction_sender_data = - ReactionSenderData { sender_id: own_user_id.to_owned(), timestamp: now }; - let new_reactions = { let mut reactions = remote_related.reactions.clone(); let reaction_by_sender = reactions.entry(annotation.key.clone()).or_default(); @@ -292,7 +288,7 @@ impl TimelineInnerState { ReactionInfo { // Note: remote event is not synced yet, so we're adding an item // with the local timestamp. - timestamp: now, + timestamp: MilliSecondsSinceUnixEpoch::now(), id: TimelineEventItemId::EventId(event_id.clone()), }, ); @@ -309,24 +305,29 @@ impl TimelineInnerState { let new_related = related.with_kind(remote_related.with_reactions(new_reactions)); - // Update the reactions stored in the timeline state + // Update the reactions stored in the timeline state. { - // Remove the local echo from reaction_map + // Remove the local echo from reaction_map. // (should the local echo already be up-to-date after event handling?) if let Some(txn_id) = local_echo_to_remove { let id = TimelineEventItemId::TransactionId(txn_id.clone()); if self.meta.reactions.map.remove(&id).is_none() { warn!( "Tried to remove reaction by transaction ID, but didn't \ - find matching reaction in the reaction map" + find matching reaction in the reaction map" ); } } - // Add the remote echo to the reaction_map + + // Add the remote echo to the reaction_map. if let Some(event_id) = remote_echo_to_add { self.meta.reactions.map.insert( TimelineEventItemId::EventId(event_id.clone()), - (reaction_sender_data, annotation.clone()), + FullReactionKey { + item: TimelineEventItemId::EventId(annotation.event_id.clone()), + key: annotation.key.clone(), + sender: own_user_id.to_owned(), + }, ); } } diff --git a/crates/matrix-sdk-ui/src/timeline/reactions.rs b/crates/matrix-sdk-ui/src/timeline/reactions.rs index c58b29e39df..7e0bc2519a0 100644 --- a/crates/matrix-sdk-ui/src/timeline/reactions.rs +++ b/crates/matrix-sdk-ui/src/timeline/reactions.rs @@ -76,10 +76,17 @@ pub(crate) struct PendingReaction { pub sender_data: ReactionSenderData, } +#[derive(Clone, Debug)] +pub(crate) struct FullReactionKey { + pub item: TimelineEventItemId, + pub key: String, + pub sender: OwnedUserId, +} + #[derive(Clone, Debug, Default)] pub(super) struct Reactions { - /// Reaction event / txn ID => sender and reaction data. - pub map: HashMap, + /// Reaction event / txn ID => full path to the reaction in some item. + pub map: HashMap, /// Mapping of events that are not in the timeline => reaction event id => /// pending reaction. pub pending: HashMap>, From d23432469022ac4b0315a63f4772599786a8bc90 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Thu, 25 Jul 2024 10:38:04 +0200 Subject: [PATCH 11/11] timeline: flatten `ReactionSenderData` into `PendingReaction` --- .../matrix-sdk-ui/src/timeline/event_handler.rs | 16 ++++++---------- crates/matrix-sdk-ui/src/timeline/mod.rs | 1 - crates/matrix-sdk-ui/src/timeline/reactions.rs | 15 ++++----------- 3 files changed, 10 insertions(+), 22 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index 6cb22e84383..bc13e8019c5 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -59,8 +59,8 @@ use super::{ polls::PollState, reactions::FullReactionKey, util::{rfind_event_by_id, rfind_event_item}, - EventTimelineItem, InReplyToDetails, Message, OtherState, ReactionSenderData, Sticker, - TimelineDetails, TimelineItem, TimelineItemContent, + EventTimelineItem, InReplyToDetails, Message, OtherState, Sticker, TimelineDetails, + TimelineItem, TimelineItemContent, }; use crate::{ events::SyncTimelineEventWithoutContent, @@ -549,11 +549,6 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { } }; - let reaction_sender_data = ReactionSenderData { - sender_id: self.ctx.sender.clone(), - timestamp: self.ctx.timestamp, - }; - if let Some((idx, event_item)) = rfind_event_by_id(self.items, reacted_to_event_id) { let Some(remote_event_item) = event_item.as_remote() else { error!("received reaction to a local echo"); @@ -591,7 +586,8 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { reaction_event_id, PendingReaction { key: c.relates_to.key.clone(), - sender_data: reaction_sender_data.clone(), + sender_id: self.ctx.sender.clone(), + timestamp: self.ctx.timestamp, }, ); } @@ -1035,9 +1031,9 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { bundled.entry(reaction.key).or_default(); group.insert( - reaction.sender_data.sender_id, + reaction.sender_id, ReactionInfo { - timestamp: reaction.sender_data.timestamp, + timestamp: reaction.timestamp, id: TimelineEventItemId::EventId(reaction_event_id), }, ); diff --git a/crates/matrix-sdk-ui/src/timeline/mod.rs b/crates/matrix-sdk-ui/src/timeline/mod.rs index a7c4d7b7562..572534e5fed 100644 --- a/crates/matrix-sdk-ui/src/timeline/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/mod.rs @@ -97,7 +97,6 @@ pub use self::{ item::{TimelineItem, TimelineItemKind}, pagination::LiveBackPaginationStatus, polls::PollResult, - reactions::ReactionSenderData, traits::RoomExt, virtual_item::VirtualTimelineItem, }; diff --git a/crates/matrix-sdk-ui/src/timeline/reactions.rs b/crates/matrix-sdk-ui/src/timeline/reactions.rs index 7e0bc2519a0..a42edd37ae0 100644 --- a/crates/matrix-sdk-ui/src/timeline/reactions.rs +++ b/crates/matrix-sdk-ui/src/timeline/reactions.rs @@ -59,23 +59,16 @@ pub(super) enum ReactionState { Sending(OwnedTransactionId), } -/// Data associated with a reaction sender. It can be used to display -/// a details UI component for a reaction with both sender -/// names and the date at which they sent a reaction. -#[derive(Clone, Debug, Eq, Hash, PartialEq)] -pub struct ReactionSenderData { +#[derive(Clone, Debug)] +pub(crate) struct PendingReaction { + /// The annotation used for the reaction. + pub key: String, /// Sender identifier. pub sender_id: OwnedUserId, /// Date at which the sender reacted. pub timestamp: MilliSecondsSinceUnixEpoch, } -#[derive(Clone, Debug)] -pub(crate) struct PendingReaction { - pub key: String, - pub sender_data: ReactionSenderData, -} - #[derive(Clone, Debug)] pub(crate) struct FullReactionKey { pub item: TimelineEventItemId,