Skip to content

Commit

Permalink
refactor(ui): Remove TimelineSettings::vectordiffs_as_inputs.
Browse files Browse the repository at this point in the history 
From now on, this patch considers that `VectorDiff`s are the official
input type for the `Timeline`, via `RoomEventCacheUpdate` (notably
`::UpdateTimelineEvents`).

This patch removes `TimelineSettings::vectordiffs_as_inputs`. It thus
removes all deduplication logics, as it is supposed to be managed by the
`EventCache` itself.
  • Loading branch information
@Hywan
Hywan committed Jan 8, 2025
1 parent 493da24 commit c6e0da6
Show file tree
Hide file tree
Showing 6 changed files with 16 additions and 134 deletions.
41 changes: 11 additions & 30 deletions crates/matrix-sdk-ui/src/timeline/builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,7 @@ use super::{
to_device::{handle_forwarded_room_key_event, handle_room_key_event},
DateDividerMode, Error, Timeline, TimelineDropHandle, TimelineFocus,
};
use crate::{
timeline::{controller::TimelineNewItemPosition, event_item::RemoteEventOrigin},
unable_to_decrypt_hook::UtdHookManager,
};
use crate::{timeline::event_item::RemoteEventOrigin, unable_to_decrypt_hook::UtdHookManager};

/// Builder that allows creating and configuring various parts of a
/// [`Timeline`].
Expand Down Expand Up @@ -145,14 +142,6 @@ impl TimelineBuilder {
self
}

/// Use `VectorDiff`s as the new “input mechanism” for the `Timeline`.
///
/// Read `TimelineSettings::vectordiffs_as_inputs` to learn more.
pub fn with_vectordiffs_as_inputs(mut self) -> Self {
self.settings.vectordiffs_as_inputs = true;
self
}

/// Create a [`Timeline`] with the options set on this builder.
#[tracing::instrument(
skip(self),
Expand All @@ -162,17 +151,11 @@ impl TimelineBuilder {
)
)]
pub async fn build(self) -> Result<Timeline, Error> {
let Self { room, mut settings, unable_to_decrypt_hook, focus, internal_id_prefix } = self;
let Self { room, settings, unable_to_decrypt_hook, focus, internal_id_prefix } = self;

let client = room.client();
let event_cache = client.event_cache();

// Enable `TimelineSettings::vectordiffs_as_inputs` if and only if the event
// cache storage is enabled.
settings.vectordiffs_as_inputs = event_cache.has_storage();

let settings_vectordiffs_as_inputs = settings.vectordiffs_as_inputs;

// Subscribe the event cache to sync responses, in case we hadn't done it yet.
event_cache.subscribe()?;

Expand Down Expand Up @@ -291,17 +274,15 @@ impl TimelineBuilder {
}

RoomEventCacheUpdate::UpdateTimelineEvents { diffs, origin } => {
if settings_vectordiffs_as_inputs {
trace!("Received new timeline events diffs");

inner.handle_remote_events_with_diffs(
diffs,
match origin {
EventsOrigin::Sync => RemoteEventOrigin::Sync,
EventsOrigin::Pagination => RemoteEventOrigin::Pagination,
}
).await;
}
trace!("Received new timeline events diffs");

inner.handle_remote_events_with_diffs(
diffs,
match origin {
EventsOrigin::Sync => RemoteEventOrigin::Sync,
EventsOrigin::Pagination => RemoteEventOrigin::Pagination,
}
).await;
}

RoomEventCacheUpdate::AddEphemeralEvents { events } => {
Expand Down
9 changes: 0 additions & 9 deletions crates/matrix-sdk-ui/src/timeline/controller/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,12 +141,6 @@ pub(super) struct TimelineSettings {

/// Should the timeline items be grouped by day or month?
pub(super) date_divider_mode: DateDividerMode,

/// Whether `VectorDiff` is the “input mechanism” to use.
///
/// This mechanism will replace the existing one, but this runtime feature
/// flag is necessary for the transition and the testing phase.
pub(super) vectordiffs_as_inputs: bool,
}

#[cfg(not(tarpaulin_include))]
Expand All @@ -155,7 +149,6 @@ impl fmt::Debug for TimelineSettings {
f.debug_struct("TimelineSettings")
.field("track_read_receipts", &self.track_read_receipts)
.field("add_failed_to_parse", &self.add_failed_to_parse)
.field("vectordiffs_as_inputs", &self.vectordiffs_as_inputs)
.finish_non_exhaustive()
}
}
Expand All @@ -167,7 +160,6 @@ impl Default for TimelineSettings {
event_filter: Arc::new(default_event_filter),
add_failed_to_parse: true,
date_divider_mode: DateDividerMode::Daily,
vectordiffs_as_inputs: false,
}
}
}
Expand Down Expand Up @@ -789,7 +781,6 @@ impl<P: RoomDataProvider> TimelineController<P> {
txn_id,
send_handle,
content,
&self.settings,
)
.await;
}
Expand Down
34 changes: 2 additions & 32 deletions crates/matrix-sdk-ui/src/timeline/controller/state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,6 @@ impl TimelineState {
txn_id: OwnedTransactionId,
send_handle: Option<SendHandle>,
content: TimelineEventKind,
settings: &TimelineSettings,
) {
let ctx = TimelineEventContext {
sender: own_user_id,
Expand All @@ -241,7 +240,7 @@ impl TimelineState {

let mut date_divider_adjuster = DateDividerAdjuster::new(date_divider_mode);

TimelineEventHandler::new(&mut txn, ctx, settings)
TimelineEventHandler::new(&mut txn, ctx)
.handle_event(&mut date_divider_adjuster, content)
.await;

Expand Down Expand Up @@ -745,9 +744,7 @@ impl TimelineStateTransaction<'_> {
};

// Handle the event to create or update a timeline item.
TimelineEventHandler::new(self, ctx, settings)
.handle_event(date_divider_adjuster, event_kind)
.await
TimelineEventHandler::new(self, ctx).handle_event(date_divider_adjuster, event_kind).await
}

/// Remove one timeline item by its `event_index`.
Expand Down Expand Up @@ -846,39 +843,12 @@ impl TimelineStateTransaction<'_> {
room_data_provider: &P,
settings: &TimelineSettings,
) {
/// Remove duplicated events.
///
/// If `VectorDiff`s are the inputs of the `Timeline`, this is not
/// necessary, as they are generated by the `EventCache`, which supports
/// its own deduplication algorithm.
fn deduplicate(
new_event_id: &EventId,
items: &mut ObservableItemsTransaction<'_>,
settings: &TimelineSettings,
) {
if settings.vectordiffs_as_inputs {
return;
}

if let Some(pos) = items
.all_remote_events()
.iter()
.position(|EventMeta { event_id, .. }| event_id == new_event_id)
{
items.remove_remote_event(pos);
}
}

match position {
TimelineItemPosition::Start { .. } => {
deduplicate(event_meta.event_id, &mut self.items, settings);

self.items.push_front_remote_event(event_meta.base_meta())
}

TimelineItemPosition::End { .. } => {
deduplicate(event_meta.event_id, &mut self.items, settings);

self.items.push_back_remote_event(event_meta.base_meta());
}

Expand Down
53 changes: 2 additions & 51 deletions crates/matrix-sdk-ui/src/timeline/event_handler.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ use tracing::{debug, error, field::debug, info, instrument, trace, warn};
use super::{
controller::{
ObservableItemsTransaction, ObservableItemsTransactionEntry, PendingEdit, PendingEditKind,
TimelineMetadata, TimelineSettings, TimelineStateTransaction,
TimelineMetadata, TimelineStateTransaction,
},
date_dividers::DateDividerAdjuster,
event_item::{
Expand Down Expand Up @@ -338,17 +338,15 @@ pub(super) struct TimelineEventHandler<'a, 'o> {
meta: &'a mut TimelineMetadata,
ctx: TimelineEventContext,
result: HandleEventResult,
settings: &'a TimelineSettings,
}

impl<'a, 'o> TimelineEventHandler<'a, 'o> {
pub(super) fn new(
state: &'a mut TimelineStateTransaction<'o>,
ctx: TimelineEventContext,
settings: &'a TimelineSettings,
) -> Self {
let TimelineStateTransaction { items, meta, .. } = state;
Self { items, meta, ctx, result: HandleEventResult::default(), settings }
Self { items, meta, ctx, result: HandleEventResult::default() }
}

/// Handle an event.
Expand Down Expand Up @@ -1115,8 +1113,6 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
&mut item,
Some(event_id),
txn_id.as_ref().map(AsRef::as_ref),
self.meta,
self.settings,
);
let item = new_timeline_item(self.meta, item, removed_duplicated_timeline_item);

Expand All @@ -1136,8 +1132,6 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
&mut item,
Some(event_id),
txn_id.as_ref().map(AsRef::as_ref),
self.meta,
self.settings,
);
let item = new_timeline_item(self.meta, item, removed_duplicated_timeline_item);

Expand Down Expand Up @@ -1191,8 +1185,6 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
&mut item,
Some(event_id),
txn_id.as_ref().map(AsRef::as_ref),
self.meta,
self.settings,
);
let item = new_timeline_item(self.meta, item, removed_duplicated_timeline_item);

Expand Down Expand Up @@ -1268,21 +1260,13 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {

/// Remove the local timeline item matching the `event_id` or the
/// `transaction_id` of `new_event_timeline_item` if it exists.
///
/// Let's also try to deduplicate remote events. If `VectorDiff`s are the
/// inputs of the `Timeline`, this is not necessary, as they are
/// generated by the `EventCache`, which supports its own deduplication
/// algorithm.
//
// Note: this method doesn't take `&mut self` to avoid a borrow checker
// conflict with `TimelineEventHandler::add_item`.
fn deduplicate_local_timeline_item(
items: &mut ObservableItemsTransaction<'_>,
new_event_timeline_item: &mut EventTimelineItem,
event_id: Option<&EventId>,
transaction_id: Option<&TransactionId>,
metadata: &TimelineMetadata,
settings: &TimelineSettings,
) -> Option<Arc<TimelineItem>> {
// Start with the canonical case: detect a local timeline item that matches
// `event_id` or `transaction_id`.
Expand Down Expand Up @@ -1310,39 +1294,6 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
return Some(items.remove(local_timeline_item_index));
};

if !settings.vectordiffs_as_inputs {
if let Some((remote_timeline_item_index, remote_timeline_item)) =
rfind_event_item(items, |event_timeline_item| {
if event_timeline_item.is_remote_event() {
event_id == event_timeline_item.event_id()
} else {
false
}
})
{
trace!(
?event_id,
?transaction_id,
?remote_timeline_item_index,
"Removing remote timeline item (it is a duplicate)"
);

if remote_timeline_item.content.is_redacted()
&& !new_event_timeline_item.content.is_redacted()
{
warn!("Got original form of an event that was previously redacted");
new_event_timeline_item.content =
new_event_timeline_item.content.redact(&metadata.room_version);
new_event_timeline_item.reactions.clear();
}

transfer_details(new_event_timeline_item, &remote_timeline_item);

// Remove the remote timeline item.
return Some(items.remove(remote_timeline_item_index));
}
}

None
}

Expand Down
1 change: 1 addition & 0 deletions crates/matrix-sdk-ui/src/timeline/event_item/content/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,7 @@ impl TimelineItemContent {
as_variant!(self, Self::UnableToDecrypt)
}

#[cfg(test)]
pub(crate) fn is_redacted(&self) -> bool {
matches!(self, Self::RedactedMessage)
}
Expand Down
12 changes: 0 additions & 12 deletions crates/matrix-sdk-ui/src/timeline/pagination.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ use matrix_sdk::event_cache::{
use tracing::{instrument, trace, warn};

use super::Error;
use crate::timeline::{controller::TimelineNewItemPosition, event_item::RemoteEventOrigin};

impl super::Timeline {
/// Add more events to the start of the timeline.
Expand Down Expand Up @@ -77,17 +76,6 @@ impl super::Timeline {
let num_events = events.len();
trace!("Back-pagination succeeded with {num_events} events");

// If `TimelineSettings::vectordiffs_as_inputs` is enabled,
// we don't need to add events manually: everything we need
// is to let the `EventCache` receive the events from this
// pagination, and emit its updates as `VectorDiff`s, which
// will be handled by the `Timeline` naturally.
if !self.controller.settings.vectordiffs_as_inputs {
self.controller
.add_events_at(events.into_iter(), TimelineNewItemPosition::Start { origin: RemoteEventOrigin::Pagination })
.await;
}

if num_events == 0 && !reached_start {
// As an exceptional contract: if there were no events in the response,
// and we've not hit the start of the timeline, retry until we get
Expand Down

0 comments on commit c6e0da6

Please sign in to comment.