Changing the sync-mode of a list on-the-fly is necessary to optimise the
new `RoomList` API. For example, we can start with a list in a selective
mode, a range of `0..=50` and a `timeline_limit=0` to fetch the
beginning of the room list, and then _change_ the sync-mode to growing
to continue to sync the room list in the background.
Today, this is done with 2 lists and a merge of the lists, but
(i) this is error-prone, (ii) this is not optimal. Thank to
`SlidingSyncList::set_sync_mode`, merging lists is no more necessary,
thus removing a class of bugs in client's code.
`SlidingSyncMode` was previously declared as:
```rust
enum SlidingSyncMode {
Selective,
Paging,
Growing,
}
```
Now, the new declaration is:
```rust
enum SlidingSyncMode {
Selective,
Paging {
batch_size: u32,
maximum_number_of_rooms_to_fetch: Option<u32>,
},
Growing {
batch_size: u32,
maximum_number_of_rooms_to_fetch: Option<u32>,
}
}
```
First off, it helps to remove the `full_sync_batch_size` and
`full_sync_maximum_number_of_rooms_to_fetch` methods and fields from
`SlidingSyncListBuilder`. It was containing default values in case of.
That was useless and needed to be fixed. Also, calling a `full_sync_*`
method with the `Selective` mode had no effect, which could disturb
the user. Well, now everything is clean on that front.
Second, `SlidingSyncListRequestGenerator` no longer has constructors,
but a `From<SlidingSyncMode>` implementation. However, the constructors
now live in `SlidingSyncMode` with `new_selective`, `new_paging` and
`new_growing` methods which are helpers. All in all, it makes creating a
`SlidingSyncListRequestGeneator` simpler: just call `sync_mode.into()`
and boom.
Finally, this patch removes the `default_with_fullsync` list builder
helper, which makes no sense at all.
This bug has been found by @bnjbvr, all the credits go to him. I've just
added some comments around his code.
Prior to this patch, the room unsubscription buffer
(`SlidingSync::room_unsubscriptions`) was reset before the request was
sent. So if something went wrong, the next request would not include the
room unsubscriptions.
This patch updates this behavior. First, it replaces `Vec` by `HashSet`
to avoid a O(n^2) look up.
Second, a copy of room unsubscriptions used by the request is kept, so
that it can be used to cherry-pick which room unsubscription to remove
from the buffer once a response from the server is received. It's
important to not clear the entire room unsubscriptions buffer as more
unsubscriptions could have been inserted meanwhile.
This has slightly drifted from the initial design I thought about in the issue.
Instead of having `build()` be fallible and mutably borrow some substate (namely `BTreeMap<OwnedRoomId, SlidingSyncRoom>`) from `SlidingSync` (that may or may not already exist), I've introduced a new `add_cached_list` method on `SlidingSync` and `SlidingSyncBuilder`. This method hides all the underlying machinery, and injects the room data read from the list cache into the sliding sync room map.
In particular, with these changes:
- any list added with `add_list` **won't** be loaded from/written to the cache storage,
- any list added with `add_cached_list` will be cached, and an attempt to reload it from the cache will be done during this call (hence `async` + `Result`).
- `SlidingSyncBuilder::build()` now only fetches the `SlidingSync` data from the cache (assuming the storage key has been defined), not that of the lists anymore.
Fixes#1737.
Signed-off-by: Benjamin Bouvier <public@benj.me>
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
We were using `txn_id` as a way to identify the running stream. Now
things are cleaner so we can remove this “debug tool”. This class of
problems should not appear anymore. For the record, the biggest problem
was the following: It was possible to start multiple `stream`s at the
same time, and thus a stream could receive a response sent by another
stream. Since we no longer need to restart SlidingSync anymore (i.e. no
need to start multiple `stream`s at the same time), this problem should
not happen.
Prior to this patch, the `v4::Response`, aka the SlidingSync response,
was processed by the client and transformed into a `SyncResponse` into
`sync_once` before being passed to `handle_response`. Now it's done in
`handle_response`.
This is not mandatory _now_, but it will be helpful in a close future.
