From 6aee1f62bd25dcbc128b2dd0e7727f1cead19192 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Fri, 22 Mar 2024 13:14:28 +0100 Subject: [PATCH] day divider: make it impossible to handle an event without adjusting day dividers (#3267) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous API relied on the callers not forgetting about adjusting day dividers after handling an event. This makes it statically impossible, by requiring that `TimelineEventHandler` takes a `&mut DayDividerAdjuster` when operating, that it marks as not "consumed". Later, the caller must call `DayDividerAdjuster::maybe_adjust_day_dividers()`, to mark it as consumed. When dropping, we check that it's been consumed, otherwise we panic — as it's a developer error to not call `maybe_adjust_day_dividers()`. --- .../src/timeline/day_dividers.rs | 43 +++++++++++++-- .../src/timeline/event_handler.rs | 9 ++- .../matrix-sdk-ui/src/timeline/inner/state.rs | 55 +++++++++++++------ 3 files changed, 84 insertions(+), 23 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/day_dividers.rs b/crates/matrix-sdk-ui/src/timeline/day_dividers.rs index 83987379e..d350c563a 100644 --- a/crates/matrix-sdk-ui/src/timeline/day_dividers.rs +++ b/crates/matrix-sdk-ui/src/timeline/day_dividers.rs @@ -21,6 +21,8 @@ use eyeball_im::ObservableVectorTransaction; use ruma::MilliSecondsSinceUnixEpoch; use tracing::{event_enabled, instrument, trace, warn, Level}; +#[cfg(doc)] +use super::event_handler::TimelineEventHandler; use super::{ inner::TimelineInnerMetadata, util::timestamp_to_date, TimelineItem, TimelineItemKind, VirtualTimelineItem, @@ -28,17 +30,48 @@ use super::{ /// Algorithm ensuring that day dividers are adjusted correctly, according to /// new items that have been inserted. -#[derive(Default)] pub(super) struct DayDividerAdjuster { + /// The list of recorded operations to apply, after analyzing the latest + /// items. ops: Vec, + + /// A boolean indicating whether the struct has been used and thus must be + /// mark unused manually by calling [`Self::maybe_adjust_day_dividers`]. + consumed: bool, +} + +impl Drop for DayDividerAdjuster { + fn drop(&mut self) { + assert!( + self.consumed, + "the DayDividerAdjuster must be consumed with maybe_adjust_day_dividers" + ); + } +} + +impl Default for DayDividerAdjuster { + fn default() -> Self { + Self { + ops: Default::default(), + // The adjuster starts as consumed, and it will be marked no consumed iff it's used + // with `mark_used`. + consumed: true, + } + } } impl DayDividerAdjuster { + /// Returns a [`DayDividerToken`] ready for consumption. + pub fn mark_used(&mut self) { + // Mark the adjuster as needing to be consumed. + self.consumed = false; + } + /// Ensures that date separators are properly inserted/removed when needs /// be. #[instrument(skip_all)] pub fn maybe_adjust_day_dividers( - mut self, + &mut self, items: &mut ObservableVectorTransaction<'_, Arc>, meta: &mut TimelineInnerMetadata, ) { @@ -94,6 +127,8 @@ impl DayDividerAdjuster { #[cfg(not(debug))] warn!("{report}"); } + + self.consumed = true; } #[inline] @@ -268,14 +303,14 @@ impl DayDividerAdjuster { /// /// Returns a report if and only if there was at least one error. fn check_invariants<'a, 'o>( - self, + &mut self, items: &'a ObservableVectorTransaction<'o, Arc>, initial_state: Option>>, ) -> Option> { let mut report = DayDividerInvariantsReport { initial_state, errors: Vec::new(), - operations: self.ops, + operations: std::mem::take(&mut self.ops), final_state: items, }; diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index f87e02a7a..1a5bfbb60 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -48,6 +48,7 @@ use ruma::{ use tracing::{debug, error, field::debug, info, instrument, trace, warn}; use super::{ + day_dividers::DayDividerAdjuster, event_item::{ AnyOtherFullStateEventContent, BundledReactions, EventItemIdentifier, EventSendState, EventTimelineItemKind, LocalEventTimelineItem, Profile, RemoteEventOrigin, @@ -273,9 +274,15 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { /// /// Returns the number of timeline updates that were made. #[instrument(skip_all, fields(txn_id, event_id, position))] - pub(super) fn handle_event(mut self, event_kind: TimelineEventKind) -> HandleEventResult { + pub(super) fn handle_event( + mut self, + day_divider_adjuster: &mut DayDividerAdjuster, + event_kind: TimelineEventKind, + ) -> HandleEventResult { let span = tracing::Span::current(); + day_divider_adjuster.mark_used(); + let should_add = match &self.ctx.flow { Flow::Local { txn_id, .. } => { span.record("txn_id", debug(txn_id)); diff --git a/crates/matrix-sdk-ui/src/timeline/inner/state.rs b/crates/matrix-sdk-ui/src/timeline/inner/state.rs index f40003762..62e8dc246 100644 --- a/crates/matrix-sdk-ui/src/timeline/inner/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/inner/state.rs @@ -184,10 +184,15 @@ impl TimelineInnerState { }; let mut txn = self.transaction(); - TimelineEventHandler::new(&mut txn, ctx) - .handle_event(TimelineEventKind::Message { content, relations: Default::default() }); - txn.maybe_adjust_day_dividers(); + let mut day_divider_adjuster = DayDividerAdjuster::default(); + + TimelineEventHandler::new(&mut txn, ctx).handle_event( + &mut day_divider_adjuster, + TimelineEventKind::Message { content, relations: Default::default() }, + ); + + txn.adjust_day_dividers(day_divider_adjuster); txn.commit(); } @@ -218,20 +223,24 @@ impl TimelineInnerState { let mut txn = self.transaction(); let timeline_event_handler = TimelineEventHandler::new(&mut txn, ctx); + let mut day_divider_adjuster = DayDividerAdjuster::default(); + match to_redact { EventItemIdentifier::TransactionId(txn_id) => { - timeline_event_handler.handle_event(TimelineEventKind::LocalRedaction { - redacts: txn_id, - content: content.clone(), - }); + timeline_event_handler.handle_event( + &mut day_divider_adjuster, + TimelineEventKind::LocalRedaction { redacts: txn_id, content: content.clone() }, + ); } EventItemIdentifier::EventId(event_id) => { - timeline_event_handler - .handle_event(TimelineEventKind::Redaction { redacts: event_id, content }); + timeline_event_handler.handle_event( + &mut day_divider_adjuster, + TimelineEventKind::Redaction { redacts: event_id, content }, + ); } } - txn.maybe_adjust_day_dividers(); + txn.adjust_day_dividers(day_divider_adjuster); txn.commit(); } @@ -249,6 +258,8 @@ impl TimelineInnerState { { let mut txn = self.transaction(); + let mut day_divider_adjuster = DayDividerAdjuster::default(); + // Loop through all the indices, in order so we don't decrypt edits // before the event being edited, if both were UTD. Keep track of // index change as UTDs are removed instead of updated. @@ -269,6 +280,7 @@ impl TimelineInnerState { TimelineItemPosition::Update(idx), room_data_provider, settings, + &mut day_divider_adjuster, ) .await; @@ -279,7 +291,7 @@ impl TimelineInnerState { } } - txn.maybe_adjust_day_dividers(); + txn.adjust_day_dividers(day_divider_adjuster); txn.commit(); } @@ -443,16 +455,24 @@ impl TimelineInnerStateTransaction<'_> { TimelineEnd::Back { from_cache } => TimelineItemPosition::End { from_cache }, }; + let mut day_divider_adjuster = DayDividerAdjuster::default(); + for event in events { let handle_one_res = self - .handle_remote_event(event.into(), position, room_data_provider, settings) + .handle_remote_event( + event.into(), + position, + room_data_provider, + settings, + &mut day_divider_adjuster, + ) .await; total.items_added += handle_one_res.item_added as u64; total.items_updated += handle_one_res.items_updated as u64; } - self.maybe_adjust_day_dividers(); + self.adjust_day_dividers(day_divider_adjuster); total } @@ -466,6 +486,7 @@ impl TimelineInnerStateTransaction<'_> { position: TimelineItemPosition, room_data_provider: &P, settings: &TimelineInnerSettings, + day_divider_adjuster: &mut DayDividerAdjuster, ) -> HandleEventResult { let raw = event.event; let (event_id, sender, timestamp, txn_id, event_kind, should_add) = match raw.deserialize() @@ -576,7 +597,7 @@ impl TimelineInnerStateTransaction<'_> { }, }; - TimelineEventHandler::new(self, ctx).handle_event(event_kind) + TimelineEventHandler::new(self, ctx).handle_event(day_divider_adjuster, event_kind) } fn clear(&mut self) { @@ -686,10 +707,8 @@ impl TimelineInnerStateTransaction<'_> { } } - /// Inserts/removes any day dividers that might be missing or superfluous, - /// according to the events we just handled. - fn maybe_adjust_day_dividers(&mut self) { - DayDividerAdjuster::default().maybe_adjust_day_dividers(&mut self.items, &mut self.meta); + fn adjust_day_dividers(&mut self, mut adjuster: DayDividerAdjuster) { + adjuster.maybe_adjust_day_dividers(&mut self.items, &mut self.meta); } }