Merge pull request #17598 from smoogipoo/playlist-quick-reorder

Fix playlist refreshing all items on order change

osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerPlaylist.cs

@@ -13,6 +13,7 @@ using osu.Framework.Testing;
 using osu.Game.Beatmaps;
 using osu.Game.Graphics.Sprites;
 using osu.Game.Graphics.UserInterface;
+using osu.Game.Online.API.Requests.Responses;
 using osu.Game.Online.Multiplayer;
 using osu.Game.Online.Rooms;
 using osu.Game.Rulesets;
@@ -183,14 +184,41 @@ namespace osu.Game.Tests.Visual.Multiplayer
             assertItemInHistoryListStep(2, 0);
         }
 
+        [Test]
+        public void TestInsertedItemDoesNotRefreshAllOthers()
+        {
+            AddStep("change to round robin queue mode", () => MultiplayerClient.ChangeSettings(new MultiplayerRoomSettings { QueueMode = QueueMode.AllPlayersRoundRobin }).WaitSafely());
+
+            // Add a few items for the local user.
+            addItemStep();
+            addItemStep();
+            addItemStep();
+            addItemStep();
+            addItemStep();
+
+            DrawableRoomPlaylistItem[] drawableItems = null;
+            AddStep("get drawable items", () => drawableItems = this.ChildrenOfType<DrawableRoomPlaylistItem>().ToArray());
+
+            // Add 1 item for another user.
+            AddStep("join second user", () => MultiplayerClient.AddUser(new APIUser { Id = 10 }));
+            addItemStep(userId: 10);
+
+            // New item inserted towards the top of the list.
+            assertItemInQueueListStep(7, 1);
+            AddAssert("all previous playlist items remained", () => drawableItems.All(this.ChildrenOfType<DrawableRoomPlaylistItem>().Contains));
+        }
+
         /// <summary>
         /// Adds a step to create a new playlist item.
         /// </summary>
-        private void addItemStep(bool expired = false) => AddStep("add item", () => MultiplayerClient.AddPlaylistItem(new MultiplayerPlaylistItem(new PlaylistItem(importedBeatmap)
+        private void addItemStep(bool expired = false, int? userId = null) => AddStep("add item", () =>
         {
-            Expired = expired,
+            MultiplayerClient.AddUserPlaylistItem(userId ?? API.LocalUser.Value.OnlineID, new MultiplayerPlaylistItem(new PlaylistItem(importedBeatmap)
-            PlayedAt = DateTimeOffset.Now
+            {
-        })));
+                Expired = expired,
+                PlayedAt = DateTimeOffset.Now
+            })).WaitSafely();
+        });
 
         /// <summary>
         /// Asserts the position of a given playlist item in the queue list.

osu.Game/Online/Rooms/PlaylistItem.cs

@@ -11,6 +11,7 @@ using osu.Framework.Bindables;
 using osu.Game.Beatmaps;
 using osu.Game.Online.API;
 using osu.Game.Online.API.Requests.Responses;
+using osu.Game.Utils;
 
 namespace osu.Game.Online.Rooms
 {
@@ -84,6 +85,19 @@ namespace osu.Game.Online.Rooms
             Beatmap = beatmap;
         }
 
+        public PlaylistItem(MultiplayerPlaylistItem item)
+            : this(new APIBeatmap { OnlineID = item.BeatmapID })
+        {
+            ID = item.ID;
+            OwnerID = item.OwnerID;
+            RulesetID = item.RulesetID;
+            Expired = item.Expired;
+            PlaylistOrder = item.PlaylistOrder;
+            PlayedAt = item.PlayedAt;
+            RequiredMods = item.RequiredMods.ToArray();
+            AllowedMods = item.AllowedMods.ToArray();
+        }
+
         public void MarkInvalid() => valid.Value = false;
 
         #region Newtonsoft.Json implicit ShouldSerialize() methods
@@ -101,13 +115,13 @@ namespace osu.Game.Online.Rooms
 
         #endregion
 
-        public PlaylistItem With(IBeatmapInfo beatmap) => new PlaylistItem(beatmap)
+        public PlaylistItem With(Optional<IBeatmapInfo> beatmap = default, Optional<ushort?> playlistOrder = default) => new PlaylistItem(beatmap.GetOr(Beatmap))
         {
             ID = ID,
             OwnerID = OwnerID,
             RulesetID = RulesetID,
             Expired = Expired,
-            PlaylistOrder = PlaylistOrder,
+            PlaylistOrder = playlistOrder.GetOr(PlaylistOrder),
             PlayedAt = PlayedAt,
             AllowedMods = AllowedMods,
             RequiredMods = RequiredMods,
@@ -119,6 +133,7 @@ namespace osu.Game.Online.Rooms
                && Beatmap.OnlineID == other.Beatmap.OnlineID
                && RulesetID == other.RulesetID
                && Expired == other.Expired
+               && PlaylistOrder == other.PlaylistOrder
                && AllowedMods.SequenceEqual(other.AllowedMods)
                && RequiredMods.SequenceEqual(other.RequiredMods);
     }

osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs

@@ -117,8 +117,24 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist
         {
             base.PlaylistItemChanged(item);
 
-            removeItemFromLists(item.ID);
+            var newApiItem = Playlist.SingleOrDefault(i => i.ID == item.ID);
-            addItemToLists(item);
+            var existingApiItemInQueue = queueList.Items.SingleOrDefault(i => i.ID == item.ID);
+
+            // Test if the only change between the two playlist items is the order.
+            if (newApiItem != null && existingApiItemInQueue != null && existingApiItemInQueue.With(playlistOrder: newApiItem.PlaylistOrder).Equals(newApiItem))
+            {
+                // Set the new playlist order directly without refreshing the DrawablePlaylistItem.
+                existingApiItemInQueue.PlaylistOrder = newApiItem.PlaylistOrder;
+
+                // The following isn't really required, but is here for safety and explicitness.
+                // MultiplayerQueueList internally binds to changes in Playlist to invalidate its own layout, which is mutated on every playlist operation.
+                queueList.Invalidate();
+            }
+            else
+            {
+                removeItemFromLists(item.ID);
+                addItemToLists(item);
+            }
         }
 
         private void addItemToLists(MultiplayerPlaylistItem item)

osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs

@@ -31,7 +31,11 @@ namespace osu.Game.Tests.Visual.Multiplayer
         public override IBindable<bool> IsConnected => isConnected;
         private readonly Bindable<bool> isConnected = new Bindable<bool>(true);
 
+        /// <summary>
+        /// The local client's <see cref="Room"/>. This is not always equivalent to the server-side room.
+        /// </summary>
         public new Room? APIRoom => base.APIRoom;
+
         public Action<MultiplayerRoom>? RoomSetupAction;
 
         public bool RoomJoined { get; private set; }
@@ -46,6 +50,11 @@ namespace osu.Game.Tests.Visual.Multiplayer
         /// </summary>
         private readonly List<MultiplayerPlaylistItem> serverSidePlaylist = new List<MultiplayerPlaylistItem>();
 
+        /// <summary>
+        /// Guaranteed up-to-date API room.
+        /// </summary>
+        private Room? serverSideAPIRoom;
+
         private MultiplayerPlaylistItem? currentItem => Room?.Playlist[currentIndex];
         private int currentIndex;
         private long lastPlaylistItemId;
@@ -192,13 +201,13 @@ namespace osu.Game.Tests.Visual.Multiplayer
 
         protected override async Task<MultiplayerRoom> JoinRoom(long roomId, string? password = null)
         {
-            var apiRoom = roomManager.ServerSideRooms.Single(r => r.RoomID.Value == roomId);
+            serverSideAPIRoom = roomManager.ServerSideRooms.Single(r => r.RoomID.Value == roomId);
 
-            if (password != apiRoom.Password.Value)
+            if (password != serverSideAPIRoom.Password.Value)
                 throw new InvalidOperationException("Invalid password.");
 
             serverSidePlaylist.Clear();
-            serverSidePlaylist.AddRange(apiRoom.Playlist.Select(item => new MultiplayerPlaylistItem(item)));
+            serverSidePlaylist.AddRange(serverSideAPIRoom.Playlist.Select(item => new MultiplayerPlaylistItem(item)));
             lastPlaylistItemId = serverSidePlaylist.Max(item => item.ID);
 
             var localUser = new MultiplayerRoomUser(api.LocalUser.Value.Id)
@@ -210,11 +219,11 @@ namespace osu.Game.Tests.Visual.Multiplayer
             {
                 Settings =
                 {
-                    Name = apiRoom.Name.Value,
+                    Name = serverSideAPIRoom.Name.Value,
-                    MatchType = apiRoom.Type.Value,
+                    MatchType = serverSideAPIRoom.Type.Value,
                     Password = password,
-                    QueueMode = apiRoom.QueueMode.Value,
+                    QueueMode = serverSideAPIRoom.QueueMode.Value,
-                    AutoStartDuration = apiRoom.AutoStartDuration.Value
+                    AutoStartDuration = serverSideAPIRoom.AutoStartDuration.Value
                 },
                 Playlist = serverSidePlaylist.ToList(),
                 Users = { localUser },
@@ -449,8 +458,8 @@ namespace osu.Game.Tests.Visual.Multiplayer
         public async Task EditUserPlaylistItem(int userId, MultiplayerPlaylistItem item)
         {
             Debug.Assert(Room != null);
-            Debug.Assert(APIRoom != null);
             Debug.Assert(currentItem != null);
+            Debug.Assert(serverSideAPIRoom != null);
 
             item.OwnerID = userId;
 
@@ -469,6 +478,7 @@ namespace osu.Game.Tests.Visual.Multiplayer
             item.PlaylistOrder = existingItem.PlaylistOrder;
 
             serverSidePlaylist[serverSidePlaylist.IndexOf(existingItem)] = item;
+            serverSideAPIRoom.Playlist[serverSideAPIRoom.Playlist.IndexOf(serverSideAPIRoom.Playlist.Single(i => i.ID == item.ID))] = new PlaylistItem(item);
 
             await ((IMultiplayerClient)this).PlaylistItemChanged(item).ConfigureAwait(false);
         }
@@ -479,6 +489,7 @@ namespace osu.Game.Tests.Visual.Multiplayer
         {
             Debug.Assert(Room != null);
             Debug.Assert(APIRoom != null);
+            Debug.Assert(serverSideAPIRoom != null);
 
             var item = serverSidePlaylist.Find(i => i.ID == playlistItemId);
 
@@ -495,6 +506,7 @@ namespace osu.Game.Tests.Visual.Multiplayer
                 throw new InvalidOperationException("Attempted to remove an item which has already been played.");
 
             serverSidePlaylist.Remove(item);
+            serverSideAPIRoom.Playlist.RemoveAll(i => i.ID == item.ID);
             await ((IMultiplayerClient)this).PlaylistItemRemoved(playlistItemId).ConfigureAwait(false);
 
             await updateCurrentItem(Room).ConfigureAwait(false);
@@ -576,10 +588,12 @@ namespace osu.Game.Tests.Visual.Multiplayer
         private async Task addItem(MultiplayerPlaylistItem item)
         {
             Debug.Assert(Room != null);
+            Debug.Assert(serverSideAPIRoom != null);
 
             item.ID = ++lastPlaylistItemId;
 
             serverSidePlaylist.Add(item);
+            serverSideAPIRoom.Playlist.Add(new PlaylistItem(item));
             await ((IMultiplayerClient)this).PlaylistItemAdded(item).ConfigureAwait(false);
 
             await updatePlaylistOrder(Room).ConfigureAwait(false);
@@ -603,6 +617,8 @@ namespace osu.Game.Tests.Visual.Multiplayer
 
         private async Task updatePlaylistOrder(MultiplayerRoom room)
         {
+            Debug.Assert(serverSideAPIRoom != null);
+
             List<MultiplayerPlaylistItem> orderedActiveItems;
 
             switch (room.Settings.QueueMode)
@@ -648,6 +664,10 @@ namespace osu.Game.Tests.Visual.Multiplayer
 
                 await ((IMultiplayerClient)this).PlaylistItemChanged(item).ConfigureAwait(false);
             }
+
+            // Also ensure that the API room's playlist is correct.
+            foreach (var item in serverSideAPIRoom.Playlist)
+                item.PlaylistOrder = serverSidePlaylist.Single(i => i.ID == item.ID).PlaylistOrder;
         }
     }
 }