Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changelog of 3.1.0-SNAPSHOT #274

Closed
sockeqwe opened this issue Sep 11, 2017 · 8 comments
Closed

Changelog of 3.1.0-SNAPSHOT #274

sockeqwe opened this issue Sep 11, 2017 · 8 comments

Comments

@sockeqwe
Copy link
Owner

sockeqwe commented Sep 11, 2017

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.

  • Backstack support for MVP Fragments ViewPager on back stack calls Presenter's method detachView(retainInstance=true) #271. Keep in mind that MvpBasePresenter will just do nothing if view is not attached like Fragment on the backstack. In that case any information will be "lost" because there is no View to update. we recommend looking into MVI or use ViewState + MvpQueuingBasePresenter or write your own Presenter that handles state properly (not that hard as it sounds but requires a unidirectional data flow and state driven by business logic.)
  • Added MvpQueuingBasePresenter that can be used together with ViewState feature to have better backstack support. The idea is that while View is detached (i.e. while fragment is not visible but on backstack) MvpQueuingBasePresenter holds internally a queue of ViewActions that will be executed once the View is reattached. Use presenter.onceViewAttached(ViewAction) like this:
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.

  • Added LceViewState.isLoadingState() to interface LceViewState. Unfortunately this is a breaking API change if you use your own LceViewState implementation that has been compiled with Mosby 3.0.x. So should we increase Version to 4.0.0 instead of 3.1.0 ? But this would also mean that the package will be renamed to com.hannesdorfmann.mosby4 which seems to be much more annoying than the handful user (if not zero users) that use a custom already compiled (i.e. part of another android library the app uses) LceViewState.
  • FragmentMvpViewStateDelegateImpl has changed internally a little bit. Presenters are now available in Fragment.onStart() (was Fragment.onViewCreated() before). This only affects ViewState Fragment. Since you should use (and always should have used) onNewViewState() and onViewStateRestored() to start loading data in a ViewState Fragment this internal change should effect nobody.

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
Copy link
Contributor

Thomas-Vos commented Sep 23, 2017

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
Copy link
Owner Author

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
Copy link
Contributor

Thomas-Vos commented Sep 23, 2017

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
Copy link
Contributor

Thomas-Vos commented Sep 24, 2017

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
Copy link
Owner Author

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
Copy link
Contributor

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
Copy link
Owner Author

sockeqwe commented Oct 31, 2017 via email

@Thomas-Vos
Copy link
Contributor

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants