From 85f7e5e1e0a50c34c01002bb931a646dd6bdf71e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Fri, 17 Jan 2025 14:20:17 +0100 Subject: [PATCH 01/15] Create `SyncOrchestrator` to centralise the sync start/stop flow through the whole app. The decision is based on several inputs: sync state, network available, app in foreground, app in call, app needing to sync an event for a notification. --- .../android/appnav/LoggedInFlowNode.kt | 48 +-- .../io/element/android/appnav/RootFlowNode.kt | 14 +- .../appnav/di/DefaultSyncOrchestrator.kt | 148 ++++++++ ...ClientsHolder.kt => MatrixSessionCache.kt} | 42 ++- .../appnav/root/RootNavStateFlowFactory.kt | 6 +- .../appnav/DefaultSyncOrchestratorTest.kt | 333 ++++++++++++++++++ .../appnav/di/MatrixClientsHolderTest.kt | 97 ----- .../appnav/di/MatrixSessionCacheTest.kt | 132 +++++++ .../analytics/impl/AnalyticsOptInNode.kt | 4 +- features/call/impl/build.gradle.kts | 2 + .../call/impl/ui/CallScreenPresenter.kt | 15 +- .../call/ui/CallScreenPresenterTest.kt | 30 +- .../impl/root/CreateRoomRootNode.kt | 4 +- .../impl/DefaultLockScreenService.kt | 2 +- .../lockscreen/impl/unlock/PinUnlockNode.kt | 5 +- .../features/login/impl/LoginFlowNode.kt | 4 +- .../createaccount/CreateAccountNode.kt | 4 +- .../features/logout/impl/LogoutNode.kt | 5 +- .../features/messages/impl/MessagesNode.kt | 4 +- .../pinned/PinnedEventsTimelineProvider.kt | 7 +- .../PinnedMessagesBannerPresenterTest.kt | 3 +- .../list/PinnedMessagesListPresenterTest.kt | 4 +- .../preferences/impl/about/AboutNode.kt | 4 +- .../impl/developer/DeveloperSettingsNode.kt | 5 +- .../developer/DeveloperSettingsPresenter.kt | 8 +- .../impl/root/PreferencesRootNode.kt | 4 +- .../rageshake/impl/bugreport/BugReportNode.kt | 5 +- .../features/roomlist/impl/RoomListNode.kt | 4 +- .../impl/reset/ResetIdentityFlowNode.kt | 4 +- .../impl/outgoing/VerifySelfSessionNode.kt | 4 +- .../designsystem/utils/ForceOrientation.kt | 5 +- .../matrix/impl/timeline/RustTimeline.kt | 15 +- .../push/impl/push/SyncOnNotifiableEvent.kt | 34 +- .../impl/push/SyncOnNotifiableEventTest.kt | 121 +++++-- .../api/AppForegroundStateService.kt | 22 +- .../appnavstate/api/SyncOrchestrator.kt | 16 + .../api/SyncOrchestratorProvider.kt | 20 ++ .../impl/DefaultAppForegroundStateService.kt | 18 +- .../impl/DefaultAppNavigationStateService.kt | 2 +- .../test/FakeAppForegroundStateService.kt | 22 +- 40 files changed, 935 insertions(+), 291 deletions(-) create mode 100644 appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt rename appnav/src/main/kotlin/io/element/android/appnav/di/{MatrixClientsHolder.kt => MatrixSessionCache.kt} (68%) create mode 100644 appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt delete mode 100644 appnav/src/test/kotlin/io/element/android/appnav/di/MatrixClientsHolderTest.kt create mode 100644 appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt create mode 100644 services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestrator.kt create mode 100644 services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestratorProvider.kt diff --git a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt index ced5d0033a7..ea1a095cfaa 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt @@ -14,9 +14,7 @@ import androidx.compose.runtime.Composable import androidx.compose.runtime.collectAsState import androidx.compose.runtime.getValue import androidx.compose.ui.Modifier -import androidx.lifecycle.Lifecycle import androidx.lifecycle.lifecycleScope -import androidx.lifecycle.repeatOnLifecycle import com.bumble.appyx.core.composable.PermanentChild import com.bumble.appyx.core.lifecycle.subscribe import com.bumble.appyx.core.modality.BuildContext @@ -52,8 +50,6 @@ import io.element.android.features.ftue.api.FtueEntryPoint import io.element.android.features.ftue.api.state.FtueService import io.element.android.features.ftue.api.state.FtueState import io.element.android.features.logout.api.LogoutEntryPoint -import io.element.android.features.networkmonitor.api.NetworkMonitor -import io.element.android.features.networkmonitor.api.NetworkStatus import io.element.android.features.preferences.api.PreferencesEntryPoint import io.element.android.features.roomdirectory.api.RoomDescription import io.element.android.features.roomdirectory.api.RoomDirectoryEntryPoint @@ -77,18 +73,14 @@ import io.element.android.libraries.matrix.api.core.RoomIdOrAlias import io.element.android.libraries.matrix.api.core.UserId import io.element.android.libraries.matrix.api.core.toRoomIdOrAlias import io.element.android.libraries.matrix.api.permalink.PermalinkData -import io.element.android.libraries.matrix.api.sync.SyncState import io.element.android.libraries.matrix.api.verification.SessionVerificationRequestDetails import io.element.android.libraries.matrix.api.verification.SessionVerificationServiceListener import io.element.android.libraries.preferences.api.store.EnableNativeSlidingSyncUseCase import io.element.android.services.appnavstate.api.AppNavigationStateService +import io.element.android.services.appnavstate.api.SyncOrchestratorProvider import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.FlowPreview -import kotlinx.coroutines.flow.combine -import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach -import kotlinx.coroutines.flow.onStart import kotlinx.coroutines.launch import kotlinx.parcelize.Parcelize import timber.log.Timber @@ -107,7 +99,6 @@ class LoggedInFlowNode @AssistedInject constructor( private val userProfileEntryPoint: UserProfileEntryPoint, private val ftueEntryPoint: FtueEntryPoint, private val coroutineScope: CoroutineScope, - private val networkMonitor: NetworkMonitor, private val ftueService: FtueService, private val roomDirectoryEntryPoint: RoomDirectoryEntryPoint, private val shareEntryPoint: ShareEntryPoint, @@ -116,6 +107,7 @@ class LoggedInFlowNode @AssistedInject constructor( private val logoutEntryPoint: LogoutEntryPoint, private val incomingVerificationEntryPoint: IncomingVerificationEntryPoint, private val enableNativeSlidingSyncUseCase: EnableNativeSlidingSyncUseCase, + private val syncOrchestratorProvider: SyncOrchestratorProvider, snackbarDispatcher: SnackbarDispatcher, ) : BaseFlowNode( backstack = BackStack( @@ -133,7 +125,6 @@ class LoggedInFlowNode @AssistedInject constructor( fun onOpenBugReport() } - private val syncService = matrixClient.syncService() private val loggedInFlowProcessor = LoggedInEventProcessor( snackbarDispatcher, matrixClient.roomMembershipObserver(), @@ -147,6 +138,9 @@ class LoggedInFlowNode @AssistedInject constructor( override fun onBuilt() { super.onBuilt() + + syncOrchestratorProvider.getSyncOrchestrator(sessionId = matrixClient.sessionId)?.start() + lifecycle.subscribe( onCreate = { appNavigationStateService.onNavigateToSession(id, matrixClient.sessionId) @@ -165,12 +159,6 @@ class LoggedInFlowNode @AssistedInject constructor( } .launchIn(lifecycleScope) }, - onStop = { - coroutineScope.launch { - // Counterpart startSync is done in observeSyncStateAndNetworkStatus method. - syncService.stopSync() - } - }, onDestroy = { appNavigationStateService.onLeavingSpace(id) appNavigationStateService.onLeavingSession(id) @@ -178,7 +166,6 @@ class LoggedInFlowNode @AssistedInject constructor( matrixClient.sessionVerificationService().setListener(null) } ) - observeSyncStateAndNetworkStatus() setupSendingQueue() } @@ -186,31 +173,6 @@ class LoggedInFlowNode @AssistedInject constructor( sendingQueue.launchIn(lifecycleScope) } - @OptIn(FlowPreview::class) - private fun observeSyncStateAndNetworkStatus() { - lifecycleScope.launch { - repeatOnLifecycle(Lifecycle.State.STARTED) { - 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) - } - .onStart { - // Temporary fix to ensure that the sync is started even if the networkStatus is offline. - syncService.startSync() - } - .collect { (syncState, networkStatus) -> - Timber.d("Sync state: $syncState, network status: $networkStatus") - if (syncState != SyncState.Running && networkStatus == NetworkStatus.Online) { - syncService.startSync() - } - } - } - } - } - sealed interface NavTarget : Parcelable { @Parcelize data object Placeholder : NavTarget diff --git a/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt index 5f5eb7566ed..c52ee9cb5d0 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt @@ -27,7 +27,7 @@ import dagger.assisted.Assisted import dagger.assisted.AssistedInject import im.vector.app.features.analytics.plan.JoinedRoom import io.element.android.anvilannotations.ContributesNode -import io.element.android.appnav.di.MatrixClientsHolder +import io.element.android.appnav.di.MatrixSessionCache import io.element.android.appnav.intent.IntentResolver import io.element.android.appnav.intent.ResolvedIntent import io.element.android.appnav.root.RootNavStateFlowFactory @@ -62,7 +62,7 @@ class RootFlowNode @AssistedInject constructor( @Assisted plugins: List, private val authenticationService: MatrixAuthenticationService, private val navStateFlowFactory: RootNavStateFlowFactory, - private val matrixClientsHolder: MatrixClientsHolder, + private val matrixSessionCache: MatrixSessionCache, private val presenter: RootPresenter, private val bugReportEntryPoint: BugReportEntryPoint, private val viewFolderEntryPoint: ViewFolderEntryPoint, @@ -78,14 +78,14 @@ class RootFlowNode @AssistedInject constructor( plugins = plugins ) { override fun onBuilt() { - matrixClientsHolder.restoreWithSavedState(buildContext.savedStateMap) + matrixSessionCache.restoreWithSavedState(buildContext.savedStateMap) super.onBuilt() observeNavState() } override fun onSaveInstanceState(state: MutableSavedStateMap) { super.onSaveInstanceState(state) - matrixClientsHolder.saveIntoSavedState(state) + matrixSessionCache.saveIntoSavedState(state) navStateFlowFactory.saveIntoSavedState(state) } @@ -118,7 +118,7 @@ class RootFlowNode @AssistedInject constructor( } private fun switchToNotLoggedInFlow() { - matrixClientsHolder.removeAll() + matrixSessionCache.removeAll() backstack.safeRoot(NavTarget.NotLoggedInFlow) } @@ -131,7 +131,7 @@ class RootFlowNode @AssistedInject constructor( onFailure: () -> Unit, onSuccess: (SessionId) -> Unit, ) { - matrixClientsHolder.getOrRestore(sessionId) + matrixSessionCache.getOrRestore(sessionId) .onSuccess { Timber.v("Succeed to restore session $sessionId") onSuccess(sessionId) @@ -200,7 +200,7 @@ class RootFlowNode @AssistedInject constructor( override fun resolve(navTarget: NavTarget, buildContext: BuildContext): Node { return when (navTarget) { is NavTarget.LoggedInFlow -> { - val matrixClient = matrixClientsHolder.getOrNull(navTarget.sessionId) ?: return splashNode(buildContext).also { + val matrixClient = matrixSessionCache.getOrNull(navTarget.sessionId) ?: return splashNode(buildContext).also { Timber.w("Couldn't find any session, go through SplashScreen") } val inputs = LoggedInAppScopeFlowNode.Inputs(matrixClient) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt new file mode 100644 index 00000000000..5b5da2d8f26 --- /dev/null +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt @@ -0,0 +1,148 @@ +/* + * Copyright 2025 New Vector Ltd. + * + * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial + * Please see LICENSE files in the repository root for full details. + */ + +package io.element.android.appnav.di + +import dagger.assisted.Assisted +import dagger.assisted.AssistedFactory +import dagger.assisted.AssistedInject +import io.element.android.features.networkmonitor.api.NetworkMonitor +import io.element.android.features.networkmonitor.api.NetworkStatus +import io.element.android.libraries.core.coroutine.CoroutineDispatchers +import io.element.android.libraries.matrix.api.MatrixClient +import io.element.android.libraries.matrix.api.sync.SyncState +import io.element.android.services.appnavstate.api.AppForegroundStateService +import io.element.android.services.appnavstate.api.SyncOrchestrator +import kotlinx.coroutines.CoroutineName +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.FlowPreview +import kotlinx.coroutines.cancel +import kotlinx.coroutines.delay +import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.debounce +import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.launch +import kotlinx.coroutines.sync.Mutex +import timber.log.Timber +import java.util.concurrent.atomic.AtomicBoolean +import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.seconds + +class DefaultSyncOrchestrator @AssistedInject constructor( + @Assisted matrixClient: MatrixClient, + private val baseCoroutineScope: CoroutineScope = matrixClient.sessionCoroutineScope, + private val appForegroundStateService: AppForegroundStateService, + private val networkMonitor: NetworkMonitor, + private val dispatchers: CoroutineDispatchers, +) : SyncOrchestrator { + @AssistedFactory + interface Factory { + fun create(matrixClient: MatrixClient): DefaultSyncOrchestrator + } + + private val syncService = matrixClient.syncService() + + private val initialSyncMutex = Mutex() + + private var coroutineScope: CoroutineScope? = null + + private val tag = "SyncOrchestrator" + + private val started = AtomicBoolean(false) + + /** + * Starting observing the app state and network state to start/stop the sync service. + * + * Before observing the state, a first attempt at starting the sync service will happen if it's not already running. + */ + @OptIn(FlowPreview::class) + override fun start() { + if (!started.compareAndSet(false, true)) { + Timber.tag(tag).d("already started, exiting early") + return + } + + Timber.tag(tag).d("start observing the app and network state") + + if (syncService.syncState.value != SyncState.Running) { + Timber.tag(tag).d("initial startSync") + baseCoroutineScope.launch(dispatchers.io) { + try { + initialSyncMutex.lock() + syncService.startSync() + + // Wait until it's running + syncService.syncState.first { it == SyncState.Running } + } finally { + initialSyncMutex.unlock() + } + } + } + + coroutineScope = CoroutineScope(baseCoroutineScope.coroutineContext + CoroutineName(tag) + dispatchers.io) + + coroutineScope?.launch { + // Wait until the initial sync is done, either successfully or failing + initialSyncMutex.lock() + + combine( + // small debounce to avoid spamming startSync when the state is changing quickly in case of error. + syncService.syncState.debounce(100.milliseconds), + networkMonitor.connectivity, + appForegroundStateService.isInForeground, + appForegroundStateService.isInCall, + appForegroundStateService.isSyncingNotificationEvent, + ) { syncState, networkState, isInForeground, isInCall, isSyncingNotificationEvent -> + val isAppActive = isInForeground || isInCall || isSyncingNotificationEvent + val isNetworkAvailable = networkState == NetworkStatus.Online + + Timber.tag(tag).d("isAppActive=$isAppActive, isNetworkAvailable=$isNetworkAvailable") + if (syncState == SyncState.Running && (!isAppActive || !isNetworkAvailable)) { + // Don't stop the sync immediately, wait a bit to avoid starting/stopping the sync too often + delay(3.seconds) + SyncStateAction.StopSync + } else if (syncState != SyncState.Running && isAppActive && isNetworkAvailable) { + SyncStateAction.StartSync + } else { + SyncStateAction.NoOp + } + } + .distinctUntilChanged() + .collect { action -> + when (action) { + SyncStateAction.StartSync -> { + syncService.startSync() + } + SyncStateAction.StopSync -> { + syncService.stopSync() + } + SyncStateAction.NoOp -> Unit + } + } + } + } + + /** + * Stop observing the app state and network state. + */ + override fun stop() { + if (!started.compareAndSet(true, false)) { + Timber.tag(tag).d("already stopped, exiting early") + return + } + Timber.tag(tag).d("stop observing the app and network state") + coroutineScope?.cancel() + coroutineScope = null + } +} + +private enum class SyncStateAction { + StartSync, + StopSync, + NoOp, +} diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt similarity index 68% rename from appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt rename to appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt index 2871df28998..04a94b74488 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt @@ -16,6 +16,8 @@ import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.MatrixClientProvider import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService import io.element.android.libraries.matrix.api.core.SessionId +import io.element.android.services.appnavstate.api.SyncOrchestrator +import io.element.android.services.appnavstate.api.SyncOrchestratorProvider import kotlinx.coroutines.runBlocking import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock @@ -25,17 +27,27 @@ import javax.inject.Inject private const val SAVE_INSTANCE_KEY = "io.element.android.x.di.MatrixClientsHolder.SaveInstanceKey" +/** + * In-memory cache for logged in Matrix sessions. + * + * This component contains both the [MatrixClient] and the [SyncOrchestrator] for each session. + */ @SingleIn(AppScope::class) -@ContributesBinding(AppScope::class) -class MatrixClientsHolder @Inject constructor( +@ContributesBinding(AppScope::class, boundType = MatrixClientProvider::class) +@ContributesBinding(AppScope::class, boundType = SyncOrchestratorProvider::class) +class MatrixSessionCache @Inject constructor( private val authenticationService: MatrixAuthenticationService, -) : MatrixClientProvider { - private val sessionIdsToMatrixClient = ConcurrentHashMap() + private val syncOrchestratorFactory: DefaultSyncOrchestrator.Factory, +) : MatrixClientProvider, SyncOrchestratorProvider { + private val sessionIdsToMatrixClient = ConcurrentHashMap() private val restoreMutex = Mutex() init { authenticationService.listenToNewMatrixClients { matrixClient -> - sessionIdsToMatrixClient[matrixClient.sessionId] = matrixClient + sessionIdsToMatrixClient[matrixClient.sessionId] = InMemoryMatrixSession( + matrixClient = matrixClient, + syncOrchestrator = syncOrchestratorFactory.create(matrixClient) + ) } } @@ -48,18 +60,22 @@ class MatrixClientsHolder @Inject constructor( } override fun getOrNull(sessionId: SessionId): MatrixClient? { - return sessionIdsToMatrixClient[sessionId] + return sessionIdsToMatrixClient[sessionId]?.matrixClient } override suspend fun getOrRestore(sessionId: SessionId): Result { return restoreMutex.withLock { - when (val matrixClient = getOrNull(sessionId)) { + when (val cached = getOrNull(sessionId)) { null -> restore(sessionId) - else -> Result.success(matrixClient) + else -> Result.success(cached) } } } + override fun getSyncOrchestrator(sessionId: SessionId): SyncOrchestrator? { + return sessionIdsToMatrixClient[sessionId]?.syncOrchestrator + } + @Suppress("UNCHECKED_CAST") fun restoreWithSavedState(state: SavedStateMap?) { Timber.d("Restore state") @@ -88,10 +104,18 @@ class MatrixClientsHolder @Inject constructor( Timber.d("Restore matrix session: $sessionId") return authenticationService.restoreSession(sessionId) .onSuccess { matrixClient -> - sessionIdsToMatrixClient[matrixClient.sessionId] = matrixClient + sessionIdsToMatrixClient[matrixClient.sessionId] = InMemoryMatrixSession( + matrixClient = matrixClient, + syncOrchestrator = syncOrchestratorFactory.create(matrixClient) + ) } .onFailure { Timber.e(it, "Fail to restore session") } } } + +private data class InMemoryMatrixSession( + val matrixClient: MatrixClient, + val syncOrchestrator: SyncOrchestrator, +) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavStateFlowFactory.kt b/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavStateFlowFactory.kt index 731072889dd..08e999245bf 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavStateFlowFactory.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavStateFlowFactory.kt @@ -9,7 +9,7 @@ package io.element.android.appnav.root import com.bumble.appyx.core.state.MutableSavedStateMap import com.bumble.appyx.core.state.SavedStateMap -import io.element.android.appnav.di.MatrixClientsHolder +import io.element.android.appnav.di.MatrixSessionCache import io.element.android.features.login.api.LoginUserStory import io.element.android.features.preferences.api.CacheService import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService @@ -31,7 +31,7 @@ private const val SAVE_INSTANCE_KEY = "io.element.android.x.RootNavStateFlowFact class RootNavStateFlowFactory @Inject constructor( private val authenticationService: MatrixAuthenticationService, private val cacheService: CacheService, - private val matrixClientsHolder: MatrixClientsHolder, + private val matrixSessionCache: MatrixSessionCache, private val imageLoaderHolder: ImageLoaderHolder, private val loginUserStory: LoginUserStory, private val sessionPreferencesStoreFactory: SessionPreferencesStoreFactory, @@ -63,7 +63,7 @@ class RootNavStateFlowFactory @Inject constructor( val initialCacheIndex = savedStateMap.getCacheIndexOrDefault() return cacheService.clearedCacheEventFlow .onEach { sessionId -> - matrixClientsHolder.remove(sessionId) + matrixSessionCache.remove(sessionId) // Ensure image loader will be recreated with the new MatrixClient imageLoaderHolder.remove(sessionId) // Also remove cached value for SessionPreferencesStore diff --git a/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt new file mode 100644 index 00000000000..46354576373 --- /dev/null +++ b/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt @@ -0,0 +1,333 @@ +/* + * Copyright 2025 New Vector Ltd. + * + * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial + * Please see LICENSE files in the repository root for full details. + */ + +package io.element.android.appnav + +import io.element.android.appnav.di.DefaultSyncOrchestrator +import io.element.android.features.networkmonitor.api.NetworkStatus +import io.element.android.features.networkmonitor.test.FakeNetworkMonitor +import io.element.android.libraries.matrix.api.sync.SyncState +import io.element.android.libraries.matrix.test.FakeMatrixClient +import io.element.android.libraries.matrix.test.sync.FakeSyncService +import io.element.android.services.appnavstate.test.FakeAppForegroundStateService +import io.element.android.tests.testutils.WarmUpRule +import io.element.android.tests.testutils.lambda.lambdaRecorder +import io.element.android.tests.testutils.testCoroutineDispatchers +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.advanceTimeBy +import kotlinx.coroutines.test.runTest +import org.junit.Rule +import org.junit.Test +import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.seconds + +@OptIn(ExperimentalCoroutinesApi::class) +class DefaultSyncOrchestratorTest { + @get:Rule + val warmUpRule = WarmUpRule() + + @Test + fun `when the sync wasn't running before, an initial sync will always take place, even with no network`() = runTest { + val stateFlow = MutableStateFlow(SyncState.Idle) + val startSyncRecorder = lambdaRecorder> { Result.success(Unit) } + val syncService = FakeSyncService(syncStateFlow = stateFlow).apply { + startSyncLambda = startSyncRecorder + } + val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Offline) + val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor) + + // We start observing + syncOrchestrator.start() + + // Advance the time to make sure we left the initial sync behind + advanceTimeBy(1.seconds) + + // Start sync will be called shortly after + startSyncRecorder.assertions().isCalledOnce() + + // Stop observing + syncOrchestrator.stop() + } + + @Test + fun `when the app goes to background and the sync was running, it will be stopped after a delay`() = runTest { + val stateFlow = MutableStateFlow(SyncState.Running) + val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } + val syncService = FakeSyncService(syncStateFlow = stateFlow).apply { + stopSyncLambda = stopSyncRecorder + } + val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Online) + val appForegroundStateService = FakeAppForegroundStateService(initialForegroundValue = true) + val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor, appForegroundStateService) + + // We start observing, we skip the initial sync attempt since the state is running + syncOrchestrator.start() + + // Advance the time to make sure we left the initial sync behind + advanceTimeBy(1.seconds) + + // Stop sync was never called + stopSyncRecorder.assertions().isNeverCalled() + + // Now we send the app to background + appForegroundStateService.isInForeground.value = false + + // Stop sync will be called after some delay + stopSyncRecorder.assertions().isNeverCalled() + advanceTimeBy(10.seconds) + stopSyncRecorder.assertions().isCalledOnce() + + // Stop observing + syncOrchestrator.stop() + } + + @Test + fun `when the app was in background and we receive a notification, a sync will be started then stopped`() = runTest { + val stateFlow = MutableStateFlow(SyncState.Running) + val startSyncRecorder = lambdaRecorder> { Result.success(Unit) } + val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } + val syncService = FakeSyncService(syncStateFlow = stateFlow).apply { + startSyncLambda = startSyncRecorder + stopSyncLambda = stopSyncRecorder + } + val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Online) + val appForegroundStateService = FakeAppForegroundStateService( + initialForegroundValue = false, + initialIsSyncingNotificationEventValue = false, + ) + val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor, appForegroundStateService) + + // We start observing, we skip the initial sync attempt since the state is running + syncOrchestrator.start() + + // Advance the time to make sure we left the initial sync behind + advanceTimeBy(1.seconds) + + // Start sync was never called + startSyncRecorder.assertions().isNeverCalled() + + // We stop the ongoing sync, give the sync service some time to stop + stateFlow.value = SyncState.Idle + advanceTimeBy(10.seconds) + stopSyncRecorder.assertions().isCalledOnce() + + // Now we receive a notification and need to sync + appForegroundStateService.updateIsSyncingNotificationEvent(true) + + // Start sync will be called shortly after + advanceTimeBy(1.milliseconds) + startSyncRecorder.assertions().isCalledOnce() + + // If the sync is running and we mark the notification sync as no longer necessary, the sync stops after a delay + stateFlow.value = SyncState.Running + appForegroundStateService.updateIsSyncingNotificationEvent(false) + + advanceTimeBy(10.seconds) + stopSyncRecorder.assertions().isCalledExactly(2) + + // Stop observing + syncOrchestrator.stop() + } + + @Test + fun `when the app was in background and we join a call, a sync will be started`() = runTest { + val stateFlow = MutableStateFlow(SyncState.Running) + val startSyncRecorder = lambdaRecorder> { Result.success(Unit) } + val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } + val syncService = FakeSyncService(syncStateFlow = stateFlow).apply { + startSyncLambda = startSyncRecorder + stopSyncLambda = stopSyncRecorder + } + val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Online) + val appForegroundStateService = FakeAppForegroundStateService( + initialForegroundValue = false, + initialIsSyncingNotificationEventValue = false, + ) + val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor, appForegroundStateService) + + // We start observing, we skip the initial sync attempt since the state is running + syncOrchestrator.start() + + // Advance the time to make sure we left the initial sync behind + advanceTimeBy(1.seconds) + + // Start sync was never called + startSyncRecorder.assertions().isNeverCalled() + + // We stop the ongoing sync, give the sync service some time to stop + stateFlow.value = SyncState.Idle + advanceTimeBy(10.seconds) + stopSyncRecorder.assertions().isCalledOnce() + + // Now we join a call + appForegroundStateService.updateIsInCallState(true) + + // Start sync will be called shortly after + advanceTimeBy(1.milliseconds) + startSyncRecorder.assertions().isCalledOnce() + + // If the sync is running and we mark the in-call state as false, the sync stops after a delay + stateFlow.value = SyncState.Running + appForegroundStateService.updateIsInCallState(false) + + advanceTimeBy(10.seconds) + stopSyncRecorder.assertions().isCalledExactly(2) + + // Stop observing + syncOrchestrator.stop() + } + + @Test + fun `when the app is in foreground, we sync for a notification and a call is ongoing, the sync will only stop when all conditions are false`() = runTest { + val stateFlow = MutableStateFlow(SyncState.Running) + val startSyncRecorder = lambdaRecorder> { Result.success(Unit) } + val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } + val syncService = FakeSyncService(syncStateFlow = stateFlow).apply { + startSyncLambda = startSyncRecorder + stopSyncLambda = stopSyncRecorder + } + val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Online) + val appForegroundStateService = FakeAppForegroundStateService( + initialForegroundValue = true, + initialIsSyncingNotificationEventValue = true, + initialIsInCallValue = true, + ) + val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor, appForegroundStateService) + + // We start observing, we skip the initial sync attempt since the state is running + syncOrchestrator.start() + + // Advance the time to make sure we left the initial sync behind + advanceTimeBy(1.seconds) + + // Start sync was never called + startSyncRecorder.assertions().isNeverCalled() + + // We send the app to background, it's still syncing + appForegroundStateService.givenIsInForeground(false) + advanceTimeBy(10.seconds) + stopSyncRecorder.assertions().isNeverCalled() + + // We stop the notification sync, it's still syncing + appForegroundStateService.updateIsSyncingNotificationEvent(false) + advanceTimeBy(10.seconds) + stopSyncRecorder.assertions().isNeverCalled() + + // We set the in-call state to false, now it stops syncing after a delay + appForegroundStateService.updateIsInCallState(false) + advanceTimeBy(10.seconds) + stopSyncRecorder.assertions().isCalledOnce() + + // Stop observing + syncOrchestrator.stop() + } + + @Test + fun `if the sync was running, it's set to be stopped but something triggers a sync again, the sync is not stopped`() = runTest { + val stateFlow = MutableStateFlow(SyncState.Running) + val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } + val syncService = FakeSyncService(syncStateFlow = stateFlow).apply { + stopSyncLambda = stopSyncRecorder + } + val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Online) + val appForegroundStateService = FakeAppForegroundStateService( + initialForegroundValue = true, + initialIsSyncingNotificationEventValue = true, + initialIsInCallValue = true, + ) + val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor, appForegroundStateService) + + // We start observing, we skip the initial sync attempt since the state is running + syncOrchestrator.start() + + // Advance the time to make sure we left the initial sync behind + advanceTimeBy(1.seconds) + + // This will set the sync to stop + appForegroundStateService.givenIsInForeground(false) + + // But if we reset it quickly before the stop sync takes place, the sync is not stopped + appForegroundStateService.givenIsInForeground(true) + + advanceTimeBy(10.seconds) + stopSyncRecorder.assertions().isNeverCalled() + + // Stop observing + syncOrchestrator.stop() + } + + @Test + fun `when network is offline, sync service should not start`() = runTest { + val stateFlow = MutableStateFlow(SyncState.Running) + val startSyncRecorder = lambdaRecorder> { Result.success(Unit) } + val syncService = FakeSyncService(syncStateFlow = stateFlow).apply { + startSyncLambda = startSyncRecorder + } + val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Offline) + val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor) + + // We start observing, we skip the initial sync attempt since the state is running + syncOrchestrator.start() + + // Advance the time to make sure we left the initial sync behind + advanceTimeBy(1.seconds) + + // Set the sync state to idle + stateFlow.value = SyncState.Idle + + // This should still not trigger a sync, since there is no network + advanceTimeBy(10.seconds) + startSyncRecorder.assertions().isNeverCalled() + + // Stop observing + syncOrchestrator.stop() + } + + @Test + fun `when sync was running and network is now offline, sync service should be stopped`() = runTest { + val stateFlow = MutableStateFlow(SyncState.Running) + val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } + val syncService = FakeSyncService(syncStateFlow = stateFlow).apply { + stopSyncLambda = stopSyncRecorder + } + val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Online) + val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor) + + // We start observing, we skip the initial sync attempt since the state is running + syncOrchestrator.start() + + // Advance the time to make sure we left the initial sync behind + advanceTimeBy(1.seconds) + + // Network is now offline + networkMonitor.connectivity.value = NetworkStatus.Offline + + // This will stop the sync after some delay + stopSyncRecorder.assertions().isNeverCalled() + advanceTimeBy(10.seconds) + stopSyncRecorder.assertions().isCalledOnce() + + // Stop observing + syncOrchestrator.stop() + } + + private fun TestScope.createSyncOrchestrator( + syncService: FakeSyncService = FakeSyncService(), + networkMonitor: FakeNetworkMonitor = FakeNetworkMonitor(), + appForegroundStateService: FakeAppForegroundStateService = FakeAppForegroundStateService(), + ) = DefaultSyncOrchestrator( + matrixClient = FakeMatrixClient(syncService = syncService), + networkMonitor = networkMonitor, + appForegroundStateService = appForegroundStateService, + baseCoroutineScope = CoroutineScope(coroutineContext + SupervisorJob()), + dispatchers = testCoroutineDispatchers(), + ) +} diff --git a/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixClientsHolderTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixClientsHolderTest.kt deleted file mode 100644 index 23dfc9c5222..00000000000 --- a/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixClientsHolderTest.kt +++ /dev/null @@ -1,97 +0,0 @@ -/* - * Copyright 2023, 2024 New Vector Ltd. - * - * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial - * Please see LICENSE files in the repository root for full details. - */ - -package io.element.android.appnav.di - -import com.bumble.appyx.core.state.MutableSavedStateMapImpl -import com.google.common.truth.Truth.assertThat -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.auth.FakeMatrixAuthenticationService -import kotlinx.coroutines.test.runTest -import org.junit.Test - -class MatrixClientsHolderTest { - @Test - fun `test getOrNull`() { - val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixClientsHolder = MatrixClientsHolder(fakeAuthenticationService) - assertThat(matrixClientsHolder.getOrNull(A_SESSION_ID)).isNull() - } - - @Test - fun `test getOrRestore`() = runTest { - val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixClientsHolder = MatrixClientsHolder(fakeAuthenticationService) - val fakeMatrixClient = FakeMatrixClient() - fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) - assertThat(matrixClientsHolder.getOrNull(A_SESSION_ID)).isNull() - assertThat(matrixClientsHolder.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) - // Do it again to hit the cache - assertThat(matrixClientsHolder.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) - assertThat(matrixClientsHolder.getOrNull(A_SESSION_ID)).isEqualTo(fakeMatrixClient) - } - - @Test - fun `test remove`() = runTest { - val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixClientsHolder = MatrixClientsHolder(fakeAuthenticationService) - val fakeMatrixClient = FakeMatrixClient() - fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) - assertThat(matrixClientsHolder.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) - assertThat(matrixClientsHolder.getOrNull(A_SESSION_ID)).isEqualTo(fakeMatrixClient) - // Remove - matrixClientsHolder.remove(A_SESSION_ID) - assertThat(matrixClientsHolder.getOrNull(A_SESSION_ID)).isNull() - } - - @Test - fun `test remove all`() = runTest { - val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixClientsHolder = MatrixClientsHolder(fakeAuthenticationService) - val fakeMatrixClient = FakeMatrixClient() - fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) - assertThat(matrixClientsHolder.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) - assertThat(matrixClientsHolder.getOrNull(A_SESSION_ID)).isEqualTo(fakeMatrixClient) - // Remove all - matrixClientsHolder.removeAll() - assertThat(matrixClientsHolder.getOrNull(A_SESSION_ID)).isNull() - } - - @Test - fun `test save and restore`() = runTest { - val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixClientsHolder = MatrixClientsHolder(fakeAuthenticationService) - val fakeMatrixClient = FakeMatrixClient() - fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) - matrixClientsHolder.getOrRestore(A_SESSION_ID) - val savedStateMap = MutableSavedStateMapImpl { true } - matrixClientsHolder.saveIntoSavedState(savedStateMap) - assertThat(savedStateMap.size).isEqualTo(1) - // Test Restore with non-empty map - matrixClientsHolder.restoreWithSavedState(savedStateMap) - // Empty the map - matrixClientsHolder.removeAll() - assertThat(matrixClientsHolder.getOrNull(A_SESSION_ID)).isNull() - // Restore again - matrixClientsHolder.restoreWithSavedState(savedStateMap) - assertThat(matrixClientsHolder.getOrNull(A_SESSION_ID)).isEqualTo(fakeMatrixClient) - } - - @Test - fun `test AuthenticationService listenToNewMatrixClients emits a Client value and we save it`() = runTest { - val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixClientsHolder = MatrixClientsHolder(fakeAuthenticationService) - assertThat(matrixClientsHolder.getOrNull(A_SESSION_ID)).isNull() - - fakeAuthenticationService.givenMatrixClient(FakeMatrixClient(sessionId = A_SESSION_ID)) - val loginSucceeded = fakeAuthenticationService.login("user", "pass") - - assertThat(loginSucceeded.isSuccess).isTrue() - assertThat(matrixClientsHolder.getOrNull(A_SESSION_ID)).isNotNull() - } -} diff --git a/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt new file mode 100644 index 00000000000..d68500b7bde --- /dev/null +++ b/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt @@ -0,0 +1,132 @@ +/* + * Copyright 2023, 2024 New Vector Ltd. + * + * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial + * Please see LICENSE files in the repository root for full details. + */ + +package io.element.android.appnav.di + +import com.bumble.appyx.core.state.MutableSavedStateMapImpl +import com.google.common.truth.Truth.assertThat +import io.element.android.features.networkmonitor.test.FakeNetworkMonitor +import io.element.android.libraries.matrix.api.MatrixClient +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.auth.FakeMatrixAuthenticationService +import io.element.android.services.appnavstate.test.FakeAppForegroundStateService +import io.element.android.tests.testutils.testCoroutineDispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.runTest +import org.junit.Test + +class MatrixSessionCacheTest { + @Test + fun `test getOrNull`() = runTest { + val fakeAuthenticationService = FakeMatrixAuthenticationService() + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() + } + + @OptIn(ExperimentalCoroutinesApi::class) + @Test + fun `test getSyncOrchestratorOrNull`() = runTest { + val fakeAuthenticationService = FakeMatrixAuthenticationService() + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + + // With no matrix client there is no sync orchestrator + assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() + assertThat(matrixSessionCache.getSyncOrchestrator(A_SESSION_ID)).isNull() + + // But as soon as we receive a client, we can get the sync orchestrator + val fakeMatrixClient = FakeMatrixClient() + fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) + assertThat(matrixSessionCache.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) + assertThat(matrixSessionCache.getSyncOrchestrator(A_SESSION_ID)).isNotNull() + } + + @Test + fun `test getOrRestore`() = runTest { + val fakeAuthenticationService = FakeMatrixAuthenticationService() + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + val fakeMatrixClient = FakeMatrixClient() + fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) + assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() + assertThat(matrixSessionCache.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) + // Do it again to hit the cache + assertThat(matrixSessionCache.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) + assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isEqualTo(fakeMatrixClient) + } + + @Test + fun `test remove`() = runTest { + val fakeAuthenticationService = FakeMatrixAuthenticationService() + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + val fakeMatrixClient = FakeMatrixClient() + fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) + assertThat(matrixSessionCache.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) + assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isEqualTo(fakeMatrixClient) + // Remove + matrixSessionCache.remove(A_SESSION_ID) + assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() + } + + @Test + fun `test remove all`() = runTest { + val fakeAuthenticationService = FakeMatrixAuthenticationService() + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + val fakeMatrixClient = FakeMatrixClient() + fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) + assertThat(matrixSessionCache.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) + assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isEqualTo(fakeMatrixClient) + // Remove all + matrixSessionCache.removeAll() + assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() + } + + @Test + fun `test save and restore`() = runTest { + val fakeAuthenticationService = FakeMatrixAuthenticationService() + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + val fakeMatrixClient = FakeMatrixClient() + fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) + matrixSessionCache.getOrRestore(A_SESSION_ID) + val savedStateMap = MutableSavedStateMapImpl { true } + matrixSessionCache.saveIntoSavedState(savedStateMap) + assertThat(savedStateMap.size).isEqualTo(1) + // Test Restore with non-empty map + matrixSessionCache.restoreWithSavedState(savedStateMap) + // Empty the map + matrixSessionCache.removeAll() + assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() + // Restore again + matrixSessionCache.restoreWithSavedState(savedStateMap) + assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isEqualTo(fakeMatrixClient) + } + + @Test + fun `test AuthenticationService listenToNewMatrixClients emits a Client value and we save it`() = runTest { + val fakeAuthenticationService = FakeMatrixAuthenticationService() + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() + + fakeAuthenticationService.givenMatrixClient(FakeMatrixClient(sessionId = A_SESSION_ID)) + val loginSucceeded = fakeAuthenticationService.login("user", "pass") + + assertThat(loginSucceeded.isSuccess).isTrue() + assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNotNull() + } + + private fun TestScope.createSyncOrchestratorFactory() = object : DefaultSyncOrchestrator.Factory { + override fun create(matrixClient: MatrixClient): DefaultSyncOrchestrator { + return DefaultSyncOrchestrator( + matrixClient, + baseCoroutineScope = this@createSyncOrchestratorFactory, + appForegroundStateService = FakeAppForegroundStateService(), + networkMonitor = FakeNetworkMonitor(), + dispatchers = testCoroutineDispatchers(), + ) + } + } +} diff --git a/features/analytics/impl/src/main/kotlin/io/element/android/features/analytics/impl/AnalyticsOptInNode.kt b/features/analytics/impl/src/main/kotlin/io/element/android/features/analytics/impl/AnalyticsOptInNode.kt index 01ec23da385..ba1073d480c 100644 --- a/features/analytics/impl/src/main/kotlin/io/element/android/features/analytics/impl/AnalyticsOptInNode.kt +++ b/features/analytics/impl/src/main/kotlin/io/element/android/features/analytics/impl/AnalyticsOptInNode.kt @@ -8,9 +8,9 @@ package io.element.android.features.analytics.impl import android.app.Activity +import androidx.activity.compose.LocalActivity import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier -import androidx.compose.ui.platform.LocalContext import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin @@ -34,7 +34,7 @@ class AnalyticsOptInNode @AssistedInject constructor( @Composable override fun View(modifier: Modifier) { - val activity = LocalContext.current as Activity + val activity = requireNotNull(LocalActivity.current) val isDark = ElementTheme.isLightTheme.not() val state = presenter.present() AnalyticsOptInView( diff --git a/features/call/impl/build.gradle.kts b/features/call/impl/build.gradle.kts index 7852b5f53c0..70e0ee631c1 100644 --- a/features/call/impl/build.gradle.kts +++ b/features/call/impl/build.gradle.kts @@ -40,6 +40,7 @@ dependencies { implementation(projects.libraries.push.api) implementation(projects.libraries.uiStrings) implementation(projects.services.analytics.api) + implementation(projects.services.appnavstate.api) implementation(projects.services.toolbox.api) implementation(libs.androidx.webkit) implementation(libs.coil.compose) @@ -59,6 +60,7 @@ dependencies { testImplementation(projects.libraries.matrix.test) testImplementation(projects.libraries.push.test) testImplementation(projects.services.analytics.test) + testImplementation(projects.services.appnavstate.test) testImplementation(projects.tests.testutils) testImplementation(libs.androidx.compose.ui.test.junit) testReleaseImplementation(libs.androidx.compose.ui.test.manifest) diff --git a/features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/CallScreenPresenter.kt b/features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/CallScreenPresenter.kt index 048e361ac10..63c5953802d 100644 --- a/features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/CallScreenPresenter.kt +++ b/features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/CallScreenPresenter.kt @@ -39,6 +39,7 @@ import io.element.android.libraries.matrix.api.sync.SyncState import io.element.android.libraries.matrix.api.widget.MatrixWidgetDriver import io.element.android.libraries.network.useragent.UserAgentProvider import io.element.android.services.analytics.api.ScreenTracker +import io.element.android.services.appnavstate.api.AppForegroundStateService import io.element.android.services.toolbox.api.systemclock.SystemClock import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.flow.launchIn @@ -58,9 +59,9 @@ class CallScreenPresenter @AssistedInject constructor( private val dispatchers: CoroutineDispatchers, private val matrixClientsProvider: MatrixClientProvider, private val screenTracker: ScreenTracker, - private val appCoroutineScope: CoroutineScope, private val activeCallManager: ActiveCallManager, private val languageTagProvider: LanguageTagProvider, + private val appForegroundStateService: AppForegroundStateService, ) : Presenter { @AssistedFactory interface Factory { @@ -226,18 +227,14 @@ class CallScreenPresenter @AssistedInject constructor( if (state == SyncState.Running) { client.notifyCallStartIfNeeded(callType.roomId) } else { - client.syncService().startSync() + appForegroundStateService.updateIsInCallState(true) } } } onDispose { - // We can't use the local coroutine scope here because it will be disposed before this effect - appCoroutineScope.launch { - client.syncService().run { - if (syncState.value == SyncState.Running) { - stopSync() - } - } + // Make sure we mark the call as ended in the app state + if (appForegroundStateService.isInCall.value) { + appForegroundStateService.updateIsInCallState(false) } } } diff --git a/features/call/impl/src/test/kotlin/io/element/android/features/call/ui/CallScreenPresenterTest.kt b/features/call/impl/src/test/kotlin/io/element/android/features/call/ui/CallScreenPresenterTest.kt index 2ab7156878b..2024485d4aa 100644 --- a/features/call/impl/src/test/kotlin/io/element/android/features/call/ui/CallScreenPresenterTest.kt +++ b/features/call/impl/src/test/kotlin/io/element/android/features/call/ui/CallScreenPresenterTest.kt @@ -32,10 +32,9 @@ import io.element.android.libraries.matrix.test.widget.FakeMatrixWidgetDriver import io.element.android.libraries.network.useragent.UserAgentProvider import io.element.android.services.analytics.api.ScreenTracker import io.element.android.services.analytics.test.FakeScreenTracker +import io.element.android.services.appnavstate.test.FakeAppForegroundStateService import io.element.android.services.toolbox.api.systemclock.SystemClock import io.element.android.tests.testutils.WarmUpRule -import io.element.android.tests.testutils.consumeItemsUntilTimeout -import io.element.android.tests.testutils.lambda.assert import io.element.android.tests.testutils.lambda.lambdaRecorder import io.element.android.tests.testutils.lambda.value import io.element.android.tests.testutils.testCoroutineDispatchers @@ -243,7 +242,7 @@ class CallScreenPresenterTest { } @Test - fun `present - automatically starts the Matrix client sync when on RoomCall`() = runTest { + fun `present - automatically sets the isInCall state when starting the call and disposing the screen`() = runTest { val navigator = FakeCallScreenNavigator() val widgetDriver = FakeMatrixWidgetDriver() val startSyncLambda = lambdaRecorder> { Result.success(Unit) } @@ -251,6 +250,7 @@ class CallScreenPresenterTest { this.startSyncLambda = startSyncLambda } val matrixClient = FakeMatrixClient(syncService = syncService) + val appForegroundStateService = FakeAppForegroundStateService() val presenter = createCallScreenPresenter( callType = CallType.RoomCall(A_SESSION_ID, A_ROOM_ID), widgetDriver = widgetDriver, @@ -258,6 +258,7 @@ class CallScreenPresenterTest { dispatchers = testCoroutineDispatchers(useUnconfinedTestDispatcher = true), matrixClientsProvider = FakeMatrixClientProvider(getClient = { Result.success(matrixClient) }), screenTracker = FakeScreenTracker {}, + appForegroundStateService = appForegroundStateService, ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() @@ -296,11 +297,25 @@ class CallScreenPresenterTest { } } - hasRun.lock() + appForegroundStateService.isInCall.test { + // The initial isInCall state will always be false + assertThat(awaitItem()).isFalse() - job.cancelAndJoin() + // Wait until the call starts + hasRun.lock() - assert(stopSyncLambda).isCalledOnce() + // Then it'll be true once the call is active + assertThat(awaitItem()).isTrue() + + // If we dispose the screen + job.cancelAndJoin() + + // The isInCall state is now false + assertThat(awaitItem()).isFalse() + + // And there are no more events + ensureAllEventsConsumed() + } } @Test @@ -354,6 +369,7 @@ class CallScreenPresenterTest { matrixClientsProvider: FakeMatrixClientProvider = FakeMatrixClientProvider(), activeCallManager: FakeActiveCallManager = FakeActiveCallManager(), screenTracker: ScreenTracker = FakeScreenTracker(), + appForegroundStateService: FakeAppForegroundStateService = FakeAppForegroundStateService(), ): CallScreenPresenter { val userAgentProvider = object : UserAgentProvider { override fun provide(): String { @@ -369,10 +385,10 @@ class CallScreenPresenterTest { clock = clock, dispatchers = dispatchers, matrixClientsProvider = matrixClientsProvider, - appCoroutineScope = this, activeCallManager = activeCallManager, screenTracker = screenTracker, languageTagProvider = FakeLanguageTagProvider("en-US"), + appForegroundStateService = appForegroundStateService, ) } } diff --git a/features/createroom/impl/src/main/kotlin/io/element/android/features/createroom/impl/root/CreateRoomRootNode.kt b/features/createroom/impl/src/main/kotlin/io/element/android/features/createroom/impl/root/CreateRoomRootNode.kt index aef27b94dbf..f3a836f4412 100644 --- a/features/createroom/impl/src/main/kotlin/io/element/android/features/createroom/impl/root/CreateRoomRootNode.kt +++ b/features/createroom/impl/src/main/kotlin/io/element/android/features/createroom/impl/root/CreateRoomRootNode.kt @@ -8,9 +8,9 @@ package io.element.android.features.createroom.impl.root import android.app.Activity +import androidx.activity.compose.LocalActivity import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier -import androidx.compose.ui.platform.LocalContext import com.bumble.appyx.core.lifecycle.subscribe import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node @@ -55,7 +55,7 @@ class CreateRoomRootNode @AssistedInject constructor( @Composable override fun View(modifier: Modifier) { val state = presenter.present() - val activity = LocalContext.current as Activity + val activity = requireNotNull(LocalActivity.current) CreateRoomRootView( state = state, modifier = modifier, diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/DefaultLockScreenService.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/DefaultLockScreenService.kt index fa2a5b493e3..681375357b8 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/DefaultLockScreenService.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/DefaultLockScreenService.kt @@ -94,7 +94,7 @@ class DefaultLockScreenService @Inject constructor( */ private fun observeAppForegroundState() { coroutineScope.launch { - appForegroundStateService.start() + appForegroundStateService.startObservingForeground() appForegroundStateService.isInForeground.collect { isInForeground -> if (isInForeground) { lockJob?.cancel() diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockNode.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockNode.kt index 9f7baed365a..f62df0a55d4 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockNode.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockNode.kt @@ -7,11 +7,10 @@ package io.element.android.features.lockscreen.impl.unlock -import android.app.Activity +import androidx.activity.compose.LocalActivity import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect import androidx.compose.ui.Modifier -import androidx.compose.ui.platform.LocalContext import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin @@ -42,7 +41,7 @@ class PinUnlockNode @AssistedInject constructor( @Composable override fun View(modifier: Modifier) { val state = presenter.present() - val activity = LocalContext.current as Activity + val activity = requireNotNull(LocalActivity.current) val isDark = ElementTheme.isLightTheme.not() LaunchedEffect(state.isUnlocked) { if (state.isUnlocked) { diff --git a/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/LoginFlowNode.kt b/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/LoginFlowNode.kt index c49ba828095..ef8673801e2 100644 --- a/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/LoginFlowNode.kt +++ b/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/LoginFlowNode.kt @@ -9,10 +9,10 @@ package io.element.android.features.login.impl import android.app.Activity import android.os.Parcelable +import androidx.activity.compose.LocalActivity import androidx.compose.runtime.Composable import androidx.compose.runtime.DisposableEffect import androidx.compose.ui.Modifier -import androidx.compose.ui.platform.LocalContext import androidx.lifecycle.lifecycleScope import com.bumble.appyx.core.lifecycle.subscribe import com.bumble.appyx.core.modality.BuildContext @@ -199,7 +199,7 @@ class LoginFlowNode @AssistedInject constructor( @Composable override fun View(modifier: Modifier) { - activity = LocalContext.current as? Activity + activity = LocalActivity.current darkTheme = !ElementTheme.isLightTheme DisposableEffect(Unit) { onDispose { diff --git a/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/screens/createaccount/CreateAccountNode.kt b/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/screens/createaccount/CreateAccountNode.kt index 6cd039a2c75..128a4c03e87 100644 --- a/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/screens/createaccount/CreateAccountNode.kt +++ b/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/screens/createaccount/CreateAccountNode.kt @@ -8,9 +8,9 @@ package io.element.android.features.login.impl.screens.createaccount import android.app.Activity +import androidx.activity.compose.LocalActivity import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier -import androidx.compose.ui.platform.LocalContext import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin @@ -41,7 +41,7 @@ class CreateAccountNode @AssistedInject constructor( @Composable override fun View(modifier: Modifier) { - val activity = LocalContext.current as Activity + val activity = requireNotNull(LocalActivity.current) val isDark = ElementTheme.isLightTheme.not() val state = presenter.present() CreateAccountView( diff --git a/features/logout/impl/src/main/kotlin/io/element/android/features/logout/impl/LogoutNode.kt b/features/logout/impl/src/main/kotlin/io/element/android/features/logout/impl/LogoutNode.kt index 140982f0e8c..2ecc027fba3 100644 --- a/features/logout/impl/src/main/kotlin/io/element/android/features/logout/impl/LogoutNode.kt +++ b/features/logout/impl/src/main/kotlin/io/element/android/features/logout/impl/LogoutNode.kt @@ -7,10 +7,9 @@ package io.element.android.features.logout.impl -import android.app.Activity +import androidx.activity.compose.LocalActivity import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier -import androidx.compose.ui.platform.LocalContext import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin @@ -42,7 +41,7 @@ class LogoutNode @AssistedInject constructor( @Composable override fun View(modifier: Modifier) { val state = presenter.present() - val activity = LocalContext.current as Activity + val activity = requireNotNull(LocalActivity.current) val isDark = ElementTheme.isLightTheme.not() LogoutView( state = state, diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesNode.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesNode.kt index 2ab3bbbc862..4060f612779 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesNode.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesNode.kt @@ -9,6 +9,7 @@ package io.element.android.features.messages.impl import android.app.Activity import android.content.Context +import androidx.activity.compose.LocalActivity import androidx.compose.runtime.Composable import androidx.compose.runtime.CompositionLocalProvider import androidx.compose.runtime.LaunchedEffect @@ -17,7 +18,6 @@ import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.saveable.rememberSaveable import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier -import androidx.compose.ui.platform.LocalContext import androidx.lifecycle.Lifecycle import com.bumble.appyx.core.lifecycle.subscribe import com.bumble.appyx.core.modality.BuildContext @@ -215,7 +215,7 @@ class MessagesNode @AssistedInject constructor( @Composable override fun View(modifier: Modifier) { - val activity = LocalContext.current as Activity + val activity = requireNotNull(LocalActivity.current) val isDark = ElementTheme.isLightTheme.not() CompositionLocalProvider( LocalTimelineItemPresenterFactories provides timelineItemPresenterFactories, diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/PinnedEventsTimelineProvider.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/PinnedEventsTimelineProvider.kt index 8840411a24a..43b4fef3dd0 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/PinnedEventsTimelineProvider.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/PinnedEventsTimelineProvider.kt @@ -8,6 +8,7 @@ package io.element.android.features.messages.impl.pinned import io.element.android.libraries.architecture.AsyncData +import io.element.android.libraries.core.coroutine.CoroutineDispatchers import io.element.android.libraries.core.coroutine.mapState import io.element.android.libraries.di.RoomScope import io.element.android.libraries.di.SingleIn @@ -26,6 +27,7 @@ import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.withContext import javax.inject.Inject @SingleIn(RoomScope::class) @@ -33,6 +35,7 @@ class PinnedEventsTimelineProvider @Inject constructor( private val room: MatrixRoom, private val syncService: SyncService, private val featureFlagService: FeatureFlagService, + private val dispatchers: CoroutineDispatchers, ) : TimelineProvider { private val _timelineStateFlow: MutableStateFlow> = MutableStateFlow(AsyncData.Uninitialized) @@ -100,7 +103,9 @@ class PinnedEventsTimelineProvider @Inject constructor( when (timelineStateFlow.value) { is AsyncData.Uninitialized, is AsyncData.Failure -> { timelineStateFlow.emit(AsyncData.Loading()) - room.pinnedEventsTimeline() + withContext(dispatchers.io) { + room.pinnedEventsTimeline() + } .fold( { timelineStateFlow.emit(AsyncData.Success(it)) }, { timelineStateFlow.emit(AsyncData.Failure(it)) } diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerPresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerPresenterTest.kt index fdd01f5ad1e..8aa7bee7796 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerPresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerPresenterTest.kt @@ -194,7 +194,8 @@ class PinnedMessagesBannerPresenterTest { syncService = syncService, featureFlagService = FakeFeatureFlagService( initialState = mapOf(FeatureFlags.PinnedEvents.key to isFeatureEnabled) - ) + ), + dispatchers = testCoroutineDispatchers(), ) timelineProvider.launchIn(backgroundScope) diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListPresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListPresenterTest.kt index 3799eb2c741..976c7da29cc 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListPresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListPresenterTest.kt @@ -38,6 +38,7 @@ import io.element.android.tests.testutils.lambda.assert import io.element.android.tests.testutils.lambda.lambdaRecorder import io.element.android.tests.testutils.lambda.value import io.element.android.tests.testutils.test +import io.element.android.tests.testutils.testCoroutineDispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.test.TestScope @@ -302,7 +303,8 @@ class PinnedMessagesListPresenterTest { syncService = syncService, featureFlagService = FakeFeatureFlagService( initialState = mapOf(FeatureFlags.PinnedEvents.key to isFeatureEnabled) - ) + ), + dispatchers = testCoroutineDispatchers(), ) timelineProvider.launchIn(backgroundScope) return PinnedMessagesListPresenter( diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/about/AboutNode.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/about/AboutNode.kt index ea3a411d0a3..08be5069906 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/about/AboutNode.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/about/AboutNode.kt @@ -8,9 +8,9 @@ package io.element.android.features.preferences.impl.about import android.app.Activity +import androidx.activity.compose.LocalActivity import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier -import androidx.compose.ui.platform.LocalContext import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin @@ -41,7 +41,7 @@ class AboutNode @AssistedInject constructor( @Composable override fun View(modifier: Modifier) { - val activity = LocalContext.current as Activity + val activity = requireNotNull(LocalActivity.current) val isDark = ElementTheme.isLightTheme.not() val state = presenter.present() AboutView( diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/developer/DeveloperSettingsNode.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/developer/DeveloperSettingsNode.kt index 91f8afcb8ef..fafe5fc9205 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/developer/DeveloperSettingsNode.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/developer/DeveloperSettingsNode.kt @@ -7,10 +7,9 @@ package io.element.android.features.preferences.impl.developer -import android.app.Activity +import androidx.activity.compose.LocalActivity import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier -import androidx.compose.ui.platform.LocalContext import com.airbnb.android.showkase.models.Showkase import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node @@ -29,7 +28,7 @@ class DeveloperSettingsNode @AssistedInject constructor( ) : Node(buildContext, plugins = plugins) { @Composable override fun View(modifier: Modifier) { - val activity = LocalContext.current as Activity + val activity = requireNotNull(LocalActivity.current) fun openShowkase() { val intent = Showkase.getBrowserIntent(activity) activity.startActivity(intent) diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/developer/DeveloperSettingsPresenter.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/developer/DeveloperSettingsPresenter.kt index 66c8d8d7b3e..0dd16492ccf 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/developer/DeveloperSettingsPresenter.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/developer/DeveloperSettingsPresenter.kt @@ -79,10 +79,10 @@ class DeveloperSettingsPresenter @Inject constructor( .doesHideImagesAndVideosFlow() .collectAsState(initial = false) - val tracingLogLevel by appPreferencesStore - .getTracingLogLevelFlow() - .map { AsyncData.Success(it.toLogLevelItem()) } - .collectAsState(initial = AsyncData.Uninitialized) + val tracingLogLevelFlow = remember { + appPreferencesStore.getTracingLogLevelFlow().map { AsyncData.Success(it.toLogLevelItem()) } + } + val tracingLogLevel by tracingLogLevelFlow.collectAsState(initial = AsyncData.Uninitialized) LaunchedEffect(Unit) { FeatureFlags.entries diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/root/PreferencesRootNode.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/root/PreferencesRootNode.kt index 57196621b92..887f14a7f7b 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/root/PreferencesRootNode.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/root/PreferencesRootNode.kt @@ -8,9 +8,9 @@ package io.element.android.features.preferences.impl.root import android.app.Activity +import androidx.activity.compose.LocalActivity import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier -import androidx.compose.ui.platform.LocalContext import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin @@ -113,7 +113,7 @@ class PreferencesRootNode @AssistedInject constructor( @Composable override fun View(modifier: Modifier) { val state = presenter.present() - val activity = LocalContext.current as Activity + val activity = requireNotNull(LocalActivity.current) val isDark = ElementTheme.isLightTheme.not() PreferencesRootView( state = state, diff --git a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportNode.kt b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportNode.kt index e1e394c46f5..c124971efbd 100644 --- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportNode.kt +++ b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportNode.kt @@ -7,10 +7,9 @@ package io.element.android.features.rageshake.impl.bugreport -import android.app.Activity +import androidx.activity.compose.LocalActivity import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier -import androidx.compose.ui.platform.LocalContext import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin @@ -38,7 +37,7 @@ class BugReportNode @AssistedInject constructor( @Composable override fun View(modifier: Modifier) { val state = presenter.present() - val activity = LocalContext.current as? Activity + val activity = LocalActivity.current BugReportView( state = state, modifier = modifier, diff --git a/features/roomlist/impl/src/main/kotlin/io/element/android/features/roomlist/impl/RoomListNode.kt b/features/roomlist/impl/src/main/kotlin/io/element/android/features/roomlist/impl/RoomListNode.kt index ba69e31ca2c..fd72d88c172 100644 --- a/features/roomlist/impl/src/main/kotlin/io/element/android/features/roomlist/impl/RoomListNode.kt +++ b/features/roomlist/impl/src/main/kotlin/io/element/android/features/roomlist/impl/RoomListNode.kt @@ -8,9 +8,9 @@ package io.element.android.features.roomlist.impl import android.app.Activity +import androidx.activity.compose.LocalActivity import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier -import androidx.compose.ui.platform.LocalContext import com.bumble.appyx.core.lifecycle.subscribe import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node @@ -92,7 +92,7 @@ class RoomListNode @AssistedInject constructor( @Composable override fun View(modifier: Modifier) { val state = presenter.present() - val activity = LocalContext.current as Activity + val activity = requireNotNull(LocalActivity.current) RoomListView( state = state, diff --git a/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/reset/ResetIdentityFlowNode.kt b/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/reset/ResetIdentityFlowNode.kt index 5e97f4193da..c8e9072c6b2 100644 --- a/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/reset/ResetIdentityFlowNode.kt +++ b/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/reset/ResetIdentityFlowNode.kt @@ -9,11 +9,11 @@ package io.element.android.features.securebackup.impl.reset import android.app.Activity import android.os.Parcelable +import androidx.activity.compose.LocalActivity import androidx.compose.runtime.Composable import androidx.compose.runtime.collectAsState import androidx.compose.runtime.getValue import androidx.compose.ui.Modifier -import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.window.DialogProperties import androidx.lifecycle.DefaultLifecycleObserver import androidx.lifecycle.LifecycleOwner @@ -160,7 +160,7 @@ class ResetIdentityFlowNode @AssistedInject constructor( override fun View(modifier: Modifier) { // Workaround to get the current activity if (!this::activity.isInitialized) { - activity = LocalContext.current as Activity + activity = requireNotNull(LocalActivity.current) } val startResetState by resetIdentityFlowManager.currentHandleFlow.collectAsState() diff --git a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/outgoing/VerifySelfSessionNode.kt b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/outgoing/VerifySelfSessionNode.kt index bf7afffaadb..f58d22619e7 100644 --- a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/outgoing/VerifySelfSessionNode.kt +++ b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/outgoing/VerifySelfSessionNode.kt @@ -8,9 +8,9 @@ package io.element.android.features.verifysession.impl.outgoing import android.app.Activity +import androidx.activity.compose.LocalActivity import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier -import androidx.compose.ui.platform.LocalContext import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin @@ -45,7 +45,7 @@ class VerifySelfSessionNode @AssistedInject constructor( @Composable override fun View(modifier: Modifier) { val state = presenter.present() - val activity = LocalContext.current as Activity + val activity = requireNotNull(LocalActivity.current) val isDark = ElementTheme.isLightTheme.not() VerifySelfSessionView( state = state, diff --git a/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/utils/ForceOrientation.kt b/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/utils/ForceOrientation.kt index bb2833a0224..37e6e1c93e0 100644 --- a/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/utils/ForceOrientation.kt +++ b/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/utils/ForceOrientation.kt @@ -8,14 +8,13 @@ package io.element.android.libraries.designsystem.utils import android.content.pm.ActivityInfo -import androidx.activity.ComponentActivity +import androidx.activity.compose.LocalActivity import androidx.compose.runtime.Composable import androidx.compose.runtime.DisposableEffect -import androidx.compose.ui.platform.LocalContext @Composable fun ForceOrientation(orientation: ScreenOrientation) { - val activity = LocalContext.current as? ComponentActivity ?: return + val activity = LocalActivity.current ?: return val orientationFlags = when (orientation) { ScreenOrientation.PORTRAIT -> ActivityInfo.SCREEN_ORIENTATION_PORTRAIT ScreenOrientation.LANDSCAPE -> ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustTimeline.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustTimeline.kt index 2cb1a4efa50..1fbfea7b13d 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustTimeline.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustTimeline.kt @@ -48,6 +48,7 @@ import io.element.android.services.toolbox.api.systemclock.SystemClock import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.cancel import kotlinx.coroutines.flow.Flow @@ -296,7 +297,7 @@ class RustTimeline( htmlBody: String?, intentionalMentions: List, ): Result = withContext(dispatcher) { - runCatching { + runCatching { val editedContent = EditedContent.RoomMessage( content = MessageEventContent.from( body = body, @@ -324,10 +325,12 @@ class RustTimeline( }, mentions = null, ) - inner.edit( - newContent = editedContent, - eventOrTransactionId = eventOrTransactionId.toRustEventOrTransactionId(), - ) + withContext(Dispatchers.IO) { + inner.edit( + newContent = editedContent, + eventOrTransactionId = eventOrTransactionId.toRustEventOrTransactionId(), + ) + } } } @@ -519,7 +522,7 @@ class RustTimeline( newContent = editedContent, eventOrTransactionId = RustEventOrTransactionId.EventId(pollStartId.value), ) - }.map { } + } } override suspend fun sendPollResponse( diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEvent.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEvent.kt index 58e148c184a..3eb6f74eaad 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEvent.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEvent.kt @@ -13,46 +13,50 @@ import io.element.android.libraries.featureflag.api.FeatureFlags import io.element.android.libraries.matrix.api.MatrixClientProvider import io.element.android.libraries.matrix.api.core.EventId import io.element.android.libraries.matrix.api.room.MatrixRoom -import io.element.android.libraries.matrix.api.sync.SyncService import io.element.android.libraries.matrix.api.timeline.MatrixTimelineItem import io.element.android.libraries.push.impl.notifications.model.NotifiableEvent import io.element.android.libraries.push.impl.notifications.model.NotifiableRingingCallEvent import io.element.android.services.appnavstate.api.AppForegroundStateService +import io.element.android.services.appnavstate.api.SyncOrchestratorProvider import kotlinx.coroutines.flow.first import kotlinx.coroutines.withContext import kotlinx.coroutines.withTimeoutOrNull -import java.util.concurrent.atomic.AtomicInteger import javax.inject.Inject import kotlin.time.Duration import kotlin.time.Duration.Companion.seconds class SyncOnNotifiableEvent @Inject constructor( private val matrixClientProvider: MatrixClientProvider, + private val syncOrchestratorProvider: SyncOrchestratorProvider, private val featureFlagService: FeatureFlagService, private val appForegroundStateService: AppForegroundStateService, private val dispatchers: CoroutineDispatchers, ) { - private var syncCounter = AtomicInteger(0) - suspend operator fun invoke(notifiableEvent: NotifiableEvent) = withContext(dispatchers.io) { val isRingingCallEvent = notifiableEvent is NotifiableRingingCallEvent if (!featureFlagService.isFeatureEnabled(FeatureFlags.SyncOnPush) && !isRingingCallEvent) { return@withContext } val client = matrixClientProvider.getOrRestore(notifiableEvent.sessionId).getOrNull() ?: return@withContext + + // Start the sync if it wasn't already started + syncOrchestratorProvider.getSyncOrchestrator(notifiableEvent.sessionId)?.start() ?: return@withContext + client.getRoom(notifiableEvent.roomId)?.use { room -> room.subscribeToSync() - // If the app is in foreground, sync is already running, so just add the subscription. + // If the app is in foreground, sync is already running, so we just add the subscription above. if (!appForegroundStateService.isInForeground.value) { - val syncService = client.syncService() - syncService.startSyncIfNeeded() if (isRingingCallEvent) { room.waitsUntilUserIsInTheCall(timeout = 60.seconds) } else { - room.waitsUntilEventIsKnown(eventId = notifiableEvent.eventId, timeout = 10.seconds) + try { + appForegroundStateService.updateIsSyncingNotificationEvent(true) + room.waitsUntilEventIsKnown(eventId = notifiableEvent.eventId, timeout = 10.seconds) + } finally { + appForegroundStateService.updateIsSyncingNotificationEvent(false) + } } - syncService.stopSyncIfNeeded() } } } @@ -81,16 +85,4 @@ class SyncOnNotifiableEvent @Inject constructor( } } } - - private suspend fun SyncService.startSyncIfNeeded() { - if (syncCounter.getAndIncrement() == 0) { - startSync() - } - } - - private suspend fun SyncService.stopSyncIfNeeded() { - if (syncCounter.decrementAndGet() == 0 && !appForegroundStateService.isInForeground.value) { - stopSync() - } - } } diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEventTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEventTest.kt index 08b14e5680a..ee5a74db437 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEventTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEventTest.kt @@ -7,6 +7,8 @@ package io.element.android.libraries.push.impl.push +import app.cash.turbine.test +import com.google.common.truth.Truth.assertThat import io.element.android.libraries.featureflag.api.FeatureFlags import io.element.android.libraries.featureflag.test.FakeFeatureFlagService import io.element.android.libraries.matrix.api.MatrixClient @@ -17,22 +19,27 @@ import io.element.android.libraries.matrix.test.A_UNIQUE_ID import io.element.android.libraries.matrix.test.FakeMatrixClient import io.element.android.libraries.matrix.test.FakeMatrixClientProvider import io.element.android.libraries.matrix.test.room.FakeMatrixRoom +import io.element.android.libraries.matrix.test.room.aRoomInfo import io.element.android.libraries.matrix.test.sync.FakeSyncService import io.element.android.libraries.matrix.test.timeline.FakeTimeline import io.element.android.libraries.matrix.test.timeline.anEventTimelineItem import io.element.android.libraries.push.impl.notifications.fixtures.aNotifiableCallEvent import io.element.android.libraries.push.impl.notifications.fixtures.aNotifiableMessageEvent +import io.element.android.services.appnavstate.api.SyncOrchestrator import io.element.android.services.appnavstate.test.FakeAppForegroundStateService import io.element.android.tests.testutils.lambda.assert import io.element.android.tests.testutils.lambda.lambdaRecorder import io.element.android.tests.testutils.testCoroutineDispatchers -import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.launch import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.advanceTimeBy import kotlinx.coroutines.test.runTest import org.junit.Test +import java.util.concurrent.atomic.AtomicBoolean +import kotlin.time.Duration.Companion.seconds class SyncOnNotifiableEventTest { private val timelineItems = MutableStateFlow>(emptyList()) @@ -73,60 +80,98 @@ class SyncOnNotifiableEventTest { assert(subscribeToSyncLambda).isNeverCalled() } + @OptIn(ExperimentalCoroutinesApi::class) @Test - fun `when feature flag is enabled, a ringing call starts and stops the sync`() = runTest { - val sut = createSyncOnNotifiableEvent(client = client, isAppInForeground = false, isSyncOnPushEnabled = true) + fun `when feature flag is enabled, a ringing call waits until the room is in 'in-call' state`() = runTest { + val appForegroundStateService = FakeAppForegroundStateService( + initialForegroundValue = false, + ) + val sut = createSyncOnNotifiableEvent(client = client, appForegroundStateService = appForegroundStateService, isSyncOnPushEnabled = true) + val unlocked = AtomicBoolean(false) + launch { + advanceTimeBy(1.seconds) + unlocked.set(true) + room.givenRoomInfo(aRoomInfo(hasRoomCall = true)) + } sut(incomingCallNotifiableEvent) - assert(startSyncLambda).isCalledOnce() - assert(stopSyncLambda).isCalledOnce() - assert(subscribeToSyncLambda).isCalledOnce() + // The process was completed before the timeout + assertThat(unlocked.get()).isTrue() } + @OptIn(ExperimentalCoroutinesApi::class) @Test - fun `when feature flag is disabled, a ringing call starts and stops the sync`() = runTest { - val sut = createSyncOnNotifiableEvent(client = client, isAppInForeground = false, isSyncOnPushEnabled = false) + fun `when feature flag is enabled, a ringing call waits until the room is in 'in-call' state or timeouts`() = runTest { + val appForegroundStateService = FakeAppForegroundStateService( + initialForegroundValue = false, + ) + val sut = createSyncOnNotifiableEvent(client = client, appForegroundStateService = appForegroundStateService, isSyncOnPushEnabled = true) + val unlocked = AtomicBoolean(false) + launch { + advanceTimeBy(120.seconds) + unlocked.set(true) + room.givenRoomInfo(aRoomInfo(hasRoomCall = true)) + } sut(incomingCallNotifiableEvent) - assert(startSyncLambda).isCalledOnce() - assert(stopSyncLambda).isCalledOnce() - assert(subscribeToSyncLambda).isCalledOnce() + // Didn't unlock before the timeout + assertThat(unlocked.get()).isFalse() } @Test fun `when feature flag is enabled and app is in foreground, sync is not started`() = runTest { - val sut = createSyncOnNotifiableEvent(client = client, isAppInForeground = true, isSyncOnPushEnabled = true) + val appForegroundStateService = FakeAppForegroundStateService( + initialForegroundValue = true, + ) + val sut = createSyncOnNotifiableEvent(client = client, appForegroundStateService = appForegroundStateService, isSyncOnPushEnabled = true) - sut(notifiableEvent) - sut(incomingCallNotifiableEvent) + appForegroundStateService.isSyncingNotificationEvent.test { + sut(notifiableEvent) + sut(incomingCallNotifiableEvent) - assert(startSyncLambda).isNeverCalled() - assert(stopSyncLambda).isNeverCalled() - assert(subscribeToSyncLambda).isCalledExactly(2) + // It's initially false + assertThat(awaitItem()).isFalse() + // It never becomes true + ensureAllEventsConsumed() + } } @Test fun `when feature flag is enabled and app is in background, sync is started and stopped`() = runTest { - val sut = createSyncOnNotifiableEvent(client = client, isAppInForeground = false, isSyncOnPushEnabled = true) + val appForegroundStateService = FakeAppForegroundStateService( + initialForegroundValue = false, + ) + val sut = createSyncOnNotifiableEvent(client = client, appForegroundStateService = appForegroundStateService, isSyncOnPushEnabled = true) timelineItems.emit( listOf(MatrixTimelineItem.Event(A_UNIQUE_ID, anEventTimelineItem())) ) - syncService.emitSyncState(SyncState.Running) - sut(notifiableEvent) - assert(startSyncLambda).isCalledOnce() - assert(stopSyncLambda).isCalledOnce() - assert(subscribeToSyncLambda).isCalledOnce() + appForegroundStateService.isSyncingNotificationEvent.test { + syncStateFlow.emitSyncState(SyncState.Running) + sut(notifiableEvent) + + // It's initially false + assertThat(awaitItem()).isFalse() + // Then it becomes true when we receive the push + assertThat(awaitItem()).isTrue() + // It becomes false once when the push is processed + assertThat(awaitItem()).isFalse() + + ensureAllEventsConsumed() + } } @Test fun `when feature flag is enabled and app is in background, running multiple time only call once`() = runTest { - val sut = createSyncOnNotifiableEvent(client = client, isAppInForeground = false, isSyncOnPushEnabled = true) + val appForegroundStateService = FakeAppForegroundStateService( + initialForegroundValue = false, + ) + val sut = createSyncOnNotifiableEvent(client = client, appForegroundStateService = appForegroundStateService, isSyncOnPushEnabled = true) - coroutineScope { + appForegroundStateService.isSyncingNotificationEvent.test { launch { sut(notifiableEvent) } launch { sut(notifiableEvent) } launch { @@ -135,32 +180,42 @@ class SyncOnNotifiableEventTest { listOf(MatrixTimelineItem.Event(A_UNIQUE_ID, anEventTimelineItem())) ) } - } - assert(startSyncLambda).isCalledOnce() - assert(stopSyncLambda).isCalledOnce() - assert(subscribeToSyncLambda).isCalledExactly(2) + // It's initially false + assertThat(awaitItem()).isFalse() + // Then it becomes true once, for the first received push + assertThat(awaitItem()).isTrue() + // It becomes false once all pushes are processed + assertThat(awaitItem()).isFalse() + + ensureAllEventsConsumed() + } } private fun TestScope.createSyncOnNotifiableEvent( client: MatrixClient = FakeMatrixClient(), isSyncOnPushEnabled: Boolean = true, - isAppInForeground: Boolean = true, + appForegroundStateService: FakeAppForegroundStateService = FakeAppForegroundStateService( + initialForegroundValue = true, + ) ): SyncOnNotifiableEvent { val featureFlagService = FakeFeatureFlagService( initialState = mapOf( FeatureFlags.SyncOnPush.key to isSyncOnPushEnabled ) ) - val appForegroundStateService = FakeAppForegroundStateService( - initialValue = isAppInForeground - ) val matrixClientProvider = FakeMatrixClientProvider { Result.success(client) } return SyncOnNotifiableEvent( matrixClientProvider = matrixClientProvider, featureFlagService = featureFlagService, appForegroundStateService = appForegroundStateService, dispatchers = testCoroutineDispatchers(), + syncOrchestratorProvider = { client -> FakeSyncOrchestrator() } ) } } + +private class FakeSyncOrchestrator : SyncOrchestrator { + override fun start() = Unit + override fun stop() = Unit +} diff --git a/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/AppForegroundStateService.kt b/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/AppForegroundStateService.kt index 97909a07cab..0455d01a132 100644 --- a/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/AppForegroundStateService.kt +++ b/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/AppForegroundStateService.kt @@ -18,8 +18,28 @@ interface AppForegroundStateService { */ val isInForeground: StateFlow + /** + * Updates to whether the app is in an active call or not will be emitted here. + */ + val isInCall: StateFlow + + /** + * Updates to whether the app is syncing a notification event or not will be emitted here. + */ + val isSyncingNotificationEvent: StateFlow + /** * Start observing the foreground state. */ - fun start() + fun startObservingForeground() + + /** + * Update the in-call state. + */ + fun updateIsInCallState(isInCall: Boolean) + + /** + * Update the active state for the syncing notification event flow. + */ + fun updateIsSyncingNotificationEvent(isSyncingNotificationEvent: Boolean) } diff --git a/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestrator.kt b/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestrator.kt new file mode 100644 index 00000000000..53e14f394b9 --- /dev/null +++ b/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestrator.kt @@ -0,0 +1,16 @@ +/* + * Copyright 2025 New Vector Ltd. + * + * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial + * Please see LICENSE files in the repository root for full details. + */ + +package io.element.android.services.appnavstate.api + +/** + * Observes the app state and network state to start/stop the sync service. + */ +interface SyncOrchestrator { + fun start() + fun stop() +} diff --git a/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestratorProvider.kt b/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestratorProvider.kt new file mode 100644 index 00000000000..e1e77946458 --- /dev/null +++ b/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestratorProvider.kt @@ -0,0 +1,20 @@ +/* + * Copyright 2025 New Vector Ltd. + * + * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial + * Please see LICENSE files in the repository root for full details. + */ + +package io.element.android.services.appnavstate.api + +import io.element.android.libraries.matrix.api.core.SessionId + +/** + * Provides a [SyncOrchestrator] for a given [SessionId]. + */ +fun interface SyncOrchestratorProvider { + /** + * Get a [SyncOrchestrator] for the given [SessionId]. + */ + fun getSyncOrchestrator(sessionId: SessionId): SyncOrchestrator? +} diff --git a/services/appnavstate/impl/src/main/kotlin/io/element/android/services/appnavstate/impl/DefaultAppForegroundStateService.kt b/services/appnavstate/impl/src/main/kotlin/io/element/android/services/appnavstate/impl/DefaultAppForegroundStateService.kt index 1f9d7d79ad4..dcafcc50fb8 100644 --- a/services/appnavstate/impl/src/main/kotlin/io/element/android/services/appnavstate/impl/DefaultAppForegroundStateService.kt +++ b/services/appnavstate/impl/src/main/kotlin/io/element/android/services/appnavstate/impl/DefaultAppForegroundStateService.kt @@ -12,19 +12,27 @@ import androidx.lifecycle.LifecycleEventObserver import androidx.lifecycle.ProcessLifecycleOwner import io.element.android.services.appnavstate.api.AppForegroundStateService import kotlinx.coroutines.flow.MutableStateFlow -import kotlinx.coroutines.flow.StateFlow class DefaultAppForegroundStateService : AppForegroundStateService { - private val state = MutableStateFlow(false) - override val isInForeground: StateFlow = state + override val isInForeground = MutableStateFlow(false) + override val isInCall = MutableStateFlow(false) + override val isSyncingNotificationEvent = MutableStateFlow(false) private val appLifecycle: Lifecycle by lazy { ProcessLifecycleOwner.get().lifecycle } - override fun start() { + override fun startObservingForeground() { appLifecycle.addObserver(lifecycleObserver) } - private val lifecycleObserver = LifecycleEventObserver { _, _ -> state.value = getCurrentState() } + override fun updateIsInCallState(isInCall: Boolean) { + this.isInCall.value = isInCall + } + + override fun updateIsSyncingNotificationEvent(isSyncingNotificationEvent: Boolean) { + this.isSyncingNotificationEvent.value = isSyncingNotificationEvent + } + + private val lifecycleObserver = LifecycleEventObserver { _, _ -> isInForeground.value = getCurrentState() } private fun getCurrentState(): Boolean = appLifecycle.currentState.isAtLeast(Lifecycle.State.STARTED) } diff --git a/services/appnavstate/impl/src/main/kotlin/io/element/android/services/appnavstate/impl/DefaultAppNavigationStateService.kt b/services/appnavstate/impl/src/main/kotlin/io/element/android/services/appnavstate/impl/DefaultAppNavigationStateService.kt index d6a1bd735bf..f173812fed0 100644 --- a/services/appnavstate/impl/src/main/kotlin/io/element/android/services/appnavstate/impl/DefaultAppNavigationStateService.kt +++ b/services/appnavstate/impl/src/main/kotlin/io/element/android/services/appnavstate/impl/DefaultAppNavigationStateService.kt @@ -48,7 +48,7 @@ class DefaultAppNavigationStateService @Inject constructor( init { coroutineScope.launch { - appForegroundStateService.start() + appForegroundStateService.startObservingForeground() appForegroundStateService.isInForeground.collect { isInForeground -> state.getAndUpdate { it.copy(isInForeground = isInForeground) } } diff --git a/services/appnavstate/test/src/main/kotlin/io/element/android/services/appnavstate/test/FakeAppForegroundStateService.kt b/services/appnavstate/test/src/main/kotlin/io/element/android/services/appnavstate/test/FakeAppForegroundStateService.kt index 627a355f0a3..ad39e4b6de3 100644 --- a/services/appnavstate/test/src/main/kotlin/io/element/android/services/appnavstate/test/FakeAppForegroundStateService.kt +++ b/services/appnavstate/test/src/main/kotlin/io/element/android/services/appnavstate/test/FakeAppForegroundStateService.kt @@ -9,19 +9,29 @@ package io.element.android.services.appnavstate.test import io.element.android.services.appnavstate.api.AppForegroundStateService import kotlinx.coroutines.flow.MutableStateFlow -import kotlinx.coroutines.flow.StateFlow class FakeAppForegroundStateService( - initialValue: Boolean = true, + initialForegroundValue: Boolean = true, + initialIsInCallValue: Boolean = false, + initialIsSyncingNotificationEventValue: Boolean = false ) : AppForegroundStateService { - private val state = MutableStateFlow(initialValue) - override val isInForeground: StateFlow = state + override val isInForeground = MutableStateFlow(initialForegroundValue) + override val isInCall = MutableStateFlow(initialIsInCallValue) + override val isSyncingNotificationEvent = MutableStateFlow(initialIsSyncingNotificationEventValue) - override fun start() { + override fun startObservingForeground() { // No-op } fun givenIsInForeground(isInForeground: Boolean) { - state.value = isInForeground + this.isInForeground.value = isInForeground + } + + override fun updateIsInCallState(isInCall: Boolean) { + this.isInCall.value = isInCall + } + + override fun updateIsSyncingNotificationEvent(isSyncingNotificationEvent: Boolean) { + this.isSyncingNotificationEvent.value = isSyncingNotificationEvent } } From 64f5a3063e3acbf176e574588ed631d41c617983 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Wed, 5 Feb 2025 10:02:35 +0100 Subject: [PATCH 02/15] Fix errors after rebase --- .../appnav/DefaultSyncOrchestratorTest.kt | 35 +++++++------------ .../call/ui/CallScreenPresenterTest.kt | 28 --------------- .../impl/push/SyncOnNotifiableEventTest.kt | 2 +- 3 files changed, 14 insertions(+), 51 deletions(-) diff --git a/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt index 46354576373..bcfeda5cdff 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt @@ -20,7 +20,6 @@ import io.element.android.tests.testutils.testCoroutineDispatchers import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.SupervisorJob -import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.advanceTimeBy import kotlinx.coroutines.test.runTest @@ -36,9 +35,8 @@ class DefaultSyncOrchestratorTest { @Test fun `when the sync wasn't running before, an initial sync will always take place, even with no network`() = runTest { - val stateFlow = MutableStateFlow(SyncState.Idle) val startSyncRecorder = lambdaRecorder> { Result.success(Unit) } - val syncService = FakeSyncService(syncStateFlow = stateFlow).apply { + val syncService = FakeSyncService(initialSyncState = SyncState.Idle).apply { startSyncLambda = startSyncRecorder } val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Offline) @@ -59,9 +57,8 @@ class DefaultSyncOrchestratorTest { @Test fun `when the app goes to background and the sync was running, it will be stopped after a delay`() = runTest { - val stateFlow = MutableStateFlow(SyncState.Running) val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } - val syncService = FakeSyncService(syncStateFlow = stateFlow).apply { + val syncService = FakeSyncService(initialSyncState = SyncState.Running).apply { stopSyncLambda = stopSyncRecorder } val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Online) @@ -91,10 +88,9 @@ class DefaultSyncOrchestratorTest { @Test fun `when the app was in background and we receive a notification, a sync will be started then stopped`() = runTest { - val stateFlow = MutableStateFlow(SyncState.Running) val startSyncRecorder = lambdaRecorder> { Result.success(Unit) } val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } - val syncService = FakeSyncService(syncStateFlow = stateFlow).apply { + val syncService = FakeSyncService(initialSyncState = SyncState.Running).apply { startSyncLambda = startSyncRecorder stopSyncLambda = stopSyncRecorder } @@ -115,7 +111,7 @@ class DefaultSyncOrchestratorTest { startSyncRecorder.assertions().isNeverCalled() // We stop the ongoing sync, give the sync service some time to stop - stateFlow.value = SyncState.Idle + syncService.emitSyncState(SyncState.Idle) advanceTimeBy(10.seconds) stopSyncRecorder.assertions().isCalledOnce() @@ -127,7 +123,7 @@ class DefaultSyncOrchestratorTest { startSyncRecorder.assertions().isCalledOnce() // If the sync is running and we mark the notification sync as no longer necessary, the sync stops after a delay - stateFlow.value = SyncState.Running + syncService.emitSyncState(SyncState.Running) appForegroundStateService.updateIsSyncingNotificationEvent(false) advanceTimeBy(10.seconds) @@ -139,10 +135,9 @@ class DefaultSyncOrchestratorTest { @Test fun `when the app was in background and we join a call, a sync will be started`() = runTest { - val stateFlow = MutableStateFlow(SyncState.Running) val startSyncRecorder = lambdaRecorder> { Result.success(Unit) } val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } - val syncService = FakeSyncService(syncStateFlow = stateFlow).apply { + val syncService = FakeSyncService(initialSyncState = SyncState.Running).apply { startSyncLambda = startSyncRecorder stopSyncLambda = stopSyncRecorder } @@ -163,7 +158,7 @@ class DefaultSyncOrchestratorTest { startSyncRecorder.assertions().isNeverCalled() // We stop the ongoing sync, give the sync service some time to stop - stateFlow.value = SyncState.Idle + syncService.emitSyncState(SyncState.Idle) advanceTimeBy(10.seconds) stopSyncRecorder.assertions().isCalledOnce() @@ -175,7 +170,7 @@ class DefaultSyncOrchestratorTest { startSyncRecorder.assertions().isCalledOnce() // If the sync is running and we mark the in-call state as false, the sync stops after a delay - stateFlow.value = SyncState.Running + syncService.emitSyncState(SyncState.Running) appForegroundStateService.updateIsInCallState(false) advanceTimeBy(10.seconds) @@ -187,10 +182,9 @@ class DefaultSyncOrchestratorTest { @Test fun `when the app is in foreground, we sync for a notification and a call is ongoing, the sync will only stop when all conditions are false`() = runTest { - val stateFlow = MutableStateFlow(SyncState.Running) val startSyncRecorder = lambdaRecorder> { Result.success(Unit) } val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } - val syncService = FakeSyncService(syncStateFlow = stateFlow).apply { + val syncService = FakeSyncService(initialSyncState = SyncState.Running).apply { startSyncLambda = startSyncRecorder stopSyncLambda = stopSyncRecorder } @@ -232,9 +226,8 @@ class DefaultSyncOrchestratorTest { @Test fun `if the sync was running, it's set to be stopped but something triggers a sync again, the sync is not stopped`() = runTest { - val stateFlow = MutableStateFlow(SyncState.Running) val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } - val syncService = FakeSyncService(syncStateFlow = stateFlow).apply { + val syncService = FakeSyncService(initialSyncState = SyncState.Running).apply { stopSyncLambda = stopSyncRecorder } val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Online) @@ -266,9 +259,8 @@ class DefaultSyncOrchestratorTest { @Test fun `when network is offline, sync service should not start`() = runTest { - val stateFlow = MutableStateFlow(SyncState.Running) val startSyncRecorder = lambdaRecorder> { Result.success(Unit) } - val syncService = FakeSyncService(syncStateFlow = stateFlow).apply { + val syncService = FakeSyncService(initialSyncState = SyncState.Running).apply { startSyncLambda = startSyncRecorder } val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Offline) @@ -281,7 +273,7 @@ class DefaultSyncOrchestratorTest { advanceTimeBy(1.seconds) // Set the sync state to idle - stateFlow.value = SyncState.Idle + syncService.emitSyncState(SyncState.Idle) // This should still not trigger a sync, since there is no network advanceTimeBy(10.seconds) @@ -293,9 +285,8 @@ class DefaultSyncOrchestratorTest { @Test fun `when sync was running and network is now offline, sync service should be stopped`() = runTest { - val stateFlow = MutableStateFlow(SyncState.Running) val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } - val syncService = FakeSyncService(syncStateFlow = stateFlow).apply { + val syncService = FakeSyncService(initialSyncState = SyncState.Running).apply { stopSyncLambda = stopSyncRecorder } val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Online) diff --git a/features/call/impl/src/test/kotlin/io/element/android/features/call/ui/CallScreenPresenterTest.kt b/features/call/impl/src/test/kotlin/io/element/android/features/call/ui/CallScreenPresenterTest.kt index 2024485d4aa..fb894ac9de3 100644 --- a/features/call/impl/src/test/kotlin/io/element/android/features/call/ui/CallScreenPresenterTest.kt +++ b/features/call/impl/src/test/kotlin/io/element/android/features/call/ui/CallScreenPresenterTest.kt @@ -260,34 +260,6 @@ class CallScreenPresenterTest { screenTracker = FakeScreenTracker {}, appForegroundStateService = appForegroundStateService, ) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { - consumeItemsUntilTimeout() - - assert(startSyncLambda).isCalledOnce() - - cancelAndIgnoreRemainingEvents() - } - } - - @Test - fun `present - automatically stops the Matrix client sync on dispose`() = runTest { - val navigator = FakeCallScreenNavigator() - val widgetDriver = FakeMatrixWidgetDriver() - val stopSyncLambda = lambdaRecorder> { Result.success(Unit) } - val syncService = FakeSyncService(SyncState.Running).apply { - this.stopSyncLambda = stopSyncLambda - } - val matrixClient = FakeMatrixClient(syncService = syncService) - val presenter = createCallScreenPresenter( - callType = CallType.RoomCall(A_SESSION_ID, A_ROOM_ID), - widgetDriver = widgetDriver, - navigator = navigator, - dispatchers = testCoroutineDispatchers(useUnconfinedTestDispatcher = true), - matrixClientsProvider = FakeMatrixClientProvider(getClient = { Result.success(matrixClient) }), - screenTracker = FakeScreenTracker {}, - ) val hasRun = Mutex(true) val job = launch { moleculeFlow(RecompositionMode.Immediate) { diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEventTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEventTest.kt index ee5a74db437..620555d7519 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEventTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEventTest.kt @@ -150,7 +150,7 @@ class SyncOnNotifiableEventTest { ) appForegroundStateService.isSyncingNotificationEvent.test { - syncStateFlow.emitSyncState(SyncState.Running) + syncService.emitSyncState(SyncState.Running) sut(notifiableEvent) // It's initially false From fa41a0acc595f5976be974ac110e4ec69467d52b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Wed, 5 Feb 2025 10:02:54 +0100 Subject: [PATCH 03/15] Make network monitor return network connectivity status, not internet connectivity --- .../networkmonitor/api/NetworkMonitor.kt | 8 ++++++++ .../impl/DefaultNetworkMonitor.kt | 18 ++---------------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/features/networkmonitor/api/src/main/kotlin/io/element/android/features/networkmonitor/api/NetworkMonitor.kt b/features/networkmonitor/api/src/main/kotlin/io/element/android/features/networkmonitor/api/NetworkMonitor.kt index d9a26dd5bbf..196eb893105 100644 --- a/features/networkmonitor/api/src/main/kotlin/io/element/android/features/networkmonitor/api/NetworkMonitor.kt +++ b/features/networkmonitor/api/src/main/kotlin/io/element/android/features/networkmonitor/api/NetworkMonitor.kt @@ -9,6 +9,14 @@ package io.element.android.features.networkmonitor.api import kotlinx.coroutines.flow.StateFlow +/** + * Monitors the network status of the device, providing the current network connectivity status as a flow. + * + * **Note:** network connectivity does not imply internet connectivity. The device can be connected to a network that can't reach the homeserver. + */ interface NetworkMonitor { + /** + * A flow containing the current network connectivity status. + */ val connectivity: StateFlow } diff --git a/features/networkmonitor/impl/src/main/kotlin/io/element/android/features/networkmonitor/impl/DefaultNetworkMonitor.kt b/features/networkmonitor/impl/src/main/kotlin/io/element/android/features/networkmonitor/impl/DefaultNetworkMonitor.kt index eb0d4a7429d..37ac5b6e607 100644 --- a/features/networkmonitor/impl/src/main/kotlin/io/element/android/features/networkmonitor/impl/DefaultNetworkMonitor.kt +++ b/features/networkmonitor/impl/src/main/kotlin/io/element/android/features/networkmonitor/impl/DefaultNetworkMonitor.kt @@ -12,7 +12,6 @@ package io.element.android.features.networkmonitor.impl import android.content.Context import android.net.ConnectivityManager import android.net.Network -import android.net.NetworkCapabilities import android.net.NetworkRequest import com.squareup.anvil.annotations.ContributesBinding import io.element.android.features.networkmonitor.api.NetworkMonitor @@ -66,9 +65,7 @@ class DefaultNetworkMonitor @Inject constructor( } } trySendBlocking(connectivityManager.activeNetworkStatus()) - val request = NetworkRequest.Builder() - .addCapability(NetworkCapabilities.NET_CAPABILITY_VALIDATED) - .build() + val request = NetworkRequest.Builder().build() connectivityManager.registerNetworkCallback(request, callback) Timber.d("Subscribe") @@ -85,17 +82,6 @@ class DefaultNetworkMonitor @Inject constructor( .stateIn(appCoroutineScope, SharingStarted.WhileSubscribed(), connectivityManager.activeNetworkStatus()) private fun ConnectivityManager.activeNetworkStatus(): NetworkStatus { - return activeNetwork?.let { - getNetworkCapabilities(it)?.getNetworkStatus() - } ?: NetworkStatus.Offline - } - - private fun NetworkCapabilities.getNetworkStatus(): NetworkStatus { - val hasInternet = hasCapability(NetworkCapabilities.NET_CAPABILITY_VALIDATED) - return if (hasInternet) { - NetworkStatus.Online - } else { - NetworkStatus.Offline - } + return if (activeNetwork != null) NetworkStatus.Online else NetworkStatus.Offline } } From 4218140453045cc1389ab4d6f501d7efcc3712bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Wed, 5 Feb 2025 11:27:15 +0100 Subject: [PATCH 04/15] Don't stop the `SyncService` when network connection is lost, let it fail instead This prevents an issue when using the offline mode of the SDK, which made the wrong UI states to be shown when the `SyncState` is `Idle` (that is, after the service being manually stopped). --- .../appnav/di/DefaultSyncOrchestrator.kt | 2 +- .../appnav/DefaultSyncOrchestratorTest.kt | 27 ------------------- 2 files changed, 1 insertion(+), 28 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt index 5b5da2d8f26..ade9694e294 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt @@ -102,7 +102,7 @@ class DefaultSyncOrchestrator @AssistedInject constructor( val isNetworkAvailable = networkState == NetworkStatus.Online Timber.tag(tag).d("isAppActive=$isAppActive, isNetworkAvailable=$isNetworkAvailable") - if (syncState == SyncState.Running && (!isAppActive || !isNetworkAvailable)) { + if (syncState == SyncState.Running && !isAppActive) { // Don't stop the sync immediately, wait a bit to avoid starting/stopping the sync too often delay(3.seconds) SyncStateAction.StopSync diff --git a/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt index bcfeda5cdff..17a2ec07f58 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt @@ -283,33 +283,6 @@ class DefaultSyncOrchestratorTest { syncOrchestrator.stop() } - @Test - fun `when sync was running and network is now offline, sync service should be stopped`() = runTest { - val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } - val syncService = FakeSyncService(initialSyncState = SyncState.Running).apply { - stopSyncLambda = stopSyncRecorder - } - val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Online) - val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor) - - // We start observing, we skip the initial sync attempt since the state is running - syncOrchestrator.start() - - // Advance the time to make sure we left the initial sync behind - advanceTimeBy(1.seconds) - - // Network is now offline - networkMonitor.connectivity.value = NetworkStatus.Offline - - // This will stop the sync after some delay - stopSyncRecorder.assertions().isNeverCalled() - advanceTimeBy(10.seconds) - stopSyncRecorder.assertions().isCalledOnce() - - // Stop observing - syncOrchestrator.stop() - } - private fun TestScope.createSyncOrchestrator( syncService: FakeSyncService = FakeSyncService(), networkMonitor: FakeNetworkMonitor = FakeNetworkMonitor(), From 64abdf62ba7f04ac9bd0e4d1623eeefd6d1666c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Wed, 5 Feb 2025 11:30:10 +0100 Subject: [PATCH 05/15] Rename `NetworkStatus.Online/Offline` to `Connected/Disconnected` so they're not easily mistaken with internet connectivity instead --- .../android/appnav/di/DefaultSyncOrchestrator.kt | 2 +- .../android/appnav/loggedin/SendQueues.kt | 2 +- .../appnav/DefaultSyncOrchestratorTest.kt | 14 +++++++------- .../features/networkmonitor/api/NetworkStatus.kt | 16 ++++++++++++++-- .../networkmonitor/impl/DefaultNetworkMonitor.kt | 6 +++--- .../networkmonitor/test/FakeNetworkMonitor.kt | 2 +- 6 files changed, 27 insertions(+), 15 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt index ade9694e294..059bb7d37c5 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt @@ -99,7 +99,7 @@ class DefaultSyncOrchestrator @AssistedInject constructor( appForegroundStateService.isSyncingNotificationEvent, ) { syncState, networkState, isInForeground, isInCall, isSyncingNotificationEvent -> val isAppActive = isInForeground || isInCall || isSyncingNotificationEvent - val isNetworkAvailable = networkState == NetworkStatus.Online + val isNetworkAvailable = networkState == NetworkStatus.Connected Timber.tag(tag).d("isAppActive=$isAppActive, isNetworkAvailable=$isNetworkAvailable") if (syncState == SyncState.Running && !isAppActive) { diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/SendQueues.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/SendQueues.kt index 458e6e5c495..8e66baf8362 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/SendQueues.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/SendQueues.kt @@ -32,7 +32,7 @@ class SendQueues @Inject constructor( ) { /** * Launches the send queues retry mechanism in the given [coroutineScope]. - * Makes sure to re-enable all send queues when the network status is [NetworkStatus.Online]. + * Makes sure to re-enable all send queues when the network status is [NetworkStatus.Connected]. */ @OptIn(FlowPreview::class) fun launchIn(coroutineScope: CoroutineScope) { diff --git a/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt index 17a2ec07f58..7b8d479e3a9 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt @@ -39,7 +39,7 @@ class DefaultSyncOrchestratorTest { val syncService = FakeSyncService(initialSyncState = SyncState.Idle).apply { startSyncLambda = startSyncRecorder } - val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Offline) + val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Disconnected) val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor) // We start observing @@ -61,7 +61,7 @@ class DefaultSyncOrchestratorTest { val syncService = FakeSyncService(initialSyncState = SyncState.Running).apply { stopSyncLambda = stopSyncRecorder } - val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Online) + val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Connected) val appForegroundStateService = FakeAppForegroundStateService(initialForegroundValue = true) val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor, appForegroundStateService) @@ -94,7 +94,7 @@ class DefaultSyncOrchestratorTest { startSyncLambda = startSyncRecorder stopSyncLambda = stopSyncRecorder } - val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Online) + val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Connected) val appForegroundStateService = FakeAppForegroundStateService( initialForegroundValue = false, initialIsSyncingNotificationEventValue = false, @@ -141,7 +141,7 @@ class DefaultSyncOrchestratorTest { startSyncLambda = startSyncRecorder stopSyncLambda = stopSyncRecorder } - val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Online) + val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Connected) val appForegroundStateService = FakeAppForegroundStateService( initialForegroundValue = false, initialIsSyncingNotificationEventValue = false, @@ -188,7 +188,7 @@ class DefaultSyncOrchestratorTest { startSyncLambda = startSyncRecorder stopSyncLambda = stopSyncRecorder } - val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Online) + val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Connected) val appForegroundStateService = FakeAppForegroundStateService( initialForegroundValue = true, initialIsSyncingNotificationEventValue = true, @@ -230,7 +230,7 @@ class DefaultSyncOrchestratorTest { val syncService = FakeSyncService(initialSyncState = SyncState.Running).apply { stopSyncLambda = stopSyncRecorder } - val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Online) + val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Connected) val appForegroundStateService = FakeAppForegroundStateService( initialForegroundValue = true, initialIsSyncingNotificationEventValue = true, @@ -263,7 +263,7 @@ class DefaultSyncOrchestratorTest { val syncService = FakeSyncService(initialSyncState = SyncState.Running).apply { startSyncLambda = startSyncRecorder } - val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Offline) + val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Disconnected) val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor) // We start observing, we skip the initial sync attempt since the state is running diff --git a/features/networkmonitor/api/src/main/kotlin/io/element/android/features/networkmonitor/api/NetworkStatus.kt b/features/networkmonitor/api/src/main/kotlin/io/element/android/features/networkmonitor/api/NetworkStatus.kt index 59deb7a8d0a..aebcb5a1d13 100644 --- a/features/networkmonitor/api/src/main/kotlin/io/element/android/features/networkmonitor/api/NetworkStatus.kt +++ b/features/networkmonitor/api/src/main/kotlin/io/element/android/features/networkmonitor/api/NetworkStatus.kt @@ -7,7 +7,19 @@ package io.element.android.features.networkmonitor.api +/** + * Network connectivity status of the device. + * + * **Note:** this is *network* connectivity status, not *internet* connectivity status. + */ enum class NetworkStatus { - Online, - Offline + /** + * The device is connected to a network. + */ + Connected, + + /** + * The device is not connected to any networks. + */ + Disconnected } diff --git a/features/networkmonitor/impl/src/main/kotlin/io/element/android/features/networkmonitor/impl/DefaultNetworkMonitor.kt b/features/networkmonitor/impl/src/main/kotlin/io/element/android/features/networkmonitor/impl/DefaultNetworkMonitor.kt index 37ac5b6e607..6d6952ab141 100644 --- a/features/networkmonitor/impl/src/main/kotlin/io/element/android/features/networkmonitor/impl/DefaultNetworkMonitor.kt +++ b/features/networkmonitor/impl/src/main/kotlin/io/element/android/features/networkmonitor/impl/DefaultNetworkMonitor.kt @@ -54,13 +54,13 @@ class DefaultNetworkMonitor @Inject constructor( override fun onLost(network: Network) { if (activeNetworksCount.decrementAndGet() == 0) { - trySendBlocking(NetworkStatus.Offline) + trySendBlocking(NetworkStatus.Disconnected) } } override fun onAvailable(network: Network) { if (activeNetworksCount.incrementAndGet() > 0) { - trySendBlocking(NetworkStatus.Online) + trySendBlocking(NetworkStatus.Connected) } } } @@ -82,6 +82,6 @@ class DefaultNetworkMonitor @Inject constructor( .stateIn(appCoroutineScope, SharingStarted.WhileSubscribed(), connectivityManager.activeNetworkStatus()) private fun ConnectivityManager.activeNetworkStatus(): NetworkStatus { - return if (activeNetwork != null) NetworkStatus.Online else NetworkStatus.Offline + return if (activeNetwork != null) NetworkStatus.Connected else NetworkStatus.Disconnected } } diff --git a/features/networkmonitor/test/src/main/kotlin/io/element/android/features/networkmonitor/test/FakeNetworkMonitor.kt b/features/networkmonitor/test/src/main/kotlin/io/element/android/features/networkmonitor/test/FakeNetworkMonitor.kt index 0566512df72..298e06aa5bd 100644 --- a/features/networkmonitor/test/src/main/kotlin/io/element/android/features/networkmonitor/test/FakeNetworkMonitor.kt +++ b/features/networkmonitor/test/src/main/kotlin/io/element/android/features/networkmonitor/test/FakeNetworkMonitor.kt @@ -11,6 +11,6 @@ import io.element.android.features.networkmonitor.api.NetworkMonitor import io.element.android.features.networkmonitor.api.NetworkStatus import kotlinx.coroutines.flow.MutableStateFlow -class FakeNetworkMonitor(initialStatus: NetworkStatus = NetworkStatus.Online) : NetworkMonitor { +class FakeNetworkMonitor(initialStatus: NetworkStatus = NetworkStatus.Connected) : NetworkMonitor { override val connectivity = MutableStateFlow(initialStatus) } From 68aa4a4c09fc38d6d932eb14240d4ea6f8d973f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Wed, 5 Feb 2025 13:24:23 +0100 Subject: [PATCH 06/15] Remove `DefaultSyncOrchestrator.stop()` --- .../appnav/di/DefaultSyncOrchestrator.kt | 22 +----- .../appnav/DefaultSyncOrchestratorTest.kt | 72 +++++++++++++++---- 2 files changed, 60 insertions(+), 34 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt index 059bb7d37c5..bdcf1e4376d 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt @@ -20,7 +20,6 @@ import io.element.android.services.appnavstate.api.SyncOrchestrator import kotlinx.coroutines.CoroutineName import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.FlowPreview -import kotlinx.coroutines.cancel import kotlinx.coroutines.delay import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.debounce @@ -49,10 +48,10 @@ class DefaultSyncOrchestrator @AssistedInject constructor( private val initialSyncMutex = Mutex() - private var coroutineScope: CoroutineScope? = null - private val tag = "SyncOrchestrator" + private val coroutineScope = CoroutineScope(baseCoroutineScope.coroutineContext + CoroutineName(tag) + dispatchers.io) + private val started = AtomicBoolean(false) /** @@ -84,9 +83,7 @@ class DefaultSyncOrchestrator @AssistedInject constructor( } } - coroutineScope = CoroutineScope(baseCoroutineScope.coroutineContext + CoroutineName(tag) + dispatchers.io) - - coroutineScope?.launch { + coroutineScope.launch { // Wait until the initial sync is done, either successfully or failing initialSyncMutex.lock() @@ -126,19 +123,6 @@ class DefaultSyncOrchestrator @AssistedInject constructor( } } } - - /** - * Stop observing the app state and network state. - */ - override fun stop() { - if (!started.compareAndSet(true, false)) { - Timber.tag(tag).d("already stopped, exiting early") - return - } - Timber.tag(tag).d("stop observing the app and network state") - coroutineScope?.cancel() - coroutineScope = null - } } private enum class SyncStateAction { diff --git a/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt index 7b8d479e3a9..f14a4f114e5 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt @@ -20,6 +20,7 @@ import io.element.android.tests.testutils.testCoroutineDispatchers import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.advanceTimeBy import kotlinx.coroutines.test.runTest @@ -40,7 +41,12 @@ class DefaultSyncOrchestratorTest { startSyncLambda = startSyncRecorder } val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Disconnected) - val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor) + val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) + val syncOrchestrator = createSyncOrchestrator( + syncService = syncService, + networkMonitor = networkMonitor, + baseCoroutineScope = coroutineScope + ) // We start observing syncOrchestrator.start() @@ -52,7 +58,7 @@ class DefaultSyncOrchestratorTest { startSyncRecorder.assertions().isCalledOnce() // Stop observing - syncOrchestrator.stop() + coroutineScope.cancel() } @Test @@ -63,7 +69,13 @@ class DefaultSyncOrchestratorTest { } val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Connected) val appForegroundStateService = FakeAppForegroundStateService(initialForegroundValue = true) - val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor, appForegroundStateService) + val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) + val syncOrchestrator = createSyncOrchestrator( + syncService = syncService, + networkMonitor = networkMonitor, + appForegroundStateService = appForegroundStateService, + baseCoroutineScope = coroutineScope + ) // We start observing, we skip the initial sync attempt since the state is running syncOrchestrator.start() @@ -83,7 +95,7 @@ class DefaultSyncOrchestratorTest { stopSyncRecorder.assertions().isCalledOnce() // Stop observing - syncOrchestrator.stop() + coroutineScope.cancel() } @Test @@ -99,7 +111,13 @@ class DefaultSyncOrchestratorTest { initialForegroundValue = false, initialIsSyncingNotificationEventValue = false, ) - val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor, appForegroundStateService) + val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) + val syncOrchestrator = createSyncOrchestrator( + syncService = syncService, + networkMonitor = networkMonitor, + appForegroundStateService = appForegroundStateService, + baseCoroutineScope = coroutineScope + ) // We start observing, we skip the initial sync attempt since the state is running syncOrchestrator.start() @@ -130,7 +148,7 @@ class DefaultSyncOrchestratorTest { stopSyncRecorder.assertions().isCalledExactly(2) // Stop observing - syncOrchestrator.stop() + coroutineScope.cancel() } @Test @@ -146,7 +164,13 @@ class DefaultSyncOrchestratorTest { initialForegroundValue = false, initialIsSyncingNotificationEventValue = false, ) - val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor, appForegroundStateService) + val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) + val syncOrchestrator = createSyncOrchestrator( + syncService = syncService, + networkMonitor = networkMonitor, + appForegroundStateService = appForegroundStateService, + baseCoroutineScope = coroutineScope + ) // We start observing, we skip the initial sync attempt since the state is running syncOrchestrator.start() @@ -177,7 +201,7 @@ class DefaultSyncOrchestratorTest { stopSyncRecorder.assertions().isCalledExactly(2) // Stop observing - syncOrchestrator.stop() + coroutineScope.cancel() } @Test @@ -194,7 +218,13 @@ class DefaultSyncOrchestratorTest { initialIsSyncingNotificationEventValue = true, initialIsInCallValue = true, ) - val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor, appForegroundStateService) + val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) + val syncOrchestrator = createSyncOrchestrator( + syncService = syncService, + networkMonitor = networkMonitor, + appForegroundStateService = appForegroundStateService, + baseCoroutineScope = coroutineScope + ) // We start observing, we skip the initial sync attempt since the state is running syncOrchestrator.start() @@ -221,7 +251,7 @@ class DefaultSyncOrchestratorTest { stopSyncRecorder.assertions().isCalledOnce() // Stop observing - syncOrchestrator.stop() + coroutineScope.cancel() } @Test @@ -236,7 +266,13 @@ class DefaultSyncOrchestratorTest { initialIsSyncingNotificationEventValue = true, initialIsInCallValue = true, ) - val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor, appForegroundStateService) + val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) + val syncOrchestrator = createSyncOrchestrator( + syncService = syncService, + networkMonitor = networkMonitor, + appForegroundStateService = appForegroundStateService, + baseCoroutineScope = coroutineScope + ) // We start observing, we skip the initial sync attempt since the state is running syncOrchestrator.start() @@ -254,7 +290,7 @@ class DefaultSyncOrchestratorTest { stopSyncRecorder.assertions().isNeverCalled() // Stop observing - syncOrchestrator.stop() + coroutineScope.cancel() } @Test @@ -264,7 +300,12 @@ class DefaultSyncOrchestratorTest { startSyncLambda = startSyncRecorder } val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Disconnected) - val syncOrchestrator = createSyncOrchestrator(syncService, networkMonitor) + val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) + val syncOrchestrator = createSyncOrchestrator( + syncService = syncService, + networkMonitor = networkMonitor, + baseCoroutineScope = coroutineScope + ) // We start observing, we skip the initial sync attempt since the state is running syncOrchestrator.start() @@ -280,18 +321,19 @@ class DefaultSyncOrchestratorTest { startSyncRecorder.assertions().isNeverCalled() // Stop observing - syncOrchestrator.stop() + coroutineScope.cancel() } private fun TestScope.createSyncOrchestrator( syncService: FakeSyncService = FakeSyncService(), networkMonitor: FakeNetworkMonitor = FakeNetworkMonitor(), appForegroundStateService: FakeAppForegroundStateService = FakeAppForegroundStateService(), + baseCoroutineScope: CoroutineScope = CoroutineScope(coroutineContext + SupervisorJob()), ) = DefaultSyncOrchestrator( matrixClient = FakeMatrixClient(syncService = syncService), networkMonitor = networkMonitor, appForegroundStateService = appForegroundStateService, - baseCoroutineScope = CoroutineScope(coroutineContext + SupervisorJob()), + baseCoroutineScope = baseCoroutineScope, dispatchers = testCoroutineDispatchers(), ) } From d1f224004cfdd11dc327337a900b008664e94cd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Wed, 5 Feb 2025 13:25:10 +0100 Subject: [PATCH 07/15] Start the SyncOrchestrator as soon as the Client is created --- .../android/appnav/LoggedInFlowNode.kt | 4 -- .../android/appnav/di/MatrixSessionCache.kt | 26 +++++++------ .../appnav/di/MatrixSessionCacheTest.kt | 39 +++++++++++++++---- .../push/impl/push/SyncOnNotifiableEvent.kt | 5 --- .../impl/push/SyncOnNotifiableEventTest.kt | 7 ---- .../appnavstate/api/SyncOrchestrator.kt | 1 - 6 files changed, 46 insertions(+), 36 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt index ea1a095cfaa..1d3c78b88a3 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt @@ -77,7 +77,6 @@ import io.element.android.libraries.matrix.api.verification.SessionVerificationR import io.element.android.libraries.matrix.api.verification.SessionVerificationServiceListener import io.element.android.libraries.preferences.api.store.EnableNativeSlidingSyncUseCase import io.element.android.services.appnavstate.api.AppNavigationStateService -import io.element.android.services.appnavstate.api.SyncOrchestratorProvider import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach @@ -107,7 +106,6 @@ class LoggedInFlowNode @AssistedInject constructor( private val logoutEntryPoint: LogoutEntryPoint, private val incomingVerificationEntryPoint: IncomingVerificationEntryPoint, private val enableNativeSlidingSyncUseCase: EnableNativeSlidingSyncUseCase, - private val syncOrchestratorProvider: SyncOrchestratorProvider, snackbarDispatcher: SnackbarDispatcher, ) : BaseFlowNode( backstack = BackStack( @@ -139,8 +137,6 @@ class LoggedInFlowNode @AssistedInject constructor( override fun onBuilt() { super.onBuilt() - syncOrchestratorProvider.getSyncOrchestrator(sessionId = matrixClient.sessionId)?.start() - lifecycle.subscribe( onCreate = { appNavigationStateService.onNavigateToSession(id, matrixClient.sessionId) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt index 04a94b74488..2f37f13c57b 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt @@ -39,28 +39,30 @@ class MatrixSessionCache @Inject constructor( private val authenticationService: MatrixAuthenticationService, private val syncOrchestratorFactory: DefaultSyncOrchestrator.Factory, ) : MatrixClientProvider, SyncOrchestratorProvider { - private val sessionIdsToMatrixClient = ConcurrentHashMap() + private val sessionIdsToMatrixSession = ConcurrentHashMap() private val restoreMutex = Mutex() init { authenticationService.listenToNewMatrixClients { matrixClient -> - sessionIdsToMatrixClient[matrixClient.sessionId] = InMemoryMatrixSession( + val syncOrchestrator = syncOrchestratorFactory.create(matrixClient) + sessionIdsToMatrixSession[matrixClient.sessionId] = InMemoryMatrixSession( matrixClient = matrixClient, - syncOrchestrator = syncOrchestratorFactory.create(matrixClient) + syncOrchestrator = syncOrchestrator, ) + syncOrchestrator.start() } } fun removeAll() { - sessionIdsToMatrixClient.clear() + sessionIdsToMatrixSession.clear() } fun remove(sessionId: SessionId) { - sessionIdsToMatrixClient.remove(sessionId) + sessionIdsToMatrixSession.remove(sessionId) } override fun getOrNull(sessionId: SessionId): MatrixClient? { - return sessionIdsToMatrixClient[sessionId]?.matrixClient + return sessionIdsToMatrixSession[sessionId]?.matrixClient } override suspend fun getOrRestore(sessionId: SessionId): Result { @@ -73,13 +75,13 @@ class MatrixSessionCache @Inject constructor( } override fun getSyncOrchestrator(sessionId: SessionId): SyncOrchestrator? { - return sessionIdsToMatrixClient[sessionId]?.syncOrchestrator + return sessionIdsToMatrixSession[sessionId]?.syncOrchestrator } @Suppress("UNCHECKED_CAST") fun restoreWithSavedState(state: SavedStateMap?) { Timber.d("Restore state") - if (state == null || sessionIdsToMatrixClient.isNotEmpty()) { + if (state == null || sessionIdsToMatrixSession.isNotEmpty()) { Timber.w("Restore with non-empty map") return } @@ -95,7 +97,7 @@ class MatrixSessionCache @Inject constructor( } fun saveIntoSavedState(state: MutableSavedStateMap) { - val sessionKeys = sessionIdsToMatrixClient.keys.toTypedArray() + val sessionKeys = sessionIdsToMatrixSession.keys.toTypedArray() Timber.d("Save matrix session keys = ${sessionKeys.map { it.value }}") state[SAVE_INSTANCE_KEY] = sessionKeys } @@ -104,10 +106,12 @@ class MatrixSessionCache @Inject constructor( Timber.d("Restore matrix session: $sessionId") return authenticationService.restoreSession(sessionId) .onSuccess { matrixClient -> - sessionIdsToMatrixClient[matrixClient.sessionId] = InMemoryMatrixSession( + val syncOrchestrator = syncOrchestratorFactory.create(matrixClient) + sessionIdsToMatrixSession[matrixClient.sessionId] = InMemoryMatrixSession( matrixClient = matrixClient, - syncOrchestrator = syncOrchestratorFactory.create(matrixClient) + syncOrchestrator = syncOrchestrator, ) + syncOrchestrator.start() } .onFailure { Timber.e(it, "Fail to restore session") diff --git a/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt index d68500b7bde..0437ed09068 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt @@ -16,7 +16,10 @@ import io.element.android.libraries.matrix.test.FakeMatrixClient import io.element.android.libraries.matrix.test.auth.FakeMatrixAuthenticationService import io.element.android.services.appnavstate.test.FakeAppForegroundStateService import io.element.android.tests.testutils.testCoroutineDispatchers +import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.runTest import org.junit.Test @@ -32,8 +35,9 @@ class MatrixSessionCacheTest { @OptIn(ExperimentalCoroutinesApi::class) @Test fun `test getSyncOrchestratorOrNull`() = runTest { + val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory(coroutineScope)) // With no matrix client there is no sync orchestrator assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() @@ -44,12 +48,15 @@ class MatrixSessionCacheTest { fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) assertThat(matrixSessionCache.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) assertThat(matrixSessionCache.getSyncOrchestrator(A_SESSION_ID)).isNotNull() + + coroutineScope.cancel() } @Test fun `test getOrRestore`() = runTest { + val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory(coroutineScope)) val fakeMatrixClient = FakeMatrixClient() fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() @@ -57,12 +64,15 @@ class MatrixSessionCacheTest { // Do it again to hit the cache assertThat(matrixSessionCache.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isEqualTo(fakeMatrixClient) + + coroutineScope.cancel() } @Test fun `test remove`() = runTest { + val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory(coroutineScope)) val fakeMatrixClient = FakeMatrixClient() fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) assertThat(matrixSessionCache.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) @@ -70,12 +80,15 @@ class MatrixSessionCacheTest { // Remove matrixSessionCache.remove(A_SESSION_ID) assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() + + coroutineScope.cancel() } @Test fun `test remove all`() = runTest { + val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory(coroutineScope)) val fakeMatrixClient = FakeMatrixClient() fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) assertThat(matrixSessionCache.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) @@ -83,12 +96,15 @@ class MatrixSessionCacheTest { // Remove all matrixSessionCache.removeAll() assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() + + coroutineScope.cancel() } @Test fun `test save and restore`() = runTest { + val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory(coroutineScope)) val fakeMatrixClient = FakeMatrixClient() fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) matrixSessionCache.getOrRestore(A_SESSION_ID) @@ -103,12 +119,15 @@ class MatrixSessionCacheTest { // Restore again matrixSessionCache.restoreWithSavedState(savedStateMap) assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isEqualTo(fakeMatrixClient) + + coroutineScope.cancel() } @Test fun `test AuthenticationService listenToNewMatrixClients emits a Client value and we save it`() = runTest { + val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory(coroutineScope)) assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() fakeAuthenticationService.givenMatrixClient(FakeMatrixClient(sessionId = A_SESSION_ID)) @@ -116,13 +135,17 @@ class MatrixSessionCacheTest { assertThat(loginSucceeded.isSuccess).isTrue() assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNotNull() + + coroutineScope.cancel() } - private fun TestScope.createSyncOrchestratorFactory() = object : DefaultSyncOrchestrator.Factory { + private fun TestScope.createSyncOrchestratorFactory( + baseCoroutineScope: CoroutineScope = this, + ) = object : DefaultSyncOrchestrator.Factory { override fun create(matrixClient: MatrixClient): DefaultSyncOrchestrator { return DefaultSyncOrchestrator( matrixClient, - baseCoroutineScope = this@createSyncOrchestratorFactory, + baseCoroutineScope = baseCoroutineScope, appForegroundStateService = FakeAppForegroundStateService(), networkMonitor = FakeNetworkMonitor(), dispatchers = testCoroutineDispatchers(), diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEvent.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEvent.kt index 3eb6f74eaad..8d1d866edd5 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEvent.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEvent.kt @@ -17,7 +17,6 @@ import io.element.android.libraries.matrix.api.timeline.MatrixTimelineItem import io.element.android.libraries.push.impl.notifications.model.NotifiableEvent import io.element.android.libraries.push.impl.notifications.model.NotifiableRingingCallEvent import io.element.android.services.appnavstate.api.AppForegroundStateService -import io.element.android.services.appnavstate.api.SyncOrchestratorProvider import kotlinx.coroutines.flow.first import kotlinx.coroutines.withContext import kotlinx.coroutines.withTimeoutOrNull @@ -27,7 +26,6 @@ import kotlin.time.Duration.Companion.seconds class SyncOnNotifiableEvent @Inject constructor( private val matrixClientProvider: MatrixClientProvider, - private val syncOrchestratorProvider: SyncOrchestratorProvider, private val featureFlagService: FeatureFlagService, private val appForegroundStateService: AppForegroundStateService, private val dispatchers: CoroutineDispatchers, @@ -39,9 +37,6 @@ class SyncOnNotifiableEvent @Inject constructor( } val client = matrixClientProvider.getOrRestore(notifiableEvent.sessionId).getOrNull() ?: return@withContext - // Start the sync if it wasn't already started - syncOrchestratorProvider.getSyncOrchestrator(notifiableEvent.sessionId)?.start() ?: return@withContext - client.getRoom(notifiableEvent.roomId)?.use { room -> room.subscribeToSync() diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEventTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEventTest.kt index 620555d7519..35dddbce6fe 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEventTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/push/SyncOnNotifiableEventTest.kt @@ -25,7 +25,6 @@ import io.element.android.libraries.matrix.test.timeline.FakeTimeline import io.element.android.libraries.matrix.test.timeline.anEventTimelineItem import io.element.android.libraries.push.impl.notifications.fixtures.aNotifiableCallEvent import io.element.android.libraries.push.impl.notifications.fixtures.aNotifiableMessageEvent -import io.element.android.services.appnavstate.api.SyncOrchestrator import io.element.android.services.appnavstate.test.FakeAppForegroundStateService import io.element.android.tests.testutils.lambda.assert import io.element.android.tests.testutils.lambda.lambdaRecorder @@ -210,12 +209,6 @@ class SyncOnNotifiableEventTest { featureFlagService = featureFlagService, appForegroundStateService = appForegroundStateService, dispatchers = testCoroutineDispatchers(), - syncOrchestratorProvider = { client -> FakeSyncOrchestrator() } ) } } - -private class FakeSyncOrchestrator : SyncOrchestrator { - override fun start() = Unit - override fun stop() = Unit -} diff --git a/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestrator.kt b/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestrator.kt index 53e14f394b9..f4d3978daa9 100644 --- a/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestrator.kt +++ b/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestrator.kt @@ -12,5 +12,4 @@ package io.element.android.services.appnavstate.api */ interface SyncOrchestrator { fun start() - fun stop() } From d1afcb4c67e30367e9b83195be12e70b593545d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Wed, 5 Feb 2025 13:31:46 +0100 Subject: [PATCH 08/15] Remove the force initial sync, the orchestrator should handle that in the existing loop when offline mode is enabled for the Client in the SDK --- .../appnav/di/DefaultSyncOrchestrator.kt | 28 ++---------------- .../appnav/DefaultSyncOrchestratorTest.kt | 29 +------------------ .../appnav/di/MatrixSessionCacheTest.kt | 2 +- 3 files changed, 5 insertions(+), 54 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt index bdcf1e4376d..82e3fcc387e 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt @@ -24,9 +24,7 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.distinctUntilChanged -import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch -import kotlinx.coroutines.sync.Mutex import timber.log.Timber import java.util.concurrent.atomic.AtomicBoolean import kotlin.time.Duration.Companion.milliseconds @@ -34,10 +32,10 @@ import kotlin.time.Duration.Companion.seconds class DefaultSyncOrchestrator @AssistedInject constructor( @Assisted matrixClient: MatrixClient, - private val baseCoroutineScope: CoroutineScope = matrixClient.sessionCoroutineScope, + sessionCoroutineScope: CoroutineScope = matrixClient.sessionCoroutineScope, private val appForegroundStateService: AppForegroundStateService, private val networkMonitor: NetworkMonitor, - private val dispatchers: CoroutineDispatchers, + dispatchers: CoroutineDispatchers, ) : SyncOrchestrator { @AssistedFactory interface Factory { @@ -46,11 +44,9 @@ class DefaultSyncOrchestrator @AssistedInject constructor( private val syncService = matrixClient.syncService() - private val initialSyncMutex = Mutex() - private val tag = "SyncOrchestrator" - private val coroutineScope = CoroutineScope(baseCoroutineScope.coroutineContext + CoroutineName(tag) + dispatchers.io) + private val coroutineScope = CoroutineScope(sessionCoroutineScope.coroutineContext + CoroutineName(tag) + dispatchers.io) private val started = AtomicBoolean(false) @@ -68,25 +64,7 @@ class DefaultSyncOrchestrator @AssistedInject constructor( Timber.tag(tag).d("start observing the app and network state") - if (syncService.syncState.value != SyncState.Running) { - Timber.tag(tag).d("initial startSync") - baseCoroutineScope.launch(dispatchers.io) { - try { - initialSyncMutex.lock() - syncService.startSync() - - // Wait until it's running - syncService.syncState.first { it == SyncState.Running } - } finally { - initialSyncMutex.unlock() - } - } - } - coroutineScope.launch { - // Wait until the initial sync is done, either successfully or failing - initialSyncMutex.lock() - combine( // small debounce to avoid spamming startSync when the state is changing quickly in case of error. syncService.syncState.debounce(100.milliseconds), diff --git a/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt index f14a4f114e5..bf5fc7bd6d6 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt @@ -34,33 +34,6 @@ class DefaultSyncOrchestratorTest { @get:Rule val warmUpRule = WarmUpRule() - @Test - fun `when the sync wasn't running before, an initial sync will always take place, even with no network`() = runTest { - val startSyncRecorder = lambdaRecorder> { Result.success(Unit) } - val syncService = FakeSyncService(initialSyncState = SyncState.Idle).apply { - startSyncLambda = startSyncRecorder - } - val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Disconnected) - val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) - val syncOrchestrator = createSyncOrchestrator( - syncService = syncService, - networkMonitor = networkMonitor, - baseCoroutineScope = coroutineScope - ) - - // We start observing - syncOrchestrator.start() - - // Advance the time to make sure we left the initial sync behind - advanceTimeBy(1.seconds) - - // Start sync will be called shortly after - startSyncRecorder.assertions().isCalledOnce() - - // Stop observing - coroutineScope.cancel() - } - @Test fun `when the app goes to background and the sync was running, it will be stopped after a delay`() = runTest { val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } @@ -333,7 +306,7 @@ class DefaultSyncOrchestratorTest { matrixClient = FakeMatrixClient(syncService = syncService), networkMonitor = networkMonitor, appForegroundStateService = appForegroundStateService, - baseCoroutineScope = baseCoroutineScope, + sessionCoroutineScope = baseCoroutineScope, dispatchers = testCoroutineDispatchers(), ) } diff --git a/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt index 0437ed09068..be9bbfc06a1 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt @@ -145,7 +145,7 @@ class MatrixSessionCacheTest { override fun create(matrixClient: MatrixClient): DefaultSyncOrchestrator { return DefaultSyncOrchestrator( matrixClient, - baseCoroutineScope = baseCoroutineScope, + sessionCoroutineScope = baseCoroutineScope, appForegroundStateService = FakeAppForegroundStateService(), networkMonitor = FakeNetworkMonitor(), dispatchers = testCoroutineDispatchers(), From 27e841ab9cebc649c6a55e55df32bcd4c4aea829 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Wed, 5 Feb 2025 13:36:13 +0100 Subject: [PATCH 09/15] Remove `SyncOrchestrator` interface, rename `DefaultSyncOrchestrator` to it, remove the existing provider interface. --- .../android/appnav/di/MatrixSessionCache.kt | 11 +++++----- ...yncOrchestrator.kt => SyncOrchestrator.kt} | 9 ++++----- ...stratorTest.kt => SyncOrchestratorTest.kt} | 6 +++--- .../appnav/di/MatrixSessionCacheTest.kt | 6 +++--- .../appnavstate/api/SyncOrchestrator.kt | 15 -------------- .../api/SyncOrchestratorProvider.kt | 20 ------------------- 6 files changed, 15 insertions(+), 52 deletions(-) rename appnav/src/main/kotlin/io/element/android/appnav/di/{DefaultSyncOrchestrator.kt => SyncOrchestrator.kt} (94%) rename appnav/src/test/kotlin/io/element/android/appnav/{DefaultSyncOrchestratorTest.kt => SyncOrchestratorTest.kt} (99%) delete mode 100644 services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestrator.kt delete mode 100644 services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestratorProvider.kt diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt index 2f37f13c57b..b5d5a84c39e 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt @@ -7,6 +7,7 @@ package io.element.android.appnav.di +import androidx.annotation.VisibleForTesting import com.bumble.appyx.core.state.MutableSavedStateMap import com.bumble.appyx.core.state.SavedStateMap import com.squareup.anvil.annotations.ContributesBinding @@ -16,8 +17,6 @@ import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.MatrixClientProvider import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService import io.element.android.libraries.matrix.api.core.SessionId -import io.element.android.services.appnavstate.api.SyncOrchestrator -import io.element.android.services.appnavstate.api.SyncOrchestratorProvider import kotlinx.coroutines.runBlocking import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock @@ -34,11 +33,10 @@ private const val SAVE_INSTANCE_KEY = "io.element.android.x.di.MatrixClientsHold */ @SingleIn(AppScope::class) @ContributesBinding(AppScope::class, boundType = MatrixClientProvider::class) -@ContributesBinding(AppScope::class, boundType = SyncOrchestratorProvider::class) class MatrixSessionCache @Inject constructor( private val authenticationService: MatrixAuthenticationService, - private val syncOrchestratorFactory: DefaultSyncOrchestrator.Factory, -) : MatrixClientProvider, SyncOrchestratorProvider { + private val syncOrchestratorFactory: SyncOrchestrator.Factory, +) : MatrixClientProvider { private val sessionIdsToMatrixSession = ConcurrentHashMap() private val restoreMutex = Mutex() @@ -74,7 +72,8 @@ class MatrixSessionCache @Inject constructor( } } - override fun getSyncOrchestrator(sessionId: SessionId): SyncOrchestrator? { + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + internal fun getSyncOrchestrator(sessionId: SessionId): SyncOrchestrator? { return sessionIdsToMatrixSession[sessionId]?.syncOrchestrator } diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/SyncOrchestrator.kt similarity index 94% rename from appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt rename to appnav/src/main/kotlin/io/element/android/appnav/di/SyncOrchestrator.kt index 82e3fcc387e..edac22460fb 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/DefaultSyncOrchestrator.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/SyncOrchestrator.kt @@ -16,7 +16,6 @@ import io.element.android.libraries.core.coroutine.CoroutineDispatchers import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.sync.SyncState import io.element.android.services.appnavstate.api.AppForegroundStateService -import io.element.android.services.appnavstate.api.SyncOrchestrator import kotlinx.coroutines.CoroutineName import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.FlowPreview @@ -30,16 +29,16 @@ import java.util.concurrent.atomic.AtomicBoolean import kotlin.time.Duration.Companion.milliseconds import kotlin.time.Duration.Companion.seconds -class DefaultSyncOrchestrator @AssistedInject constructor( +class SyncOrchestrator @AssistedInject constructor( @Assisted matrixClient: MatrixClient, sessionCoroutineScope: CoroutineScope = matrixClient.sessionCoroutineScope, private val appForegroundStateService: AppForegroundStateService, private val networkMonitor: NetworkMonitor, dispatchers: CoroutineDispatchers, -) : SyncOrchestrator { +) { @AssistedFactory interface Factory { - fun create(matrixClient: MatrixClient): DefaultSyncOrchestrator + fun create(matrixClient: MatrixClient): SyncOrchestrator } private val syncService = matrixClient.syncService() @@ -56,7 +55,7 @@ class DefaultSyncOrchestrator @AssistedInject constructor( * Before observing the state, a first attempt at starting the sync service will happen if it's not already running. */ @OptIn(FlowPreview::class) - override fun start() { + fun start() { if (!started.compareAndSet(false, true)) { Timber.tag(tag).d("already started, exiting early") return diff --git a/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt similarity index 99% rename from appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt rename to appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt index bf5fc7bd6d6..89ad88138a9 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/DefaultSyncOrchestratorTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt @@ -7,7 +7,7 @@ package io.element.android.appnav -import io.element.android.appnav.di.DefaultSyncOrchestrator +import io.element.android.appnav.di.SyncOrchestrator import io.element.android.features.networkmonitor.api.NetworkStatus import io.element.android.features.networkmonitor.test.FakeNetworkMonitor import io.element.android.libraries.matrix.api.sync.SyncState @@ -30,7 +30,7 @@ import kotlin.time.Duration.Companion.milliseconds import kotlin.time.Duration.Companion.seconds @OptIn(ExperimentalCoroutinesApi::class) -class DefaultSyncOrchestratorTest { +class SyncOrchestratorTest { @get:Rule val warmUpRule = WarmUpRule() @@ -302,7 +302,7 @@ class DefaultSyncOrchestratorTest { networkMonitor: FakeNetworkMonitor = FakeNetworkMonitor(), appForegroundStateService: FakeAppForegroundStateService = FakeAppForegroundStateService(), baseCoroutineScope: CoroutineScope = CoroutineScope(coroutineContext + SupervisorJob()), - ) = DefaultSyncOrchestrator( + ) = SyncOrchestrator( matrixClient = FakeMatrixClient(syncService = syncService), networkMonitor = networkMonitor, appForegroundStateService = appForegroundStateService, diff --git a/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt index be9bbfc06a1..9192eb49a0b 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt @@ -141,9 +141,9 @@ class MatrixSessionCacheTest { private fun TestScope.createSyncOrchestratorFactory( baseCoroutineScope: CoroutineScope = this, - ) = object : DefaultSyncOrchestrator.Factory { - override fun create(matrixClient: MatrixClient): DefaultSyncOrchestrator { - return DefaultSyncOrchestrator( + ) = object : SyncOrchestrator.Factory { + override fun create(matrixClient: MatrixClient): SyncOrchestrator { + return SyncOrchestrator( matrixClient, sessionCoroutineScope = baseCoroutineScope, appForegroundStateService = FakeAppForegroundStateService(), diff --git a/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestrator.kt b/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestrator.kt deleted file mode 100644 index f4d3978daa9..00000000000 --- a/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestrator.kt +++ /dev/null @@ -1,15 +0,0 @@ -/* - * Copyright 2025 New Vector Ltd. - * - * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial - * Please see LICENSE files in the repository root for full details. - */ - -package io.element.android.services.appnavstate.api - -/** - * Observes the app state and network state to start/stop the sync service. - */ -interface SyncOrchestrator { - fun start() -} diff --git a/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestratorProvider.kt b/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestratorProvider.kt deleted file mode 100644 index e1e77946458..00000000000 --- a/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/SyncOrchestratorProvider.kt +++ /dev/null @@ -1,20 +0,0 @@ -/* - * Copyright 2025 New Vector Ltd. - * - * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial - * Please see LICENSE files in the repository root for full details. - */ - -package io.element.android.services.appnavstate.api - -import io.element.android.libraries.matrix.api.core.SessionId - -/** - * Provides a [SyncOrchestrator] for a given [SessionId]. - */ -fun interface SyncOrchestratorProvider { - /** - * Get a [SyncOrchestrator] for the given [SessionId]. - */ - fun getSyncOrchestrator(sessionId: SessionId): SyncOrchestrator? -} From b38464c05313edc6b89287b30abc4c9c307a54bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Wed, 5 Feb 2025 17:45:58 +0100 Subject: [PATCH 10/15] Use the right coroutine scope for `SyncOrchestrator`: before the app coroutine scope was being incorrectly injected instead. --- .../android/appnav/di/MatrixSessionCache.kt | 2 +- .../android/appnav/di/SyncOrchestrator.kt | 76 ++++++++++--------- .../android/appnav/SyncOrchestratorTest.kt | 37 +-------- .../appnav/di/MatrixSessionCacheTest.kt | 50 ++++-------- .../libraries/matrix/impl/RustMatrixClient.kt | 49 ++++++------ 5 files changed, 79 insertions(+), 135 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt index b5d5a84c39e..302a12d99b9 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixSessionCache.kt @@ -32,7 +32,7 @@ private const val SAVE_INSTANCE_KEY = "io.element.android.x.di.MatrixClientsHold * This component contains both the [MatrixClient] and the [SyncOrchestrator] for each session. */ @SingleIn(AppScope::class) -@ContributesBinding(AppScope::class, boundType = MatrixClientProvider::class) +@ContributesBinding(AppScope::class) class MatrixSessionCache @Inject constructor( private val authenticationService: MatrixAuthenticationService, private val syncOrchestratorFactory: SyncOrchestrator.Factory, diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/SyncOrchestrator.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/SyncOrchestrator.kt index edac22460fb..8149b847e7c 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/SyncOrchestrator.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/SyncOrchestrator.kt @@ -13,17 +13,18 @@ import dagger.assisted.AssistedInject import io.element.android.features.networkmonitor.api.NetworkMonitor import io.element.android.features.networkmonitor.api.NetworkStatus import io.element.android.libraries.core.coroutine.CoroutineDispatchers +import io.element.android.libraries.core.coroutine.childScope import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.sync.SyncState import io.element.android.services.appnavstate.api.AppForegroundStateService -import kotlinx.coroutines.CoroutineName -import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.FlowPreview import kotlinx.coroutines.delay import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.distinctUntilChanged -import kotlinx.coroutines.launch +import kotlinx.coroutines.flow.launchIn +import kotlinx.coroutines.flow.onCompletion +import kotlinx.coroutines.flow.onEach import timber.log.Timber import java.util.concurrent.atomic.AtomicBoolean import kotlin.time.Duration.Companion.milliseconds @@ -31,7 +32,6 @@ import kotlin.time.Duration.Companion.seconds class SyncOrchestrator @AssistedInject constructor( @Assisted matrixClient: MatrixClient, - sessionCoroutineScope: CoroutineScope = matrixClient.sessionCoroutineScope, private val appForegroundStateService: AppForegroundStateService, private val networkMonitor: NetworkMonitor, dispatchers: CoroutineDispatchers, @@ -45,7 +45,7 @@ class SyncOrchestrator @AssistedInject constructor( private val tag = "SyncOrchestrator" - private val coroutineScope = CoroutineScope(sessionCoroutineScope.coroutineContext + CoroutineName(tag) + dispatchers.io) + private val coroutineScope = matrixClient.sessionCoroutineScope.childScope(dispatchers.io, tag) private val started = AtomicBoolean(false) @@ -63,42 +63,44 @@ class SyncOrchestrator @AssistedInject constructor( Timber.tag(tag).d("start observing the app and network state") - coroutineScope.launch { - combine( - // small debounce to avoid spamming startSync when the state is changing quickly in case of error. - syncService.syncState.debounce(100.milliseconds), - networkMonitor.connectivity, - appForegroundStateService.isInForeground, - appForegroundStateService.isInCall, - appForegroundStateService.isSyncingNotificationEvent, - ) { syncState, networkState, isInForeground, isInCall, isSyncingNotificationEvent -> - val isAppActive = isInForeground || isInCall || isSyncingNotificationEvent - val isNetworkAvailable = networkState == NetworkStatus.Connected + combine( + // small debounce to avoid spamming startSync when the state is changing quickly in case of error. + syncService.syncState.debounce(100.milliseconds), + networkMonitor.connectivity, + appForegroundStateService.isInForeground, + appForegroundStateService.isInCall, + appForegroundStateService.isSyncingNotificationEvent, + ) { syncState, networkState, isInForeground, isInCall, isSyncingNotificationEvent -> + val isAppActive = isInForeground || isInCall || isSyncingNotificationEvent + val isNetworkAvailable = networkState == NetworkStatus.Connected - Timber.tag(tag).d("isAppActive=$isAppActive, isNetworkAvailable=$isNetworkAvailable") - if (syncState == SyncState.Running && !isAppActive) { - // Don't stop the sync immediately, wait a bit to avoid starting/stopping the sync too often - delay(3.seconds) - SyncStateAction.StopSync - } else if (syncState != SyncState.Running && isAppActive && isNetworkAvailable) { - SyncStateAction.StartSync - } else { - SyncStateAction.NoOp - } + Timber.tag(tag).d("isAppActive=$isAppActive, isNetworkAvailable=$isNetworkAvailable") + if (syncState == SyncState.Running && !isAppActive) { + // Don't stop the sync immediately, wait a bit to avoid starting/stopping the sync too often + delay(3.seconds) + SyncStateAction.StopSync + } else if (syncState != SyncState.Running && isAppActive && isNetworkAvailable) { + SyncStateAction.StartSync + } else { + SyncStateAction.NoOp } - .distinctUntilChanged() - .collect { action -> - when (action) { - SyncStateAction.StartSync -> { - syncService.startSync() - } - SyncStateAction.StopSync -> { - syncService.stopSync() - } - SyncStateAction.NoOp -> Unit + } + .distinctUntilChanged() + .onEach { action -> + when (action) { + SyncStateAction.StartSync -> { + syncService.startSync() + } + SyncStateAction.StopSync -> { + syncService.stopSync() } + SyncStateAction.NoOp -> Unit } - } + } + .onCompletion { + Timber.tag(tag).d("has been stopped") + } + .launchIn(coroutineScope) } } diff --git a/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt index 89ad88138a9..d6fb24ed149 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt @@ -17,10 +17,7 @@ import io.element.android.services.appnavstate.test.FakeAppForegroundStateServic import io.element.android.tests.testutils.WarmUpRule import io.element.android.tests.testutils.lambda.lambdaRecorder import io.element.android.tests.testutils.testCoroutineDispatchers -import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.SupervisorJob -import kotlinx.coroutines.cancel import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.advanceTimeBy import kotlinx.coroutines.test.runTest @@ -42,12 +39,10 @@ class SyncOrchestratorTest { } val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Connected) val appForegroundStateService = FakeAppForegroundStateService(initialForegroundValue = true) - val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val syncOrchestrator = createSyncOrchestrator( syncService = syncService, networkMonitor = networkMonitor, appForegroundStateService = appForegroundStateService, - baseCoroutineScope = coroutineScope ) // We start observing, we skip the initial sync attempt since the state is running @@ -66,9 +61,6 @@ class SyncOrchestratorTest { stopSyncRecorder.assertions().isNeverCalled() advanceTimeBy(10.seconds) stopSyncRecorder.assertions().isCalledOnce() - - // Stop observing - coroutineScope.cancel() } @Test @@ -84,12 +76,10 @@ class SyncOrchestratorTest { initialForegroundValue = false, initialIsSyncingNotificationEventValue = false, ) - val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val syncOrchestrator = createSyncOrchestrator( syncService = syncService, networkMonitor = networkMonitor, appForegroundStateService = appForegroundStateService, - baseCoroutineScope = coroutineScope ) // We start observing, we skip the initial sync attempt since the state is running @@ -119,9 +109,6 @@ class SyncOrchestratorTest { advanceTimeBy(10.seconds) stopSyncRecorder.assertions().isCalledExactly(2) - - // Stop observing - coroutineScope.cancel() } @Test @@ -137,12 +124,10 @@ class SyncOrchestratorTest { initialForegroundValue = false, initialIsSyncingNotificationEventValue = false, ) - val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val syncOrchestrator = createSyncOrchestrator( syncService = syncService, networkMonitor = networkMonitor, appForegroundStateService = appForegroundStateService, - baseCoroutineScope = coroutineScope ) // We start observing, we skip the initial sync attempt since the state is running @@ -172,9 +157,6 @@ class SyncOrchestratorTest { advanceTimeBy(10.seconds) stopSyncRecorder.assertions().isCalledExactly(2) - - // Stop observing - coroutineScope.cancel() } @Test @@ -191,12 +173,10 @@ class SyncOrchestratorTest { initialIsSyncingNotificationEventValue = true, initialIsInCallValue = true, ) - val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val syncOrchestrator = createSyncOrchestrator( syncService = syncService, networkMonitor = networkMonitor, appForegroundStateService = appForegroundStateService, - baseCoroutineScope = coroutineScope ) // We start observing, we skip the initial sync attempt since the state is running @@ -222,9 +202,6 @@ class SyncOrchestratorTest { appForegroundStateService.updateIsInCallState(false) advanceTimeBy(10.seconds) stopSyncRecorder.assertions().isCalledOnce() - - // Stop observing - coroutineScope.cancel() } @Test @@ -239,12 +216,10 @@ class SyncOrchestratorTest { initialIsSyncingNotificationEventValue = true, initialIsInCallValue = true, ) - val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val syncOrchestrator = createSyncOrchestrator( syncService = syncService, networkMonitor = networkMonitor, appForegroundStateService = appForegroundStateService, - baseCoroutineScope = coroutineScope ) // We start observing, we skip the initial sync attempt since the state is running @@ -261,9 +236,6 @@ class SyncOrchestratorTest { advanceTimeBy(10.seconds) stopSyncRecorder.assertions().isNeverCalled() - - // Stop observing - coroutineScope.cancel() } @Test @@ -273,11 +245,9 @@ class SyncOrchestratorTest { startSyncLambda = startSyncRecorder } val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Disconnected) - val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val syncOrchestrator = createSyncOrchestrator( syncService = syncService, networkMonitor = networkMonitor, - baseCoroutineScope = coroutineScope ) // We start observing, we skip the initial sync attempt since the state is running @@ -292,21 +262,16 @@ class SyncOrchestratorTest { // This should still not trigger a sync, since there is no network advanceTimeBy(10.seconds) startSyncRecorder.assertions().isNeverCalled() - - // Stop observing - coroutineScope.cancel() } private fun TestScope.createSyncOrchestrator( syncService: FakeSyncService = FakeSyncService(), networkMonitor: FakeNetworkMonitor = FakeNetworkMonitor(), appForegroundStateService: FakeAppForegroundStateService = FakeAppForegroundStateService(), - baseCoroutineScope: CoroutineScope = CoroutineScope(coroutineContext + SupervisorJob()), ) = SyncOrchestrator( - matrixClient = FakeMatrixClient(syncService = syncService), + matrixClient = FakeMatrixClient(syncService = syncService, sessionCoroutineScope = backgroundScope), networkMonitor = networkMonitor, appForegroundStateService = appForegroundStateService, - sessionCoroutineScope = baseCoroutineScope, dispatchers = testCoroutineDispatchers(), ) } diff --git a/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt index 9192eb49a0b..52443fa4a1e 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixSessionCacheTest.kt @@ -16,10 +16,7 @@ import io.element.android.libraries.matrix.test.FakeMatrixClient import io.element.android.libraries.matrix.test.auth.FakeMatrixAuthenticationService import io.element.android.services.appnavstate.test.FakeAppForegroundStateService import io.element.android.tests.testutils.testCoroutineDispatchers -import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.SupervisorJob -import kotlinx.coroutines.cancel import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.runTest import org.junit.Test @@ -35,77 +32,64 @@ class MatrixSessionCacheTest { @OptIn(ExperimentalCoroutinesApi::class) @Test fun `test getSyncOrchestratorOrNull`() = runTest { - val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory(coroutineScope)) + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) // With no matrix client there is no sync orchestrator assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() assertThat(matrixSessionCache.getSyncOrchestrator(A_SESSION_ID)).isNull() // But as soon as we receive a client, we can get the sync orchestrator - val fakeMatrixClient = FakeMatrixClient() + val fakeMatrixClient = FakeMatrixClient(sessionCoroutineScope = backgroundScope) fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) assertThat(matrixSessionCache.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) assertThat(matrixSessionCache.getSyncOrchestrator(A_SESSION_ID)).isNotNull() - - coroutineScope.cancel() } @Test fun `test getOrRestore`() = runTest { - val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory(coroutineScope)) - val fakeMatrixClient = FakeMatrixClient() + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + val fakeMatrixClient = FakeMatrixClient(sessionCoroutineScope = backgroundScope) fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() assertThat(matrixSessionCache.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) // Do it again to hit the cache assertThat(matrixSessionCache.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isEqualTo(fakeMatrixClient) - - coroutineScope.cancel() } @Test fun `test remove`() = runTest { - val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory(coroutineScope)) - val fakeMatrixClient = FakeMatrixClient() + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + val fakeMatrixClient = FakeMatrixClient(sessionCoroutineScope = backgroundScope) fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) assertThat(matrixSessionCache.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isEqualTo(fakeMatrixClient) // Remove matrixSessionCache.remove(A_SESSION_ID) assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() - - coroutineScope.cancel() } @Test fun `test remove all`() = runTest { - val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory(coroutineScope)) - val fakeMatrixClient = FakeMatrixClient() + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + val fakeMatrixClient = FakeMatrixClient(sessionCoroutineScope = backgroundScope) fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) assertThat(matrixSessionCache.getOrRestore(A_SESSION_ID).getOrNull()).isEqualTo(fakeMatrixClient) assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isEqualTo(fakeMatrixClient) // Remove all matrixSessionCache.removeAll() assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() - - coroutineScope.cancel() } @Test fun `test save and restore`() = runTest { - val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory(coroutineScope)) - val fakeMatrixClient = FakeMatrixClient() + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) + val fakeMatrixClient = FakeMatrixClient(sessionCoroutineScope = backgroundScope) fakeAuthenticationService.givenMatrixClient(fakeMatrixClient) matrixSessionCache.getOrRestore(A_SESSION_ID) val savedStateMap = MutableSavedStateMapImpl { true } @@ -119,33 +103,25 @@ class MatrixSessionCacheTest { // Restore again matrixSessionCache.restoreWithSavedState(savedStateMap) assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isEqualTo(fakeMatrixClient) - - coroutineScope.cancel() } @Test fun `test AuthenticationService listenToNewMatrixClients emits a Client value and we save it`() = runTest { - val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val fakeAuthenticationService = FakeMatrixAuthenticationService() - val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory(coroutineScope)) + val matrixSessionCache = MatrixSessionCache(fakeAuthenticationService, createSyncOrchestratorFactory()) assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNull() - fakeAuthenticationService.givenMatrixClient(FakeMatrixClient(sessionId = A_SESSION_ID)) + fakeAuthenticationService.givenMatrixClient(FakeMatrixClient(sessionId = A_SESSION_ID, sessionCoroutineScope = backgroundScope)) val loginSucceeded = fakeAuthenticationService.login("user", "pass") assertThat(loginSucceeded.isSuccess).isTrue() assertThat(matrixSessionCache.getOrNull(A_SESSION_ID)).isNotNull() - - coroutineScope.cancel() } - private fun TestScope.createSyncOrchestratorFactory( - baseCoroutineScope: CoroutineScope = this, - ) = object : SyncOrchestrator.Factory { + private fun TestScope.createSyncOrchestratorFactory() = object : SyncOrchestrator.Factory { override fun create(matrixClient: MatrixClient): SyncOrchestrator { return SyncOrchestrator( matrixClient, - sessionCoroutineScope = baseCoroutineScope, appForegroundStateService = FakeAppForegroundStateService(), networkMonitor = FakeNetworkMonitor(), dispatchers = testCoroutineDispatchers(), diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt index 8bc69769240..ea82df0c925 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt @@ -495,31 +495,32 @@ class RustMatrixClient( override suspend fun logout(userInitiated: Boolean, ignoreSdkError: Boolean): String? { var result: String? = null + sessionCoroutineScope.cancel() // Remove current delegate so we don't receive an auth error - clientDelegateTaskHandle?.cancelAndDestroy() - clientDelegateTaskHandle = null - withContext(sessionDispatcher) { - if (userInitiated) { - try { - result = innerClient.logout() - } catch (failure: Throwable) { - if (ignoreSdkError) { - Timber.e(failure, "Fail to call logout on HS. Still delete local files.") - } else { - // If the logout failed we need to restore the delegate - clientDelegateTaskHandle = innerClient.setDelegate(sessionDelegate) - Timber.e(failure, "Fail to call logout on HS.") - throw failure - } - } - } - close() - - deleteSessionDirectory(deleteCryptoDb = true) - if (userInitiated) { - sessionStore.removeSession(sessionId.value) - } - } +// clientDelegateTaskHandle?.cancelAndDestroy() +// clientDelegateTaskHandle = null +// withContext(sessionDispatcher) { +// if (userInitiated) { +// try { +// result = innerClient.logout() +// } catch (failure: Throwable) { +// if (ignoreSdkError) { +// Timber.e(failure, "Fail to call logout on HS. Still delete local files.") +// } else { +// // If the logout failed we need to restore the delegate +// clientDelegateTaskHandle = innerClient.setDelegate(sessionDelegate) +// Timber.e(failure, "Fail to call logout on HS.") +// throw failure +// } +// } +// } +// close() +// +// deleteSessionDirectory(deleteCryptoDb = true) +// if (userInitiated) { +// sessionStore.removeSession(sessionId.value) +// } +// } return result } From ef8c5245fa5c52167616e65cf6b12d9db9c26150 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Wed, 5 Feb 2025 17:46:12 +0100 Subject: [PATCH 11/15] Add test to ensure the delay for stopping the sync is honoured --- .../android/appnav/SyncOrchestratorTest.kt | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt index d6fb24ed149..5346a696d4a 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt @@ -63,6 +63,49 @@ class SyncOrchestratorTest { stopSyncRecorder.assertions().isCalledOnce() } + @Test + fun `when the app state changes several times in a short while, stop sync is only called once`() = runTest { + val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } + val syncService = FakeSyncService(initialSyncState = SyncState.Running).apply { + stopSyncLambda = stopSyncRecorder + } + val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Connected) + val appForegroundStateService = FakeAppForegroundStateService(initialForegroundValue = true) + val syncOrchestrator = createSyncOrchestrator( + syncService = syncService, + networkMonitor = networkMonitor, + appForegroundStateService = appForegroundStateService, + ) + + // We start observing, we skip the initial sync attempt since the state is running + syncOrchestrator.start() + + // Advance the time to make sure we left the initial sync behind + advanceTimeBy(1.seconds) + + // Stop sync was never called + stopSyncRecorder.assertions().isNeverCalled() + + // Now we send the app to background + appForegroundStateService.isInForeground.value = false + + // Ensure the stop action wasn't called yet + stopSyncRecorder.assertions().isNeverCalled() + advanceTimeBy(1.seconds) + appForegroundStateService.isInForeground.value = true + advanceTimeBy(1.seconds) + + // Ensure the stop action wasn't called yet either, since we didn't give it enough time to emit after the expected delay + stopSyncRecorder.assertions().isNeverCalled() + + // Now change it again and wait for enough time + appForegroundStateService.isInForeground.value = false + advanceTimeBy(3.seconds) + + // And confirm it's now called + stopSyncRecorder.assertions().isCalledOnce() + } + @Test fun `when the app was in background and we receive a notification, a sync will be started then stopped`() = runTest { val startSyncRecorder = lambdaRecorder> { Result.success(Unit) } From efcc10c99cb678290d4855581817bd50f0d63e83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Thu, 6 Feb 2025 08:09:28 +0100 Subject: [PATCH 12/15] Restore logout code --- .../libraries/matrix/impl/RustMatrixClient.kt | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt index ea82df0c925..a37c57b16c9 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt @@ -497,30 +497,30 @@ class RustMatrixClient( var result: String? = null sessionCoroutineScope.cancel() // Remove current delegate so we don't receive an auth error -// clientDelegateTaskHandle?.cancelAndDestroy() -// clientDelegateTaskHandle = null -// withContext(sessionDispatcher) { -// if (userInitiated) { -// try { -// result = innerClient.logout() -// } catch (failure: Throwable) { -// if (ignoreSdkError) { -// Timber.e(failure, "Fail to call logout on HS. Still delete local files.") -// } else { -// // If the logout failed we need to restore the delegate -// clientDelegateTaskHandle = innerClient.setDelegate(sessionDelegate) -// Timber.e(failure, "Fail to call logout on HS.") -// throw failure -// } -// } -// } -// close() -// -// deleteSessionDirectory(deleteCryptoDb = true) -// if (userInitiated) { -// sessionStore.removeSession(sessionId.value) -// } -// } + clientDelegateTaskHandle?.cancelAndDestroy() + clientDelegateTaskHandle = null + withContext(sessionDispatcher) { + if (userInitiated) { + try { + result = innerClient.logout() + } catch (failure: Throwable) { + if (ignoreSdkError) { + Timber.e(failure, "Fail to call logout on HS. Still delete local files.") + } else { + // If the logout failed we need to restore the delegate + clientDelegateTaskHandle = innerClient.setDelegate(sessionDelegate) + Timber.e(failure, "Fail to call logout on HS.") + throw failure + } + } + } + close() + + deleteSessionDirectory(deleteCryptoDb = true) + if (userInitiated) { + sessionStore.removeSession(sessionId.value) + } + } return result } From 07ed484c62957e0e60f43dac09f64f33067a4954 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Thu, 6 Feb 2025 08:16:59 +0100 Subject: [PATCH 13/15] Nit fixes --- .../android/features/call/impl/ui/CallScreenPresenter.kt | 4 +--- .../io/element/android/features/login/impl/LoginFlowNode.kt | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/CallScreenPresenter.kt b/features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/CallScreenPresenter.kt index 63c5953802d..ac0c89f0fd4 100644 --- a/features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/CallScreenPresenter.kt +++ b/features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/CallScreenPresenter.kt @@ -233,9 +233,7 @@ class CallScreenPresenter @AssistedInject constructor( } onDispose { // Make sure we mark the call as ended in the app state - if (appForegroundStateService.isInCall.value) { - appForegroundStateService.updateIsInCallState(false) - } + appForegroundStateService.updateIsInCallState(false) } } } diff --git a/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/LoginFlowNode.kt b/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/LoginFlowNode.kt index ef8673801e2..0f2328656e7 100644 --- a/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/LoginFlowNode.kt +++ b/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/LoginFlowNode.kt @@ -199,7 +199,7 @@ class LoginFlowNode @AssistedInject constructor( @Composable override fun View(modifier: Modifier) { - activity = LocalActivity.current + activity = requireNotNull(LocalActivity.current) darkTheme = !ElementTheme.isLightTheme DisposableEffect(Unit) { onDispose { From ecd76d80fdc02dbf8cdd385f14ee8506f5a2b8d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Thu, 6 Feb 2025 12:52:28 +0100 Subject: [PATCH 14/15] Replace the `delay` with a `debounce` for stopping the sync, simplify tests --- .../android/appnav/di/SyncOrchestrator.kt | 7 +- .../android/appnav/SyncOrchestratorTest.kt | 73 ++++++++----------- 2 files changed, 33 insertions(+), 47 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/SyncOrchestrator.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/SyncOrchestrator.kt index 8149b847e7c..e4ca0cbffeb 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/SyncOrchestrator.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/SyncOrchestrator.kt @@ -18,7 +18,6 @@ import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.sync.SyncState import io.element.android.services.appnavstate.api.AppForegroundStateService import kotlinx.coroutines.FlowPreview -import kotlinx.coroutines.delay import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.distinctUntilChanged @@ -76,8 +75,6 @@ class SyncOrchestrator @AssistedInject constructor( Timber.tag(tag).d("isAppActive=$isAppActive, isNetworkAvailable=$isNetworkAvailable") if (syncState == SyncState.Running && !isAppActive) { - // Don't stop the sync immediately, wait a bit to avoid starting/stopping the sync too often - delay(3.seconds) SyncStateAction.StopSync } else if (syncState != SyncState.Running && isAppActive && isNetworkAvailable) { SyncStateAction.StartSync @@ -86,6 +83,10 @@ class SyncOrchestrator @AssistedInject constructor( } } .distinctUntilChanged() + .debounce { action -> + // Don't stop the sync immediately, wait a bit to avoid starting/stopping the sync too often + if (action == SyncStateAction.StopSync) 3.seconds else 0.seconds + } .onEach { action -> when (action) { SyncStateAction.StartSync -> { diff --git a/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt index 5346a696d4a..a1b8a1cf5eb 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt @@ -45,11 +45,11 @@ class SyncOrchestratorTest { appForegroundStateService = appForegroundStateService, ) - // We start observing, we skip the initial sync attempt since the state is running + // We start observing syncOrchestrator.start() - // Advance the time to make sure we left the initial sync behind - advanceTimeBy(1.seconds) + // Advance the time to make sure the orchestrator has had time to start processing the inputs + advanceTimeBy(100.milliseconds) // Stop sync was never called stopSyncRecorder.assertions().isNeverCalled() @@ -77,11 +77,11 @@ class SyncOrchestratorTest { appForegroundStateService = appForegroundStateService, ) - // We start observing, we skip the initial sync attempt since the state is running + // We start observing syncOrchestrator.start() - // Advance the time to make sure we left the initial sync behind - advanceTimeBy(1.seconds) + // Advance the time to make sure the orchestrator has had time to start processing the inputs + advanceTimeBy(100.milliseconds) // Stop sync was never called stopSyncRecorder.assertions().isNeverCalled() @@ -100,7 +100,7 @@ class SyncOrchestratorTest { // Now change it again and wait for enough time appForegroundStateService.isInForeground.value = false - advanceTimeBy(3.seconds) + advanceTimeBy(4.seconds) // And confirm it's now called stopSyncRecorder.assertions().isCalledOnce() @@ -110,7 +110,7 @@ class SyncOrchestratorTest { fun `when the app was in background and we receive a notification, a sync will be started then stopped`() = runTest { val startSyncRecorder = lambdaRecorder> { Result.success(Unit) } val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } - val syncService = FakeSyncService(initialSyncState = SyncState.Running).apply { + val syncService = FakeSyncService(initialSyncState = SyncState.Idle).apply { startSyncLambda = startSyncRecorder stopSyncLambda = stopSyncRecorder } @@ -125,20 +125,15 @@ class SyncOrchestratorTest { appForegroundStateService = appForegroundStateService, ) - // We start observing, we skip the initial sync attempt since the state is running + // We start observing syncOrchestrator.start() - // Advance the time to make sure we left the initial sync behind - advanceTimeBy(1.seconds) + // Advance the time to make sure the orchestrator has had time to start processing the inputs + advanceTimeBy(100.milliseconds) // Start sync was never called startSyncRecorder.assertions().isNeverCalled() - // We stop the ongoing sync, give the sync service some time to stop - syncService.emitSyncState(SyncState.Idle) - advanceTimeBy(10.seconds) - stopSyncRecorder.assertions().isCalledOnce() - // Now we receive a notification and need to sync appForegroundStateService.updateIsSyncingNotificationEvent(true) @@ -151,14 +146,14 @@ class SyncOrchestratorTest { appForegroundStateService.updateIsSyncingNotificationEvent(false) advanceTimeBy(10.seconds) - stopSyncRecorder.assertions().isCalledExactly(2) + stopSyncRecorder.assertions().isCalledOnce() } @Test fun `when the app was in background and we join a call, a sync will be started`() = runTest { val startSyncRecorder = lambdaRecorder> { Result.success(Unit) } val stopSyncRecorder = lambdaRecorder> { Result.success(Unit) } - val syncService = FakeSyncService(initialSyncState = SyncState.Running).apply { + val syncService = FakeSyncService(initialSyncState = SyncState.Idle).apply { startSyncLambda = startSyncRecorder stopSyncLambda = stopSyncRecorder } @@ -173,20 +168,15 @@ class SyncOrchestratorTest { appForegroundStateService = appForegroundStateService, ) - // We start observing, we skip the initial sync attempt since the state is running + // We start observing syncOrchestrator.start() - // Advance the time to make sure we left the initial sync behind - advanceTimeBy(1.seconds) + // Advance the time to make sure the orchestrator has had time to start processing the inputs + advanceTimeBy(100.milliseconds) // Start sync was never called startSyncRecorder.assertions().isNeverCalled() - // We stop the ongoing sync, give the sync service some time to stop - syncService.emitSyncState(SyncState.Idle) - advanceTimeBy(10.seconds) - stopSyncRecorder.assertions().isCalledOnce() - // Now we join a call appForegroundStateService.updateIsInCallState(true) @@ -199,7 +189,7 @@ class SyncOrchestratorTest { appForegroundStateService.updateIsInCallState(false) advanceTimeBy(10.seconds) - stopSyncRecorder.assertions().isCalledExactly(2) + stopSyncRecorder.assertions().isCalledOnce() } @Test @@ -222,11 +212,11 @@ class SyncOrchestratorTest { appForegroundStateService = appForegroundStateService, ) - // We start observing, we skip the initial sync attempt since the state is running + // We start observing syncOrchestrator.start() - // Advance the time to make sure we left the initial sync behind - advanceTimeBy(1.seconds) + // Advance the time to make sure the orchestrator has had time to start processing the inputs + advanceTimeBy(100.milliseconds) // Start sync was never called startSyncRecorder.assertions().isNeverCalled() @@ -256,8 +246,8 @@ class SyncOrchestratorTest { val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Connected) val appForegroundStateService = FakeAppForegroundStateService( initialForegroundValue = true, - initialIsSyncingNotificationEventValue = true, - initialIsInCallValue = true, + initialIsSyncingNotificationEventValue = false, + initialIsInCallValue = false, ) val syncOrchestrator = createSyncOrchestrator( syncService = syncService, @@ -265,16 +255,17 @@ class SyncOrchestratorTest { appForegroundStateService = appForegroundStateService, ) - // We start observing, we skip the initial sync attempt since the state is running + // We start observing syncOrchestrator.start() - // Advance the time to make sure we left the initial sync behind - advanceTimeBy(1.seconds) + // Advance the time to make sure the orchestrator has had time to start processing the inputs + advanceTimeBy(100.milliseconds) // This will set the sync to stop appForegroundStateService.givenIsInForeground(false) - + // But if we reset it quickly before the stop sync takes place, the sync is not stopped + advanceTimeBy(2.seconds) appForegroundStateService.givenIsInForeground(true) advanceTimeBy(10.seconds) @@ -284,7 +275,7 @@ class SyncOrchestratorTest { @Test fun `when network is offline, sync service should not start`() = runTest { val startSyncRecorder = lambdaRecorder> { Result.success(Unit) } - val syncService = FakeSyncService(initialSyncState = SyncState.Running).apply { + val syncService = FakeSyncService(initialSyncState = SyncState.Idle).apply { startSyncLambda = startSyncRecorder } val networkMonitor = FakeNetworkMonitor(initialStatus = NetworkStatus.Disconnected) @@ -293,15 +284,9 @@ class SyncOrchestratorTest { networkMonitor = networkMonitor, ) - // We start observing, we skip the initial sync attempt since the state is running + // We start observing syncOrchestrator.start() - // Advance the time to make sure we left the initial sync behind - advanceTimeBy(1.seconds) - - // Set the sync state to idle - syncService.emitSyncState(SyncState.Idle) - // This should still not trigger a sync, since there is no network advanceTimeBy(10.seconds) startSyncRecorder.assertions().isNeverCalled() From b3bba14da10515889278b7ecb471cea9f92e5848 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Thu, 6 Feb 2025 13:22:59 +0100 Subject: [PATCH 15/15] Fix lint issues caused by whitespace --- .../kotlin/io/element/android/appnav/SyncOrchestratorTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt index a1b8a1cf5eb..bdccdec9a4d 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/SyncOrchestratorTest.kt @@ -263,7 +263,7 @@ class SyncOrchestratorTest { // This will set the sync to stop appForegroundStateService.givenIsInForeground(false) - + // But if we reset it quickly before the stop sync takes place, the sync is not stopped advanceTimeBy(2.seconds) appForegroundStateService.givenIsInForeground(true)