Zhuinden / simple-stack-compose-integration

[ACTIVE/BETA] Compose integration for Simple-Stack.
Apache License 2.0
103 stars 9 forks source link

LocalViewModelStoreOwner needs to implement HasDefaultViewModelProviderFactory #23

Open Zhuinden opened 1 year ago

Zhuinden commented 1 year ago

Since 2023-02-16, finally, the navigation compose hilt integration no longer asks explicitly for "a ViewModelStoreOwner" and then deliberately ignores it if it's not a NavBackStackEntry, but still expects a HasDefaultViewModelProviderFactory instead.

Zhuinden commented 1 year ago

To support this, the following parameters must be set:

public fun CreationExtras.createSavedStateHandle(): SavedStateHandle {
    val savedStateRegistryOwner = this[SAVED_STATE_REGISTRY_OWNER_KEY] //<--
        ?: throw IllegalArgumentException(
            "CreationExtras must have a value by `SAVED_STATE_REGISTRY_OWNER_KEY`"
        )
    val viewModelStateRegistryOwner = this[VIEW_MODEL_STORE_OWNER_KEY] // <--
        ?: throw IllegalArgumentException(
            "CreationExtras must have a value by `VIEW_MODEL_STORE_OWNER_KEY`"
        )

    val defaultArgs = this[DEFAULT_ARGS_KEY] //<--
    val key = this[VIEW_MODEL_KEY] ?: throw IllegalArgumentException( // <--
        "CreationExtras must have a value by `VIEW_MODEL_KEY`"
    )
    return createSavedStateHandle(
        savedStateRegistryOwner, viewModelStateRegistryOwner, key, defaultArgs
    )
}

As specified in https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:lifecycle/lifecycle-viewmodel-savedstate/src/main/java/androidx/lifecycle/SavedStateHandleSupport.kt?q=VIEW_MODEL_KEY .

I have a feeling that until this is supported, the VMs stored in the ViewModelStoreOwner cannot get automatic SavedStateHandle / Application, SavedStateHandle support like the original https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:lifecycle/lifecycle-viewmodel-savedstate/src/main/java/androidx/lifecycle/SavedStateViewModelFactory.kt;l=44?q=savedstateviewmodelfactory

Which however requires extras[ViewModelProvider.AndroidViewModelFactory.APPLICATION_KEY] too.

The coolest thing would be if we supported ALL of those, BUT we also had a BackstackCreationExtraKey (name subject to change) to get the Backstack into the ViewModel.

This is actually kinda required by https://github.com/Zhuinden/simple-stack-compose-integration/issues/15 which was released on 2023-02-23 🤔

Zhuinden commented 1 year ago

This would have actually been fairly straightforward to support if SavedStateViewModelFactory wasn't limited to androidx library 😒

And then they have OnRequeryFactory which is ALSO restricted to library 😒

It is literally impossible to imitate their original SavedStateViewModelFactory behavior by hand, but it's possible via SavedStateHandleSupport.enableSavedStateHandle().

/**
 * Enables the support of [SavedStateHandle] in a component.
 *
 * After this method, [createSavedStateHandle] can be called on [CreationExtras] containing this
 * [SavedStateRegistryOwner] / [ViewModelStoreOwner].
 *
 * Must be called while component is in `INITIALIZED` or `CREATED` state and before
 * a [ViewModel] with [SavedStateHandle] is requested.
 */
@MainThread
fun <T> T.enableSavedStateHandles()
    where T : SavedStateRegistryOwner, T : ViewModelStoreOwner {
    val currentState = lifecycle.currentState
    require(
        currentState == Lifecycle.State.INITIALIZED || currentState == Lifecycle.State.CREATED
    )

    // Add the SavedStateProvider used to save SavedStateHandles
    // if we haven't already registered the provider
    if (savedStateRegistry.getSavedStateProvider(SAVED_STATE_KEY) == null) {
        val provider = SavedStateHandlesProvider(savedStateRegistry, this)
        savedStateRegistry.registerSavedStateProvider(SAVED_STATE_KEY, provider)
        lifecycle.addObserver(SavedStateHandleAttacher(provider))
    }
}

/**
 * Creates `SavedStateHandle` that can be used in your ViewModels
 *
 * This function requires [enableSavedStateHandles] call during the component
 * initialization. Latest versions of androidx components like `ComponentActivity`, `Fragment`,
 * `NavBackStackEntry` makes this call automatically.
 *
 * This [CreationExtras] must contain [SAVED_STATE_REGISTRY_OWNER_KEY],
 * [VIEW_MODEL_STORE_OWNER_KEY] and [VIEW_MODEL_KEY].
 *
 * @throws IllegalArgumentException if this `CreationExtras` are missing required keys:
 * `ViewModelStoreOwnerKey`, `SavedStateRegistryOwnerKey`, `VIEW_MODEL_KEY`
 */
@MainThread
public fun CreationExtras.createSavedStateHandle(): SavedStateHandle {
    val savedStateRegistryOwner = this[SAVED_STATE_REGISTRY_OWNER_KEY]
        ?: throw IllegalArgumentException(
            "CreationExtras must have a value by `SAVED_STATE_REGISTRY_OWNER_KEY`"
        )
    val viewModelStateRegistryOwner = this[VIEW_MODEL_STORE_OWNER_KEY]
        ?: throw IllegalArgumentException(
            "CreationExtras must have a value by `VIEW_MODEL_STORE_OWNER_KEY`"
        )

    val defaultArgs = this[DEFAULT_ARGS_KEY]
    val key = this[VIEW_MODEL_KEY] ?: throw IllegalArgumentException(
        "CreationExtras must have a value by `VIEW_MODEL_KEY`"
    )
    return createSavedStateHandle(
        savedStateRegistryOwner, viewModelStateRegistryOwner, key, defaultArgs
    )
}

I'll try to put something together this weekend if I get to it.

Zhuinden commented 1 year ago

TL;DR a Viewmodel created in a LocalViewModelStoreOwner.current scoped to a Key must be able to support SavedStateHandle

I'm not sure I can actually look at it this weekend. I got stuff piled up. 😒 Maybe next week?

Zhuinden commented 1 month ago

I forgot about this issue entirely, it's been 1.5 years. I don't even remember this code at all.

Zhuinden commented 1 month ago

@matejdro i'm surprised this was not blocking you, considering it means SavedStateHandle wouldn't work in the ViewModels created with this library. What am I missing?

matejdro commented 1 month ago

We are actually mostly using ScopedService + Bundleable, so we actually did not notice this. AndroidX ViewModel support is only for some legacy screens that do not appear to use this.

Zhuinden commented 1 month ago

I'm glad this version is still suitable then and not causing any problems, then. 👍

This "module" of simple-stack, we don't actually use it, we still need Fragments (and technically I guess it would be possible to use the AndroidFragment composable but I'm not sure how trustworthy that is), so I always feel a slight bit of unease regarding whether it actually works well. I prefer to release something we actually use.

Then again, I guess https://github.com/Zhuinden/flow-ziptuple-kt continues to work but I never found an actual usecase for it either.

matejdro commented 1 month ago

To be completely honest, we are also not using this module. We have our own module (https://github.com/inovait/kotlinova/tree/master/navigation), but we have backported some of our changes to this one to give back to the community.