Skip to content
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

Dagger/Hilt ViewModel Injection (with compose and navigation-compose) #2166

Closed
jaqxues opened this issue Nov 1, 2020 · 19 comments
Closed

Dagger/Hilt ViewModel Injection (with compose and navigation-compose) #2166

jaqxues opened this issue Nov 1, 2020 · 19 comments

Comments

@jaqxues
Copy link

jaqxues commented Nov 1, 2020

Hello,

I am currently trying to build an app with only Compose (meaning no Fragments and navigation-compose, along with architecture components such as Hilt and ViewModel).

I tried using the viewModel function with the defaultViewModelProviderFactory of the Activity.

    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.ViewModelProvider.get(ViewModelProvider.java:185)
        at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.java:150)
        at androidx.compose.ui.viewinterop.ViewModelKt.get(ViewModel.kt:75)
        at androidx.compose.ui.viewinterop.ViewModelKt.viewModel(ViewModel.kt:60)

I had to move this code inside a NavHost Composable. I reported this on the KotlinLang Slack and was told this issue relates to #2152 . It uses the incorrect Scope for a Navigation Composable.

In the case of the related issue, the scope is too small and in my case, it is the exact opposite Problem.

In that issue I linked, the fragment is within the Navigation graph, so the issue is that the saved state is too small of a scope (the navigation graph encompasses multiple fragments).
In your Compose case, the entire navigation graph in within the single Activity/Fragment, so there the scope is too large and you end up saving state multiple times with the same key.

Although the issue should be fixed by a more correct approach to scoping, it is still worth to file a bug for the inverse problem with saved state.

@Tlaster
Copy link

Tlaster commented Nov 2, 2020

Run into the same issue, a quick workaround is to pass a UUID to viewModel() as key, but this will create a new view model every time.

@danysantiago
Copy link
Member

What version of the AndroidX ViewModel extension are you using? The issue that caused the java.lang.IllegalArgumentException: SavedStateProvider with the given key is already registered was fixed in alpha02 of the extension: https://developer.android.com/jetpack/androidx/releases/hilt#1.0.0-alpha02

@Tlaster
Copy link

Tlaster commented Nov 3, 2020

I'm using 1.0.0-alpha02.
Note that this happens when using navigation-compose with jetpack compose only, which means that there's only a single activity without any fragment.
A quick example like this:

@AndroidEntryPoint
class ComposeActivity : ComponentActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContent {
            val navController = rememberNavController()
            NavHost(navController = navController, startDestination = "home") {
                composable("home") {
                    val viewModel = viewModel<HomeViewModel>(factory = defaultViewModelProviderFactory)
                    Button(onClick = {
                        navController.navigate("home2")
                    }) {
                        Text(text = "Click me!")
                    }
                }
                composable("home2") {
                    val viewModel = viewModel<HomeViewModel>(factory = defaultViewModelProviderFactory)
                    Text(text = "home2")
                }
            }
        }
    }
}

class HomeViewModel @ViewModelInject constructor(
    val sharedPreferences: SharedPreferences
) : ViewModel()

When you click the button and navigate to "home2", the app will crash

@Bodo1981
Copy link

Bodo1981 commented Nov 6, 2020

I am facing the same issue but for me the app crashes immediatelly when navigating to the second screen.
it would be great if we can have a temporarly workaround and maybe a fixed version soon.
thx for the great work

@danysantiago
Copy link
Member

Thanks for the sample code! It looks like you are hitting the same issue I've described here: #2152 (comment)

In essence using the activity or fragment as the SavedStateRegistryOwner when the ViewModelStoreOwners is not the same will cause your ViewModel to be different between the your two navigation destinations but because the SavedStateRegistryOwner has a higher scope it complains when trying to provide the same SavedStateHandle that was already consumed. We need to make HiltViewModelFactory use the SavedStateRegistryOwner provided by the Navigation library and specifically the NavBackStackEntry.

Sadly there is no workaround for now since HiltViewModelFactory's constructor is package-protected so you can't build it yourself with the right SavedStateRegistryOwner. We'll try to get this fixed soon!

@Tlaster
Copy link

Tlaster commented Nov 9, 2020

Here is a complete workaround using reflection

@Composable
inline fun <reified VM : ViewModel> navViewModel(
    key: String? = null,
    factory: ViewModelProvider.Factory? = AmbientViewModelProviderFactory.current,
): VM {
    val navController = AmbientNavController.current
    val backStackEntry = navController.currentBackStackEntryAsState().value
    return if (backStackEntry != null) {
        // Hack for navigation viewModel
        val application = AmbientApplication.current
        val viewModelFactories = AmbientViewModelFactoriesMap.current
        val delegate = SavedStateViewModelFactory(application, backStackEntry, null)
        val hiltViewModelFactory = HiltViewModelFactory::class.java.declaredConstructors.first()
            .newInstance(backStackEntry, null, delegate, viewModelFactories) as HiltViewModelFactory
        viewModel(key, hiltViewModelFactory)
    } else {
        viewModel(key, factory)
    }
}

@Composable
fun ProvideNavigationViewModelFactoryMap(factory: HiltViewModelFactory, content: @Composable () -> Unit) {
    // Hack for navigation viewModel
    val factories =
        HiltViewModelFactory::class.java.getDeclaredField("mViewModelFactories").also { it.isAccessible = true }
            .get(factory).let {
                it as Map<String, ViewModelAssistedFactory<out ViewModel>>
            }
    Providers(
        AmbientViewModelFactoriesMap provides factories
    ) {
        content.invoke()
    }
}

usage:

val AmbientApplication = staticAmbientOf<Application>()

@AndroidEntryPoint
class ComposeActivity : ComponentActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContent {
            val navController = rememberNavController()
            Providers(
                AmbientApplication provides application
            ) {
                ProvideNavigationViewModelFactoryMap(factory = defaultViewModelProviderFactory as HiltViewModelFactory) {
                    NavHost(navController = navController, startDestination = "home") {
                        composable("home") {
                            val viewModel = navViewModel<HomeViewModel>()
                            Button(onClick = {
                                navController.navigate("home2")
                            }) {
                                Text(text = "Click me!")
                            }
                        }
                        composable("home2") {
                            val viewModel = navViewModel<HomeViewModel>()
                            Text(text = "home2")
                        }
                    }
                }
            }
        }
    }
}

class HomeViewModel @ViewModelInject constructor(
    val sharedPreferences: SharedPreferences
) : ViewModel()

@Bradleycorn
Copy link

@Tlaster - In your workaround, what are AmbientViewModelProviderFactory, AmbientNavController, and AmbientViewModelFactoriesMap? Where do you set those up?

copybara-service bot pushed a commit to androidx/androidx that referenced this issue Jan 13, 2021
Add androidx.hilt:hilt-navigation and androidx.hilt:hilt-navigation-fragment containing APIs for using @HiltViewModel and Hilt's ViewModelFactory with AndroidX Navigation.
The APIs will allow users to retrieve a HiltViewModelFactory whose view model owner and saved state owner is the NavBackStackEntry. Additionally it provide a kotlin extension, `hiltNavGraphViewModels` which mimics `navGraphViewModels`.

These artifacts essentially tackle:
* google/dagger#2152
* google/dagger#2166

Test: HiltNavGraphViewModelLazyTest
Relnote: Provide APIs for using @HiltViewModel with Navigation.

Change-Id: I00e675363f5af3922205a30f4670a4c33877a7b3
@Guimareshh
Copy link

@danysantiago just saw you referenced the issue in a commit inside AndroidX for Hilt package. Do you have any ETA on this being released ? The last update for androidx.hilt was in July 2020.

Also, for those using only Dagger2 and not Hilt. If we want to go full Compose (no Fragments and navigation-compose) do we have to switch to Hilt to make DI works ?

@danysantiago
Copy link
Member

@Guimareshh, we have a scheduled release of the androidx.hilt artifacts on January 27th (if all goes well), sorry for the trouble. As the reference comment shows, there will be a hilt-navigation artifact you can use to get retrieve a ViewModel out of a NavBackStackEntry

You don't have to switch to Hilt to make DI work with Compose, but Hilt still makes some things easier, such as App, Activity and Service injections along with ViewModel injection. Note that the functions in hilt-navigation scheduled for release will also let you use navigation-compose ViewModels which have more granular scopes.

@Guimareshh
Copy link

Thanks for the detailed reply @danysantiago 👍

About your answer on Dagger/Hilt: I asked you this because I'm not comfortable with the design decisions of Hilt not being able to insert components in the middle of the hierarchy (explained here). I imagine that if I want to go with Dagger2 without Hilt, with a full Compose app (with navigation-compose), I will have to build my own Factories.

@rafipanoyan
Copy link

rafipanoyan commented Jan 15, 2021

@danysantiago Indeed as soon as a custom component is needed to inject our objects from, Hilt does not avoid the dagger-style boilerplate since we need entry points to inject the dependencies if I'm not mistaken

@argestes
Copy link

@danysantiago Did you release said component?

@danysantiago
Copy link
Member

danysantiago commented Jan 28, 2021

Hey - For Compose you can use hilt-navigation:

val myViewModel: MyViewModel = viewModel(HiltViewModelFactory(AmbientContext.current, backStackEntry))

We didn't release a hilt-navigation-compose artifact because we are still trying to hash out the APIs: https://android-review.googlesource.com/c/platform/frameworks/support/+/1551264

I'll close this for now since HiltViewModelFactory(AmbientContext.current, backStackEntry) should unblock you but I do think we can make this better and nicer. :)

@mitchtabian
Copy link

mitchtabian commented Jan 28, 2021

The hilt-navigation version 1.0.0-alpha03 is great. ViewModels are retained via the NavBackStackEntry now. Love it.

There's still a limitation with building a "compose only" navigation system though. I will try to explain with an example.

Suppose you have a bottom nav with 3 items. You could easily implement that with something like this:

val navController = rememberNavController()
Scaffold(
    bottomBar = {
        BottomNav(navController)
    }
) {
    NavHost(navController = navController, startDestination = Home.route) {
        composable(route = Home.route) {
            HomeScreen()
        }
        composable(route = Profile.route) {
            ProfileScreen()
        }
        composable(route = Settings.route) {
            SettingsScreen()
        }
    }
}

nav demo

Navigating using the back button works great now (Data is persisted) since the ViewModel gets its ViewModelStoreOwner from the NavBackStackEntry. Horray.

The problem

But if you click on any of the entries a second time, the screen stacks. A new viewmodel is created since a new NavBackStackEntry is created.

2

Solution?

We need something to prevent the stacking of back stack entries. If an item is re-selected, it should be removed from its position in the stack and placed at the top.
3

launchSingleTop is great but only works if the entry is already at the top of the stack.

Thanks and sorry for the long post I probably should have created a feature request. One might already exists?

@danysantiago
Copy link
Member

@mitchtabian, these screenshots are amazing at explaining the issue, thanks!

However, this is not a Hilt + ViewModel issue and more of the way Navigation currently works. If the NavBackStackEntry is being removed and a new one is created so will the ViewModels. It seems you want multiple back stacks which is a highly requested feature in the Navigation library already and something that is being worked on. I suggest you file a feature request in the Navigation issue tracker here to make sure this is also available for navigation-compose.

@05nelsonm
Copy link

05nelsonm commented Jan 28, 2021

@mitchtabian See below for your boolean:

val isOnBackstack = try {
    navController.getBackStackEntry(destinationID)
    true
} catch(e: IllegalArgumentException) {
    false
}

@mitchtabian
Copy link

mitchtabian commented Jan 28, 2021

@danysantiago Thank you for your reply!

@05nelsonm Yes of course I can get a boolean that tells me if it's already in the stack. What I meant was if we as developers had a simple boolean similar to launchSingleTop that we could set to tell the navigation system how to behave.

@mitchtabian
Copy link

Created a feature request in case anyone wanted to star/follow. https://issuetracker.google.com/issues/178796184

@kasem-sm
Copy link

This is now fixed na? You told us in a video and we starred the issue haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests