Split MatrixRoom into MatrixRoom and JoinedMatrixRoom
#4561
Conversation
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4561 +/- ##
===========================================
- Coverage 80.01% 79.99% -0.03%
===========================================
Files 2103 2107 +4
Lines 55786 55811 +25
Branches 6939 6954 +15
===========================================
+ Hits 44638 44644 +6
- Misses 8755 8772 +17
- Partials 2393 2395 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jmartinesp
force-pushed
the
feat/split-matrix-room-into-joined-and-not-joined
branch
from
39bbc07 to
a045768
Compare
| import kotlinx.coroutines.flow.StateFlow | ||
| import java.io.Closeable | ||
| import java.io.File | ||
|
|
||
| interface MatrixRoom : Closeable { |
There was a problem hiding this comment.
Maybe we should take the opportunity to simplify names?
MatrixRoom to BaseRoomor simply Room
JoinedMatrixRoom to JoinedRoom
| maxSize = CACHE_SIZE, | ||
| onEntryRemoved = { evicted, roomId, oldRoom, _ -> | ||
| Timber.d("On room removed from cache: $roomId, evicted: $evicted") | ||
| oldRoom.roomListItem.close() |
There was a problem hiding this comment.
The room cache should not be needed if we have the persistent event cache in the app, and it can even be problematic to have it around since clearing the cache for a room needs the room to not have a live timeline instantiated, otherwise the cache won't be cleared, and if we keep references to the Rust Room we also keep references to its timeline, I think.
There was a problem hiding this comment.
So lets remove completely the cache?
There was a problem hiding this comment.
I think I did that, did I miss anything 🤔 ?
There was a problem hiding this comment.
Indeed, I think I got tricked by github ui
| * Forget the room if we had access to it, and it was left or banned. | ||
| */ | ||
| suspend fun forget(): Result<Unit> | ||
| interface NotJoinedRoom : AutoCloseable { |
There was a problem hiding this comment.
I'm not sure I like this NotJoinedRoom because it's a bit confusing how the api looks like in comparison with JoinedRoom
Maybe just being able to fetch RoomPreviewInfo and when needed a BaseRoom using getRoom would be enough?
There was a problem hiding this comment.
At the SDK level we were thinking about making the getRoomPreview methods return a struct (or a data class in Kotlin) with both the preview info and the optional Room instance, so I made these changes trying to follow that suggestion. Maybe we can discuss this with the Rust team if the API is not what we want.
There was a problem hiding this comment.
Ok, this just feel a bit strange to use, but why not...
| @@ -52,8 +53,8 @@ interface MatrixClient { | |||
| val mediaLoader: MatrixMediaLoader | |||
| val sessionCoroutineScope: CoroutineScope | |||
| val ignoredUsersFlow: StateFlow<ImmutableList<UserId>> | |||
| suspend fun getJoinedRoom(roomId: RoomId): JoinedMatrixRoom? | |||
There was a problem hiding this comment.
Maybe I'd prefer a method on the BaseRoom to initialise a JoinedRoom from it
class BaseRoom {
...
suspend fun asJoined(): JoinedMatrixRoom()?
}
There was a problem hiding this comment.
We actually need the RoomListItem to instantiate the live timeline, so it may not be that easy 🫤 . We could keep a reference to the item in the base room, maybe, or to the RoomListService.
This new interface is the one which has guaranteed access to a live timeline, while the `MatrixRoom` one has access to a subset of features rooms both in joined and not joined can access.
This interface now has some `RoomPreviewInfo` and optional access to the `MatrixRoom` if it's a local room we can get from the SDK.
This means creating `getBaseRoom` and `getJoinedRoomOrPreview` methods to get either a `MatrixRoom` or either a `JoinedMatrixRoom` or `NotJoinedRoom` from the SDK. The previously used room cache has been removed, as it's no longer needed with the persistent event cache.
- JoinedRoom - NotJoinedRoom - BaseRoom - RoomInfo - RoomMembersState - RoomInfoMapper - RoomPowerLevels etc.
…Factory` once per method call.
jmartinesp
force-pushed
the
feat/split-matrix-room-into-joined-and-not-joined
branch
from
a045768 to
b59a4f2
Compare
|
jmartinesp
deleted the
feat/split-matrix-room-into-joined-and-not-joined
branch
Content
Motivation and context
Since some time ago, the joined rooms in the SDK don't need a live timeline anymore. This was previously a precondition and we had to get a
RoomfromRoomListItem.fullRoom(), having initialised the live timeline before.Now we can instead just call
Client.getRoom(roomId)to get a room, joined or not, if it exists locally in the SDK's DB, and the timeline doesn't have to be initialised at that point, so we can initialise the timeline if needed lazily. In a previous PR I tried doing exactly this, but initialising the timeline can fail so having a joined room with no timeline in some UI contexts could be dangerous and would need new error UI states or retries.So in the end, the solution was to have the
RustRoomFactoryreturn both rooms with and without timeline, depending on what's asked from it, and make the joined room having a timeline mandatory for those UI contexts, but not for the rest of the app.Tests
It's difficult to say what we should test, but I guess it would be joining and leaving rooms, checking the preview of rooms known and unknown and of course anything inside the chat screen or a child node of it should still work as before. So to sum up, everything should still work as it used to, and rooms should still be fast to load.
Tested devices
Checklist