diff --git a/osu.Framework.Tests/Bindables/BindableListTest.cs b/osu.Framework.Tests/Bindables/BindableListTest.cs index 9df3106dd9..e5225e4ea6 100644 --- a/osu.Framework.Tests/Bindables/BindableListTest.cs +++ b/osu.Framework.Tests/Bindables/BindableListTest.cs @@ -377,6 +377,26 @@ public void TestAddWithListContainingItemsDoesNotOverrideItems(string str) Assert.Contains(item, bindableStringList); } + [Test] + public void TestAddBranchingBinds() + { + var b1 = new BindableList { 1, 2, 3, 4, 5 }; + + var b2 = b1.GetBoundCopy(); + var b3 = b1.GetBoundCopy(); + + var b4 = new BindableList(); + b4.BindTo(b2); + b4.BindTo(b3); + + b1.Add(6); + + Assert.That(b1.Count, Is.EqualTo(6)); + Assert.That(b2.Count, Is.EqualTo(6)); + Assert.That(b3.Count, Is.EqualTo(6)); + Assert.That(b4.Count, Is.EqualTo(6)); + } + #endregion #region .AddRange(items) @@ -435,6 +455,26 @@ IEnumerable valueEnumerable() Assert.That(counter, Is.EqualTo(1)); } + [Test] + public void TestAddRangeBranchingBinds() + { + var b1 = new BindableList { 1, 2, 3, 4, 5 }; + + var b2 = b1.GetBoundCopy(); + var b3 = b1.GetBoundCopy(); + + var b4 = new BindableList(); + b4.BindTo(b2); + b4.BindTo(b3); + + b1.AddRange(new[] { 6, 7 }); + + Assert.That(b1.Count, Is.EqualTo(7)); + Assert.That(b2.Count, Is.EqualTo(7)); + Assert.That(b3.Count, Is.EqualTo(7)); + Assert.That(b4.Count, Is.EqualTo(7)); + } + #endregion #region .Move(item) @@ -574,6 +614,28 @@ public void TestMoveNotifiesBoundListSubscriptions() Assert.That(triggeredArgsB2, Is.Not.Null); } + [Test] + public void TestMoveRangeBranchingBinds() + { + var b1 = new BindableList { 1, 2, 3, 4, 5 }; + + var b2 = b1.GetBoundCopy(); + var b3 = b1.GetBoundCopy(); + + var b4 = new BindableList(); + + b4.BindTo(b2); + b4.BindTo(b3); + + b1.Move(0, 1); + + foreach (var list in new[] { b1, b2, b3, b4 }) + { + Assert.That(list[0], Is.EqualTo(2)); + Assert.That(list[1], Is.EqualTo(1)); + } + } + #endregion #region .Insert @@ -648,6 +710,26 @@ public void TestInsertInsertsItemAtIndexInBoundList() }); } + [Test] + public void TestInsertBranchingBinds() + { + var b1 = new BindableList { 1, 2, 3, 4, 5 }; + + var b2 = b1.GetBoundCopy(); + var b3 = b1.GetBoundCopy(); + + var b4 = new BindableList(); + b4.BindTo(b2); + b4.BindTo(b3); + + b1.Insert(0, 0); + + Assert.That(b1.Count, Is.EqualTo(6)); + Assert.That(b2.Count, Is.EqualTo(6)); + Assert.That(b3.Count, Is.EqualTo(6)); + Assert.That(b4.Count, Is.EqualTo(6)); + } + #endregion #region .Remove(item) @@ -995,6 +1077,21 @@ public void TestRemoveAtNotifiesBoundLists() Assert.That(triggeredArgs.OldStartingIndex, Is.EqualTo(0)); } + [Test] + public void TestRemoveAtBranchingBinds() + { + var b1 = new BindableList { 1, 2, 3, 4, 5 }; + + var b2 = b1.GetBoundCopy(); + var b3 = b1.GetBoundCopy(); + + var b4 = new BindableList(); + b4.BindTo(b2); + b4.BindTo(b3); + + b1.RemoveAt(b1.Count - 1); + } + #endregion #region .RemoveAll(match) @@ -1349,7 +1446,7 @@ public void TestDisabledNotifiesBoundLists() #endregion - #region .GetEnumberator() + #region .GetEnumerator() [Test] public void TestGetEnumeratorDoesNotReturnNull() diff --git a/osu.Framework/Bindables/BindableList.cs b/osu.Framework/Bindables/BindableList.cs index 5f5095acb2..e904381135 100644 --- a/osu.Framework/Bindables/BindableList.cs +++ b/osu.Framework/Bindables/BindableList.cs @@ -54,11 +54,13 @@ public BindableList(IEnumerable items = null) public T this[int index] { get => collection[index]; - set => setIndex(index, value, null); + set => setIndex(index, value, new HashSet>()); } - private void setIndex(int index, T item, BindableList caller) + private void setIndex(int index, T item, HashSet> appliedInstances) { + if (checkAlreadyApplied(appliedInstances)) return; + ensureMutationAllowed(); T lastItem = collection[index]; @@ -68,12 +70,7 @@ private void setIndex(int index, T item, BindableList caller) if (bindings != null) { foreach (var b in bindings) - { - // prevent re-adding the item back to the callee. - // That would result in a . - if (b != caller) - b.setIndex(index, item, this); - } + b.setIndex(index, item, appliedInstances); } notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, item, lastItem, index)); @@ -85,10 +82,12 @@ private void setIndex(int index, T item, BindableList caller) /// The item to be added. /// Thrown when this is . public void Add(T item) - => add(item, null); + => add(item, new HashSet>()); - private void add(T item, BindableList caller) + private void add(T item, HashSet> appliedInstances) { + if (checkAlreadyApplied(appliedInstances)) return; + ensureMutationAllowed(); collection.Add(item); @@ -96,12 +95,7 @@ private void add(T item, BindableList caller) if (bindings != null) { foreach (var b in bindings) - { - // prevent re-adding the item back to the callee. - // That would result in a . - if (b != caller) - b.add(item, this); - } + b.add(item, appliedInstances); } notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item, collection.Count - 1)); @@ -121,10 +115,12 @@ private void add(T item, BindableList caller) /// The item to insert. /// Thrown when this is . public void Insert(int index, T item) - => insert(index, item, null); + => insert(index, item, new HashSet>()); - private void insert(int index, T item, BindableList caller) + private void insert(int index, T item, HashSet> appliedInstances) { + if (checkAlreadyApplied(appliedInstances)) return; + ensureMutationAllowed(); collection.Insert(index, item); @@ -132,12 +128,7 @@ private void insert(int index, T item, BindableList caller) if (bindings != null) { foreach (var b in bindings) - { - // prevent re-adding the item back to the callee. - // That would result in a . - if (b != caller) - b.insert(index, item, this); - } + b.insert(index, item, appliedInstances); } notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item, index)); @@ -148,10 +139,12 @@ private void insert(int index, T item, BindableList caller) /// /// Thrown when this is . public void Clear() - => clear(null); + => clear(new HashSet>()); - private void clear(BindableList caller) + private void clear(HashSet> appliedInstances) { + if (checkAlreadyApplied(appliedInstances)) return; + ensureMutationAllowed(); if (collection.Count <= 0) @@ -165,12 +158,7 @@ private void clear(BindableList caller) if (bindings != null) { foreach (var b in bindings) - { - // prevent re-adding the item back to the callee. - // That would result in a . - if (b != caller) - b.clear(this); - } + b.clear(appliedInstances); } notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, clearedItems, 0)); @@ -191,10 +179,13 @@ public bool Contains(T item) /// true if the removal was successful. /// Thrown if this is . public bool Remove(T item) - => remove(item, null); + => remove(item, new HashSet>()); - private bool remove(T item, BindableList caller) + private bool remove(T item, HashSet> appliedInstances) { + if (checkAlreadyApplied(appliedInstances)) + return false; + ensureMutationAllowed(); int index = collection.IndexOf(item); @@ -214,8 +205,7 @@ private bool remove(T item, BindableList caller) { // prevent re-removing from the callee. // That would result in a . - if (b != caller) - b.remove(listItem, this); + b.remove(listItem, appliedInstances); } } @@ -231,11 +221,13 @@ private bool remove(T item, BindableList caller) /// The count of items to be removed. public void RemoveRange(int index, int count) { - removeRange(index, count, null); + removeRange(index, count, new HashSet>()); } - private void removeRange(int index, int count, BindableList caller) + private void removeRange(int index, int count, HashSet> appliedInstances) { + if (checkAlreadyApplied(appliedInstances)) return; + ensureMutationAllowed(); var removedItems = collection.GetRange(index, count); @@ -251,8 +243,7 @@ private void removeRange(int index, int count, BindableList caller) { // Prevent re-adding the item back to the callee. // That would result in a . - if (b != caller) - b.removeRange(index, count, this); + b.removeRange(index, count, appliedInstances); } } @@ -265,10 +256,12 @@ private void removeRange(int index, int count, BindableList caller) /// The index of the item to remove. /// Thrown if this is . public void RemoveAt(int index) - => removeAt(index, null); + => removeAt(index, new HashSet>()); - private void removeAt(int index, BindableList caller) + private void removeAt(int index, HashSet> appliedInstances) { + if (checkAlreadyApplied(appliedInstances)) return; + ensureMutationAllowed(); T item = collection[index]; @@ -278,12 +271,7 @@ private void removeAt(int index, BindableList caller) if (bindings != null) { foreach (var b in bindings) - { - // prevent re-adding the item back to the callee. - // That would result in a . - if (b != caller) - b.removeAt(index, this); - } + b.removeAt(index, appliedInstances); } notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, item, index)); @@ -294,10 +282,13 @@ private void removeAt(int index, BindableList caller) /// /// The predicate. public int RemoveAll(Predicate match) - => removeAll(match, null); + => removeAll(match, new HashSet>()); - private int removeAll(Predicate match, BindableList caller) + private int removeAll(Predicate match, HashSet> appliedInstances) { + if (checkAlreadyApplied(appliedInstances)) + return 0; + ensureMutationAllowed(); var removed = collection.FindAll(match); @@ -310,12 +301,7 @@ private int removeAll(Predicate match, BindableList caller) if (bindings != null) { foreach (var b in bindings) - { - // prevent re-adding the item back to the callee. - // That would result in a . - if (b != caller) - b.removeAll(match, this); - } + b.removeAll(match, appliedInstances); } notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, removed)); @@ -330,10 +316,12 @@ private int removeAll(Predicate match, BindableList caller) /// The count of items to be removed. /// The items to replace the removed items with. public void ReplaceRange(int index, int count, IEnumerable newItems) - => replaceRange(index, count, newItems as IList ?? newItems.ToArray(), null); + => replaceRange(index, count, newItems as IList ?? newItems.ToArray(), new HashSet>()); - private void replaceRange(int index, int count, IList newItems, BindableList caller) + private void replaceRange(int index, int count, IList newItems, HashSet> appliedInstances) { + if (checkAlreadyApplied(appliedInstances)) return; + ensureMutationAllowed(); var removedItems = collection.GetRange(index, count); @@ -350,14 +338,26 @@ private void replaceRange(int index, int count, IList newItems, BindableList { // Prevent re-adding the item back to the callee. // That would result in a . - if (b != caller) - b.replaceRange(index, count, newItems, this); + b.replaceRange(index, count, newItems, appliedInstances); } } notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, newItems, removedItems, index)); } + /// + /// As we are applying non-atomic operations on a potentially complex bindable tree, care needs to be taken to not apply + /// the same operation twice to the same bindable instance. + /// + /// This call tracks all instances which an operation has already been applied to. + /// + /// A hash set to be passed down the recursive call tree tracking all applied instances. + /// Whether the current instance has already been applied. + private bool checkAlreadyApplied(HashSet> appliedInstances) + { + return !appliedInstances.Add(this); + } + /// /// Copies the contents of this to the given array, starting at the given index. /// @@ -538,10 +538,12 @@ public virtual void UnbindFrom(IUnbindable them) /// The collection whose items should be added to this collection. /// Thrown if this collection is public void AddRange(IEnumerable items) - => addRange(items as IList ?? items.ToArray(), null); + => addRange(items as IList ?? items.ToArray(), new HashSet>()); - private void addRange(IList items, BindableList caller) + private void addRange(IList items, HashSet> appliedInstances) { + if (checkAlreadyApplied(appliedInstances)) return; + ensureMutationAllowed(); collection.AddRange(items.Cast()); @@ -549,12 +551,7 @@ private void addRange(IList items, BindableList caller) if (bindings != null) { foreach (var b in bindings) - { - // prevent re-adding the item back to the callee. - // That would result in a . - if (b != caller) - b.addRange(items, this); - } + b.addRange(items, appliedInstances); } notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, items, collection.Count - items.Count)); @@ -566,10 +563,12 @@ private void addRange(IList items, BindableList caller) /// The index of the item to move. /// The index specifying the new location of the item. public void Move(int oldIndex, int newIndex) - => move(oldIndex, newIndex, null); + => move(oldIndex, newIndex, new HashSet>()); - private void move(int oldIndex, int newIndex, BindableList caller) + private void move(int oldIndex, int newIndex, HashSet> appliedInstances) { + if (checkAlreadyApplied(appliedInstances)) return; + ensureMutationAllowed(); T item = collection[oldIndex]; @@ -580,12 +579,7 @@ private void move(int oldIndex, int newIndex, BindableList caller) if (bindings != null) { foreach (var b in bindings) - { - // prevent re-adding the item back to the callee. - // That would result in a . - if (b != caller) - b.move(oldIndex, newIndex, this); - } + b.move(oldIndex, newIndex, appliedInstances); } notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Move, item, newIndex, oldIndex));