From 0647be1bc3fa211ab08f720d5a9d2f15c6a4eb92 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 4 Dec 2024 16:46:14 +0100 Subject: [PATCH] refactor(ui): Move `AllRemoteEvents` inside `observable_items`. This patch moves `AllRemoteEvents` inside `observable_items` so that more methods can be made private, which reduces the risk of misuses of this API. In particular, the following methods are now strictly private: - `clear` - `push_front` - `push_back` - `remove` - `timeline_item_has_been_inserted_at` - `timeline_item_has_been_removed_at` In fact, now, all `&mut self` method (except `get_by_event_id_mut`) are now strictly private! --- .../src/timeline/controller/mod.rs | 43 ++-- .../timeline/controller/observable_items.rs | 215 +++++++++++++++++- .../src/timeline/controller/state.rs | 198 ++-------------- .../src/timeline/day_dividers.rs | 62 ++--- .../src/timeline/event_handler.rs | 28 +-- .../src/timeline/read_receipts.rs | 60 +++-- 6 files changed, 341 insertions(+), 265 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs index d6b55a4ab..0a58951b6 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs @@ -54,10 +54,10 @@ use tracing::{ #[cfg(test)] pub(super) use self::observable_items::ObservableItems; pub(super) use self::{ - observable_items::ObservableItemsTransaction, + observable_items::{AllRemoteEvents, ObservableItemsTransaction}, state::{ - AllRemoteEvents, FullEventMeta, PendingEdit, PendingEditKind, TimelineMetadata, - TimelineNewItemPosition, TimelineState, TimelineStateTransaction, + FullEventMeta, PendingEdit, PendingEditKind, TimelineMetadata, TimelineNewItemPosition, + TimelineState, TimelineStateTransaction, }, }; use super::{ @@ -1487,13 +1487,22 @@ impl TimelineController { match receipt_type { SendReceiptType::Read => { - if let Some((old_pub_read, _)) = - state.meta.user_receipt(own_user_id, ReceiptType::Read, room).await + if let Some((old_pub_read, _)) = state + .meta + .user_receipt( + own_user_id, + ReceiptType::Read, + room, + state.items.all_remote_events(), + ) + .await { trace!(%old_pub_read, "found a previous public receipt"); - if let Some(relative_pos) = - state.meta.compare_events_positions(&old_pub_read, event_id) - { + if let Some(relative_pos) = state.meta.compare_events_positions( + &old_pub_read, + event_id, + state.items.all_remote_events(), + ) { trace!("event referred to new receipt is {relative_pos:?} the previous receipt"); return relative_pos == RelativePosition::After; } @@ -1506,9 +1515,11 @@ impl TimelineController { state.latest_user_read_receipt(own_user_id, room).await { trace!(%old_priv_read, "found a previous private receipt"); - if let Some(relative_pos) = - state.meta.compare_events_positions(&old_priv_read, event_id) - { + if let Some(relative_pos) = state.meta.compare_events_positions( + &old_priv_read, + event_id, + state.items.all_remote_events(), + ) { trace!("event referred to new receipt is {relative_pos:?} the previous receipt"); return relative_pos == RelativePosition::After; } @@ -1517,9 +1528,11 @@ impl TimelineController { SendReceiptType::FullyRead => { if let Some(prev_event_id) = self.room_data_provider.load_fully_read_marker().await { - if let Some(relative_pos) = - state.meta.compare_events_positions(&prev_event_id, event_id) - { + if let Some(relative_pos) = state.meta.compare_events_positions( + &prev_event_id, + event_id, + state.items.all_remote_events(), + ) { return relative_pos == RelativePosition::After; } } @@ -1535,7 +1548,7 @@ impl TimelineController { /// it's folded into another timeline item. pub(crate) async fn latest_event_id(&self) -> Option { let state = self.state.read().await; - state.meta.all_remote_events.last().map(|event_meta| &event_meta.event_id).cloned() + state.items.all_remote_events().last().map(|event_meta| &event_meta.event_id).cloned() } } diff --git a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs index 7c5c2e448..fc0a8aa6f 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -12,19 +12,36 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::{ops::Deref, sync::Arc}; +use std::{ + cmp::Ordering, + collections::{vec_deque::Iter, VecDeque}, + ops::Deref, + sync::Arc, +}; use eyeball_im::{ ObservableVector, ObservableVectorEntries, ObservableVectorEntry, ObservableVectorTransaction, ObservableVectorTransactionEntry, VectorSubscriber, }; use imbl::Vector; +use ruma::EventId; -use super::TimelineItem; +use super::{state::EventMeta, TimelineItem}; #[derive(Debug)] pub struct ObservableItems { + /// All timeline items. + /// + /// Yeah, there are here! items: ObservableVector>, + + /// List of all the remote events as received in the timeline, even the ones + /// that are discarded in the timeline items. + /// + /// This is useful to get this for the moment as it helps the `Timeline` to + /// compute read receipts and read markers. It also helps to map event to + /// timeline item, see [`EventMeta::timeline_item_index`] to learn more. + all_remote_events: AllRemoteEvents, } impl ObservableItems { @@ -34,9 +51,14 @@ impl ObservableItems { // sliding-sync tests with 20 events lag. This should still be // small enough. items: ObservableVector::with_capacity(32), + all_remote_events: AllRemoteEvents::default(), } } + pub fn all_remote_events(&self) -> &AllRemoteEvents { + &self.all_remote_events + } + pub fn is_empty(&self) -> bool { self.items.is_empty() } @@ -50,7 +72,10 @@ impl ObservableItems { } pub fn transaction(&mut self) -> ObservableItemsTransaction<'_> { - ObservableItemsTransaction { items: self.items.transaction() } + ObservableItemsTransaction { + items: self.items.transaction(), + all_remote_events: &mut self.all_remote_events, + } } pub fn set( @@ -85,6 +110,7 @@ impl Deref for ObservableItems { #[derive(Debug)] pub struct ObservableItemsTransaction<'observable_items> { items: ObservableVectorTransaction<'observable_items, Arc>, + all_remote_events: &'observable_items mut AllRemoteEvents, } impl<'observable_items> ObservableItemsTransaction<'observable_items> { @@ -92,6 +118,29 @@ impl<'observable_items> ObservableItemsTransaction<'observable_items> { self.items.get(timeline_item_index) } + pub fn all_remote_events(&self) -> &AllRemoteEvents { + &self.all_remote_events + } + + pub fn remove_remote_event(&mut self, event_index: usize) -> Option { + self.all_remote_events.remove(event_index) + } + + pub fn push_front_remote_event(&mut self, event_meta: EventMeta) { + self.all_remote_events.push_front(event_meta); + } + + pub fn push_back_remote_event(&mut self, event_meta: EventMeta) { + self.all_remote_events.push_back(event_meta); + } + + pub fn get_remote_event_by_event_id_mut( + &mut self, + event_id: &EventId, + ) -> Option<&mut EventMeta> { + self.all_remote_events.get_by_event_id_mut(event_id) + } + pub fn set( &mut self, timeline_item_index: usize, @@ -101,23 +150,36 @@ impl<'observable_items> ObservableItemsTransaction<'observable_items> { } pub fn remove(&mut self, timeline_item_index: usize) -> Arc { - self.items.remove(timeline_item_index) + let removed_timeline_item = self.items.remove(timeline_item_index); + self.all_remote_events.timeline_item_has_been_removed_at(timeline_item_index); + + removed_timeline_item } - pub fn insert(&mut self, timeline_item_index: usize, timeline_item: Arc) { + pub fn insert( + &mut self, + timeline_item_index: usize, + timeline_item: Arc, + event_index: Option, + ) { self.items.insert(timeline_item_index, timeline_item); + self.all_remote_events.timeline_item_has_been_inserted_at(timeline_item_index, event_index); } - pub fn push_front(&mut self, timeline_item: Arc) { + pub fn push_front(&mut self, timeline_item: Arc, event_index: Option) { self.items.push_front(timeline_item); + self.all_remote_events.timeline_item_has_been_inserted_at(0, event_index); } - pub fn push_back(&mut self, timeline_item: Arc) { + pub fn push_back(&mut self, timeline_item: Arc, event_index: Option) { self.items.push_back(timeline_item); + self.all_remote_events + .timeline_item_has_been_inserted_at(self.items.len().saturating_sub(1), event_index); } pub fn clear(&mut self) { self.items.clear(); + self.all_remote_events.clear(); } pub fn for_each(&mut self, f: F) @@ -140,3 +202,142 @@ impl Deref for ObservableItemsTransaction<'_> { &self.items } } + +/// A type for all remote events. +/// +/// Having this type helps to know exactly which parts of the code and how they +/// use all remote events. It also helps to give a bit of semantics on top of +/// them. +#[derive(Clone, Debug, Default)] +pub struct AllRemoteEvents(VecDeque); + +impl AllRemoteEvents { + /// Return a front-to-back iterator over all remote events. + pub fn iter(&self) -> Iter<'_, EventMeta> { + self.0.iter() + } + + /// Remove all remote events. + fn clear(&mut self) { + self.0.clear(); + } + + /// Insert a new remote event at the front of all the others. + fn push_front(&mut self, event_meta: EventMeta) { + // If there is an associated `timeline_item_index`, shift all the + // `timeline_item_index` that come after this one. + if let Some(new_timeline_item_index) = event_meta.timeline_item_index { + self.increment_all_timeline_item_index_after(new_timeline_item_index); + } + + // Push the event. + self.0.push_front(event_meta) + } + + /// Insert a new remote event at the back of all the others. + fn push_back(&mut self, event_meta: EventMeta) { + // If there is an associated `timeline_item_index`, shift all the + // `timeline_item_index` that come after this one. + if let Some(new_timeline_item_index) = event_meta.timeline_item_index { + self.increment_all_timeline_item_index_after(new_timeline_item_index); + } + + // Push the event. + self.0.push_back(event_meta) + } + + /// Remove one remote event at a specific index, and return it if it exists. + fn remove(&mut self, event_index: usize) -> Option { + // Remove the event. + let event_meta = self.0.remove(event_index)?; + + // If there is an associated `timeline_item_index`, shift all the + // `timeline_item_index` that come after this one. + if let Some(removed_timeline_item_index) = event_meta.timeline_item_index { + self.decrement_all_timeline_item_index_after(removed_timeline_item_index); + }; + + Some(event_meta) + } + + /// Return a reference to the last remote event if it exists. + pub fn last(&self) -> Option<&EventMeta> { + self.0.back() + } + + /// Return the index of the last remote event if it exists. + pub fn last_index(&self) -> Option { + self.0.len().checked_sub(1) + } + + /// Get a mutable reference to a specific remote event by its ID. + pub fn get_by_event_id_mut(&mut self, event_id: &EventId) -> Option<&mut EventMeta> { + self.0.iter_mut().rev().find(|event_meta| event_meta.event_id == event_id) + } + + /// Shift to the right all timeline item indexes that are equal to or + /// greater than `new_timeline_item_index`. + fn increment_all_timeline_item_index_after(&mut self, new_timeline_item_index: usize) { + for event_meta in self.0.iter_mut() { + if let Some(timeline_item_index) = event_meta.timeline_item_index.as_mut() { + if *timeline_item_index >= new_timeline_item_index { + *timeline_item_index += 1; + } + } + } + } + + /// Shift to the left all timeline item indexes that are greater than + /// `removed_wtimeline_item_index`. + fn decrement_all_timeline_item_index_after(&mut self, removed_timeline_item_index: usize) { + for event_meta in self.0.iter_mut() { + if let Some(timeline_item_index) = event_meta.timeline_item_index.as_mut() { + if *timeline_item_index > removed_timeline_item_index { + *timeline_item_index -= 1; + } + } + } + } + + fn timeline_item_has_been_inserted_at( + &mut self, + new_timeline_item_index: usize, + event_index: Option, + ) { + self.increment_all_timeline_item_index_after(new_timeline_item_index); + + if let Some(event_index) = event_index { + if let Some(event_meta) = self.0.get_mut(event_index) { + event_meta.timeline_item_index = Some(new_timeline_item_index); + } + } + } + + fn timeline_item_has_been_removed_at(&mut self, timeline_item_index_to_remove: usize) { + for event_meta in self.0.iter_mut() { + let mut remove_timeline_item_index = false; + + // A `timeline_item_index` is removed. Let's shift all indexes that come + // after the removed one. + if let Some(timeline_item_index) = event_meta.timeline_item_index.as_mut() { + match (*timeline_item_index).cmp(&timeline_item_index_to_remove) { + Ordering::Equal => { + remove_timeline_item_index = true; + } + + Ordering::Greater => { + *timeline_item_index -= 1; + } + + Ordering::Less => {} + } + } + + // This is the `event_meta` that holds the `timeline_item_index` that is being + // removed. So let's clean it. + if remove_timeline_item_index { + event_meta.timeline_item_index = None; + } + } + } +} diff --git a/crates/matrix-sdk-ui/src/timeline/controller/state.rs b/crates/matrix-sdk-ui/src/timeline/controller/state.rs index 72916c1c7..925e1d45d 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/state.rs @@ -13,8 +13,7 @@ // limitations under the License. use std::{ - cmp::Ordering, - collections::{vec_deque::Iter, HashMap, VecDeque}, + collections::HashMap, future::Future, num::NonZeroUsize, sync::{Arc, RwLock}, @@ -46,7 +45,7 @@ use ruma::{ use tracing::{debug, instrument, trace, warn}; use super::{ - observable_items::{ObservableItems, ObservableItemsTransaction}, + observable_items::{AllRemoteEvents, ObservableItems, ObservableItemsTransaction}, HandleManyEventsResult, TimelineFocusKind, TimelineSettings, }; use crate::{ @@ -546,7 +545,7 @@ impl TimelineStateTransaction<'_> { }; // Remember the event before returning prematurely. - // See [`TimelineMetadata::all_remote_events`]. + // See [`ObservableItems::all_remote_events`]. self.add_or_update_remote_event( event_meta, position, @@ -585,7 +584,7 @@ impl TimelineStateTransaction<'_> { }; // Remember the event before returning prematurely. - // See [`TimelineMetadata::all_remote_events`]. + // See [`ObservableItems::all_remote_events`]. self.add_or_update_remote_event( event_meta, position, @@ -611,7 +610,7 @@ impl TimelineStateTransaction<'_> { }; // Remember the event. - // See [`TimelineMetadata::all_remote_events`]. + // See [`ObservableItems::all_remote_events`]. self.add_or_update_remote_event(event_meta, position, room_data_provider, settings).await; let sender_profile = room_data_provider.profile_from_user_id(&sender).await; @@ -623,7 +622,7 @@ impl TimelineStateTransaction<'_> { read_receipts: if settings.track_read_receipts && should_add { self.meta.read_receipts.compute_event_receipts( &event_id, - &self.meta.all_remote_events, + self.items.all_remote_events(), matches!(position, TimelineItemPosition::End { .. }), ) } else { @@ -702,7 +701,7 @@ impl TimelineStateTransaction<'_> { } /// Add or update a remote event in the - /// [`TimelineMetadata::all_remote_events`] collection. + /// [`ObservableItems::all_remote_events`] collection. /// /// This method also adjusts read receipt if needed. async fn add_or_update_remote_event( @@ -712,7 +711,7 @@ impl TimelineStateTransaction<'_> { room_data_provider: &P, settings: &TimelineSettings, ) { - // Detect if an event already exists in [`TimelineMetadata::all_remote_events`]. + // Detect if an event already exists in [`ObservableItems::all_remote_events`]. // // Returns its position, in this case. fn event_already_exists( @@ -725,27 +724,27 @@ impl TimelineStateTransaction<'_> { match position { TimelineItemPosition::Start { .. } => { if let Some(pos) = - event_already_exists(event_meta.event_id, &self.meta.all_remote_events) + event_already_exists(event_meta.event_id, &self.items.all_remote_events()) { - self.meta.all_remote_events.remove(pos); + self.items.remove_remote_event(pos); } - self.meta.all_remote_events.push_front(event_meta.base_meta()) + self.items.push_front_remote_event(event_meta.base_meta()) } TimelineItemPosition::End { .. } => { if let Some(pos) = - event_already_exists(event_meta.event_id, &self.meta.all_remote_events) + event_already_exists(event_meta.event_id, &self.items.all_remote_events()) { - self.meta.all_remote_events.remove(pos); + self.items.remove_remote_event(pos); } - self.meta.all_remote_events.push_back(event_meta.base_meta()); + self.items.push_back_remote_event(event_meta.base_meta()); } TimelineItemPosition::UpdateDecrypted { .. } => { if let Some(event) = - self.meta.all_remote_events.get_by_event_id_mut(event_meta.event_id) + self.items.get_remote_event_by_event_id_mut(event_meta.event_id) { if event.visible != event_meta.visible { event.visible = event_meta.visible; @@ -918,13 +917,6 @@ pub(in crate::timeline) struct TimelineMetadata { /// the device has terabytes of RAM. next_internal_id: u64, - /// List of all the remote events as received in the timeline, even the ones - /// that are discarded in the timeline items. - /// - /// This is useful to get this for the moment as it helps the `Timeline` to - /// compute read receipts and read markers. - pub all_remote_events: AllRemoteEvents, - /// State helping matching reactions to their associated events, and /// stashing pending reactions. pub reactions: Reactions, @@ -967,7 +959,6 @@ impl TimelineMetadata { ) -> Self { Self { own_user_id, - all_remote_events: Default::default(), next_internal_id: Default::default(), reactions: Default::default(), pending_poll_events: Default::default(), @@ -987,7 +978,6 @@ impl TimelineMetadata { pub(crate) fn clear(&mut self) { // Note: we don't clear the next internal id to avoid bad cases of stale unique // ids across timeline clears. - self.all_remote_events.clear(); self.reactions.clear(); self.pending_poll_events.clear(); self.pending_edits.clear(); @@ -1008,6 +998,7 @@ impl TimelineMetadata { &self, event_a: &EventId, event_b: &EventId, + all_remote_events: &AllRemoteEvents, ) -> Option { if event_a == event_b { return Some(RelativePosition::Same); @@ -1015,11 +1006,11 @@ impl TimelineMetadata { // We can make early returns here because we know all events since the end of // the timeline, so the first event encountered is the oldest one. - for meta in self.all_remote_events.iter().rev() { - if meta.event_id == event_a { + for event_meta in all_remote_events.iter().rev() { + if event_meta.event_id == event_a { return Some(RelativePosition::Before); } - if meta.event_id == event_b { + if event_meta.event_id == event_b { return Some(RelativePosition::After); } } @@ -1092,8 +1083,7 @@ impl TimelineMetadata { // Only insert the read marker if it is not at the end of the timeline. if idx + 1 < items.len() { let idx = idx + 1; - items.insert(idx, TimelineItem::read_marker()); - self.all_remote_events.timeline_item_has_been_inserted_at(idx, None); + items.insert(idx, TimelineItem::read_marker(), None); self.has_up_to_date_read_marker_item = true; } else { // The next event might require a read marker to be inserted at the current @@ -1114,7 +1104,6 @@ impl TimelineMetadata { if from + 1 == items.len() { // The read marker has nothing after it. An item disappeared; remove it. items.remove(from); - self.all_remote_events.timeline_item_has_been_removed_at(from); } self.has_up_to_date_read_marker_item = true; return; @@ -1122,15 +1111,13 @@ impl TimelineMetadata { let prev_len = items.len(); let read_marker = items.remove(from); - self.all_remote_events.timeline_item_has_been_removed_at(from); // Only insert the read marker if it is not at the end of the timeline. if to + 1 < prev_len { // Since the fully-read event's index was shifted to the left // by one position by the remove call above, insert the fully- // read marker at its previous position, rather than that + 1 - items.insert(to, read_marker); - self.all_remote_events.timeline_item_has_been_inserted_at(to, None); + items.insert(to, read_marker, None); self.has_up_to_date_read_marker_item = true; } else { self.has_up_to_date_read_marker_item = false; @@ -1140,145 +1127,6 @@ impl TimelineMetadata { } } -/// A type for all remote events. -/// -/// Having this type helps to know exactly which parts of the code and how they -/// use all remote events. It also helps to give a bit of semantics on top of -/// them. -#[derive(Clone, Debug, Default)] -pub(crate) struct AllRemoteEvents(VecDeque); - -impl AllRemoteEvents { - /// Return a front-to-back iterator over all remote events. - pub fn iter(&self) -> Iter<'_, EventMeta> { - self.0.iter() - } - - /// Remove all remote events. - pub fn clear(&mut self) { - self.0.clear(); - } - - /// Insert a new remote event at the front of all the others. - pub fn push_front(&mut self, event_meta: EventMeta) { - // If there is an associated `timeline_item_index`, shift all the - // `timeline_item_index` that come after this one. - if let Some(new_timeline_item_index) = event_meta.timeline_item_index { - self.increment_all_timeline_item_index_after(new_timeline_item_index); - } - - // Push the event. - self.0.push_front(event_meta) - } - - /// Insert a new remote event at the back of all the others. - pub fn push_back(&mut self, event_meta: EventMeta) { - // If there is an associated `timeline_item_index`, shift all the - // `timeline_item_index` that come after this one. - if let Some(new_timeline_item_index) = event_meta.timeline_item_index { - self.increment_all_timeline_item_index_after(new_timeline_item_index); - } - - // Push the event. - self.0.push_back(event_meta) - } - - /// Remove one remote event at a specific index, and return it if it exists. - pub fn remove(&mut self, event_index: usize) -> Option { - // Remove the event. - let event_meta = self.0.remove(event_index)?; - - // If there is an associated `timeline_item_index`, shift all the - // `timeline_item_index` that come after this one. - if let Some(removed_timeline_item_index) = event_meta.timeline_item_index { - self.decrement_all_timeline_item_index_after(removed_timeline_item_index); - }; - - Some(event_meta) - } - - /// Return a reference to the last remote event if it exists. - pub fn last(&self) -> Option<&EventMeta> { - self.0.back() - } - - /// Return the index of the last remote event if it exists. - pub fn last_index(&self) -> Option { - self.0.len().checked_sub(1) - } - - /// Get a mutable reference to a specific remote event by its ID. - pub fn get_by_event_id_mut(&mut self, event_id: &EventId) -> Option<&mut EventMeta> { - self.0.iter_mut().rev().find(|event_meta| event_meta.event_id == event_id) - } - - /// Shift to the right all timeline item indexes that are equal to or - /// greater than `new_timeline_item_index`. - fn increment_all_timeline_item_index_after(&mut self, new_timeline_item_index: usize) { - for event_meta in self.0.iter_mut() { - if let Some(timeline_item_index) = event_meta.timeline_item_index.as_mut() { - if *timeline_item_index >= new_timeline_item_index { - *timeline_item_index += 1; - } - } - } - } - - /// Shift to the left all timeline item indexes that are greater than - /// `removed_wtimeline_item_index`. - fn decrement_all_timeline_item_index_after(&mut self, removed_timeline_item_index: usize) { - for event_meta in self.0.iter_mut() { - if let Some(timeline_item_index) = event_meta.timeline_item_index.as_mut() { - if *timeline_item_index > removed_timeline_item_index { - *timeline_item_index -= 1; - } - } - } - } - - pub fn timeline_item_has_been_inserted_at( - &mut self, - new_timeline_item_index: usize, - event_index: Option, - ) { - self.increment_all_timeline_item_index_after(new_timeline_item_index); - - if let Some(event_index) = event_index { - if let Some(event_meta) = self.0.get_mut(event_index) { - event_meta.timeline_item_index = Some(new_timeline_item_index); - } - } - } - - pub fn timeline_item_has_been_removed_at(&mut self, timeline_item_index_to_remove: usize) { - for event_meta in self.0.iter_mut() { - let mut remove_timeline_item_index = false; - - // A `timeline_item_index` is removed. Let's shift all indexes that come - // after the removed one. - if let Some(timeline_item_index) = event_meta.timeline_item_index.as_mut() { - match (*timeline_item_index).cmp(&timeline_item_index_to_remove) { - Ordering::Equal => { - remove_timeline_item_index = true; - } - - Ordering::Greater => { - *timeline_item_index -= 1; - } - - Ordering::Less => {} - } - } - - // This is the `event_meta` that holds the `timeline_item_index` that is being - // removed. So let's clean it. - if remove_timeline_item_index { - event_meta.timeline_item_index = None; - } - } - } -} - /// Full metadata about an event. /// /// Only used to group function parameters. @@ -1317,9 +1165,9 @@ pub(crate) struct EventMeta { /// Foundation for the mapping between remote events to timeline items. /// /// Let's explain it. The events represent the first set and are stored in - /// [`TimelineMetadata::all_remote_events`], and the timeline + /// [`ObservableItems::all_remote_events`], and the timeline /// items represent the second set and are stored in - /// [`TimelineState::items`]. + /// [`ObservableItems::items`]. /// /// Each event is mapped to at most one timeline item: /// diff --git a/crates/matrix-sdk-ui/src/timeline/day_dividers.rs b/crates/matrix-sdk-ui/src/timeline/day_dividers.rs index 6f02196c1..05385723f 100644 --- a/crates/matrix-sdk-ui/src/timeline/day_dividers.rs +++ b/crates/matrix-sdk-ui/src/timeline/day_dividers.rs @@ -301,11 +301,11 @@ impl DayDividerAdjuster { // Keep push semantics, if we're inserting at the front or the back. if at == items.len() { - items.push_back(item); + items.push_back(item, None); } else if at == 0 { - items.push_front(item); + items.push_front(item, None); } else { - items.insert(at, item); + items.insert(at, item, None); } offset += 1; @@ -654,9 +654,12 @@ mod tests { let timestamp_next_day = MilliSecondsSinceUnixEpoch((42 + 3600 * 24 * 1000).try_into().unwrap()); - txn.push_back(meta.new_timeline_item(event_with_ts(timestamp))); - txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp_next_day))); - txn.push_back(meta.new_timeline_item(VirtualTimelineItem::ReadMarker)); + txn.push_back(meta.new_timeline_item(event_with_ts(timestamp)), None); + txn.push_back( + meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp_next_day)), + None, + ); + txn.push_back(meta.new_timeline_item(VirtualTimelineItem::ReadMarker), None); let mut adjuster = DayDividerAdjuster::default(); adjuster.run(&mut txn, &mut meta); @@ -690,10 +693,13 @@ mod tests { assert_ne!(timestamp_to_date(timestamp), timestamp_to_date(timestamp_next_day)); let event = event_with_ts(timestamp); - txn.push_back(meta.new_timeline_item(event.clone())); - txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp_next_day))); - txn.push_back(meta.new_timeline_item(VirtualTimelineItem::ReadMarker)); - txn.push_back(meta.new_timeline_item(event)); + txn.push_back(meta.new_timeline_item(event.clone()), None); + txn.push_back( + meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp_next_day)), + None, + ); + txn.push_back(meta.new_timeline_item(VirtualTimelineItem::ReadMarker), None); + txn.push_back(meta.new_timeline_item(event), None); let mut adjuster = DayDividerAdjuster::default(); adjuster.run(&mut txn, &mut meta); @@ -721,12 +727,12 @@ mod tests { MilliSecondsSinceUnixEpoch((42 + 3600 * 24 * 1000).try_into().unwrap()); assert_ne!(timestamp_to_date(timestamp), timestamp_to_date(timestamp_next_day)); - txn.push_back(meta.new_timeline_item(event_with_ts(timestamp))); - txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp))); - txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp))); - txn.push_back(meta.new_timeline_item(VirtualTimelineItem::ReadMarker)); - txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp))); - txn.push_back(meta.new_timeline_item(event_with_ts(timestamp_next_day))); + txn.push_back(meta.new_timeline_item(event_with_ts(timestamp)), None); + txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp)), None); + txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp)), None); + txn.push_back(meta.new_timeline_item(VirtualTimelineItem::ReadMarker), None); + txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp)), None); + txn.push_back(meta.new_timeline_item(event_with_ts(timestamp_next_day)), None); let mut adjuster = DayDividerAdjuster::default(); adjuster.run(&mut txn, &mut meta); @@ -755,10 +761,10 @@ mod tests { MilliSecondsSinceUnixEpoch((42 + 3600 * 24 * 1000).try_into().unwrap()); assert_ne!(timestamp_to_date(timestamp), timestamp_to_date(timestamp_next_day)); - txn.push_back(meta.new_timeline_item(event_with_ts(timestamp_next_day))); - txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp))); - txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp))); - txn.push_back(meta.new_timeline_item(event_with_ts(timestamp_next_day))); + txn.push_back(meta.new_timeline_item(event_with_ts(timestamp_next_day)), None); + txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp)), None); + txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp)), None); + txn.push_back(meta.new_timeline_item(event_with_ts(timestamp_next_day)), None); let mut adjuster = DayDividerAdjuster::default(); adjuster.run(&mut txn, &mut meta); @@ -782,9 +788,9 @@ mod tests { let timestamp = MilliSecondsSinceUnixEpoch(uint!(42)); - txn.push_back(meta.new_timeline_item(event_with_ts(timestamp))); - txn.push_back(meta.new_timeline_item(VirtualTimelineItem::ReadMarker)); - txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp))); + txn.push_back(meta.new_timeline_item(event_with_ts(timestamp)), None); + txn.push_back(meta.new_timeline_item(VirtualTimelineItem::ReadMarker), None); + txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp)), None); let mut adjuster = DayDividerAdjuster::default(); adjuster.run(&mut txn, &mut meta); @@ -808,9 +814,9 @@ mod tests { let timestamp = MilliSecondsSinceUnixEpoch(uint!(42)); - txn.push_back(meta.new_timeline_item(VirtualTimelineItem::ReadMarker)); - txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp))); - txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp))); + txn.push_back(meta.new_timeline_item(VirtualTimelineItem::ReadMarker), None); + txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp)), None); + txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp)), None); let mut adjuster = DayDividerAdjuster::default(); adjuster.run(&mut txn, &mut meta); @@ -831,8 +837,8 @@ mod tests { let mut meta = test_metadata(); let timestamp = MilliSecondsSinceUnixEpoch(uint!(42)); - txn.push_back(meta.new_timeline_item(VirtualTimelineItem::ReadMarker)); - txn.push_back(meta.new_timeline_item(event_with_ts(timestamp))); + txn.push_back(meta.new_timeline_item(VirtualTimelineItem::ReadMarker), None); + txn.push_back(meta.new_timeline_item(event_with_ts(timestamp)), None); let mut adjuster = DayDividerAdjuster::default(); adjuster.run(&mut txn, &mut meta); diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index e04b03997..b8e6457dd 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -514,7 +514,6 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { // wouldn't normally be visible. Remove it. trace!("Removing UTD that was successfully retried"); self.items.remove(timeline_item_index); - self.meta.all_remote_events.timeline_item_has_been_removed_at(timeline_item_index); self.result.item_removed = true; } @@ -1095,7 +1094,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { trace!("Adding new local timeline item"); let item = self.meta.new_timeline_item(item); - self.items.push_back(item); + self.items.push_back(item, None); } Flow::Remote { position: TimelineItemPosition::Start { .. }, event_id, .. } => { @@ -1112,8 +1111,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { trace!("Adding new remote timeline item at the start"); let item = self.meta.new_timeline_item(item); - self.items.push_front(item); - self.meta.all_remote_events.timeline_item_has_been_inserted_at(0, Some(0)); + self.items.push_front(item, Some(0)); } Flow::Remote { @@ -1189,6 +1187,13 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { }) .unwrap_or(0); + let event_index = + Some(self.items.all_remote_events().last_index() + // The last remote event is necessarily associated to this + // timeline item, see the contract of this method. + .expect("A timeline item is being added but its associated remote event is missing") + ); + // Try to keep precise insertion semantics here, in this exact order: // // * _push back_ when the new item is inserted after all items (the assumption @@ -1199,26 +1204,17 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { if timeline_item_index == self.items.len() { trace!("Adding new remote timeline item at the back"); - self.items.push_back(new_item); + self.items.push_back(new_item, event_index); } else if timeline_item_index == 0 { trace!("Adding new remote timeline item at the front"); - self.items.push_front(new_item); + self.items.push_front(new_item, event_index); } else { trace!( timeline_item_index, "Adding new remote timeline item at specific index" ); - self.items.insert(timeline_item_index, new_item); + self.items.insert(timeline_item_index, new_item, event_index); } - - self.meta.all_remote_events.timeline_item_has_been_inserted_at( - timeline_item_index, - Some(self.meta.all_remote_events.last_index() - // The last remote event is necessarily associated to this - // timeline item, see the contract of this method. - .expect("A timeline item is being added but its associated remote event is missing") - ), - ); } Flow::Remote { diff --git a/crates/matrix-sdk-ui/src/timeline/read_receipts.rs b/crates/matrix-sdk-ui/src/timeline/read_receipts.rs index 100402300..8cc0c8c21 100644 --- a/crates/matrix-sdk-ui/src/timeline/read_receipts.rs +++ b/crates/matrix-sdk-ui/src/timeline/read_receipts.rs @@ -99,9 +99,10 @@ impl ReadReceipts { &mut self, new_receipt: FullReceipt<'_>, is_own_user_id: bool, - all_events: &AllRemoteEvents, timeline_items: &mut ObservableItemsTransaction<'_>, ) { + let all_events = timeline_items.all_remote_events(); + // Get old receipt. let old_receipt = self.get_latest(new_receipt.user_id, &new_receipt.receipt_type); if old_receipt @@ -382,7 +383,6 @@ impl TimelineStateTransaction<'_> { self.meta.read_receipts.maybe_update_read_receipt( full_receipt, is_own_user_id, - &self.meta.all_remote_events, &mut self.items, ); } @@ -417,7 +417,6 @@ impl TimelineStateTransaction<'_> { self.meta.read_receipts.maybe_update_read_receipt( full_receipt, user_id == own_user_id, - &self.meta.all_remote_events, &mut self.items, ); } @@ -446,7 +445,6 @@ impl TimelineStateTransaction<'_> { self.meta.read_receipts.maybe_update_read_receipt( full_receipt, is_own_event, - &self.meta.all_remote_events, &mut self.items, ); } @@ -456,8 +454,8 @@ impl TimelineStateTransaction<'_> { pub(super) fn maybe_update_read_receipts_of_prev_event(&mut self, event_id: &EventId) { // Find the previous visible event, if there is one. let Some(prev_event_meta) = self - .meta - .all_remote_events + .items + .all_remote_events() .iter() .rev() // Find the event item. @@ -487,7 +485,7 @@ impl TimelineStateTransaction<'_> { let read_receipts = self.meta.read_receipts.compute_event_receipts( &remote_prev_event_item.event_id, - &self.meta.all_remote_events, + &self.items.all_remote_events(), false, ); @@ -535,18 +533,24 @@ impl TimelineState { user_id: &UserId, room_data_provider: &P, ) -> Option<(OwnedEventId, Receipt)> { - let public_read_receipt = - self.meta.user_receipt(user_id, ReceiptType::Read, room_data_provider).await; - let private_read_receipt = - self.meta.user_receipt(user_id, ReceiptType::ReadPrivate, room_data_provider).await; + let all_remote_events = self.items.all_remote_events(); + let public_read_receipt = self + .meta + .user_receipt(user_id, ReceiptType::Read, room_data_provider, all_remote_events) + .await; + let private_read_receipt = self + .meta + .user_receipt(user_id, ReceiptType::ReadPrivate, room_data_provider, all_remote_events) + .await; // Let's assume that a private read receipt should be more recent than a public // read receipt, otherwise there's no point in the private read receipt, // and use it as default. - match self - .meta - .compare_optional_receipts(public_read_receipt.as_ref(), private_read_receipt.as_ref()) - { + match self.meta.compare_optional_receipts( + public_read_receipt.as_ref(), + private_read_receipt.as_ref(), + self.items.all_remote_events(), + ) { Ordering::Greater => public_read_receipt, Ordering::Less => private_read_receipt, _ => unreachable!(), @@ -568,16 +572,19 @@ impl TimelineState { // Let's assume that a private read receipt should be more recent than a public // read receipt, otherwise there's no point in the private read receipt, // and use it as default. - let (latest_receipt_id, _) = - match self.meta.compare_optional_receipts(public_read_receipt, private_read_receipt) { - Ordering::Greater => public_read_receipt?, - Ordering::Less => private_read_receipt?, - _ => unreachable!(), - }; + let (latest_receipt_id, _) = match self.meta.compare_optional_receipts( + public_read_receipt, + private_read_receipt, + self.items.all_remote_events(), + ) { + Ordering::Greater => public_read_receipt?, + Ordering::Less => private_read_receipt?, + _ => unreachable!(), + }; // Find the corresponding visible event. - self.meta - .all_remote_events + self.items + .all_remote_events() .iter() .rev() .skip_while(|ev| ev.event_id != *latest_receipt_id) @@ -597,6 +604,7 @@ impl TimelineMetadata { user_id: &UserId, receipt_type: ReceiptType, room_data_provider: &P, + all_remote_events: &AllRemoteEvents, ) -> Option<(OwnedEventId, Receipt)> { if let Some(receipt) = self.read_receipts.get_latest(user_id, &receipt_type) { // Since it is in the timeline, it should be the most recent. @@ -616,6 +624,7 @@ impl TimelineMetadata { match self.compare_optional_receipts( main_thread_read_receipt.as_ref(), unthreaded_read_receipt.as_ref(), + all_remote_events, ) { Ordering::Greater => main_thread_read_receipt, Ordering::Less => unthreaded_read_receipt, @@ -633,6 +642,7 @@ impl TimelineMetadata { &self, lhs: Option<&(OwnedEventId, Receipt)>, rhs_or_default: Option<&(OwnedEventId, Receipt)>, + all_remote_events: &AllRemoteEvents, ) -> Ordering { // If we only have one, use it. let Some((lhs_event_id, lhs_receipt)) = lhs else { @@ -643,7 +653,9 @@ impl TimelineMetadata { }; // Compare by position in the timeline. - if let Some(relative_pos) = self.compare_events_positions(lhs_event_id, rhs_event_id) { + if let Some(relative_pos) = + self.compare_events_positions(lhs_event_id, rhs_event_id, all_remote_events) + { if relative_pos == RelativePosition::Before { return Ordering::Greater; }