Closed sockeqwe closed 6 years ago
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.
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.
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.
See #274
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).
Until now
Presenter
basically just have to "lifecycle callbacks":Presenter.attachViiew(V view)
andPresenter.detachView(boolean retainInstance)
.Activity and Fragment (since #257 ) use the lifecycle pair
onStart()
-onStop()
to callpresenter.attachView()
andpresenter.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 inonStop()
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 ActivityB
withFLAG_ACTIVITY_CLEAR_TOP
fromE
. Then Activity C and D have reachedonStop()
before and calledpresenter.detachView( retainInstance = true )
but afterwards C.onDestoy() and D.onDestroy() is called but we don't callpresenter.detachView(retainInstance = false)
from there.So we could change the lifecylce from pair
onStart()
-onStop()
toonCreate()
-onDestroy()
but this has other complications like for MVI intents might be triggered in Activitysuper.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:presenter.detachView()
inonStop()
and maybe inonDestroy()
again. We guarantee thatpresenter.detachView(retainInstance = false )
is called only once. Example from the previous withFLAG_ACTIVITY_CLEAR_TOP
. Then Activity C.onStop() callspresenter.detachView(retainInstance = true)
and some time laterpresenter.detachView(retainInstance = false)
is called too (when Activity B is started with with FLAG_ACTIVITY_CLEAR_TOP).Presenter.detachView(boolean retainInstance)
into two methodspresenter.detachView()
(without boolean parameter) and this one is called inonStop()
andpresenter.destroy()
which is called inonDestroy()
once we are sure that the Activity / Fragment is destroyed permanently. Sopresenter.destroy()
is exactly the same aspresenter.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()
andpresenter.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?