voyager
voyager copied to clipboard
adrielcafe
`AndroidScreen` is not working as intended in `1.0.0-rc06` - Hilt
Hello,
Recently I've started using Voyager. I've been enjoying how simple it is and how quick we can start getting into it, but have also been getting a few issues. I might have missed something, but the documentation is confusing regarding AndroidScreen, Hilt ViewModels and ScreenLifecycleProvider. This seems to be a bug introduced in 1.0.0-rc06.
Some context: I have some Tabs, and each Tab has its own Navigator with a SlideTransition. In each Tab, I can push an Item (which is a Screen)
First, I tried using AndroidScreen with unique keys for my Item.
When I navigate from any Tab to this AndroidScreen, everything works, and my ViewModel is correctly created.
The issue is when navigating back to the initial screen of the Tab, and navigating to this AndroidScreen again, but this time for another item (which loads different data but is the same screen type). When loading the new item, it will show all the old data for the item that was first shown - as in, it's the same ViewModel as the first Item.
Apparently AndroidScreen uses DefaultScreenLifecycleOwner, and when using getViewModel() it will always use the parent Activity as the owner, which means it will always be the same ViewModel for all these screens.
This was introduced in 1.0.0-rc06(https://github.com/adrielcafe/voyager/commit/33e28d258ea54906623b845d796c0e0e861814ef)
I believe this goes against what is specified in the documentation.
According to the Documentation, we can also use ScreenLifecycleProvider to get a similar behaviour as AndroidScreen, but using ScreenLifecycleProvider.get(...) actually gives us the expected behaviour since it will create a ScreenLifecycleOwner, which should be unique for each screen (provided that the key is unique, I suppose).
But when I tried to use ScreenLifecycleProvider.get(screen) by overriding ScreenLifecycleProvider, I was getting this error:
Crash: java.lang.IllegalArgumentException: Key Screen#0131d128-773c-4d9d-b1e1-4ba91ed961a5:transition:lifecycle was used multiple times at androidx.compose.runtime.saveable.SaveableStateHolderImpl$SaveableStateProvider$1$1.invoke(SaveableStateHolder.kt:89) at androidx.compose.runtime.saveable.SaveableStateHolderImpl$SaveableStateProvider$1$1.invoke(SaveableStateHolder.kt:88) at androidx.compose.runtime.DisposableEffectImpl.onRemembered(Effects.kt:81) at androidx.compose.runtime.CompositionImpl$RememberEventDispatcher.dispatchRememberObservers(Composition.kt:1105) at androidx.compose.runtime.CompositionImpl.applyChangesInLocked(Composition.kt:820) at androidx.compose.runtime.CompositionImpl.applyChanges(Composition.kt:842) at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$2.invoke(Recomposer.kt:592) at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$2.invoke(Recomposer.kt:510) at androidx.compose.ui.platform.AndroidUiFrameClock$withFrameNanos$2$callback$1.doFrame(AndroidUiFrameClock.android.kt:34) at androidx.compose.ui.platform.AndroidUiDispatcher.performFrameDispatch(AndroidUiDispatcher.android.kt:109) at androidx.compose.ui.platform.AndroidUiDispatcher.access$performFrameDispatch(AndroidUiDispatcher.android.kt:41) at androidx.compose.ui.platform.AndroidUiDispatcher$dispatchCallback$1.doFrame(AndroidUiDispatcher.android.kt:69) at android.view.Choreographer$CallbackRecord.run(Choreographer.java:970) at android.view.Choreographer.doCallbacks(Choreographer.java:796) at android.view.Choreographer.doFrame(Choreographer.java:727) at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:957) at android.os.Handler.handleCallback(Handler.java:938) at android.os.Handler.dispatchMessage(Handler.java:99) at android.os.Looper.loop(Looper.java:223) at android.app.ActivityThread.main(ActivityThread.java:7656) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947) Suppressed: kotlinx.coroutines.DiagnosticCoroutineContextException: [androidx.compose.runtime.PausableMonotonicFrameClock@22011f3, androidx.compose.ui.platform.MotionDurationScaleImpl@211deb0, StandaloneCoroutine{Cancelling}@f95a129, AndroidUiDispatcher@877adf1]
Downgrading Voyager to 1.0.0-rc05 solved the issue though, so it seems like a bug introduced with 1.0.0-rc06? Everything works as expected using ScreenLifecycleProvider.get(screen) or when using AndroidScreen in 1.0.0-rc05, as it also uses ScreenLifecycleProvider.get(screen).
So, I guess there are two points here:
- Shouldn't
AndroidScreenstill be doingoverride fun getLifecycleOwner(): ScreenLifecycleOwner = AndroidScreenLifecycleOwner.get(this)? Why not? -
ScreenLifecycleProvider.get(screen)crashes in1.0.0-rc06. In1.0.0-rc05it works as expected.
The documentation also says it gets a key but it actually needs the whole Screen (this). I guess this is just slightly outdated.
I might be missing some details here though, so feel free to point out any mistakes or things I might've misunderstood. Let me know if more context is needed, I can't share the codebase but can give more details into what we're doing.
Thank you in advance for your help!
@adrielcafe Mind taking a look at this issue when you have the chance? It's quite a big blocker for us and it also impedes us of going to future Voyager releases so that we can update our Compose & Dagger2 dependencies properly
After updating, I'm getting a:
java.lang.IllegalStateException: You can consumeRestoredStateForKey only after super.onCreate of corresponding component
But not sure if this is due to an issue on our end. I'll take a closer look and get back to you as soon as possible. Thanks!
AndroidScreen still uses DefaultScreenLifecycleOwner, so it still does not work as expected.
And using:
override fun getLifecycleOwner(): ScreenLifecycleOwner = AndroidScreenLifecycleOwner.get(this)
Still results in:
java.lang.IllegalArgumentException: Key Screen#c2a37e25-0134-46ff-91ef-b590532f9c55:transition:lifecycle was used multiple times
at androidx.compose.runtime.saveable.SaveableStateHolderImpl$SaveableStateProvider$1$1.invoke(SaveableStateHolder.kt:89)
at androidx.compose.runtime.saveable.SaveableStateHolderImpl$SaveableStateProvider$1$1.invoke(SaveableStateHolder.kt:88)
at androidx.compose.runtime.DisposableEffectImpl.onRemembered(Effects.kt:82)
at androidx.compose.runtime.CompositionImpl$RememberEventDispatcher.dispatchRememberObservers(Composition.kt:1137)
at androidx.compose.runtime.CompositionImpl.applyChangesInLocked(Composition.kt:828)
at androidx.compose.runtime.CompositionImpl.applyChanges(Composition.kt:849)
at androidx.compose.runtime.Recomposer.composeInitial$runtime_release(Recomposer.kt:1041)
at androidx.compose.runtime.CompositionImpl.setContent(Composition.kt:520)
at androidx.compose.ui.platform.WrappedComposition$setContent$1.invoke(Wrapper.android.kt:142)
at androidx.compose.ui.platform.WrappedComposition$setContent$1.invoke(Wrapper.android.kt:133)
at androidx.compose.ui.platform.AndroidComposeView.setOnViewTreeOwnersAvailable(AndroidComposeView.android.kt:1191)
at androidx.compose.ui.platform.WrappedComposition.setContent(Wrapper.android.kt:133)
at androidx.compose.ui.platform.WrappedComposition.onStateChanged(Wrapper.android.kt:183)
at androidx.lifecycle.LifecycleRegistry$ObserverWithState.dispatchEvent(LifecycleRegistry.kt:314)
at androidx.lifecycle.LifecycleRegistry.addObserver(LifecycleRegistry.kt:192)
at androidx.compose.ui.platform.WrappedComposition$setContent$1.invoke(Wrapper.android.kt:140)
at androidx.compose.ui.platform.WrappedComposition$setContent$1.invoke(Wrapper.android.kt:133)
at androidx.compose.ui.platform.AndroidComposeView.onAttachedToWindow(AndroidComposeView.android.kt:1266)
at android.view.View.dispatchAttachedToWindow(View.java:20479)
at android.view.ViewGroup.dispatchAttachedToWindow(ViewGroup.java:3489)
at android.view.ViewGroup.dispatchAttachedToWindow(ViewGroup.java:3496)
at android.view.ViewGroup.dispatchAttachedToWindow(ViewGroup.java:3496)
at android.view.ViewGroup.dispatchAttachedToWindow(ViewGroup.java:3496)
at android.view.ViewGroup.dispatchAttachedToWindow(ViewGroup.java:3496)
at android.view.ViewGroup.dispatchAttachedToWindow(ViewGroup.java:3496)
at android.view.ViewGroup.dispatchAttachedToWindow(ViewGroup.java:3496)
at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:2417)
at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:1952)
at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:8171)
at android.view.Choreographer$CallbackRecord.run(Choreographer.java:972)
at android.view.Choreographer.doCallbacks(Choreographer.java:796)
at android.view.Choreographer.doFrame(Choreographer.java:731)
at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:957)
at android.os.Handler.handleCallback(Handler.java:938)
at android.os.Handler.dispatchMessage(Handler.java:99)
at android.os.Looper.loop(Looper.java:223)
at android.app.ActivityThread.main(ActivityThread.java:7656)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)
Which seems to be the same or a similar stack trace as before
The other exception above (java.lang.IllegalStateException: You can consumeRestoredStateForKey only after super.onCreate of corresponding component) seems to be due to our custom AndroidScreenLifecycleOwner that we created in order to set arguments for the SavedStateHandle, which was not possible with Voyager's AndroidScreenLifecycleOwner.
It seems that our custom AndroidScreenLifecycleOwner was not going through its onCreate, which does controller.performRestore(savedState) (we do also have a custom getViewModel() that removes the hard cast from AndroidScreenLifecycleOwner). Actually it would be good if that hard cast could be removed in favour of this:
@Composable
inline fun <reified T : ViewModel> Screen.getViewModel(
viewModelProviderFactory: ViewModelProvider.Factory? = null
): T {
val context = LocalContext.current
return remember(key1 = T::class) {
val activity = context.componentActivity
val lifecycleOwner = (this as? ScreenLifecycleProvider)?.getLifecycleOwner()
val defaultHasProvider = lifecycleOwner as? HasDefaultViewModelProviderFactory ?: activity
val factory = VoyagerHiltViewModelFactories.getVoyagerFactory(
activity = activity,
delegateFactory = viewModelProviderFactory ?: defaultHasProvider.defaultViewModelProviderFactory
)
val provider = ViewModelProvider(
store = (lifecycleOwner as? ViewModelStoreOwner)?.viewModelStore ?: activity.viewModelStore,
factory = factory,
defaultCreationExtras = defaultHasProvider.defaultViewModelCreationExtras
)
provider[T::class.java]
}
}
By providing a custom LocalNavigatorScreenLifecycleProvider that returns our custom AndroidScreenLifecycleOwner, we can get the same exception as with the regular AndroidScreenLifecycleOwner
TL;DR: Issue still exists in 1.0.0-rc07 😞
Edit: It would also be nice if we could scope a ViewModel to a Navigator, similar to how we can scope a ViewModel to a NavGraph with the Navigation Component. But I guess we would need a separate issue for that 😛
Edit2: I actually think that maybe we could avoid the custom AndroidScreenLifecycleOwner if we just add the creation extras in getViewModel(), adding DEFAULT_ARGS_KEY. But this is just a note for our codebase after reading a bit more into the Voyager/Hilt integration, the main issue with Voyager would still exist
@adrielcafe I believe it's due to this bit of code in Navigator.kt:
val lifecycleOwner = rememberScreenLifecycleOwner(screen)
val navigatorScreenLifecycleOwners = getNavigatorScreenLifecycleProvider(screen)
val composed = remember(lifecycleOwner, navigatorScreenLifecycleOwners) {
listOf(lifecycleOwner) + navigatorScreenLifecycleOwners
}
This adds the same screen twice to the list of savable states. This was added in 1.0.0-rc06, so it makes sense to be the culprit. This is the commit: https://github.com/adrielcafe/voyager/commit/5deb781d7283c9b8f402a66607e2312acc619864
When logging provideSaveableState's providedStateKey, we can see that the same screen results in these logs:
Add Screen#eca46b83-2a9d-4b44-98f1-d8def587acc2:transition:lifecycle
Add Screen#eca46b83-2a9d-4b44-98f1-d8def587acc2:transition:lifecycle
Add Screen#eca46b83-2a9d-4b44-98f1-d8def587acc2:currentScreen:lifecycle
Add Screen#eca46b83-2a9d-4b44-98f1-d8def587acc2:currentScreen:lifecycle
To replicate this issue, feel free to create a new Android project, add these dependencies:
val voyagerVersion = "1.0.0-rc07"
implementation("cafe.adriel.voyager:voyager-navigator:$voyagerVersion")
implementation("cafe.adriel.voyager:voyager-transitions:$voyagerVersion")
Make sure to update the BOM to 2023.08.00 (to get the latest Compose versions just like Voyager uses)
And in MainActivity.kt:
class MainActivity : ComponentActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
setContent {
VoyagerDemoTheme {
Navigator(Screen1("1")) { navigator ->
SlideTransition(navigator) {
CurrentScreen()
}
}
}
}
}
}
abstract class UniqueScreen : Screen, ScreenLifecycleProvider {
override val key: ScreenKey = uniqueScreenKey
override fun getLifecycleOwner(): ScreenLifecycleOwner = AndroidScreenLifecycleOwner.get(this)
}
class Screen1(private val name: String) : UniqueScreen() {
@Composable
override fun Content() {
Text(text = "Hello $name!")
}
}
This will result in the same crash as we have in our project:
java.lang.IllegalArgumentException: Key Screen#7ec31df0-30b4-4c67-8e84-d9a6f135b468:currentScreen:lifecycle was used multiple times
@L-Andrade Can you try something like this if you haven't?
class MainActivity : ComponentActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
setContent {
VoyagerDemoTheme {
CompositionLocalProvider(
LocalNavigatorScreenLifecycleProvider provides EmptyNavigatorScreenLifecycleProvider
) {
Navigator(Screen1("1")) { navigator ->
SlideTransition(navigator) {
CurrentScreen()
}
}
}
}
}
}
}
abstract class UniqueScreen : Screen, ScreenLifecycleProvider {
override val key: ScreenKey = uniqueScreenKey
override fun getLifecycleOwner(): ScreenLifecycleOwner = AndroidScreenLifecycleOwner.get(this)
}
class Screen1(private val name: String) : UniqueScreen() {
@Composable
override fun Content() {
Text(text = "Hello $name!")
}
}
internal object EmptyNavigatorScreenLifecycleProvider : NavigatorScreenLifecycleProvider{
override fun provide(screen: Screen): List<ScreenLifecycleOwner> = emptyList()
}
Hey @DevSrSouza!
Yes, I've tried something like that. While that fixes it for the first screen that is shown, when we add navigation it crashes.
By using this snippet:
class MainActivity : ComponentActivity() {
@OptIn(ExperimentalVoyagerApi::class)
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
setContent {
VoyagerDemoTheme {
CompositionLocalProvider(
LocalNavigatorScreenLifecycleProvider provides EmptyNavigatorScreenLifecycleProvider
) {
Navigator(Screen1("1")) { navigator ->
SlideTransition(navigator) {
CurrentScreen()
}
}
}
}
}
}
}
abstract class UniqueScreen : Screen, ScreenLifecycleProvider {
override val key: ScreenKey = uniqueScreenKey
override fun getLifecycleOwner(): ScreenLifecycleOwner = AndroidScreenLifecycleOwner.get(this)
}
class Screen1(private val name: String) : UniqueScreen() {
@Composable
override fun Content() {
val navigator = LocalNavigator.currentOrThrow
Text(
text = "Hello $name!",
modifier = Modifier.clickable { navigator.push(Screen2(name)) }
)
}
}
class Screen2(private val name: String) : UniqueScreen() {
@Composable
override fun Content() {
Text(text = "Another screen with name $name!")
}
}
@OptIn(ExperimentalVoyagerApi::class)
internal object EmptyNavigatorScreenLifecycleProvider : NavigatorScreenLifecycleProvider {
override fun provide(screen: Screen): List<ScreenLifecycleOwner> = emptyList()
}
When we click on the text to navigate to the second screen, we get the same (or similar) crash:
Process: com.andradel.voyagerdemo, PID: 23201
java.lang.IllegalArgumentException: Key Screen#1ca577e5-61af-4fd0-9fb3-7181cbe56c85:currentScreen was used multiple times
at androidx.compose.runtime.saveable.SaveableStateHolderImpl$SaveableStateProvider$1$1.invoke(SaveableStateHolder.kt:89)
at androidx.compose.runtime.saveable.SaveableStateHolderImpl$SaveableStateProvider$1$1.invoke(SaveableStateHolder.kt:88)
at androidx.compose.runtime.DisposableEffectImpl.onRemembered(Effects.kt:82)
at androidx.compose.runtime.CompositionImpl$RememberEventDispatcher.dispatchRememberObservers(Composition.kt:1137)
at androidx.compose.runtime.CompositionImpl.applyChangesInLocked(Composition.kt:828)
at androidx.compose.runtime.CompositionImpl.applyChanges(Composition.kt:849)
at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$1.invoke(Recomposer.kt:625)
at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$1.invoke(Recomposer.kt:537)
at androidx.compose.ui.platform.AndroidUiFrameClock$withFrameNanos$2$callback$1.doFrame(AndroidUiFrameClock.android.kt:41)
at androidx.compose.ui.platform.AndroidUiDispatcher.performFrameDispatch(AndroidUiDispatcher.android.kt:109)
at androidx.compose.ui.platform.AndroidUiDispatcher.access$performFrameDispatch(AndroidUiDispatcher.android.kt:41)
at androidx.compose.ui.platform.AndroidUiDispatcher$dispatchCallback$1.doFrame(AndroidUiDispatcher.android.kt:69)
at android.view.Choreographer$CallbackRecord.run(Choreographer.java:970)
at android.view.Choreographer.doCallbacks(Choreographer.java:796)
at android.view.Choreographer.doFrame(Choreographer.java:727)
at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:957)
at android.os.Handler.handleCallback(Handler.java:938)
at android.os.Handler.dispatchMessage(Handler.java:99)
at android.os.Looper.loop(Looper.java:223)
at android.app.ActivityThread.main(ActivityThread.java:7656)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)
Suppressed: kotlinx.coroutines.internal.DiagnosticCoroutineContextException: [androidx.compose.runtime.PausableMonotonicFrameClock@3a140a0, androidx.compose.ui.platform.MotionDurationScaleImpl@9197459, StandaloneCoroutine{Cancelling}@71fd81e, AndroidUiDispatcher@19c58ff]
Did you try not use CurrentScreen, instead use it.Content() ?
I did not see any reasoning why the used of the CurrentScreen in the SlideTransition block, can you elaborate on that?
If you take a look at the implementation of all Transitions and SlideTransition, the CurrentScreen is not used.
the ScreenTransition does the saveableState for you, there is no need to call CurrentScreen inside it.
@DevSrSouza Restore with transition still does not work talked about some times ago: https://github.com/adrielcafe/voyager/issues/27#issuecomment-1555908104
I tested here and was not able to reproduce the state not being restored.
class BasicNavigationActivity : ComponentActivity() {
@OptIn(ExperimentalVoyagerApi::class)
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
setContent {
CompositionLocalProvider(
LocalNavigatorScreenLifecycleProvider provides EmptyNavigatorScreenLifecycleProvider
) {
Navigator(Screen1("1")) { navigator ->
SlideTransition(navigator) {
it.Content()
}
}
}
}
}
}
abstract class UniqueScreen : Screen, ScreenLifecycleProvider {
override val key: ScreenKey = uniqueScreenKey
override fun getLifecycleOwner(): ScreenLifecycleOwner = AndroidScreenLifecycleOwner.get(this)
}
class Screen1(private val name: String) : UniqueScreen() {
@Composable
override fun Content() {
val navigator = LocalNavigator.currentOrThrow
Column {
Text(
text = "Hello $name!",
modifier = Modifier.clickable { navigator.push(Screen2(name)) }
)
val testData = remember {
(1..200).map { "Example $it" }
}
Column(modifier = Modifier.verticalScroll(rememberScrollState())) {
for(a in testData) {
Text(a)
}
}
}
}
}
class Screen2(private val name: String) : UniqueScreen() {
@Composable
override fun Content() {
Text(text = "Another screen with name $name!")
}
}
@OptIn(ExperimentalVoyagerApi::class)
internal object EmptyNavigatorScreenLifecycleProvider : NavigatorScreenLifecycleProvider {
override fun provide(screen: Screen): List<ScreenLifecycleOwner> = emptyList()
}
The scroll state is being kept when I navigate to the second screen, put the app in background and call adb shell am kill cafe.adriel.voyager.sample, return to the app and do a Backstack
Can you try the normal way? AndroidScreen and not disabling the necessary lifecycle.
And check in Screen2 that the state is correctly restored
the
ScreenTransitiondoes thesaveableStatefor you, there is no need to callCurrentScreeninside it.
Oh, cool. Didn't see that detail. A regular Navigator uses CurrentScreen, but Transitions do seem to use it.Content() instead. This is easy to miss in my opinion.
None of these things are mentioned in the documentation though. There's no mention of LocalNavigatorScreenLifecycleProvider or how/why it should be used or overriden.
Would be nice to have an update in the docs, including a note to not use CurrentScreen with Transitions. I actually had screen.Content() at the start, but changed it to CurrentScreen when trying to understand the other issues.
Thanks for the help @DevSrSouza. I've updated our project and it seems to be working as expected for now.
AndroidScreen still does not seem to work as intended, due to using DefaultScreenLifecycleOwner, unless I'm missing something. I haven't checked it to make sure though, as I have other priorities right now. We can close this issue if/once it is fixed.
Also worth pointing out that a BottomSheetNavigator needs to be provided the EmptyNavigatorScreenLifecycleProvider, otherwise it will also crash.
AndroidScreenLifecycleOwner is currently meant to be added by DefaultNavigatorScreenLifecycleProvider for Screens on Android
Yeh, LocalNavigatorScreenLifecycleProvider is a new API that I did not have time to document currently.
It did introduce a behavior change that was documented in the release notes that all Screen on Android are by default now with the behavior of the AndroidScreenLifecycle.
This new API and changes was done to make Voyager more flexible and simple for 1.0.
Can you reavaliate if everything is working as expected with Empty NavigatorScreenLifecycleProvider + using it.Content() in the transitions?
About the BottomSheetNavigator, I will need to investigate further, if you have any example to show it or open another issue would be awesome. But as far I know, we are using the latest voyager version where we work and we use BottomSheetNavigator and we don't have any custom NavigatorScreenLifecycleProvider or custom ScreenLifecycleProvider and having be work just as fine.
There's different things here between @Syer10 and @DevSrSouza
Is there a way to have the DefaultNavigatorScreenLifecycleProvider works on Android to not have to force an empty one and add overrides on all the Screens?
Currently using AndroidScreen, while I can refactor the 98 Screens it would be nice to know what is actually the proper way to do things in the future.
Can you reavaliate if everything is working as expected with Empty NavigatorScreenLifecycleProvider + using
it.Content()in the transitions?
Everything seems to be working as expected right now - doing some final checks.
About the BottomSheetNavigator, I will need to investigate further, if you have any example to show it or open another issue would be awesome. But as far I know, we are using the latest voyager version where we work and we use BottomSheetNavigator and we don't have any custom
NavigatorScreenLifecycleProvideror customScreenLifecycleProviderand having be work just as fine.
Just meant that we need to apply the Empty provider to the BottomSheetNavigator as well. I think you get what I mean using this snippet:
class MainActivity : ComponentActivity() {
@OptIn(ExperimentalVoyagerApi::class)
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
setContent {
VoyagerDemoTheme {
BottomSheetNavigator {
CompositionLocalProvider(
LocalNavigatorScreenLifecycleProvider provides EmptyNavigatorScreenLifecycleProvider
) {
Navigator(Screen1("1")) { navigator ->
SlideTransition(navigator) {
it.Content()
}
}
}
}
}
}
}
}
abstract class UniqueScreen : Screen, ScreenLifecycleProvider {
override val key: ScreenKey = uniqueScreenKey
override fun getLifecycleOwner(): ScreenLifecycleOwner = AndroidScreenLifecycleOwner.get(this)
}
class Screen1(private val name: String) : UniqueScreen() {
@Composable
override fun Content() {
val navigator = LocalNavigator.currentOrThrow
val bottomSheetNavigator = LocalBottomSheetNavigator.current
Column {
Text(
text = "Hello $name!",
modifier = Modifier.clickable { navigator.push(Screen2(name)) }
)
Button(onClick = { bottomSheetNavigator.show(Screen1("Bottom sheet")) }) {
Text(text = "Show bottom sheet")
}
}
}
}
class Screen2(private val name: String) : UniqueScreen() {
@Composable
override fun Content() {
Text(text = "Another screen with name $name!")
}
}
@OptIn(ExperimentalVoyagerApi::class)
internal object EmptyNavigatorScreenLifecycleProvider : NavigatorScreenLifecycleProvider {
override fun provide(screen: Screen): List<ScreenLifecycleOwner> = emptyList()
}
With this, the app crashes with the same exception as before:
java.lang.IllegalArgumentException: Key Screen#66770ea4-98d0-4272-b59a-fd1110684438:currentScreen:lifecycle was used multiple times
By providing the Empty provider before the BottomSheetNavigator, it works as expected.
At least from your previous responses, I thought the BottomSheetNavigator would not need it, since it's not a regular Navigator, I didn't go too deep on it though. It uses CurrentScreen() internally as default for the sheet content though.
AndroidScreenLifecycleOwner is currently meant to be added by DefaultNavigatorScreenLifecycleProvider for Screens on Android
Does this mean we don't need to implement ScreenLifecycleProvider? I don't think regular Android ViewModel initialisation will work if we do that, will it? I'll try to check that out later.
Edit: What I mean is a ViewModel scoped to a Screen - since we use getViewModel(), which casts the Screen to ScreenLifecycleProvider to get the LifecycleOwner. If it's not a ScreenLifecycleProvider, it will use the Activitys ViewModelStore instead of the Screen's.
Thanks for the help @DevSrSouza! Feels good to finally be able to update our dependencies.
@Tolriq the NavigatorScreenLifecycleProvider is the currently proper way to ScreenLifecycle to all screens at once. If you have custom implementation that depends on your custom screen parameters, etc, you can still fallback to ScreenLifecycleProvider and provide an EmptyNavigatorScreenLifecycleProvider if you don't want or need the default behavior.
It will depend on your use case. You can keep everything you are doing and use EmptyNavigatorScreenLifecycleProvider or if your use case fits, you can simplify your API by just creating your own custom NavigatorScreenLifecycleProvider.
So it should work OOB with Android screen ?
I need the viewmodels to be properly scoped to the screens and not the single activity and to work with hilt and savestatehandler.
Thanks for the help @DevSrSouza! Feels good to finally be able to update our dependencies.
Sorry the delay to help with the issue.
Does this mean we don't need to implement ScreenLifecycleProvider? I don't think regular Android ViewModel initialisation will work if we do that, will it? I'll try to check that out later.
AndroidScreen by default does that behavior, but, it is not required anymore if you use the default ScreenLifecycleProvider from it, because, in the new version, this behavior that was provided by AndroidScreen is now the default behavior of all Screens with the DefaultNavigatorScreenLifecycleProvider that is applied by default
Just meant that we need to apply the Empty provider to the BottomSheetNavigator as well. I think you get what I mean using this snippet:
I need to investigate, but, if you want to be applied in the BottomSheetNavigator as well, you can move the CompositionLocalProvider to the top.
So it should work OOB with Android screen ?
I need the viewmodels to be properly scoped to the screens and not the single activity and to work with hilt and savestatehandler.
Yes, if the default AndroidScreen was already working for you, it should work as intended when you do the update without specifing directly the AndroidScreenLifecycle on the AndroidScreen.
If this behavior does not follow, submit an issue with a reproducible code because it should just work out of the box
Edit: It would also be nice if we could scope a ViewModel to a Navigator, similar to how we can scope a ViewModel to a NavGraph with the Navigation Component. But I guess we would need a separate issue for that 😛
@L-Andrade about this comment, this changes that we did is towards making in the future ViewModel/"ScreenModel" scoped at the Navigator level, did not test or implemented it yet.
AndroidScreen by default does that behavior, but, it is not required anymore if you use the default ScreenLifecycleProvider from it, because, in the new version, this behavior that was provided by AndroidScreen is now the default behavior of all Screens with the DefaultNavigatorScreenLifecycleProvider that is applied by default
Ok but as said in this issue and in the other issue, when using AndroidScreen this does not work. Your solution and what the OP is now using is to not use AndroidScreen.
https://github.com/adrielcafe/voyager/issues/155#issuecomment-1706358087
This is the whole point of this I guess, AndroidScreen is not working as expected. But I suppose I can migrate to Screen and do the manual way too.
@Tolriq AndroidScreenLifecycleOwner is now applied by default for all Screens in the Navigator. This is why AndroidScreen does not have any implementation anymore because is already applied.
If you have a custom AndroidScreenLifecycleOwner, then, you should use the EmptyNavigatorScreenLifecycleProvider that I said.
I have nothing custom. Simple AndroidScreen. FadeTransition don't restore.
I'll see if hilt viewmodels support savedstatehandle with simple Screen.
@Tolriq Can you open an issue about that with a reproducible code and a reproducible way to validate that? As far I tested already, by using rememberSaveable is being restored. I don't think your issue has any connection with @L-Andrade issue.
@L-Andrade Hi, did my snippets fix the issue for you? If so, can we close this one?