We may have to make breaking API changes for Presenter

[sockeqwe]

Until now Presenter basically just have to "lifecycle callbacks": Presenter.attachViiew(V view) and Presenter.detachView(boolean retainInstance).

Activity and Fragment (since #257 ) use the lifecycle pair onStart() - onStop() to call presenter.attachView() and presenter.detachView(retainInstance).

There are some problems with this while determining if a presenter should be retained (the boolean flag of presenter.detachView(retainInstance)). The problem is that in onStop() we don't know whether or not the Activity or Fragment will finish and therefore Presenter destroyed permanently (presenter.detachView(retainInstance = false)).

Example Activity with FLAG_ACTIVITY_CLEAR_TOP. Let's say we have the following Activities:
A -> B -> C -> D -> E (E foreground) and then we call start the Activity B with FLAG_ACTIVITY_CLEAR_TOP from E. Then Activity C and D have reached onStop() before and called presenter.detachView( retainInstance = true )� but afterwards C.onDestoy() and D.onDestroy() is called but we don't call presenter.detachView(retainInstance = false) from there.

So we could change the lifecylce from pair onStart() - onStop() to onCreate() - onDestroy() but this has other complications like for MVI intents might be triggered in Activity super.onCreate() but the UI hasn't been initialized yet (i.e. setContentView() is traditionally called after super.onCreate() ).

The easier path to solve this problem is to keep onStart() - onStop() pair but either:

1. Call presenter.detachView() in onStop() and maybe in onDestroy() again. We guarantee that presenter.detachView(retainInstance = false ) is called only once. Example from the previous with FLAG_ACTIVITY_CLEAR_TOP. Then Activity C.onStop() calls presenter.detachView(retainInstance = true) and some time later presenter.detachView(retainInstance = false) is called too (when Activity B is started with with FLAG_ACTIVITY_CLEAR_TOP).
2. BREAKING API CHANGE: We split Presenter.detachView(boolean retainInstance) into two methods presenter.detachView() (without boolean parameter) and this one is called in onStop() and presenter.destroy() which is called in onDestroy() once we are sure that the Activity / Fragment is destroyed permanently. So presenter.destroy() is exactly the same as presenter.detachView(retainInstance = false) right now.

Option 1. is not a breaking API change and most apps should be fine. We also never claimed that there are equaly many presenter.attachView() and presenter.detachView() calls.

Option 2. is a breaking API change and would be released as Mosby 4.0.0

I would like to hear your feedback. Any thoughts?

[dimsuz]

I have some concerns about the 1st option. Although Mosby doesn't specify that calls to attachView()/detachView() are symmetrical, I feel like it is natural of user to assume this, because this is semantically correct behavior. If a view got attached once, why would it be detached more than once? And actually would this be happening if the first scenario will be implemented? Would it be detached twice? I think no :) What will isViewAttached() return in first detachView() and in second detachView()? So this kinda breaks the natural expectations...

So I think this will make this callback potentially misused, by a misguided user. I understand that this is a workaround and they tend to be ugly.

I'm not sure exactly what better option for 1. to suggest though. But what do you think about maybe going with scenario 2, but without making it a breaking change? It seems like you could leave the current detachView(boolean) function, but mark it as @Deprecated and document so that users would use new detachView() and destroy() callbacks instead.

[sockeqwe]

Exactly my thoughts, thanks for your feedback. I would also go with Option 2 and add a @Deprecated on detachView(boolean). But yeah, detachView(boolean) will then be called (sometimes) twice: Once with true and once with false basically like this and maybe not symmetrical (with attachView(), but as you have already said, we never have claimed that attachView() and detachView() are symmetrical)

interface MvpPresenter<V extends MvpView> {
    public void attachView(V view);
    @Deprecated public void detachView(boolean retain); // old

    public void detachView(); // new
    public void destroy();  // new
}

class MvpBasePresenter<V> implements MvpPresenter<V> {

   @Deprecated
   @Override
   public void detachView(boolean retainInstance){

    }


   @Override
   public void detachView(){
          detachView(true);
   }


  @Override
  public void destroy(){
      detachView(false);
  }

}

However, the only remainig question is whether or not we can remove detachView(boolean) from MvpPresenter interface without breaking things:

interface MvpPresenter<V extends MvpView> {
    public void attachView(V view);
  
    public void detachView(); // new
    public void destroy();  // new
}

I think it is possible to do so because in concrete class MvpBasePresenter we can define this method @Deprecated void detachView(boolean) or do you see any problem with this approach?

So basically remove the deprecated method from interface but add it to each Presenter class to avoid compile errors.

[Thomas-Vos]

I think it's best to go with option 2 but without making it a breaking change. You should be able to remove the method void detachView(boolean) from the presenter interface but keep in in MvpBasePresenter. If a presenter extends MvpBasePresenter it still "overrides" the void detachView(boolean) method, so it shouldn't give a compile error. Then sometime in the future we could remove the void detachView(boolean) method when everyone has had the time to change their code.

I'm currently having some issues with the Fragment's back stack and option 2 should solve this.

[sockeqwe]

See #274

[mradzinski]

Reading this I have a quick question regarding the current's presenter lifecycle. Is there any method that currently signalize an onDestroy for the presenter or is just simply implied in onDestroyView that the presenter has been destroyed? If so maybe just stating the destruction reason in onDestroyView through some param can be enough (maybe I got this all wrong, sorry if that's the case).