Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
tklingert opened this issue Aug 5, 2017 · 6 comments · May be fixed by #637
Open

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

tklingert opened this issue Aug 5, 2017 · 6 comments · May be fixed by #637
Labels
bug

Comments

@tklingert
Copy link

tklingert commented Aug 5, 2017 •
edited 

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 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
Copy link
Contributor

Definitely sounds like a bug, thanks for raising it.

@nigel-sampson nigel-sampson added this to the v3.2.0 milestone Sep 11, 2017
@nigel-sampson
Copy link
Contributor

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.

@nigel-sampson nigel-sampson removed this from the v3.2.0 milestone Sep 12, 2017
@tklingert
Copy link
Author

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 is used to store the items.

@nigel-sampson
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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.

@adamgauthier adamgauthier linked a pull request Sep 24, 2019 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Unset Parent on Conductors' Items.Clear() adamgauthier/Caliburn.Micro
2 participants
@nigel-sampson @tklingert