sockeqwe / mosby

A Model-View-Presenter / Model-View-Intent library for modern Android apps
http://hannesdorfmann.com/mosby/
Apache License 2.0
5.49k stars 841 forks source link

[Discussion] Dependency injection tactics and reusability tips - seeking advice/discussion applicable for scaling production MVI apps #276

Open jhowens89 opened 7 years ago

jhowens89 commented 7 years ago

If @sockeqwe believes this is an inappropriate spot for this topic, I will move it but I don't really know of any other places to have a prolonged dialog centering around MVI on Android.

First let me thank Hannes for the library and being incredibly helpful as I stumbled through my first couple months of understanding MVI. I've boldly (or questionably) adopted this new architecture on a larger project that's seen about five years of band-aids and contractors. It's been a huge undertaking for me as I was never actually allotted time for this complete overhaul, but overall I'm very glad I made the decision.

Since MVI has been sort of a newer development for Android, there's not a whole lot of literature that explains past todos and shopping carts. On this post, I'm hoping to find others like me and engage on the topics of dependency injection tactics as it pertains to MVI and reusability/abstraction of MVI units.

To give this discussion a starting point and better illustrate my perspective, I'm going to show 2 screenshots of a screen in my app. The first is normal and the second is annotated with what I would call my "MVI units". I've given them names for easier discussion. I would like to summarize what I've done so far and my pain points and questions going forward.

Normal Annotated
regular_leaderboard annotated_leaderboard

Every box here represents a different MVI unit. The difference in color is only to contrast them from their neighbor. Brief description:

RefreshMenuItem - An action layout in the form of a FrameLayout for the toolbar Banner - MviFragment that also acts as the container for the rest of the screen. WeatherDrawer - Expandable drawer Selector - Two tab layouts encapsulated as a custom ViewGroup. The top changes the contents of the bottom. The bottom controls the ViewPager for the FragmentStatePagerAdapter ScoreHeader - MviLinearLayout inside a child fragment of the FragmentStatePagerAdapter MatchAdapter - MviFragment with a RecyclerView. Is the child fragment of the FragmentStatePagerAdapter. SponsorFooter - MviLinearLayout

So there's a bit of everything going on here. My MVI units mirrors the setup of Hannes almost exactly. Interactor, Presenter, ViewState (Kotlin file that also contains ViewStateAction and reducer function), View, and insert android ui container here. My MVI units have no parent-child relationships with each other.

Dependency Injection Tactics

All of my MVI units being separate is a godsend. It drastically reduces complexity of state management. I have rule of thumb that whenever state management starts giving me a headache, it's time to break up the MVI unit. It really amazes me how well this line of thinking has served me but it also makes sense when you consider that the addition of extra state variables is met with exponential growth in state possibilities. The MVI units being separate also makes it extremely easy to take a unit and plop it into another screen.

But it also leads to one problem, what about if my units share a tiny bit of their state? What if this state has no business living in a database. With my example, MatchAdapter needs to know which day and which play format it should be using as parameters for its query.

My solution has been to use scoped Dagger components. Dagger was already supplying me with my presenters, interactors, and datasources. So in this instance, I expose the two sides of a Relay in my dagger component.

    /*
     * Shared business logic
     */
    fun sessionsForSelectedDayObservable(): Observable<TournamentDayModel.Description>
    fun sessionsForSelectedDayConsumer(): Consumer<TournamentDayModel.Description>

Because dagger is building my interactors, I have my observable and consumer handily available in my interactor. I'm happy with this solution, but it does create an eyesore. I have to have my component inject all my MVI units and because of the nested nature, they're in very inconvenient places.

In my main container fragment:

override fun onAttach(context: Context?) {
        super.onAttach(context)
        componentHolder = ViewModelProviders.of(this).get(DualTeamScoringComponentViewModel::class.java)
        componentHolder.initialize(lifecycle, this)
    }

    override fun onCreateView(inflater: LayoutInflater?, container: ViewGroup?, savedInstanceState: Bundle?): View? {
        val view = inflater?.inflate(R.layout.dual_team_scoring_fragment, container, false)
        componentHolder.featureComponent.inject(view?.sessionSelectorStackedTabLayout as SessionSelectorTabStackLayout)
        componentHolder.featureComponent.inject(view.weatherAndBroadcastLayout as WeatherAndBroadcastLayout)
        componentHolder.featureComponent.inject(view.findViewById<PCupTournamentHeaderLayout>(R.id.pCupTournamentHeader)as PCupTournamentHeaderLayout)
        componentHolder.featureComponent.inject(view.findViewById<PCupLiveBannerLayout>(R.id.liveBannerPcup) as PCupLiveBannerLayout)
        return view
    }

    override fun onAttachFragment(childFragment: Fragment?) {
        super.onAttachFragment(childFragment)
        if (childFragment is MatchSummariesFragment) {
            componentHolder.featureComponent.inject(childFragment)
        }
    } 

override fun onCreateOptionsMenu(menu: Menu?, inflater: MenuInflater?) {
        inflater?.inflate(R.menu.mvi_refresh, menu)
        menu?.let {
            val refreshMenuItem = it.findItem(R.id.menu_mvi_refresh)
            val actionLayout = refreshMenuItem.actionView as RefreshActionLayout
            componentHolder.featureComponent.inject(actionLayout)
        }
    }

What's worse is that ScoreHeader is inside a child fragment inside the view pager inside my normal fragment. So I end up having to inject the child fragment with ScoreHeader's presenter, passing it to ScoreHeader and nullify the reference. I'm proud of how clean the rest of my architecture is, but I really want to find a better solution for managing my feature components.

From what I've read, I'm on kinda on the right approach in that I inject my views/fragments and my views/fragments don't inject themselves. I've heard the term "coordination layer" tossed around and the idea has stuck with me. One idea I've been kinda kicking around in my head is taking a page from the Coordinators library and utilizing ViewGroup.OnHierarchyChangeListener in combination with type and/or tag checks to inject my necessary views. I could separate the container fragment as a rightful "coordination layer" and Banner render code would not have to coexist with the component. The view pager might present a roadblock (not sure how view hierarchy would be represented there) but there could be a path forward when used in conjunction with onAttachFragment to deal with child fragments.

Does anyone have thoughts or advice they could pass along for dealing with this type of situation?

Reusability/Abstraction

So I have been through a major release and written a lot of new code from scratch. It's been so delightful to just plop my MVI units into xml, add a one line dagger inject, and have my unit just work. But I'm looking to bring this magic to the older parts of the app. A lot of these pieces are really similar, but not completely the same. For example, WeatherDrawer has the same contents, but looks completely different. MatchAdapter displays the same views, but queries rounds of a tournament instead of the day/format.

Does anyone have general advice on how and where abstractions should ideally take place? Here are a couple questions that come up when I ponder this:

sockeqwe commented 7 years ago

Hey there, I'm sorry for the late response but I have been busy last week. I'm happy that you have started this discussion and for now I can't think of any better place then the issue tracker, so that's completely fine.

I really like the idea of "MVI units". We have a similar setup at work and it worked out well so far for us.

Regarding Dependency injection tactics: Unfortunately Android is not build in a dependency injection friendly way. You can't pass dependencies as constructor parameter to Activities or Fragments or ViewGroups if inflated from xml. However, I played around a little bit with the idea of instantiating ViewGroups manually (instead of xml layout) so that I can pass in dependencies as constructor parameter. Unfortunately, I haven't had too much time to create bigger and more complex examples, but so far I can tell that an advantage of that approach is that dependency injection is easier, espacially scoping because all "MVI units" that share the same scope are basically living in the same "scope container" like a Fragment (or Activity). So Fragment is just some kind of "Container" and I instantiate all MVI Units, which are just MVI ViewGroups, manually instead of xml inflation and pass the dependecies to those MVI Units manually as constructor parameter. Of course this also has some disadvantages: Styling of this MVI units also has to be done mostly in code which compared to xml is a little bit cumbersome (but that is basically how iOS guys are doing it too). Regarding Coordinators: This "pattern" is widely used and accepted on iOS development but only works well there because iOS developers have full control over instantiating ViewControllers. Ray Ryan gave a talk Reactive Workflows which seems to go into a similar direction, but I haven't had time yet to check that out in detail.

Regarding abstractions: The question I ask myself is: what is the thing that takes long and slows me down? Usually, for me, the answer is writing UI. I mean, creating a Presenter, Interactor or business logic is quite simple and fast to do (i.e. define a retrofit interface). So i defenitely think duplicating code (for example for interactor or presenter) is better than searching for a proper abstraction for them to reuse it. Those are cheap to duplicate. For UI i try to keep it simple and stylable through xml. If I still need a similar UI widget with different behavior, I think duplicating is a good idea. Also delegating stuff worked quite good for me in UI layer rather than abstraction or inheritances. i.e. Widget A and widget B does some similar parts, but others are different. Then putting out the similar part and put it into an own class and delegate from both widgets.

I'm sorry, but I don't have concrete answers for your questions 😄

jhowens89 commented 7 years ago

I thought about going more programmatically or finding a place to sneak injects in early in the inflation process, but breaking the xml part of our toolset is a dealbreaker and even if I was fine with it I wouldn't want to put my coworkers through that.

It's funny you mention that talk. I was discussing a link to the slides with another guy on reddit and it's what motivated me to write out this post. I really hope they post the video soon. I've tried to read through the slides, but I don't learn very well from bullet points lol

So this weekend, I've finally decided to get my feet wet with the Coordinators and I'm actually really encouraged by these early results. I actually forked the Coordinators library and added a Coordinators::installRecursiveBinder where I took the binder and wrapped it in this old gist from JW: https://gist.github.com/JakeWharton/7189309

It works like a charm and picks up everything even with all the view pagers and nested goodness. So I got the "coordination layer" I wanted because there wasn't a need to worry about porting my component around.

I decided to stop using the MVI delegates for now as they were causing me issues related to the timing of the lifecycle and delegate's createPresenter. So at this point I have no presenter code in my layouts, but I obviously need to hook up my business logic and view states somehow. But now thanks to view my tree traversal which has me traversing MVI layouts looking like this:

class DualTeamBannerLayout : RelativeLayout(), DualTeamBannerView

and the fact all my mosby presenters look like:

class DualTeamBannerPresenter: MviBasePresenter<DualTeamBannerView, DualTeamBannerViewState>()

It makes it really convenient to figure out what presenters should be going where. In fact, I've started pulling the implementation out and sticking it in a dagger module:

@Binds
    @IntoMap
    @ClassKey(DualTeamBannerLayout::class)
    fun coordinatorScoring(coordinate: PresenterProvidingCoordinator<DualTeamBannerPresenter, DualTeamBannerView>): Coordinator

I can be creative with my keys (the Coordinators examples even use xml) and I'm still trying to figure out the best system. I think having this layer entirely separate will give me a lot more options as far as swapping things in and out because my navigation/UI layer and business logic/plumbing layer now have a greater separation of concerns. There are still a lot of issues I need to keep thinking through since this is a pretty big change, but I feel excited about the direction.

devAseemSharma commented 7 years ago

@jhowens89 I was working on refactoring my existing code, after going through your discussion thread I was having some confusion while implementing things as per the guide, basically I need to know how exactly we can use Dagger2 in this MVI architecture? I hope you must have gone through same situation at the start, it would be really helpful if you could provide some reference regarding this. Thanks

sevar83 commented 7 years ago

Hey guys, @jhowens89 I've just seen your invite to come over here. Yes, nesting is one of the hardest problems I've faced. I've got 2 screens with nesting. They are decently stable but not as clean as I wish:

ViewPager controlled by BottomNav not tabs: https://lh3.googleusercontent.com/mEXnPiy31qt-kQdLR-djW4MRIHpP_kc4JYqoDz2jmBiRhpXHjoVRYMyleXGg5hiSVA=h900-rw

ViewPager with tabs: https://lh3.googleusercontent.com/MeY4tPIIEoBQJs8HWIvJ2JCirZy8v9su9XFGErnm6_tp9d-LjsPvVdvay00CLWfohHQ=h900-rw

All pages of the ViewPagers on both screens are separate MVI controllers (Conductor's equivalent of Fragment). Their parent containers are also MVI Controllers. Parent presenters know nothing about children and vice versa. States and Interactors also have no coupling between. Coupling is only between the parent and children Controller (View) side but as weak as possible. The important thing is that the parent state does not contain/include/is composed of the children states. For example in 2nd screen the whole state of the Search parent controller is just 2 fields - searchText: String and activeTab: Int. This was just my way to do it, there are other ways which look more perspective which I don't have time to explore.

Some ideas

Making the parent state include also the children state, e.g. a SearchState including both pages for movies and TV. Then add a constructor parameter for the child controller - let's call it "nested mode" flag. Nested mode should prohibit the controller from acting standalone but only as a child of another controller. This means in "nested mode" child controller should not create its own presenter, the parent presenter will bind to its intents and the parent will call to child's render(). Basically this makes it a passive stateless Android widget with the handy render() method. It's parent who is responsible for everything. And when child is not in nested mode it's a plain normal MVI unit - with own presenter. In this way the controllers could be reused in various setups - imagine some complicated multi-view tablet layouts. Maybe it's possible with Mosby but I didn't tried this yet.

Root container

The idea is that no matter what screen it is - phone or tablet, there always should be a container not necessarily an MVI - responsible for correct placement of the child MVI units in every screen size and rotation. This should not be a responsibility of the MVI units itself. They must render their own data but "someone from above" should know how they are actually arranged or if they are visible at all. E.g. I have a very simple toast-like balloon at the bottom showing "offline" text when no connection. It's a separate MVI unit which I place it in all screens. The problem in some screens I put it above the BottomNav and in landscape the BottomNav may be missing. In other screens there is no BottomNav.

I just released my app these days: https://play.google.com/store/apps/details?id=com.mediaclient.movielovers.mobile

Any feedback and 5-stars won't be refused :)

sockeqwe commented 7 years ago

Thanks for sharing your opinion. I'm a little bit busy right now, I will take a look at this later this week.

jhowens89 commented 7 years ago

I've been really busy like @sockeqwe, but I've not forgotten about this thread. I hope to create a detailed post this weekend. I've been putting in a lot of work toward my Coordinator approach and I'm hoping for it to be in state to merge into the rest of my project soon. The one thing I'm stumbling over is ridiculous Ad loading/refresh business logic imposed by the client. But on the positive side, it's making me fully flush out problems I might later encounter.

@sevar83, I wasn't aware you and Bacillus were the same person! Always kinda funny when that realization happens. I've spoke to you as /u/reconcilable on reddit too. You actually sparked my current coordination layer approach when I was digging around on the Dagger issue tracker and read your comment here: https://github.com/google/dagger/issues/720

Our approaches seem quite similar in many ways with most of the differences stemming from the fact that you're using conductor and I'm using fragments. I use fragments because the new architecture components give me a lot of lifecycle tools. I use ViewModels to hold my dagger components and it gives me an easy way to determine to determine whether my fragment or activity is getting destroyed permanently or is it just a configuration change. My viewmodels also inherit from LifecycleObserver and I hope to use that to leverage some kinda Relay setup to coordinate items (such as my ads) that would normally rely on lifecycle logic not available to a viewgroup. The ViewModels of the children contain subcomponents and being able to tie that to an independent lifecycle makes scoping pretty easy.

The main differences that stick out to me is that your equivalent of my fragments, your controllers seem to do a lot more. My controllers don't have presenters and calling my child controllers' render method wouldn't be applicable because my controllers don't have view code in them. My controllers currently have the following responsibilities:

My focus is keeping my MVI units free from "coordination" code. I'm fine with my controllers not being reusable if I accomplish this goal because they're fairly free from any complicated logic that can't be copy/pasted and then tweaked a bit.

So that being said, I'm not sure I'm an immediate fan of the parent calling the child's render, but I would need to know more about what your controllers do (what's in that render method) and what is the problem you're trying to solve with the parent-child controller relationship?

sockeqwe commented 7 years ago

In an early prototype of Mosby 3.0 there was no createPresenter() in MviActivity or MviFragment (haven't started with MviViewGroup implementation), so the View layer had no knowledge of the presenter. Rather there was some kind of "coordinator / factory" that was listening for lifecycle events was responsible to do createPresenter() there and attach the view to the presenter. The advantage is that you don't have to inject a presenter into your view. This design allows to reuse the View as the view is basically just an interface with intents and a render method (and a ViewState). I presented this solution to some developers, however, they found it rather inconvenient to create a factory / coordinator for each View. So at the end this idea has been discarded, although I have some projects where I still use this approach (closed source, sorry). I also had the feeling that reusability and building independent components was not the highest priority for them.

Regarding reusability in general my approach is to figure out what is actually expensive to build and time-consuming so that you would like to share / reuse it. I come to the conclusion that it is "business logic stuff" and UI. Business logic is easy to share and reuse as you have full controll over that. In contrast to business logic, for UI / View Layer, you are not in full control of instantiation, lifecycle and other android specific things. So I really just want to reuse the UI Component (view layer) where I define a interface with intents this UI component can trigger and a ViewState that this UI component expects to get to render. So I basically put this 3 classes / interfaces (ViewState, View Interface and concrete View implementation) into one library and reuse it. So usually I don't pack the presenter into this "component bundle", only the View layer as from my point of view the presenter is easy to write your own and also depends a little bit on the business logic you want to interact with (and as described before, business logic is easy to reuse).

I'm considering going back to that early prototype approach and introducing "coordinators / factories" in Mosby 3.2 or 4.x0 to fully get rid of the dependency to the Presenter in View layer. This should make it more dependency injection friendly as no dependency must be injected in View at all 😄. Unfortunately, i'm not sure how this approach solves nesting MVI components, but perhaps this is then just a matter of using views in xml layout hierarchies and put the (shared) state computation together somewhere else ...