MVVM and collections of VMs

Question

A common senario: A model with a collection of item models.
E.g a House with a collection of People.

How to structure this correctly for MVVM - particulary with regard to updating the Model and ViewModel collections with additions and deletes?

Model House contains a collection of model People (normally a List<People>).
View model HouseVM contains the House object that it wraps and an ObservableCollection of view model PeopleVM (ObservableCollection<PeopleVM>). Note that we end up here with the HouseVM holding two collections (that require syncing):
1. HouseVM.House.List<People>
2. HouseVM.ObservableCollection<PeopleVM>

When the House is updated with new People (add) or People leave (remove) that event must now be handled in both collections the Model House People collection AND the the VM HouseVM PeopleVM ObservableCollection.

Is this structure correct MVVM?
Is there anyway to avoid having to do the double update for Adds and Removes?

Answer 1

Your general approach is perfectly fine MVVM, having a ViewModel exposing a collection of other ViewModels is a very common scenario, which I use all over the place. I would not recommend exposing items directly in a ViewModel, like nicodemus13 said, as you end up with your view binding to models without ViewModels in between for your collection's items. So, the answer to your first question is: Yes, this is valid MVVM.

The problem you are addressing in your second question is the synchronization between the list of people models in your house model and the list of people ViewModels in your house ViewModel. You have to do this manually. So, no there is no way to avoid this.

What you can do: Implement a custom ObservableCollection<T>, ViewModelCollection<T>, which pushes it's changes to an underlying collection. To get two way synching, make the model's collection an ObservableCollection<> too and register to the CollectionChanged event in your ViewModelCollection.

This is my implementation. It uses a ViewModelFactory service and so on, but just have a look at the general principal. I hope it helps...

/// <summary>
/// Observable collection of ViewModels that pushes changes to a related collection of models
/// </summary>
/// <typeparam name="TViewModel">Type of ViewModels in collection</typeparam>
/// <typeparam name="TModel">Type of models in underlying collection</typeparam>
public class VmCollection<TViewModel, TModel> : ObservableCollection<TViewModel>
    where TViewModel : class, IViewModel
    where TModel : class

{
    private readonly object _context;
    private readonly ICollection<TModel> _models;
    private bool _synchDisabled;
    private readonly IViewModelProvider _viewModelProvider;

    /// <summary>
    /// Constructor
    /// </summary>
    /// <param name="models">List of models to synch with</param>
    /// <param name="viewModelProvider"></param>
    /// <param name="context"></param>
    /// <param name="autoFetch">
    /// Determines whether the collection of ViewModels should be
    /// fetched from the model collection on construction
    /// </param>
    public VmCollection(ICollection<TModel> models, IViewModelProvider viewModelProvider, object context = null, bool autoFetch = true)
    {
        _models = models;
        _context = context;

        _viewModelProvider = viewModelProvider;

        // Register change handling for synchronization
        // from ViewModels to Models
        CollectionChanged += ViewModelCollectionChanged;

        // If model collection is observable register change
        // handling for synchronization from Models to ViewModels
        if (models is ObservableCollection<TModel>)
        {
            var observableModels = models as ObservableCollection<TModel>;
            observableModels.CollectionChanged += ModelCollectionChanged;
        }


        // Fecth ViewModels
        if (autoFetch) FetchFromModels();
    }

    /// <summary>
    /// CollectionChanged event of the ViewModelCollection
    /// </summary>
    public override sealed event NotifyCollectionChangedEventHandler CollectionChanged
    {
        add { base.CollectionChanged += value; }
        remove { base.CollectionChanged -= value; }
    }

    /// <summary>
    /// Load VM collection from model collection
    /// </summary>
    public void FetchFromModels()
    {
        // Deactivate change pushing
        _synchDisabled = true;

        // Clear collection
        Clear();

        // Create and add new VM for each model
        foreach (var model in _models)
            AddForModel(model);

        // Reactivate change pushing
        _synchDisabled = false;
    }

    private void ViewModelCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
    {
        // Return if synchronization is internally disabled
        if (_synchDisabled) return;

        // Disable synchronization
        _synchDisabled = true;

        switch (e.Action)
        {
            case NotifyCollectionChangedAction.Add:
                foreach (var m in e.NewItems.OfType<IViewModel>().Select(v => v.Model).OfType<TModel>())
                    _models.Add(m);
                break;

            case NotifyCollectionChangedAction.Remove:
                foreach (var m in e.OldItems.OfType<IViewModel>().Select(v => v.Model).OfType<TModel>())
                    _models.Remove(m);
                break;

            case NotifyCollectionChangedAction.Reset:
                _models.Clear();
                foreach (var m in e.NewItems.OfType<IViewModel>().Select(v => v.Model).OfType<TModel>())
                    _models.Add(m);
                break;
        }

        //Enable synchronization
        _synchDisabled = false;
    }

    private void ModelCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
    {
        if (_synchDisabled) return;
        _synchDisabled = true;

        switch (e.Action)
        {
            case NotifyCollectionChangedAction.Add:
                foreach (var m in e.NewItems.OfType<TModel>()) 
                    this.AddIfNotNull(CreateViewModel(m));
                break;

            case NotifyCollectionChangedAction.Remove:
                    foreach (var m in e.OldItems.OfType<TModel>()) 
                        this.RemoveIfContains(GetViewModelOfModel(m));
                break;

            case NotifyCollectionChangedAction.Reset:
                Clear();
                FetchFromModels();
                break;
        }

        _synchDisabled = false;
    }

    private TViewModel CreateViewModel(TModel model)
    {
        return _viewModelProvider.GetFor<TViewModel>(model, _context);
    }

    private TViewModel GetViewModelOfModel(TModel model)
    {
        return Items.OfType<IViewModel<TModel>>().FirstOrDefault(v => v.IsViewModelOf(model)) as TViewModel;
    }

    /// <summary>
    /// Adds a new ViewModel for the specified Model instance
    /// </summary>
    /// <param name="model">Model to create ViewModel for</param>
    public void AddForModel(TModel model)
    {
        Add(CreateViewModel(model));
    }

    /// <summary>
    /// Adds a new ViewModel with a new model instance of the specified type,
    /// which is the ModelType or derived from the Model type
    /// </summary>
    /// <typeparam name="TSpecificModel">Type of Model to add ViewModel for</typeparam>
    public void AddNew<TSpecificModel>() where TSpecificModel : TModel, new()
    {
        var m = new TSpecificModel();
        Add(CreateViewModel(m));
    }
}

Answer 2

In this situation I simply make the model expose ObservableCollections rather than Lists. There's no particular reason why it shouldn't. The ObservableCollection is in the System.Collections.ObjectModel namespace of the System assembly, so there's no unreasonable extra dependencies, you almost certainly have System anyway. List is in mscorlib, but that's as much a historical artefact as anything.

This simplifies the model-viewmodel interactions massively, I can't see a reason not to do it, using Lists on the model just creates lots of unpleasant boiler-plate code. You are interested in the events, after all.

Also, why is your HouseVM wrapping an ObservableCollection<PeopleVM>, rather than ObservableCollection<People>? VMs are for binding to views, so I would think that whatever is binding to your ObservableCollection<PeopleVM> is actually interested in People, otherwise you're binding-within-a-binding, or is there a specific reason why this is useful? I wouldn't generally have a VM expose other VMs, but maybe that's just me.

Edit about libraries/WCF

I don't see why having a model in a library, or even exposed by a WCF-server should affect whether they raise events or not, it seems perfectly valid to me (obviously the WCF-service won't expose the events directly). If you don't like this, I think you're stuck with having to chain multiple updates, though I wonder if you're actually just manually doing the same work as the event would do in an ObservableCollection, unless I've misunderstood some of it.

Personally, like I said, I'd keep the VMs simple, and have them expose the minimum and not expose other VMs. It can take some redesign and make certain parts a bit of a pain (e.g. Converters, however, you end up with a simple, easy-to-manage design with some simple-to-handle irritations on the edges.

It seems to me that your current route is going to end up very complex rather quickly and, most importantly, awkward to follow... However, YMMV, it's just my experience :)

Perhaps moving some of the logic to explicit services might help?