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

[meta] Room List API #1911

Closed
62 tasks done
Hywan opened this issue May 11, 2023 · 9 comments
Closed
62 tasks done

[meta] Room List API #1911

Hywan opened this issue May 11, 2023 · 9 comments
Assignees

Comments

@Hywan
Copy link
Member

Hywan commented May 11, 2023

output

Image generated with DeepAI

We want to provide a RoomList API inside matrix-sdk. The goal is to provide an abstraction on top of SlidingSync for classical usages of it, for example in common clients (ElementX, Fractal etc.). This can also be used by bots and so on.

This meta issue provides a list of issues to achieve this goal.

Step 1, the foundation

The idea is the following. It happens in 2 phases:

  1. RoomList will create one all_rooms list, with Selective mode and timeline_limit = 0, and a range of 20/30 rooms, so that we can sync a small room list, with no timeline but at least their names,
  2. Once 1 is sent, i.e. once this all_rooms has received a response, we change its mode to Growing with a batch of 100, still with timeline_limit = 0, and it will sync all rooms, one after the other:
    1. At the same time, we create a new visible_rooms list, with Selective mode and timeline_limit = 1
    2. At the same time, we will also add two more lists: one for spaces, one for invites.

Right now, it's not possible to change the mode of a list. But nothing is difficult: it's a matter of resetting it, and exposing a setter for the mode.

This workflow above avoids to merge multiple lists together, as described in #1644 and as done right now by ElementX iOS, which can be very tricky and error-prone.

Main PRs are:

  • Add ability to change the mode of a SlidingSyncList on-the-fly.
  • Create the crates/matrix-sdk-ui/src/room_list.rs module. It's outside the crates/matrix-sdk/src/sliding_sync/ module on purpose.
  • Forward the observable room_list from a particular list to RoomList (i.e. it will be from all_rooms).
  • Expose a proper observable room_list to the public which is a “duplicate” of the previous list above, because it can be filtered. Maybe, let's take time to think about a FilteredObservable type, or a FilteredSubscriber, or something like this. This will allow to support fuzzy search. The general idea is to have a “source of truth room_list” and a “public room_list” that can be filtered
  • Accept inputs from the client:
    • Update the timeline_limit maybe (not necessary)
    • Update the range
    • Update the batch size maybe (not necessary)
    • Update the required state
    • Update the filters
    • Be able to register observers for: state, maximum_number_of_rooms, room_list diffs etc. (not necessary)
    • Get a SlidingSyncRoom
    • Be able to notify when a room is opened or closed, i.e. we need to subscribe/unsubscribe from that room
  • Add unit tests.
  • Add integration tests.

Step 2, moaaar features

Step 3, the clean up and appendix tasks

Step 4, make RoomList fasteeeer

RoomList is fast, but sometimes we see some “slowness”. They might not be related to RoomList itself, but I think this issue is a good place to reference them and to explain them, as the root is this new RoomList API.

Here are the list of extra issues we track:

@Hywan Hywan self-assigned this May 11, 2023
@manuroe
Copy link
Contributor

manuroe commented May 11, 2023

This looks really great. Thanks for compiling the tasks. They look good to me.

I have just few remarks or questions:

I am bit scared with the tiny timeline_limit = 1 and that we do not prefetch more events to pre-fill the timeline or better resolve the last message. But, as discussed in a call, we can experiment and update the value.

Should we plan works around caching? Will data from all list be stored?

Update the range maybe

It is mandatory for the visible_rooms list in phase 2. We should be able to prioritise the display of the last message for the room being displayed over fetching data for the all_rooms list in background.

Ensure the room has at least one visible event in its timeline when the room appears in the search result

There will be a new API, an updated version of /messages, to be able to fetch messages for several rooms in a single request. Before it lands, we could call the existing /messages on every displayed room.

An extension to this behavior would be to use /messages on every room that does not have a displayable last message.

@Hywan
Copy link
Member Author

Hywan commented May 11, 2023

The timeline_limit = 1 is from the actual ElementX-iOS code, but I'm happy to change to anything that makes your happy :-). As said, it'll super easy to modify, it won't change anything to the logic of the RoomList in itself.

@Hywan
Copy link
Member Author

Hywan commented May 11, 2023

Should we plan works around caching? Will data from all list be stored?

We must rely on the caching API of SlidingSync itself. Adding another cache layer on top will just increase the surface of potential bugs I reckon. Having a better caching API for SlidingSync was already on our roadmap, even if PR like #1876 already improves it greatly.

@Hywan
Copy link
Member Author

Hywan commented May 11, 2023

There will be a new API, an updated version of /messages, to be able to fetch messages for several rooms in a single request. Before it lands, we could call the existing /messages on every displayed room.

I wonder how it will fit… SlidingSync feeds a Timeline by using… well… sync'ed data. New data are also sync'ed y SS and then dispatched in the SDK to reach the Timeline handler. When calling another HTTP endpoint, I hope it won't create duplicated events. I understand that SS can't pick rooms to sync with, but maybe having all_rooms with a timeline_limit = 1 would be a solution?

@stefanceriu
Copy link
Member

One note on something we just realised: once your session expires we need to restart the initial app SS flow (in which we start with a small range and small limits etc.) to avoid long initial syncs e.g. don't continue from a full all rooms range.
We also need to make sure we don't lose existing room subscriptions when we do that.

@stefanceriu
Copy link
Member

@Hywan Can we please make sure we run this with all the "common extensions"?

@stefanceriu
Copy link
Member

@Hywan Note: we need a replacement for the old SlidingSyncObserver.did_receive_sync_update so we have something to hook up the session verification banner and initial loading indicator to.

@stefanceriu
Copy link
Member

@Hywan Like we talked about today we should introduce custom RoomListService list states: not-loaded, partially loaded (from cache or growing underneath) and loaded

@stefanceriu
Copy link
Member

@Hywan We also talked about removing duplicates (checking move operations i.e delete + insert) and doing transactional diff publishing on both the room list and the timeline

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

No branches or pull requests

3 participants