From 4d1c06c0612a76d527d6458d04e1bb87486482be Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Fri, 10 Dec 2021 01:03:36 +0900 Subject: [PATCH 1/5] Add support for host enqueueing in TestMultiplayerClient --- .../Multiplayer/TestMultiplayerClient.cs | 35 +++++++++++-------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs b/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs index f151add430..1516d0e473 100644 --- a/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs +++ b/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs @@ -309,31 +309,36 @@ namespace osu.Game.Tests.Visual.Multiplayer Debug.Assert(APIRoom != null); Debug.Assert(currentItem != null); + bool isNewAddition = item.ID == 0; + if (Room.Settings.QueueMode == QueueMode.HostOnly && Room.Host?.UserID != LocalUser?.UserID) throw new InvalidOperationException("Local user is not the room host."); item.OwnerID = userId; - switch (Room.Settings.QueueMode) + if (isNewAddition) { - case QueueMode.HostOnly: - // In host-only mode, the current item is re-used. - item.ID = currentItem.ID; - item.PlaylistOrder = currentItem.PlaylistOrder; + await addItem(item).ConfigureAwait(false); + await updateCurrentItem(Room).ConfigureAwait(false); + } + else + { + var existingItem = serverSidePlaylist.SingleOrDefault(i => i.ID == item.ID); - serverSidePlaylist[currentIndex] = item; - await ((IMultiplayerClient)this).PlaylistItemChanged(item).ConfigureAwait(false); + if (existingItem == null) + throw new InvalidOperationException("Attempted to change an item that doesn't exist."); - // Note: Unlike the server, this is the easiest way to update the current item at this point. - await updateCurrentItem(Room, false).ConfigureAwait(false); - break; + if (existingItem.OwnerID != userId && Room.Host?.UserID != LocalUser?.UserID) + throw new InvalidOperationException("Attempted to change an item which is not owned by the user."); - default: - await addItem(item).ConfigureAwait(false); + if (existingItem.Expired) + throw new InvalidOperationException("Attempted to change an item which has already been played."); - // The current item can change as a result of an item being added. For example, if all items earlier in the queue were expired. - await updateCurrentItem(Room).ConfigureAwait(false); - break; + // Ensure the playlist order doesn't change. + item.PlaylistOrder = existingItem.PlaylistOrder; + + serverSidePlaylist[serverSidePlaylist.IndexOf(existingItem)] = item; + await ((IMultiplayerClient)this).PlaylistItemChanged(item).ConfigureAwait(false); } } From 048a4951156593ae4c2a45d019fe15210228ca2c Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Fri, 10 Dec 2021 01:08:54 +0900 Subject: [PATCH 2/5] Add edit button to DrawableRoomPlaylistItem --- .../TestSceneDrawableRoomPlaylist.cs | 1 + .../OnlinePlay/DrawableRoomPlaylist.cs | 27 ++++++++++++- .../OnlinePlay/DrawableRoomPlaylistItem.cs | 38 +++++++++++++++++++ 3 files changed, 65 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs index 24d7d62aa3..f9784384fd 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs @@ -255,6 +255,7 @@ namespace osu.Game.Tests.Visual.Multiplayer { p.AllowDeletion = true; p.AllowShowingResults = true; + p.AllowEditing = true; }); } diff --git a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs index 5e180afcf9..57bb4253cb 100644 --- a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs +++ b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs @@ -33,6 +33,11 @@ namespace osu.Game.Screens.OnlinePlay /// public Action RequestResults; + /// + /// Invoked when an item requests to be edited. + /// + public Action RequestEdit; + private bool allowReordering; /// @@ -104,6 +109,24 @@ namespace osu.Game.Screens.OnlinePlay } } + private bool allowEditing; + + /// + /// Whether to allow items to be edited. + /// If true, requests to edit items may be satisfied via . + /// + public bool AllowEditing + { + get => allowEditing; + set + { + allowEditing = value; + + foreach (var item in ListContainer.OfType()) + item.AllowEditing = value; + } + } + private bool showItemOwners; /// @@ -135,12 +158,14 @@ namespace osu.Game.Screens.OnlinePlay { d.SelectedItem.BindTarget = SelectedItem; d.RequestDeletion = i => RequestDeletion?.Invoke(i); + d.RequestResults = i => RequestResults?.Invoke(i); + d.RequestEdit = i => RequestEdit?.Invoke(i); d.AllowReordering = AllowReordering; d.AllowDeletion = AllowDeletion; d.AllowSelection = AllowSelection; d.AllowShowingResults = AllowShowingResults; + d.AllowEditing = AllowEditing; d.ShowItemOwner = ShowItemOwners; - d.RequestResults = i => RequestResults?.Invoke(i); }); protected virtual DrawableRoomPlaylistItem CreateDrawablePlaylistItem(PlaylistItem item) => new DrawableRoomPlaylistItem(item); diff --git a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs index 9640d9db46..8042f7d772 100644 --- a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs +++ b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs @@ -51,6 +51,11 @@ namespace osu.Game.Screens.OnlinePlay /// public Action RequestResults; + /// + /// Invoked when this item requests to be edited. + /// + public Action RequestEdit; + /// /// The currently-selected item, used to show a border around this item. /// May be updated by this item if is true. @@ -74,6 +79,7 @@ namespace osu.Game.Screens.OnlinePlay private FillFlowContainer buttonsFlow; private UpdateableAvatar ownerAvatar; private Drawable showResultsButton; + private Drawable editButton; private Drawable removeButton; private PanelBackground panelBackground; private FillFlowContainer mainFillFlow; @@ -213,6 +219,23 @@ namespace osu.Game.Screens.OnlinePlay } } + private bool allowEditing; + + /// + /// Whether this item can be edited. + /// + public bool AllowEditing + { + get => allowEditing; + set + { + allowEditing = value; + + if (editButton != null) + editButton.Alpha = value ? 1 : 0; + } + } + private bool showItemOwner; /// @@ -416,6 +439,13 @@ namespace osu.Game.Screens.OnlinePlay TooltipText = "View results" }, Item.Beatmap.Value == null ? Empty() : new PlaylistDownloadButton(Item), + editButton = new PlaylistEditButton + { + Size = new Vector2(30, 30), + Alpha = AllowEditing ? 1 : 0, + Action = () => RequestEdit?.Invoke(Item), + TooltipText = "Edit" + }, removeButton = new PlaylistRemoveButton { Size = new Vector2(30, 30), @@ -432,6 +462,14 @@ namespace osu.Game.Screens.OnlinePlay return true; } + public class PlaylistEditButton : GrayButton + { + public PlaylistEditButton() + : base(FontAwesome.Solid.Edit) + { + } + } + public class PlaylistRemoveButton : GrayButton { public PlaylistRemoveButton() From 671582a9255706334c67f75a0cece57a454be14a Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Fri, 10 Dec 2021 01:15:15 +0900 Subject: [PATCH 3/5] Allow host to enqeue items and items to be edited --- .../TestSceneAllPlayersQueueMode.cs | 4 +- .../Multiplayer/TestSceneHostOnlyQueueMode.cs | 24 ++++++++++- .../Multiplayer/TestSceneMultiplayer.cs | 3 +- .../TestSceneMultiplayerMatchSongSelect.cs | 2 +- .../Match/Playlist/MultiplayerPlaylist.cs | 9 +++- .../Match/Playlist/MultiplayerQueueList.cs | 6 ++- .../Multiplayer/MultiplayerMatchSongSelect.cs | 8 +++- .../Multiplayer/MultiplayerMatchSubScreen.cs | 42 +++++++------------ .../OnlinePlay/OnlinePlaySongSelect.cs | 12 +++--- 9 files changed, 67 insertions(+), 43 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneAllPlayersQueueMode.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneAllPlayersQueueMode.cs index ccfae1deef..8373979308 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneAllPlayersQueueMode.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneAllPlayersQueueMode.cs @@ -87,9 +87,9 @@ namespace osu.Game.Tests.Visual.Multiplayer private void addItem(Func beatmap) { - AddStep("click edit button", () => + AddStep("click add button", () => { - InputManager.MoveMouseTo(this.ChildrenOfType().Single().AddOrEditPlaylistButton); + InputManager.MoveMouseTo(this.ChildrenOfType().Single()); InputManager.Click(MouseButton.Left); }); diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneHostOnlyQueueMode.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneHostOnlyQueueMode.cs index 1de7289446..ccac3de304 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneHostOnlyQueueMode.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneHostOnlyQueueMode.cs @@ -7,6 +7,7 @@ using NUnit.Framework; using osu.Framework.Testing; using osu.Game.Beatmaps; using osu.Game.Online.Multiplayer; +using osu.Game.Screens.OnlinePlay; using osu.Game.Screens.OnlinePlay.Multiplayer; using osuTK.Input; @@ -74,11 +75,19 @@ namespace osu.Game.Tests.Visual.Multiplayer AddUntilStep("api room updated", () => Client.APIRoom?.QueueMode.Value == QueueMode.AllPlayers); } + [Test] + public void TestAddItemsAsHost() + { + addItem(() => OtherBeatmap); + + AddAssert("playlist contains two items", () => Client.APIRoom?.Playlist.Count == 2); + } + private void selectNewItem(Func beatmap) { AddStep("click edit button", () => { - InputManager.MoveMouseTo(this.ChildrenOfType().Single().AddOrEditPlaylistButton); + InputManager.MoveMouseTo(this.ChildrenOfType().First()); InputManager.Click(MouseButton.Left); }); @@ -90,5 +99,18 @@ namespace osu.Game.Tests.Visual.Multiplayer AddUntilStep("wait for return to match", () => CurrentSubScreen is MultiplayerMatchSubScreen); AddUntilStep("selected item is new beatmap", () => Client.CurrentMatchPlayingItem.Value?.Beatmap.Value?.OnlineID == otherBeatmap.OnlineID); } + + private void addItem(Func beatmap) + { + AddStep("click add button", () => + { + InputManager.MoveMouseTo(this.ChildrenOfType().Single()); + InputManager.Click(MouseButton.Left); + }); + + AddUntilStep("wait for song select", () => CurrentSubScreen is Screens.Select.SongSelect select && select.BeatmapSetsLoaded); + AddStep("select other beatmap", () => ((Screens.Select.SongSelect)CurrentSubScreen).FinaliseSelection(beatmap())); + AddUntilStep("wait for return to match", () => CurrentSubScreen is MultiplayerMatchSubScreen); + } } } diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs index 2411f39ae3..5eb0abc830 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs @@ -416,8 +416,7 @@ namespace osu.Game.Tests.Visual.Multiplayer AddStep("Enter song select", () => { var currentSubScreen = ((Screens.OnlinePlay.Multiplayer.Multiplayer)multiplayerScreenStack.CurrentScreen).CurrentSubScreen; - - ((MultiplayerMatchSubScreen)currentSubScreen).SelectBeatmap(); + ((MultiplayerMatchSubScreen)currentSubScreen).OpenSongSelection(client.CurrentMatchPlayingItem.Value); }); AddUntilStep("wait for song select", () => this.ChildrenOfType().FirstOrDefault()?.BeatmapSetsLoaded == true); diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerMatchSongSelect.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerMatchSongSelect.cs index a5229702a8..d671673d3c 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerMatchSongSelect.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerMatchSongSelect.cs @@ -179,7 +179,7 @@ namespace osu.Game.Tests.Visual.Multiplayer public new BeatmapCarousel Carousel => base.Carousel; public TestMultiplayerMatchSongSelect(Room room, WorkingBeatmap beatmap = null, RulesetInfo ruleset = null) - : base(room, beatmap, ruleset) + : base(room, null, beatmap, ruleset) { } } diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs index c3245b550f..4971489769 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs @@ -1,6 +1,7 @@ // 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 System.Linq; using osu.Framework.Allocation; using osu.Framework.Bindables; @@ -19,6 +20,11 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist { public readonly Bindable DisplayMode = new Bindable(); + /// + /// Invoked when an item requests to be edited. + /// + public Action RequestEdit; + private MultiplayerQueueList queueList; private MultiplayerHistoryList historyList; private bool firstPopulation = true; @@ -46,7 +52,8 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist queueList = new MultiplayerQueueList { RelativeSizeAxes = Axes.Both, - SelectedItem = { BindTarget = SelectedItem } + SelectedItem = { BindTarget = SelectedItem }, + RequestEdit = item => RequestEdit?.Invoke(item) }, historyList = new MultiplayerHistoryList { diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs index 8832c9bd60..3e0f663d42 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs @@ -78,8 +78,10 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist private void updateDeleteButtonVisibility() { - AllowDeletion = (Item.OwnerID == api.LocalUser.Value.OnlineID || multiplayerClient.IsHost) - && SelectedItem.Value != Item; + bool isItemOwner = Item.OwnerID == api.LocalUser.Value.OnlineID || multiplayerClient.IsHost; + + AllowDeletion = isItemOwner && SelectedItem.Value != Item; + AllowEditing = isItemOwner; } } } diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSongSelect.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSongSelect.cs index 44efef53f5..80a0289ba9 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSongSelect.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSongSelect.cs @@ -24,17 +24,22 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer [Resolved] private MultiplayerClient client { get; set; } + private readonly PlaylistItem itemToEdit; + private LoadingLayer loadingLayer; /// /// Construct a new instance of multiplayer song select. /// /// The room. + /// The item to be edited. May be null, in which case a new item will be added to the playlist. /// An optional initial beatmap selection to perform. /// An optional initial ruleset selection to perform. - public MultiplayerMatchSongSelect(Room room, WorkingBeatmap beatmap = null, RulesetInfo ruleset = null) + public MultiplayerMatchSongSelect(Room room, PlaylistItem itemToEdit = null, WorkingBeatmap beatmap = null, RulesetInfo ruleset = null) : base(room) { + this.itemToEdit = itemToEdit; + if (beatmap != null || ruleset != null) { Schedule(() => @@ -61,6 +66,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer client.AddPlaylistItem(new MultiplayerPlaylistItem { + ID = itemToEdit?.ID ?? 0, BeatmapID = item.BeatmapID, BeatmapChecksum = item.Beatmap.Value.MD5Hash, RulesetID = item.RulesetID, diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs index 3a25bd7b06..96a9804067 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs @@ -14,7 +14,6 @@ using osu.Framework.Screens; using osu.Framework.Threading; using osu.Game.Beatmaps; using osu.Game.Configuration; -using osu.Game.Graphics.UserInterface; using osu.Game.Online; using osu.Game.Online.Multiplayer; using osu.Game.Online.Rooms; @@ -44,8 +43,6 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer public override string ShortTitle => "room"; - public OsuButton AddOrEditPlaylistButton { get; private set; } - [Resolved] private MultiplayerClient client { get; set; } @@ -57,6 +54,8 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer [CanBeNull] private IDisposable readyClickOperation; + private AddItemButton addItemButton; + public MultiplayerMatchSubScreen(Room room) : base(room) { @@ -134,12 +133,12 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer new Drawable[] { new OverlinedHeader("Beatmap") }, new Drawable[] { - AddOrEditPlaylistButton = new PurpleTriangleButton + addItemButton = new AddItemButton { RelativeSizeAxes = Axes.X, Height = 40, - Action = SelectBeatmap, - Alpha = 0 + Text = "Add item", + Action = () => OpenSongSelection(null) }, }, null, @@ -147,7 +146,8 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer { new MultiplayerPlaylist { - RelativeSizeAxes = Axes.Both + RelativeSizeAxes = Axes.Both, + RequestEdit = OpenSongSelection } }, new[] @@ -220,12 +220,12 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer } }; - internal void SelectBeatmap() + internal void OpenSongSelection(PlaylistItem itemToEdit) { if (!this.IsCurrentScreen()) return; - this.Push(new MultiplayerMatchSongSelect(Room)); + this.Push(new MultiplayerMatchSongSelect(Room, itemToEdit)); } protected override Drawable CreateFooter() => new MultiplayerMatchFooter @@ -385,23 +385,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer return; } - switch (client.Room.Settings.QueueMode) - { - case QueueMode.HostOnly: - AddOrEditPlaylistButton.Text = "Edit beatmap"; - AddOrEditPlaylistButton.Alpha = client.IsHost ? 1 : 0; - break; - - case QueueMode.AllPlayers: - case QueueMode.AllPlayersRoundRobin: - AddOrEditPlaylistButton.Text = "Add beatmap"; - AddOrEditPlaylistButton.Alpha = 1; - break; - - default: - AddOrEditPlaylistButton.Alpha = 0; - break; - } + addItemButton.Alpha = client.IsHost || Room.QueueMode.Value != QueueMode.HostOnly ? 1 : 0; Scheduler.AddOnce(UpdateMods); } @@ -466,7 +450,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer return; } - this.Push(new MultiplayerMatchSongSelect(Room, beatmap, ruleset)); + this.Push(new MultiplayerMatchSongSelect(Room, SelectedItem.Value, beatmap, ruleset)); } protected override void Dispose(bool isDisposing) @@ -481,5 +465,9 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer modSettingChangeTracker?.Dispose(); } + + public class AddItemButton : PurpleTriangleButton + { + } } } diff --git a/osu.Game/Screens/OnlinePlay/OnlinePlaySongSelect.cs b/osu.Game/Screens/OnlinePlay/OnlinePlaySongSelect.cs index 4bc0b55433..63957caee3 100644 --- a/osu.Game/Screens/OnlinePlay/OnlinePlaySongSelect.cs +++ b/osu.Game/Screens/OnlinePlay/OnlinePlaySongSelect.cs @@ -33,14 +33,14 @@ namespace osu.Game.Screens.OnlinePlay [Resolved(typeof(Room), nameof(Room.Playlist))] protected BindableList Playlist { get; private set; } + [CanBeNull] + [Resolved(CanBeNull = true)] + protected IBindable SelectedItem { get; private set; } + protected override UserActivity InitialActivity => new UserActivity.InLobby(room); protected readonly Bindable> FreeMods = new Bindable>(Array.Empty()); - [CanBeNull] - [Resolved(CanBeNull = true)] - private IBindable selectedItem { get; set; } - private readonly FreeModSelectOverlay freeModSelectOverlay; private readonly Room room; @@ -80,8 +80,8 @@ namespace osu.Game.Screens.OnlinePlay // At this point, Mods contains both the required and allowed mods. For selection purposes, it should only contain the required mods. // Similarly, freeMods is currently empty but should only contain the allowed mods. - Mods.Value = selectedItem?.Value?.RequiredMods.Select(m => m.DeepClone()).ToArray() ?? Array.Empty(); - FreeMods.Value = selectedItem?.Value?.AllowedMods.Select(m => m.DeepClone()).ToArray() ?? Array.Empty(); + Mods.Value = SelectedItem?.Value?.RequiredMods.Select(m => m.DeepClone()).ToArray() ?? Array.Empty(); + FreeMods.Value = SelectedItem?.Value?.AllowedMods.Select(m => m.DeepClone()).ToArray() ?? Array.Empty(); Mods.BindValueChanged(onModsChanged); Ruleset.BindValueChanged(onRulesetChanged); From 88670c3b014ce53d805dc51fd5426ef8540824bb Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 10 Dec 2021 14:14:22 +0900 Subject: [PATCH 4/5] Document `OpenSongSelection` and mark null param --- .../OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs index 96a9804067..946c749db3 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs @@ -138,7 +138,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer RelativeSizeAxes = Axes.X, Height = 40, Text = "Add item", - Action = () => OpenSongSelection(null) + Action = () => OpenSongSelection() }, }, null, @@ -220,7 +220,11 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer } }; - internal void OpenSongSelection(PlaylistItem itemToEdit) + /// + /// Opens the song selection screen to add or edit an item. + /// + /// An optional playlist item to edit. If null, a new item will be added instead. + internal void OpenSongSelection([CanBeNull] PlaylistItem itemToEdit = null) { if (!this.IsCurrentScreen()) return; From de0f37b08d60477048a9015d1dcd8bb96e019996 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Fri, 10 Dec 2021 14:44:35 +0900 Subject: [PATCH 5/5] Separate editing and adding playlist items --- .../Multiplayer/IMultiplayerRoomServer.cs | 6 ++ .../Online/Multiplayer/MultiplayerClient.cs | 2 + .../Multiplayer/OnlineMultiplayerClient.cs | 8 +++ .../Multiplayer/MultiplayerMatchSongSelect.cs | 9 ++- .../Multiplayer/TestMultiplayerClient.cs | 60 ++++++++++--------- 5 files changed, 54 insertions(+), 31 deletions(-) diff --git a/osu.Game/Online/Multiplayer/IMultiplayerRoomServer.cs b/osu.Game/Online/Multiplayer/IMultiplayerRoomServer.cs index 65467e6ba9..73fda78d00 100644 --- a/osu.Game/Online/Multiplayer/IMultiplayerRoomServer.cs +++ b/osu.Game/Online/Multiplayer/IMultiplayerRoomServer.cs @@ -83,6 +83,12 @@ namespace osu.Game.Online.Multiplayer /// The item to add. Task AddPlaylistItem(MultiplayerPlaylistItem item); + /// + /// Edits an existing playlist item with new values. + /// + /// The item to edit, containing new properties. Must have an ID. + Task EditPlaylistItem(MultiplayerPlaylistItem item); + /// /// Removes an item from the playlist. /// diff --git a/osu.Game/Online/Multiplayer/MultiplayerClient.cs b/osu.Game/Online/Multiplayer/MultiplayerClient.cs index 34dc7ea5ea..55b4def908 100644 --- a/osu.Game/Online/Multiplayer/MultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/MultiplayerClient.cs @@ -335,6 +335,8 @@ namespace osu.Game.Online.Multiplayer public abstract Task AddPlaylistItem(MultiplayerPlaylistItem item); + public abstract Task EditPlaylistItem(MultiplayerPlaylistItem item); + public abstract Task RemovePlaylistItem(long playlistItemId); Task IMultiplayerClient.RoomStateChanged(MultiplayerRoomState state) diff --git a/osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs b/osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs index 7314603603..d268d2bf69 100644 --- a/osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs @@ -162,6 +162,14 @@ namespace osu.Game.Online.Multiplayer return connection.InvokeAsync(nameof(IMultiplayerServer.AddPlaylistItem), item); } + public override Task EditPlaylistItem(MultiplayerPlaylistItem item) + { + if (!IsConnected.Value) + return Task.CompletedTask; + + return connection.InvokeAsync(nameof(IMultiplayerServer.EditPlaylistItem), item); + } + public override Task RemovePlaylistItem(long playlistItemId) { if (!IsConnected.Value) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSongSelect.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSongSelect.cs index 80a0289ba9..8d3686dd6d 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSongSelect.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSongSelect.cs @@ -4,6 +4,7 @@ using System; using System.Diagnostics; using System.Linq; +using System.Threading.Tasks; using Microsoft.AspNetCore.SignalR; using osu.Framework.Allocation; using osu.Framework.Logging; @@ -64,7 +65,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer { loadingLayer.Show(); - client.AddPlaylistItem(new MultiplayerPlaylistItem + var multiplayerItem = new MultiplayerPlaylistItem { ID = itemToEdit?.ID ?? 0, BeatmapID = item.BeatmapID, @@ -72,7 +73,11 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer RulesetID = item.RulesetID, RequiredMods = item.RequiredMods.Select(m => new APIMod(m)).ToArray(), AllowedMods = item.AllowedMods.Select(m => new APIMod(m)).ToArray() - }).ContinueWith(t => + }; + + Task task = itemToEdit != null ? client.EditPlaylistItem(multiplayerItem) : client.AddPlaylistItem(multiplayerItem); + + task.ContinueWith(t => { Schedule(() => { diff --git a/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs b/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs index 1516d0e473..d22f0415e6 100644 --- a/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs +++ b/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs @@ -309,49 +309,51 @@ namespace osu.Game.Tests.Visual.Multiplayer Debug.Assert(APIRoom != null); Debug.Assert(currentItem != null); - bool isNewAddition = item.ID == 0; - if (Room.Settings.QueueMode == QueueMode.HostOnly && Room.Host?.UserID != LocalUser?.UserID) throw new InvalidOperationException("Local user is not the room host."); item.OwnerID = userId; - if (isNewAddition) - { - await addItem(item).ConfigureAwait(false); - await updateCurrentItem(Room).ConfigureAwait(false); - } - else - { - var existingItem = serverSidePlaylist.SingleOrDefault(i => i.ID == item.ID); - - if (existingItem == null) - throw new InvalidOperationException("Attempted to change an item that doesn't exist."); - - if (existingItem.OwnerID != userId && Room.Host?.UserID != LocalUser?.UserID) - throw new InvalidOperationException("Attempted to change an item which is not owned by the user."); - - if (existingItem.Expired) - throw new InvalidOperationException("Attempted to change an item which has already been played."); - - // Ensure the playlist order doesn't change. - item.PlaylistOrder = existingItem.PlaylistOrder; - - serverSidePlaylist[serverSidePlaylist.IndexOf(existingItem)] = item; - await ((IMultiplayerClient)this).PlaylistItemChanged(item).ConfigureAwait(false); - } + await addItem(item).ConfigureAwait(false); + await updateCurrentItem(Room).ConfigureAwait(false); } public override Task AddPlaylistItem(MultiplayerPlaylistItem item) => AddUserPlaylistItem(api.LocalUser.Value.OnlineID, item); + public async Task EditUserPlaylistItem(int userId, MultiplayerPlaylistItem item) + { + Debug.Assert(Room != null); + Debug.Assert(APIRoom != null); + Debug.Assert(currentItem != null); + + item.OwnerID = userId; + + var existingItem = serverSidePlaylist.SingleOrDefault(i => i.ID == item.ID); + + if (existingItem == null) + throw new InvalidOperationException("Attempted to change an item that doesn't exist."); + + if (existingItem.OwnerID != userId && Room.Host?.UserID != LocalUser?.UserID) + throw new InvalidOperationException("Attempted to change an item which is not owned by the user."); + + if (existingItem.Expired) + throw new InvalidOperationException("Attempted to change an item which has already been played."); + + // Ensure the playlist order doesn't change. + item.PlaylistOrder = existingItem.PlaylistOrder; + + serverSidePlaylist[serverSidePlaylist.IndexOf(existingItem)] = item; + + await ((IMultiplayerClient)this).PlaylistItemChanged(item).ConfigureAwait(false); + } + + public override Task EditPlaylistItem(MultiplayerPlaylistItem item) => EditUserPlaylistItem(api.LocalUser.Value.OnlineID, item); + public async Task RemoveUserPlaylistItem(int userId, long playlistItemId) { Debug.Assert(Room != null); Debug.Assert(APIRoom != null); - if (Room.Settings.QueueMode == QueueMode.HostOnly) - throw new InvalidOperationException("Items cannot be removed in host-only mode."); - var item = serverSidePlaylist.Find(i => i.ID == playlistItemId); if (item == null)