From 29f3d6d72575930baad062426f28a8a9fec43a3d Mon Sep 17 00:00:00 2001 From: ganfra Date: Wed, 12 Apr 2023 15:45:53 +0200 Subject: [PATCH] AppNav: introduce a owner param so we avoid crash on AppNavigationState when switching quickly between screens --- .../android/appnav/LoggedInFlowNode.kt | 11 ++-- .../io/element/android/appnav/RoomFlowNode.kt | 4 +- .../appnavstate/api/AppNavigationState.kt | 20 +++++--- .../api/AppNavigationStateService.kt | 16 +++--- .../impl/DefaultAppNavigationStateService.kt | 51 +++++++++++-------- .../DefaultAppNavigationStateServiceTest.kt | 20 +++++--- 6 files changed, 73 insertions(+), 49 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 6f9319f923..9f3a211228 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt @@ -56,10 +56,7 @@ import io.element.android.libraries.matrix.api.core.RoomId import io.element.android.libraries.matrix.ui.di.MatrixUIBindings import io.element.android.services.appnavstate.api.AppNavigationStateService import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.runBlocking import kotlinx.parcelize.Parcelize -import kotlin.coroutines.coroutineContext @ContributesNode(AppScope::class) class LoggedInFlowNode @AssistedInject constructor( @@ -110,17 +107,17 @@ class LoggedInFlowNode @AssistedInject constructor( val imageLoaderFactory = bindings().loggedInImageLoaderFactory() Coil.setImageLoader(imageLoaderFactory) inputs.matrixClient.startSync() - appNavigationStateService.onNavigateToSession(inputs.matrixClient.sessionId) + appNavigationStateService.onNavigateToSession(id, inputs.matrixClient.sessionId) // TODO We do not support Space yet, so directly navigate to main space - appNavigationStateService.onNavigateToSpace(MAIN_SPACE) + appNavigationStateService.onNavigateToSpace(id, MAIN_SPACE) loggedInFlowProcessor.observeEvents(coroutineScope) }, onDestroy = { val imageLoaderFactory = bindings().notLoggedInImageLoaderFactory() Coil.setImageLoader(imageLoaderFactory) plugins().forEach { it.onFlowReleased(inputs.matrixClient) } - appNavigationStateService.onLeavingSpace() - appNavigationStateService.onLeavingSession() + appNavigationStateService.onLeavingSpace(id) + appNavigationStateService.onLeavingSession(id) loggedInFlowProcessor.stopObserving() } ) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/RoomFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/RoomFlowNode.kt index be3b88a18c..ba23dd22d4 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/RoomFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/RoomFlowNode.kt @@ -82,13 +82,13 @@ class RoomFlowNode @AssistedInject constructor( onCreate = { Timber.v("OnCreate") plugins().forEach { it.onFlowCreated(inputs.room) } - appNavigationStateService.onNavigateToRoom(inputs.room.roomId) + appNavigationStateService.onNavigateToRoom(id, inputs.room.roomId) }, onDestroy = { Timber.v("OnDestroy") inputs.room.close() plugins().forEach { it.onFlowReleased(inputs.room) } - appNavigationStateService.onLeavingRoom() + appNavigationStateService.onLeavingRoom(id) } ) diff --git a/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/AppNavigationState.kt b/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/AppNavigationState.kt index af6d1d7b0a..8a96c35af1 100644 --- a/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/AppNavigationState.kt +++ b/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/AppNavigationState.kt @@ -21,26 +21,34 @@ import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.matrix.api.core.SpaceId import io.element.android.libraries.matrix.api.core.ThreadId -sealed interface AppNavigationState { - object Root : AppNavigationState +/** + * Can represent the current global app navigation state. + * @param owner mostly a Node identifier associated with the state. + */ +sealed class AppNavigationState(open val owner: String) { + object Root : AppNavigationState("ROOT") data class Session( + override val owner: String, val sessionId: SessionId, - ) : AppNavigationState + ) : AppNavigationState(owner) data class Space( + override val owner: String, // Can be fake value, if no space is selected val spaceId: SpaceId, val parentSession: Session, - ) : AppNavigationState + ) : AppNavigationState(owner) data class Room( + override val owner: String, val roomId: RoomId, val parentSpace: Space, - ) : AppNavigationState + ) : AppNavigationState(owner) data class Thread( + override val owner: String, val threadId: ThreadId, val parentRoom: Room, - ) : AppNavigationState + ) : AppNavigationState(owner) } diff --git a/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/AppNavigationStateService.kt b/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/AppNavigationStateService.kt index c68db8afb7..4bb40b7b75 100644 --- a/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/AppNavigationStateService.kt +++ b/services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/AppNavigationStateService.kt @@ -25,15 +25,15 @@ import kotlinx.coroutines.flow.StateFlow interface AppNavigationStateService { val appNavigationStateFlow: StateFlow - fun onNavigateToSession(sessionId: SessionId) - fun onLeavingSession() + fun onNavigateToSession(owner: String, sessionId: SessionId) + fun onLeavingSession(owner: String) - fun onNavigateToSpace(spaceId: SpaceId) - fun onLeavingSpace() + fun onNavigateToSpace(owner: String, spaceId: SpaceId) + fun onLeavingSpace(owner: String) - fun onNavigateToRoom(roomId: RoomId) - fun onLeavingRoom() + fun onNavigateToRoom(owner: String, roomId: RoomId) + fun onLeavingRoom(owner: String) - fun onNavigateToThread(threadId: ThreadId) - fun onLeavingThread() + fun onNavigateToThread(owner: String, threadId: ThreadId) + fun onLeavingThread(owner: String) } 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 4e0071cf46..bf20a04b11 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 @@ -40,11 +40,10 @@ private val loggerTag = LoggerTag("Navigation") @SingleIn(AppScope::class) class DefaultAppNavigationStateService @Inject constructor() : AppNavigationStateService { - private val currentAppNavigationState = MutableStateFlow(AppNavigationState.Root) - + private val currentAppNavigationState: MutableStateFlow = MutableStateFlow(AppNavigationState.Root) override val appNavigationStateFlow: StateFlow = currentAppNavigationState - override fun onNavigateToSession(sessionId: SessionId) { + override fun onNavigateToSession(owner: String, sessionId: SessionId) { val currentValue = currentAppNavigationState.value Timber.tag(loggerTag.value).d("Navigating to session $sessionId. Current state: $currentValue") val newValue: AppNavigationState.Session = when (currentValue) { @@ -52,53 +51,54 @@ class DefaultAppNavigationStateService @Inject constructor() : AppNavigationStat is AppNavigationState.Space, is AppNavigationState.Room, is AppNavigationState.Thread, - AppNavigationState.Root -> AppNavigationState.Session(sessionId) + is AppNavigationState.Root -> AppNavigationState.Session(owner, sessionId) } currentAppNavigationState.value = newValue } - override fun onNavigateToSpace(spaceId: SpaceId) { + override fun onNavigateToSpace(owner: String, spaceId: SpaceId) { val currentValue = currentAppNavigationState.value Timber.tag(loggerTag.value).d("Navigating to space $spaceId. Current state: $currentValue") val newValue: AppNavigationState.Space = when (currentValue) { AppNavigationState.Root -> error("onNavigateToSession() must be called first") - is AppNavigationState.Session -> AppNavigationState.Space(spaceId, currentValue) - is AppNavigationState.Space -> AppNavigationState.Space(spaceId, currentValue.parentSession) - is AppNavigationState.Room -> AppNavigationState.Space(spaceId, currentValue.parentSpace.parentSession) - is AppNavigationState.Thread -> AppNavigationState.Space(spaceId, currentValue.parentRoom.parentSpace.parentSession) + is AppNavigationState.Session -> AppNavigationState.Space(owner, spaceId, currentValue) + is AppNavigationState.Space -> AppNavigationState.Space(owner, spaceId, currentValue.parentSession) + is AppNavigationState.Room -> AppNavigationState.Space(owner, spaceId, currentValue.parentSpace.parentSession) + is AppNavigationState.Thread -> AppNavigationState.Space(owner, spaceId, currentValue.parentRoom.parentSpace.parentSession) } currentAppNavigationState.value = newValue } - override fun onNavigateToRoom(roomId: RoomId) { + override fun onNavigateToRoom(owner: String, roomId: RoomId) { val currentValue = currentAppNavigationState.value Timber.tag(loggerTag.value).d("Navigating to room $roomId. Current state: $currentValue") val newValue: AppNavigationState.Room = when (currentValue) { AppNavigationState.Root -> error("onNavigateToSession() must be called first") is AppNavigationState.Session -> error("onNavigateToSpace() must be called first") - is AppNavigationState.Space -> AppNavigationState.Room(roomId, currentValue) - is AppNavigationState.Room -> AppNavigationState.Room(roomId, currentValue.parentSpace) - is AppNavigationState.Thread -> AppNavigationState.Room(roomId, currentValue.parentRoom.parentSpace) + is AppNavigationState.Space -> AppNavigationState.Room(owner, roomId, currentValue) + is AppNavigationState.Room -> AppNavigationState.Room(owner, roomId, currentValue.parentSpace) + is AppNavigationState.Thread -> AppNavigationState.Room(owner, roomId, currentValue.parentRoom.parentSpace) } currentAppNavigationState.value = newValue } - override fun onNavigateToThread(threadId: ThreadId) { + override fun onNavigateToThread(owner: String, threadId: ThreadId) { val currentValue = currentAppNavigationState.value Timber.tag(loggerTag.value).d("Navigating to thread $threadId. Current state: $currentValue") val newValue: AppNavigationState.Thread = when (currentValue) { AppNavigationState.Root -> error("onNavigateToSession() must be called first") is AppNavigationState.Session -> error("onNavigateToSpace() must be called first") is AppNavigationState.Space -> error("onNavigateToRoom() must be called first") - is AppNavigationState.Room -> AppNavigationState.Thread(threadId, currentValue) - is AppNavigationState.Thread -> AppNavigationState.Thread(threadId, currentValue.parentRoom) + is AppNavigationState.Room -> AppNavigationState.Thread(owner, threadId, currentValue) + is AppNavigationState.Thread -> AppNavigationState.Thread(owner, threadId, currentValue.parentRoom) } currentAppNavigationState.value = newValue } - override fun onLeavingThread() { + override fun onLeavingThread(owner: String) { val currentValue = currentAppNavigationState.value Timber.tag(loggerTag.value).d("Leaving thread. Current state: $currentValue") + if (!currentValue.assertOwner(owner)) return val newValue: AppNavigationState.Room = when (currentValue) { AppNavigationState.Root -> error("onNavigateToSession() must be called first") is AppNavigationState.Session -> error("onNavigateToSpace() must be called first") @@ -109,9 +109,10 @@ class DefaultAppNavigationStateService @Inject constructor() : AppNavigationStat currentAppNavigationState.value = newValue } - override fun onLeavingRoom() { + override fun onLeavingRoom(owner: String) { val currentValue = currentAppNavigationState.value Timber.tag(loggerTag.value).d("Leaving room. Current state: $currentValue") + if (!currentValue.assertOwner(owner)) return val newValue: AppNavigationState.Space = when (currentValue) { AppNavigationState.Root -> error("onNavigateToSession() must be called first") is AppNavigationState.Session -> error("onNavigateToSpace() must be called first") @@ -122,9 +123,10 @@ class DefaultAppNavigationStateService @Inject constructor() : AppNavigationStat currentAppNavigationState.value = newValue } - override fun onLeavingSpace() { + override fun onLeavingSpace(owner: String) { val currentValue = currentAppNavigationState.value Timber.tag(loggerTag.value).d("Leaving space. Current state: $currentValue") + if (!currentValue.assertOwner(owner)) return val newValue: AppNavigationState.Session = when (currentValue) { AppNavigationState.Root -> error("onNavigateToSession() must be called first") is AppNavigationState.Session -> error("onNavigateToSpace() must be called first") @@ -135,9 +137,18 @@ class DefaultAppNavigationStateService @Inject constructor() : AppNavigationStat currentAppNavigationState.value = newValue } - override fun onLeavingSession() { + override fun onLeavingSession(owner: String) { val currentValue = currentAppNavigationState.value Timber.tag(loggerTag.value).d("Leaving session. Current state: $currentValue") + if (!currentValue.assertOwner(owner)) return currentAppNavigationState.value = AppNavigationState.Root } + + private fun AppNavigationState.assertOwner(owner: String): Boolean { + if (this.owner != owner) { + Timber.tag(loggerTag.value).d("Can't leave current state as the owner is not the same (current = ${this.owner}, new = $owner)") + return false + } + return true + } } diff --git a/services/appnavstate/impl/src/test/kotlin/io/element/android/services/appnavstate/impl/DefaultAppNavigationStateServiceTest.kt b/services/appnavstate/impl/src/test/kotlin/io/element/android/services/appnavstate/impl/DefaultAppNavigationStateServiceTest.kt index 9162bdaf75..e240ada2b2 100644 --- a/services/appnavstate/impl/src/test/kotlin/io/element/android/services/appnavstate/impl/DefaultAppNavigationStateServiceTest.kt +++ b/services/appnavstate/impl/src/test/kotlin/io/element/android/services/appnavstate/impl/DefaultAppNavigationStateServiceTest.kt @@ -30,23 +30,31 @@ import kotlinx.coroutines.test.runTest import org.junit.Assert.assertThrows import org.junit.Test +private const val aSessionOwner = "aSessionOwner" +private const val aSpaceOwner = "aSpaceOwner" +private const val aRoomOwner = "aRoomOwner" +private const val aThreadOwner = "aThreadOwner" + class DefaultAppNavigationStateServiceTest { @Test fun testNavigation() = runTest { val service = DefaultAppNavigationStateService() - service.onNavigateToSession(A_SESSION_ID) - service.onNavigateToSpace(A_SPACE_ID) - service.onNavigateToRoom(A_ROOM_ID) - service.onNavigateToThread(A_THREAD_ID) + service.onNavigateToSession(aSessionOwner, A_SESSION_ID) + service.onNavigateToSpace(aSpaceOwner, A_SPACE_ID) + service.onNavigateToRoom(aRoomOwner, A_ROOM_ID) + service.onNavigateToThread(aThreadOwner, A_THREAD_ID) assertThat(service.appNavigationStateFlow.first()).isEqualTo( AppNavigationState.Thread( - A_THREAD_ID, + aThreadOwner, A_THREAD_ID, AppNavigationState.Room( + aRoomOwner, A_ROOM_ID, AppNavigationState.Space( + aSpaceOwner, A_SPACE_ID, AppNavigationState.Session( + aSessionOwner, A_SESSION_ID ) ) @@ -58,6 +66,6 @@ class DefaultAppNavigationStateServiceTest { @Test fun testFailure() = runTest { val service = DefaultAppNavigationStateService() - assertThrows(IllegalStateException::class.java) { service.onNavigateToSpace(A_SPACE_ID) } + assertThrows(IllegalStateException::class.java) { service.onNavigateToSpace(aSpaceOwner, A_SPACE_ID) } } }