codemillmatt / codemill.vmfirstnav

Xamarin.Forms ViewModel First Navigation Library
MIT License
22 stars 5 forks source link

Simplifying IViewFor Interface #5

Open chris84948 opened 7 years ago

chris84948 commented 7 years ago

First of all, thanks for this, it definitely helped me get my head around where I needed to go. Normally I'd make the changes and issue a PR, but in this case I was just grabbing some of the things you've done and rolling it my own way as well.

I had a quick suggestion on the IViewFor interface. It seems like you don't really need to have the ViewModel property in that interface. I simplified it to just -

public interface IViewFor<T> : IViewFor where T : IViewModel
{ }

Then you can change NavigationService.IntantiateView to this -

IViewFor<T> InstantiateView<T>(T viewModel) where T : class, IViewModel
{
    // Figure out what type the view model is
    var viewModelType = viewModel.GetType();

    // look up what type of view it corresponds to
    var viewType = _viewModelViewDictionary[viewModelType];

    // instantiate it
    var view = (IViewFor<T>)Activator.CreateInstance(viewType);

    view.BindingContext = viewModel;

    return view;
}

You're still getting the binding context in there, but now you don't have to go and add that property to every view.

codemillmatt commented 7 years ago

Thanks @chris84948 - I'll give this one some thought - at first glance I like it.

I do like setting the binding context in the InstantiateView.

I'm trying to think of a time where I wouldn't want the binding context set automatically for me. Right now, offhand, I can't think of any - I know I've always set the binding context in the ViewModel property in the view.

Then there are the times when you do want to get at the view model from the code behind for whatever reason ... but in this case, that would be a simple cast from the BindingContext ...

zirkelc commented 7 years ago

I think setting the BindingContext directly in InstantiateView or InstantiateViewForSwitchDetailPage makes sense, especially for the second method where we need reflection to set the ViewModel property.

Nevertheless, I agree with Matt that it's useful to access the ViewModel directly from code-behind without casting it, for example to call a ViewModel initialization method in Page.OnAppearing().

BenReierson commented 7 years ago

Personally, I prefer the way it is because I setup my vms as static and bind them in XAML, which helps when using the xaml previewer. Because of that, I don't actually need the VM set when the view is created because it's set already in the xaml, so I make sure to implement the proper such that it doesn't set the bindingcontext unless the incoming object is different.

That said, you can avoid having to add the ViewModel property in each via by using a baseclass for your views. I am using the following:

public abstract class BaseContentPage<T> : ContentPage, IViewFor<T> where T : class, IViewModel 
    {
        T IViewFor<T>.ViewModel
        {
            get => BindingContext as T;
            set { if (BindingContext != value) { BindingContext = value; OnPropertyChanged(); } }
        }

The xaml for a page looks like this:

<v:BaseContentPage x:TypeArguments="vm:SettingsViewModel"
    xmlns="http://xamarin.com/schemas/2014/forms"
    xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
    x:Class="Views.SettingsPage"
    BindingContext="{x:Static vm:ViewModelLocator.SettingsVM}}">
codemillmatt commented 7 years ago

Interesting... I'll be honest, I haven't ever thought of using static view models. At the risk of going off topic here a bit ... have you run into any issues w/ static VMs?

BenReierson commented 7 years ago

Well they aren't static really, just singletons exposed via a static factory. It's a pattern I picked up from using MVVMLight, which mainly does it in order to expose design-time data (static vms will be instantiated by blend/xaml previewer), but there's often no downside to using the singleton instance at runtime. When I need to create multiple instances of a particular vm, I certainly can, I just don't run into the need for that very often. Tends to cut down on memory leaks too.

abrasat commented 7 years ago

Does this approach mean that the ViewModels keep their "state" (and are practically persistent) when navigating through different pages, as they are created only once at first page display ?

BenReierson commented 7 years ago

Yeah, the VMs would keep their state by default, but most apps I tend to work on have a 1-1 screen-vm relationship and each screen initialises when it's navigated to or shown, so it handles clearing out or reseting its state at that point. There are certainly some types of screens/vms that don't work well that way, and nothing prevents newing up vms for those along the way. This pattern is just a bit of a shortcut that works for most scenarios. I should emphasize that I never code the vms in a way that would cause issues if multiple instances of them exist, or if they are created/destroyed multiple times at runtime.