From 51b7e920c0971603b80cc00c81cdcfa3341a0efd Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Tue, 21 Dec 2021 16:57:56 +0900 Subject: [PATCH 1/5] Fix delete button being able to show on current item --- .../Multiplayer/Match/Playlist/MultiplayerQueueList.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs index 3e0f663d42..70d01d41f0 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs @@ -78,9 +78,12 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist private void updateDeleteButtonVisibility() { + if (multiplayerClient.Room == null) + return; + bool isItemOwner = Item.OwnerID == api.LocalUser.Value.OnlineID || multiplayerClient.IsHost; - AllowDeletion = isItemOwner && SelectedItem.Value != Item; + AllowDeletion = isItemOwner && Item.ID != multiplayerClient.Room.Settings.PlaylistItemId; AllowEditing = isItemOwner; } } From a34d24fb20bc57361e797debbe636bf6622f059f Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Tue, 21 Dec 2021 18:37:42 +0900 Subject: [PATCH 2/5] Disallow expired items from showing delete button This isn't possible in practice since expired items are removed from the queue list, but this helps out in tests. --- .../Multiplayer/Match/Playlist/MultiplayerQueueList.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs index 70d01d41f0..4143ede949 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs @@ -83,8 +83,8 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist bool isItemOwner = Item.OwnerID == api.LocalUser.Value.OnlineID || multiplayerClient.IsHost; - AllowDeletion = isItemOwner && Item.ID != multiplayerClient.Room.Settings.PlaylistItemId; - AllowEditing = isItemOwner; + AllowDeletion = isItemOwner && !Item.Expired && Item.ID != multiplayerClient.Room.Settings.PlaylistItemId; + AllowEditing = isItemOwner && !Item.Expired; } } } From e2f8c7108117cdffb28b70427e744fa5ae1eef52 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Tue, 21 Dec 2021 18:38:31 +0900 Subject: [PATCH 3/5] Fix test --- .../Multiplayer/TestSceneMultiplayerQueueList.cs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerQueueList.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerQueueList.cs index ef71dfe772..33741e3aa2 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerQueueList.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerQueueList.cs @@ -6,7 +6,6 @@ using System.Linq; using NUnit.Framework; using osu.Framework.Allocation; using osu.Framework.Audio; -using osu.Framework.Bindables; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Framework.Platform; @@ -26,8 +25,6 @@ namespace osu.Game.Tests.Visual.Multiplayer { public class TestSceneMultiplayerQueueList : MultiplayerTestScene { - private readonly Bindable selectedItem = new Bindable(); - [Cached(typeof(UserLookupCache))] private readonly TestUserLookupCache userLookupCache = new TestUserLookupCache(); @@ -50,14 +47,11 @@ namespace osu.Game.Tests.Visual.Multiplayer AddStep("create playlist", () => { - selectedItem.Value = null; - Child = playlist = new MultiplayerQueueList { Anchor = Anchor.Centre, Origin = Anchor.Centre, Size = new Vector2(500, 300), - SelectedItem = { BindTarget = selectedItem }, Items = { BindTarget = Client.APIRoom!.Playlist } }; }); @@ -111,12 +105,13 @@ namespace osu.Game.Tests.Visual.Multiplayer addPlaylistItem(() => API.LocalUser.Value.OnlineID); - AddStep("select item 0", () => selectedItem.Value = playlist.ChildrenOfType>().ElementAt(0).Model); assertDeleteButtonVisibility(0, false); assertDeleteButtonVisibility(1, true); - AddStep("select item 1", () => selectedItem.Value = playlist.ChildrenOfType>().ElementAt(1).Model); - assertDeleteButtonVisibility(0, true); + AddStep("finish current item", () => Client.FinishCurrentItem()); + AddUntilStep("wait for next item to be selected", () => Client.Room?.Settings.PlaylistItemId == 2); + + assertDeleteButtonVisibility(0, false); assertDeleteButtonVisibility(1, false); } From 4ce61d426099b4f99f0be921d5290913b5c7c8be Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Tue, 21 Dec 2021 18:40:26 +0900 Subject: [PATCH 4/5] Directly bind to room updates Currently doesn't really cause any difference, however it may in the future if we decide the queueing algorithm shouldn't update PlaylistOrder when an item has been played. --- .../Match/Playlist/MultiplayerQueueList.cs | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs index 4143ede949..1653d416d8 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs @@ -8,7 +8,6 @@ using osu.Framework.Bindables; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Game.Online.API; -using osu.Game.Online.API.Requests.Responses; using osu.Game.Online.Multiplayer; using osu.Game.Online.Rooms; using osuTK; @@ -54,12 +53,6 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist [Resolved] private MultiplayerClient multiplayerClient { get; set; } - [Resolved(typeof(Room), nameof(Room.Host))] - private Bindable host { get; set; } - - [Resolved(typeof(Room), nameof(Room.QueueMode))] - private Bindable queueMode { get; set; } - public QueuePlaylistItem(PlaylistItem item) : base(item) { @@ -71,11 +64,12 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist RequestDeletion = item => multiplayerClient.RemovePlaylistItem(item.ID); - host.BindValueChanged(_ => updateDeleteButtonVisibility()); - queueMode.BindValueChanged(_ => updateDeleteButtonVisibility()); - SelectedItem.BindValueChanged(_ => updateDeleteButtonVisibility(), true); + multiplayerClient.RoomUpdated += onRoomUpdated; + onRoomUpdated(); } + private void onRoomUpdated() => Scheduler.AddOnce(updateDeleteButtonVisibility); + private void updateDeleteButtonVisibility() { if (multiplayerClient.Room == null) @@ -86,6 +80,14 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist AllowDeletion = isItemOwner && !Item.Expired && Item.ID != multiplayerClient.Room.Settings.PlaylistItemId; AllowEditing = isItemOwner && !Item.Expired; } + + protected override void Dispose(bool isDisposing) + { + base.Dispose(isDisposing); + + if (multiplayerClient != null) + multiplayerClient.RoomUpdated -= onRoomUpdated; + } } } } From ee64ab638382c30dd42f16cd54e23d1bd4a2ec28 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Wed, 22 Dec 2021 09:55:16 +0900 Subject: [PATCH 5/5] Fix delete button test failures Can be tested by adding a Thread.Sleep() in DrawableRoomPlaylistItem.load(). --- .../Visual/Multiplayer/TestSceneMultiplayerQueueList.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerQueueList.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerQueueList.cs index 33741e3aa2..1a646d5e7e 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerQueueList.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerQueueList.cs @@ -110,6 +110,7 @@ namespace osu.Game.Tests.Visual.Multiplayer AddStep("finish current item", () => Client.FinishCurrentItem()); AddUntilStep("wait for next item to be selected", () => Client.Room?.Settings.PlaylistItemId == 2); + AddUntilStep("wait for two items in playlist", () => playlist.ChildrenOfType().Count() == 2); assertDeleteButtonVisibility(0, false); assertDeleteButtonVisibility(1, false); @@ -136,7 +137,10 @@ namespace osu.Game.Tests.Visual.Multiplayer } private void assertDeleteButtonVisibility(int index, bool visible) - => AddUntilStep($"delete button {index} {(visible ? "is" : "is not")} visible", - () => (playlist.ChildrenOfType().ElementAt(index).Alpha > 0) == visible); + => AddUntilStep($"delete button {index} {(visible ? "is" : "is not")} visible", () => + { + var button = playlist.ChildrenOfType().ElementAtOrDefault(index); + return (button?.Alpha > 0) == visible; + }); } }