square / mortar

A simple library that makes it easy to pair thin views with dedicated controllers, isolated from most of the vagaries of the Activity life cycle.
Apache License 2.0
2.16k stars 155 forks source link

Give presenters a safer hook for view detaching #156

Open rjrjr opened 9 years ago

rjrjr commented 9 years ago

Drop getView(), replace onLoad(Bundle) with onTakeView(View,Bundle) and onDropView(), make dropView() final.

Lifecycle becomes:

enterScope()
  onTakeView(View, Bundle)
  onDropView()
exitScope()

With onSave(Bundle) being called any arbitrary number of times between enter and exit.

Also worth thinking about decoupling Presenter and Bundle entirely.

loganj commented 9 years ago

I feel pretty good about decoupling Presenter and Bundler. That should be a pretty mechanical change to delegation, and would significantly simplify Presenter.

Less sure about de-scoping Bundler.

rjrjr commented 9 years ago

Paves the way to closing issue #103

pyricau commented 9 years ago

Just to clarify: does that mean that we'll need to reimplement the current Presenter view handling code in a subclass?

rjrjr commented 9 years ago

No. It means the contract would be that you can hold on to the View passed into onTakeView() yourself, and you should drop it when onDropView() is called. But the current debouncing behavior would still be in place.

We just no longer have a getView() method that's hard to document. Instead there's "here's your view," and "you should forget about that view now".

On Mon, May 4, 2015 at 4:44 PM Pierre-Yves Ricau notifications@github.com wrote:

Just to clarify: does that mean that we'll need to reimplement the current Presenter view handling code in a subclass?

— Reply to this email directly or view it on GitHub https://github.com/square/mortar/issues/156#issuecomment-98885729.

pyricau commented 9 years ago

Ok. So technically the change means:

loganj commented 9 years ago

My bet:

  1. Most presenters don't even need to keep a field reference to the view.
  2. This change makes presenters a lot easier to understand because the template methods are actually symmetric and there's no superclass magically managing a field that you have to assume might be null at any moment.

If my bet is wrong for your particular app, there's nothing stopping you from introducing a Presenter subclass to use as a base class for your own presenters.

rjrjr commented 9 years ago

The change to make the API symmetric is separate from the decision to hide getView(), so your second point doesn't bolster your argument. That leaves us with typical use, and I think the jury is still out on that.

I do like the fact that the new API is more minimal, less side-effecty.

loganj commented 9 years ago

I don't think it's a completely separate decision. Making the API symmetric makes getView/hasView superfluous.

rjrjr commented 9 years ago

Talked about this some more. Also starting to seem like enterScope / exitScope should not be built into presenter. Orthogonal concerns.

urandom commented 9 years ago

Would it be possible to somehow obtain the bundle during onDropView(), so that extra stuff about the view can be saved and later easily obtained back during onTakeView()?

valeriyo commented 8 years ago

Good stuff. I assume onTakeView / onDropView could be repeated N times (e.g. on rotation) ?

Btw, whenever view invokes presenter's callbacks, it could just pass this as parameter - no need for getView() in that case.

rjrjr commented 8 years ago

Yes, exactly.