sdk-ui: Add `TimelineFocus::PinnedEvents` #3773

jmartinesp · 2024-07-29T11:03:19Z

Changes

Create a new type of TimelineFocus, TimelineFocus::PinnedEvents that will use the pinned event ids in RoomInfo to load the associated events, cache them and sort them chronologically.
Create PinnedEventsRoom trait to be able to provide the necessary data to the new timeline type.
Change the is_live parameter used in the Timeline code for a LiveTimelineUpdatesAllowed enum to distinguish between timelines that need to process every update (live timeline), those who don't need to process any (the focused one) and those which may need to process some items (pinned events one).
Add tests and a benchmark for the concurrent pinned event loading.
Public API changes documented in changelogs (optional)

Signed-off-by:

codecov · 2024-08-01T14:58:42Z

Codecov Report

Attention: Patch coverage is 87.97468% with 19 lines in your changes missing coverage. Please review.

Project coverage is 84.04%. Comparing base (21efd60) to head (4b33508).
Report is 1 commits behind head on main.

Files	Patch %	Lines
benchmarks/benches/room_bench.rs	0.00%	11 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/inner/mod.rs	85.71%	5 Missing ⚠️
...matrix-sdk-ui/src/timeline/pinned_events_loader.rs	96.49%	2 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/event_handler.rs	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3773      +/-   ##
==========================================
+ Coverage   84.03%   84.04%   +0.01%     
==========================================
  Files         259      261       +2     
  Lines       27202    27334     +132     
==========================================
+ Hits        22859    22974     +115     
- Misses       4343     4360      +17

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

andybalaam

Looks good! Some questions.

andybalaam · 2024-08-01T16:07:11Z

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

@@ -175,6 +176,27 @@ impl TimelineBuilder {
        let room = inner.room();
        let client = room.client();

+        let mut pinned_event_ids_stream = room.pinned_event_ids_stream();


Can this line move inside the if?

andybalaam · 2024-08-01T16:09:25Z

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

-                                match origin {
-                                    EventsOrigin::Sync => RemoteEventOrigin::Sync,
+                            // Special case for pinned events: when we receive new events what we'll do is
+                            // updated the cache for those events that are pinned and reload the


updated -> update

andybalaam · 2024-08-01T16:10:20Z

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

+                            // updated the cache for those events that are pinned and reload the
+                            // list.
+                            match &*focus.clone() {
+                                TimelineFocus::PinnedEvents { .. } => {


So we ignore the events that appeared via AddTimelineEvents and just refresh the whole list? Might be good to make this clear in the comment as I didn't quite get it.

Ok, I'll try to come up with a better comment to explain this.

andybalaam · 2024-08-01T16:11:10Z

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

+                            // Special case for pinned events: when we receive new events what we'll do is
+                            // updated the cache for those events that are pinned and reload the
+                            // list.
+                            match &*focus.clone() {


If you can, I would prefer an if let here instead of match, because the pinned case is currently making the normal case hard to see.

andybalaam · 2024-08-01T16:15:31Z

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

+        }
+
+        if !event_ids_to_request.is_empty() {
+            let semaphore = Arc::new(Semaphore::new(self.max_concurrent_requests));


Can you extract the logic for performing-things-concurrently-with-a-limit into a separate function, and then pass in the list of inputs (event IDs) and a closure for the thing to do? Don't worry if not.

It seems like I can't because I'd have to make the closure async and that's unstable.

Normally when you want an async closure you can get away with a closure that returns a Future by doing:

|args| async { body }

Not sure here.

I think I'll try doing this in a separate PR, since the team implementing the feature is a bit blocked without it merged.

Yep, fine - thanks.

andybalaam · 2024-08-01T16:17:19Z

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

+    fn room_pinned_event_ids(&self) -> Vec<OwnedEventId>;
+
+    /// Checks whether an event id is pinned in this room.
+    fn room_is_pinned_event_id(&self, event_id: &EventId) -> bool;


I think just is_pinned_event_id or is_pinned_event, or even is_pinned.

(Basically I think room is not helpful here.

The problem with these is there already are some existing methods in Room that have these names and they access private properties, and then I had to create new methods to expose them in the PinnedEventsRoom trait and use them in this file.

I can't just replace the existing methods because they need access to those private properties to work and they can't have the same name because the compiler will mix them up. Unless there's some way to distinguish between those?

A good example of this is PinnedEventsRoom::room_pinned_event_ids and Room::pinned_event_ids.

Ok, I think I got it with 237ee9a.

Looks good.

andybalaam · 2024-08-01T16:18:18Z

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

+    async fn room_event(&self, event_id: &EventId) -> Result<SyncTimelineEvent, PaginatorError>;
+
+    /// Get the pinned event ids for a room.
+    fn room_pinned_event_ids(&self) -> Vec<OwnedEventId>;


I suggest pinned_event_ids

Same as above, 237ee9a.

andybalaam · 2024-08-01T16:20:36Z

benchmarks/benches/room_bench.rs

+
+    runtime.block_on(
+        Mock::given(method("GET"))
+            .and(path_regex(r"/_matrix/client/r0/rooms/.*/event/.*"))


Is it possible to make the mocked response slow? If so I think it would be helpful, to demonstrate that we are fast because we wait for the responses in parallel.

The mocked response already has a delay of 50ms. When tested agains the sequential approach, the concurrent one was 0.55s vs 5.3s.

Perfect, thanks!

… stream and checking whether an event is pinned in the room or not

…nedEvents` to load the live pinned events for a room. Add `PinnedEventsLoader` to encapsulate this logic, being able to provide a max number of events to load and how many can be loaded at the same time. Also implement an event cache for it. Add `PinnedEventsRoom` trait to use it in the same way as `PaginableRoom`, only for pinned events.

…line

Velin92 mentioned this pull request Jul 31, 2024

Pinned items timeline implementation for the banner element-hq/element-x-ios#3099

Merged

jmartinesp force-pushed the jme/feat/focused-pinned-events-timeline branch from ba74a50 to dae69d2 Compare August 1, 2024 13:16

jmartinesp changed the title ~~WIP: Add PinnedEvents focus for the timeline~~ sdk-ui: Add TimelineFocus::PinnedEvents Aug 1, 2024

jmartinesp marked this pull request as ready for review August 1, 2024 13:32

jmartinesp requested a review from a team as a code owner August 1, 2024 13:32

jmartinesp requested review from andybalaam and removed request for a team August 1, 2024 13:32

andybalaam approved these changes Aug 1, 2024

View reviewed changes

jmartinesp added 5 commits August 2, 2024 12:48

sdb-base: add methods for getting the pinned event ids of a Room as a…

1c313e4

… stream and checking whether an event is pinned in the room or not

benchmark: add a benchmark to measure how loading pinned events performs

28f44b3

sdk-ui: add and fix tests

3cc90a5

ffi: add FFI bindings for creating the new pinned events focused time…

4b33508

…line

jmartinesp force-pushed the jme/feat/focused-pinned-events-timeline branch from 237ee9a to 4b33508 Compare August 2, 2024 10:48

jmartinesp enabled auto-merge (rebase) August 2, 2024 10:51

jmartinesp merged commit 1ca4377 into main Aug 2, 2024
40 checks passed

jmartinesp deleted the jme/feat/focused-pinned-events-timeline branch August 2, 2024 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sdk-ui: Add `TimelineFocus::PinnedEvents` #3773

sdk-ui: Add `TimelineFocus::PinnedEvents` #3773

jmartinesp commented Jul 29, 2024 •

edited

Loading

codecov bot commented Aug 1, 2024 •

edited

Loading

andybalaam left a comment

andybalaam Aug 1, 2024

andybalaam Aug 1, 2024

andybalaam Aug 1, 2024

jmartinesp Aug 2, 2024

andybalaam Aug 1, 2024

andybalaam Aug 1, 2024

jmartinesp Aug 2, 2024

andybalaam Aug 2, 2024

jmartinesp Aug 2, 2024

andybalaam Aug 2, 2024

andybalaam Aug 1, 2024

andybalaam Aug 1, 2024

jmartinesp Aug 2, 2024

jmartinesp Aug 2, 2024

andybalaam Aug 2, 2024

andybalaam Aug 1, 2024

jmartinesp Aug 2, 2024

andybalaam Aug 1, 2024

jmartinesp Aug 2, 2024

andybalaam Aug 2, 2024

sdk-ui: Add TimelineFocus::PinnedEvents #3773

sdk-ui: Add TimelineFocus::PinnedEvents #3773

Conversation

jmartinesp commented Jul 29, 2024 • edited Loading

Changes

codecov bot commented Aug 1, 2024 • edited Loading

Codecov Report

andybalaam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdk-ui: Add `TimelineFocus::PinnedEvents` #3773

sdk-ui: Add `TimelineFocus::PinnedEvents` #3773

jmartinesp commented Jul 29, 2024 •

edited

Loading

codecov bot commented Aug 1, 2024 •

edited

Loading