Mvvmcross: NavigationService improvements

[martijn00]

The first version of a new navigation service should support:

• [x] Async await enabled
• [x] Return results
• [x] Type save
• [x] Deeplinking / URI navigation enabled
• [ ] CanNavigate check
• [x] Events on navigation steps

The aim for the second update would be

• [x] Bug fixes for first version
• [x] Unit Tests
• [ ] Nested routed navigation
• [x] Support for dialogs / modals
• [ ] Fully async await
• [x] Type safe all the way

Secondary requirements are:

• [ ] NavigationAware, OnNavigatedTo, OnNavigatedFrom
• [ ] Nested ViewModels / ChildViewModels
• [ ] ViewModel backstack
• [ ] Central Attributes for navigation patterns (#1671)
• [ ] Override navigation from specific view
• [ ] Animations
• [ ] Optional: View first navigation support too
• [x] Cancellation support

Other remarks

• [x] ShowViewModel stays for 100% compatibility
• [x] Testability
• [x] Support for Xamarin Forms

Proposal for new interface:
```c#
public interface IMvxNavigationService
{
Task Navigate() where TViewModel : IMvxViewModel;
Task Navigate(TParameter param) where TViewModel : IMvxViewModel;
Task Navigate(TParameter param) where TViewModel : IMvxViewModel;
Task Navigate() where TViewModel : IMvxViewModel;
Task Navigate(string path);
Task Navigate(string path, TParameter param);
Task Navigate(string path);
Task Navigate(string path, TParameter param);
Task CanNavigate() where TViewModel : IMvxViewModel;
Task CanNavigate(string path);
Task Close(IMvxViewModel viewModel);
}

public static class MvxNavigationExtensions
{
public static Task CanNavigate(this IMvxNavigationService navigationService, Uri path)
public static Task Navigate(this IMvxNavigationService navigationService, Uri path)
public static Task Navigate(this IMvxNavigationService navigationService, Uri path, TParameter param)
public static Task Navigate(this IMvxNavigationService navigationService, Uri path)
public static Task Navigate(this IMvxNavigationService navigationService, Uri path, TParameter param)
Task Close(this IMvxNavigationService navigationService)
}

The `Uri` navigation will build the navigation stack if required. This will also enable deeplinking and building up the navigationstack for it. Every ViewModel added to the stack can split up into multiple paths of it's own backstack. This will enable all kinds of layout structures as Hamburger, Tab or Top navigation.

Implement this on your ViewModel to support parameters and results. We can also add this to the existing `MvxViewModel` but that might break some stuff.
```c#
interface IMvxNavigationObject<TParameter>
Task Init (TParameter param);

interface IMvxNavigationObject<TParameter, TResult>
Task Init (TParameter param);
Task<TResult> OnComplete ();

interface IMvxNavigationObject<TResult>
Task<TResult> OnComplete ();
````

We need to add something to the `MvxViewModel`to indicate that the ViewModel is Closed and trigger it in the NavigationService.
- delegate OnComplete;

Example of result
```c#
var result = await navService.Navigate<SomeViewModel, SomeResult>();
if(result.Something)
DoSomething();

An alternative would be to use an Action which is called with the result.

How the result might work in the NavigationService. This needs to be investigated.
```c#
Task Navigate()
{
TResult result;
var viewmodel = eargaera;
viewmodel.OnComplete = (r) => {
result = r;
}
await viewmodel.Init();

return yield result;

}

Implementation on ViewModel which will be triggered when `Close();` is called on viewmodel.
```c#
Task<TResult> OnComplete ()
{
    return TResult;

}

The presenters should also be organized to support all of this. The proposal is the make 1 new presenter per platform, and let other presenters inherit it. This new presenter will be the default.

[kjeremy]

I think this looks really great and I love how you spelled out the interface. A few questions:

1. Is this more about 'page' based layouts or would it be easy to handle multiple view/viewmodels in one screen (fly out panels, master/detail, difference between phone/tablet etc)? I'm assuming the latter based on the individual back stack support and child ViewModels. This really excites me.

2. I've wrestled with Close() in a few projects now on Android and sometimes this sort of thing needs to happen in OnDestroyView and other times in OnDestroy etc. How do we plan to handle the ambiguity of when to call Close and the difference between when MvvmCross thinks a MvxViewModel is closed and when the platform may kill a view? Do we provide a default and then allow the user to specify? Maybe we shouldn't even have a guarantee that this happens automatically?

3. Should we provide cancellation support? In most cases (moving between pages) this probably doesn't make a lot of sense but I could see a case for dialogs or where a user is suddenly logged out and has to go back to a login screen for example.

[martijn00]

Thanks!

1. Yes, this is especially for multiple views/viewmodels on a screen. But off course 'page' based would work with that as well.

2. I see your concern. I don't really know yet, so i guess we have to find out in development.

3. Hmm, interesting one. That might be a good idea.

[thefex]

Looks good, just some implementation details

1. Remove unnecessary interface methods, create extension methods, example:
   Task Navigate(Uri path);
Task Navigate(string path);

We don't need method declaration with both Uri/string as interface method parameter. Instead replace one of that with Extension method - interface implementation will be easier.

1. Instead of returning nothing or returning TResult in NavigateTo<> lets wrap it into LayerResponse class (that should go to MvvmCross as it enforce null-safe coding, let's finally forget that null exists).

public class LayerResponse
    {
        private bool _forceFailedResponse;

        public LayerResponse()
        {
            ErrorMessages = new List<string>();
        }

        [JsonIgnore]
        public bool IsSuccess => !ErrorMessages.Any() && !_forceFailedResponse;

        [JsonProperty("errorMessages")]
        public IList<string> ErrorMessages { get; set; }

        [JsonIgnore]
        public string FormattedErrorMessages => ErrorMessages.Any()
            ? ErrorMessages.Aggregate((prev, current) => prev + Environment.NewLine + current)
            : string.Empty;


        public LayerResponse AddErrorMessage(string errorMsg)
        {
            ErrorMessages.Add(errorMsg);
            return this;
        }

        public LayerResponse SetAsFailureResponse()
        {
            _forceFailedResponse = true;
            return this;
        }
    }

    public sealed class LayerResponse<TResult> : Response
    {
        public LayerResponse(TResult results)
        {
            Results = results;
        }

        public LayerResponse()
        {
        }

        public TResult Results { get; }

        public new LayerResponse<TResult> AddErrorMessage(string errorMsg)
        {
            base.AddErrorMessage(errorMsg);
            return this;
        }

        public new LayerResponse<TResult> SetAsFailureResponse()
        {
            base.SetAsFailureResponse();
            return this;
        }
    }

        public static class LayerResponseExtensions
    {
        public static void Handle(this LayerResponse response, Action onSuccess, Action<string> onFailure)
        {
            if (response.IsSuccess)
                onSuccess();
            else
                onFailure(response.FormattedErrorMessages);
        }

        public static async Task Handle(this LayerResponse response, Func<Task> onSuccess, Action<string> onFailure)
        {
            if (response.IsSuccess)
                await onSuccess().ConfigureAwait(false);
            else
                onFailure(response.FormattedErrorMessages);
        }

        public static async Task Handle(this LayerResponse response, Func<Task> onSuccess, Func<string, Task> onFailure)
        {
            if (response.IsSuccess)
                await onSuccess().ConfigureAwait(false);
            else
                await onFailure(response.FormattedErrorMessages).ConfigureAwait(false);
        }

        public static LayerResponse CloneFailedResponse(this LayerResponse response)
        {
            var clonedResponse = new Response();
                        if (response.IsSuccess)
                throw new InvalidOperationException("LayerResponse finished with success.");

            foreach (var error in response.ErrorMessages)
                clonedResponse.AddErrorMessage(error);

            return clonedResponse;
        }

        public static LayerResponse<TResult> CloneFailedResponse<TResult>(this LayerResponse response)
        {
            if (response.IsSuccess)
                throw new InvalidOperationException("LayerResponse finished with success.");

            var clonedResponse = new LayerResponse<TResult>();

            foreach (var error in response.ErrorMessages)
                clonedResponse.AddErrorMessage(error);

            return clonedResponse;
        }
    }

Response-Based NavigateTo method would give "cancellation" support - in case Init of ViewModel failed (for ex. loading of initial list from REST failed) - we could automatically close that viewmodel (and return failed response further to library-client with error message why it has failed).

1. Add compile time validation of NavigateTo<TViewModel>, NavigateTo<TViewModel, TParam> - perhaps by generic interface constraint (for ex. we can navigate to ViewModel with parameter if it implements IMvxNavigatingObject<TParam>)

[softlion]

I don't like your error interfaces. I prefer a switch to Serilog.

[Cheesebaron]

I don't like carrots, please use cauliflowers...

[kjeremy]

@thefex can you give an example of how a client would use the code from your point 2? LayerResponse looks awkward (and I know I'm bikeshedding but that's a very generic name).

[thefex]

@kjeremy I would see LayerResponse as return type for instance in MvvmCross Plugins instead of ugly null returned in case of failure.

That is simple, ViewModel side:

public async Task<Response> Init (string id) {
   Response<ApiDataModel> apiDataResponse = await LoadApiData(id);
   if (apiDataResponse.IsSuccess)
      HandeApiData(apiDataResponse.Result);
   return apiDataResponse;
}

Navigation Service side:

public Task<Response> Navigate<TViewModel>()
{
    var viewModelInitResponse = await viewmodel.Init();
    if (!viewModelInitResponse.IsSuccess)
           viewModel.Close() <- would be awesome if it was awaitable too

        return viewModelInitResponse;
}

ViewModel which calls navigate to side

public MvxCommand SomeMvxCommand => new MvxCommand( () => {
   var navigationResponse = await NavigationService.NavigateTo<SomeViewModel>(someViewModelIdString);
   navigationResponse.Handle(() => { }, errorMsg => DisplayErrorDialog(title: UserInterfaceResources.ApiDataLoadFailure, errorMsg);
});

That's a simple use-case of Response based return type.

[mvanbeusekom]

I really like the suggestions by @thefex, I think we should really consider them. We can discuss the implementation of the Response object, but I see added value in being able to communicate errors in a clear way.

Also the reasoning regarding the extension methods is really solid. Lets keep implementation as simple as possible and add extensions when needed.

[Cheesebaron]

In the case of the MvxCommand. To me it looks very awkward to have to call Handle to get the error message or a success. Since we've already awaited a Task right before.

I am also not a big fan of callbacks as people tend to abuse them or nest code unnecessarily when using them."

I am not against the proposal of having a return value with errors etc.

[kjeremy]

Hm. It's interesting but I'm not convinced. Callbacks have a way of nesting. I'd rather have something more explicit that the developer is forced to deal with. Personally if Init fails I'd like to have the option to throw an exception that's caught by the navigation service, undoes the transaction (ie removes the view) and rethrows so that I can handle any errors at the call site. It's entirely to easy to forget to check return values, especially when you have to check a flag AND a value.

[thefex]

Callbacks are just extension methods. You can use "IsSuccess" / "Result" and "ErrorMessages" properties.
If we agree that Init should be used to Initialize/Load ViewModel (like - do a rest request) then Init failure won't be an exceptional case. Therefore @kjeremy we will use exceptions to control app flow - in my opinion it's gonna be an architecture mistake.

It's entirely too easy to forget to check exception as well :)

[mvanbeusekom]

I agree that exceptions are just as easy if not easier to forget (especially when not documented). The developer has no reference when or which exception is thrown until it is to late. When we return an response object, at least the dev can examine it's properties / methods.

[kjeremy]

That's definitely true... but then your program dies horribly in a :fire: so you at least know about it. If you forget an exception handler you're screwed. I like my code to die as quickly and horribly as it can so I can catch my mistakes but I know that a lot of people don't like that. I think it comes from my C++ days when unwinding the stack was more useful. Regardless I still want the option for try/catch/finally. It makes cleanup easier for complicated interactions.

[softlion]

I'm against this proposal. It is not the responsibility of the navigation service to handle errors in viewmodel creation.

Btw it is the responsibility of the viewmodel's init method to handle its own errors. There is no need for an infrastructure to transfer errors to the caller.

And if the viewmodel's init method is a sync, it must not wait for answers from webservices as it will freeze the UI by preventing the view from displaying.

It is the responsibility of the init method to display a waiting data state instead of delaying the transition. If an error occurs it is the responsibility of the init method to choose what to do. Not the caller responsibility.

[azzlack]

I agree that it is not the navigation service responsibility to handler viewmodel init errors. But it is definitely useful to have a callback so you can handle different scenarios.
However, that callback does not need to say anything about errors, it just needs to transport an object of the devs own choosing. The navigation service does not need to know if it is an error or not, the dev can handle that part in the callback.

[softlion]

This won't work either, as a parent viewmodel can be removed from the stack without notice. For example if the opened vm closes 1 whole stack hierarchy. Like in an onboarding process. Where this result would go ?

You have 2 usage scenarii: a common one where you know a vm will go back to its parent. This vm could return a result, but not a result of a failed initialization, a result of its own depending of what it does.
Second scenarii is where a vm is the end of a stream of vm and closes this stream and go back to a 'root' vm..

[Nickolas-]

@thefex I think it's gonna be an architecture mistake to return result of action on another viewModel (if viewModel not returning anything like StartActivityForResult) . If you showing some viewModel, you need handle errors and other exception on other viewModel on what exception triggered, not on the viewModel that presented it. It will cause to create GOD classes can handle everything.

About Task<TResult> Navigate<TViewModel, TParameter, TResult>(TParameter param) where TViewModel : IMvxViewModel; this good when you want something like StartActivityForResult or ShowViewModel that will give us result of presented ViewModel and we can remove MxMessenger that helps better understand application flow , I really like it.

[softlion]

About my scenarii. For the 1st it's a StartActivityForResult like method as Nickolas explained. For the 2nd it's a MxvMessenger. Note that usage of MxvMessenger subscriptions requires that viewmodel are forced disposed in the app presenter. Without it, i found lots of cases where subscriptions still receive the messages but there is no more view to receive them. And sometime the view binding handlers are not even removed which leads to crashs.

[MKuckert]

Does Task<TResult> Navigate<TViewModel, TResult>() enforce any type constraints on TResult? Like passing presentation parameters to ShowViewModel where each property is limited to simply types like int and string so it can be passed as Parcelable in Android or URI on Windows Platforms.
And how does returning the result work if a calling Activity is stopped and restartet?

[martijn00]

There should be no type constraints on TResult. I don't think we have taken in account situations where activity is stopped or restarted yet. We need to find out and improve on it during development.

[Cheesebaron]

I added Bug fixes and Unit Tests as requirements for 2nd version.

[ferrydeboer]

Because of issue #2080 I've started writing some more unit tests for the NavigationService.

[Rajkumarcs41]

Hi @martijn00

How we access Viewmodel instance in NavigationService?

Task<TResult> Navigate<TViewModel, TResult>()
{
    TResult result;
    **var viewmodel = eargaera;**
    viewmodel.OnComplete = (r) => {
        result = r;
    }   
    await viewmodel.Init();

    return yield result;
}

In above code sample you have set viewmodel =eargaera . But i don't know how to get viewmodel instance in nav service. Could you please guide me on this

[nickrandolph]

@Rajkumarcs41 if you download the source code and run the playground sample you should be able to step through the navigation workflow. In doing this you'll see how the ViewModel is created and how you can access it.

[GiuseppeMarraffa]

Does the navigation service work also for other platforms? Our requirement is that our mobile app has to be ported also to a web application and later also on windows as windows application. If I use the mvvmcross navigation service in a viewmodel behind SaveCustomerClick event of a button, that returns to the previous view --> This would work without doubts onAndroid and iOS as shown on the examples, does it also navigate through WPF pages? And also through web pages? This is very crucial for us, becaus ethe viewmodel is our last common layer, after that, we have to be able to port the viewmodel to any UI technology (WPF etc..). If we could also embed the navigation in the viewmodel, it would be perfct, because we would write the application only once! Are there settings during the registration of the navigation service that I could do to tell mvvmcross: this i a WPFNavigationService... or StandardNavigationService (for Mobile) etc...
We did a similar implementation on our own that supports navigation for webpages and WInForms...
It would be great if mvvmcross hasthe abilty to navigate through other UI technologies...

Thx!