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

NavigationService improvements #1634

Closed
martijn00 opened this Issue Feb 28, 2017 · 25 comments

Comments

@martijn00
Copy link
Member

martijn00 commented Feb 28, 2017

The first version of a new navigation service should support:

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

The aim for the second update would be

  • Bug fixes for first version
  • Unit Tests
  • Nested routed navigation
  • Support for dialogs / modals
  • Fully async await
  • 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
  • Cancellation support

Other remarks

  • ShowViewModel stays for 100% compatibility
  • Testability
  • Support for Xamarin Forms

Proposal for new interface:

public interface IMvxNavigationService
{
Task Navigate<TViewModel>() where TViewModel : IMvxViewModel;
Task Navigate<TViewModel, TParameter>(TParameter param) where TViewModel : IMvxViewModel;
Task<TResult> Navigate<TViewModel, TParameter, TResult>(TParameter param) where TViewModel : IMvxViewModel;
Task<TResult> Navigate<TViewModel, TResult>() where TViewModel : IMvxViewModel;
Task Navigate(string path);
Task Navigate<TParameter>(string path, TParameter param);
Task<TResult> Navigate<TResult>(string path);
Task<TResult> Navigate<TParameter, TResult>(string path, TParameter param);
Task<bool> CanNavigate<TViewModel>() where TViewModel : IMvxViewModel;
Task<bool> CanNavigate(string path);
Task<bool> Close(IMvxViewModel viewModel);
}

public static class MvxNavigationExtensions
{
public static Task<bool> CanNavigate(this IMvxNavigationService navigationService, Uri path)
public static Task Navigate(this IMvxNavigationService navigationService, Uri path)
public static Task Navigate<TParameter>(this IMvxNavigationService navigationService, Uri path, TParameter param)
public static Task<TResult> Navigate<TResult>(this IMvxNavigationService navigationService, Uri path)
public static Task<TResult> Navigate<TParameter, TResult>(this IMvxNavigationService navigationService, Uri path, TParameter param)
Task<bool> Close<TViewModel>(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.

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 MvxViewModelto indicate that the ViewModel is Closed and trigger it in the NavigationService.

  • delegate OnComplete;

Example of result

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.

Task<TResult> Navigate<TViewModel, TResult>()
{
	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.

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

This comment has been minimized.

Copy link
Contributor

kjeremy commented Feb 28, 2017

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

This comment has been minimized.

Copy link
Member

martijn00 commented Feb 28, 2017

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

This comment has been minimized.

Copy link
Member

thefex commented Mar 1, 2017

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

This comment has been minimized.

Copy link
Contributor

softlion commented Mar 1, 2017

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

@Cheesebaron

This comment has been minimized.

Copy link
Member

Cheesebaron commented Mar 1, 2017

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

@kjeremy

This comment has been minimized.

Copy link
Contributor

kjeremy commented Mar 1, 2017

@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

This comment has been minimized.

Copy link
Member

thefex commented Mar 1, 2017

@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

This comment has been minimized.

Copy link
Member

mvanbeusekom commented Mar 1, 2017

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

This comment has been minimized.

Copy link
Member

Cheesebaron commented Mar 1, 2017

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

This comment has been minimized.

Copy link
Contributor

kjeremy commented Mar 1, 2017

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

This comment has been minimized.

Copy link
Member

thefex commented Mar 1, 2017

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

This comment has been minimized.

Copy link
Member

mvanbeusekom commented Mar 1, 2017

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

This comment has been minimized.

Copy link
Contributor

kjeremy commented Mar 1, 2017

That's definitely true... but then your program dies horribly in a 🔥 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

This comment has been minimized.

Copy link
Contributor

softlion commented Mar 2, 2017

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

This comment has been minimized.

Copy link
Contributor

azzlack commented Mar 2, 2017

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 comment has been minimized.

Copy link
Contributor

softlion commented Mar 2, 2017

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-

This comment has been minimized.

Copy link
Member

Nickolas- commented Mar 8, 2017

@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

This comment has been minimized.

Copy link
Contributor

softlion commented Mar 8, 2017

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.

@nmilcoff nmilcoff referenced this issue Apr 2, 2017

Merged

New MvxIosViewPresenter for iOS #1671

18 of 18 tasks complete
@MKuckert

This comment has been minimized.

Copy link
Member

MKuckert commented Apr 5, 2017

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

This comment has been minimized.

Copy link
Member

martijn00 commented Apr 5, 2017

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

This comment has been minimized.

Copy link
Member

Cheesebaron commented May 29, 2017

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

@martijn00 martijn00 modified the milestones: 5.2.0, 5.1.0 Jul 17, 2017

@martijn00 martijn00 changed the title New navigation service NavigationService improvements Aug 9, 2017

@ferrydeboer

This comment has been minimized.

Copy link

ferrydeboer commented Sep 2, 2017

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

@martijn00 martijn00 modified the milestones: 5.3.0, 5.2.0 Sep 12, 2017

@martijn00 martijn00 modified the milestones: 5.3.0, 5.4.0 Oct 11, 2017

@martijn00 martijn00 modified the milestones: 5.4.0, 5.3.0 Oct 31, 2017

@martijn00 martijn00 closed this Oct 31, 2017

@Rajkumarcs41

This comment has been minimized.

Copy link

Rajkumarcs41 commented Jun 6, 2018

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

This comment has been minimized.

Copy link
Contributor

nickrandolph commented Jun 6, 2018

@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

This comment has been minimized.

Copy link

GiuseppeMarraffa commented Dec 21, 2018

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment