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

onNewViewStateInstance called every time when returning Fragment from backstack #153

Closed ghost closed 7 years ago

ghost commented 8 years ago

There is how I put fragment in backstack

    private fun <T : Fragment> putContentFragment(fragment: T): T {
        val fragmentTag = fragment.javaClass.name
        logger.debug("putContentFragment: tag: {}", fragmentTag)

        fragmentMng.beginTransaction()
                .setTransition(FragmentTransaction.TRANSIT_FRAGMENT_OPEN)
                .replace(R.id.content_frame_layout, fragment, fragmentTag)
                .addToBackStack(fragmentTag)
                .commit()

        return fragment
    }

And there is how I navigateBack to the previous Fragment:

    override fun navigateBack() {
        if (fragmentMng.backStackEntryCount > 1) {
            callback.forceBackPressed() // it calls super.onBackPressed() for MainActivity
        } else {
            callback.finish() // it calls finish() for MainActivity
        }
    }

I also call setRetainInstance(true) on my Fragment. Class variables are restored as they must. I test it via incrementing int var every onViewCreated():

    private var someValue = 0

    override fun onViewCreated(view: View?, savedInstanceState: Bundle?) {
        super.onViewCreated(view, savedInstanceState)
        someValue++
        logger.debug("someValue: {}", someValue)
        ...
    }

    override fun createViewState() = NotesViewState()

UPD

Sometimes onNewViewStateInstance() called, sometimes not.

For example, I navigate to new fragment and then return back to the previous: image onNewViewStateInstance called only for new NoteFragment and NotesFragment was restored.

Okay. Lets restart my app and do the same: image

There it is: onNewViewStateInstance called every time when returning Fragment from backstack

sockeqwe commented 8 years ago

Yes, this is the desired behavior. Mosby saves the ViewState in the Fragment. Since the Fragment is on the backstack it can not use setRetainInstance(true). Therefore, ViewState has to be Parcelable. Hence return a ViewState implements RestorableParcelableViewState in createViewState()

See documentation: http://hannesdorfmann.com/mosby/viewstate/ section with the name "How does the ViewState survive screen orientation changes?"

Maybe this will be changed in Mosby 3.0.

ghost commented 8 years ago

Since the Fragment is on the backstack it can not use setRetainInstance(true).

But it uses. See example in my first message. someValue increments when I navigate to other Fragment and back. ViewState must be kept too. (it kept sometimes until I restart my app, see screenshots)

Class variables are restored as they must. I test it via incrementing int var every onViewCreated(): ...

sockeqwe commented 8 years ago

I assume that in your app you are navigating from NotesFragment to NoteFragment. So in you example the navigation stack is:

  1. NoteFragment (currently visible to the user)
  2. NotesFragment (on the back stack)

Furthermore, I assume that with "restart my app and do the same" you mean that you kill the app process (or destroy hosting activity) and then restart the app so that the activity and so on are restored (Bundle in onCreate() is not null) ?

If both assumptions are correct, then this works as expected. Not sure someValue. I doubt that this value survives restarts without putting it into a bundle.

ghost commented 8 years ago

I assume that in your app you are navigating from NotesFragment to NoteFragment. So in you example the navigation stack is:

NoteFragment (currently visible to the user) NotesFragment (on the back stack)

Right.

I'm not so good in English :)

I will try to rephrase. In some app sessions, ViewState is restored when I navigate to the previous Fragment from backstack. In others, it don't. someValue not stored into Bundle. It survives by setRetainINnstance(true)(but ViewState often recreates).

p.s. "restart my app" == "start new app session"

sockeqwe commented 8 years ago

I might be wrong and something has been changed in latest support library releases but as far as I know it is not possible to put a Fragment with setRetainInstance(true) on the backstack .

ghost commented 8 years ago

I will create demo app and will write back soon.

ghost commented 8 years ago

Done. https://github.com/Try4W/MosbyRetainingFragmentBackstackDemo

There is no Mosby included in the project, but it proofs that "backstacked" Fragments usually retained.

sockeqwe commented 8 years ago

Thanks for this demo. I'm wondering when this has been changed, because I remember that it was not possible to add retaining fragments to the backstack.

So you still run into the problem that onNewViewState() is called every time you come back from backstack to the previous fragment, right?

Btw. in your Activity you have to check if (savedInstanceState == null) before adding FirstFragment, otherwise your code will create a new FirstFragment on each screen orientation change.

Like this:

  @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);

        if (savedInstanceState == null) {
            putFragment(new FirstFragment());
        }
    }
ghost commented 8 years ago

Btw. in your Activity you have to check if (savedInstanceState == null) before adding FirstFragment, otherwise your code will create a new FirstFragment on each screen orientation change.

Yes, I forgot about it :)

So you still run into the problem that onNewViewState() is called every time you come back from backstack to the previous fragment, right?

Yeah. But sometimes onNewViewState() called like it must until I restart my app:

In some app sessions, ViewState is restored when I navigate to the previous Fragment from backstack.

ghost commented 8 years ago

It looks that there is no elegant way to update backstacked Views.

For example, I have two fragments.

The first one is ProjectFragment which holds ViewPager with multiple nested fragments. The second is EditProjectFragment.

User open app, see ProjectFragment and navigates to EditProjectFragment

When EditProjectFragment saves Project into model, I push ProjectEdited event into my RxBus. Then, ViewPager's fragments(presenters) handle this event and trying to update Views that is detached.

This problem can be solved via placing ViewState between View and Presenter. Like moxy does:

moxy demonstration

sockeqwe commented 8 years ago

yes that is true ... That is more a "philosophical" question: If no View exists (or is detached), why should a presenter exist for such non existing view?

Nevertheless, I understand the problem you are facing, but I think this problem can be solved differently. No EventBus, just ensure that all presenters observe the same model.

In Mosby 3.0 I will introduce a new Model-View-Intent module. This will solve that "observing same model" problem and also remove the ViewState at all...

ghost commented 8 years ago

yes that is true ... That is more a "philosophical" question: If no View exists (or is detached), why should a presenter exist for such non existing view?

That sounds logical, yes.

In Mosby 3.0 I will introduce a new Model-View-Intent module. This will solve that "observing same model" problem and also remove the ViewState at all...

Interesting. I will read about it. When Mosby 3.0 is expected to be released? :)

... this problem can be solved differently. No EventBus, just ensure that all presenters observe the same model.

What do you mean?


I think that the easiest solution for now is prevent ViewState from retaining when the user navigates back to previous Fragment. Or how can I force ViewState to be recreated after returning to the previous Fragment from backstack?

ghost commented 8 years ago

My workaround to update only at the first start&returning from backstack:

private boolean isBackStacked = false;
private boolean isFreshInstance = true;

@Override
public void onActivityCreated(@Nullable Bundle savedInstanceState) {
    super.onActivityCreated(savedInstanceState);
    if(isFreshInstance || isBackStacked) { // fresh instance or returned from back stack
        isBackStacked = false;
        logger.debug("initializing presenter...");
        presenter.initialize();
    }
}

@Override
public void onDestroyView() {
    super.onDestroyView();
    isFreshInstance = false;
}

@Override
public void onStop() {
    super.onStop();
    if(!getActivity().isChangingConfigurations()) {
        isBackStacked = true;
    }
}

I think that better solution will be to write my own MvpDelegate but it looks very complicated to understand&edit Mosby's architecture without UML diagrams or something like that.

fmnstr commented 8 years ago

I had the same issue on my app. What I found was that if onSaveInstanceState was not called (eg. from screen orientation) mosby library could not correctly update the "applyViewState" flag on MvpViewStateInternalDelegate . This flag gets updated in saveViewState and in createOrRestoreViewState. The latter is not called from a retained fragment coming from backstack so the flag is not applied correctly. So I came up with the following delegate to address this issue.

/**
 * Delegate to help an {@link com.hannesdorfmann.mosby.mvp.viewstate.MvpViewStateFragment} retain its viewstate
 * when added to backstack without an orientation change.
 * When adding a fragment to backstack {@link android.support.v4.app.Fragment#onSaveInstanceState(Bundle)}
 * is not called, so the mvp library does not know that a retained viewstate exists. After the first creation we need the mvp
 * library to reset its flags in order to use the retained state with a call in {@link FragmentMvpDelegate#onCreate(Bundle)}.
 *
 * @see MvpViewStateInternalDelegate#applyViewState
 */
public class MvpViewStateBackStackSupportDelegate {

    private boolean mHasBundleOrFirstCreation;

    public MvpViewStateBackStackSupportDelegate() {
        mHasBundleOrFirstCreation = true;
    }

    public void onViewCreated(ViewState viewState, FragmentMvpDelegate mvpDelegate, Bundle savedInstanceState) {
        if (!mHasBundleOrFirstCreation && viewState != null) { //retained view state
            mvpDelegate.onCreate(savedInstanceState); //restore variables to later apply state
        }

        if (mHasBundleOrFirstCreation)
            mHasBundleOrFirstCreation = false; //reset flag, after above check
    }

    public void onSaveInstanceState() {
        mHasBundleOrFirstCreation = true;
    }

Example usage:

    private MvpViewStateBackStackSupportDelegate mBackStackSupportDelegate;

    @Override
    public void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        mBackStackSupportDelegate = new MvpViewStateBackStackSupportDelegate();
    }

    @Override
    public void onViewCreated(View view, @Nullable Bundle savedInstanceState) {
        super.onViewCreated(view, savedInstanceState);

        setRetainInstance(true);

        mBackStackSupportDelegate.onViewCreated(getViewState(), getMvpDelegate(), savedInstanceState);
    }

    @Override
    public void onSaveInstanceState(Bundle outState) {
        super.onSaveInstanceState(outState);
        mBackStackSupportDelegate.onSaveInstanceState();
    }
sockeqwe commented 8 years ago

thanks for your feedback! I will investigate and evaluate that and build that into the internal delegate

sockeqwe commented 7 years ago

Please note that I have added a Model-View-Intent (MVI) module to mosby. A sample is available in this repo. Pre release (alpha release of Mosby 3.0.0-alpha1) coming soon. I think MVI solves this kind of problem in a more elegant way. Nevertheless, I will invest some time to find a solution for MVP too.

sockeqwe commented 7 years ago

Fixed for MVP 3.0