[Android] Let inflight life cycle events finish up before tearing them down #3187
Conversation
PureWeen
added
the
DO-NOT-MERGE-!!! 🛑
…ent is going away
|
build --uitests |
PureWeen
added
p/Android
and removed
DO-NOT-MERGE-!!! 🛑
| RunningApp.Tap("Start Over"); | ||
| } | ||
|
|
||
| // Various tests are commented out on certain platforms because |
There was a problem hiding this comment.
Can we get references in the comments for the known issues? That way it'll be easier to clean this up once they get fixed.
| using Android.Views; | ||
| using Android.Widget; | ||
|
|
||
| namespace Xamarin.Forms.Internals |
There was a problem hiding this comment.
Is this the correct namespace for this? Or should it be Xamarin.Forms.Platform.Android? It's already an internal interface, so I don't think we have to worry about third parties taking a dependency.
| { | ||
| public static FragmentTransaction RemoveEx(this FragmentTransaction @this, Fragment fragment) | ||
| { | ||
| return @this.Remove(fragment); |
There was a problem hiding this comment.
I would suggest changing @this to fragmentTransaction. Less confusing.
| namespace Xamarin.Forms.Platform.Android | ||
| { | ||
| // This is a way to centralize all fragment modifications which makes it a lot easier to debug | ||
| internal static class FragmentManagerExtensions |
There was a problem hiding this comment.
While this does make the debugging easier, it's also forcing an extra method call on every fragment transaction operation. You could probably achieve the same effect by decorating FragmentTransaction with another class and only using that decorator class when DEBUG is defined. That way you have a central place for debugging but you don't impact the Release version as much.
Something like this:
https://gist.github.com/hartez/a7824a102c595ca096fce8bdca1cec78
There was a problem hiding this comment.
I think the extra call get optimized out don't they?
I like the idea of the decorator because it also provides a useful place to expand on the debug implementation but I'm concerned about other anomalies of the wrapper or parts of it that can't be overridden like
public FragmentTransaction SetBreadCrumbShortTitle(string text);
public FragmentTransaction SetBreadCrumbTitle(string text);Though I'm pretty sure these just execute the ICharSequence Overrides
Either way I just envision some hard to find inconsistency between debug/release caused by this
There was a problem hiding this comment.
Either way I just envision some hard to find inconsistency between debug/release caused by this
That's a reasonable fear. I'm fine with the extension method overhead trade-off for the consistency.
samhouts
added
i/high
Description of Change
OnAppearing for Android fires sometimes during OnAttached and sometimes during "OnResume"
when you swapped out the MainPage it would call RemoveViews mid fragment lifecycle which would then lead to the fragment not found when FragmentManager would try to Resume the fragment
Added an interface to allow a renderer to be declared MarkedForDeath so that things like adding more fragments or pending operations can just be halted
Centralized on interactions with Fragments into a single Extension class because that just made it easier to break point or print debug messages for any Fragment modifications.
Moved all the code for changing out the main page into the MainLooper call so that all "active" life cycle events will just settle before the main page does it's clean up and "realizing"
Moved the code for when you swap out Details or Master pages it'll use MainLooper if replacing an existing page to ensure life cycle events have concluded
Issues Resolved
Platforms Affected
PR Checklist