sockeqwe / mosby

A Model-View-Presenter / Model-View-Intent library for modern Android apps
http://hannesdorfmann.com/mosby/
Apache License 2.0
5.49k stars 841 forks source link

MVI - support views not attached to activities (but to the WindowManager instead) #305

Closed MFlisar closed 6 years ago

MFlisar commented 6 years ago

If I want to use a MviRelativeLayout and attach it to the WindowManager this won't work because the ViewGroupMviDelegate is looking for an activity and will never find one.

Would it be possible to support a view that will be attached to the WindowManager as well?

sockeqwe commented 6 years ago

I don't think so, but needs some more investigation ... Activity is needed to understand the lifecycle the View logically belongs too. As far as I know WindowManager doesn't have lifecycle alike callbacks or other ways to figure out whether or not a view is destroyed permanently or just destroyed temporarily because of orientation changes (that*s why ViewGroupMviDelegate needs an Activity. If you don't need that the presenter is retained during screen orientation changes, than you can use ViewGroupMviDelegateImpl(keepPresenterDuringScreenOrientationChange = false) (so that activity is never used and therefore can be used with WindowManager)

MFlisar commented 6 years ago

using keepPresenterDuringScreenOrientationChange = false and retaining my presenter manually should work as well, shouldn't it? In my service I can decide when to reset the presenter and when not...

MFlisar commented 6 years ago

Sadly this won't work, as I said in my first post, the delegate will search for the parent activity here:

https://github.com/sockeqwe/mosby/blob/master/mvi/src/main/java/com/hannesdorfmann/mosby3/ViewGroupMviDelegateImpl.java#L71

Even If I set keepPresenterDuringScreenOrientationChange = false...

And here again, it would crash I think because there is no activity https://github.com/sockeqwe/mosby/blob/master/mvi/src/main/java/com/hannesdorfmann/mosby3/ViewGroupMviDelegateImpl.java#L121

MFlisar commented 6 years ago

What do you think of an extension of the ViewGroupMviDelegateImpl like following:

// add a field like followng => set it true to don't change any old behaviour
boolean forceActivityContext = true;

// 
void setAllowNonActivityContext() {
    forceActivityContext = false;
}

If the field is set to true, everything stays as it is, otherwise I will change the code to not throw an exception. Result is, that the user will have to manually call the code in here https://github.com/sockeqwe/mosby/blob/master/mvi/src/main/java/com/hannesdorfmann/mosby3/ViewGroupMviDelegateImpl.java#L239 to clean up everything because there will not be any destroy callback.

Anything that speaks against this approach or any better idea?

sockeqwe commented 6 years ago

Ah, sorry, I have overlooked this.

A better idea would to write your own ViewGroupMviDelegate. All the ViewGroupMviDelegate does is once View is attached to window or detached from Window call createPresenter() and presenter.attachView(), presenter.detachView() and presenter.destroy(). Should not be too hard to implement such a custom delegate.

class MyMviRelativeLayout<V, P> extends MviRelativeLayout<V, P>{

   private ViewGroupMviDelegate<V, P> delegate;
   ... 

   @Override 
   protected ViewGroupMviDelegate<V, P> getMviDelegate() {
      if (delegate == null) {
        delegate = new MyCustomViewGroupMviDelegateImpl<>();
      }

      return delegate;
    }
}

While I see how you can keep a presenter in memory manually during screen orientation changes (so presenter.attachView() and presenter.detachView() should be easy to implement in your custom delegate, I'm not sure how you would do the "clean up" a.k.a calling presenter.destroy().

MFlisar commented 6 years ago

Actually I totally copied the ViewGroupMviDelegateImpl, replaced the internal activity variable with a context variable and disabled the retaining of the presenter in it (did not remove the overhead).

I would say it should be ok, to create the views in my service and attach them. If my service get's destroyed, I detach my views AND destroy them, if the screen is rotated, the views are only detached and reattached.

Result looks like following: https://gist.github.com/MFlisar/9d0be636fd5868338d300d400a3367a0

This is what I call if my service stops after I detached the view: https://gist.github.com/MFlisar/9d0be636fd5868338d300d400a3367a0#file-myviewgroupmvidelegateimpl-java-L241

I think this should work

Btw, it may at least make sense to support a service context in the PresenterManager wouldn't it? This way I could enable keepPresenterDuringScreenOrientationChange in my MyViewGroupMviDelegateImpl without the need of copying this class as well

sockeqwe commented 6 years ago

Could be cleaned up a little bit more, but overall it sholuld work.