Closed L-Andrade closed 5 months ago
L-Andrade
commented
1 year ago @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
adrielcafe
commented
1 year ago The same happens on 1.0.0-rc07?
L-Andrade
commented
1 year ago After updating, I'm getting a:
java.lang.IllegalStateException: You can consumeRestoredStateForKey only after super.onCreate of corresponding componentBut 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!
L-Andrade
commented
1 year ago 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
L-Andrade
commented
1 year ago @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:lifecycleTo 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
commented
1 year ago @adrielcafe Can you take another look when you have the chance please? đ
DevSrSouza
commented
1 year ago @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.
DevSrSouza
commented
1 year ago the ScreenTransition does the saveableState for you, there is no need to call CurrentScreen inside it.
Tolriq
commented
1 year ago @DevSrSouza Restore with transition still does not work talked about some times ago: https://github.com/adrielcafe/voyager/issues/27#issuecomment-1555908104
DevSrSouza
commented
1 year ago 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
Tolriq
commented
1 year ago Can you try the normal way? AndroidScreen and not disabling the necessary lifecycle.
And check in Screen2 that the state is correctly restored
L-Andrade
commented
1 year ago 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.
L-Andrade
commented
1 year ago Also worth pointing out that a BottomSheetNavigator needs to be provided the EmptyNavigatorScreenLifecycleProvider, otherwise it will also crash.
Syer10
commented
1 year ago AndroidScreenLifecycleOwner is currently meant to be added by DefaultNavigatorScreenLifecycleProvider for Screens on Android
DevSrSouza
commented
1 year ago 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.
Tolriq
commented
1 year ago 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.
L-Andrade
commented
1 year ago 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.
DevSrSouza
commented
1 year ago @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.
Tolriq
commented
1 year ago 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.
DevSrSouza
commented
1 year ago 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.
DevSrSouza
commented
1 year ago 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
DevSrSouza
commented
1 year ago 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.
Tolriq
commented
1 year ago 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.
DevSrSouza
commented
1 year ago @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.
Tolriq
commented
1 year ago I have nothing custom. Simple AndroidScreen. FadeTransition don't restore.
I'll see if hilt viewmodels support savedstatehandle with simple Screen.
DevSrSouza
commented
1 year ago @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.
DevSrSouza
commented
1 year ago @L-Andrade Hi, did my snippets fix the issue for you? If so, can we close this one?
L-Andrade
commented
1 year ago @DevSrSouza Yes, after the changes we discussed, the issues we were first experiencing are fixed.
We're not using AndroidScreen though, nor a custom AndroidScreenLifecycleOwner anymore. We're using a similar implementation as above (UniqueScreen), the EmptyNavigatorScreenLifecycleProvider, and a custom getViewModel() to add arguments to our ViewModels via SavedStateHandle.
I haven't had the chance to try out the default NavigatorScreenLifecycleProvider though. I suspect it won't correctly scope ViewModels due to the concerns mentioned in the other comments above. It would be nice if it would, as it could simplify things a bit.
I believe we can close this issue, and I will open a new one in case the above does not work as expected.
Once again, thanks for your help!
Syer10
commented
1 year ago The NavigatorScreenLifecycleProvider provides a AndroidScreenLifecycleOwner, so it will scope ViewModels properly with the default configuration, using EmptyNavigatorScreenLifecycleProvider is what removes the default AndroidScreenLifecycleOwner for the Screen.
Tolriq
commented
1 year ago @DevSrSouza I don't know how you did test but even with your code it fails to restore most of the time.
See for a repro with more levels to ensure it fails : https://github.com/adrielcafe/voyager/issues/207
L-Andrade
commented
1 year ago The
NavigatorScreenLifecycleProviderprovides aAndroidScreenLifecycleOwner, so it will scope ViewModels properly with the default configuration, usingEmptyNavigatorScreenLifecycleProvideris what removes the defaultAndroidScreenLifecycleOwnerfor the Screen.
I just tested that interaction and it does not work as expected. I removed the EmptyNavigatorScreenLifecycleProvider, of course.
There's no interaction with NavigatorScreenLifecycleProvider at all in getViewModel(). I'm not sure what I could be missing here.
It can be easily checked by debugging getViewModel(). There's this bit of code:
val lifecycleOwner = (this as? ScreenLifecycleProvider)
?.getLifecycleOwner() as? AndroidScreenLifecycleOwner
?: activityWhere this is the current Screen.
Now, if we don't need to set the Screen to implement ScreenLifecycleProvider at all, it means that the cast fails because the Screen is not a ScreenLifecycleProvider, which makes it fallback to the Activity.
If we do implement ScreenLifecycleProvider to the Screen like this (same as AndroidScreen!):
@OptIn(InternalVoyagerApi::class)
override fun getLifecycleOwner(): ScreenLifecycleOwner = DefaultScreenLifecycleOwnerIt still does not work. Because DefaultScreenLifecycleOwner is not an AndroidScreenLifecycleOwner.
Therefore, getViewModel() scopes it to the Activity as well.
The ViewModel is scoped to the Activity in both cases. AndroidScreen does not work either, because it uses "method 2".
Right now, the only way we correctly scope it to an AndroidScreenLifecycleOwner is to specify it correctly with ScreenLifecycleProvider in the Screen.
For it to work out of the box, you would need to change Voyager's getViewModel() to:
@OptIn(ExperimentalVoyagerApi::class)
@Composable
inline fun <reified T : ViewModel> Screen.getViewModel(
viewModelProviderFactory: ViewModelProvider.Factory? = null
): T {
val context = LocalContext.current
val provided = LocalNavigatorScreenLifecycleProvider.current.provide(this).firstOrNull()
return remember(key1 = T::class) {
val activity = context.componentActivity
val lifecycleOwner = (this as? ScreenLifecycleProvider)
?.getLifecycleOwner() as? AndroidScreenLifecycleOwner
?: provided as? AndroidScreenLifecycleOwner
?: activity
val factory = VoyagerHiltViewModelFactories.getVoyagerFactory(
activity = activity,
delegateFactory = viewModelProviderFactory ?: lifecycleOwner.defaultViewModelProviderFactory
)
val provider = ViewModelProvider(
store = lifecycleOwner.viewModelStore,
factory = factory,
defaultCreationExtras = lifecycleOwner.defaultViewModelCreationExtras
)
provider[T::class.java]
}
}Where the 2nd line of the function is what makes it work as expected. Or any other similar solution that will get the proper Screen LifecycleOwner instead of allowing it to fallback to the Activity
L-Andrade
commented
1 year ago @Syer10
https://github.com/adrielcafe/voyager/pull/212 from @programadorthi also seems to fix this issue since AndroidScreenLifecycleOwner has these provides:
LocalLifecycleOwner provides this,
LocalViewModelStoreOwner provides this,Just tested it out and it seems to be working well.
DevSrSouza
commented
10 months ago Fixed on 1.0.0-rc09
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, HiltViewModels andScreenLifecycleProvider. This seems to be a bug introduced in1.0.0-rc06.Some context: I have some
Tabs, and eachTabhas its ownNavigatorwith aSlideTransition. In eachTab, I can push anItem(which is aScreen)First, I tried using
AndroidScreenwith unique keys for myItem. When I navigate from any Tab to thisAndroidScreen, everything works, and myViewModelis correctly created.The issue is when navigating back to the initial screen of the
Tab, and navigating to thisAndroidScreenagain, 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 sameViewModelas the firstItem.Apparently
AndroidScreenusesDefaultScreenLifecycleOwner, and when usinggetViewModel()it will always use the parentActivityas the owner, which means it will always be the sameViewModelfor all these screens. This was introduced in1.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
ScreenLifecycleProviderto get a similar behaviour asAndroidScreen, but usingScreenLifecycleProvider.get(...)actually gives us the expected behaviour since it will create aScreenLifecycleOwner, 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 overridingScreenLifecycleProvider, I was getting this error:Downgrading Voyager to
1.0.0-rc05solved the issue though, so it seems like a bug introduced with1.0.0-rc06? Everything works as expected usingScreenLifecycleProvider.get(screen)or when usingAndroidScreenin1.0.0-rc05, as it also usesScreenLifecycleProvider.get(screen).So, I guess there are two points here:
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
keybut it actually needs the wholeScreen(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!