feat(ui): `RoomList`, step 1: the Foundation #1953

Hywan · 2023-05-22T13:20:54Z

Address #1911.

This PR is quite large, but it's quite simple to review. It introduces the new matrix_sdk_ui::RoomList type, which acts as a state machine. The code contains the necessary documentation. If something isn't clear, please tell me.

jplatte · 2023-05-31T10:14:25Z

crates/matrix-sdk-ui/src/room_list/mod.rs

+    pub fn state(&self) -> State {
+        self.state.get()
+    }
+
+    pub fn state_stream(&self) -> impl Stream<Item = State> {
+        Observable::subscribe(&self.state)
+    }


If you just have one function that returns a Subscriber<State>, that can be used to obtain both the current value and wait for updates.

Yeah but I don't want to create a new stream every time one wants to get a single value.

"Create a new stream" is a trivial operation: https://docs.rs/eyeball/latest/src/eyeball/subscriber.rs.html#26-28

Addressed in #2035.

This patch does several things. First, `SlidingSync::on_list` is now async, and accept async closures. Second, `SlidingSync::lists` and `::rooms` are behind an `AsyncRwLock` instead of a `StdRwLock`. The rest of the patch updates the consequence of this.

This patch adds a new method on `SlidingSyncList` named `room_list_filtered_stream`, which uses the new `eyeball-im-util` crate with its new `FilteredSubscriber` type.

This patch implements the `RoomList::entries` method, which allows to get a stream of `VectorDiff` over the room list, and that can be filtered.

…ng_sync`.

This patch lands the first design of the `Input` API. An `Input` is something external that can be understood by the `RoomList` state machine. The first inpuput is `Viewport` to change the “viewport” of the `RoomList`, which translates to the change of the ranges of one specific Sliding Sync list.

bindings/matrix-sdk-ffi/src/sliding_sync.rs

This patch implements `RoomList::room`, which returns a `Result<Room>`: the room ay or may not exist. A new `Room` type is created. It wraps an `Arc<RoomInner>` type, so that it's cheap to clone it. The `RoomInner` type owns a `SlidingSyncRoom` and `matrix_sdk::room::Room` type. If some API or data are missing on the former, the latter acts as a backup. This is the case for the `Room::name` method which makes it best to return a name to the caller.

This patch changes `RoomInner` to have an `room: Option<matrix_sdk::room::Room>` field to `room: matrix_sdk::room::Room`. `room` is not longer an `Option`. Then, it's easier to have a new `timeline: Timeline` field, along with a `sneaky_timeline: Timeline` field. Both are used by the new `Room::timeline()` and `Room::latest_event()` method.

First off, `SlidingSync::sync_once` no longer returns `Option<UpdateSummary>` but `UpdateSummary`. It simplifies the rest of the patch. Next, `SlidingSync::sync` returns either `Ok(…)` and continues to sync, or it returns `Err(…)` and it stops the sync stream immediately. No more error could be returned with the stream to continue syncing. Finally, the `UnknownPos` error no longer emits an err, but silently skip over the sync-loop until the threshold is reached; which in this case will generate a proper error.

This patch handles errors from the Sliding Sync loop properly. When an error is received, the `RoomList` state machine enters the `Terminated` state. Immediately after that, the sync stream is stopped. When the sync stream is restarted, the next state will be calculated. When the current state is `Terminated`, the next state is the state that led to the `Terminated` state. To avoid having a “first” huge sync (imagine a room list of 1000 rooms, you don't want to “resume” from an error with a first sync for 1000 rooms), lists are partially “reset” depending of the state where the machine is in. An important test has been added to test all possibles cases with errors and list' states.

bnjbvr

Awesome stuff, and all the testing gives lot of confidence that it behaves as intended, thanks and great work!

I'm thinking about #2004 in this PR. On the one hand this PR is already gigantic, on the other hand it's really trivial to fix as a one-liner; feel free to embed that fix here too, if you'd like to!

My only concerns below is about add_cached_list that I think we should temporarily revert to a add_list, to not trigger our ultra slow cache restoration edge case. Other than that, this is good to go 👍

crates/matrix-sdk-ui/src/room_list/mod.rs

crates/matrix-sdk/src/sliding_sync/list/mod.rs

crates/matrix-sdk-ui/src/room_list/mod.rs

bnjbvr · 2023-06-05T12:41:39Z

crates/matrix-sdk-ui/src/room_list/mod.rs

+    /// An operation has been requested on an unknown list.
+    #[error("Unknown list `{0}`")]
+    UnknownList(String),


This is a bit strange to expose this error, as it's very internal: it's used only for the ALL_ROOMS_LIST, which is always added at start. I think it would be confused for the user; can we unwrap() (scary) or at least soft fail with a warning, when that list isn't found anymore?

At the limit. InputHasNotBeenApplied really means "I couldn't find the list VISIBLE_ROOMS", so if you wanted to keep UnknownList, we could reuse it here.

I agree. Let's address that with another PR maybe.

bnjbvr · 2023-06-05T12:44:19Z

crates/matrix-sdk-ui/src/room_list/mod.rs

+    /// An input was asked to be applied but it wasn't possible to apply it.
+    #[error("The input has been not applied")]
+    InputHasNotBeenApplied(Input),


We could encode a type state machine, so that the apply_input method is only accessible if you're after the AllRooms state 🙈 (and get rid of this error)

Eh, yeah, maybe!

crates/matrix-sdk-ui/src/room_list/mod.rs

crates/matrix-sdk-ui/tests/integration/room_list.rs

bnjbvr · 2023-06-05T13:11:10Z

crates/matrix-sdk-ui/src/room_list/mod.rs

+        let sliding_sync = client
+            .sliding_sync()
+            .storage_key(Some("matrix-sdk-ui-roomlist".to_string()))
+            .add_cached_list(


⚠️ This is what caused the slowness on startup of ElementX iOS, since the ALL_ROOMS list is likely to contain many lists and rooms, and reloading from cache + building a timeline will attempt to decrypt all the previous messages present in those rooms.

Perhaps disable with a big TODO that it should be added back in the future, when the timeline is properly cached?

Let's address in another PR. The goal of this PR is not to fix this problem.

Just to make sure we're on the same page: I am fine with that, but that means we can't use the new Room API until we have 1. either made restoring much faster, 2. or replaced this line with add_list until we do (1). I'm suggesting we could do (2) right now, so as to be able to use the Room API as soon as possible in the apps.

bnjbvr · 2023-06-05T13:46:16Z

Another question raised by @jplatte: should we cap the number of events in a timeline owned in a room, to avoid trashing the user's memory with too many events? If I understand correctly, elements are duplicated in the timeline_queue of the SlidingSyncRoom as well as the new Room's Timeline (and sneaky timeline), so that means multiply # of events by 3, at most?

Hywan · 2023-06-05T17:12:19Z

Another question raised by @jplatte: should we cap the number of events in a timeline owned in a room, to avoid trashing the user's memory with too many events? If I understand correctly, elements are duplicated in the timeline_queue of the SlidingSyncRoom as well as the new Room's Timeline (and sneaky timeline), so that means multiply # of events by 3, at most?

That's indeed a problem, but the goal is not to solve it here. Let's fix that with another PR.

codecov · 2023-06-05T18:50:09Z

Codecov Report

Patch coverage: 89.74% and project coverage change: +0.68 🎉

Comparison is base (9148eaa) 74.56% compared to head (16932ab) 75.24%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1953      +/-   ##
==========================================
+ Coverage   74.56%   75.24%   +0.68%     
==========================================
  Files         138      147       +9     
  Lines       15016    16052    +1036     
==========================================
+ Hits        11196    12079     +883     
- Misses       3820     3973     +153

Impacted Files	Coverage Δ
crates/matrix-sdk/src/sliding_sync/list/builder.rs	`61.66% <ø> (+61.66%)`	⬆️
...atrix-sdk/src/sliding_sync/list/room_list_entry.rs	`84.61% <ø> (+84.61%)`	⬆️
crates/matrix-sdk/src/sliding_sync/mod.rs	`86.03% <84.21%> (+86.03%)`	⬆️
crates/matrix-sdk-ui/src/room_list/mod.rs	`88.69% <88.69%> (ø)`
crates/matrix-sdk/src/sliding_sync/builder.rs	`50.00% <100.00%> (ø)`
crates/matrix-sdk/src/sliding_sync/cache.rs	`90.76% <100.00%> (ø)`
crates/matrix-sdk/src/sliding_sync/list/mod.rs	`92.78% <100.00%> (+92.78%)`	⬆️
...rix-sdk/src/sliding_sync/list/request_generator.rs	`98.24% <100.00%> (ø)`

... and 16 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Hywan mentioned this pull request May 22, 2023

[meta] Room List API #1911

Closed

62 tasks

Hywan force-pushed the feat-ui-roomlist branch 4 times, most recently from 70688d3 to b669ca5 Compare May 26, 2023 11:59

jplatte reviewed May 31, 2023

View reviewed changes

Hywan force-pushed the feat-ui-roomlist branch 3 times, most recently from 9f56b20 to f2286da Compare June 1, 2023 06:47

Hywan added 21 commits June 1, 2023 09:48

feat(ui): Oh, a roomlist module.

5f58438

!foo

7706b00

!bar

f47c2ba

!baz

92c3003

feat(ui): Create an Init state, and add state observer for RoomList.

5f81d82

feat(ui): Update according to the last patch.

d1708ec

test(ui): Improve testability of RoomList.

4e07ac1

!foo

01366a0

chore(ui): Update after the rebase.

f89060b

test(sdk): Store the sync-mode in SlidingSyncListInner only for tests.

bd175d9

test(ui): Test all RoomList actions.

26a764d

feat(ui) Add State::Terminated in RoomList.

4d6a6ac

test(ui): Clean up a test.

61f903a

feat(ui): Add room_list::Error.

1173636

test(ui): Test RoomList sync from Init to Enjoy.

aa70c85

feat(sdk): Add SlidingSyncList::room_list_filtered_stream.

f058c59

This patch adds a new method on `SlidingSyncList` named `room_list_filtered_stream`, which uses the new `eyeball-im-util` crate with its new `FilteredSubscriber` type.

feat(ui): Add RoomList::entries.

4557c2a

This patch implements the `RoomList::entries` method, which allows to get a stream of `VectorDiff` over the room list, and that can be filtered.

chore(ui): Polish after the rebase.

1caa8b1

feat(ui): Implement RoomList::entries_stream.

a3c96dc

test(ui): Write an assert_entries_stream macro.

954d798

Hywan added 4 commits June 1, 2023 15:58

fix(ffi): Update according to last commits.

f5c950c

test(sdk): Fix according to previous commits.

e2d2cf7

feat(sdk): Expose the Range and Ranges type in `matrix_sdk::slidi…

13d3899

…ng_sync`.

Hywan marked this pull request as ready for review June 1, 2023 14:57

Hywan requested a review from a team as a code owner June 1, 2023 14:57

Hywan requested review from bnjbvr and removed request for a team June 1, 2023 14:57

jplatte reviewed Jun 1, 2023

View reviewed changes

bindings/matrix-sdk-ffi/src/sliding_sync.rs Show resolved Hide resolved

Hywan added 9 commits June 2, 2023 20:06

doc(ui): Write documentation for RoomList.

8ee84be

feat(ui): Set timeline_limit for all_rooms and visible_rooms.

e18728a

test(ui): Ensure the timeline_limit is not reset.

dcad897

feat(sdk): SlidingSync::get_rooms? are now async.

537f95b

test(sdk): Simplify a test.

4a62949

bnjbvr approved these changes Jun 5, 2023

View reviewed changes

Hywan added 2 commits June 5, 2023 19:13

chore(ui): Address feedbacks.

ac7a576

Merge branch 'main' into feat-ui-roomlist

08559b5

Hywan enabled auto-merge June 5, 2023 17:22

Hywan added 4 commits June 5, 2023 19:40

chore: Make CI happy.

d1bccac

fix: Fix merge commit.

42df0d0

chore: Make Clippy happy.

e6fdcfd

chore: Make Tarpaulin happy.

16932ab

Hywan merged commit f61fdcb into matrix-org:main Jun 5, 2023
47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui): `RoomList`, step 1: the Foundation #1953

feat(ui): `RoomList`, step 1: the Foundation #1953

Hywan commented May 22, 2023 •

edited

Loading

jplatte May 31, 2023

Hywan Jun 1, 2023

jplatte Jun 1, 2023

Hywan Jun 7, 2023

bnjbvr left a comment

bnjbvr Jun 5, 2023

Hywan Jun 5, 2023

bnjbvr Jun 5, 2023

Hywan Jun 5, 2023

bnjbvr Jun 5, 2023

Hywan Jun 5, 2023

bnjbvr Jun 5, 2023

bnjbvr commented Jun 5, 2023

Hywan commented Jun 5, 2023

codecov bot commented Jun 5, 2023

feat(ui): RoomList, step 1: the Foundation #1953

feat(ui): RoomList, step 1: the Foundation #1953

Conversation

Hywan commented May 22, 2023 • edited Loading

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

bnjbvr 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

bnjbvr commented Jun 5, 2023

Hywan commented Jun 5, 2023

codecov bot commented Jun 5, 2023

Codecov Report

feat(ui): `RoomList`, step 1: the Foundation #1953

feat(ui): `RoomList`, step 1: the Foundation #1953

Hywan commented May 22, 2023 •

edited

Loading