Add an onCreate() method

[EricKuck]

Currently developers are expected to do controller initialization in the constructor, which is great for some stuff, but makes other things a lot harder. Controllers don't have a reference to a router, activity, or parent controllers in the constructor, so things like dagger injection become more tricky. If you require references to these things for your initialization, you have to wait for the onCreateView() callback, then make sure you only run that code block once. Awkward.

An onCreate() method would be the callback signaling that the controller is fully configured (minus the view) and has all the references it'll need.

[kboyarshinov]

Would be very useful! Now I managed to have it with internal listener on preCreateView event.

[sockeqwe]

Im not a fan of adding another lifecycle method. I do those things in onCreateView() because usually you want to have the UI ready anyway and it works good for me ... Otherwise you might also need a onDestoy() as companion to onCreate() .. I can see those things getting out of hands quickly i.e. compared with Fragments some people do initialization stuff in onCreate() others do stuff in onCreateView() others in onPostCreateView() others in onActivityCreated() ... It's a mess and imho one of the reasons why people are failing using fragments properly.

Therefore, I think adding onCreate() is more irritating than helping. I think it would be better to say once and for all that onCreateView() should be used for that ( or another already existing lifecycle method if you think another one is better suited)

Otherwise, I wouldn't call this method onCreate() but rather init() (no unInit() / onDestroy()/needed then) so that its not seen as lifecycle method but rather as a one time initialization mehod ...

But again, I think that onCreate() / init() is not needed.

Btw. as far as I controller could be UI less, per purpose?

Kirill Boyarshinov notifications@github.com schrieb am Mi., 27. Apr. 2016, 02:35:

Would be very useful! Now I managed to have it with internal listener on preCreateView event.

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/bluelinelabs/Conductor/issues/34#issuecomment-214929549

[kboyarshinov]

@sockeqwe onDestroy already exists. So onCreate will serve as companion.

Btw I see that confusion may happen with adding onCreate() and existing onRestoreInstanceState(Bundle). Now onRestoreInstanceState(Bundle) is called right after Controller creation before any other method. Original Activity.onRestoreInstanceState(Bundle) happens between onStart() and onPostCreate(), while most of developers restore state in Activity.onCreate(Bundle). So when do you see Controller.onCreate() being called? I can suggest to have onCreate() called before onRestoreInstanceState(Bundle), or they may be combined like in Activity.

[EricKuck]

@sockeqwe Currently the constructor marks the beginning of the lifecycle, and is the opposite of the existing onDestroy() method. An onCreate() method would basically replace the constructor as the lifecycle start, since some of the information you need to initialize your constructor is often not available in the constructor. While this is technically another lifecycle method, the number of lifecycle methods would remain the same conceptually, as the constructor would no longer be considered part of it.

In my apps I've been creating a makeshift onCreate() method for dagger injection using the onCreateView() method as you suggest. The downside is that onCreateView() can be called more than one time in the lifecycle, which makes it less than ideal for things like dagger injection. In every project I've done, I've ended up creating a base controller class that has something like this to get around that weirdness:

    private var created: Boolean = false

    final override fun onCreateView(inflater: LayoutInflater, container: ViewGroup): View {
        if (!created) {
            created = true
            onCreate()
        }

        return inflateView(inflater, container)
    }

    open fun onCreate() { }

[adipascu]

I am also currently using a makeshift onCreate but using onRestoreInstanceState and one of the constructors (related to #22) I propose to add an onCreate method, I am not sure what arguments it should have (if any)

[EricKuck]

If an onCreate() method was added (which I'm still thinking it should be), it would probably have no arguments. I can't think of a good reason to pass any.

[sockeqwe]

@kboyarshinov You are right, I have overlooked the fact that there is already an onDestroy() method.

Well, my concern about onCreate() is it will only be called once somewhen before (or after?) onRestoreInstanceState() right?

But the problem is that pople might run into memory leaks etc. if they do initialization stuff in onCreate(). For example: What if the an RecyclerView Adapter in instantiated in onCreate() with a LayoutInflater that uses the Activity. After screen orientation changes, as we all know, a new Activity will be instantiated, but the Adapter is still using a LayoutInflater from previous Activity (before screen orientation changes).

Imho, its safer to do initialization stuff in onViewCreated(). Same if you setup an dagger scope meant to be "controller scoped" but actually depends on an Activity (i.e. Dagger Module has a reference to an activity, passed by Dagger Module constructor) ...

However, I do understand your point of view and the need for an onCreate(), but I, personally, would prefer to do that in onViewCreated() ... I can do it anyway in onCreateView() even if you introduce onCreate() 😄

Just my 2 cents.

[EricKuck]

That's a great point that I hadn't fully thought through, @sockeqwe. If it was a one-time lifecycle event, the developer still shouldn't reference the Activity, as it could be destroyed on rotation. I guess onCreate() isn't such a good idea after all.

Instead of a one-time per controller event, there needs to be a one-time per controller/activity pair event. Maybe there should just be an onActivitySet() method that isn't really a true lifecycle event?

I still don't think onViewCreated() is the right place for this kind of initialization, as this method will be called every time the view is recreated, and not just when it's gotten an activity reference. Depending on how the controller is used, stuff like dagger injections could end up happening over and over again when they don't have to.

[hannesstruss]

@EricKuck You have been using Conductor internally for a couple of apps already, how have you dealt with things like Dagger injection so far?

It appears nobody is blocked by this currently, maybe it makes sense to wait for a while and let patterns (and their drawbacks) emerge among users, then pull the best solution into the library.

Additionally we can come up with examples and documentation to prevent new users from falling into the inject-on-every-onCreateView trap.

[adipascu]

If you create the component inside a controller and provide the ActivityModule a callback that gets the current activity (if any) instead of directly passing it the activity, you need to inject only once. Instead of keeping a reference to the activity you have a reference to a controller. My issue with dagger currently is how to handle nesting dagger scopes/components from different controllers, but that probably is offtopic here.

[leonardo2204]

@adi1133 Didn't have time to test it properly, but I'm using Mortar and I came up with something like that: https://github.com/leonardo2204/ConductorSample/blob/master/app/src/main/java/leonardo2204/com/br/flowtests/mortar/MortarChangeHandler.java

I really think that a Dagger2 example in the samples would be great.

[sockeqwe]

I still don't think onViewCreated() is the right place for this kind of initialization, as this method will be called every time the view is recreated, and not just when it's gotten an activity reference. Depending on how the controller is used, stuff like dagger injections could end up happening over and over again when they don't have to.

You are right, I overlooked that because I mainly use Mosby that ensures that the presenter is only created once in onCreateView() and I don't inject things into controller but only the Presenter is instantiated by a Dagger component with all required dependencies (doesn't have Activity / Context dependencies at all).

Instead of a one-time per controller event, there needs to be a one-time per controller/activity pair event. Maybe there should just be an onActivitySet() method that isn't really a true lifecycle event?

Sounds reasonable, but I tend to agree with @hannesstruss

It appears nobody is blocked by this currently, maybe it makes sense to wait for a while and let patterns (and their drawbacks) emerge among users, then pull the best solution into the library.

What I, personally, find very important is that there is one clear solution for that (and documented in the docs). So I'm not sure if we should wait too long to pull the best solution as @hannesstruss said, but as already said, in general @hannesstruss is right, we don't have to rush. Maybe waiting some more time isn't too bad (maybe there are more things we didn't thought yet), but I certainly would like to avoid that there are 1000 ways of instantiating and configuring things like you can do in Fragments like Fragment.onCreate(), Fragment.onCreateView(), Fragment.onPostCreateView() or Fragment.onActivityCreated()

[dimsuz]

Fragments also have onAttach() method which sounds a lot like onActivitySet() which was mentioned above.

[svenjacobs]

Two put my two cents in, I had to create makeshift onCreate() like @EricKuck mentioned above. I initialise the Dagger dependency injection in the Controller* and require access to the Activity instance for one of the Dagger modules so that I'm able to inject the Activity's Context for instance. I cannot use the constructor for Dagger initialisation since the Activity is not yet attached. I require a lifecycle method that is called after the Activity has been attached but only once per Controller instance.

But all in all this feels a bit dirty ;) I'm open for suggestions on how to do this better.

*) I utilize the MVP design pattern and when I initialise Dagger in the Controller I'm able to inject the view interface (which is the Controller) into the Presenter, which is quite nice imho. Also I believe the controller is the right place for DI setup since the Controller (and Activities) cannot be injected due to the characteristics of the Android framework and since we are restricted by their lifecycle methods.

[PaulWoitaschek]

I ran in that issue too. I have a base class that initializes a controller through an abstract fun createPresenter() :Presenter

But I can't call that from the constructor because the overriding class might be not fully initializes yet. My workaround is to create the presenter lazily once.

[nomisRev]

I'm not a big fan of this idea. It would make it unnecessary complicated.

There are a tons of other solutions out there, for example I don't inject my view into my presenter but make my presenter reactive and my views listens to streams and asks the presenter to do stuff (if necessary).

Anyhow, what do you guys do when your ControllerComponent depends on your ActivityComponent for i.e. ActionBar and a config change occurs? Activity is destroyed but Controller is retained. => ActivityComponent leaked.

[svenjacobs]

There are a tons of other solutions out there, for example I don't inject my view into my presenter but make my presenter reactive and my views listens to streams and asks the presenter to do stuff (if necessary).

@nomisRev Could you please provide an example? Does your presenter offer an Observable<Event> where the events tells the view what UI actions to perform or something like that?

Anyhow, what do you guys do when your ControllerComponent depends on your ActivityComponent for i.e. ActionBar and a config change occurs? Activity is destroyed but Controller is retained. => ActivityComponent leaked.

That's a good point. Didn't think of that.

[nomisRev]

Yeah, I currently don't have something I can share. So for example if I have a list of some type I'll expose a Observable<Something> and depending on what you want to do Something could just be List<T> T being the type of the data you're showing. Or a ViewModel which holds your data and some additional information like if the data is stale, network error occured, etc.

I do this by levering functional techniques using Kotlin, but basically on a pull 2 refresh for example. I ask my presenter to refresh and my view is already subscribed to the stream. When my view unsubscribes my presenter reactively knows no1 is listening anymore (refcount) and will/could close any hot streams he holds.

The presenters refresh method could then for example return a Completable that starts and stops the pull 2 refresh animation etc.

[jshvarts]

My workaround to inject once only is using lazy delegate:

abstract class BaseView : Controller() {

    // Inject dependencies once per life of Controller
    val inject by lazy { injectDependencies() }

    override fun onContextAvailable(context: Context) {
        super.onContextAvailable(context)
        inject
    }

    protected abstract fun injectDependencies()
}

[mhdatie]

@jshvarts How do you deal with subcomponents that need to be cleared at the end of the lifecycle?

Say we inject the way you suggested, and then push the same controller which would hold the same scope hence same (already plus'ed) subcomponent. When should someone clear that subcomponent so that the newly pushed controller use a new instance?