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

Formalize notion of child presenter #52

Closed ghost closed 10 years ago

ghost commented 10 years ago

PopupPresenters and other child presenters are kind of a pain to do right. You have to remember to call their takeView method from onLoad, and override your dropview method to call theirs.

Could we provide something like:

interface FindChildView<C, V> {
  C getChildView(V parentView);
}

<C> void loadChildPresenter(Presenter<C> child, FindChildView<C, V> findView) { ... }

Where V is the parent view's view type. loadChildPresenter would be called at the end of the parent's onLoad method, where we now call the child's takeView; and the call to the child's dropView would be automatic.

e.g.:

public void onLoad(Bundle b) {
  super.onLoad(b);
  loadChildPresenter(fooPopupPresenter, new FindChildView<FooPopup, MyView>() {
      FooPopup getChildView(MyView parent) { return parent.getFooPopup(); }
    });
}
ghost commented 10 years ago

Another thought is to make the new method addChildPresenter, to be called from the constructor or wherever. No load side effect in the method.

ghost commented 10 years ago

@loganj suggests that instead we should make dropView final, and encourage a pattern where Views are in charge of the wiring in hierarchical situations. That is, instead of

protected void onLoad(Bundle b) {
  super.onLoad(b);
  childPresenter.takeView(getView().getChildView());
}

protected void dropView(View view) {
  childPresenter.dropView(view.getChildView());
  super.dropView(view);
}

We do something like:


@Inject MyPresenter presenter;
final ChildView childView;

MyView(Context context, AttributeSet attrs) {
  super(context, attrs);
  Mortar.inject(context, this);
}

protected void onFinishInflate() {
  super.onFinishInflate();
  childView = new ChildView(getContext(), presenter.getChildPresenter());

  presenter.takeView(this);
}

protected void onDetachedFromWindow() {
  presenter.dropView(this);
  super.onDetachedFromWindow();
}

And we presume ChildView will take care of its own calls to takeView and dropView. The upside being a) dropView is final, less public api, yay! And b) it is always the case that views are in charge of takeView and dropView. Much more consistent, hopefully less confusing.

I think the thing to do is move the sample app to this style and see how it feels.

rjrjr commented 10 years ago

I took a stab at this (making DropView final) and was thwarted by PopupPresenter. Will try again if we manage to kill that thing off

rjrjr commented 10 years ago

Nope, don't make it more complicated. Make it simpler.

Lesson 1: always call dropView and takeView from the View side, even if you're nesting.

  @Override protected void onFinishInflate() {
    super.onFinishInflate();
    presenter.takeView(this);
    presenter.getSubPresenter().takeView(subView);
  }

Lesson 2: to hell with Popup, see https://github.com/square/mortar/issues/102