diff --git a/bindings/matrix-sdk-ffi/src/timeline/mod.rs b/bindings/matrix-sdk-ffi/src/timeline/mod.rs index 4393df5d3..b67ec1e21 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 a3394bcbe..13bf6bf40 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 53021494c..34a8aae9f 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 114047eec..000000000 --- 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 add84ba0c..a17979b4a 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 ba5d0b180..37ed108e9 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 86b1c7bae..015742de8 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(); + let reaction_by_sender = 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" - ); - } - } - - // 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 5a929f034..a7c4d7b75 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 cb09ebeb8..90ff38618 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 5a684f988..000000000 --- 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 d5b0a38e4..7332b6975 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 ab23c28b3..e419f7dd1 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 95f20145e..cb8267c6b 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(