0

I am trying to keep my question simple and to the point.

At the moment, if I have a property that updates the underlying Model data, and it therefore needs to inform a few other properties that the source has changed, I do it like this:

public Data.MeetingInfo.Meeting Meeting
{
    get { return _Meeting; }
    set
    {
        if(value != null)
        {
            _Meeting = value;
            if (_Meeting.IsDirty)
            {
                _Model.Serialize();
                _Meeting.MarkClean();
                OnPropertyChanged("Meeting");
                OnPropertyChanged("BibleReadingMain");
                OnPropertyChanged("BibleReadingClass1");
                OnPropertyChanged("BibleReadingClass2");
            }
        }
    }
}
private Data.MeetingInfo.Meeting _Meeting;

As you can see, I added several different OnPropertyChanged method calls. Is this an acceptable way to do it? Or, can the specific properties in the Model inform the View that some of it's source has changed?

I have read about implementing the same OnPropertyChanged features in the Model classes. Thus the XAML will pick it up. But I thought those two parts of the MWWV we not supposed ot know about each other.

The thing is, the other 3 are in disabled controls, but they can be updated from two places on the window. So I don't think I can have two update source triggers can I?

Thank you.

Second attempt at explainign things:

ObservableCollection of Meeting objects. Bound to a ComboBox:

<ComboBox x:Name="comboMeetingWeek" ItemsSource="{Binding Meetings}"
                                    SelectedItem="{Binding Meeting, UpdateSourceTrigger=PropertyChanged}" />

The Meeting object contains several properties. We bind controls on the window with these properties. Example:

<ComboBox x:Name="comboNotes" IsEditable="True"
          DataContext="{Binding Meeting}"
          Text="{Binding Note, UpdateSourceTrigger=LostFocus}"
          ItemsSource="{StaticResource Notes}"/>

I do this for the majority of the controls. So the Meeting property in the view model is kept up to date and then when you select a different meeting it commits it to the model data and displays the new meeting (as previously described).

But, in some places on the window, I have some disabled text boxes. These are associated with properties nested inside the Meeting object. For example:

 <TextBox x:Name="textBibleReadingMain" Grid.Column="0" Margin="2" IsEnabled="False"
          DataContext="{Binding TFGW.BibleReadingItem.Main}"
          Text="{Binding DataContext.BibleReadingMain, ElementName=oclmEditor, Mode=TwoWay, NotifyOnSourceUpdated=True, UpdateSourceTrigger=PropertyChanged}"/>

The parent TabItem already has it's DataContext set to {Binding Meeting}. What we need to display in the text box is:

Meeting (current context).TFGW.BibleReadingItem.Main.Name

This is why I had to do it has I did. For the above text box, this is what I want to allow to happen:

  1. It should display the content of Meeting.TFGW.BibleReadingItem.Main.Name (Meeting already being a bound property).

  2. As you select a different meeting from the dates combo, this text box should update.

  3. If the user selects a name from the DataGrid and the ActiveAstudentAssignmentType combo is set to StudentAssignmentType::BibleReadingMain then I also want to update the text box.

I think what I am getting confused about is when I am supposed to derive my classes from INotifyPropertyChanged. My Model data is the Meeting objects with it's own data. Should all of these be inheriting from INotifyPropertyChanged and raising OnPropertyChanged? At the moment I do not have that implemented anywhere. I tell a lie, the only place I implemented it was for the view model itself:

public class OCLMEditorViewModel : INotifyPropertyChanged

So that is why I had to do it the way I did.

Any clearer?

Andrew Truckle
  • 17,769
  • 16
  • 66
  • 164
  • 1
    Don't do that. The `PropertyChanged` event should be raised only for the properties that actually changes. Instead of raising "fake" events, You can simply change your event handling call to handle the `Meeting` property as well as the `BibleReadingMain`, `BibleReadingClass1` and `BibleReadingClass2` properties. – Zohar Peled Jul 03 '16 at 13:42
  • @ZoharPeled They are not fake. Those three others are properties on the window. – Andrew Truckle Jul 03 '16 at 13:44
  • But do they actually change? if they change then that's OK, but if not, well, it is a "fake" event. – Zohar Peled Jul 03 '16 at 14:00
  • @ZoharPeled They change. – Andrew Truckle Jul 03 '16 at 14:01
  • Well, if they change, then the body of the `set` part of each of them should raise the `PropertyChanged` event... – Zohar Peled Jul 03 '16 at 14:03
  • @ZoharPeled They each raise their own. But maybe the three in question should also raise the meeting one. Then the meeting does not need to, and only the one out of the three is triggered. That would be better. – Andrew Truckle Jul 03 '16 at 14:06
  • It's hard to say without a broader context, but in General, less "moving parts" is a better design. – Zohar Peled Jul 03 '16 at 14:12
  • @ZoharPeled can a window control be bound to one property but the update trigger be bound to another? That is my reason for doing what I did. Is it possible? – Andrew Truckle Jul 03 '16 at 14:19
  • I don't know. Have too little experience with wpf. – Zohar Peled Jul 03 '16 at 14:39
  • Is **Data.MeetingInfo.Meeting** a data class? In my opinion, you should raise **PropertyChanged** event for each member in that data class or classes within that class by implementing **INotifyPropertyChanged** for each class. It would definitely improve the code maintainability and readability, as this way would confuse other developers who are reading your code too. – ViVi Jul 04 '16 at 03:29
  • @ViVi It is a data class. How would doing what you suggest simplify the IsDirty mechanism? – Andrew Truckle Jul 04 '16 at 04:40
  • Since I am not aware of the exact scenario, I can't exactly say how it will simplify. What is the use in forcefully calling OnPropertyChanged. You could let it occur automatically when the property changes. – ViVi Jul 04 '16 at 16:27
  • @ViVi The 3 properties concerned are disabled text boxes. They need to update when: a) the user picks a new meeting from the meeting droplist. And b) the user has set an associated combo to that assignment "type" and they then click a name on a data grid. As they click the name it updates the relevant text box. It sounds more complicated that it really is. – Andrew Truckle Jul 04 '16 at 16:46
  • Yes. It sounds quite confusing to me though. Anyways if your requirement is just to update the textbox text or to set its active/inactive state, why don't you simple bind those properties to view model and change the properties accordingly. If the VM implements the **INotifyPropertyChanged** then the text boxes will be automatically updated as well when the required action is done, like click of a grid cell or the selection of a drop down. It would be the best thing to do while following the MVVM pattern in my opinion. – ViVi Jul 05 '16 at 03:15
  • @ViVi Please see update to question. – Andrew Truckle Jul 05 '16 at 13:14
  • 1
    Those `DataContext="{Binding ...}` stand out as a red flag. You shouldn't do or need that with proper MVVM. – H H Jul 05 '16 at 13:36
  • The only time I have done this before is on classes with readonly "calculated" bound fields fields, when one of the "real" property's setters changes a value that the calculated field relies on. In that situation, the setter on the real property should also raise the changed event for the calculated property. – Bradley Uffner Jul 05 '16 at 13:40
  • @HenkHolterman If I don't sent the `DataContext` then I have to change all the binding of the values to `DataContext.Meeting.XXXX` etc. Why is that an issue? I think it seems like my properties themselves should raise the propertychanged event. – Andrew Truckle Jul 05 '16 at 13:43
  • I just think you made it too complicated. I see a binding to the DataContext of another Control... Create a proper ViewModel and set the main DataContext to that. Once, and let a MVVM framework handle that. Then Bind to Property, Property.Property and maybe Model.Property (M as property on VM). That ought to be enough. – H H Jul 05 '16 at 13:58
  • 1
    And then to your original question: It is acceptable for dependent properties (like @BradleyUffner describes) but it can also be factored out by subscribing the VM to its own Changed event. – H H Jul 05 '16 at 13:59
  • I would consider raising change notifications this way for logically unrelated elements of the VM to be a "code smell"; not an error exactly, but an indication that you *may* have some underlying structural and design issues. – Bradley Uffner Jul 05 '16 at 14:04
  • @BradleyUffner As I understand more I will change it if I need too. I best leave the discussion as is for now. Thanks for your assistance. – Andrew Truckle Jul 05 '16 at 14:12

1 Answers1

0

Based on all the comments and further reasearch ....

One of the answers stated:

  1. Viewmodel is created and wraps model
  2. Viewmodel subscribes to model's PropertyChanged event
  3. Viewmodel is set as view's DataContext, properties are bound etc
  4. View triggers action on viewmodel
  5. Viewmodel calls method on model
  6. Model updates itself
  7. Viewmodel handles model's PropertyChanged and raises its own PropertyChanged in response
  8. View reflects the changes in its bindings, closing the feedback loop

I also read a bit of this (which confused me somewhat) where it stated:

The Model notifies the ViewModel if the data in the underlying data store has changed.

So, the first thing I did was change my Meeting object to derive from INotifyPropertyChanged. In addition, I added new properties for gaining access to deeper data in the Meeting model. Example (stripped down):

public class Meeting : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;

    #region Bible Reading Name Properties
    [XmlIgnore]
    public string BibleReadingMainName
    {
        get { return _TFGW.BibleReadingItem.Main.Name; }
        set
        {
            _TFGW.BibleReadingItem.Main.Name = value;
            OnPropertyChanged("BibleReadingMainName");
        }
    }

    [XmlIgnore]
    public string BibleReadingClass1Name
    {
        get { return _TFGW.BibleReadingItem.Class1.Name; }
        set
        {
            _TFGW.BibleReadingItem.Class1.Name = value;
            OnPropertyChanged("BibleReadingClass1Name");
        }
    }

    [XmlIgnore]
    public string BibleReadingClass2Name
    {
        get { return _TFGW.BibleReadingItem.Class2.Name; }
        set
        {
            _TFGW.BibleReadingItem.Class2.Name = value;
            OnPropertyChanged("BibleReadingClass2Name");
        }
    }
    #endregion

    protected void OnPropertyChanged(string name)
    {
        PropertyChangedEventHandler handler = PropertyChanged;
        if (handler != null)
        {
            handler(this, new PropertyChangedEventArgs(name));
        }
    }
}

In my ViewModel I set it as a listener for PropertyChanged:

_Meeting.PropertyChanged += Meeting_PropertyChanged;

At this point in time, the handler just relays the property that was changed:

private void Meeting_PropertyChanged(object sender, PropertyChangedEventArgs e)
{
    OnPropertyChanged(e.PropertyName);
}

In my XAML, I adjust my TextBox to work with the new property, and I remove the DataContext reference. So I now have:

<TextBox x:Name="textBibleReadingMain" Grid.Column="0" Margin="2" IsEnabled="False"
        Text="{Binding BibleReadingMainName, Mode=OneWay, UpdateSourceTrigger=PropertyChanged}"/>

ON the right, where I have the DataGrid, when we click a row and the SelectedStudentItem is updated, we can now do:

private Student _SelectedStudentItem;
public Student SelectedStudentItem
{
    get
    {
        return _SelectedStudentItem;
    }
    set
    {
        // We need to remove this item from the previous student history
        if (_SelectedStudentItem != null)
            _SelectedStudentItem.History.Remove(Meeting.DateMeeting);

        _SelectedStudentItem = value;
        if (_SelectedStudentItem == null)
            return;

        _EditStudentButtonClickCommand.RaiseCanExecuteChanged();
        _DeleteStudentButtonClickCommand.RaiseCanExecuteChanged();
        OnPropertyChanged("SelectedStudentItem");

        if (ActiveStudentAssignmentType == StudentAssignmentType.BibleReadingMain)
            _Meeting.BibleReadingMainName = _SelectedStudentItem.Name;
        else if (ActiveStudentAssignmentType == StudentAssignmentType.BibleReadingClass1)
            _Meeting.BibleReadingClass1Name = _SelectedStudentItem.Name;
        else if (ActiveStudentAssignmentType == StudentAssignmentType.BibleReadingClass2)
            _Meeting.BibleReadingClass2Name = _SelectedStudentItem.Name;
    }

Based on the current ActiveStudentAssignmentType value we can directly update the source property. Thus the TextBox will automatically know about it due to the PropertyChange listener.

Thus, the original Meeting property code now looks like this:

public Data.MeetingInfo.Meeting Meeting
{
    get { return _Meeting; }
    set
    {
        // Has the existing meeting object changed at all?
        if(_Meeting != null && _Meeting.IsDirty)
        {
            // Yes, so save it
            _Model.Serialize();
            _Meeting.MarkClean();
        }

        // Now we can update to new value
        if (value != null)
        {
            _Meeting = value;
            OnPropertyChanged("Meeting");
        }
    }
}
private Data.MeetingInfo.Meeting _Meeting;

All of those extra OnPropertyChanged calls are now obsolete!

The thing I was missing was implementing Notification from the Model to the ViewModel. And then the ViewModel informing the View.

Community
  • 1
  • 1
Andrew Truckle
  • 17,769
  • 16
  • 66
  • 164
  • 1
    You could avoid all this hassle and save hundreds of lines of boilerplate code if you used a proper framework. e.g. [Catel](https://github.com/Catel/Catel). You would just simply decorate your viewmodel properties with `[Model]` and `[ViewModelToModel]` [attributes](https://catelproject.atlassian.net/wiki/display/CTL/Creating+a+view+model+with+a+model+and+mappings). In combination with [Catel.Fody](http://catelproject.com/tools/catel-fody/), you would even have you properties looking like `POCO` properties: `[Model] public Meeting Meeting { get; set; }` – mechanic Jul 06 '16 at 14:43
  • @mechanic Hopefully with time I will get to learn about these frameworks. Thanks for your comments. – Andrew Truckle Jul 06 '16 at 14:49