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

[Android] Forward appearing / disappearing methods only for the last item on the stack #342

Merged
merged 2 commits into from Sep 30, 2016

Conversation

Projects
None yet
9 participants
@adrianknight89
Contributor

adrianknight89 commented Sep 10, 2016

Description of Change

When Android app was backgrounded or foregrounded, any page that implements IPageContainer was invoking appearing / disappearing methods for all pages on the stack. This change ensures that only the current page triggers its life cycle events. I'm not sure how MasterDetailPage behaves as I don't use it in my app, but this change should not break existing functionality.

In addition to this, I realized that if MainPage is set to a ContentPage, backgrounding / foregrounding the app does NOT raise appearing / disappearing events on the page. I will submit a bug in case this is a bug (not intended behavior) and needs to be fixed.

Bugs Fixed

Possibly fixes:

I did not read the bug descriptions in detail or run the repros, but fixing my own use case might fix those as well.

Comments

iOS seems to never raise page life cycle events when the app is put to sleep or resumed. Instead, developers should use OnSleep and OnResume events in the Application class and use simple pub/sub messages to deal with individual pages. Android is different as each (or the topmost) fragment needs to sync up with the activity life cycle events. Can you please confirm?

UPDATE:

Please see Coordinating with the activity lifecycle

The lifecycle of the activity in which the fragment lives directly affects the lifecycle of the fragment, such that each lifecycle callback for the activity results in a similar callback for each fragment. For example, when the activity receives onPause(), each fragment in the activity receives onPause().

While this mentions how Fragments should behave, it says nothing about Pages. If XF implements each Page as a Fragment, then there should be a design decision as to whether OnPause and OnResume should translate to OnDisappearing and OnAppearing with each page on the stack (which is the case with the latest version of XF). However, bug 41322 was CONFIRMED.

@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Sep 10, 2016

Hi @adrianknight89, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

dnfclas commented Sep 10, 2016

Hi @adrianknight89, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@amccorma

This comment has been minimized.

Show comment
Hide comment
@amccorma

amccorma Sep 12, 2016

Please make sure any changes to the OnAppearing and OnDisappear does not make these problems any worse. Prefer a fix to these before any future updates to this two methods.

Please make sure any changes to the OnAppearing and OnDisappear does not make these problems any worse. Prefer a fix to these before any future updates to this two methods.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Sep 12, 2016

Contributor

@amccorma I will take a look at your samples. In my opinion, iOS and Android lifecycle events work differently. On iOS, appearing / disappearing events should never be called when app state changes. On Android, these events are called. Please read my comment section above.

Contributor

adrianknight89 commented Sep 12, 2016

@amccorma I will take a look at your samples. In my opinion, iOS and Android lifecycle events work differently. On iOS, appearing / disappearing events should never be called when app state changes. On Android, these events are called. Please read my comment section above.

@amccorma

This comment has been minimized.

Show comment
Hide comment
@amccorma

amccorma Sep 12, 2016

Ok about working different. am ok with that. executing code on pages that have been removed from the NavigationStack is not ok. Executing the onAppearing event when the ContentPage is not on TOP of the stack not OK.

Ok about working different. am ok with that. executing code on pages that have been removed from the NavigationStack is not ok. Executing the onAppearing event when the ContentPage is not on TOP of the stack not OK.

@amccorma

This comment has been minimized.

Show comment
Hide comment
@amccorma

amccorma Sep 12, 2016

can make all the changes you want, but please do not break the existing bugs to make it worse. I can live with the difference but when code run on ContentPage that do not exist this is bad really bad stuff.

can make all the changes you want, but please do not break the existing bugs to make it worse. I can live with the difference but when code run on ContentPage that do not exist this is bad really bad stuff.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Sep 12, 2016

Contributor

@amccorma Please note that I do not work for Xamarin. I just needed to fix my own use case as a community member. It's up to Xamarin to decide what to merge to the master branch. Again, I will look at those bugs as well as they affect all of us.

Contributor

adrianknight89 commented Sep 12, 2016

@amccorma Please note that I do not work for Xamarin. I just needed to fix my own use case as a community member. It's up to Xamarin to decide what to merge to the master branch. Again, I will look at those bugs as well as they affect all of us.

@bentmar

This comment has been minimized.

Show comment
Hide comment
Contributor

bentmar commented Sep 19, 2016

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Sep 27, 2016

Member

@amccorma do you mind try this pr with your issues? we are still running tests on this.

Member

rmarinho commented Sep 27, 2016

@amccorma do you mind try this pr with your issues? we are still running tests on this.

@jassmith jassmith merged commit 353525e into xamarin:master Sep 30, 2016

@TheRealAdamKemp

This comment has been minimized.

Show comment
Hide comment
@TheRealAdamKemp

TheRealAdamKemp Sep 30, 2016

Way to go, Adrian!

Way to go, Adrian!

@amccorma

This comment has been minimized.

Show comment
Hide comment
@amccorma

amccorma Sep 30, 2016

how do I get the commit to test?

how do I get the commit to test?

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Sep 30, 2016

Contributor

@amccorma I'm not aware of a simpler process to do this (or generate unofficial nugets), but you could download a copy of the current master branch and compile the following dlls yourself:

Xamarin.Forms.Core
Xamarin.Forms.Platform
Xamarin.Forms.Xaml
Xamarin.Forms.Platform.Android

Add the first three to both PCL and Android projects. The last should only go in the Android project.

Contributor

adrianknight89 commented Sep 30, 2016

@amccorma I'm not aware of a simpler process to do this (or generate unofficial nugets), but you could download a copy of the current master branch and compile the following dlls yourself:

Xamarin.Forms.Core
Xamarin.Forms.Platform
Xamarin.Forms.Xaml
Xamarin.Forms.Platform.Android

Add the first three to both PCL and Android projects. The last should only go in the Android project.

@amccorma

This comment has been minimized.

Show comment
Hide comment
@amccorma

amccorma Sep 30, 2016

I download commit 353525e referenced above and build.

I download commit 353525e referenced above and build.

@amccorma

This comment has been minimized.

Show comment
Hide comment
@amccorma

amccorma Sep 30, 2016

where does the reference for InitializeComponents come from? It looks like it is external to Xamarin Forms for xaml?

where does the reference for InitializeComponents come from? It looks like it is external to Xamarin Forms for xaml?

@amccorma

This comment has been minimized.

Show comment
Hide comment
@amccorma

amccorma Sep 30, 2016

I ran it without the xaml support.

bug report results: https://bugzilla.xamarin.com/show_bug.cgi?id=44160

failed.

Run the sample on Android.
Page 1 will Appear, get a dialogbox saying Page1 Appear (sorry adrianknight89, know you hate the popup messages) - good result

send the app to the background. (switch to another app)
switch back to the sample app.
Messages:
Page1: Appear
Page1: Disappear (wrong. Page1 is visible)

The order wrong. Page1 should appear. Not appear and disappear. If you would like, I would accept Disappear and Appear. But please do not implement Appear and Disappear.

Failed, bug: 44160.

I stopped testing after the first ContentPage.

I ran it without the xaml support.

bug report results: https://bugzilla.xamarin.com/show_bug.cgi?id=44160

failed.

Run the sample on Android.
Page 1 will Appear, get a dialogbox saying Page1 Appear (sorry adrianknight89, know you hate the popup messages) - good result

send the app to the background. (switch to another app)
switch back to the sample app.
Messages:
Page1: Appear
Page1: Disappear (wrong. Page1 is visible)

The order wrong. Page1 should appear. Not appear and disappear. If you would like, I would accept Disappear and Appear. But please do not implement Appear and Disappear.

Failed, bug: 44160.

I stopped testing after the first ContentPage.

@amccorma

This comment has been minimized.

Show comment
Hide comment
@amccorma

amccorma Sep 30, 2016

bug: 44160 as others have said is related to Android. iOS works perfectly. Unsure about winPhone.

bug: 44160 as others have said is related to Android. iOS works perfectly. Unsure about winPhone.

@amccorma

This comment has been minimized.

Show comment
Hide comment
@amccorma

amccorma Sep 30, 2016

Thanks for everyone working on these bugs. 👍

Thanks for everyone working on these bugs. 👍

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Sep 30, 2016

Contributor

@amccorma In your case, TabbedPage requires more work. See #354. You'll need to wait until that's merged also or simply copy the changes and build your own assemblies.

This PR fixes a bug that caused all pages on the stack to fire their appearing / disappearing events.

Contributor

adrianknight89 commented Sep 30, 2016

@amccorma In your case, TabbedPage requires more work. See #354. You'll need to wait until that's merged also or simply copy the changes and build your own assemblies.

This PR fixes a bug that caused all pages on the stack to fire their appearing / disappearing events.

@amccorma

This comment has been minimized.

Show comment
Hide comment
@amccorma

amccorma Sep 30, 2016

I did not test the tabpage. I tested contentpage. This fix will make the bug worse as ContentPage fire Appearing than disappearing. The current bug is disappearring than Appearing. The current bug manageable but trapping the Android "paused" state. If Android state "paused" do not fire disappearing. By doing that it would fix part of the bug.

I did not test the tabpage. I tested contentpage. This fix will make the bug worse as ContentPage fire Appearing than disappearing. The current bug is disappearring than Appearing. The current bug manageable but trapping the Android "paused" state. If Android state "paused" do not fire disappearing. By doing that it would fix part of the bug.

@amccorma

This comment has been minimized.

Show comment
Hide comment
@amccorma

amccorma Sep 30, 2016

My hack for part ondisappearing on android for Contentpage

If "Android State Paused" = false then
Fire onDisappearing
End if

This resolves the bug for ContentPages.

My hack for part ondisappearing on android for Contentpage

If "Android State Paused" = false then
Fire onDisappearing
End if

This resolves the bug for ContentPages.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Sep 30, 2016

Contributor

When I said TabbedPage, I did not mean its children. I meant tabbed page as a whole (parent and children and the way parent affects children). I know you tested its children which were content pages, but wait for #354. Your use case is at least a two-part fix. So this PR alone won't fix it for you.

That said, you shouldn't update XF version (sounds like you have some workarounds) until #354 is released.

Contributor

adrianknight89 commented Sep 30, 2016

When I said TabbedPage, I did not mean its children. I meant tabbed page as a whole (parent and children and the way parent affects children). I know you tested its children which were content pages, but wait for #354. Your use case is at least a two-part fix. So this PR alone won't fix it for you.

That said, you shouldn't update XF version (sounds like you have some workarounds) until #354 is released.

@amccorma

This comment has been minimized.

Show comment
Hide comment
@amccorma

amccorma Oct 1, 2016

I tested a single contentpage. No tabbed container. #354 which I was asked to tested failed.

amccorma commented Oct 1, 2016

I tested a single contentpage. No tabbed container. #354 which I was asked to tested failed.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Oct 1, 2016

Contributor

@amccorma Scroll up and read my description of change. If you set MainPage to ContentPage, then appearing events don't work. I even created a bug for this: https://bugzilla.xamarin.com/show_bug.cgi?id=44855

You shouldn't be testing #354 alone. You should be testing #342 (this PR) + #354 if you use a tab container. If you use a single content page, you should be testing the fix for bug 44855.

Contributor

adrianknight89 commented Oct 1, 2016

@amccorma Scroll up and read my description of change. If you set MainPage to ContentPage, then appearing events don't work. I even created a bug for this: https://bugzilla.xamarin.com/show_bug.cgi?id=44855

You shouldn't be testing #354 alone. You should be testing #342 (this PR) + #354 if you use a tab container. If you use a single content page, you should be testing the fix for bug 44855.

@jamesl77

This comment has been minimized.

Show comment
Hide comment
@jamesl77

jamesl77 Oct 3, 2016

Sorry for the off-topic:
I am trying to test some changes from the PRs but I am unable to use the compiled assemblies, please see:
http://stackoverflow.com/questions/39839401/how-to-compile-xamarin-forms-from-source-and-use-the-assemblies

@adrianknight89 any idea?
Thanks!

jamesl77 commented Oct 3, 2016

Sorry for the off-topic:
I am trying to test some changes from the PRs but I am unable to use the compiled assemblies, please see:
http://stackoverflow.com/questions/39839401/how-to-compile-xamarin-forms-from-source-and-use-the-assemblies

@adrianknight89 any idea?
Thanks!

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Oct 3, 2016

Contributor

@jamesl77 I've run into that issue before, but I can't remember how I fixed it.

  • Try compiling your assemblies in Release mode (I've never done this honestly.)
  • I get 3 of those assemblies directly from the debug folder of Xamarin.Forms.Platform.Android. (I don't grab them from their own projects). Grab Xamarin.Forms.Xaml from the Xaml project.
  • Also make sure to rebuild solution.
Contributor

adrianknight89 commented Oct 3, 2016

@jamesl77 I've run into that issue before, but I can't remember how I fixed it.

  • Try compiling your assemblies in Release mode (I've never done this honestly.)
  • I get 3 of those assemblies directly from the debug folder of Xamarin.Forms.Platform.Android. (I don't grab them from their own projects). Grab Xamarin.Forms.Xaml from the Xaml project.
  • Also make sure to rebuild solution.
@jamesl77

This comment has been minimized.

Show comment
Hide comment
@jamesl77

jamesl77 Oct 4, 2016

@adrianknight89 thanks.
I need the debug assemblies so I can also debug myself. I wish there was info how to do this right on the readme page of the project.

Anyway, does anyone care to share how to do this?

This would be very helpful for anyone who wants to join the Xamarin Forms development

jamesl77 commented Oct 4, 2016

@adrianknight89 thanks.
I need the debug assemblies so I can also debug myself. I wish there was info how to do this right on the readme page of the project.

Anyway, does anyone care to share how to do this?

This would be very helpful for anyone who wants to join the Xamarin Forms development

@bentmar

This comment has been minimized.

Show comment
Hide comment
@bentmar

bentmar Oct 4, 2016

Contributor

@amccorma s bug is not really relatable to this PR i think. This only fixes so that appearing/disappering not fires on every page in navigationstack when toggling app

Contributor

bentmar commented Oct 4, 2016

@amccorma s bug is not really relatable to this PR i think. This only fixes so that appearing/disappering not fires on every page in navigationstack when toggling app

@samhouts samhouts added this to the 2.3.4 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment