From e5f021f5cf84af93e47e43642fa1fa4c7f286f59 Mon Sep 17 00:00:00 2001 From: Jonas Platte Date: Fri, 28 Jul 2023 12:13:03 +0200 Subject: [PATCH] ui: Simplify definition of TimelineEventHandler Take a mutable reference to TimelineInnerState, instead of individual mutable references to its fields. --- .../src/timeline/event_handler.rs | 160 +++++++++--------- 1 file changed, 76 insertions(+), 84 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index 5e1a09aab..49a048fa1 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::{collections::HashMap, sync::Arc}; +use std::sync::Arc; use chrono::{Datelike, Local, TimeZone}; use eyeball_im::{ObservableVector, ObservableVectorEntry}; @@ -21,7 +21,7 @@ use matrix_sdk::deserialized_responses::EncryptionInfo; use ruma::{ events::{ reaction::ReactionEventContent, - receipt::{Receipt, ReceiptType}, + receipt::Receipt, relation::Replacement, room::{ encrypted::RoomEncryptedEventContent, @@ -40,7 +40,6 @@ use ruma::{ }, serde::Raw, EventId, MilliSecondsSinceUnixEpoch, OwnedEventId, OwnedTransactionId, OwnedUserId, - RoomVersionId, }; use tracing::{debug, error, field::debug, info, instrument, trace, warn}; @@ -52,7 +51,6 @@ use super::{ }, find_read_marker, item::{new_timeline_item, timeline_item}, - reactions::Reactions, read_receipts::maybe_add_implicit_read_receipt, rfind_event_by_id, rfind_event_item, EventTimelineItem, Message, OtherState, ReactionGroup, ReactionSenderData, Sticker, TimelineDetails, TimelineInnerState, TimelineItem, @@ -213,16 +211,9 @@ pub(super) struct HandleEventResult { // timeline item, transforming that item or creating a new one, updating the // reactive Vec). pub(super) struct TimelineEventHandler<'a> { + state: &'a mut TimelineInnerState, ctx: TimelineEventContext, - items: &'a mut ObservableVector>, - next_internal_id: &'a mut u64, - reactions: &'a mut Reactions, - fully_read_event: &'a mut Option, - event_should_update_fully_read_marker: &'a mut bool, track_read_receipts: bool, - users_read_receipts: - &'a mut HashMap>, - room_version: &'a RoomVersionId, result: HandleEventResult, } @@ -232,7 +223,7 @@ pub(super) struct TimelineEventHandler<'a> { macro_rules! update_timeline_item { ($this:ident, $event_id:expr, $action:expr, $update:expr) => { _update_timeline_item( - &mut *$this.items, + &mut $this.state.items, &mut $this.result.items_updated, $event_id, $action, @@ -247,18 +238,7 @@ impl<'a> TimelineEventHandler<'a> { ctx: TimelineEventContext, track_read_receipts: bool, ) -> Self { - Self { - ctx, - items: &mut state.items, - next_internal_id: &mut state.next_internal_id, - reactions: &mut state.reactions, - fully_read_event: &mut state.fully_read_event, - event_should_update_fully_read_marker: &mut state.event_should_update_fully_read_marker, - track_read_receipts, - users_read_receipts: &mut state.users_read_receipts, - room_version: &state.room_version, - result: HandleEventResult::default(), - } + Self { state, ctx, track_read_receipts, result: HandleEventResult::default() } } /// Handle an event. @@ -301,7 +281,10 @@ impl<'a> TimelineEventHandler<'a> { self.handle_room_message_edit(re); } AnyMessageLikeEventContent::RoomMessage(c) => { - self.add(should_add, TimelineItemContent::message(c, relations, self.items)); + self.add( + should_add, + TimelineItemContent::message(c, relations, &self.state.items), + ); } AnyMessageLikeEventContent::RoomEncrypted(c) => self.handle_room_encrypted(c), AnyMessageLikeEventContent::Sticker(content) => { @@ -364,7 +347,7 @@ impl<'a> TimelineEventHandler<'a> { // If add was not called, that means the UTD event is one that // wouldn't normally be visible. Remove it. trace!("Removing UTD that was successfully retried"); - self.items.remove(idx); + self.state.items.remove(idx); self.result.item_removed = true; } @@ -448,7 +431,7 @@ impl<'a> TimelineEventHandler<'a> { } }; - 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.state.items, event_id) { let Some(remote_event_item) = event_item.as_remote() else { error!("inconsistent state: reaction received on a non-remote event item"); return; @@ -482,7 +465,7 @@ impl<'a> TimelineEventHandler<'a> { ); trace!("Adding reaction"); - self.items.set( + self.state.items.set( idx, event_item.with_inner_kind(remote_event_item.with_reactions(reactions)), ); @@ -495,7 +478,7 @@ impl<'a> TimelineEventHandler<'a> { return; }; - let pending = self.reactions.pending.entry(event_id.to_owned()).or_default(); + let pending = self.state.reactions.pending.entry(event_id.to_owned()).or_default(); pending.insert(reaction_event_id); } @@ -503,7 +486,7 @@ impl<'a> TimelineEventHandler<'a> { if let Flow::Remote { txn_id: Some(txn_id), .. } = &self.ctx.flow { let id = EventItemIdentifier::TransactionId(txn_id.clone()); // Remove the local echo from the reaction map. - if self.reactions.map.remove(&id).is_none() { + if self.state.reactions.map.remove(&id).is_none() { warn!( "Received reaction with transaction ID, but didn't \ find matching reaction in reaction_map" @@ -514,7 +497,7 @@ impl<'a> TimelineEventHandler<'a> { sender_id: self.ctx.sender.clone(), timestamp: self.ctx.timestamp, }; - self.reactions.map.insert(reaction_id, (reaction_sender_data, c.relates_to)); + self.state.reactions.map.insert(reaction_id, (reaction_sender_data, c.relates_to)); } #[instrument(skip_all)] @@ -527,7 +510,7 @@ impl<'a> TimelineEventHandler<'a> { #[instrument(skip_all, fields(redacts_event_id = ?redacts))] fn handle_redaction(&mut self, redacts: OwnedEventId, _content: RoomRedactionEventContent) { let id = EventItemIdentifier::EventId(redacts.clone()); - if let Some((_, rel)) = self.reactions.map.remove(&id) { + if let Some((_, rel)) = self.state.reactions.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"); @@ -562,7 +545,7 @@ impl<'a> TimelineEventHandler<'a> { }); if self.result.items_updated == 0 { - if let Some(reactions) = self.reactions.pending.get_mut(&rel.event_id) { + if let Some(reactions) = self.state.reactions.pending.get_mut(&rel.event_id) { if !reactions.remove(&redacts.clone()) { error!( "inconsistent state: reaction from reaction_map not in reaction list \ @@ -589,7 +572,7 @@ impl<'a> TimelineEventHandler<'a> { return None; } - Some(event_item.redact(self.room_version)) + Some(event_item.redact(&self.state.room_version)) }); if self.result.items_updated == 0 { @@ -597,13 +580,13 @@ impl<'a> TimelineEventHandler<'a> { debug!("redaction affected no event"); } - self.items.for_each(|mut entry| { + self.state.items.for_each(|mut entry| { let Some(event_item) = entry.as_event() else { return }; 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 { - let replied_to_event = replied_to_event.redact(self.room_version); + let replied_to_event = replied_to_event.redact(&self.state.room_version); let in_reply_to = InReplyToDetails { event_id: in_reply_to.event_id.clone(), event: TimelineDetails::Ready(Box::new(replied_to_event)), @@ -624,7 +607,7 @@ impl<'a> TimelineEventHandler<'a> { _content: RoomRedactionEventContent, ) { let id = EventItemIdentifier::TransactionId(redacts); - if let Some((_, rel)) = self.reactions.map.remove(&id) { + if let Some((_, rel)) = self.state.reactions.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"); @@ -694,7 +677,7 @@ impl<'a> TimelineEventHandler<'a> { TimelineItemPosition::End { from_cache: true } => RemoteEventOrigin::Cache, TimelineItemPosition::End { from_cache: false } => RemoteEventOrigin::Sync, #[cfg(feature = "e2e-encryption")] - TimelineItemPosition::Update(idx) => self.items[*idx] + TimelineItemPosition::Update(idx) => self.state.items[*idx] .as_event() .and_then(|ev| Some(ev.as_remote()?.origin)) .unwrap_or_else(|| { @@ -725,32 +708,36 @@ impl<'a> TimelineEventHandler<'a> { trace!("Adding new local timeline item"); // Check if the latest event has the same date as this event. - if let Some(latest_event) = self.items.iter().rev().find_map(|item| item.as_event()) + if let Some(latest_event) = + self.state.items.iter().rev().find_map(|item| item.as_event()) { let old_ts = latest_event.timestamp(); if let Some(day_divider_item) = maybe_create_day_divider_from_timestamps( old_ts, timestamp, - self.next_internal_id, + &mut self.state.next_internal_id, ) { trace!("Adding day divider (local)"); - self.items.push_back(day_divider_item); + self.state.items.push_back(day_divider_item); } } else { // If there is no event item, there is no day divider yet. trace!("Adding first day divider (local)"); - self.items.push_back(new_timeline_item( + self.state.items.push_back(new_timeline_item( VirtualTimelineItem::DayDivider(timestamp), - self.next_internal_id, + &mut self.state.next_internal_id, )); } - self.items.push_back(new_timeline_item(item, self.next_internal_id)); + self.state + .items + .push_back(new_timeline_item(item, &mut self.state.next_internal_id)); } Flow::Remote { position: TimelineItemPosition::Start, event_id, .. } => { if self + .state .items .iter() .filter_map(|ev| ev.as_event()?.event_id()) @@ -764,20 +751,20 @@ impl<'a> TimelineEventHandler<'a> { // Check if the earliest day divider has the same date as this event. if let Some(VirtualTimelineItem::DayDivider(divider_ts)) = - self.items.front().and_then(|item| item.as_virtual()) + self.state.items.front().and_then(|item| item.as_virtual()) { if let Some(day_divider_item) = maybe_create_day_divider_from_timestamps( *divider_ts, timestamp, - self.next_internal_id, + &mut self.state.next_internal_id, ) { - self.items.push_front(day_divider_item); + self.state.items.push_front(day_divider_item); } } else { // The list must always start with a day divider. - self.items.push_front(new_timeline_item( + self.state.items.push_front(new_timeline_item( VirtualTimelineItem::DayDivider(timestamp), - self.next_internal_id, + &mut self.state.next_internal_id, )); } @@ -786,18 +773,20 @@ impl<'a> TimelineEventHandler<'a> { 0, &mut item, self.ctx.is_own_event, - self.items, - self.users_read_receipts, + &mut self.state.items, + &mut self.state.users_read_receipts, ); } - self.items.insert(1, new_timeline_item(item, self.next_internal_id)); + self.state + .items + .insert(1, new_timeline_item(item, &mut self.state.next_internal_id)); } Flow::Remote { position: TimelineItemPosition::End { .. }, txn_id, event_id, .. } => { - let result = rfind_event_item(self.items, |it| { + let result = rfind_event_item(&self.state.items, |it| { txn_id.is_some() && it.transaction_id() == txn_id.as_deref() || it.event_id() == Some(event_id) }); @@ -813,7 +802,7 @@ impl<'a> TimelineEventHandler<'a> { if old_item.content.is_redacted() && !item.content.is_redacted() { warn!("Got original form of an event that was previously redacted"); - item.content = item.content.redact(self.room_version); + item.content = item.content.redact(&self.state.room_version); item.as_remote_mut() .expect("Can't have a local item when flow == Remote") .reactions @@ -835,7 +824,7 @@ impl<'a> TimelineEventHandler<'a> { let old_item_id = old_item.internal_id; - if idx == self.items.len() - 1 + if idx == self.state.items.len() - 1 && timestamp_to_date(old_item.timestamp()) == timestamp_to_date(timestamp) { // If the old item is the last one and no day divider @@ -846,20 +835,20 @@ impl<'a> TimelineEventHandler<'a> { idx, &mut item, self.ctx.is_own_event, - self.items, - self.users_read_receipts, + &mut self.state.items, + &mut self.state.users_read_receipts, ); } trace!(idx, "Replacing existing event"); - self.items.set(idx, timeline_item(item, old_item_id)); + self.state.items.set(idx, timeline_item(item, old_item_id)); return; } // In more complex cases, remove the item and day // divider (if necessary) before re-adding the item. trace!("Removing local echo or duplicate timeline item"); - removed_event_item_id = Some(self.items.remove(idx).internal_id); + removed_event_item_id = Some(self.state.items.remove(idx).internal_id); assert_ne!( idx, 0, @@ -869,16 +858,16 @@ impl<'a> TimelineEventHandler<'a> { // Pre-requisites for removing the day divider: // 1. there is one preceding the old item at all - if self.items[idx - 1].is_day_divider() + if self.state.items[idx - 1].is_day_divider() // 2. the item after the old one that was removed is virtual (it should be // impossible for this to be a read marker) - && self + && self.state .items .get(idx) .map_or(true, |item| item.is_virtual()) { trace!("Removing day divider"); - removed_day_divider_id = Some(self.items.remove(idx - 1).internal_id); + removed_day_divider_id = Some(self.state.items.remove(idx - 1).internal_id); } // no return here, below code for adding a new event @@ -893,6 +882,7 @@ impl<'a> TimelineEventHandler<'a> { // Local echoes that are pending should stick to the bottom, // find the latest event that isn't that. let mut latest_event_stream = self + .state .items .iter() .enumerate() @@ -918,7 +908,7 @@ impl<'a> TimelineEventHandler<'a> { let mut insert_idx = latest_nonfailed_event_idx.map_or(0, |idx| idx + 1); // Keep push semantics, if we're inserting at the end. - let should_push = insert_idx == self.items.len(); + let should_push = insert_idx == self.state.items.len(); if let Some(latest_event) = latest_event { // Check if that event has the same date as the new one. @@ -932,8 +922,8 @@ impl<'a> TimelineEventHandler<'a> { // now need to add a new one, reuse the previous one's ID. Some(day_divider_id) => day_divider_id, None => { - let internal_id = *self.next_internal_id; - *self.next_internal_id += 1; + let internal_id = self.state.next_internal_id; + self.state.next_internal_id += 1; internal_id } }; @@ -942,9 +932,9 @@ impl<'a> TimelineEventHandler<'a> { timeline_item(VirtualTimelineItem::DayDivider(timestamp), id); if should_push { - self.items.push_back(day_divider_item); + self.state.items.push_back(day_divider_item); } else { - self.items.insert(insert_idx, day_divider_item); + self.state.items.insert(insert_idx, day_divider_item); insert_idx += 1; } } @@ -953,12 +943,12 @@ impl<'a> TimelineEventHandler<'a> { trace!("Adding first day divider (remote)"); let new_day_divider = new_timeline_item( VirtualTimelineItem::DayDivider(timestamp), - self.next_internal_id, + &mut self.state.next_internal_id, ); if should_push { - self.items.push_back(new_day_divider); + self.state.items.push_back(new_day_divider); } else { - self.items.insert(insert_idx, new_day_divider); + self.state.items.insert(insert_idx, new_day_divider); insert_idx += 1; } } @@ -968,8 +958,8 @@ impl<'a> TimelineEventHandler<'a> { insert_idx, &mut item, self.ctx.is_own_event, - self.items, - self.users_read_receipts, + &mut self.state.items, + &mut self.state.users_read_receipts, ); } @@ -979,8 +969,8 @@ impl<'a> TimelineEventHandler<'a> { // the previous item's ID. Some(id) => id, None => { - let internal_id = *self.next_internal_id; - *self.next_internal_id += 1; + let internal_id = self.state.next_internal_id; + self.state.next_internal_id += 1; internal_id } }; @@ -988,25 +978,27 @@ impl<'a> TimelineEventHandler<'a> { trace!("Adding new remote timeline item after all non-pending events"); let new_item = timeline_item(item, id); if should_push { - self.items.push_back(new_item); + self.state.items.push_back(new_item); } else { - self.items.insert(insert_idx, new_item); + self.state.items.insert(insert_idx, new_item); } } #[cfg(feature = "e2e-encryption")] Flow::Remote { position: TimelineItemPosition::Update(idx), .. } => { trace!("Updating timeline item at position {idx}"); - self.items.set(*idx, new_timeline_item(item, self.next_internal_id)); + self.state + .items + .set(*idx, new_timeline_item(item, &mut self.state.next_internal_id)); } } // See if we can update the read marker. - if *self.event_should_update_fully_read_marker { + if self.state.event_should_update_fully_read_marker { update_read_marker( - self.items, - self.fully_read_event.as_deref(), - self.event_should_update_fully_read_marker, + &mut self.state.items, + self.state.fully_read_event.as_deref(), + &mut self.state.event_should_update_fully_read_marker, ); } } @@ -1015,13 +1007,13 @@ impl<'a> TimelineEventHandler<'a> { match &self.ctx.flow { Flow::Local { .. } => None, Flow::Remote { event_id, .. } => { - let reactions = self.reactions.pending.remove(event_id)?; + let reactions = self.state.reactions.pending.remove(event_id)?; let mut bundled = IndexMap::new(); for reaction_event_id in reactions { let reaction_id = EventItemIdentifier::EventId(reaction_event_id); let Some((reaction_sender_data, annotation)) = - self.reactions.map.get(&reaction_id) + self.state.reactions.map.get(&reaction_id) else { error!( "inconsistent state: reaction from pending_reactions not in reaction_map"