Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[UWP] Make Navigation and Transition overridable for NavigationPage #12439

Merged
merged 2 commits into from
Nov 26, 2020

Conversation

GiampaoloGabba
Copy link
Contributor

@GiampaoloGabba GiampaoloGabba commented Oct 10, 2020

Description of Change

The NavigationRenderer for UWP doesn't expose any virtual method to override, closing a lot of possibilities expecially for plugin authors.

This PR comes after a conversation on Twitter with @PureWeen and adds the protected virtual access level to the following, existing, methods:

OnPopRequested
OnPopToRootRequested
OnPushRequested
OnElementChanged

Bringing the UWP NavigationRenderer in par with iOS & Android.

Plus i created a new method SetupPageTransition to override the default transition (but making sure to maintain the _transition cache for the default transition).

API Changes

Added:

  • void SetupPageTransition(Transition transition, bool isAnimated, bool isPopping)
    isPopping is needed to create a custom transition based on push or pop

Changed:

  • protected virtual void OnPopRequested(object sender, NavigationRequestedEventArgs e)
  • protected virtual void OnPopToRootRequested(object sender, NavigationRequestedEventArgs e)
  • protected virtual void OnPushRequested(object sender, NavigationRequestedEventArgs e)

Platforms Affected

  • UWP

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

  • Open the ControlGallery app
  • Click on "Test Cases"
  • Search for "Navigationpage override"
  • Tap it, then tap on "Open NavPage with overrides" and enjoy a custom navigation transition between pages in the modal stack :)

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@GiampaoloGabba
Copy link
Contributor Author

do you think we can have this for 5.0? I really want to finish UWP sharedtransitions so i can ask the toolkit if they want them when everything is done :)

@Redth Redth requested a review from PureWeen October 27, 2020 13:52
@jsuarezruiz
Copy link
Contributor

@GiampaoloGabba Yes, SharedTransitions will be a very nice addition to the Toolkit.

@rmarinho rmarinho added this to To do in vNext+1 (5.0.0) via automation Oct 30, 2020
@rmarinho rmarinho moved this from To do to In Review in vNext+1 (5.0.0) Oct 30, 2020
@rmarinho rmarinho added this to the 5.0.0 milestone Oct 30, 2020
@rmarinho rmarinho added the blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. label Nov 5, 2020
Copy link
Contributor

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GiampaoloGabba is this true?

Bringing the UWP NavigationRenderer in par with iOS & Android.

Looking through the other platforms it seems like only the Shell renderer exposes these as virtual but maybe I'm missing something obvious here?

@PureWeen PureWeen added the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Nov 13, 2020
@GiampaoloGabba
Copy link
Contributor Author

Hi @PureWeen
i used some poor wording in my pr description.

What i meant was to have in UWP the ability to override the navigation behaviours in a "similar" way like we do in iOS & Android.
Ofc there are some differences that comes from the platforms, but with this PR we should be able to do roughly the same things in UWP like Android & iOS.

Here a brief description of what we have in iOS & Android.

iOS

we can override these methods from the renderer:

OnElementChanged
OnPopToRoot
OnPopViewAsync
OnPushAsync

And we can even override methods from the UINavigationController:

PushViewController
PopViewController

Regarding the transition, we can customize it by overriding:

IUIViewControllerAnimatedTransitioning GetAnimationControllerForOperation

Android

Similar to iOS, the Android renderer exposes these virtual members:

OnElementChanged
OnPopToRootAsync
OnPopViewAsync
OnPushAsync

We can customize the transition by overriding this method:

SetupPageTransition

UWP with my PR

This PR adds the following virtual methods:

OnElementChanged
OnPopToRootRequested
OnPopRequested
OnPushRequested

Then there is the SetupPageTransition virtual method, (wich contains all the transition related stuff), so we can customize it like in iOS & Android.

So in the end we have 5 virtual methods like Android & iOS (even tough in iOS the transition management is a little different, because we override a UIViewController method, and not the renderer's one).

@PureWeen
Copy link
Contributor

@GiampaoloGabba do you by chance know if we can match the behavior on iOS/Android with how it uses Tasks?

https://github.com/xamarin/Xamarin.Forms/blob/5.0.0/Xamarin.Forms.Platform.iOS/Renderers/NavigationRenderer.cs#L596

I'm pretty sure those tasks are designed to resolve once an animation completes and we've navigated.
I briefly looked around at ways to do this but it doesn't seem super straight forward :-/

@GiampaoloGabba
Copy link
Contributor Author

@PureWeen Sincerely i dont know. The last time i tried to "play" with task and Navigation in UWP i gave up, i was getting a lot of random MemoryAccess violation that drove me crazy.... (expecially when navigation fast between pages).

It would be cool to have Tasks here like in iOS & Android but i'm a bit "scared", given my past on the subject.
Buuut, i'm a beginner in UWP so maybe i'm terribly wrong :)

@PureWeen
Copy link
Contributor

@GiampaoloGabba yea :-/ if there's a way I haven't found it. Best I can figure is to some how interpret the LayoutChanged event to determine once it's done but that just seems excessive

Just to narrow down added APIs from this PR could we not open up these APIs?

OnPopRequested
OnPopToRootRequested
OnPushRequested

What's the use case for these ones? I realize it makes it more consistent but just curious the use case to make sure just adding virtual to those serves the right purpose :-)

@GiampaoloGabba
Copy link
Contributor Author

@PureWeen to be honest they are here primarily for consistence with other platforms.

But they can be used to semplify our code (for example if i need to do something only on push, i can use the OnPushRequested override instead to use SetPage and check everytime if the NavigationRequest is a push).

Plus, during a PopRequest, if we use only SetPage we loose the NavigationRequestedEventArgs Page property (because we pass Element.Peek(1) to SetPage instead of e.Page like in PushRequested and OnPopToRootRequested

@PureWeen
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@PureWeen PureWeen removed the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Nov 25, 2020
@PureWeen PureWeen merged commit e1aa4d3 into xamarin:5.0.0 Nov 26, 2020
vNext+1 (5.0.0) automation moved this from In Review to Done Nov 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. ControlGallery p/UWP
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants