ViewPager Adapters cleanup

[nmilcoff]

Description 📜

We currently expose four ViewPager Fragment Adapters:

• MvxFragmentStatePagerAdapter
• MvxFragmentPagerAdapter
• MvxCachingFragmentStatePagerAdapter
• MvxCachingFragmentPagerAdapter

It looks like the ones with the word "Caching" are there because in 2014 there was an issue with the adapters provided by the Android SDK.

Maybe it's time to remove them?

Expected behavior 🤔

We should expose only two ViewPager adapters.

Actual behavior 🐛

We are exposing too many ViewPager adapters

Configuration 🔧

Version: 5.x

Platform:

• 📱 iOS
• 🤖 Android
• 🏁 WPF
• 🌎 UWP
• 🍎 MacOS
• 📺 tvOS
• 🐒 Xamarin.Forms

[Qwin]

@nmilcoff
Seems like
MvxFragmentStatePagerAdapter
MvxFragmentPagerAdapter
Both dont add support to add Tags on each fragment, while the caching ones do. I know we use the tags to add nested fragments within fragments.

[nmilcoff]

@Qwin can you elaborate a bit more please? I've been browsing the source code but I don't fully get it. It looks like those adapters only provide getter methods for tags.

[Qwin]

@nmilcoff yes of course, if you look at MvxCachingFragmentPagerAdapter.cs you will see this :

In the the last line you will see they are setting for the fragment a fragmentTag (which is just the name of the fragment type) by using the fragmentmanager. the tag is used for adding nested fragments. Nested fragments use the method GetFragmentByFragmentTag internally. I hope its clear if you want to know about how the nested fragment works internally I can elaborate even more on it.

Anyways the
MvxFragmentStatePagerAdapter
MvxFragmentPagerAdapter
both dont add a tag. I am guessing this is just missing ?

[Qwin]

@nmilcoff I noticed even though we are adding the Tag in the custom implementation (caching classes), it would still require some work on the: MvxAppCompatViewPresenter.cs
As MvxAppCompatViewPresenter only supports SupportFragmentManager or (if using the non appcompat) FragmentManager but both do not currently support ChildFragmentManager(what I mean is they never call that fragmentManager to check the tags in there) which is the required one to support nested fragments in ViewPagers.

I would suggest the following happens if we want to do a good job at removing the Caching classes:

1. Add the tagging by overriding the InstantiateItem in MvxFragmentPagerAdapter
2. Maybe add support for ChildFragmentManager in the android presenters (and also support for multi level nesting)
3. Remove the Caching ones.

Considering I am running into this issue anyways currently I might rewrite the MvxAppCompatViewPresenter to support it and do a PR.

[Qwin]

So I think @nmilcoff based on what I have seen so far is that we need do need to override the InstantiateItem for both MvxFragmentStatePagerAdapter and MvxFragmentPagerAdapter and call ourselves the GetItem as well and set a default tag on it like so:

This will cause each added fragment to have at least a default Tag and when people want to override that Tag they can by overriding the GetTag method.

Also looking at the presenter I already implemented a way to support ChildFragmentManager (recursive) like so (this will search the hierarchie for the right Tag on a Fragment and can then handle setting it on the view, so we would need to add this to both presenters in Android) :

I tested the code and it works as expected.

[martijn00]

@Qwin you are on the right track i think! Can you make a PR so we can test this?

[Qwin]

@martijn00 will do :)

[nmilcoff]

@Qwin based on your work on #2482 what are we missing to remove the caching adapters?

From your previous comment looks like this is still necessary:

override the InstantiateItem for both MvxFragmentStatePagerAdapter and MvxFragmentPagerAdapter and call ourselves the GetItem as well and set a default tag

[nickrandolph]

@nmilcoff @martijn00 is this still going to make it into 6.0, or can we push to 6.1?

[wh1t3cAt1k]

To reiterate what @Qwin said about fragment tags, I have just encountered this issue after switching from 4.4.0 to 5.7.0 and stopping to use caching pager adapter.

My scenario is as follows:

• Suppose I have tab layout with a single tab TabViewModel / TabView.
• I create a pager adapter that tags the fragment info as "RootViewModel --> TabViewModel".
• I navigate to a second screen that has the same tab TabViewModel / TabView.
• There, I create a pager adapter that tags the fragment info as "SecondViewModel --> TabViewModel".
• Suppose I then move my application into background.
• On saving android instance state the tab fragments have the same tags like android:switcher:2131624826:1, my tags are completely ignored.
• With this identical tag they are saved to IMvxMultipleViewModelCache.
• Which means that on restoring instance state (e.g. when I restore the app from the background) it is GetAndClear()ed from the cache on the second screen.
• When I press back, the TabViewModel is not in the cache anymore, and it is resolved anew.

If my tags were used, these view model instances would have been stored under different tags and cache would safely store / get and clear twice.

[nmilcoff]

@nickrandolph we should take the chance to work on this for v6, as it involves a breaking change.

[nickrandolph]

@nmilcoff are we able to generate a PR that demonstrates this issue in the playground, so we can get this issue resolved

[nmilcoff]

I've been digging more about this and it seems to me the most reasonable way to cleanup our adapters might be to keep the Caching ones and remove the others, because otherwise we loose support for nested fragments.

To avoid this we would need to fully override the behavior of InstantiateItem method in MvxFragmentPagerAdapter and MvxFragmentStatePagerAdapter... which is exactly what the Caching adapters are doing :)

We must then modify the Android ViewPresenters to support the nesting scenario better, but that's another step.

What do you people think about this? (cc: @Qwin @wh1t3cAt1k)

[nickrandolph]

@nmilcoff is this completed now, or is there more work to be done?

[nmilcoff]

Well... I would like to have only two ViewPager adapters and not four, but that doesn't bring any actual value to our users - and it is a breaking change.

So I would say we can schedule it for the next major version.