From 514bea5c12ff013a529c0dc0b6e7cdd258fc7589 Mon Sep 17 00:00:00 2001 From: Roman Kapustin Date: Fri, 14 Feb 2020 22:08:44 +0300 Subject: [PATCH 01/15] Add TestSceneIgnore --- .../Visual/Testing/TestSceneIgnore.cs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 osu.Framework.Tests/Visual/Testing/TestSceneIgnore.cs diff --git a/osu.Framework.Tests/Visual/Testing/TestSceneIgnore.cs b/osu.Framework.Tests/Visual/Testing/TestSceneIgnore.cs new file mode 100644 index 000000000..bdf9dceeb --- /dev/null +++ b/osu.Framework.Tests/Visual/Testing/TestSceneIgnore.cs @@ -0,0 +1,18 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System; +using NUnit.Framework; + +namespace osu.Framework.Tests.Visual.Testing +{ + public class TestSceneIgnore : FrameworkTestScene + { + [Test] + [Ignore("test")] + public void IgnoredTest() + { + AddStep($"Throw {typeof(InvalidOperationException)}", () => throw new InvalidOperationException()); + } + } +} From 890ebad693a77f5213b0cc45c82add2a65f4608f Mon Sep 17 00:00:00 2001 From: Roman Kapustin Date: Fri, 14 Feb 2020 22:25:52 +0300 Subject: [PATCH 02/15] Add IgnoredParameterizedTest --- osu.Framework.Tests/Visual/Testing/TestSceneIgnore.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/osu.Framework.Tests/Visual/Testing/TestSceneIgnore.cs b/osu.Framework.Tests/Visual/Testing/TestSceneIgnore.cs index bdf9dceeb..e9662a22f 100644 --- a/osu.Framework.Tests/Visual/Testing/TestSceneIgnore.cs +++ b/osu.Framework.Tests/Visual/Testing/TestSceneIgnore.cs @@ -14,5 +14,13 @@ namespace osu.Framework.Tests.Visual.Testing { AddStep($"Throw {typeof(InvalidOperationException)}", () => throw new InvalidOperationException()); } + + [TestCase(1)] + [TestCase(2)] + [Ignore("test")] + public void IgnoredParameterizedTest(int test) + { + AddStep($"Throw {typeof(InvalidOperationException)}", () => throw new InvalidOperationException()); + } } } From 106f652935d429a1df32effff49a163c242c101c Mon Sep 17 00:00:00 2001 From: Roman Kapustin Date: Fri, 14 Feb 2020 22:27:07 +0300 Subject: [PATCH 03/15] Handle IgnoreAttribute in TestBrowser --- osu.Framework/Testing/TestBrowser.cs | 33 +++++++++++++++------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/osu.Framework/Testing/TestBrowser.cs b/osu.Framework/Testing/TestBrowser.cs index 2f2ccb864..4eb47aeb9 100644 --- a/osu.Framework/Testing/TestBrowser.cs +++ b/osu.Framework/Testing/TestBrowser.cs @@ -467,28 +467,31 @@ namespace osu.Framework.Testing if (name.StartsWith("Test")) name = name.Substring(4); - if (m.GetCustomAttribute(typeof(TestAttribute), false) != null) + if (m.GetCustomAttribute(typeof(IgnoreAttribute), false) == null) { - hadTestAttributeTest = true; - handleTestMethod(m, name); - - if (m.GetCustomAttribute(typeof(RepeatAttribute), false) != null) + if (m.GetCustomAttribute(typeof(TestAttribute), false) != null) { - var count = (int)m.GetCustomAttributesData().Single(a => a.AttributeType == typeof(RepeatAttribute)).ConstructorArguments.Single().Value; + hadTestAttributeTest = true; + handleTestMethod(m, name); - for (int i = 2; i <= count; i++) - handleTestMethod(m, $"{name} ({i})"); + if (m.GetCustomAttribute(typeof(RepeatAttribute), false) != null) + { + var count = (int)m.GetCustomAttributesData().Single(a => a.AttributeType == typeof(RepeatAttribute)).ConstructorArguments.Single().Value; + + for (int i = 2; i <= count; i++) + handleTestMethod(m, $"{name} ({i})"); + } } - } - foreach (var tc in m.GetCustomAttributes(typeof(TestCaseAttribute), false).OfType()) - { - hadTestAttributeTest = true; - CurrentTest.AddLabel($"{name}({string.Join(", ", tc.Arguments)})"); + foreach (var tc in m.GetCustomAttributes(typeof(TestCaseAttribute), false).OfType()) + { + hadTestAttributeTest = true; + CurrentTest.AddLabel($"{name}({string.Join(", ", tc.Arguments)})"); - addSetUpSteps(); + addSetUpSteps(); - m.Invoke(CurrentTest, tc.Arguments); + m.Invoke(CurrentTest, tc.Arguments); + } } } From 32f9d66de60780305073aee88de34190e56f3b4a Mon Sep 17 00:00:00 2001 From: "dependabot-preview[bot]" <27856297+dependabot-preview[bot]@users.noreply.github.com> Date: Sat, 15 Feb 2020 15:22:02 +0000 Subject: [PATCH 04/15] Bump ppy.osu.Framework.NativeLibs from 2019.1104.0 to 2020.213.0 Bumps [ppy.osu.Framework.NativeLibs](https://github.com/ppy/osu-framework) from 2019.1104.0 to 2020.213.0. - [Release notes](https://github.com/ppy/osu-framework/releases) - [Commits](https://github.com/ppy/osu-framework/compare/2019.1104.0...2020.213.0) Signed-off-by: dependabot-preview[bot] --- osu.Framework/osu.Framework.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Framework/osu.Framework.csproj b/osu.Framework/osu.Framework.csproj index 035337e9a..46d1021b4 100644 --- a/osu.Framework/osu.Framework.csproj +++ b/osu.Framework/osu.Framework.csproj @@ -42,6 +42,6 @@ - + From 7ed2c200c7c0015c6eb5127449bdbf2bf242cf96 Mon Sep 17 00:00:00 2001 From: Roman Kapustin Date: Sun, 16 Feb 2020 21:55:34 +0300 Subject: [PATCH 05/15] Merge TestConstructor and IgnoreAttribute checks --- osu.Framework/Testing/TestBrowser.cs | 35 +++++++++++++--------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/osu.Framework/Testing/TestBrowser.cs b/osu.Framework/Testing/TestBrowser.cs index 4eb47aeb9..79c3df6bd 100644 --- a/osu.Framework/Testing/TestBrowser.cs +++ b/osu.Framework/Testing/TestBrowser.cs @@ -461,37 +461,34 @@ namespace osu.Framework.Testing { var name = m.Name; - if (name == nameof(TestScene.TestConstructor)) + if (name == nameof(TestScene.TestConstructor) || m.GetCustomAttribute(typeof(IgnoreAttribute), false) != null) continue; if (name.StartsWith("Test")) name = name.Substring(4); - if (m.GetCustomAttribute(typeof(IgnoreAttribute), false) == null) + if (m.GetCustomAttribute(typeof(TestAttribute), false) != null) { - if (m.GetCustomAttribute(typeof(TestAttribute), false) != null) + hadTestAttributeTest = true; + handleTestMethod(m, name); + + if (m.GetCustomAttribute(typeof(RepeatAttribute), false) != null) { - hadTestAttributeTest = true; - handleTestMethod(m, name); + var count = (int)m.GetCustomAttributesData().Single(a => a.AttributeType == typeof(RepeatAttribute)).ConstructorArguments.Single().Value; - if (m.GetCustomAttribute(typeof(RepeatAttribute), false) != null) - { - var count = (int)m.GetCustomAttributesData().Single(a => a.AttributeType == typeof(RepeatAttribute)).ConstructorArguments.Single().Value; - - for (int i = 2; i <= count; i++) - handleTestMethod(m, $"{name} ({i})"); - } + for (int i = 2; i <= count; i++) + handleTestMethod(m, $"{name} ({i})"); } + } - foreach (var tc in m.GetCustomAttributes(typeof(TestCaseAttribute), false).OfType()) - { - hadTestAttributeTest = true; - CurrentTest.AddLabel($"{name}({string.Join(", ", tc.Arguments)})"); + foreach (var tc in m.GetCustomAttributes(typeof(TestCaseAttribute), false).OfType()) + { + hadTestAttributeTest = true; + CurrentTest.AddLabel($"{name}({string.Join(", ", tc.Arguments)})"); - addSetUpSteps(); + addSetUpSteps(); - m.Invoke(CurrentTest, tc.Arguments); - } + m.Invoke(CurrentTest, tc.Arguments); } } From 92643ad6c005d0765a2b93c07aba278b2485d791 Mon Sep 17 00:00:00 2001 From: Roman Kapustin Date: Sun, 16 Feb 2020 22:08:48 +0300 Subject: [PATCH 06/15] Replace InvalidOperationException with false-assert --- osu.Framework.Tests/Visual/Testing/TestSceneIgnore.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/osu.Framework.Tests/Visual/Testing/TestSceneIgnore.cs b/osu.Framework.Tests/Visual/Testing/TestSceneIgnore.cs index e9662a22f..2da3100af 100644 --- a/osu.Framework.Tests/Visual/Testing/TestSceneIgnore.cs +++ b/osu.Framework.Tests/Visual/Testing/TestSceneIgnore.cs @@ -1,7 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -using System; using NUnit.Framework; namespace osu.Framework.Tests.Visual.Testing @@ -12,7 +11,7 @@ namespace osu.Framework.Tests.Visual.Testing [Ignore("test")] public void IgnoredTest() { - AddStep($"Throw {typeof(InvalidOperationException)}", () => throw new InvalidOperationException()); + AddAssert("Test ignored", () => false); } [TestCase(1)] @@ -20,7 +19,7 @@ namespace osu.Framework.Tests.Visual.Testing [Ignore("test")] public void IgnoredParameterizedTest(int test) { - AddStep($"Throw {typeof(InvalidOperationException)}", () => throw new InvalidOperationException()); + AddAssert("Test ignored", () => false); } } } From a32dba3636a59e59b07664299718cf47c1cbae8e Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Mon, 17 Feb 2020 11:45:49 +0900 Subject: [PATCH 07/15] Make IBindableList implement INotifyCollectionChanged --- .../Bindables/BindableListTest.cs | 8 +- osu.Framework/Bindables/BindableList.cs | 93 ++++++++++++++----- osu.Framework/Bindables/IBindableList.cs | 3 +- 3 files changed, 75 insertions(+), 29 deletions(-) diff --git a/osu.Framework.Tests/Bindables/BindableListTest.cs b/osu.Framework.Tests/Bindables/BindableListTest.cs index 6386e3242..991ba56ce 100644 --- a/osu.Framework.Tests/Bindables/BindableListTest.cs +++ b/osu.Framework.Tests/Bindables/BindableListTest.cs @@ -126,8 +126,8 @@ namespace osu.Framework.Tests.Bindables bindableStringList[0] = "1"; - Assert.AreEqual(removedItem.Single(), "0"); - Assert.AreEqual(addedItem.Single(), "1"); + Assert.That(removedItem.Single(), Is.EqualTo("0")); + Assert.That(addedItem.Single(), Is.EqualTo("1")); } [Test] @@ -145,8 +145,8 @@ namespace osu.Framework.Tests.Bindables bindableStringList[0] = "1"; - Assert.AreEqual(removedItem.Single(), "0"); - Assert.AreEqual(addedItem.Single(), "1"); + Assert.That(removedItem.Single(), Is.EqualTo("0")); + Assert.That(addedItem.Single(), Is.EqualTo("1")); } #endregion diff --git a/osu.Framework/Bindables/BindableList.cs b/osu.Framework/Bindables/BindableList.cs index 87c985c03..2842ccb01 100644 --- a/osu.Framework/Bindables/BindableList.cs +++ b/osu.Framework/Bindables/BindableList.cs @@ -4,6 +4,7 @@ using System; using System.Collections; using System.Collections.Generic; +using System.Collections.Specialized; using System.Linq; using osu.Framework.Caching; using osu.Framework.Lists; @@ -15,13 +16,20 @@ namespace osu.Framework.Bindables /// /// An event which is raised when any items are added to this . /// + [Obsolete("Use CollectionChanged instead.")] public event Action> ItemsAdded; /// /// An event which is raised when any items are removed from this . /// + [Obsolete("Use CollectionChanged instead.")] public event Action> ItemsRemoved; + /// + /// An event which is raised when this changes. + /// + public event NotifyCollectionChangedEventHandler CollectionChanged; + /// /// An event which is raised when 's state has changed (or manually via ). /// @@ -77,8 +85,7 @@ namespace osu.Framework.Bindables } } - ItemsRemoved?.Invoke(new[] { lastItem }); - ItemsAdded?.Invoke(new[] { item }); + notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, item, lastItem, index)); } /// @@ -106,7 +113,7 @@ namespace osu.Framework.Bindables } } - ItemsAdded?.Invoke(new[] { item }); + notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item, collection.Count - 1)); } /// @@ -142,7 +149,7 @@ namespace osu.Framework.Bindables } } - ItemsAdded?.Invoke(new[] { item }); + notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item, index)); } /// @@ -175,7 +182,12 @@ namespace osu.Framework.Bindables } } +#pragma warning disable 618 // can be removed 20200817 + // The collection changed event does not provide items upon clearing, but the old interface expects it to. ItemsRemoved?.Invoke(clearedItems); +#pragma warning restore 618 + + notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset)); } /// @@ -199,25 +211,27 @@ namespace osu.Framework.Bindables { ensureMutationAllowed(); - bool removed = collection.Remove(item); + int index = collection.IndexOf(item); - if (removed) + if (index < 0) + return false; + + collection.RemoveAt(index); + + if (bindings != null) { - if (bindings != null) + foreach (var b in bindings) { - foreach (var b in bindings) - { - // prevent re-adding the item back to the callee. - // That would result in a . - if (b != caller) - b.remove(item, this); - } + // prevent re-adding the item back to the callee. + // That would result in a . + if (b != caller) + b.remove(item, this); } - - ItemsRemoved?.Invoke(new[] { item }); } - return removed; + notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, item, index)); + + return true; } /// @@ -252,7 +266,7 @@ namespace osu.Framework.Bindables } } - ItemsRemoved?.Invoke(removedItems); + notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, removedItems, index)); } /// @@ -282,7 +296,7 @@ namespace osu.Framework.Bindables } } - ItemsRemoved?.Invoke(new[] { item }); + notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, item, index)); } /// @@ -312,7 +326,7 @@ namespace osu.Framework.Bindables } } - ItemsRemoved?.Invoke(removed); + notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, removed)); return removed.Count; } @@ -447,8 +461,12 @@ namespace osu.Framework.Bindables public void UnbindEvents() { +#pragma warning disable 618 can be removed 20200817 ItemsAdded = null; ItemsRemoved = null; +#pragma warning restore 618 + + CollectionChanged = null; DisabledChanged = null; } @@ -497,13 +515,13 @@ namespace osu.Framework.Bindables /// The collection whose items should be added to this collection. /// Thrown if this collection is public void AddRange(IEnumerable items) - => addRange(items as ICollection ?? items.ToArray(), null); + => addRange(items as IList ?? items.ToArray(), null); - private void addRange(ICollection items, BindableList caller) + private void addRange(IList items, BindableList caller) { ensureMutationAllowed(); - collection.AddRange(items); + collection.AddRange(items.Cast()); if (bindings != null) { @@ -516,7 +534,7 @@ namespace osu.Framework.Bindables } } - ItemsAdded?.Invoke(items); + notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, items, collection.Count - items.Count)); } void IBindable.BindTo(IBindable them) @@ -603,6 +621,33 @@ namespace osu.Framework.Bindables #endregion IEnumerable + private void notifyCollectionChanged(NotifyCollectionChangedEventArgs args) + { +#pragma warning disable 618 can be removed 20200817 + switch (args.Action) + { + case NotifyCollectionChangedAction.Add: + ItemsAdded?.Invoke(args.NewItems.Cast()); + break; + + case NotifyCollectionChangedAction.Replace: + case NotifyCollectionChangedAction.Move: + ItemsRemoved?.Invoke(args.OldItems.Cast()); + ItemsAdded?.Invoke(args.NewItems.Cast()); + break; + + case NotifyCollectionChangedAction.Remove: + ItemsRemoved?.Invoke(args.OldItems.Cast()); + break; + + case NotifyCollectionChangedAction.Reset: + break; + } +#pragma warning restore 618 + + CollectionChanged?.Invoke(this, args); + } + private void ensureMutationAllowed() { if (Disabled) diff --git a/osu.Framework/Bindables/IBindableList.cs b/osu.Framework/Bindables/IBindableList.cs index 75916d347..30d23b0da 100644 --- a/osu.Framework/Bindables/IBindableList.cs +++ b/osu.Framework/Bindables/IBindableList.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Collections.Specialized; namespace osu.Framework.Bindables { @@ -10,7 +11,7 @@ namespace osu.Framework.Bindables /// An readonly interface which can be bound to other s in order to watch for state and content changes. /// /// The type of value encapsulated by this . - public interface IBindableList : IReadOnlyList, IBindable + public interface IBindableList : IReadOnlyList, IBindable, INotifyCollectionChanged { /// /// An event which is raised when an range of items get added. From 7b163677256d03f0ba1be98e593eb74b7854867c Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Mon, 17 Feb 2020 14:12:11 +0900 Subject: [PATCH 08/15] Update tests + rearrangeable list to use new event --- .../Bindables/BindableListTest.cs | 295 ++++++++++++++++-- .../Containers/RearrangeableListContainer.cs | 93 +++--- 2 files changed, 324 insertions(+), 64 deletions(-) diff --git a/osu.Framework.Tests/Bindables/BindableListTest.cs b/osu.Framework.Tests/Bindables/BindableListTest.cs index 991ba56ce..ce22b56d0 100644 --- a/osu.Framework.Tests/Bindables/BindableListTest.cs +++ b/osu.Framework.Tests/Bindables/BindableListTest.cs @@ -3,9 +3,11 @@ using System; using System.Collections.Generic; +using System.Collections.Specialized; using System.Linq; using NUnit.Framework; using osu.Framework.Bindables; +using osu.Framework.Extensions.IEnumerableExtensions; namespace osu.Framework.Tests.Bindables { @@ -118,16 +120,25 @@ namespace osu.Framework.Tests.Bindables { bindableStringList.Add("0"); +#pragma warning disable 618 can be removed 20200817 IEnumerable addedItem = null; IEnumerable removedItem = null; - bindableStringList.ItemsAdded += v => addedItem = v; bindableStringList.ItemsRemoved += v => removedItem = v; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + bindableStringList.CollectionChanged += (_, args) => triggeredArgs = args; bindableStringList[0] = "1"; Assert.That(removedItem.Single(), Is.EqualTo("0")); Assert.That(addedItem.Single(), Is.EqualTo("1")); + + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Replace)); + Assert.That(triggeredArgs.OldItems, Is.EquivalentTo("0".Yield())); + Assert.That(triggeredArgs.NewItems, Is.EquivalentTo("1".Yield())); + Assert.That(triggeredArgs.OldStartingIndex, Is.Zero); + Assert.That(triggeredArgs.NewStartingIndex, Is.Zero); } [Test] @@ -135,18 +146,28 @@ namespace osu.Framework.Tests.Bindables { bindableStringList.Add("0"); - IEnumerable addedItem = null; - IEnumerable removedItem = null; - var list = new BindableList(); list.BindTo(bindableStringList); + +#pragma warning disable 618 can be removed 20200817 + IEnumerable addedItem = null; + IEnumerable removedItem = null; list.ItemsAdded += v => addedItem = v; list.ItemsRemoved += v => removedItem = v; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + list.CollectionChanged += (_, args) => triggeredArgs = args; bindableStringList[0] = "1"; Assert.That(removedItem.Single(), Is.EqualTo("0")); Assert.That(addedItem.Single(), Is.EqualTo("1")); + + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Replace)); + Assert.That(triggeredArgs.OldItems, Is.EquivalentTo("0".Yield())); + Assert.That(triggeredArgs.NewItems, Is.EquivalentTo("1".Yield())); + Assert.That(triggeredArgs.OldStartingIndex, Is.Zero); + Assert.That(triggeredArgs.NewStartingIndex, Is.Zero); } #endregion @@ -168,12 +189,20 @@ namespace osu.Framework.Tests.Bindables [TestCase(null)] public void TestAddWithStringNotifiesSubscriber(string str) { +#pragma warning disable 618 can be removed 20200817 string addedString = null; bindableStringList.ItemsAdded += s => addedString = s.SingleOrDefault(); +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + bindableStringList.CollectionChanged += (_, args) => triggeredArgs = args; bindableStringList.Add(str); Assert.AreEqual(str, addedString); + + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Add)); + Assert.That(triggeredArgs.NewItems, Is.EquivalentTo(str.Yield())); + Assert.That(triggeredArgs.NewStartingIndex, Is.Zero); } [TestCase("a random string")] @@ -181,12 +210,18 @@ namespace osu.Framework.Tests.Bindables [TestCase(null)] public void TestAddWithStringNotifiesSubscriberOnce(string str) { +#pragma warning disable 618 can be removed 20200817 int notificationCount = 0; bindableStringList.ItemsAdded += s => notificationCount++; +#pragma warning restore 618 + var triggeredArgs = new List(); + bindableStringList.CollectionChanged += (_, args) => triggeredArgs.Add(args); bindableStringList.Add(str); Assert.AreEqual(1, notificationCount); + + Assert.That(triggeredArgs, Has.Count.EqualTo(1)); } [TestCase("a random string")] @@ -194,18 +229,30 @@ namespace osu.Framework.Tests.Bindables [TestCase(null)] public void TestAddWithStringNotifiesMultipleSubscribers(string str) { +#pragma warning disable 618 can be removed 20200817 bool subscriberANotified = false; bool subscriberBNotified = false; bool subscriberCNotified = false; bindableStringList.ItemsAdded += s => subscriberANotified = true; bindableStringList.ItemsAdded += s => subscriberBNotified = true; bindableStringList.ItemsAdded += s => subscriberCNotified = true; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgsA = null; + NotifyCollectionChangedEventArgs triggeredArgsB = null; + NotifyCollectionChangedEventArgs triggeredArgsC = null; + bindableStringList.CollectionChanged += (_, args) => triggeredArgsA = args; + bindableStringList.CollectionChanged += (_, args) => triggeredArgsB = args; + bindableStringList.CollectionChanged += (_, args) => triggeredArgsC = args; bindableStringList.Add(str); Assert.IsTrue(subscriberANotified); Assert.IsTrue(subscriberBNotified); Assert.IsTrue(subscriberCNotified); + + Assert.That(triggeredArgsA, Is.Not.Null); + Assert.That(triggeredArgsB, Is.Not.Null); + Assert.That(triggeredArgsC, Is.Not.Null); } [TestCase("a random string")] @@ -213,17 +260,29 @@ namespace osu.Framework.Tests.Bindables [TestCase(null)] public void TestAddWithStringNotifiesMultipleSubscribersOnlyAfterTheAdd(string str) { +#pragma warning disable 618 can be removed 20200817 bool subscriberANotified = false; bool subscriberBNotified = false; bool subscriberCNotified = false; bindableStringList.ItemsAdded += s => subscriberANotified = true; bindableStringList.ItemsAdded += s => subscriberBNotified = true; bindableStringList.ItemsAdded += s => subscriberCNotified = true; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgsA = null; + NotifyCollectionChangedEventArgs triggeredArgsB = null; + NotifyCollectionChangedEventArgs triggeredArgsC = null; + bindableStringList.CollectionChanged += (_, args) => triggeredArgsA = args; + bindableStringList.CollectionChanged += (_, args) => triggeredArgsB = args; + bindableStringList.CollectionChanged += (_, args) => triggeredArgsC = args; Assert.IsFalse(subscriberANotified); Assert.IsFalse(subscriberBNotified); Assert.IsFalse(subscriberCNotified); + Assert.That(triggeredArgsA, Is.Null); + Assert.That(triggeredArgsB, Is.Null); + Assert.That(triggeredArgsC, Is.Null); + bindableStringList.Add(str); } @@ -309,8 +368,13 @@ namespace osu.Framework.Tests.Bindables string[] items = { "test1", "test2", "test3" }; var list = new BindableList(); bindableStringList.BindTo(list); + +#pragma warning disable 618 can be removed 20200817 IEnumerable addedItems = null; list.ItemsAdded += e => addedItems = e; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + list.CollectionChanged += (_, args) => triggeredArgs = args; bindableStringList.AddRange(items); @@ -320,6 +384,9 @@ namespace osu.Framework.Tests.Bindables CollectionAssert.AreEquivalent(items, addedItems); CollectionAssert.AreEquivalent(items, list); }); + + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Add)); + Assert.That(triggeredArgs.NewItems, Is.EquivalentTo(items)); } [Test] @@ -329,9 +396,6 @@ namespace osu.Framework.Tests.Bindables BindableList list2 = new BindableList(); list2.BindTo(list1); - int addeditem = 0; - list1.ItemsAdded += items => addeditem = items.Single(); - int counter = 0; IEnumerable valueEnumerable() @@ -341,9 +405,9 @@ namespace osu.Framework.Tests.Bindables list1.AddRange(valueEnumerable()); - Assert.That(list1[0], Is.EqualTo(0)); - Assert.That(list2[0], Is.EqualTo(0)); - Assert.That(addeditem, Is.EqualTo(0)); + Assert.That(list1, Is.EquivalentTo(0.Yield())); + Assert.That(list2, Is.EquivalentTo(0.Yield())); + Assert.That(counter, Is.EqualTo(1)); } #endregion @@ -372,12 +436,20 @@ namespace osu.Framework.Tests.Bindables bindableStringList.Add("0"); bindableStringList.Add("2"); +#pragma warning disable 618 can be removed 20200817 bool wasAdded = false; bindableStringList.ItemsAdded += _ => wasAdded = true; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + bindableStringList.CollectionChanged += (_, args) => triggeredArgs = args; bindableStringList.Insert(1, "1"); Assert.IsTrue(wasAdded); + + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Add)); + Assert.That(triggeredArgs.NewItems, Has.One.Items.EqualTo("1")); + Assert.That(triggeredArgs.NewStartingIndex, Is.EqualTo(1)); } [Test] @@ -386,15 +458,23 @@ namespace osu.Framework.Tests.Bindables bindableStringList.Add("0"); bindableStringList.Add("2"); - bool wasAdded = false; - var list = new BindableList(); list.BindTo(bindableStringList); + +#pragma warning disable 618 can be removed 20200817 + bool wasAdded = false; list.ItemsAdded += _ => wasAdded = true; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + list.CollectionChanged += (_, args) => triggeredArgs = args; bindableStringList.Insert(1, "1"); Assert.IsTrue(wasAdded); + + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Add)); + Assert.That(triggeredArgs.NewItems, Has.One.Items.EqualTo("1")); + Assert.That(triggeredArgs.NewStartingIndex, Is.EqualTo(1)); } [Test] @@ -464,12 +544,21 @@ namespace osu.Framework.Tests.Bindables { const string item = "item"; bindableStringList.Add(item); + +#pragma warning disable 618 can be removed 20200817 bool updated = false; bindableStringList.ItemsRemoved += s => updated = true; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + bindableStringList.CollectionChanged += (_, args) => triggeredArgs = args; bindableStringList.Remove(item); Assert.True(updated); + + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Remove)); + Assert.That(triggeredArgs.OldItems, Has.One.Items.EqualTo(item)); + Assert.That(triggeredArgs.OldStartingIndex, Is.EqualTo(0)); } [Test] @@ -477,12 +566,21 @@ namespace osu.Framework.Tests.Bindables { const string item = "item"; bindableStringList.Add(item); + +#pragma warning disable 618 can be removed 20200817 bool updatedA = false; bool updatedB = false; bool updatedC = false; bindableStringList.ItemsRemoved += s => updatedA = true; bindableStringList.ItemsRemoved += s => updatedB = true; bindableStringList.ItemsRemoved += s => updatedC = true; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgsA = null; + NotifyCollectionChangedEventArgs triggeredArgsB = null; + NotifyCollectionChangedEventArgs triggeredArgsC = null; + bindableStringList.CollectionChanged += (_, args) => triggeredArgsA = args; + bindableStringList.CollectionChanged += (_, args) => triggeredArgsB = args; + bindableStringList.CollectionChanged += (_, args) => triggeredArgsC = args; bindableStringList.Remove(item); @@ -492,6 +590,10 @@ namespace osu.Framework.Tests.Bindables Assert.True(updatedB); Assert.True(updatedC); }); + + Assert.That(triggeredArgsA, Is.Not.Null); + Assert.That(triggeredArgsB, Is.Not.Null); + Assert.That(triggeredArgsC, Is.Not.Null); } [Test] @@ -536,12 +638,21 @@ namespace osu.Framework.Tests.Bindables bindableStringList.Add(item); var list = new BindableList(); list.BindTo(bindableStringList); + +#pragma warning disable 618 can be removed 20200817 bool wasRemoved = false; list.ItemsRemoved += s => wasRemoved = true; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + list.CollectionChanged += (_, args) => triggeredArgs = args; bindableStringList.Remove(item); Assert.True(wasRemoved); + + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Remove)); + Assert.That(triggeredArgs.OldItems, Has.One.Items.EqualTo(item)); + Assert.That(triggeredArgs.OldStartingIndex, Is.EqualTo(0)); } [Test] @@ -551,16 +662,31 @@ namespace osu.Framework.Tests.Bindables bindableStringList.Add(item); var listA = new BindableList(); listA.BindTo(bindableStringList); + +#pragma warning disable 618 can be removed 20200817 bool wasRemovedA1 = false; bool wasRemovedA2 = false; listA.ItemsRemoved += s => wasRemovedA1 = true; listA.ItemsRemoved += s => wasRemovedA2 = true; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgsA1 = null; + NotifyCollectionChangedEventArgs triggeredArgsA2 = null; + listA.CollectionChanged += (_, args) => triggeredArgsA1 = args; + listA.CollectionChanged += (_, args) => triggeredArgsA2 = args; + var listB = new BindableList(); listB.BindTo(bindableStringList); + +#pragma warning disable 618 can be removed 20200817 bool wasRemovedB1 = false; bool wasRemovedB2 = false; listB.ItemsRemoved += s => wasRemovedB1 = true; listB.ItemsRemoved += s => wasRemovedB2 = true; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgsB1 = null; + NotifyCollectionChangedEventArgs triggeredArgsB2 = null; + listB.CollectionChanged += (_, args) => triggeredArgsB1 = args; + listB.CollectionChanged += (_, args) => triggeredArgsB2 = args; bindableStringList.Remove(item); @@ -571,6 +697,11 @@ namespace osu.Framework.Tests.Bindables Assert.IsTrue(wasRemovedB1); Assert.IsTrue(wasRemovedB2); }); + + Assert.That(triggeredArgsA1, Is.Not.Null); + Assert.That(triggeredArgsA2, Is.Not.Null); + Assert.That(triggeredArgsB1, Is.Not.Null); + Assert.That(triggeredArgsB2, Is.Not.Null); } [Test] @@ -615,17 +746,27 @@ namespace osu.Framework.Tests.Bindables bindableStringList.Add("0"); bindableStringList.Add("1"); +#pragma warning disable 618 can be removed 20200817 List itemsRemoved = null; bindableStringList.ItemsRemoved += i => itemsRemoved = i.ToList(); +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + bindableStringList.CollectionChanged += (_, args) => triggeredArgs = args; + bindableStringList.RemoveRange(1, 1); + Assert.AreEqual(1, bindableStringList.Count); + Assert.AreEqual("0", bindableStringList.FirstOrDefault()); + Assert.Multiple(() => { - Assert.AreEqual(1, bindableStringList.Count); - Assert.AreEqual("0", bindableStringList.FirstOrDefault()); Assert.AreEqual(1, itemsRemoved.Count); Assert.AreEqual("1", itemsRemoved.FirstOrDefault()); }); + + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Remove)); + Assert.That(triggeredArgs.OldItems, Has.One.Items.EqualTo("1")); + Assert.That(triggeredArgs.OldStartingIndex, Is.EqualTo(1)); } [Test] @@ -634,10 +775,15 @@ namespace osu.Framework.Tests.Bindables bindableStringList.Add("0"); bindableStringList.Add("1"); - List itemsRemoved = null; var list = new BindableList(); list.BindTo(bindableStringList); + +#pragma warning disable 618 can be removed 20200817 + List itemsRemoved = null; list.ItemsRemoved += i => itemsRemoved = i.ToList(); +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + list.CollectionChanged += (_, args) => triggeredArgs = args; bindableStringList.RemoveRange(0, 1); @@ -646,6 +792,10 @@ namespace osu.Framework.Tests.Bindables Assert.AreEqual(1, itemsRemoved.Count); Assert.AreEqual("0", itemsRemoved.FirstOrDefault()); }); + + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Remove)); + Assert.That(triggeredArgs.OldItems, Has.One.Items.EqualTo("0")); + Assert.That(triggeredArgs.OldStartingIndex, Is.EqualTo(0)); } [Test] @@ -653,14 +803,21 @@ namespace osu.Framework.Tests.Bindables { bindableStringList.Add("0"); - bool notified = false; var list = new BindableList(); list.BindTo(bindableStringList); + +#pragma warning disable 618 can be removed 20200817 + bool notified = false; list.ItemsRemoved += i => notified = true; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + list.CollectionChanged += (_, args) => triggeredArgs = args; bindableStringList.RemoveRange(0, 0); Assert.IsFalse(notified); + + Assert.That(triggeredArgs, Is.Null); } #endregion @@ -692,14 +849,22 @@ namespace osu.Framework.Tests.Bindables [Test] public void TestRemoveAtNotifiesSubscribers() { - bool wasRemoved = false; - bindableStringList.Add("abc"); + +#pragma warning disable 618 can be removed 20200817 + bool wasRemoved = false; bindableStringList.ItemsRemoved += _ => wasRemoved = true; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + bindableStringList.CollectionChanged += (_, args) => triggeredArgs = args; bindableStringList.RemoveAt(0); Assert.IsTrue(wasRemoved); + + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Remove)); + Assert.That(triggeredArgs.OldItems, Has.One.Items.EqualTo("abc")); + Assert.That(triggeredArgs.OldStartingIndex, Is.EqualTo(0)); } [Test] @@ -707,15 +872,23 @@ namespace osu.Framework.Tests.Bindables { bindableStringList.Add("abc"); - bool wasRemoved = false; - var list = new BindableList(); list.BindTo(bindableStringList); + +#pragma warning disable 618 can be removed 20200817 + bool wasRemoved = false; list.ItemsRemoved += s => wasRemoved = true; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + list.CollectionChanged += (_, args) => triggeredArgs = args; bindableStringList.RemoveAt(0); Assert.IsTrue(wasRemoved); + + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Remove)); + Assert.That(triggeredArgs.OldItems, Has.One.Items.EqualTo("abc")); + Assert.That(triggeredArgs.OldStartingIndex, Is.EqualTo(0)); } #endregion @@ -747,11 +920,19 @@ namespace osu.Framework.Tests.Bindables bindableStringList.Add("0"); bindableStringList.Add("0"); +#pragma warning disable 618 can be removed 20200817 List itemsRemoved = null; bindableStringList.ItemsRemoved += i => itemsRemoved = i.ToList(); +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + bindableStringList.CollectionChanged += (_, args) => triggeredArgs = args; + bindableStringList.RemoveAll(m => m == "0"); Assert.AreEqual(2, itemsRemoved.Count); + + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Remove)); + Assert.That(triggeredArgs.OldItems, Is.EquivalentTo(new[] { "0", "0" })); } [Test] @@ -760,14 +941,22 @@ namespace osu.Framework.Tests.Bindables bindableStringList.Add("0"); bindableStringList.Add("0"); - List itemsRemoved = null; var list = new BindableList(); list.BindTo(bindableStringList); + +#pragma warning disable 618 can be removed 20200817 + List itemsRemoved = null; list.ItemsRemoved += i => itemsRemoved = i.ToList(); +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + list.CollectionChanged += (_, args) => triggeredArgs = args; bindableStringList.RemoveAll(m => m == "0"); Assert.AreEqual(2, itemsRemoved.Count); + + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Remove)); + Assert.That(triggeredArgs.OldItems, Is.EquivalentTo(new[] { "0", "0" })); } #endregion @@ -819,24 +1008,38 @@ namespace osu.Framework.Tests.Bindables { for (int i = 0; i < 5; i++) bindableStringList.Add("testA"); + +#pragma warning disable 618 can be removed 20200817 bool wasNotified = false; bindableStringList.ItemsRemoved += items => wasNotified = true; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + bindableStringList.CollectionChanged += (_, args) => triggeredArgs = args; bindableStringList.Clear(); Assert.IsTrue(wasNotified); + + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Reset)); } [Test] public void TestClearDoesNotNotifySubscriberBeforeClear() { - bool wasNotified = false; - bindableStringList.ItemsRemoved += items => wasNotified = true; for (int i = 0; i < 5; i++) bindableStringList.Add("testA"); +#pragma warning disable 618 can be removed 20200817 + bool wasNotified = false; + bindableStringList.ItemsRemoved += items => wasNotified = true; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + bindableStringList.CollectionChanged += (_, args) => triggeredArgs = args; + Assert.IsFalse(wasNotified); + Assert.That(triggeredArgs, Is.Null); + bindableStringList.Clear(); } @@ -845,12 +1048,21 @@ namespace osu.Framework.Tests.Bindables { for (int i = 0; i < 5; i++) bindableStringList.Add("testA"); + +#pragma warning disable 618 can be removed 20200817 bool wasNotifiedA = false; - bindableStringList.ItemsRemoved += items => wasNotifiedA = true; bool wasNotifiedB = false; - bindableStringList.ItemsRemoved += items => wasNotifiedB = true; bool wasNotifiedC = false; + bindableStringList.ItemsRemoved += items => wasNotifiedA = true; + bindableStringList.ItemsRemoved += items => wasNotifiedB = true; bindableStringList.ItemsRemoved += items => wasNotifiedC = true; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgsA = null; + NotifyCollectionChangedEventArgs triggeredArgsB = null; + NotifyCollectionChangedEventArgs triggeredArgsC = null; + bindableStringList.CollectionChanged += (_, args) => triggeredArgsA = args; + bindableStringList.CollectionChanged += (_, args) => triggeredArgsB = args; + bindableStringList.CollectionChanged += (_, args) => triggeredArgsC = args; bindableStringList.Clear(); @@ -860,6 +1072,10 @@ namespace osu.Framework.Tests.Bindables Assert.IsTrue(wasNotifiedB); Assert.IsTrue(wasNotifiedC); }); + + Assert.That(triggeredArgsA, Is.Not.Null); + Assert.That(triggeredArgsB, Is.Not.Null); + Assert.That(triggeredArgsC, Is.Not.Null); } [Test] @@ -1114,6 +1330,8 @@ namespace osu.Framework.Tests.Bindables { string[] strings = { "testA", "testB", "testC" }; bindableStringList.AddRange(strings); + +#pragma warning disable 618 can be removed 20200817 bool itemsGotCleared = false; IEnumerable clearedItems = null; bindableStringList.ItemsRemoved += items => @@ -1121,6 +1339,9 @@ namespace osu.Framework.Tests.Bindables itemsGotCleared = true; clearedItems = items; }; +#pragma warning restore 618 + var triggeredArgs = new List(); + bindableStringList.CollectionChanged += (_, args) => triggeredArgs.Add(args); bindableStringList.Parse(null); @@ -1129,14 +1350,18 @@ namespace osu.Framework.Tests.Bindables CollectionAssert.AreEquivalent(strings, clearedItems); Assert.IsTrue(itemsGotCleared); }); + + Assert.That(triggeredArgs, Has.Count.EqualTo(1)); + Assert.That(triggeredArgs.First().Action, Is.EqualTo(NotifyCollectionChangedAction.Reset)); } [Test] public void TestParseWithItemsNotifiesAddRangeAndClearSubscribers() { bindableStringList.Add("test123"); - IEnumerable strings = new[] { "testA", "testB" }; + +#pragma warning disable 618 can be removed 20200817 IEnumerable addedItems = null; bool? itemsWereFirstCleaned = null; bindableStringList.ItemsAdded += items => @@ -1150,6 +1375,9 @@ namespace osu.Framework.Tests.Bindables if (itemsWereFirstCleaned == null) itemsWereFirstCleaned = true; }; +#pragma warning restore 618 + var triggeredArgs = new List(); + bindableStringList.CollectionChanged += (_, args) => triggeredArgs.Add(args); bindableStringList.Parse(strings); @@ -1160,6 +1388,12 @@ namespace osu.Framework.Tests.Bindables Assert.NotNull(itemsWereFirstCleaned); Assert.IsTrue(itemsWereFirstCleaned ?? false); }); + + Assert.That(triggeredArgs, Has.Count.EqualTo(2)); + Assert.That(triggeredArgs.First().Action, Is.EqualTo(NotifyCollectionChangedAction.Reset)); + Assert.That(triggeredArgs.ElementAt(1).Action, Is.EqualTo(NotifyCollectionChangedAction.Add)); + Assert.That(triggeredArgs.ElementAt(1).NewItems, Is.EquivalentTo(strings)); + Assert.That(triggeredArgs.ElementAt(1).NewStartingIndex, Is.EqualTo(0)); } #endregion @@ -1170,12 +1404,21 @@ namespace osu.Framework.Tests.Bindables public void TestBoundCopyWithAdd() { var boundCopy = bindableStringList.GetBoundCopy(); + +#pragma warning disable 618 can be removed 20200817 bool boundCopyItemAdded = false; boundCopy.ItemsAdded += item => boundCopyItemAdded = true; +#pragma warning restore 618 + NotifyCollectionChangedEventArgs triggeredArgs = null; + boundCopy.CollectionChanged += (_, args) => triggeredArgs = args; bindableStringList.Add("test"); Assert.IsTrue(boundCopyItemAdded); + + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Add)); + Assert.That(triggeredArgs.NewItems, Is.EquivalentTo("test".Yield())); + Assert.That(triggeredArgs.NewStartingIndex, Is.EqualTo(0)); } #endregion diff --git a/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs b/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs index bc15689ec..bae947453 100644 --- a/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs +++ b/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs @@ -2,7 +2,10 @@ // See the LICENCE file in the repository root for full licence text. using System; +using System.Collections; using System.Collections.Generic; +using System.Collections.Specialized; +using System.Linq; using osu.Framework.Bindables; using osu.Framework.Input.Events; using osuTK; @@ -48,7 +51,6 @@ namespace osu.Framework.Graphics.Containers private readonly Dictionary> itemMap = new Dictionary>(); private RearrangeableListItem currentlyDraggedItem; private Vector2 screenSpaceDragPosition; - private bool isCurrentlyRearranging; // Will be true only for the duration that indices are being moved around /// /// Creates a new . @@ -68,20 +70,65 @@ namespace osu.Framework.Graphics.Containers d.Child = ListContainer; }); - Items.ItemsAdded += itemsAdded; - Items.ItemsRemoved += itemsRemoved; + Items.CollectionChanged += collectionChanged; } - private void itemsAdded(IEnumerable items) + private void collectionChanged(object sender, NotifyCollectionChangedEventArgs e) { - if (isCurrentlyRearranging) - return; + switch (e.Action) + { + case NotifyCollectionChangedAction.Add: + addItems(e.NewItems); + reSort(); + break; - foreach (var item in items) + case NotifyCollectionChangedAction.Remove: + removeItems(e.OldItems); + reSort(); + + if (Items.Count == 0) + { + // Explicitly reset scroll position here so that ScrollContainer doesn't retain our + // scroll position if we quickly add new items after calling a Clear(). + ScrollContainer.ScrollToStart(); + } + + break; + + case NotifyCollectionChangedAction.Reset: + currentlyDraggedItem = null; + ListContainer.Clear(); + itemMap.Clear(); + break; + + case NotifyCollectionChangedAction.Replace: + removeItems(e.OldItems); + addItems(e.NewItems); + reSort(); + break; + } + } + + private void removeItems(IList items) + { + foreach (var item in items.Cast()) + { + if (currentlyDraggedItem != null && EqualityComparer.Default.Equals(currentlyDraggedItem.Model, item)) + currentlyDraggedItem = null; + + ListContainer.Remove(itemMap[item]); + itemMap.Remove(item); + } + } + + private void addItems(IList items) + { + foreach (var item in items.Cast()) { if (itemMap.ContainsKey(item)) { - throw new InvalidOperationException($"Duplicate items cannot be added to a {nameof(BindableList)} that is currently bound with a {nameof(RearrangeableListContainer)}."); + throw new InvalidOperationException( + $"Duplicate items cannot be added to a {nameof(BindableList)} that is currently bound with a {nameof(RearrangeableListContainer)}."); } var drawable = CreateDrawable(item).With(d => @@ -94,32 +141,6 @@ namespace osu.Framework.Graphics.Containers ListContainer.Add(drawable); itemMap[item] = drawable; } - - reSort(); - } - - private void itemsRemoved(IEnumerable items) - { - if (isCurrentlyRearranging) - return; - - foreach (var item in items) - { - if (currentlyDraggedItem != null && EqualityComparer.Default.Equals(currentlyDraggedItem.Model, item)) - currentlyDraggedItem = null; - - ListContainer.Remove(itemMap[item]); - itemMap.Remove(item); - } - - reSort(); - - if (Items.Count == 0) - { - // Explicitly reset scroll position here so that ScrollContainer doesn't retain our - // scroll position if we quickly add new items after calling a Clear(). - ScrollContainer.ScrollToStart(); - } } private void reSort() @@ -215,15 +236,11 @@ namespace osu.Framework.Graphics.Containers if (srcIndex == dstIndex) return; - isCurrentlyRearranging = true; - Items.RemoveAt(srcIndex); Items.Insert(dstIndex, currentlyDraggedItem.Model); // Todo: this could be optimised, but it's a very simple iteration over all the items reSort(); - - isCurrentlyRearranging = false; } /// From 2a90e860b347d6816bd905e1f2198164c9df7cd8 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Mon, 17 Feb 2020 14:38:30 +0900 Subject: [PATCH 09/15] Refactor removal --- .../Containers/RearrangeableListContainer.cs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs b/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs index bae947453..5bdcdb321 100644 --- a/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs +++ b/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs @@ -85,20 +85,16 @@ namespace osu.Framework.Graphics.Containers case NotifyCollectionChangedAction.Remove: removeItems(e.OldItems); reSort(); - - if (Items.Count == 0) - { - // Explicitly reset scroll position here so that ScrollContainer doesn't retain our - // scroll position if we quickly add new items after calling a Clear(). - ScrollContainer.ScrollToStart(); - } - break; case NotifyCollectionChangedAction.Reset: currentlyDraggedItem = null; ListContainer.Clear(); itemMap.Clear(); + + // Explicitly reset scroll position here so that ScrollContainer doesn't retain our + // scroll position if we quickly add new items after calling a Clear(). + ScrollContainer.ScrollToStart(); break; case NotifyCollectionChangedAction.Replace: From 43f25505b9197db23598d8e1864c93fd55dec485 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Mon, 17 Feb 2020 14:41:29 +0900 Subject: [PATCH 10/15] Add back rearranging block --- .../Graphics/Containers/RearrangeableListContainer.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs b/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs index 5bdcdb321..ef87c8dff 100644 --- a/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs +++ b/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs @@ -51,6 +51,7 @@ namespace osu.Framework.Graphics.Containers private readonly Dictionary> itemMap = new Dictionary>(); private RearrangeableListItem currentlyDraggedItem; private Vector2 screenSpaceDragPosition; + private bool isCurrentlyRearranging; // Will be true only for the duration that indices are being moved around /// /// Creates a new . @@ -75,6 +76,9 @@ namespace osu.Framework.Graphics.Containers private void collectionChanged(object sender, NotifyCollectionChangedEventArgs e) { + if (isCurrentlyRearranging) + return; + switch (e.Action) { case NotifyCollectionChangedAction.Add: @@ -232,9 +236,13 @@ namespace osu.Framework.Graphics.Containers if (srcIndex == dstIndex) return; + isCurrentlyRearranging = true; + Items.RemoveAt(srcIndex); Items.Insert(dstIndex, currentlyDraggedItem.Model); + isCurrentlyRearranging = false; + // Todo: this could be optimised, but it's a very simple iteration over all the items reSort(); } From 3ef261c6d6928ca84c059e5878812eda5e0c5f92 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Mon, 17 Feb 2020 15:02:59 +0900 Subject: [PATCH 11/15] Give remove events rather than reset events on clear --- osu.Framework.Tests/Bindables/BindableListTest.cs | 12 +++++++++--- osu.Framework/Bindables/BindableList.cs | 7 +------ .../Containers/RearrangeableListContainer.cs | 8 ++++---- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/osu.Framework.Tests/Bindables/BindableListTest.cs b/osu.Framework.Tests/Bindables/BindableListTest.cs index ce22b56d0..78055ec6d 100644 --- a/osu.Framework.Tests/Bindables/BindableListTest.cs +++ b/osu.Framework.Tests/Bindables/BindableListTest.cs @@ -1020,7 +1020,9 @@ namespace osu.Framework.Tests.Bindables Assert.IsTrue(wasNotified); - Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Reset)); + Assert.That(triggeredArgs.Action, Is.EqualTo(NotifyCollectionChangedAction.Remove)); + Assert.That(triggeredArgs.OldItems, Is.EquivalentTo(new[] { "testA", "testA", "testA", "testA", "testA" })); + Assert.That(triggeredArgs.OldStartingIndex, Is.EqualTo(0)); } [Test] @@ -1352,7 +1354,9 @@ namespace osu.Framework.Tests.Bindables }); Assert.That(triggeredArgs, Has.Count.EqualTo(1)); - Assert.That(triggeredArgs.First().Action, Is.EqualTo(NotifyCollectionChangedAction.Reset)); + Assert.That(triggeredArgs.First().Action, Is.EqualTo(NotifyCollectionChangedAction.Remove)); + Assert.That(triggeredArgs.First().OldItems, Is.EquivalentTo(strings)); + Assert.That(triggeredArgs.First().OldStartingIndex, Is.EqualTo(0)); } [Test] @@ -1390,7 +1394,9 @@ namespace osu.Framework.Tests.Bindables }); Assert.That(triggeredArgs, Has.Count.EqualTo(2)); - Assert.That(triggeredArgs.First().Action, Is.EqualTo(NotifyCollectionChangedAction.Reset)); + Assert.That(triggeredArgs.First().Action, Is.EqualTo(NotifyCollectionChangedAction.Remove)); + Assert.That(triggeredArgs.First().OldItems, Is.EquivalentTo("test123".Yield())); + Assert.That(triggeredArgs.First().OldStartingIndex, Is.EqualTo(0)); Assert.That(triggeredArgs.ElementAt(1).Action, Is.EqualTo(NotifyCollectionChangedAction.Add)); Assert.That(triggeredArgs.ElementAt(1).NewItems, Is.EquivalentTo(strings)); Assert.That(triggeredArgs.ElementAt(1).NewStartingIndex, Is.EqualTo(0)); diff --git a/osu.Framework/Bindables/BindableList.cs b/osu.Framework/Bindables/BindableList.cs index 2842ccb01..6141b3a35 100644 --- a/osu.Framework/Bindables/BindableList.cs +++ b/osu.Framework/Bindables/BindableList.cs @@ -182,12 +182,7 @@ namespace osu.Framework.Bindables } } -#pragma warning disable 618 // can be removed 20200817 - // The collection changed event does not provide items upon clearing, but the old interface expects it to. - ItemsRemoved?.Invoke(clearedItems); -#pragma warning restore 618 - - notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset)); + notifyCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, clearedItems, 0)); } /// diff --git a/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs b/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs index ef87c8dff..d30c4d5a9 100644 --- a/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs +++ b/osu.Framework/Graphics/Containers/RearrangeableListContainer.cs @@ -89,16 +89,16 @@ namespace osu.Framework.Graphics.Containers case NotifyCollectionChangedAction.Remove: removeItems(e.OldItems); reSort(); + + // Explicitly reset scroll position here so that ScrollContainer doesn't retain our + // scroll position if we quickly add new items after calling a Clear(). + ScrollContainer.ScrollToStart(); break; case NotifyCollectionChangedAction.Reset: currentlyDraggedItem = null; ListContainer.Clear(); itemMap.Clear(); - - // Explicitly reset scroll position here so that ScrollContainer doesn't retain our - // scroll position if we quickly add new items after calling a Clear(). - ScrollContainer.ScrollToStart(); break; case NotifyCollectionChangedAction.Replace: From c5b7d25b4219c37ef8a0507fb78289105c38e3a2 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 18 Feb 2020 01:12:44 +0300 Subject: [PATCH 12/15] Ensure parent input manager is used after pending reset inputs are applied --- osu.Framework/Testing/ManualInputManagerTestScene.cs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/osu.Framework/Testing/ManualInputManagerTestScene.cs b/osu.Framework/Testing/ManualInputManagerTestScene.cs index 5dd482232..fafb27fad 100644 --- a/osu.Framework/Testing/ManualInputManagerTestScene.cs +++ b/osu.Framework/Testing/ManualInputManagerTestScene.cs @@ -133,13 +133,12 @@ namespace osu.Framework.Testing var joystick = currentState.Joystick; joystick.Buttons.ForEach(InputManager.ReleaseJoystickButton); - InputManager.UseParentInput = true; + // schedule after children to ensure pending inputs have been applied before using parent input manager. + ScheduleAfterChildren(() => InputManager.UseParentInput = true); } - private void returnUserInput() => - InputManager.UseParentInput = true; + private void returnUserInput() => InputManager.UseParentInput = true; - private void returnTestInput() => - InputManager.UseParentInput = false; + private void returnTestInput() => InputManager.UseParentInput = false; } } From 4013cc9244635f9c00b767d0fad97d06c7ba18a6 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 18 Feb 2020 01:13:28 +0300 Subject: [PATCH 13/15] Add test scene ensuring input reset works properly and mouse position set initially --- .../TestSceneManualInputManagerTestScene.cs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 osu.Framework.Tests/Visual/Testing/TestSceneManualInputManagerTestScene.cs diff --git a/osu.Framework.Tests/Visual/Testing/TestSceneManualInputManagerTestScene.cs b/osu.Framework.Tests/Visual/Testing/TestSceneManualInputManagerTestScene.cs new file mode 100644 index 000000000..d609ced73 --- /dev/null +++ b/osu.Framework.Tests/Visual/Testing/TestSceneManualInputManagerTestScene.cs @@ -0,0 +1,36 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using NUnit.Framework; +using osu.Framework.Input; +using osu.Framework.Testing; +using osuTK; +using osuTK.Input; + +namespace osu.Framework.Tests.Visual.Testing +{ + public class TestSceneManualInputManagerTestScene : ManualInputManagerTestScene + { + protected override Vector2 InitialMousePosition => new Vector2(10f); + + [Test] + public void TestResetInput() + { + AddStep("move mouse", () => InputManager.MoveMouseTo(Vector2.Zero)); + AddStep("press mouse", () => InputManager.PressButton(MouseButton.Left)); + AddStep("press key", () => InputManager.PressKey(Key.Z)); + AddStep("press joystick", () => InputManager.PressJoystickButton(JoystickButton.Button1)); + + AddStep("reset input", ResetInput); + + AddAssert("mouse position reset", () => InputManager.CurrentState.Mouse.Position == InitialMousePosition); + AddAssert("all input states released", () => + InputManager.CurrentState.Mouse.Buttons.HasAnyButtonPressed && + InputManager.CurrentState.Keyboard.Keys.HasAnyButtonPressed && + InputManager.CurrentState.Joystick.Buttons.HasAnyButtonPressed); + } + + [Test] + public void TestMousePositionSetToInitial() => AddAssert("mouse position set to initial", () => InputManager.CurrentState.Mouse.Position == InitialMousePosition); + } +} From 1236ecc481c1170cd04164f7dddc9b5576067211 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 18 Feb 2020 09:44:37 +0900 Subject: [PATCH 14/15] Fix inverted conditional --- .../Visual/Testing/TestSceneManualInputManagerTestScene.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Framework.Tests/Visual/Testing/TestSceneManualInputManagerTestScene.cs b/osu.Framework.Tests/Visual/Testing/TestSceneManualInputManagerTestScene.cs index d609ced73..06296dd02 100644 --- a/osu.Framework.Tests/Visual/Testing/TestSceneManualInputManagerTestScene.cs +++ b/osu.Framework.Tests/Visual/Testing/TestSceneManualInputManagerTestScene.cs @@ -25,9 +25,9 @@ namespace osu.Framework.Tests.Visual.Testing AddAssert("mouse position reset", () => InputManager.CurrentState.Mouse.Position == InitialMousePosition); AddAssert("all input states released", () => - InputManager.CurrentState.Mouse.Buttons.HasAnyButtonPressed && - InputManager.CurrentState.Keyboard.Keys.HasAnyButtonPressed && - InputManager.CurrentState.Joystick.Buttons.HasAnyButtonPressed); + !InputManager.CurrentState.Mouse.Buttons.HasAnyButtonPressed && + !InputManager.CurrentState.Keyboard.Keys.HasAnyButtonPressed && + !InputManager.CurrentState.Joystick.Buttons.HasAnyButtonPressed); } [Test] From 5df1edc10eb8d4396f7742cb6d8efa7de8417f55 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 18 Feb 2020 09:45:31 +0900 Subject: [PATCH 15/15] Call private method for sanity --- osu.Framework/Testing/ManualInputManagerTestScene.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Framework/Testing/ManualInputManagerTestScene.cs b/osu.Framework/Testing/ManualInputManagerTestScene.cs index fafb27fad..b1c8982d5 100644 --- a/osu.Framework/Testing/ManualInputManagerTestScene.cs +++ b/osu.Framework/Testing/ManualInputManagerTestScene.cs @@ -134,7 +134,7 @@ namespace osu.Framework.Testing joystick.Buttons.ForEach(InputManager.ReleaseJoystickButton); // schedule after children to ensure pending inputs have been applied before using parent input manager. - ScheduleAfterChildren(() => InputManager.UseParentInput = true); + ScheduleAfterChildren(returnUserInput); } private void returnUserInput() => InputManager.UseParentInput = true;