From beb5d61a42cc69841cfdbb1c8de0cc36d5973535 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Wed, 8 Dec 2021 20:38:18 +0900 Subject: [PATCH] Separate playlist item deletion to Playlists-specific class --- .../TestSceneDrawableRoomPlaylist.cs | 97 +-------- .../TestScenePlaylistsRoomPlaylist.cs | 188 ++++++++++++++++++ .../Components/MatchBeatmapDetailArea.cs | 3 +- .../OnlinePlay/DrawableRoomPlaylist.cs | 40 +--- .../DrawableRoomPlaylistWithResults.cs | 2 +- .../Playlists/PlaylistsRoomPlaylist.cs | 24 +++ .../Playlists/PlaylistsRoomSettingsOverlay.cs | 5 +- 7 files changed, 227 insertions(+), 132 deletions(-) create mode 100644 osu.Game.Tests/Visual/Multiplayer/TestScenePlaylistsRoomPlaylist.cs create mode 100644 osu.Game/Screens/OnlinePlay/Playlists/PlaylistsRoomPlaylist.cs diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs index 55aa665ff1..c60b55d7e8 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs @@ -128,95 +128,6 @@ namespace osu.Game.Tests.Visual.Multiplayer AddAssert("item 1 is selected", () => playlist.SelectedItem.Value == playlist.Items[1]); } - [Test] - public void TestItemRemovedOnDeletion() - { - PlaylistItem selectedItem = null; - - createPlaylist(true, true); - - moveToItem(0); - AddStep("click", () => InputManager.Click(MouseButton.Left)); - AddStep("retrieve selection", () => selectedItem = playlist.SelectedItem.Value); - - moveToDeleteButton(0); - AddStep("click delete button", () => InputManager.Click(MouseButton.Left)); - - AddAssert("item removed", () => !playlist.Items.Contains(selectedItem)); - } - - [Test] - public void TestNextItemSelectedAfterDeletion() - { - createPlaylist(true, true); - - moveToItem(0); - AddStep("click", () => InputManager.Click(MouseButton.Left)); - - moveToDeleteButton(0); - AddStep("click delete button", () => InputManager.Click(MouseButton.Left)); - - AddAssert("item 0 is selected", () => playlist.SelectedItem.Value == playlist.Items[0]); - } - - [Test] - public void TestLastItemSelectedAfterLastItemDeleted() - { - createPlaylist(true, true); - - AddWaitStep("wait for flow", 5); // Items may take 1 update frame to flow. A wait count of 5 is guaranteed to result in the flow being updated as desired. - AddStep("scroll to bottom", () => playlist.ChildrenOfType>().First().ScrollToEnd(false)); - - moveToItem(19); - AddStep("click", () => InputManager.Click(MouseButton.Left)); - - moveToDeleteButton(19); - AddStep("click delete button", () => InputManager.Click(MouseButton.Left)); - - AddAssert("item 18 is selected", () => playlist.SelectedItem.Value == playlist.Items[18]); - } - - [Test] - public void TestSelectionResetWhenAllItemsDeleted() - { - createPlaylist(true, true); - - AddStep("remove all but one item", () => - { - playlist.Items.RemoveRange(1, playlist.Items.Count - 1); - }); - - moveToItem(0); - AddStep("click", () => InputManager.Click(MouseButton.Left)); - moveToDeleteButton(0); - AddStep("click delete button", () => InputManager.Click(MouseButton.Left)); - - AddAssert("no item selected", () => playlist.SelectedItem.Value == null); - } - - // Todo: currently not possible due to bindable list shortcomings (https://github.com/ppy/osu-framework/issues/3081) - // [Test] - public void TestNextItemSelectedAfterExternalDeletion() - { - createPlaylist(true, true); - - moveToItem(0); - AddStep("click", () => InputManager.Click(MouseButton.Left)); - AddStep("remove item 0", () => playlist.Items.RemoveAt(0)); - - AddAssert("item 0 is selected", () => playlist.SelectedItem.Value == playlist.Items[0]); - } - - [Test] - public void TestChangeBeatmapAndRemove() - { - createPlaylist(true, true); - - AddStep("change beatmap of first item", () => playlist.Items[0].BeatmapID = 30); - moveToDeleteButton(0); - AddStep("click delete button", () => InputManager.Click(MouseButton.Left)); - } - [Test] public void TestDownloadButtonHiddenWhenBeatmapExists() { @@ -326,12 +237,6 @@ namespace osu.Game.Tests.Visual.Multiplayer InputManager.MoveMouseTo(item.ChildrenOfType.PlaylistItemHandle>().Single(), offset); }); - private void moveToDeleteButton(int index, Vector2? offset = null) => AddStep($"move mouse to delete button {index}", () => - { - var item = playlist.ChildrenOfType>().ElementAt(index); - InputManager.MoveMouseTo(item.ChildrenOfType().ElementAt(0), offset); - }); - private void assertHandleVisibility(int index, bool visible) => AddAssert($"handle {index} {(visible ? "is" : "is not")} visible", () => (playlist.ChildrenOfType.PlaylistItemHandle>().ElementAt(index).Alpha > 0) == visible); @@ -425,7 +330,7 @@ namespace osu.Game.Tests.Visual.Multiplayer public new IReadOnlyDictionary> ItemMap => base.ItemMap; public TestPlaylist(bool allowEdit, bool allowSelection, bool showItemOwner = false) - : base(allowEdit, allowSelection, showItemOwner: showItemOwner) + : base(allowEdit, allowSelection, showItemOwner) { } } diff --git a/osu.Game.Tests/Visual/Multiplayer/TestScenePlaylistsRoomPlaylist.cs b/osu.Game.Tests/Visual/Multiplayer/TestScenePlaylistsRoomPlaylist.cs new file mode 100644 index 0000000000..5b0c7c7d55 --- /dev/null +++ b/osu.Game.Tests/Visual/Multiplayer/TestScenePlaylistsRoomPlaylist.cs @@ -0,0 +1,188 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System.Collections.Generic; +using System.Linq; +using NUnit.Framework; +using osu.Framework.Allocation; +using osu.Framework.Graphics; +using osu.Framework.Graphics.Containers; +using osu.Framework.Testing; +using osu.Game.Beatmaps; +using osu.Game.Beatmaps.Drawables; +using osu.Game.Database; +using osu.Game.Graphics.Containers; +using osu.Game.Online.API.Requests.Responses; +using osu.Game.Online.Rooms; +using osu.Game.Rulesets.Osu; +using osu.Game.Rulesets.Osu.Mods; +using osu.Game.Screens.OnlinePlay; +using osu.Game.Screens.OnlinePlay.Playlists; +using osu.Game.Tests.Beatmaps; +using osuTK; +using osuTK.Input; + +namespace osu.Game.Tests.Visual.Multiplayer +{ + public class TestScenePlaylistsRoomPlaylist : OsuManualInputManagerTestScene + { + private TestPlaylist playlist; + + [Cached(typeof(UserLookupCache))] + private readonly TestUserLookupCache userLookupCache = new TestUserLookupCache(); + + [Test] + public void TestItemRemovedOnDeletion() + { + PlaylistItem selectedItem = null; + + createPlaylist(true, true); + + moveToItem(0); + AddStep("click", () => InputManager.Click(MouseButton.Left)); + AddStep("retrieve selection", () => selectedItem = playlist.SelectedItem.Value); + + moveToDeleteButton(0); + AddStep("click delete button", () => InputManager.Click(MouseButton.Left)); + + AddAssert("item removed", () => !playlist.Items.Contains(selectedItem)); + } + + [Test] + public void TestNextItemSelectedAfterDeletion() + { + createPlaylist(true, true); + + moveToItem(0); + AddStep("click", () => InputManager.Click(MouseButton.Left)); + + moveToDeleteButton(0); + AddStep("click delete button", () => InputManager.Click(MouseButton.Left)); + + AddAssert("item 0 is selected", () => playlist.SelectedItem.Value == playlist.Items[0]); + } + + [Test] + public void TestLastItemSelectedAfterLastItemDeleted() + { + createPlaylist(true, true); + + AddWaitStep("wait for flow", 5); // Items may take 1 update frame to flow. A wait count of 5 is guaranteed to result in the flow being updated as desired. + AddStep("scroll to bottom", () => playlist.ChildrenOfType>().First().ScrollToEnd(false)); + + moveToItem(19); + AddStep("click", () => InputManager.Click(MouseButton.Left)); + + moveToDeleteButton(19); + AddStep("click delete button", () => InputManager.Click(MouseButton.Left)); + + AddAssert("item 18 is selected", () => playlist.SelectedItem.Value == playlist.Items[18]); + } + + [Test] + public void TestSelectionResetWhenAllItemsDeleted() + { + createPlaylist(true, true); + + AddStep("remove all but one item", () => + { + playlist.Items.RemoveRange(1, playlist.Items.Count - 1); + }); + + moveToItem(0); + AddStep("click", () => InputManager.Click(MouseButton.Left)); + moveToDeleteButton(0); + AddStep("click delete button", () => InputManager.Click(MouseButton.Left)); + + AddAssert("no item selected", () => playlist.SelectedItem.Value == null); + } + + // Todo: currently not possible due to bindable list shortcomings (https://github.com/ppy/osu-framework/issues/3081) + // [Test] + public void TestNextItemSelectedAfterExternalDeletion() + { + createPlaylist(true, true); + + moveToItem(0); + AddStep("click", () => InputManager.Click(MouseButton.Left)); + AddStep("remove item 0", () => playlist.Items.RemoveAt(0)); + + AddAssert("item 0 is selected", () => playlist.SelectedItem.Value == playlist.Items[0]); + } + + [Test] + public void TestChangeBeatmapAndRemove() + { + createPlaylist(true, true); + + AddStep("change beatmap of first item", () => playlist.Items[0].BeatmapID = 30); + moveToDeleteButton(0); + AddStep("click delete button", () => InputManager.Click(MouseButton.Left)); + } + + private void moveToItem(int index, Vector2? offset = null) + => AddStep($"move mouse to item {index}", () => InputManager.MoveMouseTo(playlist.ChildrenOfType().ElementAt(index), offset)); + + private void moveToDeleteButton(int index, Vector2? offset = null) => AddStep($"move mouse to delete button {index}", () => + { + var item = playlist.ChildrenOfType>().ElementAt(index); + InputManager.MoveMouseTo(item.ChildrenOfType().ElementAt(0), offset); + }); + + private void createPlaylist(bool allowEdit, bool allowSelection, bool showItemOwner = false) + { + AddStep("create playlist", () => + { + Child = playlist = new TestPlaylist(allowEdit, allowSelection, showItemOwner) + { + Anchor = Anchor.Centre, + Origin = Anchor.Centre, + Size = new Vector2(500, 300) + }; + + for (int i = 0; i < 20; i++) + { + playlist.Items.Add(new PlaylistItem + { + ID = i, + OwnerID = 2, + Beatmap = + { + Value = i % 2 == 1 + ? new TestBeatmap(new OsuRuleset().RulesetInfo).BeatmapInfo + : new BeatmapInfo + { + Metadata = new BeatmapMetadata + { + Artist = "Artist", + Author = new APIUser { Username = "Creator name here" }, + Title = "Long title used to check background colour", + }, + BeatmapSet = new BeatmapSetInfo() + } + }, + Ruleset = { Value = new OsuRuleset().RulesetInfo }, + RequiredMods = + { + new OsuModHardRock(), + new OsuModDoubleTime(), + new OsuModAutoplay() + } + }); + } + }); + + AddUntilStep("wait for items to load", () => playlist.ItemMap.Values.All(i => i.IsLoaded)); + } + + private class TestPlaylist : PlaylistsRoomPlaylist + { + public new IReadOnlyDictionary> ItemMap => base.ItemMap; + + public TestPlaylist(bool allowEdit, bool allowSelection, bool showItemOwner = false) + : base(allowEdit, allowSelection, showItemOwner) + { + } + } + } +} diff --git a/osu.Game/Screens/OnlinePlay/Components/MatchBeatmapDetailArea.cs b/osu.Game/Screens/OnlinePlay/Components/MatchBeatmapDetailArea.cs index b013cbafd8..7afebb04af 100644 --- a/osu.Game/Screens/OnlinePlay/Components/MatchBeatmapDetailArea.cs +++ b/osu.Game/Screens/OnlinePlay/Components/MatchBeatmapDetailArea.cs @@ -9,6 +9,7 @@ using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Game.Graphics.UserInterface; using osu.Game.Online.Rooms; +using osu.Game.Screens.OnlinePlay.Playlists; using osu.Game.Screens.Select; using osuTK; @@ -43,7 +44,7 @@ namespace osu.Game.Screens.OnlinePlay.Components { RelativeSizeAxes = Axes.Both, Padding = new MarginPadding { Bottom = 10 }, - Child = playlist = new DrawableRoomPlaylist(true, false) + Child = playlist = new PlaylistsRoomPlaylist(true, false) { RelativeSizeAxes = Axes.Both, } diff --git a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs index f2d31c8e67..35d1fb33ad 100644 --- a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs +++ b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs @@ -1,9 +1,8 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -using System.Collections.Specialized; +using System; using osu.Framework.Bindables; -using osu.Framework.Extensions.IEnumerableExtensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Game.Graphics.Containers; @@ -16,6 +15,11 @@ namespace osu.Game.Screens.OnlinePlay { public readonly Bindable SelectedItem = new Bindable(); + /// + /// Invoked when an item is requested to be deleted. + /// + public Action DeletionRequested; + private readonly bool allowEdit; private readonly bool allowSelection; private readonly bool showItemOwner; @@ -27,23 +31,6 @@ namespace osu.Game.Screens.OnlinePlay this.showItemOwner = showItemOwner; } - protected override void LoadComplete() - { - base.LoadComplete(); - - // Scheduled since items are removed and re-added upon rearrangement - Items.CollectionChanged += (_, args) => Schedule(() => - { - switch (args.Action) - { - case NotifyCollectionChangedAction.Remove: - if (allowSelection && args.OldItems.Contains(SelectedItem)) - SelectedItem.Value = null; - break; - } - }); - } - protected override ScrollContainer CreateScrollContainer() => base.CreateScrollContainer().With(d => { d.ScrollbarVisible = false; @@ -57,20 +44,7 @@ namespace osu.Game.Screens.OnlinePlay protected override OsuRearrangeableListItem CreateOsuDrawable(PlaylistItem item) => new DrawableRoomPlaylistItem(item, allowEdit, allowSelection, showItemOwner) { SelectedItem = { BindTarget = SelectedItem }, - RequestDeletion = requestDeletion + RequestDeletion = i => DeletionRequested?.Invoke(i) }; - - private void requestDeletion(PlaylistItem item) - { - if (allowSelection && SelectedItem.Value == item) - { - if (Items.Count == 1) - SelectedItem.Value = null; - else - SelectedItem.Value = Items.GetNext(item) ?? Items[^2]; - } - - Items.Remove(item); - } } } diff --git a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistWithResults.cs b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistWithResults.cs index 8b1bb7abc1..1acd239fc8 100644 --- a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistWithResults.cs +++ b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistWithResults.cs @@ -22,7 +22,7 @@ namespace osu.Game.Screens.OnlinePlay private readonly bool showItemOwner; public DrawableRoomPlaylistWithResults(bool showItemOwner = false) - : base(false, true, showItemOwner: showItemOwner) + : base(false, true, showItemOwner) { this.showItemOwner = showItemOwner; } diff --git a/osu.Game/Screens/OnlinePlay/Playlists/PlaylistsRoomPlaylist.cs b/osu.Game/Screens/OnlinePlay/Playlists/PlaylistsRoomPlaylist.cs new file mode 100644 index 0000000000..de0960940d --- /dev/null +++ b/osu.Game/Screens/OnlinePlay/Playlists/PlaylistsRoomPlaylist.cs @@ -0,0 +1,24 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System.Linq; +using osu.Framework.Extensions.IEnumerableExtensions; + +namespace osu.Game.Screens.OnlinePlay.Playlists +{ + public class PlaylistsRoomPlaylist : DrawableRoomPlaylist + { + public PlaylistsRoomPlaylist(bool allowEdit, bool allowSelection, bool showItemOwner = false) + : base(allowEdit, allowSelection, showItemOwner) + { + DeletionRequested = item => + { + var nextItem = Items.GetNext(item); + + Items.Remove(item); + + SelectedItem.Value = nextItem ?? Items.LastOrDefault(); + }; + } + } +} diff --git a/osu.Game/Screens/OnlinePlay/Playlists/PlaylistsRoomSettingsOverlay.cs b/osu.Game/Screens/OnlinePlay/Playlists/PlaylistsRoomSettingsOverlay.cs index 27c8dc1120..b903e9cb7b 100644 --- a/osu.Game/Screens/OnlinePlay/Playlists/PlaylistsRoomSettingsOverlay.cs +++ b/osu.Game/Screens/OnlinePlay/Playlists/PlaylistsRoomSettingsOverlay.cs @@ -205,7 +205,10 @@ namespace osu.Game.Screens.OnlinePlay.Playlists { new Drawable[] { - playlist = new DrawableRoomPlaylist(true, false) { RelativeSizeAxes = Axes.Both } + playlist = new PlaylistsRoomPlaylist(true, false) + { + RelativeSizeAxes = Axes.Both + } }, new Drawable[] {