Deferred DMs - Handle the local rooms within the new AppLayout #7154
Conversation
Florian14
requested review from
a team and
onurays
and removed request for
a team
Florian14
force-pushed
the
bugfix/fre/start_dm_app_layout
branch
from
352b872 to
babe178
Compare
Florian14
added
PR-Small
|
|
||
| roomListViewModel.onEach(HomeRoomListViewState::localRoomIds) { | ||
| // Local rooms should not exist anymore when the room list is shown | ||
| roomListViewModel.deleteLocalRooms(it) |
There was a problem hiding this comment.
I think HomeRoomListViewModel should decide this business logic.
There was a problem hiding this comment.
My problem is that the VM is still alive while the local room is opened, so moving this logic to the VM means that the local room will be deleted too early. I trigger it from the fragment to delete the rooms only when the room list is visible to the user (ie. the local timeline is closed).
It would be nice to have a foreground/background state to decide when a ViewModel should be "alive".
Do you have any suggestions about it?
There was a problem hiding this comment.
You could add an Action to the ViewModel, triggered in the onResume of the Fragment, which delete all the local rooms. So HomeRoomListViewState.localRoomIds could even be removed I think.
Also we do not add public API to the ViewModels, please call handle() and add an action.
So we will have in the Fragment:
override fun onResume() {
super.onResume()
roomListViewModel.handle(HomeRoomListAction.DeleteAllLocalRoom)
}
There was a problem hiding this comment.
Done 8999b40 I moved to onStart instead of onResume
| @@ -41,6 +42,8 @@ internal class DefaultUpdateBreadcrumbsTask @Inject constructor( | |||
| ) : UpdateBreadcrumbsTask { | |||
|
|
|||
| override suspend fun execute(params: UpdateBreadcrumbsTask.Params) { | |||
| // Do not add local rooms to the recent rooms list as they should not be known by the server | |||
| if (RoomLocalEcho.isLocalEchoId(params.newTopRoomId)) return | |||
There was a problem hiding this comment.
Good point, but maybe this could be filtered before, at https://github.com/vector-im/element-android/blob/c73c36eb26149390167d23944b9fa3264cbc10f4/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/DefaultRoomService.kt#L175, WDYT?
There was a problem hiding this comment.
Done 648498e
I was hesitating between both places, I choose the task to be sure it would never be sent to the server but the task is only called by the service until now
|
|
||
| roomListViewModel.onEach(HomeRoomListViewState::localRoomIds) { | ||
| // Local rooms should not exist anymore when the room list is shown | ||
| roomListViewModel.deleteLocalRooms(it) |
There was a problem hiding this comment.
You could add an Action to the ViewModel, triggered in the onResume of the Fragment, which delete all the local rooms. So HomeRoomListViewState.localRoomIds could even be removed I think.
Also we do not add public API to the ViewModels, please call handle() and add an action.
So we will have in the Fragment:
override fun onResume() {
super.onResume()
roomListViewModel.handle(HomeRoomListAction.DeleteAllLocalRoom)
}
14ab616 to
5e50494
Compare
|
I also added this commit to remove the read receipts from the DB (they're not sent to the server as they are only attached to local events but they were persisted in the database) |
|
|
||
| viewModelScope.launch { | ||
| localRoomIds.forEach { | ||
| session.roomService().deleteLocalRoom(it) |
There was a problem hiding this comment.
Not for this PR, but having a method RoomService.deleteAllLocalRooms() would be handy.
|
SonarCloud Quality Gate failed. |
Type of change
Content
This PR adds the deletion of the local rooms from the database when the room list is shown within the new AppLayout feature enabled.
Motivation and context
Close #7153
Tests
We can also observe logs in the console about the deletion of the local rooms when the home room list is shown again.
Tested devices
Checklist