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] NavigationPageRenderer should not create a Fragment unnecessarily #627

Merged
merged 6 commits into from Jan 12, 2017

Conversation

Projects
None yet
6 participants
@adrianknight89
Contributor

adrianknight89 commented Dec 9, 2016

Description of Change

NavigationPageRenderer is creating a new Fragment every time we push, pop, or pop to root. While it makes sense to do this in the first case, it doesn't in other cases. Instead, we can re-use existing fragments from _fragmentStack. Not sure what this means for performance, but there is no need to instantiate a Fragment object unnecessarily.

Also removed two local variables.

Bugs Fixed

  • N/A

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
{
// pop
if (removed)
return _fragmentStack[_fragmentStack.Count - 2];

This comment has been minimized.

@djcparker

djcparker Dec 9, 2016

do we need to defend against count = 0? ie: negative index

@djcparker

djcparker Dec 9, 2016

do we need to defend against count = 0? ie: negative index

This comment has been minimized.

@adrianknight89

adrianknight89 Dec 9, 2016

Contributor

Negative index would mean we're trying to pop the root and I don't think that's allowed. If we ever end up with a negative index, it's better to let it throw an exception so the issue can be fixed somewhere else.

@adrianknight89

adrianknight89 Dec 9, 2016

Contributor

Negative index would mean we're trying to pop the root and I don't think that's allowed. If we ever end up with a negative index, it's better to let it throw an exception so the issue can be fixed somewhere else.

This comment has been minimized.

@adrianknight89

adrianknight89 Jun 12, 2018

Contributor

@rmarinho With regards to #3021, I remembered having discussed this above.

@adrianknight89

adrianknight89 Jun 12, 2018

Contributor

@rmarinho With regards to #3021, I remembered having discussed this above.

@hartez

hartez approved these changes Dec 27, 2016

@hartez hartez self-assigned this Dec 30, 2016

@rmarinho rmarinho merged commit 20ed60f into xamarin:master Jan 12, 2017

3 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: 349, 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: 3585, ignored: 10
Details

@samhouts samhouts added this to Done in vNext+1 (master) Jun 26, 2018

@samhouts samhouts removed this from Done in vNext+1 (master) Jun 26, 2018

@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