Let the SDK provide the "network is available information" #4215
Conversation
| SyncState.Idle, | ||
| SyncState.Running, | ||
| SyncState.Error, | ||
| SyncState.Terminated -> NetworkStatus.Online | ||
| SyncState.Offline -> NetworkStatus.Offline |
There was a problem hiding this comment.
Shouldn't only Running be Online?
There was a problem hiding this comment.
Since NetworkStatus.Offline will make the "no network" banner to be shown, I think it's better to be optimistic here.
| } else { | ||
| NetworkStatus.Offline | ||
| } | ||
| .stateIn( |
There was a problem hiding this comment.
I think when we discussed at the time, that the network monitor was still supposed to detect network changes, so we can re-trigger a sync as soon as possible (to avoid waiting for the auto retry timeout in those cases).
Also, I think we can remove the StateFlow from here and just expose an extension method on the SyncState like isConnected()
…atically when the network is back. Also rely on the sync state to render the "Offline" banner.
bmarty
added
the
Z-NextRelease
bmarty
force-pushed
the
feature/bma/airGappedSdk
branch
from
024a9e3 to
f84aa03
Compare
|
@ganfra @jmartinesp I have reworked the PR, it's working fine on my side. Let me know what you think, then once merged we can do the release. |
|
📱 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 #4215 +/- ##
===========================================
- Coverage 80.22% 80.21% -0.01%
===========================================
Files 2042 2043 +1
Lines 54090 54095 +5
Branches 6570 6569 -1
===========================================
Hits 43393 43393
- Misses 8424 8429 +5
Partials 2273 2273 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
One remark regarding SyncState/SyncService.
Also, I think we need to change the observeSyncStateAndNetworkStatus in LoggedInFlowNode : we don't want to listen to the sync state now, we can just listen to the network monitor state.
Note : it'll be changed anyway by Jorge PR
| @@ -14,3 +14,11 @@ enum class SyncState { | |||
| Terminated, | |||
| Offline, | |||
| } | |||
|
|
|||
| fun SyncState.isConnected() = when (this) { | |||
There was a problem hiding this comment.
I know the name was from me initially, but maybe we should instead call it isOffline?
fun SyncState.isOffline() = this is SyncState.Offline.
Or even an extension method on the SyncService
fun SyncService.isOffline(): StateFlow<Boolean> = syncState.mapState { it == SyncState.Offline }
There was a problem hiding this comment.
Can we do the opposite
fun SyncService.isOnline(): StateFlow<Boolean> = syncState.mapState { it != SyncState.Offline }
it will be simpler to use.
|
Content
We may have some trouble with the current network detection, since it rely on the ability to reach random server (not to name Google's one) and this can be broken when the application is run in air-gapped environment.
This PR move the network detection to the SDK providing the ability to perform sync request or not.
Need matrix-org/matrix-rust-sdk#4592, which is not merged yet.
Motivation and context
Improve network detection and bind it to the ability to communicate with the homeserver.
This will fix the "no network" banner which was displayed when the application was run in air-gapped mode.
Screenshots / GIFs
Tests
Tested devices
Checklist