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

Whoa! Fragments! #771

Closed
wants to merge 19 commits into
base: v3.1
from

Conversation

Projects
None yet
9 participants
@Cheesebaron
Member

Cheesebaron commented Aug 22, 2014

I've spent some time fixing up Fragments and ViewModel rehydrating and saved states. This is only done in the Fragging library, but should be easy enough to pull out and put into the FullFragging library.

I've made a small sample showing it working here: https://github.com/Cheesebaron/MvxFragmentsAndHamburger there is a small readme telling what it does and what to expect.

Could someone review this, probably not the cleanest code, but it seems to work!

@kjeremy

View changes

Show outdated Hide outdated Cirrious/Cirrious.MvvmCross.Droid.Fragging/MvxCachingFragmentActivity.cs
// Only do something if we are not currently showing the tag at the contentId
if (!_currentFragments.ContainsKey(contentId) ||
(_currentFragments.ContainsKey(contentId) && _currentFragments[contentId] != tag))

This comment has been minimized.

@kjeremy

kjeremy Aug 25, 2014

Contributor

I'd assign the result of _currentFragments.ContainsKey to avoid a double look up.

@kjeremy

kjeremy Aug 25, 2014

Contributor

I'd assign the result of _currentFragments.ContainsKey to avoid a double look up.

@kjeremy

This comment has been minimized.

Show comment
Hide comment
@kjeremy

kjeremy Aug 25, 2014

Contributor

In my custom presenter I check if the view inherits from MvxDialogFragment (I'm using FullFragging) and then I simply do FragmentManager.Show(dialog, tag). I think that may be worth including in your base activity.

There's also a lot of code in ShowFragment if someone wants to override the transaction. For instance if someone wants to add to the back stack or apply a custom animation it looks like a lot of work. But now that I think about it maybe the user can do that in OnFragmentChanging?

This looks really great and it's MUCH needed. 👍

Contributor

kjeremy commented Aug 25, 2014

In my custom presenter I check if the view inherits from MvxDialogFragment (I'm using FullFragging) and then I simply do FragmentManager.Show(dialog, tag). I think that may be worth including in your base activity.

There's also a lot of code in ShowFragment if someone wants to override the transaction. For instance if someone wants to add to the back stack or apply a custom animation it looks like a lot of work. But now that I think about it maybe the user can do that in OnFragmentChanging?

This looks really great and it's MUCH needed. 👍

@slodge

This comment has been minimized.

Show comment
Hide comment
@slodge

slodge Aug 25, 2014

Contributor

Thanks @Cheesebaron - top work getting the fragments to load their own VM's from intents :)

Comments/questions - at least one duplicated from jabbr (sorry!):

  • I'm happy to merge this over the top of the existing Fragment code - it'll mean breaking changes for users but it'll be less pain for everyone than trying to mark and explain the old ones obsolete :)
  • I guess we need to update FullFragging as well as Fragging (the only difference is support library and fragmentactivity)
  • I'm wondering if there's code that could be shared between Activities and Fragment - the code in MvxFragmentExtensions.cs makes me wonder if IMvxAndroidView and IMvxFragmentView could share a common inherited interface to remove the copy and paste
  • wherever there is copy and paste (e.g. FragmentJavaName? Or was that just in a sample?) then I've love to try to avoid it if we can :)
  • I'm not sure about public static MvxFragment NewInstance(Bundle bundle) - is it used? Also, whenever I've added static factories in the past I've always had to replace them with IoC factory classes to make it customisable/overridable...
  • I don't know if the code for MvxCachingFragmentActivity should just go in MvxFragmentActivity
  • I need to look more at the savedInstanceState code in MvxCachingFragmentActivity - including its dependence on Json - compared to other code that uses IMvxNavigationSerializer and IMvxSavedStateConverter - need to look at this more (just my memory failing me!)
  • on a code note, there are some functions that are too long to fit on a page - and some if statements I might invert in order to reduce nesting (e.g. https://github.com/MvvmCross/MvvmCross/pull/771/files#diff-cba90beaacab183633da49c3fe280b53R45)
  • with the IMvxMultipleViewModelCache I would definitely wonder about whether we should do something to stop it growing too large - with the single vm cache we clear the entire cache when there is any miss
  • with the IMvxMultipleViewModelCache I'm wondering if VM type is unique enough as a key - are there situations where users want multiple VMs of the same type cached (e.g. in a list/collection of some kind or where multiple detail views are shown in a backstack)
  • is there some reason MvxMultipleViewModelCache uses a HashSet<IMvxViewModel> instead of a Dictionary<Type, IMvxViewModel> ? That could then allow quicker lookups using TryGetValue ?
  • I think public override void Show(MvxViewModelRequest request) in the fragment presenter should only create the bundle inside the if statement... also wonder if IMvxFragmentHost should have a Show method that doesn't rely on the bundle (not sure!)
  • how do we get the docs updated for this :) Wonder what samples do we need :) Currently I'm guessing that this should get pulled int 3.2.1 rather than into 3.1.x because of the breaking changes?

Overall.... yes yes yes.... very excited as the Intent-based Fragment VM loading makes the Android platform more beautiful - makes fragments really play nicely :)

Above are only questions.... your knowledge here way outstrips mine :)

Thanks again - loving it :)

Contributor

slodge commented Aug 25, 2014

Thanks @Cheesebaron - top work getting the fragments to load their own VM's from intents :)

Comments/questions - at least one duplicated from jabbr (sorry!):

  • I'm happy to merge this over the top of the existing Fragment code - it'll mean breaking changes for users but it'll be less pain for everyone than trying to mark and explain the old ones obsolete :)
  • I guess we need to update FullFragging as well as Fragging (the only difference is support library and fragmentactivity)
  • I'm wondering if there's code that could be shared between Activities and Fragment - the code in MvxFragmentExtensions.cs makes me wonder if IMvxAndroidView and IMvxFragmentView could share a common inherited interface to remove the copy and paste
  • wherever there is copy and paste (e.g. FragmentJavaName? Or was that just in a sample?) then I've love to try to avoid it if we can :)
  • I'm not sure about public static MvxFragment NewInstance(Bundle bundle) - is it used? Also, whenever I've added static factories in the past I've always had to replace them with IoC factory classes to make it customisable/overridable...
  • I don't know if the code for MvxCachingFragmentActivity should just go in MvxFragmentActivity
  • I need to look more at the savedInstanceState code in MvxCachingFragmentActivity - including its dependence on Json - compared to other code that uses IMvxNavigationSerializer and IMvxSavedStateConverter - need to look at this more (just my memory failing me!)
  • on a code note, there are some functions that are too long to fit on a page - and some if statements I might invert in order to reduce nesting (e.g. https://github.com/MvvmCross/MvvmCross/pull/771/files#diff-cba90beaacab183633da49c3fe280b53R45)
  • with the IMvxMultipleViewModelCache I would definitely wonder about whether we should do something to stop it growing too large - with the single vm cache we clear the entire cache when there is any miss
  • with the IMvxMultipleViewModelCache I'm wondering if VM type is unique enough as a key - are there situations where users want multiple VMs of the same type cached (e.g. in a list/collection of some kind or where multiple detail views are shown in a backstack)
  • is there some reason MvxMultipleViewModelCache uses a HashSet<IMvxViewModel> instead of a Dictionary<Type, IMvxViewModel> ? That could then allow quicker lookups using TryGetValue ?
  • I think public override void Show(MvxViewModelRequest request) in the fragment presenter should only create the bundle inside the if statement... also wonder if IMvxFragmentHost should have a Show method that doesn't rely on the bundle (not sure!)
  • how do we get the docs updated for this :) Wonder what samples do we need :) Currently I'm guessing that this should get pulled int 3.2.1 rather than into 3.1.x because of the breaking changes?

Overall.... yes yes yes.... very excited as the Intent-based Fragment VM loading makes the Android platform more beautiful - makes fragments really play nicely :)

Above are only questions.... your knowledge here way outstrips mine :)

Thanks again - loving it :)

@Cheesebaron

This comment has been minimized.

Show comment
Hide comment
@Cheesebaron

Cheesebaron Aug 29, 2014

Member

I'm not sure about public static MvxFragment NewInstance(Bundle bundle) - is it used? Also, whenever I've added static factories in the past I've always had to replace them with IoC factory classes to make it customisable/overridable...

There are two practices when instantiating Fragments in Android. Either using Fragment.Instantiate() or NewInstance(). The former calls the empty constructor and sets the Bundle passed to it on the Arguments property. The latter is if you manually create an Fragment somewhere in your code. So the NewInstance is there for the use case where a developer does not want to use the caching Activity.

I don't know if the code for MvxCachingFragmentActivity should just go in MvxFragmentActivity

Me neither, maybe some wants to handle this stuff himself?

on a code note, there are some functions that are too long to fit on a page - and some if statements I might invert in order to reduce nesting (e.g. https://github.com/MvvmCross/MvvmCross/pull/771/files#diff-cba90beaacab183633da49c3fe280b53R45)

I am aware of this, and it is also why I wrote it is not the cleanest code in the OP :)

is there some reason MvxMultipleViewModelCache uses a HashSet instead of a Dictionary<Type, IMvxViewModel> ? That could then allow quicker lookups using TryGetValue?

I don't think there is a reason, I think I just wanted a collection of items that only contained unique items.

I need to look more at the savedInstanceState code in MvxCachingFragmentActivity - including its dependence on Json - compared to other code that uses IMvxNavigationSerializer and IMvxSavedStateConverter - need to look at this more (just my memory failing me!)

Just what I found convenient, might work fine with the NavigationSerializer, but don't they do the same?

Member

Cheesebaron commented Aug 29, 2014

I'm not sure about public static MvxFragment NewInstance(Bundle bundle) - is it used? Also, whenever I've added static factories in the past I've always had to replace them with IoC factory classes to make it customisable/overridable...

There are two practices when instantiating Fragments in Android. Either using Fragment.Instantiate() or NewInstance(). The former calls the empty constructor and sets the Bundle passed to it on the Arguments property. The latter is if you manually create an Fragment somewhere in your code. So the NewInstance is there for the use case where a developer does not want to use the caching Activity.

I don't know if the code for MvxCachingFragmentActivity should just go in MvxFragmentActivity

Me neither, maybe some wants to handle this stuff himself?

on a code note, there are some functions that are too long to fit on a page - and some if statements I might invert in order to reduce nesting (e.g. https://github.com/MvvmCross/MvvmCross/pull/771/files#diff-cba90beaacab183633da49c3fe280b53R45)

I am aware of this, and it is also why I wrote it is not the cleanest code in the OP :)

is there some reason MvxMultipleViewModelCache uses a HashSet instead of a Dictionary<Type, IMvxViewModel> ? That could then allow quicker lookups using TryGetValue?

I don't think there is a reason, I think I just wanted a collection of items that only contained unique items.

I need to look more at the savedInstanceState code in MvxCachingFragmentActivity - including its dependence on Json - compared to other code that uses IMvxNavigationSerializer and IMvxSavedStateConverter - need to look at this more (just my memory failing me!)

Just what I found convenient, might work fine with the NavigationSerializer, but don't they do the same?

@Cheesebaron

This comment has been minimized.

Show comment
Hide comment
@Cheesebaron

Cheesebaron Aug 29, 2014

Member

how do we get the docs updated for this :) Wonder what samples do we need :) Currently I'm guessing that this should get pulled int 3.2.1 rather than into 3.1.x because of the breaking changes?

Yeah this should probably be in 3.2.1 because of the breakage.

For samples I think:

  1. Hamburger menu
  2. Tabs
  3. ViewPager
  4. Standalone Fragments
  5. Nested Fragments (haven't tested this, not sure how it will work...)
  6. App with one Activity and using Fragments for everything (that should probably be true for all the other samples as well)
Member

Cheesebaron commented Aug 29, 2014

how do we get the docs updated for this :) Wonder what samples do we need :) Currently I'm guessing that this should get pulled int 3.2.1 rather than into 3.1.x because of the breaking changes?

Yeah this should probably be in 3.2.1 because of the breakage.

For samples I think:

  1. Hamburger menu
  2. Tabs
  3. ViewPager
  4. Standalone Fragments
  5. Nested Fragments (haven't tested this, not sure how it will work...)
  6. App with one Activity and using Fragments for everything (that should probably be true for all the other samples as well)
@Cheesebaron

This comment has been minimized.

Show comment
Hide comment
@Cheesebaron

Cheesebaron Sep 12, 2014

Member

Uhm... Not going to use the IMvxNavigationSerializer, it just complicates things if you want to serialized the two dicts directly. However if it is possible to chuck them into a Bundle somehow or nicely convert them to Dict<string,string> then it would be possible.

Any ideas?

Member

Cheesebaron commented Sep 12, 2014

Uhm... Not going to use the IMvxNavigationSerializer, it just complicates things if you want to serialized the two dicts directly. However if it is possible to chuck them into a Bundle somehow or nicely convert them to Dict<string,string> then it would be possible.

Any ideas?

@slodge

This comment has been minimized.

Show comment
Hide comment
@slodge

slodge Sep 16, 2014

Contributor

Any ideas?

Main idea would be that we should take a hacksaw to the serializer and make it easier to remember what the heck it does...

This all started when we removed our dependency on Json.Net - which was maybe 2 years ago :)

Currently building a release of 3.2 - let's ship it :) Will then try to get my head around this stuff - 3.3 ?

:)

Stuart (working and cycling too much!)

Contributor

slodge commented Sep 16, 2014

Any ideas?

Main idea would be that we should take a hacksaw to the serializer and make it easier to remember what the heck it does...

This all started when we removed our dependency on Json.Net - which was maybe 2 years ago :)

Currently building a release of 3.2 - let's ship it :) Will then try to get my head around this stuff - 3.3 ?

:)

Stuart (working and cycling too much!)

@touriliY

This comment has been minimized.

Show comment
Hide comment
@touriliY

touriliY Sep 29, 2014

Related to this post http://stackoverflow.com/questions/25978575/mvvmcross-override-style-in-fragment,
I will suggest a small override to the extension in the MvxFragmentExtensions.cs to expose consumer choice for the droid context
touriliY@7151bf2#diff-77ea87c0c3989dc6c3d7c33afb1ae96b

touriliY commented Sep 29, 2014

Related to this post http://stackoverflow.com/questions/25978575/mvvmcross-override-style-in-fragment,
I will suggest a small override to the extension in the MvxFragmentExtensions.cs to expose consumer choice for the droid context
touriliY@7151bf2#diff-77ea87c0c3989dc6c3d7c33afb1ae96b

@IlSocio

This comment has been minimized.

Show comment
Hide comment
@IlSocio

IlSocio Oct 28, 2014

Contributor

Hello everyone :)
is there any news about the v3.3 release date? (including proper Fragments support)

Marco.

Contributor

IlSocio commented Oct 28, 2014

Hello everyone :)
is there any news about the v3.3 release date? (including proper Fragments support)

Marco.

@Cheesebaron

This comment has been minimized.

Show comment
Hide comment
@Cheesebaron

Cheesebaron Oct 29, 2014

Member

It will be in 3.5 and don't ask for release dates, it is a open source project. Whenever you ask for a release data god kills a kitten and postpones the release a month. If you want to speed it up, get involved.

Member

Cheesebaron commented Oct 29, 2014

It will be in 3.5 and don't ask for release dates, it is a open source project. Whenever you ask for a release data god kills a kitten and postpones the release a month. If you want to speed it up, get involved.

@IlSocio

This comment has been minimized.

Show comment
Hide comment
@IlSocio

IlSocio Oct 31, 2014

Contributor

sounds reasonable :)

Contributor

IlSocio commented Oct 31, 2014

sounds reasonable :)

@Cheesebaron

This comment has been minimized.

Show comment
Hide comment
@Cheesebaron

Cheesebaron Oct 31, 2014

Member

Some of it has already been merged into the 3.5 branch. Use at own risk ;)

Member

Cheesebaron commented Oct 31, 2014

Some of it has already been merged into the 3.5 branch. Use at own risk ;)

@slodge

This comment has been minimized.

Show comment
Hide comment
@slodge

slodge Nov 13, 2014

Contributor

WooHoo - I'm in there building the v3.5 branch :)

What do we do about FullFragging?

I'm a bit tempted to drop Gingerbread support.... then we would only need FullFragging...

Is anyone seriously targeting Gingerbread for apps now? Maybe industrial apps who bought very old tablets?

Contributor

slodge commented Nov 13, 2014

WooHoo - I'm in there building the v3.5 branch :)

What do we do about FullFragging?

I'm a bit tempted to drop Gingerbread support.... then we would only need FullFragging...

Is anyone seriously targeting Gingerbread for apps now? Maybe industrial apps who bought very old tablets?

@pkl728

This comment has been minimized.

Show comment
Hide comment
@pkl728

pkl728 Nov 13, 2014

If they're spending the time having to support Gingerbread then they can just keep using older version of MvvmCross IMO ;)

Gingerbread and below = 10.4% of total Android market share. I think it's safe to move on. http://developer.android.com/about/dashboards/index.html as of November 3, 2014

pkl728 commented Nov 13, 2014

If they're spending the time having to support Gingerbread then they can just keep using older version of MvvmCross IMO ;)

Gingerbread and below = 10.4% of total Android market share. I think it's safe to move on. http://developer.android.com/about/dashboards/index.html as of November 3, 2014

@Cheesebaron

This comment has been minimized.

Show comment
Hide comment
@Cheesebaron

Cheesebaron Nov 14, 2014

Member

Apparently there are some nutters still having old devices. However, I would be inclined to say that support for them should be dropped.

If you look at the history of devices with these old versions, they are made by unknown small cheapo manufacturers, which don't care one bit about updating the OS on the devices. Only $$$$ matters. It probably also includes some users that do not wish to update to newer version for some reason. But basically these devices running old software, should just be phased out. They have so many security and other flaws, that make people cry.

TL;DR: FUCK API 10!

Member

Cheesebaron commented Nov 14, 2014

Apparently there are some nutters still having old devices. However, I would be inclined to say that support for them should be dropped.

If you look at the history of devices with these old versions, they are made by unknown small cheapo manufacturers, which don't care one bit about updating the OS on the devices. Only $$$$ matters. It probably also includes some users that do not wish to update to newer version for some reason. But basically these devices running old software, should just be phased out. They have so many security and other flaws, that make people cry.

TL;DR: FUCK API 10!

@Dexyon

This comment has been minimized.

Show comment
Hide comment
@Dexyon

Dexyon Nov 14, 2014

Contributor

I agree on dropping the Gingerbread support. We should move forward and not try to be to kind with maintaining backwards compatibility especially I this means we cannot move forward.

My take on the GingerBread and below is 10% of the Market-Share. These are all kinds of devices. I wonder how many percent of these people can even still update other apps. Most companies I know target Android 4 and higher.

Contributor

Dexyon commented Nov 14, 2014

I agree on dropping the Gingerbread support. We should move forward and not try to be to kind with maintaining backwards compatibility especially I this means we cannot move forward.

My take on the GingerBread and below is 10% of the Market-Share. These are all kinds of devices. I wonder how many percent of these people can even still update other apps. Most companies I know target Android 4 and higher.

@slodge

This comment has been minimized.

Show comment
Hide comment
@slodge

slodge Nov 23, 2014

Contributor

So this is all now merged into 3.5 - and we still have Gingerbread (Fragging) as well as less-ancient (FullFragging) support - so it should keep everyone happy....

I've looked through the changes again - and I can't really see anything I really want to refactor or change. There are a few small copy and paste sections in the FragmentAdapter (compared to the ActivityAdapter) but I can definitely live with those for now :)

So.... who wants to write a blog post about how to use these new Fragment features - I know quite a few people have said they've used them and that they like them :)

Contributor

slodge commented Nov 23, 2014

So this is all now merged into 3.5 - and we still have Gingerbread (Fragging) as well as less-ancient (FullFragging) support - so it should keep everyone happy....

I've looked through the changes again - and I can't really see anything I really want to refactor or change. There are a few small copy and paste sections in the FragmentAdapter (compared to the ActivityAdapter) but I can definitely live with those for now :)

So.... who wants to write a blog post about how to use these new Fragment features - I know quite a few people have said they've used them and that they like them :)

@martijn00

This comment has been minimized.

Show comment
Hide comment
@martijn00

martijn00 Jan 24, 2015

Member

Since this is merged into 3.5, this PR can be closed.

Member

martijn00 commented Jan 24, 2015

Since this is merged into 3.5, this PR can be closed.

@martijn00 martijn00 closed this Jan 24, 2015

@martijn00 martijn00 added this to the 3.5.0 milestone Jan 25, 2015

@davidschwegler

This comment has been minimized.

Show comment
Hide comment
@davidschwegler

davidschwegler Jan 30, 2015

Contributor

Disclaimer: I'm sorry if this isn't the correct place to comment (if not, let me know where to post this kind of thing in the future). :)

I appreciate that support fragments were included in this release, but I just wanted to emphasize this due to the above discussion about removing them.

As I see it, it's extremely important that support for Android.Support.V4.App.Fragment is not removed from MvvmCross. It's not about whether or not to support Gingerbread (we dropped support about a year ago for it), but the stability and feature set, for example http://stackoverflow.com/questions/17295497/fragment-or-support-fragment

A few reasons:

  • There were several crashes in the Android 4.0 release of the built-in Android.App.Fragment system (in fact, there's even crashes in 4.2 https://code.google.com/p/android/issues/detail?id=42601).
  • The ChildFragmentManager isn't supported in official fragments until Android 4.2, which is a very common use-case (at least for us). http://developer.android.com/reference/android/app/Fragment.html#getChildFragmentManager()
  • There were implementation fixes/tweaks in several releases since 4.x, and that have caused slightly different behavior in edge cases. As a result, knowing that you have the same implementation on all OS versions has made for far easier debugging.
  • When I talked to Roman Nurik at Google I/O this year, he said that support packages are not going away, but actually going to be using them as much as possible because of how challenging it is to get carriers/OEMs to publish OS updates in a timely manner.
Contributor

davidschwegler commented Jan 30, 2015

Disclaimer: I'm sorry if this isn't the correct place to comment (if not, let me know where to post this kind of thing in the future). :)

I appreciate that support fragments were included in this release, but I just wanted to emphasize this due to the above discussion about removing them.

As I see it, it's extremely important that support for Android.Support.V4.App.Fragment is not removed from MvvmCross. It's not about whether or not to support Gingerbread (we dropped support about a year ago for it), but the stability and feature set, for example http://stackoverflow.com/questions/17295497/fragment-or-support-fragment

A few reasons:

  • There were several crashes in the Android 4.0 release of the built-in Android.App.Fragment system (in fact, there's even crashes in 4.2 https://code.google.com/p/android/issues/detail?id=42601).
  • The ChildFragmentManager isn't supported in official fragments until Android 4.2, which is a very common use-case (at least for us). http://developer.android.com/reference/android/app/Fragment.html#getChildFragmentManager()
  • There were implementation fixes/tweaks in several releases since 4.x, and that have caused slightly different behavior in edge cases. As a result, knowing that you have the same implementation on all OS versions has made for far easier debugging.
  • When I talked to Roman Nurik at Google I/O this year, he said that support packages are not going away, but actually going to be using them as much as possible because of how challenging it is to get carriers/OEMs to publish OS updates in a timely manner.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment