Skip to content
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

Simplifying IViewFor Interface #5

Open
chris84948 opened this issue Jun 29, 2017 · 7 comments
Open

Simplifying IViewFor Interface #5

chris84948 opened this issue Jun 29, 2017 · 7 comments

Comments

@chris84948
Copy link

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
Copy link
Owner

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
Copy link

zirkelc commented Jul 5, 2017

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
Copy link

BenReierson commented Oct 4, 2017

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
Copy link
Owner

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
Copy link

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
Copy link

abrasat commented Oct 6, 2017

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
Copy link

BenReierson commented Oct 6, 2017

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.

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

No branches or pull requests

5 participants