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

Cleaner way to inject activities in a multiple activity app? #428

Closed
plusmobileapps opened this issue Apr 17, 2019 · 15 comments
Closed

Cleaner way to inject activities in a multiple activity app? #428

plusmobileapps opened this issue Apr 17, 2019 · 15 comments

Comments

@plusmobileapps
Copy link

plusmobileapps commented Apr 17, 2019

At my day job, I'm working in a project that spans across multiple activities and there are some classes in the project that depend on an activity such as Google Pay. The team is looking for a cleaner way to inject activities into our dependencies and we were looking at Koin as a replacement to our home grown DI.

I have done research which landed me on this Koin issue on Github which has a few suggestions on injecting activities It seemed that the towards the end of the issue, the suggested approach is to use injection parameters like so:

//LocationModule.class
factory { (activity: MainActivity) -> FusedLocationProviderClient(activity) }

//MainActivity.class
val provider: FusedLocationProviderClient by inject { parametersOf(this@MainActivity) }

This is great if the dependency is actually being used in the view, however in my use case it is injected to an Interactor further back in the architecture and could be tedious to have to declare anything that requires an activity dependency in two locations instead of one.

So one approach I took to try to accomplish this was to create a kotlin object that actually keeps track of the current activity with a stack. There were some concerns about the correct activity being supplied and potential memory leaks.

object ActivityStack {
    private val stack = Stack<BaseActivity>()
    val currentActivity: BaseActivity
        get() = stack.peek()
    fun push(activity: BaseActivity) = stack.push(activity)
    fun pop() = stack.pop()
}

//then declared inside of a module 
factory<GooglePayDataSource> { GooglePayDataSourceImpl(ActivityStack.currentActivity) }
factory { GooglePayInteractor(get()) }

The second approach was creating an extension function on our BaseActivity to declare any activity dependencies. But this required me to override any dependencies declared in Koin which in my use case don't think would be an issue because the Interactor is also instantiated in the creation of the activity so it has a reference to GooglePayDataSource already. Although I could see this being an issue potentially if the dependency is not needed right away.

fun BaseActivity.declareActivityDependencies() {
    val activityModule = module {
        factory<GooglePayDataSource>(override = true) { GooglePayDataSourceImpl(this@declareActivityDependencies) }
    }

    loadKoinModules(activityModule)
}

There were also concerns of the above approach having memory leaks since the documentation on scoping state the following.

Most of Android memory leaks comes from referencing a UI/Android component from a non Android component. The system keeps a reference on it and can’t totally drop it via garbage collection.

So is there any clean way to do activity injection with Koin that would ensure there are no memory leaks and the current activity at the top of the stack is supplied?

It would be nice to just declare the dependencies like so for simplicity.

factory { GooglePayDataSource(currentActivity()) }
@droidluv
Copy link

droidluv commented Apr 17, 2019

I second this post, even I was looking for a cleaner way to deal with view controller(Activity, Fragment etc) injection far down deep in an architecture layer, like I already hate the fact that I have to provide a view controller for a presenter as a parameter like val presenterObject: SomePresenter by inject { parametersOf(this) } where "this" is the current view, this solution resolves the problem only if the injecting class has a view instance

that being said I saw something interesting in Koin documentation with scopes

scope(named<DetailActivity>) {
        scoped<DetailContract.Presenter> { DetailPresenter(get(), get()) }
    }

It would be nice to have something like

scope(named<DetailActivity>) {
        scoped<DetailContract.View> fromScopeProvider //This part
        scoped<DetailContract.Presenter> { DetailPresenter(get(), get()) }
}

@arnaudgiuliani
Copy link
Member

yeah, you can use the currentScope of an activity to inject scoped definition instead of a factory.
The thing here would be to be able to retrieve the lifecycle owner object by the currentScope property

@erickok
Copy link
Contributor

erickok commented Apr 18, 2019

Not sure if Koin should deal with this. It is easy enough to make an ActivityProvider and use that?

class ActivityProvider(application: Application) {

    var activeActivity: Activity? = null

    init {
        application.registerActivityLifecycleCallbacks(object : Application.ActivityLifecycleCallbacks {
            override fun onActivityPaused(activity: Activity?) {
                activeActivity = null
            }
            override fun onActivityResumed(activity: Activity?) {
                activeActivity = activity
            }
            // ...
        })
    }

}

val testModule = module {
    single { ActivityProvider(get()) }
    factory { GooglePayDataSource(get<ActivityProvider>().activeActivity) }
}

@arnaudgiuliani
Copy link
Member

could it be easily done at first currentScope creation also.

But yeah, don't know yet if it worth to add more android utils components like that.

@plusmobileapps
Copy link
Author

If I were to use the currentScope of the activity to inject, then wouldn't I also need to declare the scope for each activity in the app like @droidluv was suggesting? I was hoping to reduce this declaration down to one call because there is still about 15 activities left in our app and would be tedious to declare scoped dependencies for each activity?

Otherwise @erickok, that is another suggestion my team has made on how to approach this. If that were to be done though would there be a guarantee the lifecycles wouldn't do something funny and provide the wrong activity just because I remember from a talk awhile back explaining the difference between Events and States of the activity lifecycle

image

@droidluv
Copy link

Isn't injection using scopes a better way of programming for view controllers, anything that's scoped to the view controller will be tied to the lifetime of the view controller and generally seems the right way to define dependencies which should live as long as the view controller exists.

Obtaining something like the "current activity" feels very unsafe and wouldn't that require the library to track every activity that is being resumed and visible and somehow hold a reference to it?

Anyway I suppose it's upto the author, I'm enjoying this library and would like it to be suitable for any architecture types and be more powerful as a DI library than a service locator.

@plusmobileapps
Copy link
Author

Yeah I agree that this is a better way of programming, my team is currently trying to migrate to a single activity where this problem would not be as big of an issue because then it is simply one activity and could probably just use parameter injection. But with the 15 activities still left was hoping there might be a cleaner way to inject each activity with the right dependencies without having to declare in each activity.

Otherwise I really enjoy this library and use it already in all of my other pet projects I make!

@pagsantos
Copy link
Contributor

could it be easily done at first currentScope creation also.

But yeah, don't know yet if it worth to add more android utils components like that.

@arnaudgiuliani can you post a sample on how does it look like?

@arnaudgiuliani
Copy link
Member

Scope can be declared as "standalone", which mean that you handle it manually. But ... it's not tied to any Activity nor other lifecycle components. Up to you create a scope instance, retrieve it by its ID etc...

I'm not a big fan of currentActivity() or stuff that track current activity to inject stuff. For me, there is another way to do it.

First Activity can create the scope: getKoin.createScope(ID,ScopeQualifier)
On other Activity or even Fragment, just retrieve this scope with getKoin().getScope(ID)
When you have finished with it, just close it.
the only thing to keep here, it the Scope instance ID that every component shares.

@droidluv
Copy link

droidluv commented Oct 5, 2019

@arnaudgiuliani some objects are tied to the lifecycle of an activity or a Fragment, like the viewModels Koin conveniently supports, it is tied to the lifecycle of either an activity or a fragment and we need an activty or fragment context for injecting one, right now the below style of scoping works perfectly for me, but because of how Koin works I cannot retrieve say for example a viewModels coroutine scoped (viewModelScope) which I might want to pass to other injected classes so I can terminate any running jobs when the viewModel gets cleared

scope(named<LoginFragment>()) {
        scoped<LoginAPI> {
            val retrofit: Retrofit = get()
            retrofit.create(LoginAPI::class.java)
        }
        factory { LoginInteractor(get(), get()) }
        viewModel { LoginViewModel(get(), get()) }
    }

in the above case I can inject my scoped viewmodel like

private val loginViewModel: LoginViewModel by currentScope.viewModel(this)
and about shared view models since it belongs to an another activity you cannot call it with currentScope anyway

With Navigation Architecture and Single Activity architecture Koin is working wonders for me but is there a more easy or idiomatic way to inject things like LoginInteractor(get(), get()) without having to define its factory or single, so I can make it behave more like an actual DI framework, I know Koin is a service locator but this is a bit annoying that I have to define every single object that I have to inject.

@thiagolacerdas
Copy link

@arnaudgiuliani Hi!! I have a similar situation: there is Interactor that needs activity reference and I'm injecting this interactor into viewModel constructor, but this way I can't use custom arguments.

BillingInteractor(activity: Activity) {
...
}

MyViewModel(private val billingInteractor: BillingInteractor) : ViewModel() {
...
}

@makaroffandrey
Copy link

Here is my hacky solution to this (requires Koin 2.1.0 or higher).
I have created an activityScope extension like this:

inline val <reified T : AppCompatActivity> T.activityScope: Scope
    get() = lifecycleScope
        .addInstance(this)
        .addInstance(this as Activity)
        .addInstance(this as AppCompatActivity)

inline fun <reified T> Scope.addInstance(instance: T): Scope {
    val definition = BeanDefinition(
        scopeDefinition = this._scopeDefinition,
        primaryType = T::class,
        kind = Kind.Single,
        definition = { instance }
    )
    val factory = SingleInstanceFactory(this._koin, definition)
    (this._instanceRegistry.instances as HashMap<IndexKey, InstanceFactory<*>>)[indexKey(
        T::class, null
    )] = factory
    return this
}

Then in my Scope declaration, I can do this:

scope<MyActivity> {
    scoped {
        MyActivityDependentComponent(get<Activity>()) // get<MyActivity>() is also possible
    }
}

And in MyActivity I do this:

class MyActivity: AppCompatActivity() {
    private val activityDependentComponent: MyActivityDependentComponent
            by activityScope.inject()
}

A similar solution is possible for Fragments.

@SergeyBurlaka
Copy link

SergeyBurlaka commented Apr 10, 2020

Not sure if Koin should deal with this. It is easy enough to make an ActivityProvider and use that?

class ActivityProvider(application: Application) {

    var activeActivity: Activity? = null

    init {
        application.registerActivityLifecycleCallbacks(object : Application.ActivityLifecycleCallbacks {
            override fun onActivityPaused(activity: Activity?) {
                activeActivity = activity
            }
            override fun onActivityResumed(activity: Activity?) {
                activeActivity = null
            }
            // ...
        })
    }

}

val testModule = module {
    single { ActivityProvider(get()) }
    factory { GooglePayDataSource(get<ActivityProvider>().activeActivity) }
}

@erickok
You have missed up pause and resume

@erickok
Copy link
Contributor

erickok commented Apr 10, 2020

You are right, they should is course be reversed.

@ronjunevaldoz
Copy link

Not sure if Koin should deal with this. It is easy enough to make an ActivityProvider and use that?

class ActivityProvider(application: Application) {

    var activeActivity: Activity? = null

    init {
        application.registerActivityLifecycleCallbacks(object : Application.ActivityLifecycleCallbacks {
            override fun onActivityPaused(activity: Activity?) {
                activeActivity = null
            }
            override fun onActivityResumed(activity: Activity?) {
                activeActivity = activity
            }
            // ...
        })
    }

}

val testModule = module {
    single { ActivityProvider(get()) }
    factory { GooglePayDataSource(get<ActivityProvider>().activeActivity) }
}

is this supported now?

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

9 participants