Feature/fga/sync states (#1042)

* Change RoomSummaryDataSource to RoomListService to better reflects the rust api * Better Sync management * Sync: improve sync spinner rendering * Sync: make test compiles * Sync: add more test for sync spinner * Sync: more clean-up * Sync: pr review --------- Co-authored-by: ganfra <francoisg@element.io>
2023-08-09 14:37:43 +02:00
parent 4e94d4da6b
commit 226a3dbf28
44 changed files with 547 additions and 356 deletions
--- a/appnav/build.gradle.kts
+++ b/appnav/build.gradle.kts
@@ -65,6 +65,8 @@ dependencies {
    testImplementation(libs.test.truth)
    testImplementation(libs.test.turbine)
    testImplementation(projects.libraries.matrix.test)
+    testImplementation(projects.features.networkmonitor.test)
+    testImplementation(projects.tests.testutils)
    testImplementation(projects.features.rageshake.test)
    testImplementation(projects.features.rageshake.impl)
    testImplementation(projects.services.appnavstate.test)
--- a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt
+++ b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt
@@ -69,10 +69,13 @@ import io.element.android.libraries.matrix.ui.di.MatrixUIBindings
 import io.element.android.libraries.push.api.notifications.NotificationDrawerManager
 import io.element.android.services.appnavstate.api.AppNavigationStateService
 import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.FlowPreview
 import kotlinx.coroutines.flow.combine
+import kotlinx.coroutines.flow.debounce
 import kotlinx.coroutines.flow.distinctUntilChanged
 import kotlinx.coroutines.launch
 import kotlinx.parcelize.Parcelize
+import timber.log.Timber

@ContributesNode(AppScope::class)
 class LoggedInFlowNode @AssistedInject constructor(
@@ -123,7 +126,6 @@ class LoggedInFlowNode @AssistedInject constructor(

    override fun onBuilt() {
        super.onBuilt()
-
        lifecycle.subscribe(
            onCreate = {
                plugins<LifecycleCallback>().forEach { it.onFlowCreated(id, inputs.matrixClient) }
@@ -138,12 +140,8 @@ class LoggedInFlowNode @AssistedInject constructor(
                    backstack.push(NavTarget.Ftue)
                }
            },
-            onStart = {
-                lifecycleScope.launch {
-                    syncService.startSync()
-                }
-            },
            onStop = {
+                //Counterpart startSync is done in observeSyncStateAndNetworkStatus method.
                syncService.stopSync()
            },
            onDestroy = {
@@ -153,22 +151,24 @@ class LoggedInFlowNode @AssistedInject constructor(
                loggedInFlowProcessor.stopObserving()
            }
        )
-
        observeSyncStateAndNetworkStatus()
    }

+    @OptIn(FlowPreview::class)
    private fun observeSyncStateAndNetworkStatus() {
        lifecycleScope.launch {
-            repeatOnLifecycle(Lifecycle.State.RESUMED) {
+            repeatOnLifecycle(Lifecycle.State.STARTED) {
                combine(
-                    syncService.syncState,
+                    // small debounce to avoid spamming startSync when the state is changing quickly in case of error.
+                    syncService.syncState.debounce(100),
                    networkMonitor.connectivity
                ) { syncState, networkStatus ->
-                    syncState == SyncState.Error && networkStatus == NetworkStatus.Online
+                    Pair(syncState, networkStatus)
                }
                    .distinctUntilChanged()
-                    .collect { restartSync ->
-                        if (restartSync) {
+                    .collect { (syncState, networkStatus) ->
+                        Timber.d("Sync state: $syncState, network status: $networkStatus")
+                        if (syncState != SyncState.Running && networkStatus == NetworkStatus.Online) {
                            syncService.startSync()
                        }
                    }
@@ -351,3 +351,4 @@ class LoggedInFlowNode @AssistedInject constructor(
        backstack.push(NavTarget.InviteList)
    }
 }
+
--- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt
+++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt
@@ -21,16 +21,27 @@ import android.os.Build
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.runtime.collectAsState
+import androidx.compose.runtime.getValue
+import androidx.compose.runtime.mutableStateOf
+import androidx.compose.runtime.remember
+import androidx.compose.runtime.setValue
+import io.element.android.features.networkmonitor.api.NetworkMonitor
+import io.element.android.features.networkmonitor.api.NetworkStatus
 import io.element.android.libraries.architecture.Presenter
 import io.element.android.libraries.matrix.api.MatrixClient
+import io.element.android.libraries.matrix.api.roomlist.RoomListService
 import io.element.android.libraries.permissions.api.PermissionsPresenter
 import io.element.android.libraries.permissions.noop.NoopPermissionsPresenter
 import io.element.android.libraries.push.api.PushService
+import kotlinx.coroutines.delay
 import javax.inject.Inject

+private const val DELAY_BEFORE_SHOWING_SYNC_SPINNER_IN_MILLIS = 1500L
+
 class LoggedInPresenter @Inject constructor(
    private val matrixClient: MatrixClient,
    private val permissionsPresenterFactory: PermissionsPresenter.Factory,
+    private val networkMonitor: NetworkMonitor,
    private val pushService: PushService,
 ) : Presenter<LoggedInState> {

@@ -53,18 +64,25 @@ class LoggedInPresenter @Inject constructor(
            pushService.registerWith(matrixClient, pushProvider, distributor)
        }

-        val syncState = matrixClient.syncService().syncState.collectAsState()
+        val roomListState by matrixClient.roomListService.state.collectAsState()
+        val networkStatus by networkMonitor.connectivity.collectAsState()
        val permissionsState = postNotificationPermissionsPresenter.present()
-
-        // fun handleEvents(event: LoggedInEvents) {
-        //     when (event) {
-        //     }
-        // }
-
+        var showSyncSpinner by remember {
+            mutableStateOf(false)
+        }
+        LaunchedEffect(roomListState, networkStatus) {
+            showSyncSpinner = when {
+                networkStatus == NetworkStatus.Offline -> false
+                roomListState == RoomListService.State.Running -> false
+                else -> {
+                    delay(DELAY_BEFORE_SHOWING_SYNC_SPINNER_IN_MILLIS)
+                    true
+                }
+            }
+        }
        return LoggedInState(
-            syncState = syncState.value,
+            showSyncSpinner = showSyncSpinner,
            permissionsState = permissionsState,
-            // eventSink = ::handleEvents
        )
    }
 }
--- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInState.kt
+++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInState.kt
@@ -16,11 +16,9 @@

 package io.element.android.appnav.loggedin

-import io.element.android.libraries.matrix.api.sync.SyncState
 import io.element.android.libraries.permissions.api.PermissionsState

 data class LoggedInState(
-    val syncState: SyncState,
+    val showSyncSpinner: Boolean,
    val permissionsState: PermissionsState,
-    // val eventSink: (LoggedInEvents) -> Unit
 )
--- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt
+++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt
@@ -17,22 +17,20 @@
 package io.element.android.appnav.loggedin

 import androidx.compose.ui.tooling.preview.PreviewParameterProvider
-import io.element.android.libraries.matrix.api.sync.SyncState
 import io.element.android.libraries.permissions.api.createDummyPostNotificationPermissionsState

 open class LoggedInStateProvider : PreviewParameterProvider<LoggedInState> {
    override val values: Sequence<LoggedInState>
        get() = sequenceOf(
-            aLoggedInState(),
-            aLoggedInState(syncState = SyncState.Idle),
+            aLoggedInState(false),
+            aLoggedInState(true),
            // Add other state here
        )
 }

 fun aLoggedInState(
-    syncState: SyncState = SyncState.Running,
+    showSyncSpinner: Boolean = true,
 ) = LoggedInState(
-    syncState = syncState,
+    showSyncSpinner = showSyncSpinner,
    permissionsState = createDummyPostNotificationPermissionsState(),
-    // eventSink = {}
 )
--- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt
+++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt
@@ -47,7 +47,7 @@ fun LoggedInView(
            modifier = Modifier
                .padding(top = 8.dp)
                .align(Alignment.TopCenter),
-            syncState = state.syncState,
+            isVisible = state.showSyncSpinner,
        )
        PermissionsView(
            state = state.permissionsState,
--- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/SyncStateView.kt
+++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/SyncStateView.kt
@@ -38,19 +38,18 @@ import io.element.android.libraries.designsystem.preview.ElementPreview
 import io.element.android.libraries.designsystem.theme.components.CircularProgressIndicator
 import io.element.android.libraries.designsystem.theme.components.Surface
 import io.element.android.libraries.designsystem.theme.components.Text
-import io.element.android.libraries.matrix.api.sync.SyncState
 import io.element.android.libraries.theme.ElementTheme
 import io.element.android.libraries.ui.strings.CommonStrings

@Composable
 fun SyncStateView(
-    syncState: SyncState,
+    isVisible: Boolean,
    modifier: Modifier = Modifier
 ) {
    val animationSpec = spring<Float>(stiffness = 500F)
    AnimatedVisibility(
        modifier = modifier,
-        visible = syncState.mustBeVisible(),
+        visible = isVisible,
        enter = fadeIn(animationSpec = animationSpec),
        exit = fadeOut(animationSpec = animationSpec),
    ) {
@@ -60,15 +59,15 @@ fun SyncStateView(
        ) {
            Row(
                modifier = Modifier
-                    .background(color = ElementTheme.colors.bgSubtleSecondary)
-                    .padding(horizontal = 24.dp, vertical = 10.dp),
+                        .background(color = ElementTheme.colors.bgSubtleSecondary)
+                        .padding(horizontal = 24.dp, vertical = 10.dp),
                verticalAlignment = Alignment.CenterVertically,
                horizontalArrangement = Arrangement.spacedBy(10.dp)
            ) {
                CircularProgressIndicator(
                    modifier = Modifier
-                        .progressSemantics()
-                        .size(12.dp),
+                            .progressSemantics()
+                            .size(12.dp),
                    color = ElementTheme.colors.textPrimary,
                    strokeWidth = 1.5.dp,
                )
@@ -82,20 +81,13 @@ fun SyncStateView(
    }
 }

-private fun SyncState.mustBeVisible() = when (this) {
-    SyncState.Idle -> true /* Cold start of the app */
-    SyncState.Running -> false
-    SyncState.Error -> false /* In this case, the network error banner can be displayed */
-    SyncState.Terminated -> true /* The app is resumed and the sync is started again */
-}
-
@DayNightPreviews
@Composable
 internal fun SyncStateViewPreview() = ElementPreview {
    // Add a box to see the shadow
    Box(modifier = Modifier.padding(24.dp)) {
        SyncStateView(
-            syncState = SyncState.Idle
+            isVisible = true
        )
    }
 }
--- a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt
+++ b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt
@@ -20,13 +20,18 @@ import app.cash.molecule.RecompositionMode
 import app.cash.molecule.moleculeFlow
 import app.cash.turbine.test
 import com.google.common.truth.Truth.assertThat
+import io.element.android.features.networkmonitor.api.NetworkStatus
+import io.element.android.features.networkmonitor.test.FakeNetworkMonitor
 import io.element.android.libraries.matrix.api.MatrixClient
+import io.element.android.libraries.matrix.api.roomlist.RoomListService
 import io.element.android.libraries.matrix.test.FakeMatrixClient
+import io.element.android.libraries.matrix.test.roomlist.FakeRoomListService
 import io.element.android.libraries.permissions.api.PermissionsPresenter
 import io.element.android.libraries.permissions.noop.NoopPermissionsPresenter
 import io.element.android.libraries.push.api.PushService
 import io.element.android.libraries.pushproviders.api.Distributor
 import io.element.android.libraries.pushproviders.api.PushProvider
+import io.element.android.tests.testutils.consumeItemsUntilPredicate
 import kotlinx.coroutines.test.runTest
 import org.junit.Test

@@ -42,14 +47,33 @@ class LoggedInPresenterTest {
        }
    }

-    private fun createPresenter(): LoggedInPresenter {
+    @Test
+    fun `present - show sync spinner`() = runTest {
+        val roomListService = FakeRoomListService()
+        val presenter = createPresenter(roomListService, NetworkStatus.Online)
+        moleculeFlow(RecompositionMode.Immediate) {
+            presenter.present()
+        }.test {
+            val initialState = awaitItem()
+            assertThat(initialState.showSyncSpinner).isFalse()
+            consumeItemsUntilPredicate { it.showSyncSpinner }
+            roomListService.postState(RoomListService.State.Running)
+            consumeItemsUntilPredicate { !it.showSyncSpinner }
+        }
+    }
+
+    private fun createPresenter(
+        roomListService: RoomListService = FakeRoomListService(),
+        networkStatus: NetworkStatus = NetworkStatus.Offline
+    ): LoggedInPresenter {
        return LoggedInPresenter(
-            matrixClient = FakeMatrixClient(),
+            matrixClient = FakeMatrixClient(roomListService = roomListService),
            permissionsPresenterFactory = object : PermissionsPresenter.Factory {
                override fun create(permission: String): PermissionsPresenter {
                    return NoopPermissionsPresenter()
                }
            },
+            networkMonitor = FakeNetworkMonitor(networkStatus),
            pushService = object : PushService {
                override fun notificationStyleChanged() {
                }
--- a/appnav/src/test/kotlin/io/element/android/appnav/room/LoadingRoomStateFlowFactoryTest.kt
+++ b/appnav/src/test/kotlin/io/element/android/appnav/room/LoadingRoomStateFlowFactoryTest.kt
@@ -18,12 +18,12 @@ package io.element.android.appnav.room

 import app.cash.turbine.test
 import com.google.common.truth.Truth
-import io.element.android.libraries.matrix.api.room.RoomSummaryDataSource
+import io.element.android.libraries.matrix.api.roomlist.RoomList
 import io.element.android.libraries.matrix.test.A_ROOM_ID
 import io.element.android.libraries.matrix.test.A_SESSION_ID
 import io.element.android.libraries.matrix.test.FakeMatrixClient
 import io.element.android.libraries.matrix.test.room.FakeMatrixRoom
-import io.element.android.libraries.matrix.test.room.FakeRoomSummaryDataSource
+import io.element.android.libraries.matrix.test.roomlist.FakeRoomListService
 import kotlinx.coroutines.test.runTest
 import org.junit.Test

@@ -47,29 +47,29 @@ class LoadingRoomStateFlowFactoryTest {
    @Test
    fun `flow should emit Loading and then Loaded when there is a room in cache after SS is loaded`() = runTest {
        val room = FakeMatrixRoom(sessionId= A_SESSION_ID, roomId = A_ROOM_ID)
-        val roomSummaryDataSource = FakeRoomSummaryDataSource()
-        val matrixClient = FakeMatrixClient(A_SESSION_ID, roomSummaryDataSource = roomSummaryDataSource)
+        val roomListService = FakeRoomListService()
+        val matrixClient = FakeMatrixClient(A_SESSION_ID, roomListService = roomListService)
        val flowFactory = LoadingRoomStateFlowFactory(matrixClient)
        flowFactory
            .create(this, A_ROOM_ID)
            .test {
                Truth.assertThat(awaitItem()).isEqualTo(LoadingRoomState.Loading)
                matrixClient.givenGetRoomResult(A_ROOM_ID, room)
-                roomSummaryDataSource.postLoadingState(RoomSummaryDataSource.LoadingState.Loaded(1))
+                roomListService.postAllRoomsLoadingState(RoomList.LoadingState.Loaded(1))
                Truth.assertThat(awaitItem()).isEqualTo(LoadingRoomState.Loaded(room))
            }
    }

    @Test
    fun `flow should emit Loading and then Error when there is no room in cache after SS is loaded`() = runTest {
-        val roomSummaryDataSource = FakeRoomSummaryDataSource()
-        val matrixClient = FakeMatrixClient(A_SESSION_ID, roomSummaryDataSource = roomSummaryDataSource)
+        val roomListService = FakeRoomListService()
+        val matrixClient = FakeMatrixClient(A_SESSION_ID, roomListService = roomListService)
        val flowFactory = LoadingRoomStateFlowFactory(matrixClient)
        flowFactory
            .create(this, A_ROOM_ID)
            .test {
                Truth.assertThat(awaitItem()).isEqualTo(LoadingRoomState.Loading)
-                roomSummaryDataSource.postLoadingState(RoomSummaryDataSource.LoadingState.Loaded(1))
+                roomListService.postAllRoomsLoadingState(RoomList.LoadingState.Loaded(1))
                Truth.assertThat(awaitItem()).isEqualTo(LoadingRoomState.Error)
            }
    }