From a71afd181a331542ea676ed0197bb8278a13c25e Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 4 Dec 2024 10:45:48 +0100 Subject: [PATCH 01/25] refactor(ui): Add `EventMeta::timeline_item_index`. This is the foundation for the mapping between remote events and timeline items. --- .../src/timeline/controller/state.rs | 70 ++++++++++++++++++- 1 file changed, 69 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk-ui/src/timeline/controller/state.rs b/crates/matrix-sdk-ui/src/timeline/controller/state.rs index 1800bebdab7..96283554832 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/state.rs @@ -1199,7 +1199,11 @@ pub(crate) struct FullEventMeta<'a> { impl FullEventMeta<'_> { fn base_meta(&self) -> EventMeta { - EventMeta { event_id: self.event_id.to_owned(), visible: self.visible } + EventMeta { + event_id: self.event_id.to_owned(), + visible: self.visible, + timeline_item_index: None, + } } } @@ -1208,6 +1212,70 @@ impl FullEventMeta<'_> { pub(crate) struct EventMeta { /// The ID of the event. pub event_id: OwnedEventId, + /// Whether the event is among the timeline items. pub visible: bool, + + /// 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 + /// items represent the second set and are stored in + /// [`TimelineState::items`]. + /// + /// Each event is mapped to at most one timeline item: + /// + /// - `None` if the event isn't rendered in the timeline (e.g. some state + /// events, or malformed events) or is rendered as a timeline item that + /// attaches to or groups with another item, like reactions, + /// - `Some(_)` if the event is rendered in the timeline. + /// + /// This is neither a surjection nor an injection. Every timeline item may + /// not be attached to an event, for example with a virtual timeline item. + /// We can formulate other rules: + /// + /// - a timeline item that doesn't _move_ and that is represented by an + /// event has a mapping to an event, + /// - a virtual timeline item has no mapping to an event. + /// + /// Imagine the following remote events: + /// + /// | index | remote events | + /// +-------+---------------+ + /// | 0 | `$ev0` | + /// | 1 | `$ev1` | + /// | 2 | `$ev2` | + /// | 3 | `$ev3` | + /// | 4 | `$ev4` | + /// | 5 | `$ev5` | + /// + /// Once rendered in a timeline, it for example produces: + /// + /// | index | item | aside items | + /// +-------+-------------------+----------------------+ + /// | 0 | content of `$ev0` | | + /// | 1 | content of `$ev2` | reaction with `$ev4` | + /// | 2 | day divider | | + /// | 3 | content of `$ev3` | | + /// | 4 | content of `$ev5` | | + /// + /// Note the day divider that is a virtual item. Also note ``$ev4`` which is + /// a reaction to `$ev2`. Finally note that `$ev1` is not rendered in + /// the timeline. + /// + /// The mapping between remove event index to timeline item index will look + /// like this: + /// + /// | remove event index | timeline item index | comment | + /// +--------------------+---------------------+--------------------------------------------+ + /// | 0 | `Some(0)` | `$ev0` is rendered as the #0 timeline item | + /// | 1 | `None` | `$ev1` isn't rendered in the timeline | + /// | 2 | `Some(1)` | `$ev2` is rendered as the #1 timeline item | + /// | 3 | `Some(3)` | `$ev3` is rendered as the #3 timeline item | + /// | 4 | `None` | `$ev4` is a reaction to item #1 | + /// | 5 | `Some(4)` | `$ev5` is rendered as the #4 timeline item | + /// + /// Note that the #2 timeline item (the day divider) doesn't map to any + /// remote event, but if it moves, it has an impact on this mapping. + pub timeline_item_index: Option, } From 8c7c4c2eefbd9d3f163178b25e611abe5b86a848 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 4 Dec 2024 10:53:20 +0100 Subject: [PATCH 02/25] refactor(ui): Maintain `timeline_item_index` when remote events are manipulated. This patch maintains the `timeline_item_index` when a new remote events is added or removed. --- .../src/timeline/controller/state.rs | 49 ++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk-ui/src/timeline/controller/state.rs b/crates/matrix-sdk-ui/src/timeline/controller/state.rs index 96283554832..1db916cff31 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/state.rs @@ -1157,17 +1157,40 @@ impl AllRemoteEvents { /// 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 { - self.0.remove(event_index) + // 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. @@ -1179,6 +1202,30 @@ impl AllRemoteEvents { 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; + } + } + } + } } /// Full metadata about an event. From 7e5bc05e2ad95d940675f51ccb32f4c9ce4bde10 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 4 Dec 2024 11:17:07 +0100 Subject: [PATCH 03/25] refactor(ui): Maintain `timeline_item_index` when timeline items are inserted or removed. This patch maintains the `timeline_item_index` when timeline items are inserted or removed. --- .../src/timeline/controller/state.rs | 55 +++++++++++++- .../src/timeline/event_handler.rs | 72 ++++++++++++++----- 2 files changed, 107 insertions(+), 20 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/controller/state.rs b/crates/matrix-sdk-ui/src/timeline/controller/state.rs index 1db916cff31..92fb2f07500 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/state.rs @@ -13,6 +13,7 @@ // limitations under the License. use std::{ + cmp::Ordering, collections::{vec_deque::Iter, HashMap, VecDeque}, future::Future, num::NonZeroUsize, @@ -1092,7 +1093,9 @@ impl TimelineMetadata { (None, Some(idx)) => { // Only insert the read marker if it is not at the end of the timeline. if idx + 1 < items.len() { - items.insert(idx + 1, TimelineItem::read_marker()); + let idx = idx + 1; + items.insert(idx, TimelineItem::read_marker()); + self.all_remote_events.timeline_item_has_been_inserted_at(idx, 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 @@ -1113,6 +1116,7 @@ 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; @@ -1120,6 +1124,7 @@ 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 { @@ -1127,6 +1132,7 @@ impl TimelineMetadata { // 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); self.has_up_to_date_read_marker_item = true; } else { self.has_up_to_date_read_marker_item = false; @@ -1198,6 +1204,11 @@ impl AllRemoteEvents { 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) @@ -1226,6 +1237,48 @@ impl AllRemoteEvents { } } } + + 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. diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index cc01448714b..bcb312f0274 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -504,14 +504,15 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { trace!("No new item added"); if let Flow::Remote { - position: TimelineItemPosition::UpdateDecrypted { timeline_item_index: idx }, + position: TimelineItemPosition::UpdateDecrypted { timeline_item_index }, .. } = self.ctx.flow { // 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.items.remove(timeline_item_index); + self.meta.all_remote_events.timeline_item_has_been_removed_at(timeline_item_index); self.result.item_removed = true; } @@ -1010,6 +1011,16 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { } /// Add a new event item in the timeline. + /// + /// # Safety + /// + /// This method is not marked as unsafe **but** it manipulates + /// [`TimelineMetadata::all_remote_events`]. 2 rules **must** be respected: + /// + /// 1. the remote event of the item being added **must** be present in + /// `all_remote_events`, + /// 2. the lastly added or updated remote event must be associated to the + /// timeline item being added here. fn add_item( &mut self, content: TimelineItemContent, @@ -1100,6 +1111,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { 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)); } Flow::Remote { @@ -1153,19 +1165,6 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { // will run to re-add the removed item } - // Local echoes that are pending should stick to the bottom, - // find the latest event that isn't that. - let latest_event_idx = self - .items - .iter() - .enumerate() - .rev() - .find_map(|(idx, item)| (!item.as_event()?.is_local_echo()).then_some(idx)); - - // Insert the next item after the latest event item that's not a - // pending local echo, or at the start if there is no such item. - let insert_idx = latest_event_idx.map_or(0, |idx| idx + 1); - trace!("Adding new remote timeline item after all non-pending events"); let new_item = match removed_event_item_id { // If a previous version of the same item (usually a local @@ -1175,14 +1174,49 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { None => self.meta.new_timeline_item(item), }; - // Keep push semantics, if we're inserting at the front or the back. - if insert_idx == self.items.len() { + // Local events are always at the bottom. Let's find the latest remote event + // and insert after it, otherwise, if there is no remote event, insert at 0. + let timeline_item_index = self + .items + .iter() + .enumerate() + .rev() + .find_map(|(timeline_item_index, timeline_item)| { + (!timeline_item.as_event()?.is_local_echo()) + .then_some(timeline_item_index + 1) + }) + .unwrap_or(0); + + // Try to keep precise insertion semantics here, in this exact order: + // + // * _push back_ when the new item is inserted after all items (the assumption + // being that this is the hot path, because most of the time new events + // come from the sync), + // * _push front_ when the new item is inserted at index 0, + // * _insert_ otherwise. + + if timeline_item_index == self.items.len() { + trace!("Adding new remote timeline item at the back"); self.items.push_back(new_item); - } else if insert_idx == 0 { + } else if timeline_item_index == 0 { + trace!("Adding new remote timeline item at the front"); self.items.push_front(new_item); } else { - self.items.insert(insert_idx, new_item); + trace!( + timeline_item_index, + "Adding new remote timeline item at specific index" + ); + self.items.insert(timeline_item_index, new_item); } + + 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 { From 3e9026a8b1d1217a7a5eff8dc9c7511c2b227082 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 4 Dec 2024 15:01:44 +0100 Subject: [PATCH 04/25] refactor(ui): Create `ObservableItems(Transaction)`. --- .../src/timeline/controller/mod.rs | 12 +- .../timeline/controller/observable_items.rs | 142 ++++++++++++++++++ .../src/timeline/controller/state.rs | 24 ++- .../src/timeline/day_dividers.rs | 39 ++--- .../src/timeline/event_handler.rs | 10 +- .../src/timeline/read_receipts.rs | 18 +-- 6 files changed, 190 insertions(+), 55 deletions(-) create mode 100644 crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs diff --git a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs index 13638ed6230..d6b55a4abf9 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs @@ -51,9 +51,14 @@ use tracing::{ debug, error, field, field::debug, info, info_span, instrument, trace, warn, Instrument as _, }; -pub(super) use self::state::{ - AllRemoteEvents, FullEventMeta, PendingEdit, PendingEditKind, TimelineMetadata, - TimelineNewItemPosition, TimelineState, TimelineStateTransaction, +#[cfg(test)] +pub(super) use self::observable_items::ObservableItems; +pub(super) use self::{ + observable_items::ObservableItemsTransaction, + state::{ + AllRemoteEvents, FullEventMeta, PendingEdit, PendingEditKind, TimelineMetadata, + TimelineNewItemPosition, TimelineState, TimelineStateTransaction, + }, }; use super::{ event_handler::TimelineEventKind, @@ -77,6 +82,7 @@ use crate::{ unable_to_decrypt_hook::UtdHookManager, }; +mod observable_items; mod state; /// Data associated to the current timeline focus. diff --git a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs new file mode 100644 index 00000000000..7c5c2e4482f --- /dev/null +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -0,0 +1,142 @@ +// Copyright 2024 The Matrix.org Foundation C.I.C. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::{ops::Deref, sync::Arc}; + +use eyeball_im::{ + ObservableVector, ObservableVectorEntries, ObservableVectorEntry, ObservableVectorTransaction, + ObservableVectorTransactionEntry, VectorSubscriber, +}; +use imbl::Vector; + +use super::TimelineItem; + +#[derive(Debug)] +pub struct ObservableItems { + items: ObservableVector>, +} + +impl ObservableItems { + pub fn new() -> Self { + Self { + // Upstream default capacity is currently 16, which is making + // sliding-sync tests with 20 events lag. This should still be + // small enough. + items: ObservableVector::with_capacity(32), + } + } + + pub fn is_empty(&self) -> bool { + self.items.is_empty() + } + + pub fn subscribe(&self) -> VectorSubscriber> { + self.items.subscribe() + } + + pub fn clone(&self) -> Vector> { + self.items.clone() + } + + pub fn transaction(&mut self) -> ObservableItemsTransaction<'_> { + ObservableItemsTransaction { items: self.items.transaction() } + } + + pub fn set( + &mut self, + timeline_item_index: usize, + timeline_item: Arc, + ) -> Arc { + self.items.set(timeline_item_index, timeline_item) + } + + pub fn entries(&mut self) -> ObservableVectorEntries<'_, Arc> { + self.items.entries() + } + + pub fn for_each(&mut self, f: F) + where + F: FnMut(ObservableVectorEntry<'_, Arc>), + { + self.items.for_each(f) + } +} + +// It's fine to deref to an immutable reference to `Vector`. +impl Deref for ObservableItems { + type Target = Vector>; + + fn deref(&self) -> &Self::Target { + &self.items + } +} + +#[derive(Debug)] +pub struct ObservableItemsTransaction<'observable_items> { + items: ObservableVectorTransaction<'observable_items, Arc>, +} + +impl<'observable_items> ObservableItemsTransaction<'observable_items> { + pub fn get(&self, timeline_item_index: usize) -> Option<&Arc> { + self.items.get(timeline_item_index) + } + + pub fn set( + &mut self, + timeline_item_index: usize, + timeline_item: Arc, + ) -> Arc { + self.items.set(timeline_item_index, timeline_item) + } + + pub fn remove(&mut self, timeline_item_index: usize) -> Arc { + self.items.remove(timeline_item_index) + } + + pub fn insert(&mut self, timeline_item_index: usize, timeline_item: Arc) { + self.items.insert(timeline_item_index, timeline_item); + } + + pub fn push_front(&mut self, timeline_item: Arc) { + self.items.push_front(timeline_item); + } + + pub fn push_back(&mut self, timeline_item: Arc) { + self.items.push_back(timeline_item); + } + + pub fn clear(&mut self) { + self.items.clear(); + } + + pub fn for_each(&mut self, f: F) + where + F: FnMut(ObservableVectorTransactionEntry<'_, 'observable_items, Arc>), + { + self.items.for_each(f) + } + + pub fn commit(self) { + self.items.commit() + } +} + +// It's fine to deref to an immutable reference to `Vector`. +impl Deref for ObservableItemsTransaction<'_> { + type Target = Vector>; + + fn deref(&self) -> &Self::Target { + &self.items + } +} diff --git a/crates/matrix-sdk-ui/src/timeline/controller/state.rs b/crates/matrix-sdk-ui/src/timeline/controller/state.rs index 92fb2f07500..72916c1c7bc 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/state.rs @@ -20,7 +20,7 @@ use std::{ sync::{Arc, RwLock}, }; -use eyeball_im::{ObservableVector, ObservableVectorTransaction, ObservableVectorTransactionEntry}; +use eyeball_im::ObservableVectorTransactionEntry; use itertools::Itertools as _; use matrix_sdk::{ deserialized_responses::SyncTimelineEvent, ring_buffer::RingBuffer, send_queue::SendHandle, @@ -45,7 +45,10 @@ use ruma::{ }; use tracing::{debug, instrument, trace, warn}; -use super::{HandleManyEventsResult, TimelineFocusKind, TimelineSettings}; +use super::{ + observable_items::{ObservableItems, ObservableItemsTransaction}, + HandleManyEventsResult, TimelineFocusKind, TimelineSettings, +}; use crate::{ events::SyncTimelineEventWithoutContent, timeline::{ @@ -90,7 +93,7 @@ impl From for TimelineItemPosition { #[derive(Debug)] pub(in crate::timeline) struct TimelineState { - pub items: ObservableVector>, + pub items: ObservableItems, pub meta: TimelineMetadata, /// The kind of focus of this timeline. @@ -107,10 +110,7 @@ impl TimelineState { is_room_encrypted: Option, ) -> Self { Self { - // Upstream default capacity is currently 16, which is making - // sliding-sync tests with 20 events lag. This should still be - // small enough. - items: ObservableVector::with_capacity(32), + items: ObservableItems::new(), meta: TimelineMetadata::new( own_user_id, room_version, @@ -330,6 +330,7 @@ impl TimelineState { pub(super) fn transaction(&mut self) -> TimelineStateTransaction<'_> { let items = self.items.transaction(); let meta = self.meta.clone(); + TimelineStateTransaction { items, previous_meta: &mut self.meta, @@ -342,7 +343,7 @@ impl TimelineState { pub(in crate::timeline) struct TimelineStateTransaction<'a> { /// A vector transaction over the items themselves. Holds temporary state /// until committed. - pub items: ObservableVectorTransaction<'a, Arc>, + pub items: ObservableItemsTransaction<'a>, /// A clone of the previous meta, that we're operating on during the /// transaction, and that will be committed to the previous meta location in @@ -1041,10 +1042,7 @@ impl TimelineMetadata { } /// Try to update the read marker item in the timeline. - pub(crate) fn update_read_marker( - &mut self, - items: &mut ObservableVectorTransaction<'_, Arc>, - ) { + pub(crate) fn update_read_marker(&mut self, items: &mut ObservableItemsTransaction<'_>) { let Some(fully_read_event) = &self.fully_read_event else { return }; trace!(?fully_read_event, "Updating read marker"); @@ -1359,7 +1357,7 @@ pub(crate) struct EventMeta { /// | 3 | content of `$ev3` | | /// | 4 | content of `$ev5` | | /// - /// Note the day divider that is a virtual item. Also note ``$ev4`` which is + /// Note the day divider that is a virtual item. Also note `$ev4` which is /// a reaction to `$ev2`. Finally note that `$ev1` is not rendered in /// the timeline. /// diff --git a/crates/matrix-sdk-ui/src/timeline/day_dividers.rs b/crates/matrix-sdk-ui/src/timeline/day_dividers.rs index cbd76e8cce7..6f02196c10a 100644 --- a/crates/matrix-sdk-ui/src/timeline/day_dividers.rs +++ b/crates/matrix-sdk-ui/src/timeline/day_dividers.rs @@ -17,13 +17,13 @@ use std::{fmt::Display, sync::Arc}; -use eyeball_im::ObservableVectorTransaction; use ruma::MilliSecondsSinceUnixEpoch; use tracing::{error, event_enabled, instrument, trace, warn, Level}; use super::{ - controller::TimelineMetadata, util::timestamp_to_date, TimelineItem, TimelineItemKind, - VirtualTimelineItem, + controller::{ObservableItemsTransaction, TimelineMetadata}, + util::timestamp_to_date, + TimelineItem, TimelineItemKind, VirtualTimelineItem, }; /// Algorithm ensuring that day dividers are adjusted correctly, according to @@ -81,11 +81,7 @@ impl DayDividerAdjuster { /// Ensures that date separators are properly inserted/removed when needs /// be. #[instrument(skip_all)] - pub fn run( - &mut self, - items: &mut ObservableVectorTransaction<'_, Arc>, - meta: &mut TimelineMetadata, - ) { + pub fn run(&mut self, items: &mut ObservableItemsTransaction<'_>, meta: &mut TimelineMetadata) { // We're going to record vector operations like inserting, replacing and // removing day dividers. Since we may remove or insert new items, // recorded offsets will change as we're iterating over the array. The @@ -284,11 +280,7 @@ impl DayDividerAdjuster { } } - fn process_ops( - &self, - items: &mut ObservableVectorTransaction<'_, Arc>, - meta: &mut TimelineMetadata, - ) { + fn process_ops(&self, items: &mut ObservableItemsTransaction<'_>, meta: &mut TimelineMetadata) { // Record the deletion offset. let mut offset = 0i64; // Remember what the maximum index was, so we can assert that it's @@ -366,7 +358,7 @@ impl DayDividerAdjuster { /// Returns a report if and only if there was at least one error. fn check_invariants<'a, 'o>( &mut self, - items: &'a ObservableVectorTransaction<'o, Arc>, + items: &'a ObservableItemsTransaction<'o>, initial_state: Option>>, ) -> Option> { let mut report = DayDividerInvariantsReport { @@ -512,7 +504,7 @@ struct DayDividerInvariantsReport<'a, 'o> { /// The operations that have been applied on the list. operations: Vec, /// Final state after inserting the day dividers. - final_state: &'a ObservableVectorTransaction<'o, Arc>, + final_state: &'a ObservableItemsTransaction<'o>, /// Errors encountered in the algorithm. errors: Vec, } @@ -608,10 +600,9 @@ enum DayDividerInsertError { #[cfg(test)] mod tests { use assert_matches2::assert_let; - use eyeball_im::ObservableVector; use ruma::{owned_event_id, owned_user_id, uint, MilliSecondsSinceUnixEpoch}; - use super::DayDividerAdjuster; + use super::{super::controller::ObservableItems, DayDividerAdjuster}; use crate::timeline::{ controller::TimelineMetadata, event_item::{EventTimelineItemKind, RemoteEventTimelineItem}, @@ -654,7 +645,7 @@ mod tests { #[test] fn test_no_trailing_day_divider() { - let mut items = ObservableVector::new(); + let mut items = ObservableItems::new(); let mut txn = items.transaction(); let mut meta = test_metadata(); @@ -688,7 +679,7 @@ mod tests { #[test] fn test_read_marker_in_between_event_and_day_divider() { - let mut items = ObservableVector::new(); + let mut items = ObservableItems::new(); let mut txn = items.transaction(); let mut meta = test_metadata(); @@ -720,7 +711,7 @@ mod tests { #[test] fn test_read_marker_in_between_day_dividers() { - let mut items = ObservableVector::new(); + let mut items = ObservableItems::new(); let mut txn = items.transaction(); let mut meta = test_metadata(); @@ -754,7 +745,7 @@ mod tests { #[test] fn test_remove_all_day_dividers() { - let mut items = ObservableVector::new(); + let mut items = ObservableItems::new(); let mut txn = items.transaction(); let mut meta = test_metadata(); @@ -784,7 +775,7 @@ mod tests { #[test] fn test_event_read_marker_spurious_day_divider() { - let mut items = ObservableVector::new(); + let mut items = ObservableItems::new(); let mut txn = items.transaction(); let mut meta = test_metadata(); @@ -810,7 +801,7 @@ mod tests { #[test] fn test_multiple_trailing_day_dividers() { - let mut items = ObservableVector::new(); + let mut items = ObservableItems::new(); let mut txn = items.transaction(); let mut meta = test_metadata(); @@ -834,7 +825,7 @@ mod tests { #[test] fn test_start_with_read_marker() { - let mut items = ObservableVector::new(); + let mut items = ObservableItems::new(); let mut txn = items.transaction(); let mut meta = test_metadata(); diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index bcb312f0274..e04b0399719 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -15,7 +15,7 @@ use std::sync::Arc; use as_variant::as_variant; -use eyeball_im::{ObservableVectorTransaction, ObservableVectorTransactionEntry}; +use eyeball_im::ObservableVectorTransactionEntry; use indexmap::IndexMap; use matrix_sdk::{ crypto::types::events::UtdCause, @@ -51,7 +51,9 @@ use ruma::{ use tracing::{debug, error, field::debug, info, instrument, trace, warn}; use super::{ - controller::{PendingEditKind, TimelineMetadata, TimelineStateTransaction}, + controller::{ + ObservableItemsTransaction, PendingEditKind, TimelineMetadata, TimelineStateTransaction, + }, day_dividers::DayDividerAdjuster, event_item::{ extract_bundled_edit_event_json, extract_poll_edit_content, extract_room_msg_edit_content, @@ -330,7 +332,7 @@ pub(super) struct HandleEventResult { /// existing timeline item, transforming that item or creating a new one, /// updating the reactive Vec). pub(super) struct TimelineEventHandler<'a, 'o> { - items: &'a mut ObservableVectorTransaction<'o, Arc>, + items: &'a mut ObservableItemsTransaction<'o>, meta: &'a mut TimelineMetadata, ctx: TimelineEventContext, result: HandleEventResult, @@ -1243,7 +1245,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { /// After updating the timeline item `new_item` which id is /// `target_event_id`, update other items that are responses to this item. fn maybe_update_responses( - items: &mut ObservableVectorTransaction<'_, Arc>, + items: &mut ObservableItemsTransaction<'_>, target_event_id: &EventId, new_item: &EventTimelineItem, ) { diff --git a/crates/matrix-sdk-ui/src/timeline/read_receipts.rs b/crates/matrix-sdk-ui/src/timeline/read_receipts.rs index a12ebbb0967..10040230011 100644 --- a/crates/matrix-sdk-ui/src/timeline/read_receipts.rs +++ b/crates/matrix-sdk-ui/src/timeline/read_receipts.rs @@ -12,9 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::{cmp::Ordering, collections::HashMap, sync::Arc}; +use std::{cmp::Ordering, collections::HashMap}; -use eyeball_im::ObservableVectorTransaction; use futures_core::Stream; use indexmap::IndexMap; use ruma::{ @@ -27,7 +26,8 @@ use tracing::{debug, error, warn}; use super::{ controller::{ - AllRemoteEvents, FullEventMeta, TimelineMetadata, TimelineState, TimelineStateTransaction, + AllRemoteEvents, FullEventMeta, ObservableItemsTransaction, TimelineMetadata, + TimelineState, TimelineStateTransaction, }, traits::RoomDataProvider, util::{rfind_event_by_id, RelativePosition}, @@ -100,7 +100,7 @@ impl ReadReceipts { new_receipt: FullReceipt<'_>, is_own_user_id: bool, all_events: &AllRemoteEvents, - timeline_items: &mut ObservableVectorTransaction<'_, Arc>, + timeline_items: &mut ObservableItemsTransaction<'_>, ) { // Get old receipt. let old_receipt = self.get_latest(new_receipt.user_id, &new_receipt.receipt_type); @@ -284,11 +284,7 @@ struct ReadReceiptTimelineUpdate { impl ReadReceiptTimelineUpdate { /// Remove the old receipt from the corresponding timeline item. - fn remove_old_receipt( - &self, - items: &mut ObservableVectorTransaction<'_, Arc>, - user_id: &UserId, - ) { + fn remove_old_receipt(&self, items: &mut ObservableItemsTransaction<'_>, user_id: &UserId) { let Some(event_id) = &self.old_event_id else { // Nothing to do. return; @@ -319,7 +315,7 @@ impl ReadReceiptTimelineUpdate { /// Add the new receipt to the corresponding timeline item. fn add_new_receipt( self, - items: &mut ObservableVectorTransaction<'_, Arc>, + items: &mut ObservableItemsTransaction<'_>, user_id: OwnedUserId, receipt: Receipt, ) { @@ -348,7 +344,7 @@ impl ReadReceiptTimelineUpdate { /// Apply this update to the timeline. fn apply( self, - items: &mut ObservableVectorTransaction<'_, Arc>, + items: &mut ObservableItemsTransaction<'_>, user_id: OwnedUserId, receipt: Receipt, ) { From 88cd5b4a3a8e0c1d360b4e44535d3fd749c1dd9d Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 4 Dec 2024 16:46:14 +0100 Subject: [PATCH 05/25] 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 d6b55a4abf9..0a58951b6a2 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 7c5c2e4482f..fc0a8aa6fbb 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 72916c1c7bc..925e1d45d36 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 6f02196c10a..05385723fb0 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 e04b0399719..b8e6457dd98 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 10040230011..8cc0c8c2147 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; } From cefd1163f82aaafbf9481b35f67fc088ede69dd7 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 4 Dec 2024 16:54:53 +0100 Subject: [PATCH 06/25] doc(ui): Add more documentation. --- .../timeline/controller/observable_items.rs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) 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 fc0a8aa6fbb..5133bc798ba 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -28,6 +28,9 @@ use ruma::EventId; use super::{state::EventMeta, TimelineItem}; +/// An `ObservableItems` is a type similar to +/// [`ObservableVector>`] except the API is limited and, +/// internally, maintains the mapping between remote events and timeline items. #[derive(Debug)] pub struct ObservableItems { /// All timeline items. @@ -45,6 +48,7 @@ pub struct ObservableItems { } impl ObservableItems { + /// Create an empty `ObservableItems`. pub fn new() -> Self { Self { // Upstream default capacity is currently 16, which is making @@ -55,22 +59,29 @@ impl ObservableItems { } } + /// Get a reference to all remote events. pub fn all_remote_events(&self) -> &AllRemoteEvents { &self.all_remote_events } + /// Check whether there is timeline items. pub fn is_empty(&self) -> bool { self.items.is_empty() } + /// Subscribe to timeline item updates. pub fn subscribe(&self) -> VectorSubscriber> { self.items.subscribe() } + /// Get a clone of all timeline items. + /// + /// Note that it doesn't clone `Self`, only the inner timeline items. pub fn clone(&self) -> Vector> { self.items.clone() } + /// Start a new transaction to make multiple updates as one unit. pub fn transaction(&mut self) -> ObservableItemsTransaction<'_> { ObservableItemsTransaction { items: self.items.transaction(), @@ -78,6 +89,12 @@ impl ObservableItems { } } + /// Replace the timeline item at position `timeline_item_index` by + /// `timeline_item`. + /// + /// # Panics + /// + /// Panics if `timeline_item_index > total_number_of_timeline_items`. pub fn set( &mut self, timeline_item_index: usize, @@ -86,10 +103,13 @@ impl ObservableItems { self.items.set(timeline_item_index, timeline_item) } + /// Get an iterator over all the entries in this `ObservableItems`. pub fn entries(&mut self) -> ObservableVectorEntries<'_, Arc> { self.items.entries() } + /// Call the given closure for every element in this `ObservableItems`, + /// with an entry struct that allows updating or removing that element. pub fn for_each(&mut self, f: F) where F: FnMut(ObservableVectorEntry<'_, Arc>), From 6175298a2b002cd8cce108aabb277369270590c1 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 4 Dec 2024 16:56:35 +0100 Subject: [PATCH 07/25] refactor(ui): Rename `ObservableItems::set` to `replace`. This patch renames `ObservableItems(Transaction)::set` to `replace`, it conveys the semantics a bit better for new comers. --- .../src/timeline/controller/mod.rs | 18 +++++++++--------- .../timeline/controller/observable_items.rs | 4 ++-- .../src/timeline/controller/state.rs | 2 +- .../src/timeline/day_dividers.rs | 2 +- .../src/timeline/event_handler.rs | 19 ++++++++++--------- .../src/timeline/read_receipts.rs | 6 +++--- 6 files changed, 26 insertions(+), 25 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs index 0a58951b6a2..d75aae1b3a9 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs @@ -597,7 +597,7 @@ impl TimelineController

{ if reaction_info.is_some() { let new_item = item.with_reactions(reactions); - state.items.set(item_pos, new_item); + state.items.replace(item_pos, new_item); } else { warn!("reaction is missing on the item, not removing it locally, but sending redaction."); } @@ -621,7 +621,7 @@ impl TimelineController

{ .or_default() .insert(user_id.to_owned(), reaction_info); let new_item = item.with_reactions(reactions); - state.items.set(item_pos, new_item); + state.items.replace(item_pos, new_item); } else { warn!("couldn't find item to re-add reaction anymore; maybe it's been redacted?"); } @@ -817,7 +817,7 @@ impl TimelineController

{ { trace!("updated reaction status to sent"); entry.status = ReactionStatus::RemoteToRemote(event_id.to_owned()); - txn.items.set(item_pos, event_item.with_reactions(reactions)); + txn.items.replace(item_pos, event_item.with_reactions(reactions)); txn.commit(); return; } @@ -863,7 +863,7 @@ impl TimelineController

{ } let new_item = item.with_inner_kind(local_item.with_send_state(send_state)); - txn.items.set(idx, new_item); + txn.items.replace(idx, new_item); txn.commit(); } @@ -910,7 +910,7 @@ impl TimelineController

{ let mut reactions = item.reactions().clone(); if reactions.remove_reaction(&full_key.sender, &full_key.key).is_some() { let updated_item = item.with_reactions(reactions); - state.items.set(idx, updated_item); + state.items.replace(idx, updated_item); } else { warn!( "missing reaction {} for sender {} on timeline item", @@ -967,7 +967,7 @@ impl TimelineController

{ prev_item.internal_id.to_owned(), ); - txn.items.set(idx, new_item); + txn.items.replace(idx, new_item); // This doesn't change the original sending time, so there's no need to adjust // day dividers. @@ -1322,7 +1322,7 @@ impl TimelineController

{ trace!("Adding local reaction to local echo"); let new_item = item.with_reactions(reactions); - state.items.set(item_pos, new_item); + state.items.replace(item_pos, new_item); // Add it to the reaction map, so we can discard it later if needs be. state.meta.reactions.map.insert( @@ -1462,7 +1462,7 @@ impl TimelineController { event, }), )); - state.items.set(index, TimelineItem::new(item, internal_id)); + state.items.replace(index, TimelineItem::new(item, internal_id)); Ok(()) } @@ -1594,7 +1594,7 @@ async fn fetch_replied_to_event( let event_item = item.with_content(TimelineItemContent::Message(reply), None); let new_timeline_item = TimelineItem::new(event_item, internal_id); - state.items.set(index, new_timeline_item); + state.items.replace(index, new_timeline_item); // Don't hold the state lock while the network request is made drop(state); 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 5133bc798ba..804a35cca71 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -95,7 +95,7 @@ impl ObservableItems { /// # Panics /// /// Panics if `timeline_item_index > total_number_of_timeline_items`. - pub fn set( + pub fn replace( &mut self, timeline_item_index: usize, timeline_item: Arc, @@ -161,7 +161,7 @@ impl<'observable_items> ObservableItemsTransaction<'observable_items> { self.all_remote_events.get_by_event_id_mut(event_id) } - pub fn set( + pub fn replace( &mut self, timeline_item_index: usize, timeline_item: Arc, diff --git a/crates/matrix-sdk-ui/src/timeline/controller/state.rs b/crates/matrix-sdk-ui/src/timeline/controller/state.rs index 925e1d45d36..b66e29579f6 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/state.rs @@ -788,7 +788,7 @@ impl TimelineStateTransaction<'_> { // Replace the existing item with a new version with the right encryption flag let item = item.with_kind(cloned_event); - self.items.set(idx, item); + self.items.replace(idx, item); } } } diff --git a/crates/matrix-sdk-ui/src/timeline/day_dividers.rs b/crates/matrix-sdk-ui/src/timeline/day_dividers.rs index 05385723fb0..ba9cd9175b3 100644 --- a/crates/matrix-sdk-ui/src/timeline/day_dividers.rs +++ b/crates/matrix-sdk-ui/src/timeline/day_dividers.rs @@ -330,7 +330,7 @@ impl DayDividerAdjuster { unique_id.to_owned(), ); - items.set(at, item); + items.replace(at, item); max_i = i; } diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index b8e6457dd98..edc3d8c9c84 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -578,7 +578,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { Self::maybe_update_responses(self.items, &replacement.event_id, &new_item); // Update the event itself. - self.items.set(item_pos, TimelineItem::new(new_item, internal_id)); + self.items.replace(item_pos, TimelineItem::new(new_item, internal_id)); self.result.items_updated += 1; } } else if let Flow::Remote { position, raw_event, .. } = &self.ctx.flow { @@ -732,7 +732,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { }, ); - self.items.set(idx, event_item.with_reactions(reactions)); + self.items.replace(idx, event_item.with_reactions(reactions)); self.result.items_updated += 1; } else { @@ -796,7 +796,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { }; trace!("Applying poll start edit."); - self.items.set(item_pos, TimelineItem::new(new_item, item.internal_id.to_owned())); + self.items.replace(item_pos, TimelineItem::new(new_item, item.internal_id.to_owned())); self.result.items_updated += 1; } @@ -900,7 +900,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { ); trace!("Adding poll response."); - self.items.set(item_pos, TimelineItem::new(new_item, item.internal_id.to_owned())); + self.items.replace(item_pos, TimelineItem::new(new_item, item.internal_id.to_owned())); self.result.items_updated += 1; } @@ -919,7 +919,8 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { let new_item = item.with_content(TimelineItemContent::Poll(poll_state), None); trace!("Ending poll."); - self.items.set(item_pos, TimelineItem::new(new_item, item.internal_id.to_owned())); + self.items + .replace(item_pos, TimelineItem::new(new_item, item.internal_id.to_owned())); self.result.items_updated += 1; } Err(_) => { @@ -961,7 +962,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { // the replied-to event there as well. Self::maybe_update_responses(self.items, &redacted, &new_item); - self.items.set(idx, TimelineItem::new(new_item, internal_id)); + self.items.replace(idx, TimelineItem::new(new_item, internal_id)); self.result.items_updated += 1; } } else { @@ -1002,7 +1003,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { let mut reactions = item.reactions.clone(); if reactions.remove_reaction(&sender, &key).is_some() { trace!("Removing reaction"); - self.items.set(item_pos, item.with_reactions(reactions)); + self.items.replace(item_pos, item.with_reactions(reactions)); self.result.items_updated += 1; return true; } @@ -1153,7 +1154,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { // If the old item is the last one and no day divider // changes need to happen, replace and return early. trace!(idx, "Replacing existing event"); - self.items.set(idx, TimelineItem::new(item, old_item_id.to_owned())); + self.items.replace(idx, TimelineItem::new(item, old_item_id.to_owned())); return; } @@ -1228,7 +1229,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { Self::maybe_update_responses(self.items, decrypted_event_id, &item); let internal_id = self.items[*idx].internal_id.clone(); - self.items.set(*idx, TimelineItem::new(item, internal_id)); + self.items.replace(*idx, TimelineItem::new(item, internal_id)); } } diff --git a/crates/matrix-sdk-ui/src/timeline/read_receipts.rs b/crates/matrix-sdk-ui/src/timeline/read_receipts.rs index 8cc0c8c2147..30226de15ca 100644 --- a/crates/matrix-sdk-ui/src/timeline/read_receipts.rs +++ b/crates/matrix-sdk-ui/src/timeline/read_receipts.rs @@ -307,7 +307,7 @@ impl ReadReceiptTimelineUpdate { receipt doesn't have a receipt for the user" ); } - items.set(receipt_pos, TimelineItem::new(event_item, event_item_id)); + items.replace(receipt_pos, TimelineItem::new(event_item, event_item_id)); } else { warn!("received a read receipt for a local item, this should not be possible"); } @@ -336,7 +336,7 @@ impl ReadReceiptTimelineUpdate { if let Some(remote_event_item) = event_item.as_remote_mut() { remote_event_item.read_receipts.insert(user_id, receipt); - items.set(receipt_pos, TimelineItem::new(event_item, event_item_id)); + items.replace(receipt_pos, TimelineItem::new(event_item, event_item_id)); } else { warn!("received a read receipt for a local item, this should not be possible"); } @@ -495,7 +495,7 @@ impl TimelineStateTransaction<'_> { } remote_prev_event_item.read_receipts = read_receipts; - self.items.set(prev_item_pos, TimelineItem::new(prev_event_item, prev_event_item_id)); + self.items.replace(prev_item_pos, TimelineItem::new(prev_event_item, prev_event_item_id)); } } From 645840499fd88fa94ed2d53d4859f94679950ed9 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 4 Dec 2024 17:10:52 +0100 Subject: [PATCH 08/25] refactor(ui): Create `ObservableItemsEntries` and `ObservableItemsEntry`. This patch creates `ObservableItemsEntries` and particularly `ObservableItemsEntry` that wraps the equivalent `ObservableVectorEntries` and `ObservableVectorEntry` with the noticeable difference that `ObservableItemsEntry` does **not** expose the `remove` method. It only exposes `replace` (which is a renaming of `set`). --- .../src/timeline/controller/mod.rs | 14 +++---- .../timeline/controller/observable_items.rs | 39 ++++++++++++++++--- 2 files changed, 41 insertions(+), 12 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs index d75aae1b3a9..31c638befed 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs @@ -15,7 +15,7 @@ use std::{collections::BTreeSet, fmt, sync::Arc}; use as_variant::as_variant; -use eyeball_im::{ObservableVectorEntry, VectorDiff}; +use eyeball_im::VectorDiff; use eyeball_im_util::vector::VectorObserverExt; use futures_core::Stream; use imbl::Vector; @@ -54,7 +54,7 @@ use tracing::{ #[cfg(test)] pub(super) use self::observable_items::ObservableItems; pub(super) use self::{ - observable_items::{AllRemoteEvents, ObservableItemsTransaction}, + observable_items::{AllRemoteEvents, ObservableItemsEntry, ObservableItemsTransaction}, state::{ FullEventMeta, PendingEdit, PendingEditKind, TimelineMetadata, TimelineNewItemPosition, TimelineState, TimelineStateTransaction, @@ -1134,7 +1134,7 @@ impl TimelineController

{ let new_item = entry.with_kind(TimelineItemKind::Event( event_item.with_sender_profile(profile_state.clone()), )); - ObservableVectorEntry::set(&mut entry, new_item); + ObservableItemsEntry::replace(&mut entry, new_item); } }); } @@ -1160,7 +1160,7 @@ impl TimelineController

{ let updated_item = event_item.with_sender_profile(TimelineDetails::Ready(profile)); let new_item = entry.with_kind(updated_item); - ObservableVectorEntry::set(&mut entry, new_item); + ObservableItemsEntry::replace(&mut entry, new_item); } None => { if !event_item.sender_profile().is_unavailable() { @@ -1168,7 +1168,7 @@ impl TimelineController

{ let updated_item = event_item.with_sender_profile(TimelineDetails::Unavailable); let new_item = entry.with_kind(updated_item); - ObservableVectorEntry::set(&mut entry, new_item); + ObservableItemsEntry::replace(&mut entry, new_item); } else { debug!(event_id, transaction_id, "Profile already marked unavailable"); } @@ -1204,7 +1204,7 @@ impl TimelineController

{ let updated_item = event_item.with_sender_profile(TimelineDetails::Ready(profile)); let new_item = entry.with_kind(updated_item); - ObservableVectorEntry::set(&mut entry, new_item); + ObservableItemsEntry::replace(&mut entry, new_item); } } None => { @@ -1213,7 +1213,7 @@ impl TimelineController

{ let updated_item = event_item.with_sender_profile(TimelineDetails::Unavailable); let new_item = entry.with_kind(updated_item); - ObservableVectorEntry::set(&mut entry, new_item); + ObservableItemsEntry::replace(&mut entry, new_item); } else { debug!(event_id, transaction_id, "Profile already marked unavailable"); } 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 804a35cca71..9b7d8ee216f 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -104,17 +104,17 @@ impl ObservableItems { } /// Get an iterator over all the entries in this `ObservableItems`. - pub fn entries(&mut self) -> ObservableVectorEntries<'_, Arc> { - self.items.entries() + pub fn entries(&mut self) -> ObservableItemsEntries<'_> { + ObservableItemsEntries(self.items.entries()) } /// Call the given closure for every element in this `ObservableItems`, /// with an entry struct that allows updating or removing that element. - pub fn for_each(&mut self, f: F) + pub fn for_each(&mut self, mut f: F) where - F: FnMut(ObservableVectorEntry<'_, Arc>), + F: FnMut(ObservableItemsEntry<'_>), { - self.items.for_each(f) + self.items.for_each(|entry| f(ObservableItemsEntry(entry))) } } @@ -127,6 +127,35 @@ impl Deref for ObservableItems { } } +/// An “iterator“ that yields entries into an `ObservableItems`. +pub struct ObservableItemsEntries<'a>(ObservableVectorEntries<'a, Arc>); + +impl<'a> ObservableItemsEntries<'a> { + /// Advance this iterator, yielding an `ObservableItemsEntry` for the next + /// item in the timeline, or `None` if all items have been visited. + pub fn next(&mut self) -> Option> { + self.0.next().map(ObservableItemsEntry) + } +} + +/// A handle to a single timeline item in an `ObservableItems`. +pub struct ObservableItemsEntry<'a>(ObservableVectorEntry<'a, Arc>); + +impl<'a> ObservableItemsEntry<'a> { + /// Replace the timeline item by `timeline_item`. + pub fn replace(this: &mut Self, timeline_item: Arc) -> Arc { + ObservableVectorEntry::set(&mut this.0, timeline_item) + } +} + +impl Deref for ObservableItemsEntry<'_> { + type Target = Arc; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + #[derive(Debug)] pub struct ObservableItemsTransaction<'observable_items> { items: ObservableVectorTransaction<'observable_items, Arc>, From 1ced997a751211ffdc6c7106298453d9c1cdbd76 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 4 Dec 2024 17:20:32 +0100 Subject: [PATCH 09/25] doc(ui): Add more documentation for `ObservableItemsTransaction`. --- .../timeline/controller/observable_items.rs | 52 ++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) 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 9b7d8ee216f..42c0d4dede6 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -109,7 +109,7 @@ impl ObservableItems { } /// Call the given closure for every element in this `ObservableItems`, - /// with an entry struct that allows updating or removing that element. + /// with an entry struct that allows updating that element. pub fn for_each(&mut self, mut f: F) where F: FnMut(ObservableItemsEntry<'_>), @@ -156,6 +156,12 @@ impl Deref for ObservableItemsEntry<'_> { } } +/// A transaction that allows making multiple updates to an `ObservableItems` as +/// an atomic unit. +/// +/// For updates from the transaction to have affect, it has to be finalized with +/// [`Self::commit`]. If the transaction is dropped without that method being +/// called, the updates will be discarded. #[derive(Debug)] pub struct ObservableItemsTransaction<'observable_items> { items: ObservableVectorTransaction<'observable_items, Arc>, @@ -163,26 +169,38 @@ pub struct ObservableItemsTransaction<'observable_items> { } impl<'observable_items> ObservableItemsTransaction<'observable_items> { + /// Get a referene to the timeline index at position `timeline_item_index`. pub fn get(&self, timeline_item_index: usize) -> Option<&Arc> { self.items.get(timeline_item_index) } + /// Get a reference to all remote events. pub fn all_remote_events(&self) -> &AllRemoteEvents { &self.all_remote_events } + /// Remove a remote event at position `event_index`. + /// + /// Not to be confused with removing a timeline item! pub fn remove_remote_event(&mut self, event_index: usize) -> Option { self.all_remote_events.remove(event_index) } + /// Push a new remote event at the front of all remote events. + /// + /// Not to be confused with pushing front a timeline item! pub fn push_front_remote_event(&mut self, event_meta: EventMeta) { self.all_remote_events.push_front(event_meta); } + /// Push a new remote event at the back of all remote events. + /// + /// Not to be confused with pushing back a timeline item! pub fn push_back_remote_event(&mut self, event_meta: EventMeta) { self.all_remote_events.push_back(event_meta); } + /// Get a remote event by using an event ID. pub fn get_remote_event_by_event_id_mut( &mut self, event_id: &EventId, @@ -190,6 +208,8 @@ impl<'observable_items> ObservableItemsTransaction<'observable_items> { self.all_remote_events.get_by_event_id_mut(event_id) } + /// Replace a timeline item at position `timeline_item_index` by + /// `timeline_item`. pub fn replace( &mut self, timeline_item_index: usize, @@ -198,6 +218,7 @@ impl<'observable_items> ObservableItemsTransaction<'observable_items> { self.items.set(timeline_item_index, timeline_item) } + /// Remove a timeline item at position `timeline_item_index`. pub fn remove(&mut self, timeline_item_index: usize) -> Arc { let removed_timeline_item = self.items.remove(timeline_item_index); self.all_remote_events.timeline_item_has_been_removed_at(timeline_item_index); @@ -205,6 +226,14 @@ impl<'observable_items> ObservableItemsTransaction<'observable_items> { removed_timeline_item } + /// Insert a new `timeline_item` at position `timeline_item_index`, with an + /// optionally associated `event_index`. + /// + /// If `event_index` is `Some(_)`, it means `timeline_item_index` has an + /// associated remote event (at position `event_index`) that maps to it. + /// Otherwise, if it is `None`, it means there is no remote event associated + /// to it; that's the case for virtual timeline item for example. See + /// [`EventMeta::timeline_item_index`] to learn more. pub fn insert( &mut self, timeline_item_index: usize, @@ -215,22 +244,41 @@ impl<'observable_items> ObservableItemsTransaction<'observable_items> { self.all_remote_events.timeline_item_has_been_inserted_at(timeline_item_index, event_index); } + /// Push a new `timeline_item` at position 0, with an optionally associated + /// `event_index`. + /// + /// If `event_index` is `Some(_)`, it means `timeline_item_index` has an + /// associated remote event (at position `event_index`) that maps to it. + /// Otherwise, if it is `None`, it means there is no remote event associated + /// to it; that's the case for virtual timeline item for example. See + /// [`EventMeta::timeline_item_index`] to learn more. 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); } + /// Push a new `timeline_item` at position `len() - 1`, with an optionally + /// associated `event_index`. + /// + /// If `event_index` is `Some(_)`, it means `timeline_item_index` has an + /// associated remote event (at position `event_index`) that maps to it. + /// Otherwise, if it is `None`, it means there is no remote event associated + /// to it; that's the case for virtual timeline item for example. See + /// [`EventMeta::timeline_item_index`] to learn more. 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); } + /// Clear all timeline items and all remote events. pub fn clear(&mut self) { self.items.clear(); self.all_remote_events.clear(); } + /// Call the given closure for every element in this `ObservableItems`, + /// with an entry struct that allows updating that element. pub fn for_each(&mut self, f: F) where F: FnMut(ObservableVectorTransactionEntry<'_, 'observable_items, Arc>), @@ -238,6 +286,8 @@ impl<'observable_items> ObservableItemsTransaction<'observable_items> { self.items.for_each(f) } + /// Commit this transaction, persisting the changes and notifying + /// subscribers. pub fn commit(self) { self.items.commit() } From c997074ecbdb2678872115021c0d325b3cbfef53 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 4 Dec 2024 17:39:38 +0100 Subject: [PATCH 10/25] refactor(ui): Create `ObservableItemsTransactionEntry`. This patch creates `ObservableItemsTransactionEntry` that mimics `ObservableVectorTransactionEntry`. The differences are `set` is renamed `replace`, and `remove` is unsafe (because I failed to update `AllRemoteEvents` in this method due to the borrow checker). --- .../src/timeline/controller/mod.rs | 5 ++- .../timeline/controller/observable_items.rs | 37 +++++++++++++++++-- .../src/timeline/controller/state.rs | 13 +++++-- .../src/timeline/event_handler.rs | 28 +++++--------- 4 files changed, 58 insertions(+), 25 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs index 31c638befed..9a22bb91753 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs @@ -54,7 +54,10 @@ use tracing::{ #[cfg(test)] pub(super) use self::observable_items::ObservableItems; pub(super) use self::{ - observable_items::{AllRemoteEvents, ObservableItemsEntry, ObservableItemsTransaction}, + observable_items::{ + AllRemoteEvents, ObservableItemsEntry, ObservableItemsTransaction, + ObservableItemsTransactionEntry, + }, state::{ FullEventMeta, PendingEdit, PendingEditKind, TimelineMetadata, TimelineNewItemPosition, TimelineState, TimelineStateTransaction, 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 42c0d4dede6..799e88785b4 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -279,11 +279,11 @@ impl<'observable_items> ObservableItemsTransaction<'observable_items> { /// Call the given closure for every element in this `ObservableItems`, /// with an entry struct that allows updating that element. - pub fn for_each(&mut self, f: F) + pub fn for_each(&mut self, mut f: F) where - F: FnMut(ObservableVectorTransactionEntry<'_, 'observable_items, Arc>), + F: FnMut(ObservableItemsTransactionEntry<'_, 'observable_items>), { - self.items.for_each(f) + self.items.for_each(|entry| f(ObservableItemsTransactionEntry(entry))) } /// Commit this transaction, persisting the changes and notifying @@ -302,6 +302,37 @@ impl Deref for ObservableItemsTransaction<'_> { } } +/// A handle to a single timeline item in an `ObservableItemsTransaction`. +pub struct ObservableItemsTransactionEntry<'a, 'observable_items>( + ObservableVectorTransactionEntry<'a, 'observable_items, Arc>, +); + +impl<'a, 'o> ObservableItemsTransactionEntry<'a, 'o> { + /// Replace the timeline item by `timeline_item`. + pub fn replace(this: &mut Self, timeline_item: Arc) -> Arc { + ObservableVectorTransactionEntry::set(&mut this.0, timeline_item) + } + + /// Remove this timeline item. + /// + /// # Safety + /// + /// This method doesn't update `AllRemoteEvents`. Be sure that the caller + /// doesn't break the mapping between remote events and timeline items. See + /// [`EventMeta::timeline_item_index`] to learn more. + pub unsafe fn remove(this: Self) { + ObservableVectorTransactionEntry::remove(this.0); + } +} + +impl Deref for ObservableItemsTransactionEntry<'_, '_> { + type Target = Arc; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + /// A type for all remote events. /// /// Having this type helps to know exactly which parts of the code and how they diff --git a/crates/matrix-sdk-ui/src/timeline/controller/state.rs b/crates/matrix-sdk-ui/src/timeline/controller/state.rs index b66e29579f6..9df9ffb8e2f 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/state.rs @@ -19,7 +19,6 @@ use std::{ sync::{Arc, RwLock}, }; -use eyeball_im::ObservableVectorTransactionEntry; use itertools::Itertools as _; use matrix_sdk::{ deserialized_responses::SyncTimelineEvent, ring_buffer::RingBuffer, send_queue::SendHandle, @@ -45,7 +44,10 @@ use ruma::{ use tracing::{debug, instrument, trace, warn}; use super::{ - observable_items::{AllRemoteEvents, ObservableItems, ObservableItemsTransaction}, + observable_items::{ + AllRemoteEvents, ObservableItems, ObservableItemsTransaction, + ObservableItemsTransactionEntry, + }, HandleManyEventsResult, TimelineFocusKind, TimelineSettings, }; use crate::{ @@ -655,7 +657,12 @@ impl TimelineStateTransaction<'_> { // Remove all remote events and the read marker self.items.for_each(|entry| { if entry.is_remote_event() || entry.is_read_marker() { - ObservableVectorTransactionEntry::remove(entry); + // SAFETY: this method removes all events except local events. Local events + // don't have a mapping from remote events to timeline items because… well… they + // are local events, not remove events. + unsafe { + ObservableItemsTransactionEntry::remove(entry); + } } }); diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index edc3d8c9c84..9c8a63f203b 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -15,7 +15,6 @@ use std::sync::Arc; use as_variant::as_variant; -use eyeball_im::ObservableVectorTransactionEntry; use indexmap::IndexMap; use matrix_sdk::{ crypto::types::events::UtdCause, @@ -52,30 +51,23 @@ use tracing::{debug, error, field::debug, info, instrument, trace, warn}; use super::{ controller::{ - ObservableItemsTransaction, PendingEditKind, TimelineMetadata, TimelineStateTransaction, + ObservableItemsTransaction, ObservableItemsTransactionEntry, PendingEdit, PendingEditKind, + TimelineMetadata, TimelineStateTransaction, }, day_dividers::DayDividerAdjuster, event_item::{ extract_bundled_edit_event_json, extract_poll_edit_content, extract_room_msg_edit_content, AnyOtherFullStateEventContent, EventSendState, EventTimelineItemKind, - LocalEventTimelineItem, PollState, Profile, ReactionsByKeyBySender, RemoteEventOrigin, - RemoteEventTimelineItem, TimelineEventItemId, + LocalEventTimelineItem, PollState, Profile, ReactionInfo, ReactionStatus, + ReactionsByKeyBySender, RemoteEventOrigin, RemoteEventTimelineItem, TimelineEventItemId, }, - reactions::FullReactionKey, + reactions::{FullReactionKey, PendingReaction}, + traits::RoomDataProvider, util::{rfind_event_by_id, rfind_event_item}, - EventTimelineItem, InReplyToDetails, OtherState, Sticker, TimelineDetails, TimelineItem, - TimelineItemContent, -}; -use crate::{ - events::SyncTimelineEventWithoutContent, - timeline::{ - controller::PendingEdit, - event_item::{ReactionInfo, ReactionStatus}, - reactions::PendingReaction, - traits::RoomDataProvider, - RepliedToEvent, - }, + EventTimelineItem, InReplyToDetails, OtherState, RepliedToEvent, Sticker, TimelineDetails, + TimelineItem, TimelineItemContent, }; +use crate::events::SyncTimelineEventWithoutContent; /// When adding an event, useful information related to the source of the event. pub(super) enum Flow { @@ -1262,7 +1254,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { TimelineItemContent::Message(message.with_in_reply_to(in_reply_to)); let new_reply_item = entry.with_kind(event_item.with_content(new_reply_content, None)); - ObservableVectorTransactionEntry::set(&mut entry, new_reply_item); + ObservableItemsTransactionEntry::replace(&mut entry, new_reply_item); } }); } From 482e390aa3c2fb4fa603df0140212546289ad6fe Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 4 Dec 2024 17:42:32 +0100 Subject: [PATCH 11/25] doc(ui): Add more documentation for `AllRemoteEvents`. --- .../src/timeline/controller/observable_items.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) 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 799e88785b4..a78106f8337 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -169,7 +169,7 @@ pub struct ObservableItemsTransaction<'observable_items> { } impl<'observable_items> ObservableItemsTransaction<'observable_items> { - /// Get a referene to the timeline index at position `timeline_item_index`. + /// Get a reference to the timeline index at position `timeline_item_index`. pub fn get(&self, timeline_item_index: usize) -> Option<&Arc> { self.items.get(timeline_item_index) } @@ -429,6 +429,10 @@ impl AllRemoteEvents { } } + /// Notify that a timeline item has been inserted at + /// `new_timeline_item_index`. If `event_index` is `Some(_)`, it means the + /// remote event at `event_index` must be mapped to + /// `new_timeline_item_index`. fn timeline_item_has_been_inserted_at( &mut self, new_timeline_item_index: usize, @@ -443,6 +447,9 @@ impl AllRemoteEvents { } } + /// Notify that a timeline item has been removed at + /// `new_timeline_item_index`. If `event_index` is `Some(_)`, it means the + /// remote event at `event_index` must be unmapped. 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; From 581383eb2903e9ea98da519f8869513b2db50ed3 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 4 Dec 2024 19:43:54 +0100 Subject: [PATCH 12/25] chore: Make Clippy happy. --- .../src/timeline/controller/observable_items.rs | 8 ++++---- crates/matrix-sdk-ui/src/timeline/controller/state.rs | 4 ++-- crates/matrix-sdk-ui/src/timeline/read_receipts.rs | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) 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 a78106f8337..6f258edf545 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -130,7 +130,7 @@ impl Deref for ObservableItems { /// An “iterator“ that yields entries into an `ObservableItems`. pub struct ObservableItemsEntries<'a>(ObservableVectorEntries<'a, Arc>); -impl<'a> ObservableItemsEntries<'a> { +impl ObservableItemsEntries<'_> { /// Advance this iterator, yielding an `ObservableItemsEntry` for the next /// item in the timeline, or `None` if all items have been visited. pub fn next(&mut self) -> Option> { @@ -141,7 +141,7 @@ impl<'a> ObservableItemsEntries<'a> { /// A handle to a single timeline item in an `ObservableItems`. pub struct ObservableItemsEntry<'a>(ObservableVectorEntry<'a, Arc>); -impl<'a> ObservableItemsEntry<'a> { +impl ObservableItemsEntry<'_> { /// Replace the timeline item by `timeline_item`. pub fn replace(this: &mut Self, timeline_item: Arc) -> Arc { ObservableVectorEntry::set(&mut this.0, timeline_item) @@ -176,7 +176,7 @@ impl<'observable_items> ObservableItemsTransaction<'observable_items> { /// Get a reference to all remote events. pub fn all_remote_events(&self) -> &AllRemoteEvents { - &self.all_remote_events + self.all_remote_events } /// Remove a remote event at position `event_index`. @@ -307,7 +307,7 @@ pub struct ObservableItemsTransactionEntry<'a, 'observable_items>( ObservableVectorTransactionEntry<'a, 'observable_items, Arc>, ); -impl<'a, 'o> ObservableItemsTransactionEntry<'a, 'o> { +impl ObservableItemsTransactionEntry<'_, '_> { /// Replace the timeline item by `timeline_item`. pub fn replace(this: &mut Self, timeline_item: Arc) -> Arc { ObservableVectorTransactionEntry::set(&mut this.0, timeline_item) diff --git a/crates/matrix-sdk-ui/src/timeline/controller/state.rs b/crates/matrix-sdk-ui/src/timeline/controller/state.rs index 9df9ffb8e2f..520f3ac82e4 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/state.rs @@ -731,7 +731,7 @@ impl TimelineStateTransaction<'_> { match position { TimelineItemPosition::Start { .. } => { if let Some(pos) = - event_already_exists(event_meta.event_id, &self.items.all_remote_events()) + event_already_exists(event_meta.event_id, self.items.all_remote_events()) { self.items.remove_remote_event(pos); } @@ -741,7 +741,7 @@ impl TimelineStateTransaction<'_> { TimelineItemPosition::End { .. } => { if let Some(pos) = - event_already_exists(event_meta.event_id, &self.items.all_remote_events()) + event_already_exists(event_meta.event_id, self.items.all_remote_events()) { self.items.remove_remote_event(pos); } diff --git a/crates/matrix-sdk-ui/src/timeline/read_receipts.rs b/crates/matrix-sdk-ui/src/timeline/read_receipts.rs index 30226de15ca..00964de20be 100644 --- a/crates/matrix-sdk-ui/src/timeline/read_receipts.rs +++ b/crates/matrix-sdk-ui/src/timeline/read_receipts.rs @@ -485,7 +485,7 @@ impl TimelineStateTransaction<'_> { let read_receipts = self.meta.read_receipts.compute_event_receipts( &remote_prev_event_item.event_id, - &self.items.all_remote_events(), + self.items.all_remote_events(), false, ); From 729253697f91de5eb69a5a1f337b3a623ba6ad21 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 4 Dec 2024 20:51:50 +0100 Subject: [PATCH 13/25] test(ui): Write test suite for `AllRemoteEvents`. This patch adds test suite for `AllRemoteEvents`. --- .../timeline/controller/observable_items.rs | 283 ++++++++++++++++++ 1 file changed, 283 insertions(+) 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 6f258edf545..e5a2f4134c2 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -478,3 +478,286 @@ impl AllRemoteEvents { } } } + +#[cfg(test)] +mod all_remote_events_tests { + use assert_matches::assert_matches; + use ruma::event_id; + + use super::{AllRemoteEvents, EventMeta}; + + fn event_meta(event_id: &str, timeline_item_index: Option) -> EventMeta { + EventMeta { event_id: event_id.parse().unwrap(), timeline_item_index, visible: false } + } + + macro_rules! assert_events { + ( $events:ident, [ $( ( $event_id:literal, $timeline_item_index:expr ) ),* $(,)? ] ) => { + let mut iter = $events .iter(); + + $( + assert_matches!(iter.next(), Some(EventMeta { event_id, timeline_item_index, .. }) => { + assert_eq!(event_id.as_str(), $event_id ); + assert_eq!(*timeline_item_index, $timeline_item_index ); + }); + )* + + assert!(iter.next().is_none(), "Not all events have been asserted"); + } + } + + #[test] + fn test_clear() { + let mut events = AllRemoteEvents::default(); + + events.push_back(event_meta("$ev0", None)); + events.push_back(event_meta("$ev1", None)); + events.push_back(event_meta("$ev2", None)); + + assert_eq!(events.iter().count(), 3); + + events.clear(); + + assert_eq!(events.iter().count(), 0); + } + + #[test] + fn test_push_front() { + let mut events = AllRemoteEvents::default(); + + // Push front on an empty set, nothing particular. + events.push_front(event_meta("$ev0", Some(1))); + + // Push front with no `timeline_item_index`. + events.push_front(event_meta("$ev1", None)); + + // Push front with a `timeline_item_index`. + events.push_front(event_meta("$ev2", Some(0))); + + // Push front with the same `timeline_item_index`. + events.push_front(event_meta("$ev3", Some(0))); + + assert_events!( + events, + [ + // `timeline_item_index` is untouched + ("$ev3", Some(0)), + // `timeline_item_index` has been shifted once + ("$ev2", Some(1)), + // no `timeline_item_index` + ("$ev1", None), + // `timeline_item_index` has been shifted twice + ("$ev0", Some(3)), + ] + ); + } + + #[test] + fn test_push_back() { + let mut events = AllRemoteEvents::default(); + + // Push back on an empty set, nothing particular. + events.push_back(event_meta("$ev0", Some(0))); + + // Push back with no `timeline_item_index`. + events.push_back(event_meta("$ev1", None)); + + // Push back with a `timeline_item_index`. + events.push_back(event_meta("$ev2", Some(1))); + + // Push back with a `timeline_item_index` pointing to a timeline item that is + // not the last one. Is it possible in practise? Normally not, but let's test + // it anyway. + events.push_back(event_meta("$ev3", Some(1))); + + assert_events!( + events, + [ + // `timeline_item_index` is untouched + ("$ev0", Some(0)), + // no `timeline_item_index` + ("$ev1", None), + // `timeline_item_index` has been shifted once + ("$ev2", Some(2)), + // `timeline_item_index` is untouched + ("$ev3", Some(1)), + ] + ); + } + + #[test] + fn test_remove() { + let mut events = AllRemoteEvents::default(); + + // Push some events. + events.push_back(event_meta("$ev0", Some(0))); + events.push_back(event_meta("$ev1", Some(1))); + events.push_back(event_meta("$ev2", None)); + events.push_back(event_meta("$ev3", Some(2))); + + // Assert initial state. + assert_events!( + events, + [("$ev0", Some(0)), ("$ev1", Some(1)), ("$ev2", None), ("$ev3", Some(2))] + ); + + // Remove two events. + events.remove(2); // $ev2 has no `timeline_item_index` + events.remove(1); // $ev1 has a `timeline_item_index` + + assert_events!( + events, + [ + ("$ev0", Some(0)), + // `timeline_item_index` has shifted once + ("$ev3", Some(1)), + ] + ); + } + + #[test] + fn test_last() { + let mut events = AllRemoteEvents::default(); + + assert!(events.last().is_none()); + assert!(events.last_index().is_none()); + + // Push some events. + events.push_back(event_meta("$ev0", Some(0))); + events.push_back(event_meta("$ev1", Some(1))); + + assert_matches!(events.last(), Some(EventMeta { event_id, .. }) => { + assert_eq!(event_id.as_str(), "$ev1"); + }); + assert_eq!(events.last_index(), Some(1)); + } + + #[test] + fn test_get_by_event_by_mut() { + let mut events = AllRemoteEvents::default(); + + // Push some events. + events.push_back(event_meta("$ev0", Some(0))); + events.push_back(event_meta("$ev1", Some(1))); + + assert!(events.get_by_event_id_mut(event_id!("$ev0")).is_some()); + assert!(events.get_by_event_id_mut(event_id!("$ev42")).is_none()); + } + + #[test] + fn test_timeline_item_has_been_inserted_at() { + let mut events = AllRemoteEvents::default(); + + // Push some events. + events.push_back(event_meta("$ev0", Some(0))); + events.push_back(event_meta("$ev1", Some(1))); + events.push_back(event_meta("$ev2", None)); + events.push_back(event_meta("$ev3", None)); + events.push_back(event_meta("$ev4", Some(2))); + events.push_back(event_meta("$ev5", Some(3))); + events.push_back(event_meta("$ev6", None)); + + // A timeline item has been inserted at index 2, and maps to no event. + events.timeline_item_has_been_inserted_at(2, None); + + assert_events!( + events, + [ + ("$ev0", Some(0)), + ("$ev1", Some(1)), + ("$ev2", None), + ("$ev3", None), + // `timeline_item_index` is shifted once + ("$ev4", Some(3)), + // `timeline_item_index` is shifted once + ("$ev5", Some(4)), + ("$ev6", None), + ] + ); + + // A timeline item has been inserted at the back, and maps to `$ev6`. + events.timeline_item_has_been_inserted_at(5, Some(6)); + + assert_events!( + events, + [ + ("$ev0", Some(0)), + ("$ev1", Some(1)), + ("$ev2", None), + ("$ev3", None), + ("$ev4", Some(3)), + ("$ev5", Some(4)), + // `timeline_item_index` has been updated + ("$ev6", Some(5)), + ] + ); + } + + #[test] + fn test_timeline_item_has_been_removed_at() { + let mut events = AllRemoteEvents::default(); + + // Push some events. + events.push_back(event_meta("$ev0", Some(0))); + events.push_back(event_meta("$ev1", Some(1))); + events.push_back(event_meta("$ev2", None)); + events.push_back(event_meta("$ev3", None)); + events.push_back(event_meta("$ev4", Some(3))); + events.push_back(event_meta("$ev5", Some(4))); + events.push_back(event_meta("$ev6", None)); + + // A timeline item has been removed at index 2, which maps to no event. + events.timeline_item_has_been_removed_at(2); + + assert_events!( + events, + [ + ("$ev0", Some(0)), + ("$ev1", Some(1)), + ("$ev2", None), + ("$ev3", None), + // `timeline_item_index` is shifted once + ("$ev4", Some(2)), + // `timeline_item_index` is shifted once + ("$ev5", Some(3)), + ("$ev6", None), + ] + ); + + // A timeline item has been removed at index 2, which maps to `$ev4`. + events.timeline_item_has_been_removed_at(2); + + assert_events!( + events, + [ + ("$ev0", Some(0)), + ("$ev1", Some(1)), + ("$ev2", None), + ("$ev3", None), + // `timeline_item_index` has been updated + ("$ev4", None), + // `timeline_item_index` has shifted once + ("$ev5", Some(2)), + ("$ev6", None), + ] + ); + + // A timeline item has been removed at index 0, which maps to `$ev0`. + events.timeline_item_has_been_removed_at(0); + + assert_events!( + events, + [ + // `timeline_item_index` has been updated + ("$ev0", None), + // `timeline_item_index` has shifted once + ("$ev1", Some(0)), + ("$ev2", None), + ("$ev3", None), + ("$ev4", None), + // `timeline_item_index` has shifted once + ("$ev5", Some(1)), + ("$ev6", None), + ] + ); + } +} From 5fc48cc1ed5ec2df9445299276773b48359746e8 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 4 Dec 2024 21:16:48 +0100 Subject: [PATCH 14/25] test(ui): Write test suite for `ObservableItems`. --- .../timeline/controller/observable_items.rs | 737 ++++++++++++++++++ 1 file changed, 737 insertions(+) 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 e5a2f4134c2..f11301059db 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -139,6 +139,7 @@ impl ObservableItemsEntries<'_> { } /// A handle to a single timeline item in an `ObservableItems`. +#[derive(Debug)] pub struct ObservableItemsEntry<'a>(ObservableVectorEntry<'a, Arc>); impl ObservableItemsEntry<'_> { @@ -333,6 +334,740 @@ impl Deref for ObservableItemsTransactionEntry<'_, '_> { } } +#[cfg(test)] +mod observable_items_tests { + use std::ops::Not; + + use assert_matches::assert_matches; + use eyeball_im::VectorDiff; + use ruma::{ + events::room::message::{MessageType, TextMessageEventContent}, + owned_user_id, MilliSecondsSinceUnixEpoch, + }; + use stream_assert::assert_next_matches; + + use super::*; + use crate::timeline::{ + controller::{EventTimelineItemKind, RemoteEventOrigin}, + event_item::RemoteEventTimelineItem, + EventTimelineItem, Message, TimelineDetails, TimelineItemContent, TimelineUniqueId, + }; + + fn item(event_id: &str) -> Arc { + TimelineItem::new( + EventTimelineItem::new( + owned_user_id!("@ivan:mnt.io"), + TimelineDetails::Unavailable, + MilliSecondsSinceUnixEpoch(0u32.into()), + TimelineItemContent::Message(Message { + msgtype: MessageType::Text(TextMessageEventContent::plain("hello")), + in_reply_to: None, + thread_root: None, + edited: false, + mentions: None, + }), + EventTimelineItemKind::Remote(RemoteEventTimelineItem { + event_id: event_id.parse().unwrap(), + transaction_id: None, + read_receipts: Default::default(), + is_own: false, + is_highlighted: false, + encryption_info: None, + original_json: None, + latest_edit_json: None, + origin: RemoteEventOrigin::Sync, + }), + Default::default(), + false, + ), + TimelineUniqueId(format!("__id_{event_id}")), + ) + } + + fn read_marker() -> Arc { + TimelineItem::read_marker() + } + + fn event_meta(event_id: &str) -> EventMeta { + EventMeta { event_id: event_id.parse().unwrap(), timeline_item_index: None, visible: false } + } + + macro_rules! assert_event_id { + ( $timeline_item:expr, $event_id:literal $( , $message:expr )? $(,)? ) => { + assert_eq!($timeline_item.as_event().unwrap().event_id().unwrap().as_str(), $event_id $( , $message)? ); + }; + } + + macro_rules! assert_mapping { + ( on $transaction:ident: + | event_id | event_index | timeline_item_index | + | $( - )+ | $( - )+ | $( - )+ | + $( + | $event_id:literal | $event_index:literal | $( $timeline_item_index:literal )? | + )+ + ) => { + let all_remote_events = $transaction .all_remote_events(); + + $( + // Remote event exists at this index… + assert_matches!(all_remote_events.0.get( $event_index ), Some(EventMeta { event_id, timeline_item_index, .. }) => { + // … this is the remote event with the expected event ID + assert_eq!( + event_id.as_str(), + $event_id , + concat!("event #", $event_index, " should have ID ", $event_id) + ); + + + // (tiny hack to handle the case where `$timeline_item_index` is absent) + #[allow(unused_variables)] + let timeline_item_index_is_expected = false; + $( + let timeline_item_index_is_expected = true; + let _ = $timeline_item_index; + )? + + if timeline_item_index_is_expected.not() { + // … this remote event does NOT map to a timeline item index + assert!( + timeline_item_index.is_none(), + concat!("event #", $event_index, " with ID ", $event_id, " should NOT map to a timeline item index" ) + ); + } + + $( + // … this remote event maps to a timeline item index + assert_eq!( + *timeline_item_index, + Some( $timeline_item_index ), + concat!("event #", $event_index, " with ID ", $event_id, " should map to timeline item #", $timeline_item_index ) + ); + + // … this timeline index exists + assert_matches!( $transaction .get( $timeline_item_index ), Some(timeline_item) => { + // … this timelime item has the expected event ID + assert_event_id!( + timeline_item, + $event_id , + concat!("timeline item #", $timeline_item_index, " should map to event ID ", $event_id ) + ); + }); + )? + }); + )* + } +} + + #[test] + fn test_is_empty() { + let mut items = ObservableItems::new(); + + assert!(items.is_empty()); + + // Push one event to check if `is_empty` returns false. + let mut transaction = items.transaction(); + transaction.push_back(item("$ev0"), Some(0)); + transaction.commit(); + + assert!(items.is_empty().not()); + } + + #[test] + fn test_subscribe() { + let mut items = ObservableItems::new(); + let mut subscriber = items.subscribe().into_stream(); + + // Push one event to check the subscriber is emitting something. + let mut transaction = items.transaction(); + transaction.push_back(item("$ev0"), Some(0)); + transaction.commit(); + + // It does! + assert_next_matches!(subscriber, VectorDiff::PushBack { value: event } => { + assert_event_id!(event, "$ev0"); + }); + } + + #[test] + fn test_clone() { + let mut items = ObservableItems::new(); + + let mut transaction = items.transaction(); + transaction.push_back(item("$ev0"), Some(0)); + transaction.push_back(item("$ev1"), Some(1)); + transaction.commit(); + + let events = items.clone(); + assert_eq!(events.len(), 2); + assert_event_id!(events[0], "$ev0"); + assert_event_id!(events[1], "$ev1"); + } + + #[test] + fn test_replace() { + let mut items = ObservableItems::new(); + + // Push one event that will be replaced. + let mut transaction = items.transaction(); + transaction.push_back(item("$ev0"), Some(0)); + transaction.commit(); + + // That's time to replace it! + items.replace(0, item("$ev1")); + + let events = items.clone(); + assert_eq!(events.len(), 1); + assert_event_id!(events[0], "$ev1"); + } + + #[test] + fn test_entries() { + let mut items = ObservableItems::new(); + + // Push events to iterate on. + let mut transaction = items.transaction(); + transaction.push_back(item("$ev0"), Some(0)); + transaction.push_back(item("$ev1"), Some(1)); + transaction.push_back(item("$ev2"), Some(2)); + transaction.commit(); + + let mut entries = items.entries(); + + assert_matches!(entries.next(), Some(entry) => { + assert_event_id!(entry, "$ev0"); + }); + assert_matches!(entries.next(), Some(entry) => { + assert_event_id!(entry, "$ev1"); + }); + assert_matches!(entries.next(), Some(entry) => { + assert_event_id!(entry, "$ev2"); + }); + assert_matches!(entries.next(), None); + } + + #[test] + fn test_entry_replace() { + let mut items = ObservableItems::new(); + + // Push events to iterate on. + let mut transaction = items.transaction(); + transaction.push_back(item("$ev0"), Some(0)); + transaction.commit(); + + let mut entries = items.entries(); + + // Replace one event by another one. + assert_matches!(entries.next(), Some(mut entry) => { + assert_event_id!(entry, "$ev0"); + ObservableItemsEntry::replace(&mut entry, item("$ev1")); + }); + assert_matches!(entries.next(), None); + + // Check the new event. + let mut entries = items.entries(); + + assert_matches!(entries.next(), Some(entry) => { + assert_event_id!(entry, "$ev1"); + }); + assert_matches!(entries.next(), None); + } + + #[test] + fn test_for_each() { + let mut items = ObservableItems::new(); + + // Push events to iterate on. + let mut transaction = items.transaction(); + transaction.push_back(item("$ev0"), Some(0)); + transaction.push_back(item("$ev1"), Some(1)); + transaction.push_back(item("$ev2"), Some(2)); + transaction.commit(); + + let mut nth = 0; + + // Iterate over events. + items.for_each(|entry| { + match nth { + 0 => { + assert_event_id!(entry, "$ev0"); + } + 1 => { + assert_event_id!(entry, "$ev1"); + } + 2 => { + assert_event_id!(entry, "$ev2"); + } + _ => unreachable!(), + } + + nth += 1; + }); + } + + #[test] + fn test_transaction_commit() { + let mut items = ObservableItems::new(); + + // Don't commit the transaction. + let mut transaction = items.transaction(); + transaction.push_back(item("$ev0"), Some(0)); + drop(transaction); + + assert!(items.is_empty()); + + // Commit the transaction. + let mut transaction = items.transaction(); + transaction.push_back(item("$ev0"), Some(0)); + transaction.commit(); + + assert!(items.is_empty().not()); + } + + #[test] + fn test_transaction_get() { + let mut items = ObservableItems::new(); + + let mut transaction = items.transaction(); + transaction.push_back(item("$ev0"), Some(0)); + + assert_matches!(transaction.get(0), Some(event) => { + assert_event_id!(event, "$ev0"); + }); + } + + #[test] + fn test_transaction_replace() { + let mut items = ObservableItems::new(); + + let mut transaction = items.transaction(); + transaction.push_back(item("$ev0"), Some(0)); + transaction.replace(0, item("$ev1")); + + assert_matches!(transaction.get(0), Some(event) => { + assert_event_id!(event, "$ev1"); + }); + } + + #[test] + fn test_transaction_insert() { + let mut items = ObservableItems::new(); + + let mut transaction = items.transaction(); + + // Remote event with its timeline item. + transaction.push_back_remote_event(event_meta("$ev0")); + transaction.insert(0, item("$ev0"), Some(0)); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 0 | // new + } + + // Timeline item without a remote event (for example a read marker). + transaction.insert(0, read_marker(), None); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 1 | // has shifted + } + + // Remote event with its timeline item. + transaction.push_back_remote_event(event_meta("$ev1")); + transaction.insert(2, item("$ev1"), Some(1)); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 1 | + | "$ev1" | 1 | 2 | // new + } + + // Remote event without a timeline item (for example a state event). + transaction.push_back_remote_event(event_meta("$ev2")); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 1 | + | "$ev1" | 1 | 2 | + | "$ev2" | 2 | | // new + } + + // Remote event with its timeline item. + transaction.push_back_remote_event(event_meta("$ev3")); + transaction.insert(3, item("$ev3"), Some(3)); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 1 | + | "$ev1" | 1 | 2 | + | "$ev2" | 2 | | + | "$ev3" | 3 | 3 | // new + } + + // Timeline item with a remote event, but late. + // I don't know if this case is possible in reality, but let's be robust. + transaction.insert(3, item("$ev2"), Some(2)); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 1 | + | "$ev1" | 1 | 2 | + | "$ev2" | 2 | 3 | // updated + | "$ev3" | 3 | 4 | // has shifted + } + + // Let's move the read marker for the fun. + transaction.remove(0); + transaction.insert(2, read_marker(), None); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 0 | // has shifted + | "$ev1" | 1 | 1 | // has shifted + | "$ev2" | 2 | 3 | + | "$ev3" | 3 | 4 | + } + + assert_eq!(transaction.len(), 5); + } + + #[test] + fn test_transaction_push_front() { + let mut items = ObservableItems::new(); + + let mut transaction = items.transaction(); + + // Remote event with its timeline item. + transaction.push_front_remote_event(event_meta("$ev0")); + transaction.push_front(item("$ev0"), Some(0)); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 0 | // new + } + + // Timeline item without a remote event (for example a read marker). + transaction.push_front(read_marker(), None); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 1 | // has shifted + } + + // Remote event with its timeline item. + transaction.push_front_remote_event(event_meta("$ev1")); + transaction.push_front(item("$ev1"), Some(0)); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev1" | 0 | 0 | // new + | "$ev0" | 1 | 2 | // has shifted + } + + // Remote event without a timeline item (for example a state event). + transaction.push_front_remote_event(event_meta("$ev2")); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev2" | 0 | | + | "$ev1" | 1 | 0 | // has shifted + | "$ev0" | 2 | 2 | // has shifted + } + + // Remote event with its timeline item. + transaction.push_front_remote_event(event_meta("$ev3")); + transaction.push_front(item("$ev3"), Some(0)); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev3" | 0 | 0 | // new + | "$ev2" | 1 | | + | "$ev1" | 2 | 1 | // has shifted + | "$ev0" | 3 | 3 | // has shifted + } + + assert_eq!(transaction.len(), 4); + } + + #[test] + fn test_transaction_push_back() { + let mut items = ObservableItems::new(); + + let mut transaction = items.transaction(); + + // Remote event with its timeline item. + transaction.push_back_remote_event(event_meta("$ev0")); + transaction.push_back(item("$ev0"), Some(0)); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 0 | // new + } + + // Timeline item without a remote event (for example a read marker). + transaction.push_back(read_marker(), None); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 0 | + } + + // Remote event with its timeline item. + transaction.push_back_remote_event(event_meta("$ev1")); + transaction.push_back(item("$ev1"), Some(1)); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 0 | + | "$ev1" | 1 | 2 | // new + } + + // Remote event without a timeline item (for example a state event). + transaction.push_back_remote_event(event_meta("$ev2")); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 0 | + | "$ev1" | 1 | 2 | + | "$ev2" | 2 | | // new + } + + // Remote event with its timeline item. + transaction.push_back_remote_event(event_meta("$ev3")); + transaction.push_back(item("$ev3"), Some(3)); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 0 | + | "$ev1" | 1 | 2 | + | "$ev2" | 2 | | + | "$ev3" | 3 | 3 | // new + } + + assert_eq!(transaction.len(), 4); + } + + #[test] + fn test_transaction_remove() { + let mut items = ObservableItems::new(); + + let mut transaction = items.transaction(); + + // Remote event with its timeline item. + transaction.push_back_remote_event(event_meta("$ev0")); + transaction.push_back(item("$ev0"), Some(0)); + + // Timeline item without a remote event (for example a read marker). + transaction.push_back(read_marker(), None); + + // Remote event with its timeline item. + transaction.push_back_remote_event(event_meta("$ev1")); + transaction.push_back(item("$ev1"), Some(1)); + + // Remote event without a timeline item (for example a state event). + transaction.push_back_remote_event(event_meta("$ev2")); + + // Remote event with its timeline item. + transaction.push_back_remote_event(event_meta("$ev3")); + transaction.push_back(item("$ev3"), Some(3)); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 0 | + | "$ev1" | 1 | 2 | + | "$ev2" | 2 | | + | "$ev3" | 3 | 3 | + } + + // Remove the timeline item that has no event. + transaction.remove(1); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 0 | + | "$ev1" | 1 | 1 | // has shifted + | "$ev2" | 2 | | + | "$ev3" | 3 | 2 | // has shifted + } + + // Remove an timeline item that has an event. + transaction.remove(1); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 0 | + | "$ev1" | 1 | | // has been removed + | "$ev2" | 2 | | + | "$ev3" | 3 | 1 | // has shifted + } + + // Remove the last timeline item to test off by 1 error. + transaction.remove(1); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 0 | + | "$ev1" | 1 | | + | "$ev2" | 2 | | + | "$ev3" | 3 | | // has been removed + } + + // Remove all the items \o/ + transaction.remove(0); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | | // has been removed + | "$ev1" | 1 | | + | "$ev2" | 2 | | + | "$ev3" | 3 | | + } + + assert!(transaction.is_empty()); + } + + #[test] + fn test_transaction_clear() { + let mut items = ObservableItems::new(); + + let mut transaction = items.transaction(); + + // Remote event with its timeline item. + transaction.push_back_remote_event(event_meta("$ev0")); + transaction.push_back(item("$ev0"), Some(0)); + + // Timeline item without a remote event (for example a read marker). + transaction.push_back(read_marker(), None); + + // Remote event with its timeline item. + transaction.push_back_remote_event(event_meta("$ev1")); + transaction.push_back(item("$ev1"), Some(1)); + + // Remote event without a timeline item (for example a state event). + transaction.push_back_remote_event(event_meta("$ev2")); + + // Remote event with its timeline item. + transaction.push_back_remote_event(event_meta("$ev3")); + transaction.push_back(item("$ev3"), Some(3)); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 0 | + | "$ev1" | 1 | 2 | + | "$ev2" | 2 | | + | "$ev3" | 3 | 3 | + } + + assert_eq!(transaction.all_remote_events().0.len(), 4); + assert_eq!(transaction.len(), 4); + + // Let's clear everything. + transaction.clear(); + + assert!(transaction.all_remote_events().0.is_empty()); + assert!(transaction.is_empty()); + } + + #[test] + fn test_transaction_for_each() { + let mut items = ObservableItems::new(); + + // Push events to iterate on. + let mut transaction = items.transaction(); + transaction.push_back(item("$ev0"), Some(0)); + transaction.push_back(item("$ev1"), Some(1)); + transaction.push_back(item("$ev2"), Some(2)); + + let mut nth = 0; + + // Iterate over events. + transaction.for_each(|entry| { + match nth { + 0 => { + assert_event_id!(entry, "$ev0"); + } + 1 => { + assert_event_id!(entry, "$ev1"); + } + 2 => { + assert_event_id!(entry, "$ev2"); + } + _ => unreachable!(), + } + + nth += 1; + }); + } +} + /// A type for all remote events. /// /// Having this type helps to know exactly which parts of the code and how they @@ -509,12 +1244,14 @@ mod all_remote_events_tests { fn test_clear() { let mut events = AllRemoteEvents::default(); + // Push some events. events.push_back(event_meta("$ev0", None)); events.push_back(event_meta("$ev1", None)); events.push_back(event_meta("$ev2", None)); assert_eq!(events.iter().count(), 3); + // And clear them! events.clear(); assert_eq!(events.iter().count(), 0); From c41776ac6298892457d6e62db6900f950abf7388 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 9 Dec 2024 15:28:11 +0100 Subject: [PATCH 15/25] refactor(ui): `ObservableItemsTransactionEntry::remove` is no longer unsafe --- .../timeline/controller/observable_items.rs | 78 +++++++++++++++---- .../src/timeline/controller/state.rs | 7 +- 2 files changed, 65 insertions(+), 20 deletions(-) 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 f11301059db..6c5adde7b31 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -284,7 +284,9 @@ impl<'observable_items> ObservableItemsTransaction<'observable_items> { where F: FnMut(ObservableItemsTransactionEntry<'_, 'observable_items>), { - self.items.for_each(|entry| f(ObservableItemsTransactionEntry(entry))) + self.items.for_each(|entry| { + f(ObservableItemsTransactionEntry { entry, all_remote_events: self.all_remote_events }) + }) } /// Commit this transaction, persisting the changes and notifying @@ -304,25 +306,27 @@ impl Deref for ObservableItemsTransaction<'_> { } /// A handle to a single timeline item in an `ObservableItemsTransaction`. -pub struct ObservableItemsTransactionEntry<'a, 'observable_items>( - ObservableVectorTransactionEntry<'a, 'observable_items, Arc>, -); +pub struct ObservableItemsTransactionEntry<'observable_transaction_items, 'observable_items> { + entry: ObservableVectorTransactionEntry< + 'observable_transaction_items, + 'observable_items, + Arc, + >, + all_remote_events: &'observable_transaction_items mut AllRemoteEvents, +} impl ObservableItemsTransactionEntry<'_, '_> { /// Replace the timeline item by `timeline_item`. pub fn replace(this: &mut Self, timeline_item: Arc) -> Arc { - ObservableVectorTransactionEntry::set(&mut this.0, timeline_item) + ObservableVectorTransactionEntry::set(&mut this.entry, timeline_item) } /// Remove this timeline item. - /// - /// # Safety - /// - /// This method doesn't update `AllRemoteEvents`. Be sure that the caller - /// doesn't break the mapping between remote events and timeline items. See - /// [`EventMeta::timeline_item_index`] to learn more. - pub unsafe fn remove(this: Self) { - ObservableVectorTransactionEntry::remove(this.0); + pub fn remove(this: Self) { + let entry_index = ObservableVectorTransactionEntry::index(&this.entry); + + ObservableVectorTransactionEntry::remove(this.entry); + this.all_remote_events.timeline_item_has_been_removed_at(entry_index); } } @@ -330,7 +334,7 @@ impl Deref for ObservableItemsTransactionEntry<'_, '_> { type Target = Arc; fn deref(&self) -> &Self::Target { - &self.0 + &self.entry } } @@ -1066,6 +1070,52 @@ mod observable_items_tests { nth += 1; }); } + + #[test] + fn test_transaction_for_each_remove() { + let mut items = ObservableItems::new(); + + // Push events to iterate on. + let mut transaction = items.transaction(); + + transaction.push_back_remote_event(event_meta("$ev0")); + transaction.push_back(item("$ev0"), Some(0)); + + transaction.push_back_remote_event(event_meta("$ev1")); + transaction.push_back(item("$ev1"), Some(1)); + + transaction.push_back_remote_event(event_meta("$ev2")); + transaction.push_back(item("$ev2"), Some(2)); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 0 | + | "$ev1" | 1 | 1 | + | "$ev2" | 2 | 2 | + } + + // Iterate over events, and remove one. + transaction.for_each(|entry| { + if entry.as_event().unwrap().event_id().unwrap().as_str() == "$ev1" { + ObservableItemsTransactionEntry::remove(entry); + } + }); + + assert_mapping! { + on transaction: + + | event_id | event_index | timeline_item_index | + |----------|-------------|---------------------| + | "$ev0" | 0 | 0 | + | "$ev2" | 2 | 1 | // has shifted + } + + assert_eq!(transaction.all_remote_events().0.len(), 3); + assert_eq!(transaction.len(), 2); + } } /// A type for all remote events. diff --git a/crates/matrix-sdk-ui/src/timeline/controller/state.rs b/crates/matrix-sdk-ui/src/timeline/controller/state.rs index 520f3ac82e4..4264f3442f4 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/state.rs @@ -657,12 +657,7 @@ impl TimelineStateTransaction<'_> { // Remove all remote events and the read marker self.items.for_each(|entry| { if entry.is_remote_event() || entry.is_read_marker() { - // SAFETY: this method removes all events except local events. Local events - // don't have a mapping from remote events to timeline items because… well… they - // are local events, not remove events. - unsafe { - ObservableItemsTransactionEntry::remove(entry); - } + ObservableItemsTransactionEntry::remove(entry); } }); From c9a215eceab17efa576f08086ed71e7f48ceff3f Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 9 Dec 2024 15:58:48 +0100 Subject: [PATCH 16/25] doc(ui): Fix typos. --- crates/matrix-sdk-ui/src/timeline/controller/state.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/controller/state.rs b/crates/matrix-sdk-ui/src/timeline/controller/state.rs index 4264f3442f4..878573af3ea 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/state.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/state.rs @@ -1199,7 +1199,7 @@ pub(crate) struct EventMeta { /// /// Once rendered in a timeline, it for example produces: /// - /// | index | item | aside items | + /// | index | item | related items | /// +-------+-------------------+----------------------+ /// | 0 | content of `$ev0` | | /// | 1 | content of `$ev2` | reaction with `$ev4` | @@ -1211,10 +1211,10 @@ pub(crate) struct EventMeta { /// a reaction to `$ev2`. Finally note that `$ev1` is not rendered in /// the timeline. /// - /// The mapping between remove event index to timeline item index will look + /// The mapping between remote event index to timeline item index will look /// like this: /// - /// | remove event index | timeline item index | comment | + /// | remote event index | timeline item index | comment | /// +--------------------+---------------------+--------------------------------------------+ /// | 0 | `Some(0)` | `$ev0` is rendered as the #0 timeline item | /// | 1 | `None` | `$ev1` isn't rendered in the timeline | From 8b02da33504213a568ce4bc8b1c13816a360e943 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 9 Dec 2024 16:04:31 +0100 Subject: [PATCH 17/25] doc(ui): Document `ObservableItems::items` more and `TimelineItemKind`. --- .../src/timeline/controller/observable_items.rs | 6 +++++- crates/matrix-sdk-ui/src/timeline/item.rs | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) 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 6c5adde7b31..bc02b8b50d6 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -35,7 +35,11 @@ use super::{state::EventMeta, TimelineItem}; pub struct ObservableItems { /// All timeline items. /// - /// Yeah, there are here! + /// Yeah, there are here! This [`ObservableVector`] contains all the + /// timeline items that are rendered in your magnificent Matrix client. + /// + /// These items are the _core_ of the timeline, see [`TimelineItem`] to + /// learn more. items: ObservableVector>, /// List of all the remote events as received in the timeline, even the ones diff --git a/crates/matrix-sdk-ui/src/timeline/item.rs b/crates/matrix-sdk-ui/src/timeline/item.rs index 8096da48c1a..6487d28f6d7 100644 --- a/crates/matrix-sdk-ui/src/timeline/item.rs +++ b/crates/matrix-sdk-ui/src/timeline/item.rs @@ -26,13 +26,14 @@ use super::{EventTimelineItem, VirtualTimelineItem}; #[derive(Clone, Debug, PartialEq, Eq, Hash)] pub struct TimelineUniqueId(pub String); +/// The type of timeline item. #[derive(Clone, Debug)] #[allow(clippy::large_enum_variant)] pub enum TimelineItemKind { /// An event or aggregation of multiple events. Event(EventTimelineItem), /// An item that doesn't correspond to an event, for example the user's - /// own read marker. + /// own read marker, or a day divider. Virtual(VirtualTimelineItem), } From 0f584649b171b17934e234279f201949605ede3d Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 9 Dec 2024 16:11:16 +0100 Subject: [PATCH 18/25] doc(ui): Rephrase the documentation of `ObservableItems::all_remote_events`. --- .../src/timeline/controller/observable_items.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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 bc02b8b50d6..74dac26dbae 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -45,9 +45,10 @@ pub struct ObservableItems { /// 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. + /// The list of all remote events is used to compute the read receipts and + /// read markers; additionally it's used to map events to timeline items, + /// for more info about that, take a look at the documentation for + /// [`EventMeta::timeline_item_index`]. all_remote_events: AllRemoteEvents, } From bfd7b039155a645850813d81aa1c805d84c72ff8 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 9 Dec 2024 16:34:04 +0100 Subject: [PATCH 19/25] refactor(ui): Replace a panic by a sensible value + `error!`. --- .../src/timeline/event_handler.rs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index 9c8a63f203b..1eba4147626 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -1180,12 +1180,18 @@ 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") - ); + let event_index = self + .items + .all_remote_events() + .last_index() + // The last remote event is necessarily associated to this + // timeline item, see the contract of this method. Let's fallback to a similar + // value as `timeline_item_index` instead of panicking. + .or_else(|| { + error!(?event_id, "Failed to read the last event index from `AllRemoteEvents`: at least one event must be present"); + + Some(0) + }); // Try to keep precise insertion semantics here, in this exact order: // From 4ed9e73ebf6c1d205ada8be8b61e773f80fc7748 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 9 Dec 2024 16:37:07 +0100 Subject: [PATCH 20/25] refactor(ui): Rename `ObservableItems::clone` to `clone_items`. --- .../src/timeline/controller/mod.rs | 6 +++--- .../timeline/controller/observable_items.rs | 18 +++++++++--------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs index 9a22bb91753..5d82572470c 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs @@ -467,7 +467,7 @@ impl TimelineController

{ /// /// Cheap because `im::Vector` is cheap to clone. pub(super) async fn items(&self) -> Vector> { - self.state.read().await.items.clone() + self.state.read().await.items.clone_items() } pub(super) async fn subscribe( @@ -475,7 +475,7 @@ impl TimelineController

{ ) -> (Vector>, impl Stream>> + Send) { trace!("Creating timeline items signal"); let state = self.state.read().await; - (state.items.clone(), state.items.subscribe().into_stream()) + (state.items.clone_items(), state.items.subscribe().into_stream()) } pub(super) async fn subscribe_batched( @@ -483,7 +483,7 @@ impl TimelineController

{ ) -> (Vector>, impl Stream>>>) { trace!("Creating timeline items signal"); let state = self.state.read().await; - (state.items.clone(), state.items.subscribe().into_batched_stream()) + (state.items.clone_items(), state.items.subscribe().into_batched_stream()) } pub(super) async fn subscribe_filter_map( 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 74dac26dbae..a1b6697ae98 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -82,7 +82,7 @@ impl ObservableItems { /// Get a clone of all timeline items. /// /// Note that it doesn't clone `Self`, only the inner timeline items. - pub fn clone(&self) -> Vector> { + pub fn clone_items(&self) -> Vector> { self.items.clone() } @@ -498,7 +498,7 @@ mod observable_items_tests { } #[test] - fn test_clone() { + fn test_clone_items() { let mut items = ObservableItems::new(); let mut transaction = items.transaction(); @@ -506,10 +506,10 @@ mod observable_items_tests { transaction.push_back(item("$ev1"), Some(1)); transaction.commit(); - let events = items.clone(); - assert_eq!(events.len(), 2); - assert_event_id!(events[0], "$ev0"); - assert_event_id!(events[1], "$ev1"); + let items = items.clone_items(); + assert_eq!(items.len(), 2); + assert_event_id!(items[0], "$ev0"); + assert_event_id!(items[1], "$ev1"); } #[test] @@ -524,9 +524,9 @@ mod observable_items_tests { // That's time to replace it! items.replace(0, item("$ev1")); - let events = items.clone(); - assert_eq!(events.len(), 1); - assert_event_id!(events[0], "$ev1"); + let items = items.clone_items(); + assert_eq!(items.len(), 1); + assert_event_id!(items[0], "$ev1"); } #[test] From 650ac7023d39f2170079cb94d8b02b406faaaeb9 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 9 Dec 2024 16:39:40 +0100 Subject: [PATCH 21/25] doc(ui): Explain why `Deref` is fine, but `DerefMut` is not. --- .../timeline/controller/observable_items.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) 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 a1b6697ae98..ad2af19c0f7 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -124,6 +124,10 @@ impl ObservableItems { } // It's fine to deref to an immutable reference to `Vector`. +// +// We don't want, however, to deref to a mutable reference: it should be done +// via proper methods to control precisely the mapping between remote events and +// timeline items. impl Deref for ObservableItems { type Target = Vector>; @@ -154,6 +158,11 @@ impl ObservableItemsEntry<'_> { } } +// It's fine to deref to an immutable reference to `Arc`. +// +// We don't want, however, to deref to a mutable reference: it should be done +// via proper methods to control precisely the mapping between remote events and +// timeline items. impl Deref for ObservableItemsEntry<'_> { type Target = Arc; @@ -302,6 +311,10 @@ impl<'observable_items> ObservableItemsTransaction<'observable_items> { } // It's fine to deref to an immutable reference to `Vector`. +// +// We don't want, however, to deref to a mutable reference: it should be done +// via proper methods to control precisely the mapping between remote events and +// timeline items. impl Deref for ObservableItemsTransaction<'_> { type Target = Vector>; @@ -335,6 +348,11 @@ impl ObservableItemsTransactionEntry<'_, '_> { } } +// It's fine to deref to an immutable reference to `Arc`. +// +// We don't want, however, to deref to a mutable reference: it should be done +// via proper methods to control precisely the mapping between remote events and +// timeline items. impl Deref for ObservableItemsTransactionEntry<'_, '_> { type Target = Arc; From a94556a86a9b8c01284c2c057fdac0a71d199441 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 9 Dec 2024 16:50:25 +0100 Subject: [PATCH 22/25] doc(ui): Explain why `ObservableItemsEntries` does not implement `Iterator`. --- .../src/timeline/controller/observable_items.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 ad2af19c0f7..7d36c1adee0 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -136,7 +136,12 @@ impl Deref for ObservableItems { } } -/// An “iterator“ that yields entries into an `ObservableItems`. +/// An iterator that yields entries into an `ObservableItems`. +/// +/// It doesn't implement [`Iterator`] though because of a lifetime conflict: the +/// returned `Iterator::Item` could live longer than the `Iterator` itself. +/// Ideally, `Iterator::next` should take a `&'a mut self`, but this is not +/// possible. pub struct ObservableItemsEntries<'a>(ObservableVectorEntries<'a, Arc>); impl ObservableItemsEntries<'_> { From 5004670de66760ad3ba4d82018a2deca432e71bb Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 9 Dec 2024 17:10:02 +0100 Subject: [PATCH 23/25] doc(ui): Unfold a `Self` in the doc. --- .../matrix-sdk-ui/src/timeline/controller/observable_items.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 7d36c1adee0..ab6d992953a 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -180,8 +180,8 @@ impl Deref for ObservableItemsEntry<'_> { /// an atomic unit. /// /// For updates from the transaction to have affect, it has to be finalized with -/// [`Self::commit`]. If the transaction is dropped without that method being -/// called, the updates will be discarded. +/// [`ObservableItemsTransaction::commit`]. If the transaction is dropped +/// without that method being called, the updates will be discarded. #[derive(Debug)] pub struct ObservableItemsTransaction<'observable_items> { items: ObservableVectorTransaction<'observable_items, Arc>, From 04f9403c0eb41f54367d350b0cab67150eae7ce9 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 9 Dec 2024 17:10:57 +0100 Subject: [PATCH 24/25] doc(ui): Fix a typo. --- .../matrix-sdk-ui/src/timeline/controller/observable_items.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ab6d992953a..5ff68c3f513 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -189,7 +189,7 @@ pub struct ObservableItemsTransaction<'observable_items> { } impl<'observable_items> ObservableItemsTransaction<'observable_items> { - /// Get a reference to the timeline index at position `timeline_item_index`. + /// Get a reference to the timeline item at position `timeline_item_index`. pub fn get(&self, timeline_item_index: usize) -> Option<&Arc> { self.items.get(timeline_item_index) } From bd648cd86e7293503206c295898a999ac8871969 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 9 Dec 2024 17:14:44 +0100 Subject: [PATCH 25/25] doc(ui): Fix typos. --- .../src/timeline/controller/observable_items.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 5ff68c3f513..e5f62d70716 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs @@ -199,7 +199,7 @@ impl<'observable_items> ObservableItemsTransaction<'observable_items> { self.all_remote_events } - /// Remove a remote event at position `event_index`. + /// Remove a remote event at the `event_index` position. /// /// Not to be confused with removing a timeline item! pub fn remove_remote_event(&mut self, event_index: usize) -> Option { @@ -208,14 +208,14 @@ impl<'observable_items> ObservableItemsTransaction<'observable_items> { /// Push a new remote event at the front of all remote events. /// - /// Not to be confused with pushing front a timeline item! + /// Not to be confused with pushing a timeline item to the front! pub fn push_front_remote_event(&mut self, event_meta: EventMeta) { self.all_remote_events.push_front(event_meta); } /// Push a new remote event at the back of all remote events. /// - /// Not to be confused with pushing back a timeline item! + /// Not to be confused with pushing a timeline item to the back! pub fn push_back_remote_event(&mut self, event_meta: EventMeta) { self.all_remote_events.push_back(event_meta); }