bluelinelabs / Conductor

A small, yet full-featured framework that allows building View-based Android applications
Apache License 2.0
3.9k stars 344 forks source link

Compatibility with Android Architecture Components #395

Open tomblenz opened 6 years ago

tomblenz commented 6 years ago

I believe that the issues raised in https://github.com/bluelinelabs/Conductor/issues/302 merit addressing.

Due to tight coupling with Fragments, the Arch Component library doesn't play nicely with Conductor. Controllers must use their Activity as their VM provider, which binds them to the Activity lifecycle. This implies some serious problems

  1. The ViewModel will be retained for the lifetime of the Activity - forever for many Conductor apps that use a single Activity
  2. Controllers that are used in multiple places in one Activity all share the same ViewModel. This is obviously a problem for more generically designed Controllers (eg. a controller that displays a news article) but also means that Controllers that are disposed and re-added later are provided a stale ViewModel. (A workaround for this is to supply a unique key to the ViewModelProvider)

In addition, the current implementation of the experimental LifecycleController does not have parity with Activities/Fragments in that LiveData will continue to emit data to the controller after the underlying Activity is stopped.

I'll repeat what I said in the linked issue because I think it is important,

I simply consider AC to be so strong that even minor trouble integrating it with other libs should be addressed as a priority. I forsee AC to be a strong part of the future of Android development and it's important to keep up in the formative stages.

I'm not sure how these problems can be solved.

tomblenz commented 6 years ago

I've been looking into the internals and I believe this solution may work for 1 and 2 - probably needs some eyes and testing over it.

object ConductorViewModelProviders {

    private var defaultFactory: ViewModelProvider.Factory? = null

    private fun initializeFactoryIfNeeded(application: Application): ViewModelProvider.Factory {
        return defaultFactory ?: ViewModelProviders.DefaultFactory(application).apply { defaultFactory = this }
    }

    private fun checkApplication(controller: Controller): Application {
        return controller.activity?.application ?: throw IllegalStateException("Your controller is not yet attached to Application.")
    }

    fun of(controller: LifecycleController): ViewModelProvider {
        return ViewModelProvider(ConductorViewModelStores.of(controller),
                initializeFactoryIfNeeded(checkApplication(controller))
        )
    }

    fun of(controller: LifecycleController, factory: ViewModelProvider.Factory): ViewModelProvider {
        checkApplication(controller)
        return ViewModelProvider(ConductorViewModelStores.of(controller), factory)
    }
}

object ConductorViewModelStores {

    private val stores = mutableMapOf<LifecycleController, ViewModelStore>()

    fun of(controller: LifecycleController): ViewModelStore {
        var store = stores[controller]
        if (store == null) {
            store = ViewModelStore()
            stores[controller] = store

            controller.addLifecycleListener(object: Controller.LifecycleListener() {
                override fun postDestroy(controller: Controller) {
                    stores[controller]?.clear()
                    stores.remove(controller)
                }
            })
        }
        return store
    }
} 

Usage is the same as Activity/Fragment:

class MyController : LifecycleController() {

    private val viewModel by lazy { ConductorViewModelProviders.of(this).get(MyViewModel::class.java) }

    // ...
}

LifecycleController is defined pretty much the same as @StephanSchuster outlined in the above linked issue, but here it is in its entirety.

abstract class LifecycleController(args: Bundle? = null) : Controller(args), LifecycleOwner {

    private val registry by lazy { LifecycleRegistry(this) }

    init {
        registry.handleLifecycleEvent(Lifecycle.Event.ON_CREATE)
        registry.markState(Lifecycle.State.CREATED)
        addLifecycleListener(object: LifecycleListener() {
            override fun preCreateView(controller: Controller) {
                registry.handleLifecycleEvent(Lifecycle.Event.ON_START)
            }

            override fun postCreateView(controller: Controller, view: View) {
                registry.markState(Lifecycle.State.STARTED)
            }

            override fun preAttach(controller: Controller, view: View) {
                registry.handleLifecycleEvent(Lifecycle.Event.ON_RESUME)
            }

            override fun postAttach(controller: Controller, view: View) {
                registry.markState(Lifecycle.State.RESUMED)
            }

            override fun preDetach(controller: Controller, view: View) {
                registry.handleLifecycleEvent(Lifecycle.Event.ON_PAUSE)
            }

            override fun postDetach(controller: Controller, view: View) {
                registry.markState(Lifecycle.State.STARTED)
            }

            override fun preDestroyView(controller: Controller, view: View) {
                registry.handleLifecycleEvent(Lifecycle.Event.ON_STOP)
            }

            override fun postDestroyView(controller: Controller) {
                registry.markState(Lifecycle.State.CREATED)
            }

            override fun preDestroy(controller: Controller) {
                registry.handleLifecycleEvent(Lifecycle.Event.ON_DESTROY)
            }

            override fun postDestroy(controller: Controller) {
                registry.markState(Lifecycle.State.DESTROYED)
            }
        })
    }

    override fun getLifecycle() = registry
}

Activity uses its fragment manager to keep a retained instance with its ViewModelStore within. Fragment uses the same strategy but using its child fragment manager. In this way, when either activity or fragment is destroyed, its store is automatically cleaned up.

Since Controllers aren't subject to lifecycle events, we don't need to keep them in retained fragments and can map their hard reference to their store (see the stores map above). It just means that we need to manually clean up after them, which we do via Controller.LifecycleListener.postDestroy().

This doesn't solve the onStop issue described above but that issue is minor in comparison.

tomblenz commented 6 years ago

Revisting this, I think it makes more sense to fold these classes into LifecycleController. Individual controllers can handle their own view model stores and provide their own providers.

abstract class LifecycleController(args: Bundle? = null) : Controller(args), LifecycleOwner {

    private val registry by lazy { LifecycleRegistry(this) }
    val store by lazy { ViewModelStore() }

    init {
        registry.handleLifecycleEvent(Lifecycle.Event.ON_CREATE)
        registry.markState(Lifecycle.State.CREATED)
        addLifecycleListener(object: LifecycleListener() {
            override fun preCreateView(controller: Controller) =
                    registry.handleLifecycleEvent(Lifecycle.Event.ON_START)
            override fun postCreateView(controller: Controller, view: View) =
                    registry.markState(Lifecycle.State.STARTED)
            override fun preAttach(controller: Controller, view: View) =
                    registry.handleLifecycleEvent(Lifecycle.Event.ON_RESUME)
            override fun postAttach(controller: Controller, view: View) =
                    registry.markState(Lifecycle.State.RESUMED)
            override fun preDetach(controller: Controller, view: View) =
                    registry.handleLifecycleEvent(Lifecycle.Event.ON_PAUSE)
            override fun postDetach(controller: Controller, view: View) =
                    registry.markState(Lifecycle.State.STARTED)
            override fun preDestroyView(controller: Controller, view: View) =
                    registry.handleLifecycleEvent(Lifecycle.Event.ON_STOP)
            override fun postDestroyView(controller: Controller) =
                    registry.markState(Lifecycle.State.CREATED)
            override fun preDestroy(controller: Controller) =
                    registry.handleLifecycleEvent(Lifecycle.Event.ON_DESTROY)
            override fun postDestroy(controller: Controller) =
                    registry.markState(Lifecycle.State.DESTROYED) })
    }

    override fun getLifecycle() = registry

    override fun onDestroy() {
        super.onDestroy()
        store.clear()
    }

    fun viewModelProvider(factory: ViewModelProvider.Factory = defaultFactory(this)) 
            = ViewModelProvider(store, factory)

    companion object {
        private var defaultFactory: ViewModelProvider.Factory? = null

        private fun defaultFactory(controller: Controller): ViewModelProvider.Factory {
            val application = controller.activity?.application ?: throw IllegalStateException("Your controller is not yet attached to Application.")
            return defaultFactory ?: ViewModelProviders.DefaultFactory(application).apply { defaultFactory = this }
        }

    }
}

Implementing controllers get their VMs like this:

class MyController : LifecycleController() {

    private val viewModel by lazy { viewModelProvider().get(MyViewModel::class.java) }

    // ...
miquelbeltran commented 6 years ago

Hi, I had a similar need and ended doing something similar what you do on the second comment, creating my own abstract ViewModelController() that extends the LifecycleController(), which contains its own store/provider for ViewModels.

My code:

abstract class BaseViewModelController : LifecycleController() {

    // This Controller instance has its own ViewModelStore
    private val viewModelStore = ViewModelStore()

    // A custom Factory can be provided
    fun viewModelProvider(factory: ViewModelProvider.NewInstanceFactory = defaultFactory()): ViewModelProvider {
        return ViewModelProvider(viewModelStore, factory)
    }

    private fun defaultFactory() = ViewModelProviders.DefaultFactory(activity!!.application)
}
sirvon commented 6 years ago

any sample projects with this new implementation?

miquelbeltran commented 6 years ago

@sirvon here's a simple demo app showing how to use Architecture Components ViewModel with Conductor with the above solution. This is the Controller that uses the BaseViewModelController: https://github.com/miquelbeltran/conductor-talk-demo/blob/master/app/src/main/java/work/beltran/conductortalkdemo/arch/ViewModelController.kt check the rest of the project to see more uses

muccy commented 6 years ago

@miquelbeltran thanks. The talk which refers to this repo has already been released?

miquelbeltran commented 6 years ago

@muccy you welcome! The talk will happen in February at DroidKaigi, hopefully will be recorded!

Zhuinden commented 6 years ago

I'm actually a bit confused why ViewModel integration is needed in a Controller.

Controllers already survive config change, ViewModel gives them nothing of value, or so I would think?

Unless you want to get data shared by the Activity via its ViewModel's LiveData, but that doesn't have much to do with Controller lifecycle either.

tomblenz commented 6 years ago

@Zhuinden controller's views are detached when the view is stopped, so the controller must keep track of whether the view is attached. This violates MVVM, which is the architecture implied by AAC. LiveData is useless without the subscriber (ie. the Controller) being able to report on its lifecycle state. On the flip side, Conductor is redundant if you're just using it as a VM lib.

Zhuinden commented 6 years ago

the controller must keep track of whether the view is attached. This violates MVVM

Not if the View subscribes to LiveData exposed by the Controller, theoretically

yshrsmz commented 6 years ago

You can create your own ViewModel(or whatever you want to call) class which is not a sub class of AAC's ViewModel, so that you can separate View logic from your "ViewModel".

arunkumar9t2 commented 6 years ago

@Zhuinden Why I think ViewModel integration is needed, even though Controllers survive config changes is simply this: I really don't want any data loading or other logic code in my Controllers. I'd rather have them in the ViewModel which talks to Repository, does Rx transformations or executes use cases, whatever and just exposes a Data that the view renders.

One could argue, why not use a separate class (use cases? or like @yshrsmz said) that does loading and let it be a field of Controller so that logic is lifted out of Controller and also survives config changes. But again, we need something akin to ViewModel#onCleared to know when to cleanup resources. I am not sure if we can use onDestroy here since we need to differentiate between process kill, back pressed, config changes.

Controllers surviving config changes is same as a retained fragment which have to be carefully dealt with since a strong reference could cause a leak eg: #234 while detach/attach.

So what I would like to do is to let Controller be as dumb as possible (takes state from viewmodel and renders it and nothing else) and use ViewModel for loading data/business logic. This is already possible with Fragments and I would very much like if there is a way to do this with Conductor.

miquelbeltran commented 6 years ago

I think what @Zhuinden questions is what advantages provide arch-comp ViewModel vs creating your own ViewModel, like we used to do with MVVM before arch-comp, considering that Controllers survive config changes.

For me the goals are different:

  1. Arch-comp. ViewModels will hopefully become the standard way of decoupling logic from the view and be taught to new developers just like Loaders were back in the days.

  2. By providing a turn-key solution, rather than a custom one, you help beginners avoid pitfalls (especially the ones related to livecycle)

  3. You make easier for beginners to apply the same knowledge and examples for Activities and Fragments with Conductor, which leads to a larger adoption of this library.

So Zhuinden is right that you can build your own custom solution with the advantage of Controllers surviving configuration changes, but I think the advantages of having good Arch-Comp. support go beyond that.

tomblenz commented 6 years ago

@Zhuinden I get it, but at that point what's the point of Conductor?

My org treats Controllers as Fragment (and Activity) replacements which essentially means they are the Views. That they stick around after config changes is meaningless to me because I don't want my VM or Presenter logic in my View classes.

Zhuinden commented 6 years ago

@tomblenz mostly view-based design and a reliable backstack?

I'd think ViewModel of AAC is also an Android specific thing, technically a retained fragment. So if that's a VM, then fragment + ViewModel is technically the same thing as a Conductor Controller.

tomblenz commented 6 years ago

How do you architect your app frontend?

In my case, Controller is a terminating end (drill any deeper and you just get inflated XML layouts) and it is impacted by the Android lifecycle so it makes perfect sense to treat it as my View layer. While I could technically have LiveDatas in a Controller, I'd have to subscribe to them via XML databinding, which again I treat as being in the same architectural class as an activity, fragment or controller - it doesn't make sense to have a View pubsub a View to me.

So the next layer up is the VM (or Presenter but MVP is trivial to implement), and an AAC ViewModel is a perfect fit for that (by design). It exposes publishers, the View layer subscribes to them, we have (a) separation of concerns and (b) a presentation class that is completely ignorant of whatever crazy lifecycle things the activity/controller is doing alongside it.

XML+Controller (View) <-> ViewModel (state/presenter/unit testable logic) <-> Model 
tomblenz commented 6 years ago

I'd think ViewModel of AAC is also an Android specific thing, technically a retained fragment. So if that's a VM, then fragment + ViewModel is technically the same thing as a Conductor Controller.

Strictly speaking, in AAC the ViewModelProvider is held in a retained fragment (stored in the fragment manager of Activity or child fragment manager of Fragment), which in turn holds its ViewModels.

The equivalent with Conductor would be the Controller maintaining its ViewModelProvider, which again in turn holds its VMs. Because the Controller is already retained, this maintenance is trivial (a property reference is fine). This is what I've implemented in my solutions above. Again, this solution places the Controller on the same level as an Activity or Fragment: a part of the View layer.

esafirm commented 6 years ago

@arunkumar9t2 ViewModel#onCleared is called in HolderFragment#onDestroy. Isn't it the same as Controller#onDestroy?

@tomblenz like @yshrsmz say. You can easily create a class called ViewModel or whatever and have your logic there. Want the view to subscribe the data change? Easy use LivaData with AAC Lifecycle or an Observable with auto dispose.

@miquelbeltran just a random though, instead of using AAC ViewModel why not creating a simple implementation (with the same methods name and stuff but less class) for Conductor only. Like a Reactive Stream specification or something like that.. 😄 You ended up with similar syntax (ViewModelProvider.of) and less methods count.

tomblenz commented 6 years ago

What would you subscribe LiveData with? Controller isn't a LifecycleOwner and you can't use the underlying activity.

yshrsmz commented 6 years ago

Controller isn't a LifecycleOwner and you can't use the underlying activity.

https://github.com/bluelinelabs/Conductor/tree/develop/conductor-modules/arch-components-lifecycle

We have lifecycle compatible module, and this LifecycleController is LifecycleOwner.

sewerk commented 6 years ago

LifecycleController is not compatible with LiveData from ACC. Because Controller survives configuration changes its being registered (in onCreateView()) as observer each time configuration changes, and its being unregistered only when Controller finishes. Registering as observer cant be done in Controller constructor when activity is null

yshrsmz commented 6 years ago

@sewerk

Controller survives configuration changes

I believe this does not mean Controller does not re-create view after configuration changes(Controller survives configuration change, but it destroys its view everytime), and thus LiveData is usable(if not, then what is the point of Lifecycle Controller? 🤔 ).

I've created a quick demo to check LiveData compatibility

I added logs to check if LiveData is active or inactive, and I added Observer in onCreateView. Each time I rotate a device, both onInactive and onActive is called in order. So LiveData's observers will not receive data while configuration change.

here is the actual Lifecycle implementation of LifecycleController.

you can see Lifecycle's State.ON_PAUSE is dispatched on preDetach callback and State.ON_STOP on preDestroyView.

sewerk commented 6 years ago

@yshrsmz active state is being handled correctly, that's not what I meant. My concern goes to observer being register multiple times which is something fragment or activity wont do. Add

        if (liveData.hasObservers()) {
            Log.w(TAG, "Multiple observers registered");
        }
        liveData.observe(this, new Observer<String>() { ....

to your code to see how Controller - not sending ON_DESTROY when config changes - is using LiveData API in incorrect way. I'm not sure how big of an issue is this with Conductor Controllers but multiple registering the same observer is using API in undesired way which might cause issues, now or in future.

miquelbeltran commented 6 years ago

yes, @sewerk is right, I didn't think about that case. I think this other PR can solve the issue: https://github.com/bluelinelabs/Conductor/pull/383 but we would need to have ON_DESTROY in preContextUnavailable to unsubscribe the LiveData in config changes.

yshrsmz commented 6 years ago

Oh, I see the problem. But if we dispatch ON_DESTROY in preContextUnavailable, then AAC's lifecycle doesn't much Controller's lifecycle, right?(though I'm not sure how we should handle this)

btw good talk @miquelbeltran at DroidKaigi, it was inspiring to me ;)

miquelbeltran commented 6 years ago

Thanks a lot @yshrsmz !

I was researching a bit more and Fragments suffer the same problem: https://medium.com/@BladeCoder/architecture-components-pitfalls-part-1-9300dd969808 and the proposed solution by BladeCoder is the same: have ON_DESTROY in the onDestroyView (or in our case on the preContextUnavailable to match the ON_CREATE in postContextAvailable).

Alternatively the user of the library must have to manually unsubscribe in onDestroyView with removeObserver but I feel developers will always forget to do so.

sewerk commented 6 years ago

I add more code for testing and registering observer multiple time is actually an issue - event are received multiple times. Whole point of having lifecycle in Controller to pass it to observe() is to have auto-dispose and not do it manually. Also you might consider sending ON_STOP in onSaveInstanceState() - details here

mradzinski commented 6 years ago

I do believe AAC's are more of a hype than an actual necessity. Trend aside, let's be objective: Controllers do handle config changes quite well (which is mostly the sale point of AAC), you can have your reactive layer based on a VERY robust solution such as RxJava2 and follow the repository pattern using something like clean architecture + DI. On top of that you can always use a classic MVVM or even MVP to abstract the BL from views (controllers). Just because Google released them it doesn't mean we need to used them (should I remind everyone what happened to the Fragments hype and the reason why Conductor exists in first place?)

I really don't see the advantages of having Conductor support AAC's. If we're trying to set a standard for Android development then we shouldn't be using Conductor in first place but Fragments. LiveData as I said can be perfectly replaced by RxJava2 (even Room supports RxJava2!), and the rest is very basic architecture and patterns. All this being said, if someone can objectively outline the true advantages of having AAC's supported I'm all up for implementing it (I'm OOO right now so I have some spare time to invest in this).

miquelbeltran commented 6 years ago

I use ViewModel + LiveData with Conductor on my projects and at work, and even when having RxJava in it I still see advantages. Note that I still use RxJava in my business logic/model, but I expose all UI changes only via a single LiveData that contains the screen state. The solution I use is available here: https://github.com/miquelbeltran/conductor-viewmodel for everyone to use.

However, you are free to not to use it, this is an independent module from Conductor, but I will tell why I like this solution: (I will refer to a MVVM ViewModel with Rx as RxVM)

For me the overall reason is a clear separation of concerns. The ViewModel and the LiveData expose screen state to the View in an easy way, and this is independent of your app architecture.

Again, you are free to not to use AAC, just like I don't like data binding and I don't use it. But if you want to, what better than providing easy access to it here.

Zhuinden commented 6 years ago

@sewerk stop hacking the system, Controller's onDestroy matches the destroy that lifecycle owner should provide.

The fact that you need to unregister observers in onDestroyView yourself is the same thing that happens with Fragment's onDestroyView when Fragment view hierarchy is destroyed but view state is preserved (DETACH state).

Thankfully, the post above mentions https://github.com/miquelbeltran/conductor-viewmodel which is a proper plug-in module for supporting ViewModel in a Controller, although VM was originally created because Activities/Fragments don't survive config change by default.

What you gain from using such solution is that LiveData can properly handle onPause/onResume (and I guess automatic unregister when a controller is destroyed, although you need to stop observing manually yourself in onDestroyView anyways)

miquelbeltran commented 6 years ago

The solution I posted on my little library does unregister your LiveData observers on onDestroyView, no need to do it manually. And more importantly it clears the viewmodel store on onDestroy, so your viewmodel will get onCleared called.

Zhuinden commented 6 years ago

Ah yes, that is the correct behavior.

tomblenz commented 6 years ago

@mradzinski I've been following and tinkering with AAC for a while now, and I believe it offers a lot for android devs of any skill level. I'll go through a few.

MVVM with AAC works particularly well with Android. Whereas Fragments convoluted and complicated Android development, AAC makes development more straightforward - even to the point where managing Fragment state is no longer as fraught as it once was. And with platform fragments being deprecated, I forsee support fragments attaining the freedom to improve in the areas that they are still dodgy in, eg. animations.

Conductor has a place in the world of AAC and should absolutely keep up, especially if it wants to remain relevant.

mradzinski commented 6 years ago

@miquelbeltran @tomblenz I agree with some of your points, but some others I disagree.

As for the rest I agree. @miquelbeltran linked his implementation of AAC's with Conductor, and at first glance it looks very promising. So I do recommend trying it out and see how it works for your apps. Not every app has the same requirements obviously, so where AAC's can be the perfect solutions for some apps, for others it can be very limited and weak (and this is why I mentioned it being a trend rather than an objective decision... new kid in the block, everyone wants to play with it).

tomblenz commented 6 years ago

I don't think either of us is saying not to use rxjava - you should absolutely use rxjava to communicate between the model and vm. Livedata should be used to communicate between the vm and view, which is where it's strengths shine and it's drawbacks are irrelevant (view state has already been computed so livedata should just be communicating end state).

You can absolutely use rxjava in place of livedata, but you gain nothing (the power is not leveraged) and lose a lot (have to manually manage view and lifecycle state).

tomblenz commented 6 years ago

Excuse mobile typos 🙃

tomblenz commented 6 years ago

@EricKuck Not sure how out of the loop you are, but I think Miquel's PR (https://github.com/bluelinelabs/Conductor/pull/405) is a decent solution for AAC compatibility. If merged, we could finish this thread up.

yshrsmz commented 6 years ago

According to the talk from Google I/O 2018, they will add Fragment#getViewLifecycleOwner() which is tied to Fragment's view lifecycle. So maybe we should also add Controller#getViewLifecycle() to better handle this lifecycle mismatch?

Zhuinden commented 6 years ago

Makes sense

BelooS commented 4 years ago

Any news?

TemMax commented 3 years ago

@EricKuck any news about this?

TemMax commented 3 years ago

I've created a new library that allows use ViewModel like in Fragments. Check it: https://github.com/TemMax/conductor-viewmodel

TemMax commented 3 years ago

@yshrsmz I'he realized view lifecycle in my library. Link in message above in this thread. Check it out. All suggestions are welcome ;)

solidogen commented 3 years ago

https://github.com/Solidogen/conductor-mvvm

A few weeks ago I made a quite verbose sample how to use MVVM with Conductor, feel free to check