WPF MVVM INotifyPropertyChanged Implementation - Model or ViewModel

Question

I have read a number of debates on where to implement INotifyPropertyChanged here on StackOverflow and other blogs but it seems that there are cases where you have to implement it on the Model. Here is my scenario - I am looking for feedback on my conclusion or is my approach wrong.

I am using this implementation of an ObservableDictionary (ObservableDictionary) because I need performant queries using the key.

In this dictionary I place the collection of Model objects.

In my VM, I declare an instance (Books) of the dictionary and in the XAML bind to it.

    <tk:DataGrid AutoGenerateColumns="False" Grid.Row="1" ItemsSource="{Binding Mode=TwoWay, Path=Books.Store}" Grid.ColumnSpan="2" Margin="3">
        <tk:DataGrid.Columns>
            <tk:DataGridTextColumn Binding="{Binding Mode=TwoWay, Path=Value.Name}" MinWidth="100" Header="Name" />
            <tk:DataGridTextColumn Binding="{Binding Mode=TwoWay, Path=Value.Details}" MinWidth="300" Header="Details" />
        </tk:DataGrid.Columns>        
    </tk:DataGrid>  

If I implement INotifyPropertyChanged on the VM for Books and change the value of a Book name in code, the UI is not updated.

If I implement INotifyPropertyChanged on the VM for Store and change the value of a Book name in code, the UI is not updated.

If I implement INotifyProperyChanged on the Model and change the value a Book name in code, the UI is updated.

The Changed event is not fired in the first case because the Dictionary setter is not called, it's Item (a Book) is.

Am I missing something because if this is the correct interpretation, if I want consistent notifications for my Models regardless of whether they are bound to directly from XAML or via some sort of collection, I would always want the Model to implement INotifyProperyChanged.

Btw, besides the dll reference, I personally do no see INotifyPropertyChanged as a UI function - think it should be defined in a more general .net namespace - my 2 cents.

EDIT STARTS HERE:

We were having such a good semantics debate that I missed the core of my question so here it is posted again but with a very simple MVVM example illustrate my question.

The Models:

public class Book
{
    public string Title { get; set; )
    public List<Author> Authors { get; set; }
}

public class Author
{
    public string Name { get; set; }
}

The Data Provider to generate some dummy data

public class BookProvider
{
    public ObservableCollection<Book> GetBooks() {
        ObservableCollection<Book> books = new ObservableCollection<Book>();

        books.Add(new Book {
            Title = "Book1",
            Authors = new List<Author> { new Author { Name = "Joe" }, new Author { Name = "Phil" } }
        });

        books.Add(new Book {
            Title = "Book2",
            Authors = new List<Author> { new Author { Name = "Jane" }, new Author { Name = "Bob" } }
        });

        return books;
    }
}

The ViewModel

    public class BookViewModel : INotifyPropertyChanged
{
    private ObservableCollection<Book> books;
    public ObservableCollection<Book> Books {
        get { return books; }
        set {
            if (value != books) {
                books = value;
                NotifyPropertyChanged("Books");
            }
        }
    }

    private BookProvider provider;

    public BookViewModel() {
        provider = new BookProvider();
        Books = provider.GetBooks();
    }

    // For testing the example
    public void MakeChange() {
        Books[0].Title = "Changed";
    }

    public event PropertyChangedEventHandler PropertyChanged;

    protected void NotifyPropertyChanged(String info) {
        if (PropertyChanged != null) {
            PropertyChanged(this, new PropertyChangedEventArgs(info));
        }
    }
}

XAML code behind Would not normally to it this way - just for simple example

public partial class MainWindow : Window
{
    private BookViewModel vm;

    public MainWindow() {
        InitializeComponent();

        vm = new BookViewModel();
        this.DataContext = vm;
    }

    private void Button_Click(object sender, RoutedEventArgs e) {
        vm.MakeChange();
    }
}

The XAML

<Window x:Class="BookTest.MainWindow"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    Title="MainWindow" Height="350" Width="525">
<Grid>
    <Grid.RowDefinitions>
        <RowDefinition Height="242*" />
        <RowDefinition Height="69*" />
    </Grid.RowDefinitions>
    <ListBox ItemsSource="{Binding Books}">
        <ListBox.ItemTemplate>
            <DataTemplate>
                <StackPanel Orientation="Vertical">
                    <TextBlock Text="{Binding Title}" />
                    <ListBox ItemsSource="{Binding Authors}">
                        <ListBox.ItemTemplate>
                            <DataTemplate>
                                <TextBlock Text="{Binding Name}" FontStyle="Italic" />
                            </DataTemplate>
                        </ListBox.ItemTemplate>
                    </ListBox>
                </StackPanel>
            </DataTemplate>
        </ListBox.ItemTemplate>
    </ListBox>
    <Button Grid.Row="1" Content="Change" Click="Button_Click" />
</Grid>

As coded above, when I click on the button and change the value in the first Book, the UI does not change.

However, when I move the INotifyPropertyChanged to the Model it works fine (UI updates) because the change is in the Model property setter not Books in the VM:

public class Book : INotifyPropertyChanged
{
    private string title;
    public string Title {
        get { return title; }
        set {
            if (value != title) {
                title = value;
                NotifyPropertyChanged("Title");
            }
        }
    }

    public List<Author> Authors { get; set; }

    public event PropertyChangedEventHandler PropertyChanged;

    protected void NotifyPropertyChanged(String info) {
        if (PropertyChanged != null) {
            PropertyChanged(this, new PropertyChangedEventArgs(info));
        }
    }
}

So back to my original question, how do I accomplish this without implementing INotifyPropertyChanged in the Model?

Thanks.

Solution 1

The thing is that if you were following MVVM, you would have a BookViewModel for your Book model class. So you would have a INotifyPropertyChanged implementation on that view model. Exactly for that purpose MVVM exists (but not only).

That being said, the INotifyPropertyChanged has to be implemented on view model classes, not models.

UPDATE: In response to your update and our discussion in comments...

By BookViewModel I meant something else. You need to wrap in this view model not the whole collection of Book objects but an individual Book:

public class BookViewModel : INotifyPropertyChanged
{
    private Book book;

    public Book Book {
        get { return book; }    
    }

    public string Title {
        get { return Book.Title; }
        set {
            Book.Title = value;
            NotifyPropertyChanged("Title");
        }         
    }

    public BookViewModel(Book book) {
        this.book = book;
    }

    public event PropertyChangedEventHandler PropertyChanged;

    protected void NotifyPropertyChanged(String info) {
        if (PropertyChanged != null) {
            PropertyChanged(this, new PropertyChangedEventArgs(info));
        }
    }
}

And your BookProvider will return ObservableCollection<BookViewModel> instead of ObservableCollection<Book>:

public class BookProvider
{
    public ObservableCollection<BookViewModel> GetBooks() {
        ObservableCollection<BookViewModel> books = new ObservableCollection<BookViewModel>();

        books.Add(new BookViewModel(new Book {
            Title = "Book1",
            Authors = new List<Author> { new Author { Name = "Joe" }, new Author { Name = "Phil" } }
        }));

        books.Add(new BookViewModel(new Book {
            Title = "Book2",
            Authors = new List<Author> { new Author { Name = "Jane" }, new Author { Name = "Bob" } }
        }));

        return books;
    }
}

As you can see, when you are updating the Title property of the Book you will be doing it through the Title property of the corresponding view model that will raise the PropertyChanged event, which will trigger the UI update.

Solution 2

Please read this article. It explains how you can reduce code duplication by implementing INotifyPropertyChanged in the model.

Solution 3

Don't confuse INotifyPropertyChanged with MVVM.

Consider what INotifyPropertyChanged actually is -> It's an event that fires to say "Hey look, I've changed". If anyone cares then they can do something about it, whether they're a View, ViewModel, or whatever.

So let's start with your Book (Model). The Title property can fire a changed event, why would it not? It makes sense, the Book is dealing with it's own properties.

Now for BookViewModel - great, we don't need to duplicate the Title and bulk up our code! Whoo!

Consider a View where we want to see a list of books, or a book with a list of authors. Your ViewModel can handle additional properties specific for the view, such as IsSelected. This is a great example - why would the Book care if it was selected or not? This is the responsibility of the ViewModel.

---

Obviously the above depends on your architecture, but personally if I'm creating an object library I'll implement a base class with INotifyPropertyChanged and make the object properties responsible for firing the event.

Comments

[IUnknown]

hmmm, I am using MVVM. I am using a ViewModel per View not per Model. The ViewModel not only contains the Model properties that the View binds to (there are a couple) but also the command implementations for the View. A Model-only BooksViewModel that would then be added as a property to the View's ViewModel for binding would probably only implemet INotifyPropertyChanged. Why not just do that in the Model.

[Pavlo Glazkov]

@IUnknown - If you do it in the model, you will add the UI specific stuff in your model classes. There are view models for this. The rule of thumb in MVVM is not have bindings to model classes directly in your views. This way you would not have this kind of problems.

[IUnknown]

@Palvo - I am not sure I follow you. I have my ViewModels create instances of the Models as "notifyable" properties, populate them from some source, and bind (from xaml) the UI to those properties. Also handle any UI specific logic such as implementing the commands. Is this not binding the UI to my model objects (via VM property)? This brings me back to my original question. If the VM propery that the View binds to is the ObservableDictionary, the VM's IPropertyNotifyChanged implementation does not update the UI but the model's would.

[Pavlo Glazkov]

@IUnknown - I'm saying that typically in such situations you would create a view model for items in your dictionary, which means you would wrap your Book object in a BookViewModel. Then you will be able to raise PropertyChanged event when appropriate on that view model.

[IUnknown]

So this BookViewModel is not the ViewModel for the View but rather another layer between the ViewModel for the View and the Model. I would not want this to be the ViewModel for the View because there are "non-bookish" things also happening in the View's ViewModel. Calling this a ViewModel (BookViewModel) seems to be confusing. I usually reserve the term ViewModel for the View's ViewModel and call things like your BookViewModel still models because they are just either aggregating Models together or providing collection management but are still agnostic to which View they are being bound to.

[Pavlo Glazkov]

@IUnknown - Actually it IS a view model. Not every view model has to have a physical view file (xaml file). It is normal in MVVM to have a view model for items in a collection. DataTemplate often plays the role of view for such view models.

[IUnknown]

Agree that it is not always the case to have a physical xaml view file when DataTemplate can be used but those are usually the simplier View / ViewModels. In cases where the view is more complex involving a couple Models and Command handlers, the ViewModel presents and manages this for the xaml View. It is this case I am referring to. In this case, where the ViewModel is backing a more complex View and not just a presenter for a simple model, do you recommend creating a class to wrap the BookModel for the View's ViewModel or implementing INotifyPropertyChanged in the Model

[Pavlo Glazkov]

@IUnknown - I recommend creating a class to wrap the BookModel (you can call it differently, but in fact it will be a view model).

[IUnknown]

Thanks. I do agree that I need to wrap BookModel. It might be a matter of semantics but I do like these type of debates :). I have always thought the ViewModel supports the View - only one aspect of this is presenting the Model. In my case, a "BookModelView" that just wraps the BookModel in a Collection and implements INotifyPropertyChanged is not sufficient to support the View. This means I either have several layers of ViewModels with one Model or One ViewModel and several layers of Models. I guess I have usually favoured the later. Anyway, the same net effect just personal preference.

[IUnknown]

@Pavlo - sorry back at it again - please see my edits in the original question

[IUnknown]

@Palvo - Thanks. I understand the BookViewModel class you provided above. It is what I described in one of the above comments - a class to wrap the Model that only implements INotifyPropertyChanged so the Model does not have to. I guess it is down to personal design preferences but if the only reason was to add Notification then I do not see the extra class layer worthwhile. I do see this class as a way to isolate the View's ViewModel from changes in Model though. Continued in next comment....

[IUnknown]

... Continued .... I think also boils down (personally) to INotifyPropertyChanged's Namespace which makes us think we are adding UI code to the Model. I personally believe that INotifyPropertyChanged has nothing to do with UI and conceptually is just a way of notifying objects that they have changed - a very useful general purpose capability. Its too bad Microsoft put it in the namespace they did - IMO.

[IUnknown]

BTW, if you call your above BookViewModel the View Model, what to you call the class that I call the View Model?

[Pavlo Glazkov]

@IUnknown - Look, both of these things are view models. Think about it this way - everything that you create to support view (e.g. implementation of INotifyPropertyChanged) is a view model. View model is not only the class that defines commands and stuff like that. Everything that you would add to your model classes only just because of view (again, implementation of 'INotifyPropertyChanged') should go to a view model. Having a dedicated view model for each item in a collection is beneficial because in future you might also want to add more UI specific things to it, like commands.

[pfeds]

Much more valid. Why shouldn't a model class raise notification events when it's properties change. Makes more sense to me.