feat(ui): Client-side sorting in RoomList

[Hywan]

The goal of this PR is to implement client-side sorting in RoomList.

So far, we used to use the sort parameter of sliding sync to sort the rooms. It is set to by_recency and by_name (see Sliding Window API in MSC3675). But it brings some problems.

Problems

name is not the name

Sorting by name might be wrong because: the name is computed by the proxy, and was wrongly calculated sometimes, see the following patches that try to fix this problem:

• base: sort the heroes in the computed display name alphabetically #3377
• room list service: always request the room name event #3424
• room names: better make use of the hero names for locally computing a room display name #3461
• base: cache the computed display name and reuse that in the search filters #3516

We no longer use the name value from sliding sync anymore. Thus, sorting by_name may result in incorrect results: the sliding sync proxy sorts 2 rooms by their name, but the names that are calculated by the SDK can be different, thus resulting into inconsistent ordering on the client-side.

ops is costly and error-prone!

The ordering was ensured by sliding sync under the form of “sync operations” (see Sliding Window API in MSC3575). This is costly for the server, which is a major argument on itself. Removing these “sync operations” would reduce the charge of running the sliding sync proxy. But we also believe it might be the source of multiple bugs, one of them is we get duplicated rooms, see for example:

• Duplicate DM in room list seen on Element X iOS #2773
• Duplicate Rooms/People when leaving some element-hq/element-x-ios#2735
• https://github.com/element-hq/element-x-android-rageshakes/issues/1245
• https://github.com/element-hq/element-x-ios-rageshakes/issues/1793
• https://github.com/element-hq/element-x-ios-rageshakes/issues/1031

Modifying sort might feel slow

If the user wants to change the ordering, we have to send new requests, which are going to refresh the entire room list. Things can feel very slow, and “webby”/non-native. This is not the case with filters!, where everything happens dynamically, fast, in real-time. It's been possible because of contributions on eyeball and inside the SDK itself:

• Create eyeball-im-util with FilteredVectorSubscriber API jplatte/eyeball#19
• feat(ui): Implement RoomList::entries_with_dynamic_filter #2392
• feat(ui): Implement the “fuzzy match room name” filter #2335
• feat(ui): Create a new normalized_match_room_name filter #2428
• feat(ui): Add the none filter #2594
• and so on

The unnecessary RoomListEntry

So far, since sorting happens server-side, our sliding sync implementation (matrix_sdk::sliding_sync) provides a stream of rooms as a RoomListEntry, which is an enum mimicking the semantics of sliding sync's own representation of a room with Empty, Invalidated(RoomId) or Filled(RoomId). The major problem is that this RoomListEntry spreads everywhere in the matrix_sdk::sliding_sync and matrix_sdk_ui::room_list_service APIs. It means the users receive RoomListEntry in their Stream of updates and so on. It implies that the users manipulate a RoomId only, and thus must call a method such as RoomListService::room to get a proper Room! This is unnecessary FFI boundary crossings, which is costly. The whole API feels unnecessarily complex. Ideally, the user wants to manipulate a Stream<Item = Vec<VectorDiff<Room>>> directly instead of Stream<Item = Vec<VectorDiff<RoomListEntry>>>.

visible_rooms seems broken and will break for sure

RoomListService uses sliding sync with 2 lists: all_rooms and visible_rooms. all_rooms to fetch all rooms with timeline_limit=1, and visible_rooms to fetch a particular range of rooms with timeline_limit=20. In some cases, for some users, visible_rooms feels broken or inactive. The rooms in the user's app viewport are not preloaded with a timeline of 20 events. Ideally, we want to replace visible_rooms by room subscriptions (see Room Subscription API in MSC3575).

Once we have client-side sorting, visible_rooms will be entirely broken because a server-side range won't map to a client-side range anymore, making the use of visible_rooms impossible.

Simplified native sliding sync implementation

Finally, the last reason to have client-side sorting is that we are trying to simplify the sliding sync specification. Why? Because we are trying to sunset the proxy in favor of a native implementation inside Synapse, see:

• Add Sliding Sync /sync endpoint (initial implementation) element-hq/synapse#17187
• Add is_dm filtering to Sliding Sync /sync element-hq/synapse#17277
• Add is_encrypted filtering to Sliding Sync /sync element-hq/synapse#17281
• Add stream_ordering sort to Sliding Sync /sync element-hq/synapse#17293
• Add is_invite filtering to Sliding Sync /sync element-hq/synapse#17335
• Return some room data in Sliding Sync /sync element-hq/synapse#17320
• and so on

And for the SDK side, see:

• [meta] SSS & PPS #3647

Sliding sync proxy has served us well, and now we know how to provide stable and fast sync to the Matrix ecosystem. It was an experimental project. Now it's time to move onto production ready implementation, and a clean up is necessary. We want to remove the “sync operations”, which means the client has to support its own sorting.

Here we are

And here we are. Matrix Rust SDK must implement client-side sorting inside matrix_sdk_ui::room_list_service. matrix_sdk::sliding_sync, similarly to matrix_sdk::sync, sends requests, receives responses, creates or updates the rooms and so on. Then, it's up to matrix_sdk_ui::room_list_service to provide an API on tops of the rooms inside the SDK.

To be here today, we needed to tackle these tasks:

• [Meta] SlidingSyncRoom tech debt #3079 (with a lot of sub tasks…)
• chore(ui): room_list_service::Room::latest_event no longer uses SlidingSyncRoom (+ drop SlidingSyncRoomExt) #3540
• feat(base): Make Store::get_rooms wayyy faster #3552

Then, the first step was Client to export a Stream<Item = Vec<VectorDiff<Room>>>. This was done with:

• feat(base) Client-side sorting, prelude: Implement Client::rooms_stream #3068

After that, we needed room_list_service::Room to be infallible and non-async. This was done with:

• feat(ui): RoomListService::room is no longer async! #3551
• feat(sdk): room_list_service::Room::new is now infallible #3586

Finally, we needed eyeball to provide a Sort stream adapter to be able to sort a Stream (yup!):

• feat: Implement the SortBy stream adapter jplatte/eyeball#43

And now, … only now, we have everything to implement client-side sorting dear readers!

Tasks

• Migrate RoomList to manipulate Room instead of RoomListEntry
• Update all filters to use Room instead of RoomListEntry
• Remove visible_rooms
• Implement one sorter to mimic the actual sorting behaviour from the sliding sync proxy as configured by the RoomList

[codecov]

Codecov Report

Attention: Patch coverage is 83.49515% with 17 lines in your changes missing coverage. Please review.

Project coverage is 84.26%. Comparing base (38a18c3) to head (3588b88).
Report is 35 commits behind head on main.

Files	Patch %	Lines
...s/matrix-sdk-ui/src/room_list_service/room_list.rs	56.52%	10 Missing ⚠️
...x-sdk-ui/src/room_list_service/filters/category.rs	50.00%	2 Missing ⚠️
...list_service/filters/normalized_match_room_name.rs	0.00%	2 Missing ⚠️
...atrix-sdk-ui/src/room_list_service/sorters/name.rs	60.00%	2 Missing ⚠️
...matrix-sdk-ui/src/room_list_service/filters/mod.rs	94.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3585      +/-   ##
==========================================
+ Coverage   84.21%   84.26%   +0.04%     
==========================================
  Files         256      259       +3     
  Lines       26553    26505      -48     
==========================================
- Hits        22362    22334      -28     
+ Misses       4191     4171      -20     

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

[bnjbvr]

Great piece! I think there's a bug when it comes to the UTD hook, so would like to see just the minimal changes needed to make this work again. To make the next round of review super fast, I would propose to have fixup! commits so I can quickly check these, and then you could autosquash and we could merge later. How does that sound?

[bnjbvr]

This shouldn't be needed, can you revert it, please? or if you need it, a code comment would be appreciated to explain why! in the e2ee equivalent, it's required because there's no direct causality between "receiving e2ee info" and "updating the room list", but a successful decryption of the latest event requires the room list to be updated, hence the true there. Here, since we're receiving a room update, the room should automatically update because the room stream will fire.

[Hywan]

The room stream won't fire if the room already exists. A room info update is likely to trigger a room list update if the latest event changes for example. Hence the true here. I'm adding a comment.

[bnjbvr]

I think this would lead to spurious updates, because we're already saving room infos explicitly here. (The room state management is a bit of a mess, we should likely add this to the spring cleaning list). Putting true will force a spurious update here, which might not be an issue in testing, so let's see if we can keep it.

[Hywan]

Ideally, we want to detect whether an update is necessary instead of always coercing to true. But let's do that in another PR.

[bnjbvr]

The way it worked before: changing the room's data meant the vectordiff would include a change for that room (even if it's not moved position in the list: just to notify that there's a change in the room, so the subscribers can be notified). Hence, we didn't need to pass true here, since the change would happen automatically.

Has this changed? Is this the actual reason why you need to pass true here?

[bnjbvr]

Can you keep a test with this code, please?

[Hywan]

What do you mean?

[bnjbvr]

Ah, I meant to select the few lines above too. I think you deleted some test code that is now untested, and that would be nice to keep as some sort of unit testing for the manual update functionality.

[Hywan]

I think it deserves way more tests than only two tests. I'll add a couple more tests with another PR, as I also think we should revisit this room info update thingy.

[Hywan]

@bnjbvr I've rebased on top of main but I've marked all the new commits as !fixup, except 98aae99, 50c804b, 2352978, 6132841, 7bd0427, and 824cb06.

[bnjbvr]

Sweet! The commit history is messy and I would like you to tidy it up before merging, please, to avoid intermediate commits that should've been folded into other commits in the first place.

Otherwise, the new commits look good; I haven't looked again at the previous ones, since you've addressed all the comments there.

Thank you very much, and great work here 🥳

[Hywan]

I've changed the code to use the timestamp from the sliding sync room response instead of the LatestEvent origin server timestamp. Why? Because if we don't have a LatestEvent, then we can't sort the rooms. On a fresh start, it happens pretty easily, and the room list feels pretty broken. With this new strategy, everything looks like before the PR, which is what we are trying to achieve so far, except things are computed client-side instead of server-side.

@bnjbvr Can you review 9744ad4, 2229489 and 063fa52 please? @jmartinesp confirms it now work similarly to what we have previously, and I confirm that too with multiverse.

[Hywan]

@bnjbvr One more time with 1f1ff3d please 🥺?

[bnjbvr]

yay

[Hywan]

Complement crypto is expected but I've disabled it. Force merging the PR as all other CI jobs are fine.