-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
AndroidScreen
is not working as intended in 1.0.0-rc06
- Hilt
#155
Comments
AndroidScreen
is not working as intended in `1.0.0-rc06AndroidScreen
is not working as intended in 1.0.0-rc06
@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 |
The same happens on 1.0.0-rc07? |
After updating, I'm getting a:
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. |
And using:
Still results in:
Which seems to be the same or a similar stack trace as before The other exception above ( It seems that our custom
By providing a custom 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 |
@adrielcafe I believe it's due to this bit of code in
This adds the same screen twice to the list of savable states. This was added in When logging
|
To replicate this issue, feel free to create a new Android project, add these dependencies:
Make sure to update the BOM to And in
This will result in the same crash as we have in our project: |
@adrielcafe Can you take another look when you have the chance please? 🙏 |
@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:
|
Did you try not use I did not see any reasoning why the used of the CurrentScreen in the If you take a look at the implementation of all Transitions and SlideTransition, the |
the |
@DevSrSouza Restore with transition still does not work talked about some times ago: |
I tested here and was not able to reproduce the state not being restored.
The scroll state is being kept when I navigate to the second screen, put the app in background and call |
Can you try the normal way? AndroidScreen and not disabling the necessary lifecycle. And check in Screen2 that the state is correctly restored |
Oh, cool. Didn't see that detail. A regular None of these things are mentioned in the documentation though. There's no mention of Would be nice to have an update in the docs, including a note to not use Thanks for the help @DevSrSouza. I've updated our project and it seems to be working as expected for now.
|
Also worth pointing out that a |
AndroidScreenLifecycleOwner is currently meant to be added by DefaultNavigatorScreenLifecycleProvider for Screens on Android |
Yeh, 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 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 |
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. |
Everything seems to be working as expected right now - doing some final checks.
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:
By providing the Empty provider before the At least from your previous responses, I thought the
Does this mean we don't need to implement Thanks for the help @DevSrSouza! Feels good to finally be able to update our dependencies. |
@Tolriq the It will depend on your use case. You can keep everything you are doing and use |
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. |
Sorry the delay to help with the issue.
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
I need to investigate, but, if you want to be applied in the BottomSheetNavigator as well, you can move the CompositionLocalProvider to the top. |
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 |
@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. |
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. 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 If you have a custom |
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? |
@DevSrSouza Yes, after the changes we discussed, the issues we were first experiencing are fixed. We're not using I haven't had the chance to try out the default 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! |
The |
@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 : #207 |
I just tested that interaction and it does not work as expected. I removed the There's no interaction with It can be easily checked by debugging
Where Now, if we don't need to set the If we do implement
It still does not work. Because Therefore, The ViewModel is scoped to the Activity in both cases. Right now, the only way we correctly scope it to an For it to work out of the box, you would need to change Voyager's @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 |
@Syer10 LocalLifecycleOwner provides this,
LocalViewModelStoreOwner provides this, Just tested it out and it seems to be working well. |
Fixed on |
AndroidScreen
is not working as intended in 1.0.0-rc06
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
, HiltViewModel
s andScreenLifecycleProvider
. This seems to be a bug introduced in1.0.0-rc06
.Some context: I have some
Tab
s, and eachTab
has its ownNavigator
with aSlideTransition
. In eachTab
, I can push anItem
(which is aScreen
)First, I tried using
AndroidScreen
with unique keys for myItem
.When I navigate from any Tab to this
AndroidScreen
, everything works, and myViewModel
is correctly created.The issue is when navigating back to the initial screen of the
Tab
, and navigating to thisAndroidScreen
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 sameViewModel
as the firstItem
.Apparently
AndroidScreen
usesDefaultScreenLifecycleOwner
, and when usinggetViewModel()
it will always use the parentActivity
as the owner, which means it will always be the sameViewModel
for all these screens.This was introduced in
1.0.0-rc06
(33e28d2)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 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-rc05
solved the issue though, so it seems like a bug introduced with1.0.0-rc06
? Everything works as expected usingScreenLifecycleProvider.get(screen)
or when usingAndroidScreen
in1.0.0-rc05
, as it also usesScreenLifecycleProvider.get(screen)
.So, I guess there are two points here:
AndroidScreen
still be doingoverride fun getLifecycleOwner(): ScreenLifecycleOwner = AndroidScreenLifecycleOwner.get(this)
? Why not?ScreenLifecycleProvider.get(screen)
crashes in1.0.0-rc06
. In1.0.0-rc05
it works as expected.The documentation also says it gets a
key
but 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!
The text was updated successfully, but these errors were encountered: