Supporting Proxied/Wrapped ViewModels

[1iveowl]

Please read this carefully this before you dismiss this PR? The comments, I got the previous two times I tried left me with the feeling that we are not entirely on the same page, so I try once more.

From our conversation I get sense is that the idea of wrapped/proxied ViewModels feels unfamiliar. I get that; nevertheless a MVVM pattern using this approach such as that supported by Assisticant (https://www.nuget.org/packages/Assisticant/) have been downloaded ~3000 times so “yikes”, there are other “crazy”, “complicated” people out there ;-)

For me this pattern helps make ViewModels cleaner. I don’t like complexity either.

The changes proposed are:

NavigationService.cs:
#590 addressed NavigateToAsync only. However, I see that page.DataContext as INavigable since yesterday is now also used in NavigateFromAsync and NavigatedFromAsync.

I’m glad we agree that this is an improvement over checking DataContext for null, as all three methods are in fact assuming INavigable to work as intended. The new code is an improvement, but it is still insufficient for dealing with wrapped ViewModels.

Using ResolveForPage is indeed the right approach and I do apologies that my first suggestion did not recognize this. Now, let’s make sure that all three methods do in fact use ResolveForPage to deal with situations where DataContext is not INavigable. Currently only NativateToAsync does. The two others just ignore any further action if DataContext is not INavigable.

Because all three methods need to get hold of the right INavigable DataContext/ViewModel, I think the best approach is to create one single private method called “ResolveForPage(Page page)” in NavigationService.cs. The rational is reducing complexity – i.e. write the code once, not thrice.

Please notice that I’m passed the Page object and not just the Type of the page to ResolveForPage. There is an important reason for that, which leads me to the changes proposed to Bootstrapper.cs.

Bootstrapper.cs.

Here I propose to overload the ResolveForPage with a new method signature that accepts the Page object: public virtual INavigable ResolveForPage(Page page, NavigationService navigationService)

In the PR the new overloaded ResolveForPage gets the Type of the page and then passes the Type on to the original ResolveForPage method. This is just to ensure that any current implementation that might be overriding the original ResolveForPage(Type type…) is unaffected by the change.

But why overload the ResolveForPage? Because, this new overloaded ResolveForPage(Page page…) is what enables anyone working with wrapped ViewModels gain access to the Page object so that they can write code to upwrap the DataContext in order to expose the inner object that satisfy the INavigable assumption needed for the NavigationService. The existing ResoveForPage(Type type…) is insufficient for this purpose.

Just as an example: using Assisticant with the proposed modifications would look something like this:

public class BootStrapperEx : BootStrapper
{

    public override INavigable ResolveForPage(Page page, NavigationService navigationService)
    {
        return ForView.Unwrap<dynamic>(page.DataContext) as INavigable;
    }

}

I’ve tested it and this works as intended. And just as importantly it opens Template10 to the exotic bread of developers who like to wrap their ViewModels. We are people too you know ;-)

Oh and finally, I don’t see this breaking anything.

Thank you for your understanding.

[jhalbrecht]

FWIW

I use auto-properties and Fody/PropertyChanged to Inject INotifyPropertyChanged code into properties at compile time along with MvvmLight in my Template10 projects. Other than calling MvvmLight.ViewModelBase in ViewModelBase.cs I haven't had to make any other changes to Template10.

public abstract class ViewModelBase : GalaSoft.MvvmLight.ViewModelBase, Template10.Services.NavigationService.INavigable // Template10.Mvvm.ViewModelBase

[1iveowl]

Thank you jhalbrecht, I appreciate your input. I didn't know PropertyChanged.Fody. It looks interesting.

From what I understand so far PropertyChanged.Fody does not wrap the ViewModel, rather it added some code to the compiled ViewModel by using Attributes.

I agree that PropertyChanged.Fody should work fine already with Template10 without the proposed changes in this PR. Other frameworks that do implement a wrapper around the ViewModel does not work without the addition suggested in #629.

[JerryNixon]

Please call both Bootstrapper.ResolveForPage() and NavigationService.ResolveForPage() the same method name. Also, for developers using ResolveForPage(Type) already, please include ResolveForPage(Page) as an override without removing ResolveForPage(Type). This make sense?

[1iveowl]

Yes, that makes sense. Let be see if I got it right.

a) I've renamed NavigableDataContext(Page page) to ResolveForPage(Page page) in NavigationService.

b) ResolveForPage(Page page) is an overload to ResolveForPage(Type type) in Bootstrapper meaning that Developers already using ResolveForPage(Type type) will not be effected by the change. (Note: you write override above, but I suspect you mean overload? Both ResolveForPage methods are already marked virtual override.

Did I get it right?

[JerryNixon]

Nice. What throw new ArgumentNullException(); all about?

[JerryNixon]

I will (or you can) add [Obsolete] to ResolveForPage(Type). Make sense?

[1iveowl]

The throw new ArgumentNullException(); exception is thrown if no ViewModel is resolved. Throwing an Exception is the best I could come up with, because if no ViewModel is resolved then there is an issue that the developer should attend to. Or?

[1iveowl]

We could make ResolveForPage(Type) obsolute, but is it not a common practise to resolve ViewModels based on Type also? I suppose that some would argue that both approaches are valid.

Anyhow, I've added the Obsolete. Let me know if you want to keep it.

[mvermef]

How about a default that says something to the nature of no type found, but be gentle about it, this is something that was done in caliburn.micro when no view could be associated with a viewmodel

[1iveowl]

Thanks for the input. Sound interesting.

Can you be more specific about what you mean by "be more gentle about it"? Is that not to throw an exception or something different? I'm not familiar with the Caliburn.Micro implementation.

[JerryNixon]

I don't want to require a view model.

[1iveowl]

Ok. I've removed the throw of an exception. No the code will just move on a ignore the case where DataContext is not INavigable and no ViewModel is found through ResolveForPage. I can see that work out and it will be up for the developer to catch that nothing happens in testing when needed.