Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(ui): Create a mapping between remote events to timeline items #4377

Merged

Conversation

Hywan
Copy link
Member

@Hywan Hywan commented Dec 4, 2024

A messenger trying to find its path in an unknown land


Don't be fooled by the number of new lines in this PR, patches are small and relatively easy to read. Most of the new lines are documentation or tests (around 1300).

The idea of this PR is to create a mapping between events to timeline items. More precisely, a mapping between event indexes and timeline item indexes. Why is this needed? We want Timeline to receive its updates via VectorDiff. Those VectorDiff are generated by the EventCache (via linked_chunk::AsVector). The indexes from these VectorDiff —like VectorDiff::Insert { index, .. } or VectorDiff::Remove { index }— are event index. The Timeline manipulates a collection of timeline items, where each item has an event ID, but no event index.

Fortunately for us, a recent patch has clarified an internal data type, now known as AllRemoteEvents (see #4370). We can re-use this type to maintain an index between the events and the timeline items.

This PR must be reviewed patch-by-patch:

  • The first 3 patches are adding the EventMeta::timeline_item_index field, and maintains it. That's it, everything works at this step!
  • The other patches are a refactoring to make this mechanism more robust:
    • It introduces ObservableItems which is a new type that replaces/wraps ObservableVector<Arc<TimelineItem>>: the idea is to provide a small API we control!
    • Then, AllRemoteEvents is moved inside ObservableItems, so that operations on the items and the events happen “atomically”, which removes many possible errors of misuses.
  • Finally, the last patches add documentations and tests.

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 93.82716% with 15 lines in your changes missing coverage. Please review.

Project coverage is 85.21%. Comparing base (ee93c27) to head (bd648cd).
Report is 47 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk-ui/src/timeline/event_handler.rs 80.00% 7 Missing ⚠️
...tes/matrix-sdk-ui/src/timeline/controller/state.rs 84.21% 3 Missing ⚠️
...rates/matrix-sdk-ui/src/timeline/controller/mod.rs 94.28% 2 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/read_receipts.rs 92.30% 2 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/day_dividers.rs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4377      +/-   ##
==========================================
+ Coverage   85.15%   85.21%   +0.05%     
==========================================
  Files         280      281       +1     
  Lines       30831    30971     +140     
==========================================
+ Hits        26254    26391     +137     
- Misses       4577     4580       +3     

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

@Hywan Hywan force-pushed the feat-ui-timeline-event-index-to-timeline-item-index branch from db20d84 to 4c534b6 Compare December 4, 2024 15:46
Hywan added 4 commits December 4, 2024 17:43
This is the foundation for the mapping between remote events and
timeline items.
…anipulated.

This patch maintains the `timeline_item_index` when a new remote events
is added or removed.
…inserted or removed.

This patch maintains the `timeline_item_index` when timeline items are
inserted or removed.
@Hywan Hywan force-pushed the feat-ui-timeline-event-index-to-timeline-item-index branch 2 times, most recently from b7c83d3 to 0376867 Compare December 4, 2024 18:44
@Hywan Hywan marked this pull request as ready for review December 4, 2024 20:25
@Hywan Hywan requested a review from a team as a code owner December 4, 2024 20:25
@Hywan Hywan requested review from poljar and removed request for a team December 4, 2024 20:25
Hywan added 10 commits December 6, 2024 11:47
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!
This patch renames `ObservableItems(Transaction)::set` to `replace`, it
conveys the semantics a bit better for new comers.
…ry`.

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`).
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).
This patch adds test suite for `AllRemoteEvents`.
@Hywan Hywan force-pushed the feat-ui-timeline-event-index-to-timeline-item-index branch from 0059ab7 to 5fc48cc Compare December 6, 2024 10:47
@Hywan
Copy link
Member Author

Hywan commented Dec 6, 2024

I think there is a problem when replacing a timeline item. I need to investigate that. Moving back the PR as a draft.

@Hywan Hywan marked this pull request as draft December 6, 2024 11:25
@Hywan
Copy link
Member Author

Hywan commented Dec 6, 2024

I think the current PR was actually alright. When editing a timeline item, its internal event_id doesn't change, I think it would break all the replies to such timeline item otherwise. I think it's fine with the actual state of this PR.

@Hywan Hywan marked this pull request as ready for review December 6, 2024 14:16
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all of this makes sense, a bit challenging to review since some aspects of my review were already addressed in subsequent comments.

I mostly left a bunch of nits here and there. The unsafe function makes me uneasy so would be good to rethink if we could express this a bit differently.

@Hywan Hywan force-pushed the feat-ui-timeline-event-index-to-timeline-item-index branch from 09a7280 to 0cc8226 Compare December 9, 2024 16:15
@Hywan Hywan requested a review from poljar December 9, 2024 16:15
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks good.

@Hywan Hywan force-pushed the feat-ui-timeline-event-index-to-timeline-item-index branch from 0cc8226 to bd648cd Compare December 10, 2024 10:04
@Hywan Hywan merged commit cf178d6 into matrix-org:main Dec 10, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants