Merge pull request #980 from smoogipooo/fix-container-contains

Fix SortedList<T>.IndexOf(T) returning a valid value for an item not contained by it, instead of -1.

osu.Framework.Desktop.Tests/Visual/TestCaseContainerState.cs

@@ -0,0 +1,125 @@
+// Copyright (c) 2007-2017 ppy Pty Ltd <contact@ppy.sh>.
+// Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu-framework/master/LICENCE
+
+using System;
+using System.Linq;
+using NUnit.Framework;
+using osu.Framework.Allocation;
+using osu.Framework.Graphics.Containers;
+using osu.Framework.Graphics.Sprites;
+using osu.Framework.Testing;
+
+namespace osu.Framework.Desktop.Tests.Visual
+{
+    [TestFixture]
+    public class TestCaseContainerState : TestCase
+    {
+        public override string Description => "Ensuring a container's state is consistent in various scenarios.";
+
+        private readonly Container container;
+
+        public TestCaseContainerState()
+        {
+            Add(container = new Container());
+        }
+
+        [BackgroundDependencyLoader]
+        private void load()
+        {
+            testLoadedMultipleAdds();
+        }
+
+        /// <summary>
+        /// Tests if a drawable can be added to a container, removed, and then re-added to the same container.
+        /// </summary>
+        [Test]
+        public void TestPreLoadReAdding()
+        {
+            var sprite = new Sprite();
+
+            // Add
+            Assert.DoesNotThrow(() => container.Add(sprite));
+            Assert.IsTrue(container.Contains(sprite));
+
+            // Remove
+            Assert.DoesNotThrow(() => container.Remove(sprite));
+            Assert.IsFalse(container.Contains(sprite));
+
+            // Re-add
+            Assert.DoesNotThrow(() => container.Add(sprite));
+            Assert.IsTrue(container.Contains(sprite));
+        }
+
+        /// <summary>
+        /// Tests whether adding a child to multiple containers by abusing <see cref="Container{T}.Children"/>
+        /// results in a <see cref="InvalidOperationException"/>.
+        /// </summary>
+        [Test]
+        public void TestPreLoadMultipleAdds()
+        {
+            // Non-async
+            Assert.Throws<InvalidOperationException>(() =>
+            {
+                container.Add(new Container
+                {
+                    // Container is an IReadOnlyList<T>, so Children can accept a Container.
+                    // This further means that CompositeDrawable.AddInternal will try to add all of
+                    // the children of the Container that was set to Children, which should throw an exception
+                    Children = new Container { Child = new Container() }
+                });
+            });
+        }
+
+        /// <summary>
+        /// The same as <see cref="TestPreLoadMultipleAdds"/> however instead runs after the container is loaded.
+        /// </summary>
+        private void testLoadedMultipleAdds()
+        {
+            AddAssert("Test loaded multiple adds", () =>
+            {
+                try
+                {
+                    container.Add(new Container
+                    {
+                        // Container is an IReadOnlyList<T>, so Children can accept a Container.
+                        // This further means that CompositeDrawable.AddInternal will try to add all of
+                        // the children of the Container that was set to Children, which should throw an exception
+                        Children = new Container { Child = new Container() }
+                    });
+
+                    return false;
+                }
+                catch (InvalidOperationException)
+                {
+                    return true;
+                }
+            });
+        }
+
+        /// <summary>
+        /// Tests whether the result of a <see cref="Container{T}.Contains(T)"/> operation is valid between multiple containers.
+        /// This tests whether the comparator + equality operation in <see cref="CompositeDrawable.IndexOfInternal(Graphics.Drawable)"/> is valid.
+        /// </summary>
+        [Test]
+        public void TestContainerContains()
+        {
+            var drawableA = new Sprite();
+            var drawableB = new Sprite();
+            var containerA = new Container { Child = drawableA };
+            var containerB = new Container { Child = drawableB };
+
+            var newContainer = new Container<Container> { Children = new[] { containerA, containerB } };
+
+            // Because drawableA and drawableB have been added to separate containers,
+            // they will both have Depth = 0 and ChildID = 1, which leads to edge cases if a
+            // sorting comparer that doesn't compare references is used for Contains().
+            // If this is not handled properly, it may have devastating effects in, e.g. Remove().
+
+            Assert.IsTrue(newContainer.First(c => c.Contains(drawableA)) == containerA);
+            Assert.IsTrue(newContainer.First(c => c.Contains(drawableB)) == containerB);
+
+            Assert.DoesNotThrow(() => newContainer.First(c => c.Contains(drawableA)).Remove(drawableA));
+            Assert.DoesNotThrow(() => newContainer.First(c => c.Contains(drawableB)).Remove(drawableB));
+        }
+    }
+}

osu.Framework.Desktop.Tests/osu.Framework.Desktop.Tests.csproj

@@ -71,7 +71,7 @@
     <Compile Include="Visual\TestCaseLocalisation.cs" />
     <Compile Include="Visual\TestCaseMasking.cs" />
     <Compile Include="Visual\TestCaseNestedHover.cs" />
-    <Compile Include="Visual\TestCaseNonAsyncContainer.cs" />
+    <Compile Include="Visual\TestCaseContainerState.cs" />
     <Compile Include="Visual\TestCaseOnlineTextures.cs" />
     <Compile Include="Visual\TestCasePadding.cs" />
     <Compile Include="Visual\TestCaseRigidBody.cs" />

osu.Framework/Graphics/Containers/CompositeDrawable.cs

@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System;
 using System.Diagnostics;
+using System.Threading;
 using OpenTK;
 using osu.Framework.Graphics.Primitives;
 using osu.Framework.Graphics.OpenGL;
@@ -276,7 +277,7 @@ protected internal void ClearInternal(bool disposeChildren = true)
         /// Used to assign a monotonically increasing ID to children as they are added. This member is
         /// incremented whenever a child is added.
         /// </summary>
-        private ulong currentChildID;
+        private static long currentChildID;
 
         /// <summary>
         /// Adds a child to <see cref="InternalChildren"/>.
@@ -293,7 +294,7 @@ protected internal virtual void AddInternal(Drawable drawable)
             if (drawable.ChildID != 0)
                 throw new InvalidOperationException("May not add a drawable to multiple containers.");
 
-            drawable.ChildID = ++currentChildID;
+            drawable.ChildID = Interlocked.Increment(ref currentChildID);
 
             if (drawable.LoadState >= LoadState.Ready)
                 drawable.Parent = this;

osu.Framework/Graphics/Drawable.cs

@@ -234,7 +234,7 @@ protected virtual void LoadComplete()
         /// ID is unique within the <see cref="Parent"/> <see cref="CompositeDrawable"/>.
         /// The primary use case of this ID is stable sorting of Drawables with equal <see cref="Depth"/>.
         /// </summary>
-        public ulong ChildID { get; internal set; }
+        public long ChildID { get; internal set; }
 
         /// <summary>
         /// Whether this drawable has been added to a parent <see cref="CompositeDrawable"/>. Note that this does NOT imply that

osu.Framework/Lists/SortedList.cs

@@ -24,7 +24,7 @@ public class SortedList<T> : ICollection<T>, IReadOnlyList<T>
             set { list[index] = value; }
         }
 
-        public SortedList(Func<T,T,int> comparer) : this(new ComparisonComparer<T>(comparer))
+        public SortedList(Func<T, T, int> comparer) : this(new ComparisonComparer<T>(comparer))
         {
         }