Preventing infinite loops when using INotifyPropertyChanged with a WPF treeview containing checkboxes

Question

This is my first question so I apologise if it is not formatted perfectly.

I am new to WPF and MVVM and I have run into an issue I can't seem to figure out.

I have a treeview that displays a MenuItem hierarchy with a checkbox per MenuItem, both for Parent and Child nodes. The solution current allows a user to click on a parent node and all child items are checked / unchecked as required.

I now need to implement the reverse of this, where if a user clicks on one of the child nodes the parent node should be selected if it is not already selected.

The problem I currently have is that checking the parent node programmatically fires the INotifiedPropertyChanged event for the parent node which rechecks my child nodes.

How do I prevent this from happening?

Here is my MenuItem code:

public class MenuItem : INotifyPropertyChanged
    {
        string _name;
        List<MenuItem> _subItems = new List<MenuItem>();
        bool _isChecked;
        MenuItem _parent;

        public List<MenuItem> SubItems
        {
            get { return _subItems; }
            set
            {
                _subItems = value;
                RaisePropertyChanged("SubItems");
            }
        }

        public string Name
        {
            get { return _name; }
            set
            {
                _name = value;
                RaisePropertyChanged("Name");
            }
        }

        public bool IsChecked
        {
            get { return _isChecked; }
            set
            {
                _isChecked = value;
                RaisePropertyChanged("IsChecked");
            }
        }

        public MenuItem Parent
        {
            get { return _parent; }
            set
            {
                _parent = value;
                RaisePropertyChanged("Parent");
            }
        }

        public event PropertyChangedEventHandler PropertyChanged;
        private void RaisePropertyChanged(string propertyName)
        {
            if (PropertyChanged != null)
                PropertyChanged(this, new PropertyChangedEventArgs(propertyName));

            if (propertyName == "IsChecked")
            {
                if (Parent == null)
                {
                    foreach (MenuItem Child in _subItems)
                        Child.IsChecked = this.IsChecked;
                }

                //if (Parent != null)
                //{
                //    Parent.IsChecked = IsChecked ? true :Parent.IsChecked;
                //}
            }
        }
    }

The commented code above is where I am encountering the error.

Any guidance will be greatly appreciated.

Answer 1

Just a bit more elaborated answer based on the one already written by OP

    public bool IsChecked
    {
        get { return _isChecked; }
        set
        {
            _isChecked = value;

            if (_parent == null)
            {
                foreach (MenuItem Child in _subItems)
                {
                    Child._isChecked = this._isChecked;
                    Child.RaisePropertyChanged("IsChecked");
                }
            }

            if (_parent != null)
            {
                _parent.NotifyChecked(_isChecked);
            }

            RaisePropertyChanged("IsChecked");
        }
    }
    public void NotifyChecked(bool childChecked) 
    { 
       _isChecked = childChecked;
        RaisePropertyChanged("IsChecked"); 
       if (_parent != null)
       {
           _parent.NotifyChecked(_isChecked);
       }
    }

Answer 2

I think you need an other property to store if one of the children is checked. Something like IsChildChecked.

In the UI you can bind these two properties (IsChecked and IsChildChecked) to IsChecked of the node with MultiBinding. Use a converter to set it.

Answer 3

Machine Learning's comment led me to the answer:

public bool IsChecked
        {
            get { return _isChecked; }
            set
            {
                _isChecked = value;

                if (_parent == null)
                {
                    foreach (MenuItem Child in _subItems)
                    {
                        Child._isChecked = this._isChecked;
                        Child.RaisePropertyChanged("IsChecked");
                    }
                }

                if (_parent != null)
                {
                     _parent._isChecked = _isChecked ? true : _parent._isChecked;
                    _parent.RaisePropertyChanged("IsChecked");
                }

                RaisePropertyChanged("IsChecked");
            }
        }

Moving the code to the setter instead of handling it in the event worked for me.

Answer 4

there are a few different approaches you can take,

1 Calculate the parent's is checked property

this would work by the parent listening tothe child's PropertyChanged event and then if any of them are true returning true for the parents IsChecked

private bool isChecked;
public bool IsChecked
{
    get{ return isChecked || Children.Any(c=>IsChecked);}
    set
    {
        isChecked = value;
        RaisePropertyChanged("IsChecked");
        foreach(var child in Children)child.IsChecked
    }
}
public void Child_PropertyChanged(object sender,PropertyChangedEventArgs e)
{
    if(e.PropertyName == "IsChecked")
        RaisePropertyChanged("IsChecked");
}

this approach has the benefit of maintaining the parents click state independently

2 Flip 1 round and calculate the child's IsChecked property

private bool isChecked;
public bool IsChecked
{
    get{ return isChecked || Parent.IsChecked;}
    set
    {
        isChecked = value;
        RaisePropertyChanged("IsChecked");
    }
}
public void Parent_PropertyChanged(object sender,PropertyChangedEventArgs e)
{
    if(e.PropertyName == "IsChecked")
        RaisePropertyChanged("IsChecked");
}

3 Create a second route to change the status with out triggering the cascade

private bool isChecked;
public bool IsChecked
{
    get{ return isChecked;}
    set
    {
        SetIsChecked( value);
        foreach(var child in Children)Parent.SetIsChecked(isChecked)
    }
}
public void SetIsChecked(bool value)
{
    isChecked = value;
    RaisePropertyChanged("IsChecked");
}

this way as long as the children call the SetIsChecked method directly then the cascade is only triggered on a when the parent is set directly via the setter

Note: in your code you aren't handling the PropertyChanged event you are Raising it only

handling looks like this

public MenuItem Parent
{
    get { return _parent; }
    set
    {
        //remove old handler
        // this stops listening to the old parent if there is one
        if(_parent != null)
            _parent.PropertyChange-=Parent_PropertyChanged;

        //notice that the value of _parent changes here so _parent above is not the same as _parent used below
        _parent = value;

        //add new handler
        // this starts listening to the new parent if there is one
        if(_parent != null)
            _parent.PropertyChange+=Parent_PropertyChanged;

        RaisePropertyChanged("Parent");
    }
}
//handler
public void Parent_PropertyChanged(object sender,PropertyChangedEventArgs e)
{
    if(e.PropertyName == "IsChecked")
        RaisePropertyChanged("IsChecked");
}

also all of the above can bu improved by checking for if the current value has changed before making any changes