[Element Call] Keep MatrixClient alive while the call is working #1695
Conversation
jmartinesp
changed the title
jmartinesp
force-pushed
the
fix/jme/element-call-keep-matrix-client-alive
branch
from
9554f6c
to
d223cb5
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1695 +/- ##
===========================================
+ Coverage 63.64% 63.67% +0.02%
===========================================
Files 1267 1267
Lines 32881 32904 +23
Branches 6809 6815 +6
===========================================
+ Hits 20928 20950 +22
+ Misses 8805 8801 -4
- Partials 3148 3153 +5
☔ View full report in Codecov by Sentry. |
| } | ||
| ) | ||
|
|
||
| // We need to restart the Matrix client to get the needed call events | ||
| if (state.isInWidgetMode) { |
There was a problem hiding this comment.
I find it a bit weird to have the view dictate if it should start or stop the sync, could we not manage this entirely in the Presenter?
There was a problem hiding this comment.
We'd need to use the OnLifecycleEvent there instead, which is possible, but then we need to provide a LocalLifecycleOwner in a Molecule test, which is both difficult and kind of weird.
There was a problem hiding this comment.
Or maybe we could just replace it with a DisposableEffect, but that way we don't stop syncing when the call screen goes to background, right?
There was a problem hiding this comment.
Actually a DisposableEffect might be a better solution in the end. I'm replacing it, thanks!
jmartinesp
force-pushed
the
fix/jme/element-call-keep-matrix-client-alive
branch
from
7146f70
to
affaddb
Compare
jmartinesp
added
the
Z-NextRelease
| @Composable | ||
| private fun HandleMatrixClientSyncState() { | ||
| val coroutineScope = rememberCoroutineScope() | ||
| DisposableEffect(Unit) { |
There was a problem hiding this comment.
Do we really need the DisposableEffect? The coroutineScope might be enough
There was a problem hiding this comment.
We could manually stop the sync process when either we hang up the call or we receive a hang up message, but it seems safer to do it automatically when this is disposed.
| appCoroutineScope.launch { | ||
| client.syncService().run { | ||
| if (syncState.value == SyncState.Running) { | ||
| stopSync() |
There was a problem hiding this comment.
Is there no risk of stopping the sync after it's been restarted inside LoggedInFlowNode?
There was a problem hiding this comment.
LoggedInFlowNode already tracks the current state and tries to restart the flow automatically in LoggedInFlowNode.observeSyncStateAndNetworkStatus, similar to what I do here.
There was a problem hiding this comment.
Also, I just tried debugging it and the stopSync is called first, then startSync. I can't guarantee this will always happen, but the subscription in LoggedInFlowNode should help against any race conditions.
There was a problem hiding this comment.
By the way, what happens if the network is lost? (airplane)
There was a problem hiding this comment.
Oh, nice catch. Endless loops of syncService starting.
There was a problem hiding this comment.
I guess this code could be extracted and reusable :
combine(
// small debounce to avoid spamming startSync when the state is changing quickly in case of error.
syncService.syncState.debounce(100),
networkMonitor.connectivity
) { syncState, networkStatus ->
Pair(syncState, networkStatus)
}
.collect { (syncState, networkStatus) ->
Timber.d("Sync state: $syncState, network status: $networkStatus")
if (syncState != SyncState.Running && networkStatus == NetworkStatus.Online) {
syncService.startSync()
}
}
There was a problem hiding this comment.
For now I just copied it, we'll see how we can extract this to some other component or extension function.
Type of change
Content
CallType.RoomCallmode and is in foreground, we get theMatrixClientand start the sync.Motivation and context
Before this fix we were experiencing a bug that consistently made EC work only 50% of the times it was launched. This happened because the EC Widget needs some data from the Rust SDK that can only be retrieved while the sync is active.
Since in
LoggedInFlowNodewe stop the sync when it's sent to the background, these events we requested were never received and the call only actually worked if it had some cached membership events from the previous attempt to make a call, working only 50% of the time, after a previous failed attempt to establish a call.Tests
If they all work, this is working.
Tested devices
Checklist