From 7a6b2ce7c4b56fd36bebff6f57373c0e3c84295f Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 4 Dec 2024 17:39:38 +0100 Subject: [PATCH] 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 8e96cf09551..16074a20051 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); } }); }