Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix notification navigation #2780

Merged
merged 5 commits into from
May 2, 2024
Merged

Fix notification navigation #2780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
528e7b3
Remove the FtueEntryPoint.Callback, LoggedInFlowNode is already obser…
bmarty May 1, 2024
b05fa82
Rename val for clarity.
bmarty May 1, 2024
83506e5
Fix navigation issue #2778
bmarty May 1, 2024
1791387
Changelog
bmarty May 1, 2024
f807d57
Period
bmarty May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 6 additions & 21 deletions appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import com.bumble.appyx.core.plugin.plugins
import com.bumble.appyx.navmodel.backstack.BackStack
import com.bumble.appyx.navmodel.backstack.operation.push
import com.bumble.appyx.navmodel.backstack.operation.replace
import com.bumble.appyx.navmodel.backstack.operation.singleTop
import dagger.assisted.Assisted
import dagger.assisted.AssistedInject
import io.element.android.anvilannotations.ContributesNode
Expand All @@ -60,6 +59,7 @@ import io.element.android.features.securebackup.api.SecureBackupEntryPoint
import io.element.android.libraries.architecture.BackstackView
import io.element.android.libraries.architecture.BaseFlowNode
import io.element.android.libraries.architecture.createNode
import io.element.android.libraries.architecture.waitForNavTargetAttached
import io.element.android.libraries.designsystem.utils.snackbar.SnackbarDispatcher
import io.element.android.libraries.di.AppScope
import io.element.android.libraries.di.SessionScope
Expand Down Expand Up @@ -345,11 +345,6 @@ class LoggedInFlowNode @AssistedInject constructor(
}
NavTarget.Ftue -> {
ftueEntryPoint.nodeBuilder(this, buildContext)
.callback(object : FtueEntryPoint.Callback {
override fun onFtueFlowFinished() {
lifecycleScope.launch { attachRoomList() }
}
})
.build()
}
NavTarget.RoomDirectorySearch -> {
Expand All @@ -368,32 +363,22 @@ class LoggedInFlowNode @AssistedInject constructor(
}
}

suspend fun attachRoomList() {
if (!canShowRoomList()) return
attachChild<Node> {
backstack.singleTop(NavTarget.RoomList)
}
}

suspend fun attachRoom(roomId: RoomId) {
if (!canShowRoomList()) return
waitForNavTargetAttached { navTarget ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the user is in the middle of FTUE when they tap on the notification? Shouldn't we add some kind of timeout or additional check? Otherwise I think this function will be suspended until then and suddenly resumed and the user presented with the room, and if they tap on several notifications I think they all will be suspended and suddenly resumed at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmh, I did not test the use case of tapping a notification (neither tapping on multiple ones) when a ftue screen is displayed, I will test it.

But if at the end the room is displayed, I think this is the expected behavior.

As I said in the description, if the user clicks on a room permalink from outside the app, the user may have to log in first and at the end will se the room, because this method is suspending.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO that behaviour is bit weird, what if:

  1. The user taps on a notification for a room, it opens the app with FTUE displayed.
  2. Then they tap on a notification for another room.
  3. They finish the FTUE flow.

What will happen then? Will the 1st room be opened, the 2nd one, or both one after the other?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I just checked and all the rooms will be added to the backstack, and several back press will be necessary to go back to the room list. I think this is OK since:

  • it will not happen a lot (FTUE screen are only for the first time, by definition)
  • the user has performed action (click) so is expecting to see the room at some point.

We could ensure that the same room is not added twice on the backstack (i.e. click on a first notification, then get another one for the same room and click on this new one), but really this is edge cases, and this can be handled later if this is a reported issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should handle it here since we have the chance now, but if it's blocking some other PR I can approve it as is and we can handle it later.

navTarget is NavTarget.RoomList
}
attachChild<RoomFlowNode> {
backstack.singleTop(NavTarget.RoomList)
backstack.push(NavTarget.Room(roomId.toRoomIdOrAlias()))
}
}

private fun canShowRoomList(): Boolean {
return ftueService.state.value is FtueState.Complete
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, after a cold boot, ftueService.state.value was equal to Unknown which was the root cause of the issue this PR is fixing.

}

@Composable
override fun View(modifier: Modifier) {
Box(modifier = modifier) {
val lockScreenState by lockScreenStateService.lockState.collectAsState()
val isFtueDisplayed by ftueService.state.collectAsState()
val ftueState by ftueService.state.collectAsState()
BackstackView()
if (isFtueDisplayed is FtueState.Complete) {
if (ftueState is FtueState.Complete) {
PermanentChild(permanentNavModel = permanentNavModel, navTarget = NavTarget.LoggedInPermanent)
}
if (lockScreenState == LockScreenLockState.Locked) {
Expand Down
2 changes: 1 addition & 1 deletion appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ class RootFlowNode @AssistedInject constructor(
.attachSession()
.apply {
when (deeplinkData) {
is DeeplinkData.Root -> attachRoomList()
is DeeplinkData.Root -> Unit // The room list will always be shown, observing FtueState
is DeeplinkData.Room -> attachRoom(deeplinkData.roomId)
}
}
Expand Down
1 change: 1 addition & 0 deletions changelog.d/2778.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Ensure the application open the room when a notification is clicked.
6 changes: 0 additions & 6 deletions features/ftue/api/src/main/kotlin/io/element/android/features/ftue/api/FtueEntryPoint.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,12 @@ package io.element.android.features.ftue.api

import com.bumble.appyx.core.modality.BuildContext
import com.bumble.appyx.core.node.Node
import com.bumble.appyx.core.plugin.Plugin
import io.element.android.libraries.architecture.FeatureEntryPoint

interface FtueEntryPoint : FeatureEntryPoint {
fun nodeBuilder(parentNode: Node, buildContext: BuildContext): NodeBuilder

interface NodeBuilder {
fun callback(callback: Callback): NodeBuilder
fun build(): Node
}

interface Callback : Plugin {
fun onFtueFlowFinished()
}
}
5 changes: 0 additions & 5 deletions .../ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/DefaultFtueEntryPoint.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,6 @@ class DefaultFtueEntryPoint @Inject constructor() : FtueEntryPoint {
val plugins = ArrayList<Plugin>()

return object : FtueEntryPoint.NodeBuilder {
override fun callback(callback: FtueEntryPoint.Callback): FtueEntryPoint.NodeBuilder {
plugins += callback
return this
}

override fun build(): Node {
return parentNode.createNode<FtueFlowNode>(buildContext, plugins)
}
Expand Down
5 changes: 1 addition & 4 deletions features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/FtueFlowNode.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import dagger.assisted.Assisted
import dagger.assisted.AssistedInject
import io.element.android.anvilannotations.ContributesNode
import io.element.android.features.analytics.api.AnalyticsEntryPoint
import io.element.android.features.ftue.api.FtueEntryPoint
import io.element.android.features.ftue.impl.notifications.NotificationsOptInNode
import io.element.android.features.ftue.impl.sessionverification.FtueSessionVerificationFlowNode
import io.element.android.features.ftue.impl.state.DefaultFtueService
Expand Down Expand Up @@ -86,8 +85,6 @@ class FtueFlowNode @AssistedInject constructor(
data object LockScreenSetup : NavTarget
}

private val callback = plugins.filterIsInstance<FtueEntryPoint.Callback>().firstOrNull()

override fun onBuilt() {
super.onBuilt()

Expand Down Expand Up @@ -157,7 +154,7 @@ class FtueFlowNode @AssistedInject constructor(
FtueStep.LockscreenSetup -> {
backstack.newRoot(NavTarget.LockScreenSetup)
}
null -> callback?.onFtueFlowFinished()
null -> Unit
}
}

Expand Down
21 changes: 21 additions & 0 deletions ...s/architecture/src/main/kotlin/io/element/android/libraries/architecture/ParentNodeExt.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.bumble.appyx.core.children.nodeOrNull
import com.bumble.appyx.core.node.Node
import com.bumble.appyx.core.node.ParentNode
import kotlinx.coroutines.cancel
import kotlinx.coroutines.launch
import kotlinx.coroutines.suspendCancellableCoroutine
import kotlin.coroutines.resume
Expand Down Expand Up @@ -48,3 +49,23 @@
continuation.cancel()
}
}

/**
* Wait for a child to be attached to the parent node, only using the NavTarget.
*/
suspend inline fun <NavTarget : Any> ParentNode<NavTarget>.waitForNavTargetAttached(crossinline predicate: (NavTarget) -> Boolean) =
suspendCancellableCoroutine { continuation ->
lifecycleScope.launch {
children.collect { childMap ->
val node = childMap.entries
.map { it.key.navTarget }
.lastOrNull(predicate)

Check warning on line 62 in libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/ParentNodeExt.kt

View check run for this annotation

Codecov / codecov/patch

libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/ParentNodeExt.kt#L57-L62 

Added lines #L57 - L62 were not covered by tests
if (node != null && !continuation.isCompleted) {
continuation.resume(Unit)
cancel()

Check warning on line 65 in libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/ParentNodeExt.kt

View check run for this annotation

Codecov / codecov/patch

libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/ParentNodeExt.kt#L64-L65 

Added lines #L64 - L65 were not covered by tests
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think cancel() should also be invoked after line 45, since once the continuation is resumed, there is no need to invoke the predicated until the Node is destroyed.

But I do not want to do it in that PR.

Copy link
Member

@jmartinesp jmartinesp May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this also happens in the original ParentNode.waitForChildAttached(). That's kind of weird. It could be solved if they didn't use collect, but mapNotNull and then a suspending firstOrNull() or something like that, but keeping the cancel here is fine too.

}
}
}.invokeOnCompletion {
continuation.cancel()

Check warning on line 69 in libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/ParentNodeExt.kt

View check run for this annotation

Codecov / codecov/patch

libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/ParentNodeExt.kt#L68-L69 

Added lines #L68 - L69 were not covered by tests
}
}
Loading