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

[Core] Added RootPage to NavigationPage #464

Merged
merged 4 commits into from Feb 2, 2017

Conversation

Projects
None yet
6 participants
@adrianknight89
Contributor

adrianknight89 commented Oct 16, 2016

Description of Change

NavigationPage has CurrentPage which points to the tail of the stack, but it lacks a pointer to the head. RootPage (a read-only property) should be used to get access to the first element of the stack. This makes it easier to look up the root and change it if needed via InsertPageBefore()

When changing root, I should be able to do something like:

np.InsertPageBefore(newPage, np.RootPage);
await np.PopToRootAsync();

Removing root should be even easier:

np.RemovePage(np.RootPage);

Also edited & added unit tests.

API Changes

Added:

  • static readonly BindablePropertyKey RootPagePropertyKey
  • public static readonly BindableProperty RootPageProperty

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
Show outdated Hide outdated Xamarin.Forms.Core/NavigationPage.cs
@@ -297,6 +306,9 @@ protected override bool OnBackButtonPressed()
void InsertPageBefore(Page page, Page before)
{
if (page == null || before == null)
throw new ArgumentException($"Both {nameof(page)} and {nameof(before)} must be initialized.");

This comment has been minimized.

@adrianknight89

adrianknight89 Oct 16, 2016

Contributor

Inserting or removing null values should be a valid list operation, but it doesn't apply to NavigationPage.

@adrianknight89

adrianknight89 Oct 16, 2016

Contributor

Inserting or removing null values should be a valid list operation, but it doesn't apply to NavigationPage.

Show outdated Hide outdated Xamarin.Forms.Core.UnitTests/NavigationUnitTest.cs
{
navPage.Navigation.RemovePage(null);
});
}

This comment has been minimized.

@adrianknight89

adrianknight89 Oct 16, 2016

Contributor

Surprised there were no test cases for these or I just missed them. This should cover everything (hopefully).

@adrianknight89

adrianknight89 Oct 16, 2016

Contributor

Surprised there were no test cases for these or I just missed them. This should cover everything (hopefully).

CurrentPage = page;
}
void RemovePage(Page page)
{
if (page == CurrentPage && ((INavigationPageController)this).StackDepth <= 1)

This comment has been minimized.

@adrianknight89

adrianknight89 Oct 16, 2016

Contributor

What if StackDepth was 0? I think < was unnecessary but perhaps erroneous at the same time.

@adrianknight89

adrianknight89 Oct 16, 2016

Contributor

What if StackDepth was 0? I think < was unnecessary but perhaps erroneous at the same time.

Show outdated Hide outdated Xamarin.Forms.Core/NavigationPage.cs
@@ -297,6 +306,9 @@ protected override bool OnBackButtonPressed()
void InsertPageBefore(Page page, Page before)
{
if (page == null || before == null)
throw new ArgumentException($"Both {nameof(page)} and {nameof(before)} must be initialized.");

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Oct 17, 2016

Member

I'm sure you heard about ArgumentNullException (). Or we can have a lengthy conversation about what "initialized" means in this context...

@StephaneDelcroix

StephaneDelcroix Oct 17, 2016

Member

I'm sure you heard about ArgumentNullException (). Or we can have a lengthy conversation about what "initialized" means in this context...

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Oct 17, 2016

Member

also, it's not required to check arguments on non-public APIs

@StephaneDelcroix

StephaneDelcroix Oct 17, 2016

Member

also, it's not required to check arguments on non-public APIs

This comment has been minimized.

@adrianknight89

adrianknight89 Oct 17, 2016

Contributor

Sorry, not sure how I missed that part. I think I was trying to be consistent with the existing throws. You mean it's not required to check for null on non-public APIs? Or it's not required to validate parameters at all? If it's the latter, InsertPageBefore and RemovePage were already checking validity.

@adrianknight89

adrianknight89 Oct 17, 2016

Contributor

Sorry, not sure how I missed that part. I think I was trying to be consistent with the existing throws. You mean it's not required to check for null on non-public APIs? Or it's not required to validate parameters at all? If it's the latter, InsertPageBefore and RemovePage were already checking validity.

This comment has been minimized.

@adrianknight89

adrianknight89 Oct 17, 2016

Contributor

http://stackoverflow.com/a/105021/812582. See Frank's comments. Perhaps it doesn't make sense to unit test all non-public APIs, but InsertPageBefore and RemovePage are very important to managing the navigation stack. In this particular case, I'd say both the tests and the null checks should be okay.

@adrianknight89

adrianknight89 Oct 17, 2016

Contributor

http://stackoverflow.com/a/105021/812582. See Frank's comments. Perhaps it doesn't make sense to unit test all non-public APIs, but InsertPageBefore and RemovePage are very important to managing the navigation stack. In this particular case, I'd say both the tests and the null checks should be okay.

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Oct 17, 2016

Member

there is no problem in unit testing or checking argument on private api, but it's not required, that's all

@StephaneDelcroix

StephaneDelcroix Oct 17, 2016

Member

there is no problem in unit testing or checking argument on private api, but it's not required, that's all

@hartez

Mostly looks good, just a couple of small issues.

Show outdated Hide outdated Xamarin.Forms.Core.UnitTests/NavigationUnitTest.cs
await nav.PushAsync (childRoot);
Assert.False (fired);
Assert.AreEqual (childRoot, nav.CurrentPage);
Assert.AreEqual(childRoot, nav.RootPage);

This comment has been minimized.

@hartez

hartez Jan 27, 2017

Member

Is there a reason you used AreSame for these checks above and AreEqual later on? Can we make them consistent?

@hartez

hartez Jan 27, 2017

Member

Is there a reason you used AreSame for these checks above and AreEqual later on? Can we make them consistent?

This comment has been minimized.

@adrianknight89

adrianknight89 Jan 28, 2017

Contributor

I can't remember why this was the case since it's been a while. I think I had copied the test from an existing one as a template.

@adrianknight89

adrianknight89 Jan 28, 2017

Contributor

I can't remember why this was the case since it's been a while. I think I had copied the test from an existing one as a template.

Show outdated Hide outdated Xamarin.Forms.Core/NavigationPage.cs
@@ -297,8 +306,11 @@ protected override bool OnBackButtonPressed()
void InsertPageBefore(Page page, Page before)
{
if (page == null || before == null)
throw new ArgumentNullException($"Neither {nameof(page)} nor {nameof(before)} can be null.");

This comment has been minimized.

@hartez

hartez Jan 27, 2017

Member

This should be two separate checks, so the user knows which of these two arguments was null.

@hartez

hartez Jan 27, 2017

Member

This should be two separate checks, so the user knows which of these two arguments was null.

Show outdated Hide outdated Xamarin.Forms.Core/NavigationPage.cs
var root = (Page)PageController.InternalChildren.First();
var childrenToRemove = PageController.InternalChildren.ToArray().Where(c => c != root);
var childrenToRemove = PageController.InternalChildren.ToArray().Where(c => c != RootPage);

This comment has been minimized.

@hartez

hartez Jan 27, 2017

Member

Rather than using Where(), which will have to check the predicate for every page in the collection, could we use something like var childrenToRemove = PageController.InternalChildren.Skip(1).ToArray();?

@hartez

hartez Jan 27, 2017

Member

Rather than using Where(), which will have to check the predicate for every page in the collection, could we use something like var childrenToRemove = PageController.InternalChildren.Skip(1).ToArray();?

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Jan 28, 2017

Contributor

Done.

Contributor

adrianknight89 commented Jan 28, 2017

Done.

@hartez

hartez approved these changes Jan 30, 2017

@hartez hartez self-assigned this Jan 30, 2017

@rmarinho rmarinho merged commit 070f1dc into xamarin:master Feb 2, 2017

6 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passed: 352, i…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3721, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests passed: 32
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests passed: 348…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests passed: 27
Details

@samhouts samhouts added D-15.4 and removed cla-already-signed labels Oct 10, 2017

@samhouts samhouts modified the milestones: 2.3.0, 2.3.5 Jun 27, 2018

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