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

Fix potential duplicate items in RearrangeableListContainer #3707

Merged
merged 2 commits into from
Jul 10, 2020
Merged

Fix potential duplicate items in RearrangeableListContainer #3707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4bf4917
Fix potential duplicate items in RearrangeableListContainer
smoogipoo Jul 10, 2020
1e6a66c
Merge branch 'master' into fix-rearrangeablelist-multi-addition
peppy Jul 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 42 additions & 0 deletions osu.Framework.Tests/Visual/UserInterface/TestSceneRearrangeableListContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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}", () =>
Expand Down Expand Up @@ -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();
}
}
}
}
}
4 changes: 2 additions & 2 deletions osu.Framework/Graphics/Containers/RearrangeableListContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down