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

Changelog of 3.1.0-SNAPSHOT #274

Closed sockeqwe closed 6 years ago

sockeqwe commented 6 years ago

This is just a temporary note. Feedback is very welcome.

Changes:

class MyPresenter extends MvpBasePresenter<MyView> {

       public void loadData() {
             repository.loadData(new Callback() {
                 @Override void onSuccess(Data data){
                          ifViewAttached(new ViewAction(){
                               @Override void run(MyView view){
                                    view.showData(data);
                                }
                          });
                  }
             });
       }
}

So you get the reference to your view (guaranteed to not be null) as parameter. There is also a second variant ifViewAttached(boolean exceptionIfViewNotAttached, ViewAction) that allows you to set a boolean flag whether or not an exception should be thrown if you want to interact with the view but the view is detached or destroyed. This is typically the case if you haven't canceled any background task properly.

class MyPresenter extends MvpQueuingBasePresenter <MyView> {

       public void loadData() {
             repository.loadData(new Callback() {
                 @Override void onSuccess(Data data){
                          presenter.onceViewAttached(new ViewAction(){
                               @Override void run(MyView view){
                                    view.showData(data);
                                }
                          });
                  }
             });
       }
}

If you use ViewState feature, ViewState will be restored before running any queued ViewActions.

Please try 3.1.0 snapshot version and provide feedback if you face any issues. We will give you some time for feedback (lets say 1-2 weeks) to provide feedback on these changes and then we will publish a major 3.1.0 version to maven central. Thanks in advance!

Thomas-Vos commented 6 years ago

Hello Hannes,

Thank you very much for the snapshot. I'm currently in progress of testing it.

I noticed the parameter keepPresenterAndViewStateOnBackstack in the contructor of FragmentMvpViewStateDelegateImpl doesn't work correctly. This is how I initialise the class (within a Fragment):

new FragmentMvpViewStateDelegateImpl<>(this, this, true, false);

Now whenever you put a Fragment on the backstack, and then (after a few seconds) call ifViewAttached(true, ...), an exception is thrown:

java.lang.IllegalStateException: No View attached to Presenter. Presenter destroyed = false

The exception above says the presenter is not destroyed, that's why my RxJava subscription isn't stopped. Shouldn't the presenter be destroyed when the Fragment is put on the backstack?

Thanks, Thomas

sockeqwe commented 6 years ago

You are right, Presenter should be destroyed ... Thanks for reporting.

Do you have the source code where you use this somewhere on github so I can take that code for debugging? If not, no problem, it's just that it would save me some time to set up things ...

Thomas-Vos commented 6 years ago

Sorry, I don't have the code available on GitHub.

Also now there is back stack support I don't need to set the parameter to false anymore, but I thought it would be good to let you know.

Thomas-Vos commented 6 years ago

I have RxJava2 subjects that need to be subscribed/disposed between onStart and onStop (in the presenter, of course). That Subject calls onNext very often with new data for the view, so MvpQueuingBasePresenter would just make the queues too large. In a Fragment I can use attachView and detachView but it works differently in an Activity because the View is not yet initialised before calling super.onCreate(Bundle), so there is no way to display the data yet.

Is there a reason that am Activity is attached (to presenter) in onCreate and a Fragment is attached in onStart? How is the presenter supposed know when to "start" and "stop" subscribing to an infinite data "stream"?

Edit: Found a solution for now. I created a subscribe() and unsubscribe() method in the presenter and call those from the View in onStart() and onStop(). In those two method I subscribe/dispose to my data stream. This is similar to the TODO-MVP sample. (see Fragment and Presenter). I also created a onFirstCreate() method in the Presenter that is called from the View in onNewViewStateInstance() to setup some one-time view stuff. (e.g. toolbar title, loading indicator, etc). What do you think about this?

sockeqwe commented 6 years ago

The reason is that (historically) people start to invoke presenter methods right from Activity's onCreate() method which requires that the presenter is alive and has a view attached already in Activity.onCreate()

Thomas-Vos commented 6 years ago

I have a suggestion for the MvpQueuingBasePresenter. Currenly if you use this presenter implementation you can only use the method onceViewAttached(ViewAction). There is no way to just 'skip' the view action.

In an app I'm creating I made my own presenter implementation with both the methods onceViewAttached(ViewAction) and ifViewAttached(ViewAction). For example if data is loaded I call onceViewAttached(ViewAction) because this action cannot be skipped. However, if you want to show a message/error dialog to the user, should the dialog show if the user is navigating 'up' in the fragment backstack? I don't think a dialog is relevant anymore because the view could already have changed state.

What do you think about adding a method like ifViewAttached(ViewAction), or maybe a method to cancel the current view actions that are in the queue?

sockeqwe commented 6 years ago

Good point. I guess adding ifViewAttached(ViewAction) is the best way to deal with that problem. I'm wondering if we could make a method that combine both options with a parameter (boolean flag) to specify if ViewAction should be added to internal queue or not ...

Thomas Vos notifications@github.com schrieb am Di., 31. Okt. 2017, 19:41:

I have a suggestion for the MvpQueuingBasePresenter. Currenly if you use this presenter implementation you can only use the method onceViewAttached(ViewAction). There is no way to just 'skip' the view action.

In an app I'm creating I made my own presenter implementation with both the methods onceViewAttached(ViewAction) and ifViewAttached(ViewAction). For example if data is loaded I call onceViewAttached(ViewAction) because this action cannot be skipped. However, if you want to show a message/error dialog to the user, should the dialog show if the user is navigating 'up' in the fragment backstack? I don't think a dialog is relevant anymore because the view could already have changed state.

What do you think about adding a method like ifViewAttached(ViewAction), or maybe a method to cancel the current view actions that are in the queue?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/sockeqwe/mosby/issues/274#issuecomment-340862527, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnrjYjQhfCeHR-wO_jupaxefMDNQGlks5sx2nNgaJpZM4PT5ws .

Thomas-Vos commented 6 years ago

I think creating another method is the best way, so it stays similar to MvpBasePresenter (and it would be more easy to change a class from MvpBasePresenter to MvpQueuingBasePresenter). Other option would be to use something like ifViewAttached(boolean addToQueue, ViewAction action). What do you think?

BTW: I have been using Mosby 3.1.0 for a few weeks now in a published app and haven't run into any issues. Thanks a lot for this library!