feat(ui): `Timeline` consumes updates as `VectorDiff`s from the `EventCache` #4408

Hywan · 2024-12-12T10:25:38Z

This patch should be reviewed commit by commit.

With this patch, the Timeline is able to consume VectorDiffs from the EventCache via RoomEventCacheUpdate::UpdateTimelineEvents instead of RoomEventCacheUpdate::AddTimelineItems.

It has many advantages:

the Timeline has a unique entry point instead of many:
- before this patch, the sync was adding events to the Timeline via RoomEventCacheUpdate, and the (back-)pagination was adding events via internal Timeline API,
- after this patch, all events are added via RoomEventCacheUpdate,
- this fixes several issues with the EventCache persistent storage,
- it paves the road for RoomEventCacheView, a new API we are working on to improve performance of multiple Timelines for the same room (like Media gallery, Thread, Pinned event, and the regular Timeline).
the Timeline does not need to run a deduplication over events, since the EventCache already has its own Deduplicator (note that the old dedup mechanism of the Timeline must be kept for permalink support for the moment)

Blocked by refactor(ui): Introduce the AllRemoteEvents type #4370
Blocked by refactor(ui): Create a mapping between remote events to timeline items #4377
Address [meta] EventCache storage #3280
Reboot of feat(ui): Propagate RoomEventCache's VectorDiff to Timeline #3512

bnjbvr

Thank you, exciting to see this coming through! I've annotated the important review comments with a ⚠️ emoji, as I strongly feel those should be addressed before landing. Most of my other comments are about code style and typos, mostly. Great work!

bnjbvr · 2024-12-18T14:24:09Z

crates/matrix-sdk-ui/src/timeline/builder.rs

+    /// 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
+    }


⚠️ Would it make sense that it's a feature flag on the EventCache, and that the EC would choose to emit AddTimelineItems or UpdateTimelineItems based on the feature flag? Otherwise, any other user of the event cache, that's not the timeline, has to implement the same filtering mechanism.

I was thinking about this quite a lot too.

First off, this setting is temporary. It's gonna be removed as soon as we consider RoomEventCacheUpdate::UpdateTimelineEvents stable and we want to switch to it. The other variant, AddTimelineEvents will be removed at the same time.

Second, we have validated this approach during our team meeting :-p.

Third, while I share your concerns, putting the setting on the EventCache makes it so the Timeline can receive both inputs at the same time (AddTimelineEvents and UpdateTimelineEvents), which can create a lot of confusion and inconsistent state.

Fourth, this setting is used to decide if the Timeline has to apply its own deduplication mechanism or not:

with AddTimelineEvents: yes, it has to,

with UpdateTimelineEvents: no, because it's expected to be handle by the VectorDiff. Putting the settings on the EventCachemakes it way more difficult for theTimelineto decide. Actually, it requires a setting in theTimeline` anyway.

Thoughts?

Third, while I share your concerns, putting the setting on the EventCache makes it so the Timeline can receive both inputs at the same time (AddTimelineEvents and UpdateTimelineEvents), which can create a lot of confusion and inconsistent state.

No: the idea is that, the EventCache would emit AddTimelineEvents if !feature_flag, and emit UpdateTimelineEvents if feature_flag, not both at the same time.

Re: fourth, you're right the timeline would need to get this information; it's relatively trivial to add a getter on the feature flag in the event cache, and get the feature flag value from the TimelineBuilder.

(Re: second, sometimes people change their mind when they realize they were wrong, and I definitely was when I validated it 😇)

Overall, I would even call for not having a feature flag at all, and remove the previous code entirely, but I understand that it's a big change, and it would be nice to test in isolation (especially before the EOY holidays). So my preference would be to have a feature flag on the EventCache, for consistency (it would also simplify code at the FFI layer, I suppose), but we can probably live with the feature flag on the Timeline, if you strongly prefer it.

crates/matrix-sdk-ui/src/timeline/controller/mod.rs

crates/matrix-sdk-ui/src/timeline/controller/state.rs

crates/matrix-sdk-ui/src/timeline/controller/observable_items.rs

crates/matrix-sdk-ui/src/timeline/event_handler.rs

crates/matrix-sdk-ui/src/timeline/pagination.rs

crates/matrix-sdk/src/event_cache/pagination.rs

bnjbvr · 2024-12-18T17:17:04Z

(Extra review comment, since I just thought about it: might be nice to expose the feature flag at the FFI layer, for testing purposes?)

Hywan · 2024-12-18T20:22:06Z

The FFI part will be done in another PR. I wanted to keep this PR as small and simple as possible first.

bnjbvr

Approving, thanks.

I think it might be nice to have the FFI feature flag as soon as possible, and/or enable the feature flag if the event cache storage feature flag is set; otherwise, it's expected that this will break the caching feature (now that #4427 has landed).

Thank you, great work there!!

This patch adds `with_vectordiffs_as_inputs` on `TimelineBuilder` and `vectordiffs_as_inputs` on `TimelineSettings`. This new flag allows to transition from one system to another for the `Timeline`, when enabled, the `Timeline` will accept `VectorDiff<SyncTimelineEvent>` for the inputs instead of `Vec<SyncTimelineEvent>`.

Hywan · 2024-12-20T09:46:49Z

Okay so TimelineSettings::vectordiffs_as_inputs is enabled by default if and only if EventCache::has_storage() returns true. That way, we have a unique flag for both features.

The EventCache keeps sending both AddTimelineEvents and UpdateTimelineEvents though, so that we are not sure to not break existing tests with this PR in case of.

This patch adds a new variant to `RoomEventCacheUpdate`, namely `UpdateTimelineEvents. It's going to replace `AddTimelineEvents` soon once it's stable enough. This is a transition. They are read by the `Timeline` if and only if `TimelineSettings::vectordiffs_as_inputs` is turned on.

…handle_remote_events_with_diffs`. This patch updates `TimelineStateTransaction::handle_remote_events_with_diffs` to support `VectorDiff::Append`.

…n::handle_remote_events_with_diffs`. This patch updates `TimelineStateTransaction::handle_remote_events_with_diffs` to support `VectorDiff::PushFront`.

…::handle_remote_events_with_diffs`. This patch updates `TimelineStateTransaction::handle_remote_events_with_diffs` to support `VectorDiff::PushBack`.

…andle_remote_events_with_diffs`. This patch updates `TimelineStateTransaction::handle_remote_events_with_diffs` to support `VectorDiff::Clear`.

This patch adds the `ObservavbleItems::insert_remote_event` method. This is going to be useful to implement `VectorDiff::Insert` inside `TimelineStateTransaction::handle_remote_events_with_diffs`.

This patch adds the `AllRemoteEvents::range` method. This is going to be useful to support `VectorDiff::Insert` inside `TimelineStateTransaction::handle_remote_events_with_diffs`.

…handle_remote_events_with_diffs`. This patch updates `TimelineStateTransaction::handle_remote_events_with_diffs` to support `VectorDiff::Insert`.

…:handle_remote_events_with_diffs`. This patch updates `TimelineStateTransaction::handle_remote_events_with_diffs` to support `VectorDiff::Remove,`.

The `Timeline` has its own remote event deduplication mechanism. But we are transitioning to receive updates from the `EventCache` via `VectorDiff`, which are emitted via `RoomEvents`, which already runs its own deduplication mechanism (`matrix_sdk::event_cache::Deduplicator`). Deduplication from the `EventCache` will generate `VectorDiff::Remove` for example. It can create a conflict with the `Timeline` deduplication mechanism. This patch updates the deduplication mechanism from the `Timeline` when adding or updating remote events to be conditionnal: when `TimelineSettings::vectordiffs_as_inputs` is enabled, the deduplication mechanism of the `Timeline` is silent, it does nothing, otherwise it runs.

This patch updates this branch to `main` where `DayDivider` has been renamed `DateDivider`.

Since we have added a new variant to `RoomEventCacheUpdate`, a macro hits the recursion limit. It needs to be updated in order for tests to run again.

…uickly. This patch adds a trick around `SyncTimelineEvent` to avoid reaching the `recursion_limit` too quickly. Read the documentation in this patch to learn more.

A previous patch deduplicates the remote events conditionnally. This patch does the same but for timeline items. The `Timeline` has its own deduplication algorithm (for remote events, and for timeline items). The `Timeline` is about to receive its updates via the `EventCache` which has its own deduplication mechanism (`matrix_sdk::event_cache::Deduplicator`). To avoid conflicts between the two, we conditionnally deduplicate timeline items based on `TimelineSettings::vectordiffs_as_inputs`. This patch takes the liberty to refactor the deduplication mechanism of the timeline items to make it explicit with its own methods, so that it can be re-used for `TimelineItemPosition::At`. A specific short-circuit was present before, which is no more possible with the rewrite to a generic mechanism. Consequently, when a local timeline item becomes a remote timeline item, it was previously updated (via `ObservableItems::replace`), but now the local timeline item is removed (via `ObservableItems::remove`), and then the remote timeline item is inserted (via `ObservableItems::insert`). Depending of whether a virtual timeline item like a date divider is around, the position of the removal and the insertion might not be the same (!), which is perfectly fine as the date divider will be re-computed anyway. The result is exactly the same, but the `VectorDiff` updates emitted by the `Timeline` are a bit different (different paths, same result). This is why this patch needs to update a couple of tests.

This patch allows the paginated events of a `Timeline` to be received via `RoomEventCacheUpdate::UpdateTimelineEvents` as `VectorDiff`s.

…ache storage is enabled. This patch automatically enables `TimelineSettings::vectordiffs_as_inputs` if and only if the event cache storage is enabled.

codecov · 2024-12-20T12:56:31Z

Codecov Report

Attention: Patch coverage is 42.44898% with 141 lines in your changes missing coverage. Please review.

Project coverage is 85.03%. Comparing base (519f281) to head (60fefdd).
Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
...tes/matrix-sdk-ui/src/timeline/controller/state.rs	15.78%	64 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/event_handler.rs	47.16%	56 Missing ⚠️
...rates/matrix-sdk-ui/src/timeline/controller/mod.rs	9.09%	10 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/builder.rs	63.15%	7 Missing ⚠️
...sdk-ui/src/timeline/controller/observable_items.rs	60.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4408      +/-   ##
==========================================
- Coverage   85.41%   85.03%   -0.38%     
==========================================
  Files         283      283              
  Lines       31559    31736     +177     
==========================================
+ Hits        26955    26988      +33     
- Misses       4604     4748     +144

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hywan mentioned this pull request Dec 12, 2024

[meta] EventCache storage #3280

Open

37 tasks

Hywan force-pushed the feat-ui-timeline-receive-vectordiff-2 branch 6 times, most recently from c44c178 to 0ca5852 Compare December 17, 2024 16:36

bnjbvr mentioned this pull request Dec 17, 2024

feat(event cache): avoid storing useless prev-batch tokens #4427

Merged

Hywan force-pushed the feat-ui-timeline-receive-vectordiff-2 branch 4 times, most recently from 92cdf2e to 0aa794f Compare December 18, 2024 11:49

Hywan marked this pull request as ready for review December 18, 2024 12:03

Hywan requested a review from a team as a code owner December 18, 2024 12:03

Hywan requested review from jmartinesp and bnjbvr and removed request for a team and jmartinesp December 18, 2024 12:03

Hywan force-pushed the feat-ui-timeline-receive-vectordiff-2 branch from 0aa794f to 3821440 Compare December 18, 2024 13:10

bnjbvr requested changes Dec 18, 2024

View reviewed changes

bnjbvr self-requested a review December 19, 2024 13:18

bnjbvr approved these changes Dec 19, 2024

View reviewed changes

Hywan force-pushed the feat-ui-timeline-receive-vectordiff-2 branch from 2a0277a to bbf88a9 Compare December 20, 2024 09:44

Hywan added 3 commits December 20, 2024 11:57

feat(ui): Add blank handle_remote_events_with_diffs.

c48255d

task(ui): Support VectorDiff::Append in `TimelineStateTransaction::…

07b5313

…handle_remote_events_with_diffs`. This patch updates `TimelineStateTransaction::handle_remote_events_with_diffs` to support `VectorDiff::Append`.

Hywan added 16 commits December 20, 2024 11:57

task(ui): Support VectorDiff::PushFront in `TimelineStateTransactio…

5a5ad77

…n::handle_remote_events_with_diffs`. This patch updates `TimelineStateTransaction::handle_remote_events_with_diffs` to support `VectorDiff::PushFront`.

task(ui): Support VectorDiff::PushBack in `TimelineStateTransaction…

49d8a39

…::handle_remote_events_with_diffs`. This patch updates `TimelineStateTransaction::handle_remote_events_with_diffs` to support `VectorDiff::PushBack`.

task(ui): Support VectorDiff::Clear in `TimelineStateTransaction::h…

07ed0b8

…andle_remote_events_with_diffs`. This patch updates `TimelineStateTransaction::handle_remote_events_with_diffs` to support `VectorDiff::Clear`.

task(ui): Add ObservableItems::insert_remote_event.

c4da2eb

This patch adds the `ObservavbleItems::insert_remote_event` method. This is going to be useful to implement `VectorDiff::Insert` inside `TimelineStateTransaction::handle_remote_events_with_diffs`.

task(ui): Add AllRemoteEvents::range.

f8f7443

This patch adds the `AllRemoteEvents::range` method. This is going to be useful to support `VectorDiff::Insert` inside `TimelineStateTransaction::handle_remote_events_with_diffs`.

task(ui): Support VectorDiff::Insert in `TimelineStateTransaction::…

e2a8441

…handle_remote_events_with_diffs`. This patch updates `TimelineStateTransaction::handle_remote_events_with_diffs` to support `VectorDiff::Insert`.

task(ui): Support VectorDiff::Remove, in `TimelineStateTransaction:…

6ef9f73

…:handle_remote_events_with_diffs`. This patch updates `TimelineStateTransaction::handle_remote_events_with_diffs` to support `VectorDiff::Remove,`.

task(ui): DayDivider has been renamed DateDivider.

54877e9

This patch updates this branch to `main` where `DayDivider` has been renamed `DateDivider`.

test(ui): Increase the recursion_limit.

7385415

Since we have added a new variant to `RoomEventCacheUpdate`, a macro hits the recursion limit. It needs to be updated in order for tests to run again.

fix(common): Use a trick to avoid hitting the recursion_limit too q…

14597ed

…uickly. This patch adds a trick around `SyncTimelineEvent` to avoid reaching the `recursion_limit` too quickly. Read the documentation in this patch to learn more.

refactor(ui): Timeline receives pagination events as VectorDiffs!

c4becc2

This patch allows the paginated events of a `Timeline` to be received via `RoomEventCacheUpdate::UpdateTimelineEvents` as `VectorDiff`s.

chore(ui): Make Clippy happy.

1390a50

feat(ui): Enable TimelineSettings::vectordiffs_as_inputs if event c…

0466742

…ache storage is enabled. This patch automatically enables `TimelineSettings::vectordiffs_as_inputs` if and only if the event cache storage is enabled.

refactor(sdk): Rename two variables.

c3094bb

Hywan force-pushed the feat-ui-timeline-receive-vectordiff-2 branch 3 times, most recently from 5bb2b11 to 70c3d2a Compare December 20, 2024 12:25

test: Increase timeout for codecoverage.

60fefdd

Hywan force-pushed the feat-ui-timeline-receive-vectordiff-2 branch from 70c3d2a to 60fefdd Compare December 20, 2024 12:42

Hywan merged commit f4b50db into matrix-org:main Dec 20, 2024
41 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui): `Timeline` consumes updates as `VectorDiff`s from the `EventCache` #4408

feat(ui): `Timeline` consumes updates as `VectorDiff`s from the `EventCache` #4408

Hywan commented Dec 12, 2024 •

edited

Loading

bnjbvr left a comment

bnjbvr Dec 18, 2024

Hywan Dec 18, 2024

bnjbvr Dec 19, 2024

bnjbvr commented Dec 18, 2024

Hywan commented Dec 18, 2024 •

edited

Loading

bnjbvr left a comment

Hywan commented Dec 20, 2024

codecov bot commented Dec 20, 2024 •

edited

Loading

feat(ui): Timeline consumes updates as VectorDiffs from the EventCache #4408

feat(ui): Timeline consumes updates as VectorDiffs from the EventCache #4408

Conversation

Hywan commented Dec 12, 2024 • edited Loading

bnjbvr left a comment

Choose a reason for hiding this comment

bnjbvr Dec 18, 2024

Choose a reason for hiding this comment

Hywan Dec 18, 2024

Choose a reason for hiding this comment

bnjbvr Dec 19, 2024

Choose a reason for hiding this comment

bnjbvr commented Dec 18, 2024

Hywan commented Dec 18, 2024 • edited Loading

bnjbvr left a comment

Choose a reason for hiding this comment

Hywan commented Dec 20, 2024

codecov bot commented Dec 20, 2024 • edited Loading

Codecov Report

feat(ui): `Timeline` consumes updates as `VectorDiff`s from the `EventCache` #4408

feat(ui): `Timeline` consumes updates as `VectorDiff`s from the `EventCache` #4408

Hywan commented Dec 12, 2024 •

edited

Loading

Hywan commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 20, 2024 •

edited

Loading