Bug inside ViewPager

[leonardo2204]

Hey Hannes,

I created a sample project to show the bug. I'm using mosby with MVI inside a ViewPager (with stateAdapter), when you navigate to the mvi fragment, go back the view pager "destruction" offset and go forward again, the app crashes. It also happens if you start an Activity, navigate to the mvi fragment (inside view pager), go back (Destroying the Activity and start a new activity), the crash also happens.

Thanks and nice job !

Mosby Version:3.0.4

Expected behavior: Show fragment with no problem

https://github.com/leonardo2204/mvibug

[sockeqwe]

Thanks, I will take a look tomorrow.

We actually use MviFragment and ViewPager a lot at work (in Production for more than a year) and havent faced such an issue yet.

Leonardo Ferrari notifications@github.com schrieb am Fr., 21. Juli 2017, 22:24:

Hey Hannes,

I created a sample project to show the bug. I'm using mosby with MVI inside a ViewPager (with stateAdapter), when you navigate to the mvi fragment, go back the view pager "destruction" offset and go forward again, the app crashes. It also happens if you start an Activity, navigate to the mvi fragment (inside view pager), go back (Destroying the Activity and start a new activity), the crash also happens.

Thanks and nice job !

Mosby Version:3.0.4

Expected behavior: Show fragment with no problem

https://github.com/leonardo2204/mvibug

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/sockeqwe/mosby/issues/266, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnrgxbsKnd5HvQmrA7HqMZMhBdQYo7ks5sQQkagaJpZM4Of2rA .

[leonardo2204]

Thanks for the quick reply. Just a heads up, the problem seems to occur with FragmentStatePagerAdapter, changing to FragmentPagerAdapter works fine.

[sockeqwe]

Ah that might be the reason. Thanks for the information!

Leonardo Ferrari notifications@github.com schrieb am Fr., 21. Juli 2017, 22:41:

Thanks for the quick reply. Just a heads up, the problem seems to occur with FragmentStatePagerAdapter, changing to FragmentPagerAdapter works fine.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/sockeqwe/mosby/issues/266#issuecomment-317107266, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnrlH_b3lJsaz4SHzYrb8KjrG02eb9ks5sQQzjgaJpZM4Of2rA .

[leonardo2204]

Sure, I'm trying to figure out here also, I learnt that the fragment and activity variables get null'ed, but the MviDelegate stays in memory. After going back and forth, it tries to get the activity or fragment, but the delegate constructor is not called (since it's kept in memory), and it crahses.

[sockeqwe]

Without having looked at your code, i think I know what the problem is. should be easy to reproduce and fix with your provided example.

Leonardo Ferrari notifications@github.com schrieb am Fr., 21. Juli 2017, 23:07:

Sure, I'm trying to figure out here also, I learnt that the fragment and activity variables get destroyed, but the MviDelegate stays in memory. After going back and forth, it tries to get the activity or fragment, but the delegate constructor is not called (since it's kept in memory), and it crahses.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/sockeqwe/mosby/issues/266#issuecomment-317112964, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnrviccDXNlJOCeq3Gxd-60Dx6-VtFks5sQRMsgaJpZM4Of2rA .

[leonardo2204]

Cool, thanks !

[sockeqwe]

Interesting, you do the following:

    List<Fragment> fragmentList =
        Arrays.asList(new EmptyFragment(), new EmptyFragment(), new MviFragment());

    public MyAdapter(FragmentManager fm) {
      super(fm);
    }

    @Override public Fragment getItem(int position) {
      return fragmentList.get(position);
    }

    @Override public int getCount() {
      return fragmentList.size();
    }
  }

So you keep all your fragments in the ´fragmentList` and reuse the instance from the list. I don't know if this is good practice (and I never did it like that, hence I never run into this issue). from official google docs:

    /**
     * A simple pager adapter that represents 5 ScreenSlidePageFragment objects, in
     * sequence.
     */
    private class ScreenSlidePagerAdapter extends FragmentStatePagerAdapter {
        public ScreenSlidePagerAdapter(FragmentManager fm) {
            super(fm);
        }

        @Override
        public Fragment getItem(int position) {
            return new ScreenSlidePageFragment();
        }

        @Override
        public int getCount() {
            return NUM_PAGES;
        }

Source: https://developer.android.com/training/animation/screen-slide.html#viewpager

So I have no idea if this is a good idea to keep the fragment in memory like you did. Not sure if this is the intended way.

With an adapter like this (creates new fragment every time) I don't run into this bug:

class MyAdapter extends FragmentStatePagerAdapter {

    public MyAdapter(FragmentManager fm) {
      super(fm);
    }

    @Override public Fragment getItem(int position) {
            if (position == 2)
                return new MviFragment();

            return new EmptyFragment();
    }

    @Override public int getCount() {
      return fragmentList.size();
    }
  }

It also happens if you start an Activity, navigate to the mvi fragment (inside view pager), go back (Destroying the Activity and start a new activity), the crash also happens.

Not able to reproduce this add all (with your original ViewPager adapter that uses fragmentList).

[leonardo2204]

I actually have a wizard screen with variables steps, and I was creating the "steps" screen (fragments) according to necessity, adding them to a List. Bottom line, am I doing something wrong (I mean, which is the best way to reuse the fragments in a wizard like ViewPager) or maybe my sample did not work in your side ?

Again, thanks for helping !

[sockeqwe]

I could fix this crash, but I'm not sure what the consequences are and if I should let them left to the developer.

For example: Presenter gets detachView(false) or (unbindIntents() in MVI) call so that the presenter is technically "destroyed" if you swipe away from the MviFragment in ViewPager. Then if you swipe back the MviFragment it is reused from your List of Fragments, so is the associated Presenter. However, the Presenter has technically been "destoryed" before by calling detachView(false) / unbindIntents(). So we might have to setup the presenter again by calling bindIntents() etc. That would be doable. But is your presenter still working as expected after "reusing it?" it really depends if the user stores some internal state in his presenter. Then it might not be working anymore as expected. Hence instantiating a new fresh Presenter instance would be better. But then what's the point of keeping a Fragment Reference in the list?

So a possible fix could be: Don't crash, but don't reuse the presenter (so create a new fresh Presenter instance). What do you think?

[leonardo2204]

I've given this some thought and I agree with your points. I guess it was a poor implementation on my side. I completely rewrote it and it now works as expected. Thanks for your help on that matter and explanations :D