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

Reduce overhead of pushing existing navigation stack #672

Merged
merged 7 commits into from Jan 23, 2017

Conversation

Projects
None yet
7 participants
@hartez
Member

hartez commented Jan 10, 2017

Description of Change

Removes the StackCopy member of NavigationPage and replaces it with an enumerable to avoid the Stack construction and Reverse operation overhead.

Bugs Fixed

  • None

API Changes

  • Removes StackCopy from INavigationPageController
  • Adds Page Peek(int depth);
  • Adds IEnumerable<Page> Pages { get; }

Behavioral 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
@jassmith

This comment has been minimized.

Show comment
Hide comment
@jassmith

jassmith Jan 10, 2017

Member

👍

Member

jassmith commented Jan 10, 2017

👍

@jassmith jassmith self-requested a review Jan 10, 2017

@jassmith

Looks good :)

@rmarinho

Looks good, worried about people that are using and relying on StackCopy .. we should state that in the release notes. But good to go!

Show outdated Hide outdated Xamarin.Forms.Core/INavigationPageController.cs
@@ -7,7 +7,8 @@ namespace Xamarin.Forms
{
public interface INavigationPageController
{
Stack<Page> StackCopy { get; }
Page SecondToLast { get; }

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Jan 11, 2017

Member

can't this be calculated from IEnumerable<Page> Pages {get;} ?

shouldn't it be internal ?

@StephaneDelcroix

StephaneDelcroix Jan 11, 2017

Member

can't this be calculated from IEnumerable<Page> Pages {get;} ?

shouldn't it be internal ?

@hartez hartez changed the title from Reduce overhead of pushing existing navigation stack to Reduce overhead of pushing existing navigation stack [DO NOT MERGE] Jan 11, 2017

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Jan 11, 2017

Member

I never liked SecondToLast; I have another idea I hate a lot less.

Member

hartez commented Jan 11, 2017

I never liked SecondToLast; I have another idea I hate a lot less.

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Jan 11, 2017

Member

@StephaneDelcroix I've nixed SecondToLast and replaced it with Page Peek(int depth), which is just as fast and also more generally useful.

Member

hartez commented Jan 11, 2017

@StephaneDelcroix I've nixed SecondToLast and replaced it with Page Peek(int depth), which is just as fast and also more generally useful.

@hartez hartez changed the title from Reduce overhead of pushing existing navigation stack [DO NOT MERGE] to Reduce overhead of pushing existing navigation stack Jan 11, 2017

Show outdated Hide outdated Xamarin.Forms.Core/NavigationPage.cs
}
return (Page)PageController.InternalChildren[PageController.InternalChildren.Count - depth - 1];
}

This comment has been minimized.

@djcparker

djcparker Jan 12, 2017

Peek(-1) would be a bad thing, right? Should probably return null if depth < 0?

@djcparker

djcparker Jan 12, 2017

Peek(-1) would be a bad thing, right? Should probably return null if depth < 0?

This comment has been minimized.

@hartez

hartez Jan 12, 2017

Member

Good point.

@hartez

hartez Jan 12, 2017

Member

Good point.

This comment has been minimized.

@hartez

hartez Jan 12, 2017

Member

Okay, Peek(-1) now returns null.

@hartez

hartez Jan 12, 2017

Member

Okay, Peek(-1) now returns null.

hartez added some commits Jan 10, 2017

@hartez hartez removed the waiting-tests label Jan 23, 2017

@hartez hartez merged commit 2c56d62 into master Jan 23, 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: 351, 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: 3704, 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: 34…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests passed: 345…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests passed: 347…
Details

@hartez hartez deleted the stackcopy branch May 16, 2017

@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