Prism: [XF] INavigationAware additions with OnNavigatingFrom & OnNavigatingTo

[tboogh]

When using INavigationAware the navigation service call to OnNavigatedTo happens after the view is visible. Bindings updated with data from the parameters result in data changes being visible to the user. I've added OnNavigatingTo & OnNavigatingFrom which are called before the view is presented to solve this. This mimics the ViewWillAppear/ViewDidAppear pattern from iOS. Sounds interesting?

[brianlagunas]

So you're suggesting moving the call to OnNavigatedTo before the page is actually navigated to? Essentially moving the call from line 353 to 347?

https://github.com/PrismLibrary/Prism/blob/master/Source/Xamarin/Prism.Forms/Navigation/PageNavigationService.cs#L353

[dansiegel]

I'm not opposed to mimicking the ViewWillAppear/ViewDidAppear, but I would rather see a breaking change in which INavigationAware gets something like WillNavigateTo that could be called on line 347 with OnNavigatedTo remaining where it is.

[powerdude]

before introducing a breaking change, i'd suggest adding an IBeforeNavigationAware interface with OnNavigatingTo and OnNavigatingFrom (don't like the 'Will').

[brianlagunas]

I like the idea of an additional interface for handling the OnNavigatingTo and OnNavigatingFrom. As for the name, this will be tricky. Naming things are always the most difficult part of an API :)

[brianlagunas]

How about INavigatingAware?

[powerdude]

that works!

[tboogh]

I tested adding an interface named INavigatingAware and added OnNavigatingFrom and OnNavigatingTo at line 52 to handle going back and at 345 for forward navigation. Seems to work fine, at least with a NavigationPage as root.

[joacar]

Ohh API-naming my favorite topic ;)

INavigatingAware is a good name but introducing it makes INavigationAware a less ideal bad name since it all of a sudden became INavigatedAware.

Thus INavigationAware should become

public interface INavgationAware : INavigatingAware, INavigatedAware
{
}

where INavigatedAware is the "old" INavigationAware

But I realize this a breaking change and that is what we want to avoid.

[brianlagunas]

I like what you have, but as you said, this would be a major breaking change.

[dansiegel]

Perhaps we could just swap things around to keep it an opt in feature for existing code bases. Though I agree with @joacar that if we don't make it breaking, we do get some less than ideal names.

public interface INavigatingAware : INavigationAware, INavigatedAware

[tboogh]

How about adding INavigatingAware and INavigatedAware while leaving INavigationAware as is for now until a major version change? Not pretty but it introduces the interfaces and doesn't break anything for now.

[joacar]

I like that idea. That opens upp for marking it obsolete in the next release and in the release after that (or two versions don't know what the usual time frame is when obsolete is "full-filled") implement it as public interface INavgationAware : INavigatingAware, INavigatedAware (if desired)

[brianlagunas]

Actually, I just remembered a great explanation of possible issues with adding something like this on #704. Essentially, the new view hasn't been added to the navigation stack when OnNavigatingTo would be called. What happens if the code in OnNavigatingTo causes a lag or delay. The app may not behave as you would expect and give a poor end-user experience. Thoughts?

[tboogh]

I completely agree with the points made in the discussion in that issue, making the methods async would be a very bad idea. The problem is that currently if information that is reciding in memory is passed along in OnNavigatedTo the animation is awaited before the viewmodel is updated. So the viewmodel is created with default values, page is created, push animation plays, data is updated and then the bindings are updated.

Here it is illustrated in our current project:

[dvorn]

@brianlagunas This one is different from #704. Here the methods are synchronous and it is the developer's responsibility to implement them as fast as possible to create smooth end-user experience. This is the same as all *ing and *ed events in Windows.

In #704, the framework defines the method as asynchronous and thus explicitly allowed it to be long-running, bringing the responsibility for smooth user experience to the framework.

[brianlagunas]

One more question for everyone. What would be the benefit of OnNavigatingFrom? I see no usefulness in having that particular method. It simply gets called right before OnNavigatedFrom, with the exact same information. I'm not sure this method makes sense to add.

I see the usefulness of OnNavigatingTo, and even in the code it is actually called prior to navigating to a Page.

Thoughts?

[tboogh]

You're right. I think it's out of habit from other frameworks (UIKit mainly)

[powerdude]

I think it would be of value if you allowed the possibility of rejecting navigation. So OnNavigatingFrom would return a boolean that could be checked to decide to continue to leave the page

[tboogh]

Isn't that already handled by IConfirmNavigation?

[brianlagunas]

Now the million dollar question. Should we introduce a breaking change and add this directly to the INavigationAware, or create a separate interface and rely on that. I think that OnNavigatingTo will be used more often for the reasons stated in this thread. It s also early in the life of Prism for XF. Lukcily the break is easy to fix because devs can easily re-implement the interface and they are compiling again.

FYI, there will already be a breaking change in DelegateCommand because we are removing the FromAsyncHandler in Prism 6.3.

Thoughts?

[brianlagunas]

To be more specific, take @joacar suggestion and make it like this:

public interface INavgationAware : INavigatingAware, INavigatedAware
{
}

[powerdude]

@tboogh , yes, but for consistency sake, and if we're introducing INavigatingAware, i'd say it kind of belongs there.

[powerdude]

Also, OnNavigatingTo could also return a bool in order to reject navigation to the page.

[brianlagunas]

@powerdude we will not be adding this functionality to any of the navigation methods. IConfirmNavigation exists for this reason.

[brianlagunas]

Okay guys... check out PR #796. This approach makes it a breaking change. Let me know what you think. If we decide that a breaking change should be avoided, then I can modify the PR.

[tboogh]

Tested it and it worked great, thanks!

[brianlagunas]

So I am really struggling on if I should make this a breaking change or not. Any comments?

[joacar]

As you mentioned earlier, there will already be a breaking change due to FromAsync DelegateCommand.FromAsyncHandler (can't recall exact) so I vote for introducing this along with that. Probably the above will have less impact on existing Forms-project in contrast to this.

[brianlagunas]

No, there will be a breaking change in DelegateCommand because I will be removing the FromAsyncHandler mess.

[tboogh]

I think you should go ahead and make this a breaking change. I've been using the updated interfaces and it adds a missing piece to the navigation in prism for XF.

[brianlagunas]

Well, since this will be breaking, should I go through the trouble of creating separate interfaces or should I just add OnNavigatingTo to the current INavigationAware interface?

[djcparker]

As it is a breaking change, I would add it to the current INavigationAware interface. Add an example to the release notes, good to go.

Personally, I always read the release notes before updating key packages.

[msangtarash]

I see OnNavigatingTo, but I wasn't able to find OnNavigatingFrom

[dansiegel]

@msangtarash there’s no need for an OnNavigatingFrom. That event is sufficiently covered by IConfirmNavigation & IConfirmNavigationAsync

[PenguinPero]

@dansiegel what if we want to intercept the navigation back button? Is there anything we could do there beside custom renderers..

For hardware back button we can create a base ContenPage and call some method on our BaseVM inside OnBackButtonPressed and handle navigation and confirmation

But when pressing the navigation back button the CanNavigate or CanNavigateAsync methods are not called, only OnNavigatedFrom is called (which also might not be useful if we want to handle navigation on the page before on the stack with OnNavigatingTo, not with OnNavigatedTo)

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.