Merge pull request #3707 from smoogipoo/fix-rearrangeablelist-multi-a…

…ddition
ppy · Jul 10, 2020 · 11f71dc · 11f71dc
2 parents 0c3b3a6 + 1e6a66c
commit 11f71dc
Show file tree

Hide file tree

Showing 2 changed files with 44 additions and 2 deletions.
diff --git a/osu.Framework.Tests/Visual/UserInterface/TestSceneRearrangeableListContainer.cs b/osu.Framework.Tests/Visual/UserInterface/TestSceneRearrangeableListContainer.cs
@@ -4,7 +4,9 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Threading;
 using NUnit.Framework;
+using osu.Framework.Allocation;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
@@ -248,6 +250,21 @@ public void TestNotScrolledToTopOnRemove()
             AddAssert("scroll hasn't changed", () => list.ScrollPosition == scrollPosition);
         }
 
+        [Test]
+        public void TestRemoveDuringLoadAndReAdd()
+        {
+            TestDelayedLoadRearrangeableList delayedList = null;
+
+            AddStep("create list", () => Child = delayedList = new TestDelayedLoadRearrangeableList());
+
+            AddStep("add item 1", () => delayedList.Items.Add(1));
+            AddStep("remove item 1", () => delayedList.Items.Remove(1));
+            AddStep("add item 1", () => delayedList.Items.Add(1));
+            AddStep("allow load", () => delayedList.AllowLoad.Release(100));
+
+            AddAssert("only one item", () => delayedList.ChildrenOfType<BasicRearrangeableListItem<int>>().Count() == 1);
+        }
+
         private void addDragSteps(int from, int to, int[] expectedSequence)
         {
             AddStep($"move to {from}", () =>
@@ -304,5 +321,30 @@ private class TestRearrangeableList : BasicRearrangeableListContainer<int>
             public void ScrollTo(int item)
                 => ScrollContainer.ScrollTo(this.ChildrenOfType<BasicRearrangeableListItem<int>>().First(i => i.Model == item), false);
         }
+
+        private class TestDelayedLoadRearrangeableList : BasicRearrangeableListContainer<int>
+        {
+            public readonly SemaphoreSlim AllowLoad = new SemaphoreSlim(0, 100);
+
+            protected override BasicRearrangeableListItem<int> CreateBasicItem(int item) => new TestRearrangeableListItem(item, AllowLoad);
+
+            private class TestRearrangeableListItem : BasicRearrangeableListItem<int>
+            {
+                private readonly SemaphoreSlim allowLoad;
+
+                public TestRearrangeableListItem(int item, SemaphoreSlim allowLoad)
+                    : base(item, false)
+                {
+                    this.allowLoad = allowLoad;
+                }
+
+                [BackgroundDependencyLoader]
+                private void load()
+                {
+                    if (!allowLoad.Wait(TimeSpan.FromSeconds(10)))
+                        throw new TimeoutException();
+                }
+            }
+        }
     }
 }
diff --git a/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs b/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs
@@ -149,8 +149,8 @@ void addToHierarchy(IEnumerable<Drawable> drawables)
             {
                 foreach (var d in drawables.Cast<RearrangeableListItem<TModel>>())
                 {
-                    // We shouldn't add items that were removed during the async load
-                    if (itemMap.ContainsKey(d.Model))
+                    // Don't add drawables whose models were removed during the async load, or drawables that are no longer attached to the contained model.
+                    if (itemMap.TryGetValue(d.Model, out var modelDrawable) && modelDrawable == d)
                         ListContainer.Add(d);
                 }