Skip to content
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

[iOS] Change when we updated the XF INavigationPageController #291

Merged
merged 1 commit into from Aug 16, 2016

Conversation

@rmarinho
Copy link
Member

commented Aug 10, 2016

Description of Change

Our problem is the way we were updating the XF navigation stack, when a page was removed (popped) from the NavigationController via a native gesture (tap back button, or swipe from side), we were relying that the override PopViewController will be called the correct amount of times, but that's not true. If the user presses the back button very fast, PopViewController will suppress to be called if animation is still happening, the code that was trying to fix this wasn't working as it should for consecutive back button calls, usually more then 5/6 it would fail, resulting in pages being still on the NavigationStack.
To be more sure about when the page is Popped we use the override DidMoveToParentViewController , this can tell us when a VC is detach from is parent NavigationController, when we detect we are being removed we need to update the NacigationPage stack, the only exception here is if the used called RemovePage, where we pop it manually first from the navigation stack so we ignore when this happens on our method.
UITest didn't work because tapping back was to slow.

Removed not used projects

There were some style fixes also.

Bugs Fixed

https://bugzilla.xamarin.com/show_bug.cgi?id=39908
https://bugzilla.xamarin.com/show_bug.cgi?id=42341

API Changes

None

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

App.SetOrientationPortrait ();
ScreenBounds = App.RootViewRect ();
NavigateToGallery ();

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Aug 11, 2016

Member

could you avoid style "fixes", it greatly reduce the SNR and makes the PR harder to review. thanks

This comment has been minimized.

Copy link
@rmarinho

rmarinho Aug 11, 2016

Author Member

this is a little more that just style fixes as i removed the projects and cleaned up the code.. But yeah could be a different PR

@@ -799,6 +747,12 @@ public Page Child
}
}

public bool IgnorePageBeingRemoved

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Aug 11, 2016

Member

this looks similar to _ignorePopCall

This comment has been minimized.

Copy link
@rmarinho

rmarinho Aug 11, 2016

Author Member

it's a different class, this now belongs to the ParentViewController

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

lot of noise, looks like a hack (the previous implementation was a hack too :) ). Need @jassmith to review this

@@ -1,22 +0,0 @@
using NUnit.Framework;

This comment has been minimized.

Copy link
@rmarinho

rmarinho Aug 11, 2016

Author Member

this isn't important

@@ -1,29 +0,0 @@
using System;

This comment has been minimized.

Copy link
@rmarinho

rmarinho Aug 11, 2016

Author Member

this is also not important

@@ -16,22 +16,22 @@ internal abstract class BaseTestFixture
{

This comment has been minimized.

Copy link
@rmarinho

rmarinho Aug 11, 2016

Author Member

Not important only style changes

[iOS] Change when we updated the XF INavigationPageController after p…
…opping a page natively, Cleanup UITest references

@rmarinho rmarinho force-pushed the fix-39908 branch from 2fed8c5 to 0b2adb2 Aug 16, 2016

@jassmith

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2016

This looks good, removes an old and terrible hack. 👍

@jassmith jassmith merged commit 52dc625 into master Aug 16, 2016

@rmarinho rmarinho deleted the fix-39908 branch Oct 7, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.