diff --git a/appnav/src/main/kotlin/io/element/android/appnav/BackstackExt.kt b/appnav/src/main/kotlin/io/element/android/appnav/BackstackExt.kt index 1185016c0b..1b9d1b56cb 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/BackstackExt.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/BackstackExt.kt @@ -8,9 +8,15 @@ package io.element.android.appnav +import com.bumble.appyx.core.navigation.NavKey import com.bumble.appyx.navmodel.backstack.BackStack +import com.bumble.appyx.navmodel.backstack.BackStackElement +import com.bumble.appyx.navmodel.backstack.BackStackElements +import com.bumble.appyx.navmodel.backstack.operation.BackStackOperation import com.bumble.appyx.navmodel.backstack.operation.NewRoot import com.bumble.appyx.navmodel.backstack.operation.Remove +import kotlinx.parcelize.Parcelize +import kotlinx.parcelize.RawValue /** * Don't process NewRoot if the nav target already exists in the stack. @@ -32,3 +38,45 @@ fun BackStack.removeLast(element: T) { } ?: return accept(Remove(lastExpectedNavElement.key)) } + +/** + * Replace the root element of the backstack, and keep the elements on top of it, contrary to [safeRoot] which removes all the elements. + */ +fun BackStack.replaceRoot(element: T) { + accept(ReplaceRoot(element)) +} + +/** + * Replace the root element of the backstack, keeping the elements on top of it. + * + * Operation: + * + * [A, B, C] + ReplaceRoot(D) = [D, B, C] + * [A] + ReplaceRoot(D) = [D] + */ +@Parcelize +internal data class ReplaceRoot( + private val element: @RawValue T +) : BackStackOperation { + override fun isApplicable(elements: BackStackElements): Boolean = + elements.rootElement()?.key?.navTarget != element + + override fun invoke(elements: BackStackElements): BackStackElements { + val root = elements.rootElement() ?: return elements + if (root.targetState == BackStack.State.ACTIVE) { + // Nothing on top of the root, so this is the same as NewRoot + return NewRoot(element).invoke(elements) + } + // The root is stashed below other elements: drop it, as Remove does for a non active element, + // and add the new root, stashed, in place of it. + val newRoot = BackStackElement( + key = NavKey(element), + fromState = BackStack.State.STASHED, + targetState = BackStack.State.STASHED, + operation = this, + ) + return elements.map { if (it == root) newRoot else it } + } + + private fun BackStackElements.rootElement() = firstOrNull { it.targetState != BackStack.State.DESTROYED } +} diff --git a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInAppScopeFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInAppScopeFlowNode.kt index 32e1723d99..ee8398186d 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInAppScopeFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInAppScopeFlowNode.kt @@ -33,8 +33,6 @@ import io.element.android.libraries.architecture.createNode import io.element.android.libraries.architecture.inputs import io.element.android.libraries.di.DependencyInjectionGraphOwner import io.element.android.libraries.matrix.api.MatrixClient -import io.element.android.libraries.matrix.api.core.RoomId -import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.matrixmedia.api.ImageLoaderHolder import kotlinx.parcelize.Parcelize @@ -61,7 +59,6 @@ class LoggedInAppScopeFlowNode( interface Callback : Plugin { fun navigateToBugReport() fun navigateToAddAccount() - fun switchAccountAndOpenRoom(sessionId: SessionId, roomId: RoomId?) } private val callback: Callback = callback() @@ -94,10 +91,6 @@ class LoggedInAppScopeFlowNode( override fun navigateToAddAccount() { callback.navigateToAddAccount() } - - override fun switchAccountAndOpenRoom(sessionId: SessionId, roomId: RoomId?) { - callback.switchAccountAndOpenRoom(sessionId, roomId) - } } return createNode(buildContext, listOf(callback)) } 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 a361576050..270ea6b052 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt @@ -67,8 +67,6 @@ import io.element.android.features.preferences.api.PreferencesEntryPoint import io.element.android.features.roomdirectory.api.RoomDescription import io.element.android.features.roomdirectory.api.RoomDirectoryEntryPoint import io.element.android.features.securebackup.api.SecureBackupEntryPoint -import io.element.android.features.share.api.ShareEntryPoint -import io.element.android.features.share.api.ShareIntentData import io.element.android.features.startchat.api.StartChatEntryPoint import io.element.android.features.userprofile.api.UserProfileEntryPoint import io.element.android.features.verifysession.api.IncomingVerificationEntryPoint @@ -88,7 +86,6 @@ import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.core.EventId import io.element.android.libraries.matrix.api.core.RoomId import io.element.android.libraries.matrix.api.core.RoomIdOrAlias -import io.element.android.libraries.matrix.api.core.SessionId 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 @@ -138,7 +135,6 @@ class LoggedInFlowNode( private val sessionCoroutineScope: CoroutineScope, private val ftueService: FtueService, private val roomDirectoryEntryPoint: RoomDirectoryEntryPoint, - private val shareEntryPoint: ShareEntryPoint, private val matrixClient: MatrixClient, private val sendingQueue: SendQueues, private val nativeCallHost: NativeCallHost, @@ -174,7 +170,6 @@ class LoggedInFlowNode( interface Callback : Plugin { fun navigateToBugReport() fun navigateToAddAccount() - fun switchAccountAndOpenRoom(sessionId: SessionId, roomId: RoomId?) } private val callback: Callback = callback() @@ -296,9 +291,6 @@ class LoggedInFlowNode( @Parcelize data object RoomDirectory : NavTarget - @Parcelize - data class IncomingShare(val shareIntentData: ShareIntentData) : NavTarget - @Parcelize data class IncomingVerificationRequest(val data: VerificationRequest.Incoming) : NavTarget } @@ -570,36 +562,6 @@ class LoggedInFlowNode( }, ) } - is NavTarget.IncomingShare -> { - shareEntryPoint.createNode( - parentNode = this, - buildContext = buildContext, - params = ShareEntryPoint.Params(shareIntentData = navTarget.shareIntentData), - callback = object : ShareEntryPoint.Callback { - override fun onDone(sessionId: SessionId, roomIds: List) { - // Remove the incoming share screen - backstack.pop() - - if (sessionId != matrixClient.sessionId) { - // The data has been shared using another session, switch to it - callback.switchAccountAndOpenRoom(sessionId, roomIds.singleOrNull()) - return - } - - // Navigate to the room if the text/media was shared to a single one - roomIds.singleOrNull()?.let { roomId -> - lifecycleScope.launch { - // Wait until the incoming share screen is removed - backstack.elements.first { it.lastOrNull()?.key?.navTarget !is NavTarget.IncomingShare } - - // Then attach the room - attachRoom(roomId.toRoomIdOrAlias(), clearBackstack = false) - } - } - } - }, - ) - } is NavTarget.IncomingVerificationRequest -> { incomingVerificationEntryPoint.createNode( parentNode = this, @@ -659,17 +621,6 @@ class LoggedInFlowNode( } } - internal suspend fun attachIncomingShare(shareIntentData: ShareIntentData) { - waitForNavTargetAttached { navTarget -> - navTarget is NavTarget.Home - } - attachChild { - backstack.push( - NavTarget.IncomingShare(shareIntentData) - ) - } - } - @Composable override fun View(modifier: Modifier) { val colors by remember { 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 934964e447..5ff887b209 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt @@ -47,6 +47,7 @@ import io.element.android.features.announcement.api.AnnouncementService import io.element.android.features.login.api.LoginParams import io.element.android.features.login.api.accesscontrol.AccountProviderAccessControl import io.element.android.features.rageshake.api.bugreport.BugReportEntryPoint +import io.element.android.features.share.api.ShareEntryPoint import io.element.android.features.share.api.ShareIntentData import io.element.android.features.signedout.api.SignedOutEntryPoint import io.element.android.libraries.accountselect.api.AccountSelectEntryPoint @@ -112,6 +113,7 @@ class RootFlowNode( private val analyticsService: AnalyticsService, private val analyticsColdStartWatcher: AnalyticsColdStartWatcher, private val incomingVerificationRequestObserver: IncomingVerificationRequestObserver, + private val shareEntryPoint: ShareEntryPoint, @AppCoroutineScope private val appCoroutineScope: CoroutineScope, ) : BaseFlowNode( backstack = BackStack( @@ -216,7 +218,7 @@ class RootFlowNode( private fun switchToLoggedInFlow(sessionId: SessionId, navId: Int) { pendingLoginParams = null - backstack.safeRoot(NavTarget.LoggedInFlow(sessionId, navId)) + setRoot(NavTarget.LoggedInFlow(sessionId, navId)) restoreAllSessions() } @@ -286,7 +288,20 @@ class RootFlowNode( private fun switchToSignedOutFlow(sessionId: SessionId) { pendingLoginParams = null - backstack.safeRoot(NavTarget.SignedOutFlow(sessionId)) + setRoot(NavTarget.SignedOutFlow(sessionId)) + } + + /** + * Set the root of the backstack. If an incoming share is displayed, keep it on top of the new root, so that the user can + * finish sharing, for instance with another account if the current one has been signed out. + */ + private fun setRoot(navTarget: NavTarget) { + val hasIncomingShare = backstack.elements.value.any { it.key.navTarget is NavTarget.IncomingShare } + if (hasIncomingShare) { + backstack.replaceRoot(navTarget) + } else { + backstack.safeRoot(navTarget) + } } private suspend fun restoreSessionIfNeeded( @@ -367,6 +382,11 @@ class RootFlowNode( @Parcelize data object BugReport : NavTarget + @Parcelize data class IncomingShare( + val sessionId: SessionId, + val shareIntentData: ShareIntentData, + ) : NavTarget + @Parcelize data class OtherAccountIncomingVerificationRequest( val sessionId: SessionId, val verificationRequest: VerificationRequest.Incoming, @@ -389,15 +409,6 @@ class RootFlowNode( override fun navigateToAddAccount() { backstack.push(NavTarget.NotLoggedInFlow(null)) } - - override fun switchAccountAndOpenRoom(sessionId: SessionId, roomId: RoomId?) { - lifecycleScope.launch { - val loggedInFlowNode = attachSession(sessionId) - roomId?.let { - loggedInFlowNode.attachRoom(it.toRoomIdOrAlias(), clearBackstack = false) - } - } - } } val savedNavState = extractSavedStateForNavTarget(navTarget, this.buildContext.savedStateMap) val buildContext = if (savedNavState != null) { @@ -451,6 +462,35 @@ class RootFlowNode( } createNode(buildContext, plugins = listOf(inputs, callback)) } + is NavTarget.IncomingShare -> { + val callback = object : ShareEntryPoint.Callback { + override fun onDone(sessionId: SessionId, roomIds: List) { + // Remove the incoming share screen + backstack.pop() + lifecycleScope.launch { + // Switch to the session which has been used to share the data, if necessary, + // and navigate to the room if the data was shared to a single one. + val loggedInFlowNode = attachSession(sessionId) + roomIds.singleOrNull()?.let { roomId -> + loggedInFlowNode.attachRoom(roomId.toRoomIdOrAlias(), clearBackstack = false) + } + } + } + + override fun onCancel() { + backstack.pop() + } + } + shareEntryPoint.createNode( + parentNode = this, + buildContext = buildContext, + params = ShareEntryPoint.Params( + sessionId = navTarget.sessionId, + shareIntentData = navTarget.shareIntentData, + ), + callback = callback, + ) + } NavTarget.BugReport -> { val callback = object : BugReportEntryPoint.Callback { override fun onDone() { @@ -542,9 +582,11 @@ class RootFlowNode( // No session, open login switchToNotLoggedInFlow(null) } else { - // wait for the current session to be restored, then attach the incoming share node. - // In case of multiple accounts, the user can switch account from the room select screen. - attachSession(latestSessionId).attachIncomingShare(shareIntentData) + // Wait for the current session to be restored, then display the incoming share on top of it. + // The incoming share does not belong to the current session: in case of multiple accounts, the user can switch account + // from the room select screen, and the share is kept if the current session is signed out in the meantime. + attachSession(latestSessionId) + backstack.push(NavTarget.IncomingShare(latestSessionId, shareIntentData)) } } diff --git a/appnav/src/test/kotlin/io/element/android/appnav/BackstackExtTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/BackstackExtTest.kt new file mode 100644 index 0000000000..87173022a1 --- /dev/null +++ b/appnav/src/test/kotlin/io/element/android/appnav/BackstackExtTest.kt @@ -0,0 +1,56 @@ +/* + * Copyright (c) 2026 Element Creations 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 com.bumble.appyx.navmodel.backstack.BackStack +import com.bumble.appyx.navmodel.backstack.operation.pop +import com.bumble.appyx.navmodel.backstack.operation.push +import com.google.common.truth.Truth.assertThat +import org.junit.Test + +class BackstackExtTest { + @Test + fun `replaceRoot without element on top replaces the root`() { + val backStack = BackStack(initialElement = "A", savedStateMap = null) + backStack.replaceRoot("D") + assertThat(backStack.navTargets()).containsExactly("D") + assertThat(backStack.activeTarget()).isEqualTo("D") + } + + @Test + fun `replaceRoot with elements on top replaces the root and keeps the elements on top`() { + val backStack = BackStack(initialElement = "A", savedStateMap = null) + backStack.push("B") + backStack.push("C") + backStack.replaceRoot("D") + assertThat(backStack.navTargets()).containsExactly("D", "B", "C").inOrder() + assertThat(backStack.activeTarget()).isEqualTo("C") + // When the elements on top are removed, the new root is displayed + backStack.pop() + backStack.pop() + assertThat(backStack.activeTarget()).isEqualTo("D") + } + + @Test + fun `replaceRoot with the current root does nothing`() { + val backStack = BackStack(initialElement = "A", savedStateMap = null) + backStack.push("B") + backStack.replaceRoot("A") + assertThat(backStack.navTargets()).containsExactly("A", "B").inOrder() + assertThat(backStack.activeTarget()).isEqualTo("B") + } + + private fun BackStack.navTargets() = elements.value + .filter { it.targetState != BackStack.State.DESTROYED } + .map { it.key.navTarget } + + private fun BackStack.activeTarget() = elements.value + .single { it.targetState == BackStack.State.ACTIVE } + .key + .navTarget +} diff --git a/appnav/src/test/kotlin/io/element/android/appnav/RootFlowNodeTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/RootFlowNodeTest.kt index 4a20341212..d9b48c2ade 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/RootFlowNodeTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/RootFlowNodeTest.kt @@ -14,6 +14,7 @@ import android.net.Uri import androidx.arch.core.executor.testing.InstantTaskExecutorRule import com.bumble.appyx.core.modality.AncestryInfo import com.bumble.appyx.core.modality.BuildContext +import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.node.node import com.bumble.appyx.navmodel.backstack.activeElement import com.bumble.appyx.testing.junit4.util.MainDispatcherRule @@ -34,6 +35,7 @@ import io.element.android.features.login.test.FakeLoginIntentResolver import io.element.android.features.login.test.accesscontrol.FakeAccountProviderAccessControl import io.element.android.features.preferences.test.FakeCacheService import io.element.android.features.rageshake.test.FakeBugReportEntryPoint +import io.element.android.features.share.api.ShareEntryPoint import io.element.android.features.share.test.FakeShareIntentHandler import io.element.android.features.signedout.test.FakeSignedOutEntryPoint import io.element.android.libraries.accountselect.test.FakeAccountSelectEntryPoint @@ -52,6 +54,7 @@ import io.element.android.services.analytics.test.FakeAnalyticsService import io.element.android.services.analytics.test.watchers.FakeAnalyticsColdStartWatcher import io.element.android.services.apperror.test.FakeAppErrorStateService import io.element.android.services.appnavstate.test.FakeAppForegroundStateService +import io.element.android.tests.testutils.lambda.lambdaError import io.element.android.tests.testutils.node.FakeNodeFactoriesBindings import io.element.android.tests.testutils.node.FakeParentNode import io.element.android.tests.testutils.presenter.NotUsedPresenter @@ -192,6 +195,14 @@ class RootFlowNodeTest : RobolectricTest() { appForegroundStateService = FakeAppForegroundStateService(), ), appCoroutineScope = backgroundScope, + shareEntryPoint = object : ShareEntryPoint { + override fun createNode( + parentNode: Node, + buildContext: BuildContext, + params: ShareEntryPoint.Params, + callback: ShareEntryPoint.Callback, + ): Node = lambdaError() + }, ) } } diff --git a/features/share/api/src/main/kotlin/io/element/android/features/share/api/ShareEntryPoint.kt b/features/share/api/src/main/kotlin/io/element/android/features/share/api/ShareEntryPoint.kt index cf6ae2c219..3ceafc4b0b 100644 --- a/features/share/api/src/main/kotlin/io/element/android/features/share/api/ShareEntryPoint.kt +++ b/features/share/api/src/main/kotlin/io/element/android/features/share/api/ShareEntryPoint.kt @@ -16,7 +16,14 @@ import io.element.android.libraries.matrix.api.core.RoomId import io.element.android.libraries.matrix.api.core.SessionId interface ShareEntryPoint : FeatureEntryPoint { - data class Params(val shareIntentData: ShareIntentData) + /** + * @param sessionId the session to use initially. The user can select another session. + * @param shareIntentData the data to share. + */ + data class Params( + val sessionId: SessionId, + val shareIntentData: ShareIntentData, + ) fun createNode( parentNode: Node, @@ -27,10 +34,15 @@ interface ShareEntryPoint : FeatureEntryPoint { interface Callback : Plugin { /** - * Called when the share is done, or cancelled. + * Called when the data has been shared. * @param sessionId the session which has been used to share the data. It can be different from the current session. - * @param roomIds the rooms the data has been shared to, empty if the share has been cancelled. + * @param roomIds the rooms the data has been shared to. */ fun onDone(sessionId: SessionId, roomIds: List) + + /** + * Called when the user cancels the share. + */ + fun onCancel() } } diff --git a/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/DefaultShareEntryPoint.kt b/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/DefaultShareEntryPoint.kt index 98d4acc472..3ebaf7a753 100644 --- a/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/DefaultShareEntryPoint.kt +++ b/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/DefaultShareEntryPoint.kt @@ -10,12 +10,12 @@ package io.element.android.features.share.impl import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node +import dev.zacsweers.metro.AppScope import dev.zacsweers.metro.ContributesBinding import io.element.android.features.share.api.ShareEntryPoint import io.element.android.libraries.architecture.createNode -import io.element.android.libraries.di.SessionScope -@ContributesBinding(SessionScope::class) +@ContributesBinding(AppScope::class) class DefaultShareEntryPoint : ShareEntryPoint { override fun createNode( parentNode: Node, @@ -26,7 +26,10 @@ class DefaultShareEntryPoint : ShareEntryPoint { return parentNode.createNode( buildContext = buildContext, plugins = listOf( - ShareNode.Inputs(shareIntentData = params.shareIntentData), + ShareNode.Inputs( + sessionId = params.sessionId, + shareIntentData = params.shareIntentData, + ), callback, ) ) diff --git a/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/ShareNode.kt b/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/ShareNode.kt index 8b44f2bd0f..dc659391dd 100644 --- a/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/ShareNode.kt +++ b/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/ShareNode.kt @@ -18,6 +18,7 @@ import com.bumble.appyx.core.navigation.model.permanent.PermanentNavModel import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.node.ParentNode import com.bumble.appyx.core.plugin.Plugin +import dev.zacsweers.metro.AppScope import dev.zacsweers.metro.Assisted import dev.zacsweers.metro.AssistedInject import io.element.android.annotations.ContributesNode @@ -26,21 +27,19 @@ import io.element.android.features.share.api.ShareIntentData import io.element.android.libraries.architecture.NodeInputs import io.element.android.libraries.architecture.callback import io.element.android.libraries.architecture.inputs -import io.element.android.libraries.di.SessionScope import io.element.android.libraries.matrix.api.core.RoomId import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.roomselect.api.RoomSelectEntryPoint import io.element.android.libraries.roomselect.api.RoomSelectMode import kotlinx.parcelize.Parcelize -@ContributesNode(SessionScope::class) +@ContributesNode(AppScope::class) @AssistedInject class ShareNode( @Assisted buildContext: BuildContext, @Assisted plugins: List, presenterFactory: SharePresenter.Factory, private val roomSelectEntryPoint: RoomSelectEntryPoint, - private val sessionId: SessionId, ) : ParentNode( navModel = PermanentNavModel( navTargets = setOf(NavTarget), @@ -52,10 +51,13 @@ class ShareNode( @Parcelize object NavTarget : Parcelable - data class Inputs(val shareIntentData: ShareIntentData) : NodeInputs + data class Inputs( + val sessionId: SessionId, + val shareIntentData: ShareIntentData, + ) : NodeInputs private val inputs = inputs() - private val presenter = presenterFactory.create(inputs.shareIntentData, sessionId) + private val presenter = presenterFactory.create(inputs.shareIntentData) private val callback: ShareEntryPoint.Callback = callback() override fun resolve(navTarget: NavTarget, buildContext: BuildContext): Node { @@ -65,7 +67,7 @@ class ShareNode( } override fun onCancel() { - callback.onDone(sessionId, emptyList()) + callback.onCancel() } } @@ -73,7 +75,7 @@ class ShareNode( parentNode = this, buildContext = buildContext, params = RoomSelectEntryPoint.Params( - sessionId = sessionId, + sessionId = inputs.sessionId, mode = RoomSelectMode.Share, maxNumberOfRooms = RoomSelectEntryPoint.DEFAULT_MAX_NUMBER_OF_ROOMS, ), @@ -92,7 +94,7 @@ class ShareNode( val state = presenter.present() ShareView( state = state, - onShareSuccess = { roomIds -> callback.onDone(state.sessionId, roomIds) }, + onShareSuccess = { result -> callback.onDone(result.sessionId, result.roomIds) }, ) } } diff --git a/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/SharePresenter.kt b/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/SharePresenter.kt index 905840d1df..91961afb6a 100644 --- a/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/SharePresenter.kt +++ b/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/SharePresenter.kt @@ -10,9 +10,7 @@ package io.element.android.features.share.impl import androidx.compose.runtime.Composable import androidx.compose.runtime.MutableState -import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf -import androidx.compose.runtime.setValue import dev.zacsweers.metro.Assisted import dev.zacsweers.metro.AssistedFactory import dev.zacsweers.metro.AssistedInject @@ -30,7 +28,6 @@ import kotlinx.coroutines.launch @AssistedInject class SharePresenter( @Assisted private val shareIntentData: ShareIntentData, - @Assisted initialSessionId: SessionId, @AppCoroutineScope private val appCoroutineScope: CoroutineScope, private val matrixClientProvider: MatrixClientProvider, @@ -38,24 +35,21 @@ class SharePresenter( ) : Presenter { @AssistedFactory fun interface Factory { - fun create(shareIntentData: ShareIntentData, initialSessionId: SessionId): SharePresenter + fun create(shareIntentData: ShareIntentData): SharePresenter } - private val shareActionState: MutableState>> = mutableStateOf(AsyncAction.Uninitialized) + private val shareActionState: MutableState> = mutableStateOf(AsyncAction.Uninitialized) /** - * The session the data is shared with, it can be updated if the user selects rooms from another session. + * Share the data to the rooms with [roomIds], which belong to the session [sessionId]. It can be another session than the current one. */ - private var targetSessionId by mutableStateOf(initialSessionId) - fun onRoomSelected(sessionId: SessionId, roomIds: List) { if (shareActionState.value.isLoading()) return - targetSessionId = sessionId appCoroutineScope.launch { suspend { val client = matrixClientProvider.getOrRestore(sessionId).getOrThrow() shareDataSender.send(client, shareIntentData, roomIds).getOrThrow() - roomIds + ShareResult(sessionId = sessionId, roomIds = roomIds) }.runCatchingUpdatingState(shareActionState) } } @@ -69,7 +63,6 @@ class SharePresenter( } return ShareState( - sessionId = targetSessionId, shareAction = shareActionState.value, eventSink = ::handleEvent, ) diff --git a/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/ShareState.kt b/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/ShareState.kt index 10046cf3b2..eb50410526 100644 --- a/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/ShareState.kt +++ b/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/ShareState.kt @@ -13,7 +13,16 @@ import io.element.android.libraries.matrix.api.core.RoomId import io.element.android.libraries.matrix.api.core.SessionId data class ShareState( - val sessionId: SessionId, - val shareAction: AsyncAction>, + val shareAction: AsyncAction, val eventSink: (ShareEvent) -> Unit ) + +/** + * The result of a successful share. + * @param sessionId the session which has been used to share the data, it can be another session than the current one. + * @param roomIds the rooms the data has been shared to. + */ +data class ShareResult( + val sessionId: SessionId, + val roomIds: List, +) diff --git a/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/ShareStatePreviewParam.kt b/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/ShareStatePreviewParam.kt index c765db9195..38ae8369df 100644 --- a/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/ShareStatePreviewParam.kt +++ b/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/ShareStatePreviewParam.kt @@ -21,9 +21,7 @@ open class ShareStatePreviewParam : PreviewParameterProvider { shareAction = AsyncAction.Loading, ), aShareState( - shareAction = AsyncAction.Success( - listOf(RoomId("!room2:domain")), - ) + shareAction = AsyncAction.Success(aShareResult()) ), aShareState( shareAction = AsyncAction.Failure(RuntimeException("error")), @@ -31,12 +29,18 @@ open class ShareStatePreviewParam : PreviewParameterProvider { ) } -fun aShareState( - sessionId: SessionId = SessionId("@alice:server.org"), - shareAction: AsyncAction> = AsyncAction.Uninitialized, +private fun aShareState( + shareAction: AsyncAction = AsyncAction.Uninitialized, eventSink: (ShareEvent) -> Unit = {} ) = ShareState( - sessionId = sessionId, shareAction = shareAction, eventSink = eventSink ) + +private fun aShareResult( + sessionId: SessionId = SessionId("@alice:server.org"), + roomIds: List = listOf(RoomId("!room2:domain")), +) = ShareResult( + sessionId = sessionId, + roomIds = roomIds, +) diff --git a/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/ShareView.kt b/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/ShareView.kt index 2516c31a66..f2d91972cf 100644 --- a/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/ShareView.kt +++ b/features/share/impl/src/main/kotlin/io/element/android/features/share/impl/ShareView.kt @@ -13,12 +13,11 @@ import androidx.compose.ui.tooling.preview.PreviewParameter import io.element.android.libraries.designsystem.components.async.AsyncActionView import io.element.android.libraries.designsystem.preview.ElementPreview import io.element.android.libraries.designsystem.preview.PreviewsDayNight -import io.element.android.libraries.matrix.api.core.RoomId @Composable fun ShareView( state: ShareState, - onShareSuccess: (List) -> Unit, + onShareSuccess: (ShareResult) -> Unit, ) { AsyncActionView( async = state.shareAction, diff --git a/features/share/impl/src/test/kotlin/io/element/android/features/share/impl/DefaultShareEntryPointTest.kt b/features/share/impl/src/test/kotlin/io/element/android/features/share/impl/DefaultShareEntryPointTest.kt index 85068f5402..e1a5709df0 100644 --- a/features/share/impl/src/test/kotlin/io/element/android/features/share/impl/DefaultShareEntryPointTest.kt +++ b/features/share/impl/src/test/kotlin/io/element/android/features/share/impl/DefaultShareEntryPointTest.kt @@ -39,15 +39,16 @@ class DefaultShareEntryPointTest { ShareNode( buildContext = buildContext, plugins = plugins, - presenterFactory = { _, _ -> createSharePresenter() }, + presenterFactory = { createSharePresenter() }, roomSelectEntryPoint = FakeRoomSelectEntryPoint(), - sessionId = A_SESSION_ID, ) } val callback = object : ShareEntryPoint.Callback { override fun onDone(sessionId: SessionId, roomIds: List) = lambdaError() + override fun onCancel() = lambdaError() } val params = ShareEntryPoint.Params( + sessionId = A_SESSION_ID, shareIntentData = ShareIntentData.PlainText(A_MESSAGE), ) val result = entryPoint.createNode( @@ -57,7 +58,7 @@ class DefaultShareEntryPointTest { callback = callback, ) assertThat(result).isInstanceOf(ShareNode::class.java) - assertThat(result.plugins).contains(ShareNode.Inputs(params.shareIntentData)) + assertThat(result.plugins).contains(ShareNode.Inputs(params.sessionId, params.shareIntentData)) assertThat(result.plugins).contains(callback) } } diff --git a/features/share/impl/src/test/kotlin/io/element/android/features/share/impl/SharePresenterTest.kt b/features/share/impl/src/test/kotlin/io/element/android/features/share/impl/SharePresenterTest.kt index 8fc5109636..a2575d8186 100644 --- a/features/share/impl/src/test/kotlin/io/element/android/features/share/impl/SharePresenterTest.kt +++ b/features/share/impl/src/test/kotlin/io/element/android/features/share/impl/SharePresenterTest.kt @@ -44,7 +44,6 @@ class SharePresenterTest { presenter.present() }.test { val initialState = awaitItem() - assertThat(initialState.sessionId).isEqualTo(A_SESSION_ID) assertThat(initialState.shareAction.isUninitialized()).isTrue() } } @@ -86,8 +85,7 @@ class SharePresenterTest { presenter.onRoomSelected(A_SESSION_ID, listOf(A_ROOM_ID)) assertThat(awaitItem().shareAction.isLoading()).isTrue() val success = awaitItem() - assertThat(success.sessionId).isEqualTo(A_SESSION_ID) - assertThat(success.shareAction).isEqualTo(AsyncAction.Success(listOf(A_ROOM_ID))) + assertThat(success.shareAction).isEqualTo(AsyncAction.Success(ShareResult(A_SESSION_ID, listOf(A_ROOM_ID)))) sendResult.assertions().isCalledOnce().with(value(client), value(shareIntentData), value(listOf(A_ROOM_ID))) } } @@ -106,16 +104,11 @@ class SharePresenterTest { moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { - val initialState = awaitItem() - assertThat(initialState.sessionId).isEqualTo(A_SESSION_ID) + skipItems(1) presenter.onRoomSelected(A_SESSION_ID_2, listOf(A_ROOM_ID)) - val sessionChanged = awaitItem() - assertThat(sessionChanged.sessionId).isEqualTo(A_SESSION_ID_2) - assertThat(sessionChanged.shareAction.isUninitialized()).isTrue() assertThat(awaitItem().shareAction.isLoading()).isTrue() val success = awaitItem() - assertThat(success.sessionId).isEqualTo(A_SESSION_ID_2) - assertThat(success.shareAction).isEqualTo(AsyncAction.Success(listOf(A_ROOM_ID))) + assertThat(success.shareAction).isEqualTo(AsyncAction.Success(ShareResult(A_SESSION_ID_2, listOf(A_ROOM_ID)))) getClient.assertions().isCalledOnce().with(value(A_SESSION_ID_2)) sendResult.assertions().isCalledOnce() } @@ -133,7 +126,6 @@ class SharePresenterTest { }.test { skipItems(1) presenter.onRoomSelected(A_SESSION_ID_2, listOf(A_ROOM_ID)) - assertThat(awaitItem().sessionId).isEqualTo(A_SESSION_ID_2) assertThat(awaitItem().shareAction.isLoading()).isTrue() assertThat(awaitItem().shareAction).isEqualTo(AsyncAction.Failure(AN_EXCEPTION)) sendResult.assertions().isNeverCalled() @@ -161,8 +153,8 @@ class SharePresenterTest { presenter.onRoomSelected(A_SESSION_ID_2, listOf(A_ROOM_ID)) completable.complete(Result.success(Unit)) val success = awaitItem() - assertThat(success.sessionId).isEqualTo(A_SESSION_ID) - assertThat(success.shareAction).isEqualTo(AsyncAction.Success(listOf(A_ROOM_ID))) + // The second selection has been ignored + assertThat(success.shareAction).isEqualTo(AsyncAction.Success(ShareResult(A_SESSION_ID, listOf(A_ROOM_ID)))) sendResult.assertions().isCalledOnce() } } @@ -170,13 +162,11 @@ class SharePresenterTest { internal fun TestScope.createSharePresenter( shareIntentData: ShareIntentData = ShareIntentData.PlainText(A_MESSAGE), - initialSessionId: SessionId = A_SESSION_ID, matrixClientProvider: FakeMatrixClientProvider = FakeMatrixClientProvider(), shareDataSender: ShareDataSender = FakeShareDataSender(), ): SharePresenter { return SharePresenter( shareIntentData = shareIntentData, - initialSessionId = initialSessionId, appCoroutineScope = this, matrixClientProvider = matrixClientProvider, shareDataSender = shareDataSender, diff --git a/libraries/roomselect/impl/src/main/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectPresenter.kt b/libraries/roomselect/impl/src/main/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectPresenter.kt index 3de9d22ba8..d8a7a10823 100644 --- a/libraries/roomselect/impl/src/main/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectPresenter.kt +++ b/libraries/roomselect/impl/src/main/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectPresenter.kt @@ -65,17 +65,19 @@ class RoomSelectPresenter( val queryState = rememberTextFieldState() var isSearchActive by remember { mutableStateOf(false) } var isAccountListExpanded by remember { mutableStateOf(false) } + // Null until the sessions are loaded + val sessions by remember { sessionStore.sessionsFlow() }.collectAsState(initial = null) var selectedSessionId by remember { mutableStateOf(initialSessionId) } - val sessions by remember { sessionStore.sessionsFlow() }.collectAsState(initial = emptyList()) // All the accounts are proposed, even the ones with an invalid token. Selecting them displays an error. val accounts by remember { - derivedStateOf { sessions.map { it.toMatrixUser() } } + derivedStateOf { sessions.orEmpty().map { it.toMatrixUser() } } } // The selected session can have an invalid token, or be invalidated while this screen is displayed, for instance if it is - // signed out from another device. The invalidation of the initial session, which is the current one, is handled by the application. + // signed out from another device. val isSelectedSessionInvalid by remember { derivedStateOf { - selectedSessionId != initialSessionId && sessions.none { it.userId == selectedSessionId.value && it.isTokenValid } + val loadedSessions = sessions ?: return@derivedStateOf false + loadedSessions.none { it.userId == selectedSessionId.value && it.isTokenValid } } } LaunchedEffect(isSelectedSessionInvalid) { @@ -99,27 +101,31 @@ class RoomSelectPresenter( val coroutineScope = rememberCoroutineScope() // Create one data source per session. When the selected session changes, the previous // data source is removed from the composition and its coroutine scope is cancelled. - val (selectedSessionDataSource, hasClientError) = key(selectedSessionId) { - val dataSourceCoroutineScope = rememberCoroutineScope() - // The sessions are restored at startup, so the client should already be in memory. - // A null value means that the client is being restored, or that the session is invalid: its client cannot be restored, - // and an in-memory client would have been destroyed, so do not try to get it. - val initialRoomListService = if (isSelectedSessionInvalid) null else matrixClientProvider.getOrNull(selectedSessionId)?.roomListService - val roomListServiceResult by produceState(initialRoomListService?.let { Result.success(it) }) { - if (value == null && !isSelectedSessionInvalid) { - value = matrixClientProvider.getOrRestore(selectedSessionId) - .onFailure { Timber.e(it, "Failed to get the client for $selectedSessionId") } - .map { it.roomListService } + // Also use the validity of the session as a key, so that the data source is removed if the session is invalidated, and recreated + // if the session becomes valid again. + val (dataSource, hasClientError) = key(selectedSessionId, isSelectedSessionInvalid) { + val sessionId = selectedSessionId + if (isSelectedSessionInvalid) { + // Do not try to get the client of an invalid session: it cannot be restored, and an in-memory client would have been destroyed. + null to false + } else { + val dataSourceCoroutineScope = rememberCoroutineScope() + // The sessions are restored at startup, so the client should already be in memory. + // A null value means that the client is being restored. + val roomListServiceResult by produceState(matrixClientProvider.getOrNull(sessionId)?.roomListService?.let { Result.success(it) }) { + if (value == null) { + value = matrixClientProvider.getOrRestore(sessionId) + .onFailure { Timber.e(it, "Failed to get the client for $sessionId") } + .map { it.roomListService } + } } + val roomListService = roomListServiceResult?.getOrNull() + val dataSource = remember(roomListService) { + roomListService?.let { dataSourceFactory.create(dataSourceCoroutineScope, it) } + } + dataSource to (roomListServiceResult?.isFailure == true) } - val roomListService = roomListServiceResult?.getOrNull() - val dataSource = remember(roomListService) { - roomListService?.let { dataSourceFactory.create(dataSourceCoroutineScope, it) } - } - dataSource to (roomListServiceResult?.isFailure == true) } - // Do not use the data source of an invalid session, its client has been destroyed - val dataSource = selectedSessionDataSource.takeIf { !isSelectedSessionInvalid } val hasRoomListError = hasClientError || isSelectedSessionInvalid val searchQuery = queryState.text.toString() diff --git a/libraries/roomselect/impl/src/test/kotlin/io/element/android/libraries/roomselect/impl/DefaultRoomSelectEntryPointTest.kt b/libraries/roomselect/impl/src/test/kotlin/io/element/android/libraries/roomselect/impl/DefaultRoomSelectEntryPointTest.kt index c25e3303cc..7f88bb06ac 100644 --- a/libraries/roomselect/impl/src/test/kotlin/io/element/android/libraries/roomselect/impl/DefaultRoomSelectEntryPointTest.kt +++ b/libraries/roomselect/impl/src/test/kotlin/io/element/android/libraries/roomselect/impl/DefaultRoomSelectEntryPointTest.kt @@ -42,7 +42,7 @@ class DefaultRoomSelectEntryPointTest : RobolectricTest() { assertThat(initialSessionId).isEqualTo(A_SESSION_ID) assertThat(mode).isEqualTo(testMode) assertThat(maxNumberOfRooms).isEqualTo(testMaxNumberOfRooms) - createRoomSelectPresenter(mode, maxNumberOfRooms) + createRoomSelectPresenter(mode = mode, maxNumberOfRooms = maxNumberOfRooms) }, ) } diff --git a/libraries/roomselect/impl/src/test/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectPresenterTest.kt b/libraries/roomselect/impl/src/test/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectPresenterTest.kt index 0694cc27f0..95f5fefa03 100644 --- a/libraries/roomselect/impl/src/test/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectPresenterTest.kt +++ b/libraries/roomselect/impl/src/test/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectPresenterTest.kt @@ -13,6 +13,7 @@ import app.cash.turbine.Event import com.google.common.truth.Truth.assertThat import io.element.android.libraries.designsystem.theme.components.SearchBarResultState import io.element.android.libraries.matrix.api.MatrixClientProvider +import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.matrix.api.roomlist.RoomListFilter import io.element.android.libraries.matrix.api.roomlist.RoomListService import io.element.android.libraries.matrix.api.user.MatrixUser @@ -113,7 +114,7 @@ class RoomSelectPresenterTest { val presenter = createRoomSelectPresenter( mode = RoomSelectMode.Share, sessionStore = InMemorySessionStore( - initialList = listOf(aSessionData(sessionId = A_SESSION_ID.value)), + initialList = listOf(aSessionData(sessionId = A_SESSION_ID.value, isTokenValid = true)), ), ) presenter.test { @@ -208,6 +209,30 @@ class RoomSelectPresenterTest { } } + @Test + fun `present - the current account is invalidated while selected, an error is displayed`() = runTest { + val roomSummary = aRoomSummary() + val sessionStore = aSessionStoreWithTwoAccounts() + val presenter = createRoomSelectPresenter( + mode = RoomSelectMode.Share, + sessionStore = sessionStore, + roomListService = FakeRoomListService(createRoomListLambda = { FakeDynamicRoomList(summaries = MutableStateFlow(listOf(roomSummary))) }), + ) + presenter.test { + val state = consumeItemsUntilPredicate { it.resultState is SearchBarResultState.Results }.last() + state.eventSink(RoomSelectEvent.ToggleSelectedRoom(roomSummary.toSelectRoomInfo())) + assertThat(consumeItemsUntilPredicate { it.selectedRooms.isNotEmpty() }.last().selectedRooms).hasSize(1) + // The current session is signed out from another device while the screen is displayed + sessionStore.updateData(sessionStore.getSession(A_SESSION_ID.value)!!.copy(isTokenValid = false)) + val errorState = consumeItemsUntilPredicate { it.hasRoomListError && it.selectedRooms.isEmpty() }.last() + assertThat(errorState.selectedAccount.userId).isEqualTo(A_SESSION_ID) + assertThat(errorState.resultState).isInstanceOf(SearchBarResultState.Initial::class.java) + // The user can switch to the other account + assertThat(errorState.otherAccounts.map { it.userId }).containsExactly(A_SESSION_ID_2) + cancelAndIgnoreRemainingEvents() + } + } + @Test fun `present - another account is invalidated, it is still proposed and there is no error`() = runTest { val sessionStore = aSessionStoreWithTwoAccounts() @@ -389,15 +414,19 @@ class RoomSelectPresenterTest { } internal fun TestScope.createRoomSelectPresenter( + initialSessionId: SessionId = A_SESSION_ID, mode: RoomSelectMode = RoomSelectMode.Forward, maxNumberOfRooms: Int = RoomSelectEntryPoint.DEFAULT_MAX_NUMBER_OF_ROOMS, roomListService: RoomListService = FakeRoomListService(), - sessionStore: SessionStore = InMemorySessionStore(), + // The current session is always in the store + sessionStore: SessionStore = InMemorySessionStore( + initialList = listOf(aSessionData(sessionId = A_SESSION_ID.value, isTokenValid = true)), + ), matrixClientProvider: MatrixClientProvider = FakeMatrixClientProvider { sessionId -> Result.success(FakeMatrixClient(sessionId = sessionId, roomListService = roomListService)) }, ) = RoomSelectPresenter( - initialSessionId = A_SESSION_ID, + initialSessionId = initialSessionId, mode = mode, maxNumberOfRooms = maxNumberOfRooms, dataSourceFactory = object : RoomSelectSearchDataSource.Factory {