-
Notifications
You must be signed in to change notification settings - Fork 102
Adapt changes in NavigationEventHandler
#2436
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.
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
Conversation
…refine progress handling logic
|
|
||
| LaunchedEffect(enabled) { | ||
| if (enabled) { | ||
| dispatcher.transitionState |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can listen to the state passed to NavigationBackHandler down below, which is local to this handler, instead of listening to dispatcher.transitionState. As there may be several handlers in a composition and this handler may not be the active one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, I'll try tomorrow
| mutableStateOf(null) | ||
| } | ||
|
|
||
| fun getActiveProgressChannel(): Channel<BackEventCompat> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a TIL question. What is the benefit of using Channel instead of Flow these days? Is it for performance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a lighter low-level abstraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 💯
| getActiveProgressChannel().send(event) | ||
| } | ||
| val transitionState = navEventState.transitionState | ||
| if (transitionState is NavigationEventTransitionState.InProgress) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it get to run when the transition state turns from Idle to InProgress again after a composition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nvm, it does.
| } | ||
| val transitionState = navEventState.transitionState | ||
| if (transitionState is NavigationEventTransitionState.InProgress) { | ||
| LaunchedEffect(transitionState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the coroutine relaunch after every transitionState change? For example, if we see:
1. Idle
2. InProgress(eventA)
3. InProgress(eventB)
4. InProgress(eventC)
5. Idle
Does it launch 5 times?
| @Composable | ||
| actual fun PredictiveBackHandler( | ||
| enabled: Boolean, | ||
| onBack: suspend (progress: Flow<BackEventCompat>) -> Unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that onBack is still called after cancellation. Not sure if it's our fault. For example:
PredictiveBackHandler(true) { progress ->
Log.d("gyz", "start")
try {
progress.collect { Log.d("gyz", it.toString()) }
Log.d("gyz", "finish")
} catch(e: Exception) {
Log.d("gyz", "cancelled")
}
}
And after it prints "cancelled", it then prints another "start" and back event.
I'd like to try nav3 in my little cmp side project. Is that demo app public by any chance? Thanks! |
It enables a support of the Nav3 library on Desktop, Web and iOS.
n3.mp4
Fixes: CMP-8937 Adapt changes in
NavigationEventHandlerTesting
Release Notes
Features - Navigation