feat: add add_cached_list to SlidingSyncBuilder and SlidingSync

[bnjbvr]

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.

Also took the opportunity to rename pop_list to remove_list, as there's no stack/vec involved thus popping sounded a bit weird to me.

Fixes #1737.

[bnjbvr]

cc @stefanceriu, in case you want to test it, have feedback on the API, etc. :)

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (c404e37) 76.27% compared to head (fd3c456) 76.27%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1876   +/-   ##
=======================================
  Coverage   76.27%   76.27%           
=======================================
  Files         141      141           
  Lines       16089    16089           
=======================================
  Hits        12272    12272           
  Misses       3817     3817           

Impacted Files	Coverage Δ
crates/matrix-sdk/src/sliding_sync/list/builder.rs	0.00% <ø> (ø)
crates/matrix-sdk/src/sliding_sync/list/mod.rs	0.00% <ø> (ø)
crates/matrix-sdk/src/sliding_sync/mod.rs	0.00% <ø> (ø)

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

[Hywan]

Also took the opportunity to rename pop_list to remove_list, as there's no stack/vec involved thus popping sounded a bit weird to me.

pop_list is being removed in #1758 :-).

[Hywan]

General comment: We should merge this PR after #1758, as it could create conflicts in both case. #1758 changes the design of the builders quite a bit.

[Hywan]

Looks good to me so far. The design is nice and is definitely an improvement. Well done!

As I said, I would like to way on #1758 to be merged before merging this one as the behavior of add_list changes.

[Hywan]

The only way to set those values are when it's reloaded from the cache. The doc' should emphasize that :-). It's not “may be reloaded from the cache” but rather “contains the default value, or the value from the cache, cannot be modified by the user”.

Question: Did we read those values from the cache previously?

[bnjbvr]

Yes, we did! That was the data that was merged in set_from_cold, using observables (while in theory we should only have to use plain assignments, assuming reading from cache only happens at initialization).

[bnjbvr]

And good point; I've actually tweaked the data stored here, by introducing a new temporary data structure that contains only the cached data. That way, we can decide in build() if we read the cached data or the default, and that puts the code assigning the defaults closer to the data read from the cache.

[stefanceriu]

With #1758 now merged, should we rebase this and give it a shot?

[bnjbvr]

Yep, this one is next on my rebase list; I can let you Stefan know once this is ready?

[bnjbvr]

For what it's worth, modulo the doc issue, this is ready for review and testing.

[Hywan]

Will review it.

[Hywan]

Looks good to me!
@stefanceriu, can you test it before merging it please? Our tests are passing, but it's better testing than fixing.

[stefanceriu]

Okay so I gave it a shot but I'm a bit confused by how this is supposed to work.

If I do this: I get a A caching request was made but a storage key is missing in sliding sync error

            let slidingSync = try slidingSyncBuilder
                .addCachedList(v: visibleRoomsListBuilder)
//                .addList(listBuilder: visibleRoomsListBuilder)
//                .storageKey(name: "ElementX")

and if I do this: I get the same error

let slidingSync = try slidingSyncBuilder
                .addCachedList(v: visibleRoomsListBuilder)
//                .addList(listBuilder: visibleRoomsListBuilder)
                .storageKey(name: "ElementX")

If I do this instead I don't get an error anymore but the list doesn't publish its total count anymore either. It's always zero even though it has rooms in it

let slidingSync = try slidingSyncBuilder
                .storageKey(name: "ElementX")
                .addCachedList(v: visibleRoomsListBuilder)

I think we should get rid of the storage key entirely and only rely on the room name for the cache

L.E. Interestingly enough the other 2 lists I add later do publish their room count

2023-05-12T11:39:35.639990Z  INFO root: elementx: SlidingSyncListProxy.swift:54: AllRooms: Updated room count: 208
2023-05-12T11:39:35.640463Z  INFO root: elementx: SlidingSyncListProxy.swift:54: Invites: Updated room count: 0

[bnjbvr]

Thanks for testing!

Indeed we should probably tweak the builders so the storage key has to be provided first, or the other method is not available until then (or have a sub-builder for caching that takes a storage key on creation). Right now the key for each list is composed of both the global storage key provided, as well as a suffix generated from the list's name. I assume that is so, in case the Rust SDK would be used in multiple apps (but in that case they would likely use different storage locations for the caches), or the same SDK used multiple times in the same app (less likely). Should we get rid of the storage key @Hywan?

Regarding the other issue, I'll need to investigate a bit more.

[bnjbvr]

@stefanceriu confirmed (thanks!) that the latest commit fixes the issue about the initial value being not observed. I've added a test in #1922 that shows this behavior, and I can confirm it passes fine in this branch now (and didn't before the fix).

[bnjbvr]

Regarding the possibility to remove the storage_key: I've read in another issue that the clients could support multiple accounts at some point, and that support for that in the Rust SDK would be as easy as creating another SDK client with different storage keys. So I think the storage key has to stay for that reason; I'll work on a more ergonomic API in a followup PR.

[stefanceriu]

the Rust SDK would be as easy as creating another SDK client with different storage keys

So perhaps we should use the user's id as a key automagically? userId+roomName should be more than enough, no?

[poljar]

Regarding the possibility to remove the storage_key: I've read in another issue that the clients could support multiple accounts at some point, and that support for that in the Rust SDK would be as easy as creating another SDK client with different storage keys. So I think the storage key has to stay for that reason; I'll work on a more ergonomic API in a followup PR.

I don't think the SQLite store supports multiple users, you would need to use a completely different store path.

[stefanceriu]

you would need to use a completely different store path

ah good point, the sql stores are already stored in different folders based on the user id

[Hywan]

LGTM :-)