From 64cc5e9782cf9387d75ada0485d19e8c36b36d2c Mon Sep 17 00:00:00 2001 From: aringenbach Date: Thu, 29 Jun 2023 17:38:14 +0200 Subject: [PATCH 1/9] Introduce `ReactionSenderData` and `EventItemIdentifier` --- .../src/timeline/event_handler.rs | 59 ++++++++++------- .../src/timeline/event_item/content.rs | 18 +++--- .../src/timeline/event_item/mod.rs | 21 ++++++- crates/matrix-sdk-ui/src/timeline/inner.rs | 63 ++++++++++++------- .../matrix-sdk-ui/src/timeline/tests/mod.rs | 6 +- 5 files changed, 110 insertions(+), 57 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index 16fb67049d2..e256d39941a 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -43,9 +43,9 @@ use tracing::{debug, error, field::debug, info, instrument, trace, warn}; use super::{ event_item::{ - AnyOtherFullStateEventContent, BundledReactions, EventSendState, EventTimelineItemKind, - LocalEventTimelineItem, MemberProfileChange, OtherState, Profile, RemoteEventOrigin, - RemoteEventTimelineItem, RoomMembershipChange, Sticker, + AnyOtherFullStateEventContent, BundledReactions, EventItemIdentifier, EventSendState, + EventTimelineItemKind, LocalEventTimelineItem, MemberProfileChange, OtherState, Profile, + RemoteEventOrigin, RemoteEventTimelineItem, RoomMembershipChange, Sticker, }, find_read_marker, read_receipts::maybe_add_implicit_read_receipt, @@ -53,7 +53,7 @@ use super::{ ReactionGroup, TimelineDetails, TimelineInnerState, TimelineItem, TimelineItemContent, VirtualTimelineItem, DEFAULT_SANITIZER_MODE, }; -use crate::events::SyncTimelineEventWithoutContent; +use crate::{events::SyncTimelineEventWithoutContent, timeline::event_item::ReactionSenderData}; #[derive(Clone)] pub(super) enum Flow { @@ -210,10 +210,7 @@ pub(super) struct TimelineEventHandler<'a> { flow: Flow, items: &'a mut ObservableVector>, #[allow(clippy::type_complexity)] - reaction_map: &'a mut HashMap< - (Option, Option), - (OwnedUserId, Annotation), - >, + reaction_map: &'a mut HashMap, pending_reactions: &'a mut HashMap>, fully_read_event: &'a mut Option, event_should_update_fully_read_marker: &'a mut bool, @@ -421,9 +418,11 @@ impl<'a> TimelineEventHandler<'a> { fn handle_reaction(&mut self, c: ReactionEventContent) { let event_id: &EventId = &c.relates_to.event_id; let (reaction_id, old_txn_id) = match &self.flow { - Flow::Local { txn_id, .. } => ((Some(txn_id.clone()), None), None), + Flow::Local { txn_id, .. } => { + (EventItemIdentifier::TransactionId(txn_id.clone()), None) + } Flow::Remote { event_id, txn_id, .. } => { - ((None, Some(event_id.clone())), txn_id.as_ref()) + (EventItemIdentifier::EventId(event_id.clone()), txn_id.as_ref()) } }; @@ -443,15 +442,22 @@ impl<'a> TimelineEventHandler<'a> { let reaction_group = reactions.entry(c.relates_to.key.clone()).or_default(); if let Some(txn_id) = old_txn_id { + let id = EventItemIdentifier::TransactionId(txn_id.clone()); // Remove the local echo from the related event. - if reaction_group.0.remove(&(Some(txn_id.clone()), None)).is_none() { + if reaction_group.0.remove(&id).is_none() { warn!( "Received reaction with transaction ID, but didn't \ find matching reaction in the related event's reactions" ); } } - reaction_group.0.insert(reaction_id.clone(), self.meta.sender.clone()); + reaction_group.0.insert( + reaction_id.clone(), + ReactionSenderData { + sender_id: self.meta.sender.clone(), + timestamp: self.meta.timestamp, + }, + ); trace!("Adding reaction"); self.items.set( @@ -464,19 +470,20 @@ impl<'a> TimelineEventHandler<'a> { } } else { trace!("Timeline item not found, adding reaction to the pending list"); - let (None, Some(reaction_event_id)) = &reaction_id else { + let EventItemIdentifier::EventId(reaction_event_id) = reaction_id.clone() else { error!("Adding local reaction echo to event absent from the timeline"); return; }; let pending = self.pending_reactions.entry(event_id.to_owned()).or_default(); - pending.insert(reaction_event_id.clone()); + pending.insert(reaction_event_id); } if let Flow::Remote { txn_id: Some(txn_id), .. } = &self.flow { + let id = EventItemIdentifier::TransactionId(txn_id.clone()); // Remove the local echo from the reaction map. - if self.reaction_map.remove(&(Some(txn_id.clone()), None)).is_none() { + if self.reaction_map.remove(&id).is_none() { warn!( "Received reaction with transaction ID, but didn't \ find matching reaction in reaction_map" @@ -495,7 +502,8 @@ impl<'a> TimelineEventHandler<'a> { // Redacted redactions are no-ops (unfortunately) #[instrument(skip_all, fields(redacts_event_id = ?redacts))] fn handle_redaction(&mut self, redacts: OwnedEventId, _content: RoomRedactionEventContent) { - if let Some((_, rel)) = self.reaction_map.remove(&(None, Some(redacts.clone()))) { + let id = EventItemIdentifier::EventId(redacts.clone()); + if let Some((_, rel)) = self.reaction_map.remove(&id) { update_timeline_item!(self, &rel.event_id, "redaction", |event_item| { let Some(remote_event_item) = event_item.as_remote() else { error!("inconsistent state: redaction received on a non-remote event item"); @@ -510,7 +518,7 @@ impl<'a> TimelineEventHandler<'a> { }; let group = group_entry.get_mut(); - if group.0.remove(&(None, Some(redacts.clone()))).is_none() { + if group.0.remove(&id).is_none() { error!( "inconsistent state: reaction from reaction_map not in reaction list \ of timeline item" @@ -531,7 +539,7 @@ impl<'a> TimelineEventHandler<'a> { if self.result.items_updated == 0 { if let Some(reactions) = self.pending_reactions.get_mut(&rel.event_id) { - if !reactions.remove(&redacts) { + if !reactions.remove(&redacts.clone()) { error!( "inconsistent state: reaction from reaction_map not in reaction list \ of pending_reactions" @@ -577,7 +585,8 @@ impl<'a> TimelineEventHandler<'a> { redacts: OwnedTransactionId, _content: RoomRedactionEventContent, ) { - if let Some((_, rel)) = self.reaction_map.remove(&(Some(redacts.clone()), None)) { + let id = EventItemIdentifier::TransactionId(redacts); + if let Some((_, rel)) = self.reaction_map.remove(&id) { update_timeline_item!(self, &rel.event_id, "redaction", |event_item| { let Some(remote_event_item) = event_item.as_remote() else { error!("inconsistent state: redaction received on a non-remote event item"); @@ -590,7 +599,7 @@ impl<'a> TimelineEventHandler<'a> { }; let group = group_entry.get_mut(); - if group.0.remove(&(Some(redacts.clone()), None)).is_none() { + if group.0.remove(&id).is_none() { error!( "inconsistent state: reaction from reaction_map not in reaction list \ of timeline item" @@ -935,7 +944,7 @@ impl<'a> TimelineEventHandler<'a> { let mut bundled = IndexMap::new(); for reaction_event_id in reactions { - let reaction_id = (None, Some(reaction_event_id)); + let reaction_id = EventItemIdentifier::EventId(reaction_event_id); let Some((sender, annotation)) = self.reaction_map.get(&reaction_id) else { error!( "inconsistent state: reaction from pending_reactions not in reaction_map" @@ -945,7 +954,13 @@ impl<'a> TimelineEventHandler<'a> { let group: &mut ReactionGroup = bundled.entry(annotation.key.clone()).or_default(); - group.0.insert(reaction_id, sender.clone()); + group.0.insert( + reaction_id, + ReactionSenderData { + sender_id: sender.clone(), + timestamp: MilliSecondsSinceUnixEpoch::now(), + }, + ); } Some(bundled) diff --git a/crates/matrix-sdk-ui/src/timeline/event_item/content.rs b/crates/matrix-sdk-ui/src/timeline/event_item/content.rs index e9981a78c9f..2f35dfccbc3 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_item/content.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_item/content.rs @@ -59,7 +59,7 @@ use ruma::{ }; use tracing::{debug, error}; -use super::{EventTimelineItem, Profile, TimelineDetails}; +use super::{EventItemIdentifier, EventTimelineItem, Profile, ReactionSenderData, TimelineDetails}; use crate::timeline::{ traits::RoomDataProvider, Error as TimelineError, TimelineItem, DEFAULT_SANITIZER_MODE, }; @@ -397,7 +397,7 @@ pub type BundledReactions = IndexMap; // The long type after a long visibility specified trips up rustfmt currently. // This works around. Report: https://github.com/rust-lang/rustfmt/issues/5703 -type ReactionGroupInner = IndexMap<(Option, Option), OwnedUserId>; +type ReactionGroupInner = IndexMap; /// A group of reaction events on the same event with the same key. /// @@ -408,8 +408,8 @@ pub struct ReactionGroup(pub(in crate::timeline) ReactionGroupInner); impl ReactionGroup { /// The (deduplicated) senders of the reactions in this group. - pub fn senders(&self) -> impl Iterator { - self.values().unique().map(AsRef::as_ref) + pub fn senders(&self) -> impl Iterator { + self.values().unique_by(|v| &v.sender_id) } /// All reactions within this reaction group that were sent by the given @@ -421,13 +421,17 @@ impl ReactionGroup { &'a self, user_id: &'a UserId, ) -> impl Iterator, Option<&OwnedEventId>)> + 'a { - self.iter() - .filter_map(move |(k, v)| (*v == user_id).then_some((k.0.as_ref(), k.1.as_ref()))) + self.iter().filter_map(move |(k, v)| { + (v.sender_id == user_id).then_some(match k { + EventItemIdentifier::TransactionId(txn_id) => (Some(txn_id), None), + EventItemIdentifier::EventId(event_id) => (None, Some(event_id)), + }) + }) } } impl Deref for ReactionGroup { - type Target = IndexMap<(Option, Option), OwnedUserId>; + type Target = IndexMap; fn deref(&self) -> &Self::Target { &self.0 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 85ddf41b94d..2c6d044aec5 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs @@ -20,7 +20,8 @@ use once_cell::sync::Lazy; use ruma::{ events::{receipt::Receipt, room::message::MessageType, AnySyncTimelineEvent}, serde::Raw, - EventId, MilliSecondsSinceUnixEpoch, OwnedMxcUri, OwnedUserId, TransactionId, UserId, + EventId, MilliSecondsSinceUnixEpoch, OwnedEventId, OwnedMxcUri, OwnedTransactionId, + OwnedUserId, TransactionId, UserId, }; mod content; @@ -67,6 +68,24 @@ pub(super) enum EventTimelineItemKind { Remote(RemoteEventTimelineItem), } +/// A wrapper that can contain either a transaction id, or an event id. +#[derive(Clone, Debug, Eq, Hash, PartialEq)] +pub enum EventItemIdentifier { + TransactionId(OwnedTransactionId), + EventId(OwnedEventId), +} + +/// 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)] +pub struct ReactionSenderData { + /// Sender identifier. + pub sender_id: OwnedUserId, + /// Date at which the sender reacted. + pub timestamp: MilliSecondsSinceUnixEpoch, +} + impl EventTimelineItem { pub(super) fn new( sender: OwnedUserId, diff --git a/crates/matrix-sdk-ui/src/timeline/inner.rs b/crates/matrix-sdk-ui/src/timeline/inner.rs index 8f2bba1cf44..4486d2386a9 100644 --- a/crates/matrix-sdk-ui/src/timeline/inner.rs +++ b/crates/matrix-sdk-ui/src/timeline/inner.rs @@ -61,6 +61,7 @@ use super::{ update_read_marker, Flow, HandleEventResult, TimelineEventHandler, TimelineEventKind, TimelineEventMetadata, TimelineItemPosition, }, + event_item::{EventItemIdentifier, ReactionSenderData}, reactions::ReactionToggleResult, rfind_event_by_id, rfind_event_item, traits::RoomDataProvider, @@ -80,8 +81,7 @@ pub(super) struct TimelineInner { pub(super) struct TimelineInnerState { pub(super) items: ObservableVector>, /// Reaction event / txn ID => sender and reaction data. - pub(super) reaction_map: - HashMap<(Option, Option), (OwnedUserId, Annotation)>, + pub(super) reaction_map: HashMap, /// ID of event that is not in the timeline yet => List of reaction event /// IDs. pub(super) pending_reactions: HashMap>, @@ -230,12 +230,20 @@ impl TimelineInner

{ (to_redact_local, to_redact_remote) => { // The reaction exists, redact local echo and/or remote echo let no_reason = RoomRedactionEventContent::default(); + let to_redact = if let Some(to_redact_local) = to_redact_local { + EventItemIdentifier::TransactionId(to_redact_local.clone()) + } else if let Some(to_redact_remote) = to_redact_remote { + EventItemIdentifier::EventId(to_redact_remote.clone()) + } else { + error!("Transaction id and event id are both missing"); + return Err(super::Error::FailedToToggleReaction); + }; self.handle_local_redaction_internal( &mut state, sender, sender_profile, TransactionId::new(), - (to_redact_local.cloned(), to_redact_remote.cloned()), + to_redact, no_reason.clone(), ) .await; @@ -423,7 +431,7 @@ impl TimelineInner

{ pub(super) async fn handle_local_redaction( &self, txn_id: OwnedTransactionId, - to_redact: (Option, Option), + to_redact: EventItemIdentifier, content: RoomRedactionEventContent, ) { let sender = self.room_data_provider.own_user_id().to_owned(); @@ -444,7 +452,7 @@ impl TimelineInner

{ own_user_id: OwnedUserId, own_profile: Option, txn_id: OwnedTransactionId, - to_redact: (Option, Option), + to_redact: EventItemIdentifier, content: RoomRedactionEventContent, ) { let flow = Flow::Local { txn_id: txn_id.clone() }; @@ -460,23 +468,20 @@ impl TimelineInner

{ is_highlighted: false, }; - let (to_redact_local, to_redact_remote) = to_redact; - if let Some(redacts) = to_redact_local { - let kind = TimelineEventKind::LocalRedaction { redacts, content: content.clone() }; + match to_redact { + EventItemIdentifier::TransactionId(txn_id) => { + let kind = + TimelineEventKind::LocalRedaction { redacts: txn_id, content: content.clone() }; - TimelineEventHandler::new( - event_meta.clone(), - flow.clone(), - state, - self.track_read_receipts, - ) - .handle_event(kind); - } - if let Some(redacts) = to_redact_remote { - let kind = TimelineEventKind::Redaction { redacts, content }; + TimelineEventHandler::new(event_meta, flow, state, self.track_read_receipts) + .handle_event(kind); + } + EventItemIdentifier::EventId(event_id) => { + let kind = TimelineEventKind::Redaction { redacts: event_id, content }; - TimelineEventHandler::new(event_meta, flow, state, self.track_read_receipts) - .handle_event(kind); + TimelineEventHandler::new(event_meta, flow, state, self.track_read_receipts) + .handle_event(kind); + } } } @@ -1277,7 +1282,8 @@ fn update_timeline_reaction( // Remove the local echo from the related event. if let Some(txn_id) = local_echo_to_remove { - if reaction_group.0.remove(&(Some(txn_id.to_owned()), None)).is_none() { + let id = EventItemIdentifier::TransactionId(txn_id.clone()); + if reaction_group.0.remove(&id).is_none() { warn!( "Tried to remove reaction by transaction ID, but didn't \ find matching reaction in the related event's reactions" @@ -1287,7 +1293,15 @@ fn update_timeline_reaction( // Add the remote echo to the related event if let Some(event_id) = remote_echo_to_add { let own_user_id = own_user_id.to_owned(); - reaction_group.0.insert((None, Some(event_id.to_owned())), own_user_id); + reaction_group.0.insert( + EventItemIdentifier::EventId(event_id.clone()), + // Note: remote event is not synced yet, so we're adding an item + // with the local timestamp. + ReactionSenderData { + sender_id: own_user_id, + timestamp: MilliSecondsSinceUnixEpoch::now(), + }, + ); }; if reaction_group.0.is_empty() { reactions.remove(&annotation.key); @@ -1302,7 +1316,8 @@ fn update_timeline_reaction( // 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 { - if state.reaction_map.remove(&(Some(txn_id.to_owned()), None)).is_none() { + let id = EventItemIdentifier::TransactionId(txn_id.clone()); + if state.reaction_map.remove(&id).is_none() { warn!( "Tried to remove reaction by transaction ID, but didn't \ find matching reaction in the reaction map" @@ -1312,7 +1327,7 @@ fn update_timeline_reaction( // Add the remote echo to the reaction_map if let Some(event_id) = remote_echo_to_add { state.reaction_map.insert( - (None, Some(event_id.to_owned())), + EventItemIdentifier::EventId(event_id.clone()), (own_user_id.to_owned(), annotation.clone()), ); } diff --git a/crates/matrix-sdk-ui/src/timeline/tests/mod.rs b/crates/matrix-sdk-ui/src/timeline/tests/mod.rs index 673475ed8b5..ccce54f698f 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/mod.rs @@ -50,8 +50,8 @@ use ruma::{ use serde_json::{json, Value as JsonValue}; use super::{ - inner::ReactionAction, reactions::ReactionToggleResult, traits::RoomDataProvider, - EventTimelineItem, Profile, TimelineInner, TimelineItem, + event_item::EventItemIdentifier, inner::ReactionAction, reactions::ReactionToggleResult, + traits::RoomDataProvider, EventTimelineItem, Profile, TimelineInner, TimelineItem, }; mod basic; @@ -214,7 +214,7 @@ impl TestTimeline { async fn handle_local_redaction_event( &self, - redacts: (Option, Option), + redacts: EventItemIdentifier, content: RoomRedactionEventContent, ) -> OwnedTransactionId { let txn_id = TransactionId::new(); From 3144064d423f3496518ab6571bc01a98ba852b22 Mon Sep 17 00:00:00 2001 From: aringenbach Date: Thu, 29 Jun 2023 17:52:54 +0200 Subject: [PATCH 2/9] Fix current tests --- .../src/timeline/tests/reaction_group.rs | 39 +++++++++++++------ .../src/timeline/tests/reactions.rs | 5 ++- .../tests/integration/timeline/mod.rs | 2 +- 3 files changed, 33 insertions(+), 13 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/tests/reaction_group.rs b/crates/matrix-sdk-ui/src/timeline/tests/reaction_group.rs index 3f1e0a544b2..5688f210a52 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/reaction_group.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/reaction_group.rs @@ -12,9 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. -use ruma::{server_name, user_id, EventId, OwnedEventId, OwnedTransactionId, UserId}; +use assert_matches::assert_matches; +use ruma::{server_name, user_id, EventId, MilliSecondsSinceUnixEpoch, OwnedUserId, UserId}; use crate::timeline::{ + event_item::{EventItemIdentifier, ReactionSenderData}, tests::{ALICE, BOB}, ReactionGroup, }; @@ -28,13 +30,17 @@ fn by_sender() { let reaction_2 = new_reaction(); let mut reaction_group = ReactionGroup::default(); - reaction_group.0.insert(reaction_1.clone(), alice.clone()); - reaction_group.0.insert(reaction_2, bob); + 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.get(0).unwrap(); - assert_eq!(*reaction.1.unwrap(), reaction_1.1.unwrap()); + + assert_matches!( + reaction_1, + EventItemIdentifier::EventId(event_id) => { assert_eq!(reaction.1.unwrap(), &event_id) } + ) } #[test] @@ -57,9 +63,9 @@ fn by_sender_with_multiple_users() { let reaction_3 = new_reaction(); let mut reaction_group = ReactionGroup::default(); - reaction_group.0.insert(reaction_1, alice.clone()); - reaction_group.0.insert(reaction_2, alice.clone()); - reaction_group.0.insert(reaction_3, bob.clone()); + 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::>(); @@ -82,16 +88,27 @@ fn senders_are_deduplicated() { group }; - assert_eq!(group.senders().collect::>(), vec![&ALICE.to_owned(), &BOB.to_owned()]); + let senders = group.senders().map(|v| &v.sender_id).collect::>(); + assert_eq!(senders, vec![&ALICE.to_owned(), &BOB.to_owned()]); } fn insert(group: &mut ReactionGroup, sender: &UserId, count: u64) { for _ in 0..count { - group.0.insert(new_reaction(), sender.to_owned()); + group.0.insert( + new_reaction(), + ReactionSenderData { + sender_id: sender.to_owned(), + timestamp: MilliSecondsSinceUnixEpoch::now(), + }, + ); } } -fn new_reaction() -> (Option, Option) { +fn new_reaction() -> EventItemIdentifier { let event_id = EventId::new(server_name!("example.org")); - (None, Some(event_id)) + EventItemIdentifier::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 73a50fd0920..b9e1f5cfa11 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs @@ -25,6 +25,7 @@ use ruma::{ use stream_assert::assert_next_matches; use crate::timeline::{ + event_item::EventItemIdentifier, inner::ReactionAction, reactions::ReactionToggleResult, tests::{assert_event_is_updated, assert_no_more_updates, TestTimeline, ALICE, BOB}, @@ -143,7 +144,9 @@ async fn redact_reaction_from_non_existent_event() { let mut stream = timeline.subscribe().await; let reaction_id = EventId::new(server_name!("example.org")); // non existent event - timeline.handle_local_redaction_event((None, Some(reaction_id)), Default::default()).await; + timeline + .handle_local_redaction_event(EventItemIdentifier::EventId(reaction_id), Default::default()) + .await; assert_no_more_updates(&mut stream).await; } diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs b/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs index 1be48400500..57c15d3b5f4 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs @@ -223,7 +223,7 @@ async fn reaction() { assert_eq!(event_item.reactions().len(), 1); let group = &event_item.reactions()["👍"]; assert_eq!(group.len(), 1); - let senders: Vec<_> = group.senders().collect(); + let senders: Vec<_> = group.senders().map(|v| &v.sender_id).collect(); assert_eq!(senders.as_slice(), [user_id!("@bob:example.org")]); // TODO: After adding raw timeline items, check for one here From 4d833c2374901364e5531eb14d51d8e5899b2e4d Mon Sep 17 00:00:00 2001 From: aringenbach Date: Thu, 29 Jun 2023 17:59:29 +0200 Subject: [PATCH 3/9] Remove now unnecessary clippy exception and `ReactionGroupInner` --- crates/matrix-sdk-ui/src/timeline/event_handler.rs | 1 - crates/matrix-sdk-ui/src/timeline/event_item/content.rs | 7 +------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index e256d39941a..1ebf6846475 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -209,7 +209,6 @@ pub(super) struct TimelineEventHandler<'a> { meta: TimelineEventMetadata, flow: Flow, items: &'a mut ObservableVector>, - #[allow(clippy::type_complexity)] reaction_map: &'a mut HashMap, pending_reactions: &'a mut HashMap>, fully_read_event: &'a mut Option, diff --git a/crates/matrix-sdk-ui/src/timeline/event_item/content.rs b/crates/matrix-sdk-ui/src/timeline/event_item/content.rs index 2f35dfccbc3..d4941b567eb 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_item/content.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_item/content.rs @@ -394,17 +394,12 @@ impl From for EncryptedMessage { /// Key: The reaction, usually an emoji.\ /// Value: The group of reactions. pub type BundledReactions = IndexMap; - -// The long type after a long visibility specified trips up rustfmt currently. -// This works around. Report: https://github.com/rust-lang/rustfmt/issues/5703 -type ReactionGroupInner = 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) ReactionGroupInner); +pub struct ReactionGroup(pub(in crate::timeline) IndexMap); impl ReactionGroup { /// The (deduplicated) senders of the reactions in this group. From 86ee57e3752dd55f618bf61d8527b7f8c699876e Mon Sep 17 00:00:00 2001 From: aringenbach Date: Thu, 29 Jun 2023 18:02:43 +0200 Subject: [PATCH 4/9] Expose `ReactionSenderData` to FFI --- bindings/matrix-sdk-ffi/src/timeline.rs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/timeline.rs b/bindings/matrix-sdk-ffi/src/timeline.rs index 65c82f55031..9b00f174d34 100644 --- a/bindings/matrix-sdk-ffi/src/timeline.rs +++ b/bindings/matrix-sdk-ffi/src/timeline.rs @@ -326,7 +326,13 @@ impl EventTimelineItem { .map(|(k, v)| Reaction { key: k.to_owned(), count: v.len().try_into().unwrap(), - senders: v.senders().map(ToString::to_string).collect(), + senders: v + .senders() + .map(|v| ReactionSenderData { + sender_id: v.sender_id.to_string(), + timestamp: v.timestamp.0.into(), + }) + .collect(), }) .collect() } @@ -1080,13 +1086,13 @@ impl EncryptedMessage { pub struct Reaction { pub key: String, pub count: u64, - pub senders: Vec, + pub senders: Vec, } -#[derive(Clone)] -pub struct ReactionDetails { - pub id: String, - pub sender: String, +#[derive(Clone, uniffi::Record)] +pub struct ReactionSenderData { + pub sender_id: String, + pub timestamp: u64, } /// A [`TimelineItem`](super::TimelineItem) that doesn't correspond to an event. From caafaaa721633cc4cc46e3a5007631b623c6f86d Mon Sep 17 00:00:00 2001 From: aringenbach Date: Mon, 3 Jul 2023 16:50:33 +0200 Subject: [PATCH 5/9] Add timestamp tests --- .../src/timeline/tests/reaction_group.rs | 24 ++++++++++- .../src/timeline/tests/reactions.rs | 43 ++++++++++++++++++- .../src/tests/reactions.rs | 8 +++- 3 files changed, 71 insertions(+), 4 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/tests/reaction_group.rs b/crates/matrix-sdk-ui/src/timeline/tests/reaction_group.rs index 5688f210a52..544d4aed155 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/reaction_group.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/reaction_group.rs @@ -13,7 +13,8 @@ // limitations under the License. use assert_matches::assert_matches; -use ruma::{server_name, user_id, EventId, MilliSecondsSinceUnixEpoch, OwnedUserId, UserId}; +use itertools::Itertools; +use ruma::{server_name, uint, user_id, EventId, MilliSecondsSinceUnixEpoch, OwnedUserId, UserId}; use crate::timeline::{ event_item::{EventItemIdentifier, ReactionSenderData}, @@ -92,6 +93,27 @@ fn senders_are_deduplicated() { assert_eq!(senders, vec![&ALICE.to_owned(), &BOB.to_owned()]); } +#[test] +fn 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( diff --git a/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs b/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs index b9e1f5cfa11..6cdc7ca5e1d 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::sync::Arc; +use std::{ops::RangeInclusive, sync::Arc}; use assert_matches::assert_matches; use eyeball_im::VectorDiff; @@ -20,7 +20,7 @@ use futures_core::Stream; use matrix_sdk_test::async_test; use ruma::{ events::{relation::Annotation, room::message::RoomMessageEventContent}, - server_name, EventId, OwnedEventId, TransactionId, + server_name, EventId, MilliSecondsSinceUnixEpoch, OwnedEventId, TransactionId, }; use stream_assert::assert_next_matches; @@ -202,6 +202,39 @@ async fn toggle_during_request_resolves_new_action() { assert_no_more_updates(&mut stream).await; } +#[async_test] +async fn reactions_store_timestamp() { + let timeline = TestTimeline::new(); + let mut stream = timeline.subscribe().await; + let (msg_id, msg_pos) = send_first_message(&timeline, &mut stream).await; + let reaction = create_reaction(&msg_id); + + // Creating a reaction adds a valid timestamp. + let timestamp_before = MilliSecondsSinceUnixEpoch::now(); + 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; + assert!(timestamp_range_until_now_from(timestamp_before).contains(×tamp)); + + // Failing a redaction. + let _ = timeline.toggle_reaction_local(&reaction).await.unwrap(); + let _ = assert_event_is_updated(&mut stream, &msg_id, msg_pos).await; + timeline + .handle_reaction_response( + &reaction, + &ReactionToggleResult::RedactFailure { event_id: msg_id.clone() }, + ) + .await + .unwrap(); + + // 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; + assert!(timestamp_range_until_now_from(timestamp_before).contains(&new_timestamp)); +} + fn create_reaction(related_message_id: &EventId) -> Annotation { let reaction_key = REACTION_KEY.to_owned(); let msg_id = related_message_id.to_owned(); @@ -263,3 +296,9 @@ async fn assert_reactions_are_removed( let reactions = event.reactions().get(&REACTION_KEY.to_owned()); assert!(reactions.is_none()); } + +fn timestamp_range_until_now_from( + timestamp: MilliSecondsSinceUnixEpoch, +) -> RangeInclusive { + timestamp..=MilliSecondsSinceUnixEpoch::now() +} diff --git a/testing/matrix-sdk-integration-testing/src/tests/reactions.rs b/testing/matrix-sdk-integration-testing/src/tests/reactions.rs index e496d7ccc66..bdb346b7207 100644 --- a/testing/matrix-sdk-integration-testing/src/tests/reactions.rs +++ b/testing/matrix-sdk-integration-testing/src/tests/reactions.rs @@ -25,7 +25,7 @@ use matrix_sdk::{ ruma::{ api::client::room::create_room::v3::Request as CreateRoomRequest, events::{relation::Annotation, room::message::RoomMessageEventContent}, - EventId, RoomId, UserId, + EventId, MilliSecondsSinceUnixEpoch, RoomId, UserId, }, Client, LoopCtrl, }; @@ -178,6 +178,12 @@ async fn assert_remote_added( let (reaction_tx_id, reaction_event_id) = reaction; assert_matches!(reaction_tx_id, None); assert_matches!(reaction_event_id, Some(value) => value); + + // 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()); } async fn sync_room(client: &Client, room_id: &RoomId) -> Result<()> { From 1cee900a61926070f1e9186eae3c978f12062da1 Mon Sep 17 00:00:00 2001 From: aringenbach Date: Thu, 6 Jul 2023 16:20:57 +0200 Subject: [PATCH 6/9] Fix timestamp being set to now instead of expected value --- .../src/timeline/event_handler.rs | 18 ++++++++---------- crates/matrix-sdk-ui/src/timeline/inner.rs | 18 +++++++++--------- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index 1ebf6846475..843be1ed942 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -209,7 +209,7 @@ pub(super) struct TimelineEventHandler<'a> { meta: TimelineEventMetadata, flow: Flow, items: &'a mut ObservableVector>, - reaction_map: &'a mut HashMap, + reaction_map: &'a mut HashMap, pending_reactions: &'a mut HashMap>, fully_read_event: &'a mut Option, event_should_update_fully_read_marker: &'a mut bool, @@ -489,7 +489,11 @@ impl<'a> TimelineEventHandler<'a> { ); } } - self.reaction_map.insert(reaction_id, (self.meta.sender.clone(), c.relates_to)); + let reaction_sender_data = ReactionSenderData { + sender_id: self.meta.sender.clone(), + timestamp: self.meta.timestamp, + }; + self.reaction_map.insert(reaction_id, (reaction_sender_data, c.relates_to)); } #[instrument(skip_all)] @@ -944,7 +948,7 @@ impl<'a> TimelineEventHandler<'a> { for reaction_event_id in reactions { let reaction_id = EventItemIdentifier::EventId(reaction_event_id); - let Some((sender, annotation)) = self.reaction_map.get(&reaction_id) else { + let Some((reaction_sender_data, annotation)) = self.reaction_map.get(&reaction_id) else { error!( "inconsistent state: reaction from pending_reactions not in reaction_map" ); @@ -953,13 +957,7 @@ impl<'a> TimelineEventHandler<'a> { let group: &mut ReactionGroup = bundled.entry(annotation.key.clone()).or_default(); - group.0.insert( - reaction_id, - ReactionSenderData { - sender_id: sender.clone(), - timestamp: MilliSecondsSinceUnixEpoch::now(), - }, - ); + group.0.insert(reaction_id, reaction_sender_data.clone()); } Some(bundled) diff --git a/crates/matrix-sdk-ui/src/timeline/inner.rs b/crates/matrix-sdk-ui/src/timeline/inner.rs index 4486d2386a9..85fe627e686 100644 --- a/crates/matrix-sdk-ui/src/timeline/inner.rs +++ b/crates/matrix-sdk-ui/src/timeline/inner.rs @@ -81,7 +81,7 @@ pub(super) struct TimelineInner { pub(super) struct TimelineInnerState { pub(super) items: ObservableVector>, /// Reaction event / txn ID => sender and reaction data. - pub(super) reaction_map: HashMap, + pub(super) reaction_map: HashMap, /// ID of event that is not in the timeline yet => List of reaction event /// IDs. pub(super) pending_reactions: HashMap>, @@ -1275,6 +1275,12 @@ fn update_timeline_reaction( error!("inconsistent state: reaction received on a non-remote event item"); return Err(super::Error::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 new_reactions = { let mut reactions = remote_related.reactions.clone(); @@ -1292,15 +1298,9 @@ fn update_timeline_reaction( } // Add the remote echo to the related event if let Some(event_id) = remote_echo_to_add { - let own_user_id = own_user_id.to_owned(); reaction_group.0.insert( EventItemIdentifier::EventId(event_id.clone()), - // Note: remote event is not synced yet, so we're adding an item - // with the local timestamp. - ReactionSenderData { - sender_id: own_user_id, - timestamp: MilliSecondsSinceUnixEpoch::now(), - }, + reaction_sender_data.clone(), ); }; if reaction_group.0.is_empty() { @@ -1328,7 +1328,7 @@ fn update_timeline_reaction( if let Some(event_id) = remote_echo_to_add { state.reaction_map.insert( EventItemIdentifier::EventId(event_id.clone()), - (own_user_id.to_owned(), annotation.clone()), + (reaction_sender_data, annotation.clone()), ); } } From 38a60e108ad2aaa7c221cc8b406aa77575f63cf7 Mon Sep 17 00:00:00 2001 From: aringenbach Date: Thu, 6 Jul 2023 18:18:08 +0200 Subject: [PATCH 7/9] Fix formatting --- crates/matrix-sdk-ui/src/timeline/event_handler.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index 15e7bfe18e3..812428bfb45 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -937,7 +937,9 @@ impl<'a> TimelineEventHandler<'a> { for reaction_event_id in reactions { let reaction_id = EventItemIdentifier::EventId(reaction_event_id); - let Some((reaction_sender_data, annotation)) = self.reaction_map.get(&reaction_id) else { + let Some((reaction_sender_data, annotation)) = + self.reaction_map.get(&reaction_id) + else { error!( "inconsistent state: reaction from pending_reactions not in reaction_map" ); From 6c835d0ec7bd6d671153870601076ee0b8c0cfd7 Mon Sep 17 00:00:00 2001 From: aringenbach Date: Fri, 7 Jul 2023 16:54:10 +0200 Subject: [PATCH 8/9] Add test covering timestamps for timeline initial reactions --- .../matrix-sdk-ui/src/timeline/tests/mod.rs | 32 +++++++++++++++- .../src/timeline/tests/reactions.rs | 38 ++++++++++++++++++- 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/tests/mod.rs b/crates/matrix-sdk-ui/src/timeline/tests/mod.rs index 35a2b63accc..5c0f781189f 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/mod.rs @@ -261,11 +261,20 @@ impl TestTimeline { &self, sender: &UserId, content: C, + ) -> JsonValue { + self.make_message_event_with_id(sender, content, EventId::new(server_name!("dummy.server"))) + } + + fn make_message_event_with_id( + &self, + sender: &UserId, + content: C, + event_id: OwnedEventId, ) -> JsonValue { json!({ "type": content.event_type(), "content": content, - "event_id": EventId::new(server_name!("dummy.server")), + "event_id": event_id, "sender": sender, "origin_server_ts": self.next_server_ts(), }) @@ -339,6 +348,27 @@ impl TestTimeline { }) } + fn make_reaction( + &self, + sender: &UserId, + annotation: &Annotation, + timestamp: MilliSecondsSinceUnixEpoch, + ) -> JsonValue { + json!({ + "event_id": EventId::new(server_name!("dummy.server")), + "content": { + "m.relates_to": { + "event_id": annotation.event_id, + "key": annotation.key, + "rel_type": "m.annotation" + } + }, + "sender": sender, + "type": "m.reaction", + "origin_server_ts": timestamp + }) + } + fn make_receipt_event_content( &self, receipts: impl IntoIterator, diff --git a/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs b/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs index 6cdc7ca5e1d..d94a03b5740 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs @@ -17,10 +17,11 @@ use std::{ops::RangeInclusive, sync::Arc}; use assert_matches::assert_matches; use eyeball_im::VectorDiff; use futures_core::Stream; +use imbl::vector; use matrix_sdk_test::async_test; use ruma::{ events::{relation::Annotation, room::message::RoomMessageEventContent}, - server_name, EventId, MilliSecondsSinceUnixEpoch, OwnedEventId, TransactionId, + server_name, uint, EventId, MilliSecondsSinceUnixEpoch, OwnedEventId, TransactionId, }; use stream_assert::assert_next_matches; @@ -28,7 +29,10 @@ use crate::timeline::{ event_item::EventItemIdentifier, inner::ReactionAction, reactions::ReactionToggleResult, - tests::{assert_event_is_updated, assert_no_more_updates, TestTimeline, ALICE, BOB}, + tests::{ + assert_event_is_updated, assert_no_more_updates, sync_timeline_event, TestTimeline, ALICE, + BOB, + }, TimelineItem, }; @@ -235,6 +239,36 @@ async fn reactions_store_timestamp() { assert!(timestamp_range_until_now_from(timestamp_before).contains(&new_timestamp)); } +#[async_test] +async fn initial_reaction_timestamps_is_stored() { + let mut timeline = TestTimeline::new(); + + let message_event_id = EventId::new(server_name!("dummy.server")); + let reaction_timestamp = MilliSecondsSinceUnixEpoch(uint!(39845)); + + timeline + .inner + .add_initial_events(vector![ + sync_timeline_event(timeline.make_reaction( + *ALICE, + &Annotation::new(message_event_id.clone(), REACTION_KEY.to_owned()), + reaction_timestamp + )), + sync_timeline_event(timeline.make_message_event_with_id( + *ALICE, + RoomMessageEventContent::text_plain("A"), + message_event_id + )) + ]) + .await; + + let items = timeline.inner.items().await; + 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); +} + fn create_reaction(related_message_id: &EventId) -> Annotation { let reaction_key = REACTION_KEY.to_owned(); let msg_id = related_message_id.to_owned(); From 75cd56578e9752062bf0bd06686d95b38b8b8c00 Mon Sep 17 00:00:00 2001 From: aringenbach <80891108+aringenbach@users.noreply.github.com> Date: Fri, 7 Jul 2023 18:16:12 +0200 Subject: [PATCH 9/9] Update crates/matrix-sdk-ui/src/timeline/tests/reactions.rs --- crates/matrix-sdk-ui/src/timeline/tests/reactions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs b/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs index d94a03b5740..f9efb1c3959 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/reactions.rs @@ -240,7 +240,7 @@ async fn reactions_store_timestamp() { } #[async_test] -async fn initial_reaction_timestamps_is_stored() { +async fn initial_reaction_timestamp_is_stored() { let mut timeline = TestTimeline::new(); let message_event_id = EventId::new(server_name!("dummy.server"));