From 7ea2f28d96369adf93f4ea87add25ac5810c9316 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 11:26:13 +0200 Subject: [PATCH 1/5] More log about Node lifecycle. Will help to track user navigation. --- .../android/appnav/NotLoggedInFlowNode.kt | 9 ------ .../libraries/architecture/BackstackNode.kt | 14 ++++++++- .../libraries/architecture/LifecycleExt.kt | 30 +++++++++++++++++++ 3 files changed, 43 insertions(+), 10 deletions(-) create mode 100644 libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/LifecycleExt.kt diff --git a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt index 4b89b442d7..f9a0c00ec5 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt @@ -20,7 +20,6 @@ import android.os.Parcelable import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier import com.bumble.appyx.core.composable.Children -import com.bumble.appyx.core.lifecycle.subscribe import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin @@ -35,7 +34,6 @@ import io.element.android.libraries.architecture.BackstackNode import io.element.android.libraries.architecture.animation.rememberDefaultTransitionHandler import io.element.android.libraries.di.AppScope import kotlinx.parcelize.Parcelize -import timber.log.Timber @ContributesNode(AppScope::class) class NotLoggedInFlowNode @AssistedInject constructor( @@ -51,13 +49,6 @@ class NotLoggedInFlowNode @AssistedInject constructor( buildContext = buildContext, plugins = plugins, ) { - init { - lifecycle.subscribe( - onCreate = { Timber.v("OnCreate") }, - onDestroy = { Timber.v("OnDestroy") } - ) - } - sealed interface NavTarget : Parcelable { @Parcelize object OnBoarding : NavTarget diff --git a/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/BackstackNode.kt b/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/BackstackNode.kt index ec607e9281..ec22c5e21f 100644 --- a/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/BackstackNode.kt +++ b/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/BackstackNode.kt @@ -19,6 +19,7 @@ package io.element.android.libraries.architecture import androidx.compose.runtime.Stable import com.bumble.appyx.core.children.ChildEntry import com.bumble.appyx.core.modality.BuildContext +import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.node.ParentNode import com.bumble.appyx.core.plugin.Plugin import com.bumble.appyx.navmodel.backstack.BackStack @@ -39,4 +40,15 @@ abstract class BackstackNode( buildContext = buildContext, plugins = plugins, childKeepMode = childKeepMode, -) +) { + override fun onBuilt() { + super.onBuilt() + lifecycle.logLifecycle(this::class.java.simpleName) + whenChildAttached { _, child -> + // BackstackNode will be logged by their parent. + if (child !is BackstackNode<*>) { + child.lifecycle.logLifecycle(child::class.java.simpleName) + } + } + } +} diff --git a/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/LifecycleExt.kt b/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/LifecycleExt.kt new file mode 100644 index 0000000000..7c42c4cfe3 --- /dev/null +++ b/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/LifecycleExt.kt @@ -0,0 +1,30 @@ +/* + * Copyright (c) 2023 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.libraries.architecture + +import androidx.lifecycle.Lifecycle +import com.bumble.appyx.core.lifecycle.subscribe +import timber.log.Timber + +fun Lifecycle.logLifecycle(name: String) { + subscribe( + onCreate = { Timber.tag("Lifecycle").d("onCreate $name") }, + onPause = { Timber.tag("Lifecycle").d("onPause $name") }, + onResume = { Timber.tag("Lifecycle").d("onResume $name") }, + onDestroy = { Timber.tag("Lifecycle").d("onDestroy $name") }, + ) +} From 93c918ea2aa3cc709ff059b7aacbdbab52e620a8 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 12:05:31 +0200 Subject: [PATCH 2/5] Fix image not loading after a clear cache. --- .../io/element/android/appnav/LoggedInFlowNode.kt | 2 -- .../element/android/appnav/NotLoggedInFlowNode.kt | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 2 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 bc73efde71..3380dd91db 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt @@ -154,8 +154,6 @@ class LoggedInFlowNode @AssistedInject constructor( syncService.stopSync() }, onDestroy = { - val imageLoaderFactory = bindings().notLoggedInImageLoaderFactory() - Coil.setImageLoader(imageLoaderFactory) plugins().forEach { it.onFlowReleased(id, inputs.matrixClient) } appNavigationStateService.onLeavingSpace(id) appNavigationStateService.onLeavingSession(id) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt index f9a0c00ec5..ac17f6ad00 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt @@ -19,7 +19,9 @@ package io.element.android.appnav import android.os.Parcelable import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier +import coil.Coil import com.bumble.appyx.core.composable.Children +import com.bumble.appyx.core.lifecycle.subscribe import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin @@ -32,7 +34,9 @@ import io.element.android.features.login.api.LoginEntryPoint import io.element.android.features.onboarding.api.OnBoardingEntryPoint import io.element.android.libraries.architecture.BackstackNode import io.element.android.libraries.architecture.animation.rememberDefaultTransitionHandler +import io.element.android.libraries.architecture.bindings import io.element.android.libraries.di.AppScope +import io.element.android.libraries.matrix.ui.di.MatrixUIBindings import kotlinx.parcelize.Parcelize @ContributesNode(AppScope::class) @@ -49,6 +53,16 @@ class NotLoggedInFlowNode @AssistedInject constructor( buildContext = buildContext, plugins = plugins, ) { + override fun onBuilt() { + super.onBuilt() + lifecycle.subscribe( + onCreate = { + val imageLoaderFactory = bindings().notLoggedInImageLoaderFactory() + Coil.setImageLoader(imageLoaderFactory) + }, + ) + } + sealed interface NavTarget : Parcelable { @Parcelize object OnBoarding : NavTarget From a9720e36c1c5895007b3e73bd7d470d84b326c19 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 12:23:46 +0200 Subject: [PATCH 3/5] Let RootFlowNode manage MatrixClientsHolder save and restoration. --- .../io/element/android/x/MainActivity.kt | 8 +---- .../io/element/android/x/di/AppBindings.kt | 2 -- .../io/element/android/appnav/RootFlowNode.kt | 7 +++++ .../android/appnav/di/MatrixClientsHolder.kt | 31 +++++++++---------- 4 files changed, 23 insertions(+), 25 deletions(-) diff --git a/app/src/main/kotlin/io/element/android/x/MainActivity.kt b/app/src/main/kotlin/io/element/android/x/MainActivity.kt index be9f6134ba..cf37b22159 100644 --- a/app/src/main/kotlin/io/element/android/x/MainActivity.kt +++ b/app/src/main/kotlin/io/element/android/x/MainActivity.kt @@ -52,8 +52,7 @@ class MainActivity : NodeComponentActivity() { Timber.tag(loggerTag.value).w("onCreate, with savedInstanceState: ${savedInstanceState != null}") installSplashScreen() super.onCreate(savedInstanceState) - appBindings = bindings() - appBindings.matrixClientsHolder().restore(savedInstanceState) + appBindings = bindings() WindowCompat.setDecorFitsSystemWindows(window, false) setContent { MainContent(appBindings) @@ -125,9 +124,4 @@ class MainActivity : NodeComponentActivity() { super.onDestroy() Timber.tag(loggerTag.value).w("onDestroy") } - - override fun onSaveInstanceState(outState: Bundle) { - super.onSaveInstanceState(outState) - bindings().matrixClientsHolder().onSaveInstanceState(outState) - } } diff --git a/app/src/main/kotlin/io/element/android/x/di/AppBindings.kt b/app/src/main/kotlin/io/element/android/x/di/AppBindings.kt index 59f7e98d20..4d75d8601e 100644 --- a/app/src/main/kotlin/io/element/android/x/di/AppBindings.kt +++ b/app/src/main/kotlin/io/element/android/x/di/AppBindings.kt @@ -17,13 +17,11 @@ package io.element.android.x.di import com.squareup.anvil.annotations.ContributesTo -import io.element.android.appnav.di.MatrixClientsHolder import io.element.android.libraries.designsystem.utils.SnackbarDispatcher import io.element.android.libraries.di.AppScope @ContributesTo(AppScope::class) interface AppBindings { - fun matrixClientsHolder(): MatrixClientsHolder fun mainDaggerComponentOwner(): MainDaggerComponentsOwner fun snackbarDispatcher(): SnackbarDispatcher } 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 8803d574d9..4150bcaebc 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt @@ -30,6 +30,7 @@ import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.node.node import com.bumble.appyx.core.plugin.Plugin import com.bumble.appyx.core.plugin.plugins +import com.bumble.appyx.core.state.MutableSavedStateMap import com.bumble.appyx.navmodel.backstack.BackStack import com.bumble.appyx.navmodel.backstack.operation.pop import com.bumble.appyx.navmodel.backstack.operation.push @@ -90,10 +91,16 @@ class RootFlowNode @AssistedInject constructor( ) { override fun onBuilt() { + matrixClientsHolder.restore(buildContext.savedStateMap) super.onBuilt() observeLoggedInState() } + override fun onSaveInstanceState(state: MutableSavedStateMap) { + super.onSaveInstanceState(state) + matrixClientsHolder.save(state) + } + private fun observeLoggedInState() { combine( cacheService.onClearedCacheEventFlow(), diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt index 092cffd630..24c082627f 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt @@ -16,13 +16,11 @@ package io.element.android.appnav.di -import android.os.Bundle -import io.element.android.libraries.di.AppScope -import io.element.android.libraries.di.SingleIn +import com.bumble.appyx.core.state.MutableSavedStateMap +import com.bumble.appyx.core.state.SavedStateMap import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService import io.element.android.libraries.matrix.api.core.SessionId -import io.element.android.libraries.matrix.api.core.UserId import kotlinx.coroutines.runBlocking import timber.log.Timber import java.util.concurrent.ConcurrentHashMap @@ -30,7 +28,6 @@ import javax.inject.Inject private const val SAVE_INSTANCE_KEY = "io.element.android.x.di.MatrixClientsHolder.SaveInstanceKey" -@SingleIn(AppScope::class) class MatrixClientsHolder @Inject constructor(private val authenticationService: MatrixAuthenticationService) { private val sessionIdsToMatrixClient = ConcurrentHashMap() @@ -55,16 +52,18 @@ class MatrixClientsHolder @Inject constructor(private val authenticationService: return sessionIdsToMatrixClient[sessionId] } - @Suppress("DEPRECATION") - fun restore(savedInstanceState: Bundle?) { - if (savedInstanceState == null || sessionIdsToMatrixClient.isNotEmpty()) return - val userIds = savedInstanceState.getSerializable(SAVE_INSTANCE_KEY) as? Array - if (userIds.isNullOrEmpty()) return + @Suppress("UNCHECKED_CAST") + fun restore(state: SavedStateMap?) { + if (state == null || sessionIdsToMatrixClient.isNotEmpty()) return Unit.also { + Timber.w("Restore with non-empty map") + } + val sessionIds = state[SAVE_INSTANCE_KEY] as? Array + if (sessionIds.isNullOrEmpty()) return // Not ideal but should only happens in case of process recreation. This ensure we restore all the active sessions before restoring the node graphs. runBlocking { - userIds.forEach { userId -> - Timber.v("Restore matrix session: $userId") - authenticationService.restoreSession(userId) + sessionIds.forEach { sessionId -> + Timber.d("Restore matrix session: $sessionId") + authenticationService.restoreSession(sessionId) .onSuccess { matrixClient -> add(matrixClient) } @@ -75,9 +74,9 @@ class MatrixClientsHolder @Inject constructor(private val authenticationService: } } - fun onSaveInstanceState(outState: Bundle) { + fun save(state: MutableSavedStateMap) { val sessionKeys = sessionIdsToMatrixClient.keys.toTypedArray() - Timber.v("Save matrix session keys = $sessionKeys") - outState.putSerializable(SAVE_INSTANCE_KEY, sessionKeys) + Timber.d("Save matrix session keys = $sessionKeys") + state[SAVE_INSTANCE_KEY] = sessionKeys } } From c0d2de26dbf17159f92318d01162bce9f37ae204 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 12:39:41 +0200 Subject: [PATCH 4/5] Improve logs. --- .../io/element/android/appnav/di/MatrixClientsHolder.kt | 4 +++- .../libraries/matrix/api/room/RoomSummaryDataSource.kt | 3 ++- .../android/libraries/matrix/impl/room/RoomListExtensions.kt | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt index 24c082627f..bbb14d4d29 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt @@ -54,10 +54,12 @@ class MatrixClientsHolder @Inject constructor(private val authenticationService: @Suppress("UNCHECKED_CAST") fun restore(state: SavedStateMap?) { + Timber.d("Restore state") if (state == null || sessionIdsToMatrixClient.isNotEmpty()) return Unit.also { Timber.w("Restore with non-empty map") } val sessionIds = state[SAVE_INSTANCE_KEY] as? Array + Timber.d("Restore matrix session keys = ${sessionIds?.map { it.value }}") if (sessionIds.isNullOrEmpty()) return // Not ideal but should only happens in case of process recreation. This ensure we restore all the active sessions before restoring the node graphs. runBlocking { @@ -76,7 +78,7 @@ class MatrixClientsHolder @Inject constructor(private val authenticationService: fun save(state: MutableSavedStateMap) { val sessionKeys = sessionIdsToMatrixClient.keys.toTypedArray() - Timber.d("Save matrix session keys = $sessionKeys") + Timber.d("Save matrix session keys = ${sessionKeys.map { it.value }}") state[SAVE_INSTANCE_KEY] = sessionKeys } } diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/RoomSummaryDataSource.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/RoomSummaryDataSource.kt index e6c7b34056..d677d56ed9 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/RoomSummaryDataSource.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/RoomSummaryDataSource.kt @@ -38,12 +38,13 @@ interface RoomSummaryDataSource { suspend fun RoomSummaryDataSource.awaitAllRoomsAreLoaded(timeout: Duration = Duration.INFINITE) { try { + Timber.d("awaitAllRoomsAreLoaded: wait") withTimeout(timeout) { allRoomsLoadingState().firstOrNull { it is RoomSummaryDataSource.LoadingState.Loaded } } } catch (timeoutException: TimeoutCancellationException) { - Timber.v("AwaitAllRooms: no response after $timeout") + Timber.d("awaitAllRoomsAreLoaded: no response after $timeout") } } diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomListExtensions.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomListExtensions.kt index 824d477fd3..84c4eeaefb 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomListExtensions.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomListExtensions.kt @@ -62,7 +62,7 @@ fun RoomListService.roomOrNull(roomId: String): RoomListItem? { return try { room(roomId) } catch (exception: RoomListException) { - Timber.e(exception, "Failed finding room with id=$roomId") + Timber.d(exception, "Failed finding room with id=$roomId.") return null } } From 8893a8bfdce6aef9b8a8c3c0808e1fd92e63049c Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 15:32:00 +0200 Subject: [PATCH 5/5] Fix crash at first startup. Inject NotLoggedInImageLoaderFactory directly to NotLoggedInFlowNode --- .../io/element/android/appnav/NotLoggedInFlowNode.kt | 7 +++---- .../android/libraries/matrix/ui/di/MatrixUIBindings.kt | 2 -- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt index ac17f6ad00..1ed1aec678 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt @@ -34,9 +34,8 @@ import io.element.android.features.login.api.LoginEntryPoint import io.element.android.features.onboarding.api.OnBoardingEntryPoint import io.element.android.libraries.architecture.BackstackNode import io.element.android.libraries.architecture.animation.rememberDefaultTransitionHandler -import io.element.android.libraries.architecture.bindings import io.element.android.libraries.di.AppScope -import io.element.android.libraries.matrix.ui.di.MatrixUIBindings +import io.element.android.libraries.matrix.ui.media.NotLoggedInImageLoaderFactory import kotlinx.parcelize.Parcelize @ContributesNode(AppScope::class) @@ -45,6 +44,7 @@ class NotLoggedInFlowNode @AssistedInject constructor( @Assisted plugins: List, private val onBoardingEntryPoint: OnBoardingEntryPoint, private val loginEntryPoint: LoginEntryPoint, + private val notLoggedInImageLoaderFactory: NotLoggedInImageLoaderFactory, ) : BackstackNode( backstack = BackStack( initialElement = NavTarget.OnBoarding, @@ -57,8 +57,7 @@ class NotLoggedInFlowNode @AssistedInject constructor( super.onBuilt() lifecycle.subscribe( onCreate = { - val imageLoaderFactory = bindings().notLoggedInImageLoaderFactory() - Coil.setImageLoader(imageLoaderFactory) + Coil.setImageLoader(notLoggedInImageLoaderFactory) }, ) } diff --git a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixUIBindings.kt b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixUIBindings.kt index a5734f5b9c..919971b307 100644 --- a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixUIBindings.kt +++ b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixUIBindings.kt @@ -19,10 +19,8 @@ package io.element.android.libraries.matrix.ui.di import com.squareup.anvil.annotations.ContributesTo import io.element.android.libraries.di.SessionScope import io.element.android.libraries.matrix.ui.media.LoggedInImageLoaderFactory -import io.element.android.libraries.matrix.ui.media.NotLoggedInImageLoaderFactory @ContributesTo(SessionScope::class) interface MatrixUIBindings { fun loggedInImageLoaderFactory(): LoggedInImageLoaderFactory - fun notLoggedInImageLoaderFactory(): NotLoggedInImageLoaderFactory }