architecture-components-samples icon indicating copy to clipboard operation
architecture-components-samples copied to clipboard

Pass scoped parameters to ViewModel

Open kakai248 opened this issue 8 years ago • 8 comments

Hello,

I used the code in the GitHub sample. I'm currently facing an issue with the way the ViewModels are injected. Since ViewModelModule lives at the scope of the App (@Singleton), I can't pass arguments that exist at the scope of the Activity/Fragment.

I opened a thread on SO, here.

The only possible solutions I'm thinking is either having a ViewModelFactory for each ViewModel or passing the parameters after the constructor. Both options seem bad.

Is there any way around this?

Thank you.

kakai248 avatar Sep 08 '17 17:09 kakai248

First I was passing the parameters after the constructor. But I found out that I would need to have a way for each ViewModel to only initialise once and not repeat, for example, the load of data.

Then I tried the factory, and while the solution looked cleaner, I still had to write the factory. I wrote this annotationProcessor to do it for me:

https://github.com/kakai248/AutoViewModelFactory

I'm not sure if it's the best solution, but it's the best I've found so far. I would like to hear more opinions about this.

kakai248 avatar Sep 21 '17 11:09 kakai248

I also was looking for more elegant solution for this https://stackoverflow.com/questions/46278357/understanding-android-architecture-components-example-githubbrowsersample-viewm

Would be great to have at least one ViewModel with params in this sample

savjolovs avatar Sep 27 '17 13:09 savjolovs

@savjolovs That annotation processor pretty much solved my problem. I would still prefer to have only one generic factory, but with dagger-android, it seems like it's not possible.

Also, in the examples they have fragment-scoped parameters, but they pass them after the constructor. See here.

kakai248 avatar Sep 28 '17 16:09 kakai248

You may inject viewmodel at activity/fragment level by creating a viewmodel factory at each level here is an example https://gist.github.com/SamYStudiO/1510b144e6508c20522fdcf18e0e0035

SamYStudiO avatar Feb 27 '18 22:02 SamYStudiO

@kakai248 But is it ok to pass Activity/Fragment-scoped dependencies to ViewModels?

Say Foo has activity scope. After configuration change the ViewModel will be retained and keep the reference to Foo created for the dead activity, while any other place that also uses Foo will have a new instance. Looks like a (potentially) problematic situation to me.

arekolek avatar May 18 '18 10:05 arekolek

@arekolek It's okay for parameters that don't reference the activity/context. To pass an id to an object or something similar. But if that parameter holds the activity context then it's a memory leak for sure.

I prefer this approach instead of having an init in each ViewModel. I don't like to have the ViewModel in an invalid state.

kakai248 avatar May 18 '18 11:05 kakai248

@kakai248 even if it has no activity reference I see a problem. It is supposed to be a singleton for the activity scope, but the ViewModel has a different instance than anything else that is not retained.

@ActivityScope
class Foo @Inject constructor() {
    var bar: Int = 0
}

View model could write to foo.bar and activity (or whatever also has Foo) would never see it. And there could be more serious problems than that probably.

I prefer this approach instead of having an init in each ViewModel. I don't like to have the ViewModel in an invalid state.

What would be an example of a dependency for ViewModel that can't be unscoped or @Singleton-scoped? (I was recently thinking about activity-scoped dependencies for view models and the problems, but didn't have the need for them)

The only possible solutions I'm thinking is either having a ViewModelFactory for each ViewModel or passing the parameters after the constructor. Both options seem bad.

I was doing using an unscoped factory (see https://gist.github.com/arekolek/e5083776d263b16666aff38dd0d3c439) and I think it worked fine. Not sure if that ("ViewModelFactory instance for each ViewModel") is what you refer to as "ViewModelFactory for each ViewModel".

arekolek avatar May 18 '18 11:05 arekolek

@kakai248 even if it has no activity reference I see a problem. It is supposed to be a singleton for the activity scope, but the ViewModel has a different instance than anything else that is not retained.

@ActivityScope
class Foo @Inject constructor() {
    var bar: Int = 0
}

View model could write to foo.bar and activity (or whatever also has Foo) would never see it. And there could be more serious problems than that probably.

I could be making a mistake saying these parameters are activity/fragment-scoped. They are not actually annotated with a scope, so we get a new instance each time we request them. ViewModel depends on some parameters and it's the Activity/Fragment that is going to provide them.

@Module
public abstract class ProfileActivityModule extends BaseActivityModule<ProfileActivity> {

    @Nullable
    @Provides
    static String provideUserId(ProfileActivity activity) {
        return activity.getUserId();
    }
}

That getUserId() is going to return what the Activity received on the Bundle. My ViewModel depends on this id as it's a profile screen, and I need to know which user I need to fetch. The screen doesn't make sense if it doesn't have this id.

I was doing using an unscoped factory (see https://gist.github.com/arekolek/e5083776d263b16666aff38dd0d3c439) and I think it worked fine. Not sure if that ("ViewModelFactory instance for each ViewModel") is what you refer to as "ViewModelFactory for each ViewModel".

I no longer use a factory for each ViewModel. I started using a single one for all the ViewModels, as described here (or some more detail here).

kakai248 avatar May 18 '18 13:05 kakai248