Conductor<T>.Collection.OneActive/AllActive - IChild.Parent not reset on clear

[tklingert]

void Main()
{
	var conductor = new Conductor<TestItem>.Collection.AllActive();
	
	var t1 = new TestItem();
	var t2 = new TestItem();
	var t3 = new TestItem();
	conductor.Items.Add(t1);
	conductor.Items.Add(t2);
	conductor.Items.Add(t3);
	if (t1.Parent == null || t2.Parent == null || t3.Parent == null)
		throw new Exception("t1.Parent + t2.Parent + t3.Parent should have been set by conductor");
	conductor.Items[0] = null;
	if (t1.Parent != null)
		throw new Exception("t1.Parent should have been set to <null> by conductor");
	conductor.Items.Remove(t2);
	if (t2.Parent != null) 
		throw new Exception("t2.Parent should have been set to <null> by conductor");
	conductor.Items.Clear();
	if (t3.Parent != null)
		throw new Exception("t3.Parent should have been set to <null> by conductor");
}

The code above fails on 't3.Parent != null', it seems like the conductor implementation only resets the child's parent property on a normal or index-based removal, but not on a clear operation.

The root cause seems to be located within the BindableCollection<T> implementation: A simple (not really pretty) workaround:

public class Workaround<T>
	where T : class
{
	private void ConductorItems_CollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
	{
		switch (e.Action)
		{
			case NotifyCollectionChangedAction.Add:
				this._mirroredItems.AddRange(e.NewItems.OfType<T>());
				break;
			case NotifyCollectionChangedAction.Remove:
				e.OldItems.OfType<T>().Apply(i => this._mirroredItems.Remove(item: i));
				break;
			case NotifyCollectionChangedAction.Replace:
				this._mirroredItems.AddRange(e.NewItems.OfType<T>());
				e.OldItems.OfType<T>().Apply(i => this._mirroredItems.Remove(item: i));
				break;
			case NotifyCollectionChangedAction.Reset:
				if (this._conductorItems.Count == 0) // clear called
				{
					this._mirroredItems.OfType<IChild>().Apply(i => i.Parent = null);
					this._mirroredItems.Clear();
					return;
				}

				// re-synchronize mirrored items
				this._mirroredItems.Where(i => !this._conductorItems.Contains(item: i)).OfType<IChild>().Apply(i => i.Parent = null);
				this._mirroredItems.Clear();
				this._mirroredItems.AddRange(collection: this._conductorItems);
				break;
		}
	}
	public Workaround(IObservableCollection<T> conductorItems)
	{
		if (conductorItems == null) throw new ArgumentNullException(nameof(conductorItems));
		this._conductorItems = conductorItems;
		this._mirroredItems = new List<T>(collection: conductorItems);
		conductorItems.CollectionChanged += this.ConductorItems_CollectionChanged;
	}
}

=>

new Workaround<TestItem>(conductor.Items);

lets the initial example pass without errors.

[nigel-sampson]

Definitely sounds like a bug, thanks for raising it.

[nigel-sampson]

So this looks like it comes out of an issue around ObservableCollection<T> not populating the NotifyCollectionChangedEventArgs.OldItems when the collection is cleared. Because of this we don't hace access to the items in the collection that were removed.

The solution above maintains a second separate list in order to determine which items were removed. I'm not sure yet if this is the right approach.

At the moment I'd recommend using something like Items.Apply(i => Items.Remove(i)); to get what you need.

[tklingert]

I agree, it was only meant to be a workaround for cases where you cannot directly control how the items collection is cleared - e.g. A binding to UI control where the control's internal logic invokes clear directly.

The internally used BindableCollection should raise a proper notification on clear.

Regarding the conductor base implementation, it would also be quite usable to be able to control/override which concrete implementation of IObservableCollection<T> is used to store the items.

[nigel-sampson]

Yup, from discussions this post and Stack Overflow. it's a well known problem, but unfortunately has impacts into the WPF UI as where changing how notifications are done causes errors with some controls. It's hard to judge what the "proper notification" should be.

It's possible we add a new event Cleared to the collection, or leave this as a known issue.

[tklingert]

Agree, https://stackoverflow.com/a/9416535 seams to be a nice solution too, but might cause performance issues on big lists wrapped by collection views and also i wouldn't be sure if collection views are the only ones not capable of handling multiple items within a single event.

In the specific case of conductors, i don't think raising multiple remove events instead of a single clear would cause problems, at least i'm not aware of a conductor conducting thousands of items.

On a BindableCollection level, i would rather prefer having option for a suitable workaround, than having none. So the described Clearing Event seams to be a good compromise. You have access to the items before they are cleared and the implementation doesn't have any performance impact or possible unwanted side effect.

[nigel-sampson]

In the specific case of conductors, i don't think raising multiple remove events instead of a single clear would cause problems, at least i'm not aware of a conductor conducting thousands of items.

I've had issues with this as some more advanced controls trigger different animations when items are removed and a mass remove like that would be performance issue.

Leaning further towards that custom event at the moment.