From a1760731d0570fb78fb7bde400158376795d0577 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Fri, 15 Nov 2024 17:18:08 +0900 Subject: [PATCH 1/3] Add more tests, one failing with new expectations --- .../TestSceneRearrangeableListContainer.cs | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/osu.Framework.Tests/Visual/UserInterface/TestSceneRearrangeableListContainer.cs b/osu.Framework.Tests/Visual/UserInterface/TestSceneRearrangeableListContainer.cs index 9c3ede1100..9b907e1755 100644 --- a/osu.Framework.Tests/Visual/UserInterface/TestSceneRearrangeableListContainer.cs +++ b/osu.Framework.Tests/Visual/UserInterface/TestSceneRearrangeableListContainer.cs @@ -342,6 +342,82 @@ public void TestPartialReplace() AddUntilStep("wait for items to load", () => list.ItemMap.Values.All(i => i.IsLoaded)); } + [Test] + public void TestReplaceWithNoItems() + { + addItems(5); + + AddStep("replace list", () => list.Items.ReplaceRange(0, list.Items.Count, [])); + AddUntilStep("wait for clear", () => !list.ItemMap.Values.Any()); + } + + [Test] + public void TestReplaceEmptyListWithNoItems() + { + AddStep("replace list", () => list.Items.ReplaceRange(0, 0, [])); + } + + [Test] + public void TestReplaceEmptyListWithItems() + { + AddStep("replace list", () => list.Items.ReplaceRange(0, 0, [100, 101])); + AddUntilStep("wait for items to load", () => list.ItemMap.Values.All(i => i.IsLoaded)); + } + + [Test] + public void TestReplaceOnlyAppliesDifferences() + { + // ReSharper disable once LocalVariableHidesMember (intentional, using the ƒield is wrong for this test) + TestLoadCountingList list = null!; + + AddStep("create list", () => Child = list = new TestLoadCountingList + { + Anchor = Anchor.Centre, + Origin = Anchor.Centre, + Size = new Vector2(500, 300) + }); + + AddStep("replace {[]} with [1]", () => list.Items.ReplaceRange(0, list.Items.Count, [1])); + assertLoadCount(1, 1); + + AddStep("replace {[1]} with [1]", () => list.Items.ReplaceRange(0, 1, [1])); + assertLoadCount(1, 1); + + AddStep("add {1, []} with [2, 3, 4]", () => list.Items.ReplaceRange(list.Items.Count, 0, [2, 3, 4])); + assertLoadCount(2, 1); + assertLoadCount(3, 1); + assertLoadCount(4, 1); + + AddStep("replace {1, [2, 3], 4} with [3, 5]", () => list.Items.ReplaceRange(1, 2, [3, 5])); + assertLoadCount(3, 1); + assertLoadCount(5, 1); + + AddStep("replace {1, [3, 5], 4} with [2, 3]", () => list.Items.ReplaceRange(1, 2, [2, 3])); + assertLoadCount(2, 2); + assertLoadCount(3, 1); + + AddStep("replace {[1, 2, 3, 4]} with [1]", () => list.Items.ReplaceRange(0, list.Items.Count, [1])); + assertLoadCount(1, 1); + + AddStep("replace {[1]} with []", () => list.Items.ReplaceRange(0, list.Items.Count, [])); + assertLoadCount(1, 1); + + AddStep("replace {[]} with [0, 1, 2, 3, 4, 5, 6]", () => list.Items.ReplaceRange(0, list.Items.Count, [0, 1, 2, 3, 4, 5, 6])); + assertLoadCount(0, 1); + assertLoadCount(1, 2); + assertLoadCount(2, 3); + assertLoadCount(3, 2); + assertLoadCount(4, 2); + assertLoadCount(5, 2); + assertLoadCount(6, 1); + + void assertLoadCount(int item, int expectedTimesLoaded) + { + AddUntilStep("wait for items to load", () => list.ItemMap.Values.All(i => i.IsLoaded)); + AddAssert($"item {item} loaded {expectedTimesLoaded} times", () => list.LoadCount[item], () => Is.EqualTo(expectedTimesLoaded)); + } + } + private void addDragSteps(int from, int to, int[] expectedSequence) { AddStep($"move to {from}", () => @@ -425,5 +501,32 @@ private void load() } } } + + private partial class TestLoadCountingList : BasicRearrangeableListContainer + { + /// + /// Dictionary of item -> # of times a drawable was loaded for it. + /// + public readonly Dictionary LoadCount = new Dictionary(); + + public new IReadOnlyDictionary> ItemMap => base.ItemMap; + + protected override BasicRearrangeableListItem CreateBasicItem(int item) + => new TestRearrangeableListItem(item, () => LoadCount[item] = LoadCount.GetValueOrDefault(item) + 1); + + private partial class TestRearrangeableListItem : BasicRearrangeableListItem + { + private readonly Action onLoad; + + public TestRearrangeableListItem(int item, Action onLoad) + : base(item, false) + { + this.onLoad = onLoad; + } + + [BackgroundDependencyLoader] + private void load() => onLoad(); + } + } } } From abd8aab4cde8499ac7d2a9d29ecd81bd22e08fc8 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Fri, 15 Nov 2024 17:48:03 +0900 Subject: [PATCH 2/3] Make `RearrangeableListContainer<>` only replace differences --- .../Containers/RearrangeableListContainer.cs | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs b/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs index b133534c31..83a07b6ebd 100644 --- a/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs +++ b/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs @@ -4,7 +4,6 @@ #nullable disable using System; -using System.Collections; using System.Collections.Generic; using System.Collections.Specialized; using System.Linq; @@ -87,11 +86,11 @@ private void collectionChanged(object sender, NotifyCollectionChangedEventArgs e switch (e.Action) { case NotifyCollectionChangedAction.Add: - addItems(e.NewItems); + addItems(e.NewItems?.Cast() ?? []); break; case NotifyCollectionChangedAction.Remove: - removeItems(e.OldItems); + removeItems(e.OldItems?.Cast() ?? []); // Explicitly reset scroll position here so that ScrollContainer doesn't retain our // scroll position if we quickly add new items after calling a Clear(). @@ -107,8 +106,11 @@ private void collectionChanged(object sender, NotifyCollectionChangedEventArgs e break; case NotifyCollectionChangedAction.Replace: - removeItems(e.OldItems); - addItems(e.NewItems); + IEnumerable tOldItems = e.OldItems?.Cast() ?? []; + IEnumerable tNewItems = e.NewItems?.Cast() ?? []; + + removeItems(tOldItems.Except(tNewItems)); + addItems(tNewItems.Except(tOldItems)); break; case NotifyCollectionChangedAction.Move: @@ -118,9 +120,9 @@ private void collectionChanged(object sender, NotifyCollectionChangedEventArgs e } } - private void removeItems(IList items) + private void removeItems(IEnumerable items) { - foreach (var item in items.Cast()) + foreach (var item in items) { if (currentlyDraggedItem != null && EqualityComparer.Default.Equals(currentlyDraggedItem.Model, item)) currentlyDraggedItem = null; @@ -137,11 +139,11 @@ private void removeItems(IList items) OnItemsChanged(); } - private void addItems(IList items) + private void addItems(IEnumerable items) { var drawablesToAdd = new List(); - foreach (var item in items.Cast()) + foreach (var item in items) { if (itemMap.ContainsKey(item)) { From c383b67b2832fa7666a6f20fcf0e5fa33bf65b13 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Fri, 15 Nov 2024 17:50:28 +0900 Subject: [PATCH 3/3] Enable NRT for `RearrangeableListContainer<>` --- .../Containers/RearrangeableListContainer.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs b/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs index 83a07b6ebd..ebef10929e 100644 --- a/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs +++ b/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs @@ -1,8 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - using System; using System.Collections.Generic; using System.Collections.Specialized; @@ -21,6 +19,7 @@ namespace osu.Framework.Graphics.Containers /// /// The type of rearrangeable item. public abstract partial class RearrangeableListContainer : CompositeDrawable + where TModel : notnull { private const float exp_base = 1.05f; @@ -50,7 +49,7 @@ public abstract partial class RearrangeableListContainer : CompositeDraw protected IReadOnlyDictionary> ItemMap => itemMap; private readonly Dictionary> itemMap = new Dictionary>(); - private RearrangeableListItem currentlyDraggedItem; + private RearrangeableListItem? currentlyDraggedItem; private Vector2 screenSpaceDragPosition; /// @@ -81,7 +80,7 @@ protected virtual void OnItemsChanged() { } - private void collectionChanged(object sender, NotifyCollectionChangedEventArgs e) + private void collectionChanged(object? sender, NotifyCollectionChangedEventArgs e) { switch (e.Action) { @@ -193,7 +192,7 @@ private void sortItems() if (drawable.Parent != ListContainer) continue; - ListContainer!.SetLayoutPosition(drawable, i); + ListContainer.SetLayoutPosition(drawable, i); } } @@ -218,9 +217,7 @@ protected override void Update() protected override void UpdateAfterChildren() { base.UpdateAfterChildren(); - - if (currentlyDraggedItem != null) - updateArrangement(); + updateArrangement(); } private void updateScrollPosition() @@ -245,6 +242,9 @@ private void updateScrollPosition() private void updateArrangement() { + if (currentlyDraggedItem == null) + return; + var localPos = ListContainer.ToLocalSpace(screenSpaceDragPosition); int srcIndex = Items.IndexOf(currentlyDraggedItem.Model);