Beta13 hiltviewmodel crash when navigating back to same destination

[Tolriq]

So the fix for the replace issue have now generated a worse issue.

    java.lang.IllegalArgumentException: SavedStateProvider with the given key is already registered
        at androidx.savedstate.SavedStateRegistry.registerSavedStateProvider(SavedStateRegistry.java:111)
        at androidx.lifecycle.SavedStateHandleController.attachToLifecycle(SavedStateHandleController.java:50)
        at androidx.lifecycle.SavedStateHandleController.create(SavedStateHandleController.java:70)
        at androidx.lifecycle.AbstractSavedStateViewModelFactory.create(AbstractSavedStateViewModelFactory.java:67)
        at androidx.lifecycle.AbstractSavedStateViewModelFactory.create(AbstractSavedStateViewModelFactory.java:84)
        at dagger.hilt.android.internal.lifecycle.HiltViewModelFactory.create(HiltViewModelFactory.java:109)
        at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.kt:171)
        at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.kt:139)

it will crash whenever you navigate again to a destination.

So push(Screen) pop() push(Screen) will crash.

As said in https://github.com/adrielcafe/voyager/issues/17 the root issue is more about transitions and autodispose happening too early.

There's side effects too with ScreenModel that are disposed too early (even before the next screen is composed). This can corrupt what is displayed during transition and cause hard to diagnose side effects.

This is more a core issue for @adrielcafe than an hilt specific one. (The "fix" stills needs to be reverted) While we can disable autodispose at the navigator level we have no way to built custom transitions that would do a proper dispose after the end of the transition.

[programadorthi]

Hi @Tolriq. This issue happens because using @HiltViewModel couple it instance creation to the default ViewModelFactory provided by Hilt and your own StateRegistry that is the current Activity. We have overrided this behavior to use voyager AndroidScreenLifecycleOwner instead. Checkout #21

[Tolriq]

Well I know my English is bad but the dispose too early issue is real too :) I'll open another outside of the crashes if necessary.

BTW

internal val Context.componentActivity: ComponentActivity
    get() = this as? ComponentActivity

Is not covering all cases, from discussions with Compose team there's cases when LocalContent.current can be a context wrapper and you need to check the baseContext until you find it

fun Context.findComponentActivity(): ComponentActivity? = when (this) {
    is ComponentActivity-> this
    is ContextWrapper -> baseContext.findActivity()
    else -> null
}

[programadorthi]

@Tolriq Sorry if you think that we are not understanding what you are saying. You are helping us a lot and thank for that. About dispose been called early, this is not a voyager fault. Voyager Navigator has DisposableEffect with key being the current screen key. Navigator change from current screen to next replacing current composition. This behavior triggers DisposableEffect onDispose before next screen has finished its composition. It's a default Jetpack Compose Behavior. Here a sample to you see that dispose early is not a Voyager fault:

class MainActivity : ComponentActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContent {
            ComposeInternalsTheme {
                Surface(color = MaterialTheme.colors.background) {
                    var state by remember { mutableStateOf(true) }
                    if (state) {
                        Greeting(
                            name = "Screen 1",
                            modifier = Modifier.clickable { state = !state },
                        )
                    } else {
                        Greeting(
                            name = "Screen 2",
                            modifier = Modifier.clickable { state = !state },
                        )
                    }
                }
            }
        }
    }
}

@Composable
fun Greeting(modifier: Modifier = Modifier, name: String) {
    DisposableEffect(key1 = name) {
        println(">>>> creating effect to $name")
        onDispose {
            println(">>>> disposing screen $name")
        }
    }
    Text(text = "Hello $name!", style = MaterialTheme.typography.h1, modifier = modifier)
}

If you run this code you'll see this sequence of prints:

// First run
I/System.out: >>>> creating effect to Screen 1
// After click
I/System.out: >>>> disposing screen Screen 1
I/System.out: >>>> creating effect to Screen 2
// After click again
I/System.out: >>>> disposing screen Screen 2
I/System.out: >>>> creating effect to Screen 1

As you see, Jetpack Compose is always disposing previous composition before compose next.

Thanks again about Context extension function. Yes, I forgot that someone can use Voyager inside custom view to have integrations with legacy code. I think that the way to LocalContext be a ContextWrapper is when it's called inside a Custom View. I'll update the extension function in a current open pull request.

[programadorthi]

Context extension function improved on #22

[Tolriq]

Sorry if you think that we are not understanding what you are saying.

It's a little the same as the hilt empty maps, the whole context of what I write is important not just the first part leading to explanation of how compose / dagger works.

Anyway the issue is about Transitions when you transition from screen 1 to screen 2, currently screen 1 is disposed while still visible on screen before the transition even starts. With autodispose this will dispose the viewmodel, leading to state changes, leading to what is visible on screen during the transition being wrong or the viewmodel being recreated using resources during transition.

When using transitions, autodispose from the navigator should be at false, and transition should be responsible for the viewmodel disposal after the transition ends not before it starts.

Unfortunately this is not possible with current all internal disposal code to hack something ourselves even in custom transitions.

[programadorthi]

Hey guys, the voyager owner is very busy and his wife is pregnant. Updates will come soon. Stay up to date.

[Tolriq]

I hope he have stored enough sleep :) He's next 1,5 years will remove a lot of it :p