Add automatic downloading support when spectating a multiplayer room

[peppy]

Addresses #29634 (quite literally the spectator button downloads the beatmap 😅).

See inline commentary for commentary.

[smoogipoo]

Do you have a testing setup for this? This doesn't properly consider the ordering of items in the playlist - it's not always .First(!Expired).

diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerSpectateButton.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerSpectateButton.cs
index bb6cd6cdaa..ad0d89de84 100644
--- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerSpectateButton.cs
+++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerSpectateButton.cs
@@ -1,7 +1,6 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
-using System.Linq;
 using System.Threading;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
@@ -58,6 +57,12 @@ private void load(OsuConfigManager config)
             automaticallyDownload = config.GetBindable<bool>(OsuSetting.AutomaticallyDownloadMissingBeatmaps);
         }
 
+        protected override void LoadComplete()
+        {
+            base.LoadComplete();
+            CurrentPlaylistItem.BindValueChanged(currentItemChanged, true);
+        }
+
         protected override void OnRoomUpdated()
         {
             base.OnRoomUpdated();
@@ -83,8 +88,6 @@ private void updateState()
             button.Enabled.Value = Client.Room != null
                                    && Client.Room.State != MultiplayerRoomState.Closed
                                    && !operationInProgress.Value;
-
-            Scheduler.AddOnce(checkForAutomaticDownload);
         }
 
         #region Automatic download handling
@@ -102,19 +105,18 @@ private void updateState()
 
         private CancellationTokenSource? downloadCheckCancellation;
 
-        protected override void PlaylistItemChanged(MultiplayerPlaylistItem item)
+        private void currentItemChanged(ValueChangedEvent<PlaylistItem> item)
         {
-            base.PlaylistItemChanged(item);
             Scheduler.AddOnce(checkForAutomaticDownload);
         }
 
         private void checkForAutomaticDownload()
         {
-            MultiplayerPlaylistItem? item = Client.Room?.Playlist.FirstOrDefault(i => !i.Expired);
+            PlaylistItem? currentItem = CurrentPlaylistItem.Value;
 
             downloadCheckCancellation?.Cancel();
 
-            if (item == null)
+            if (currentItem == null)
                 return;
 
             if (!automaticallyDownload.Value)
@@ -132,7 +134,7 @@ private void checkForAutomaticDownload()
             // In a perfect world we'd use BeatmapAvailability, but there's no event-driven flow for when a selection changes.
             // ie. if selection changes from "not downloaded" to another "not downloaded" we wouldn't get a value changed raised.
             beatmapLookupCache
-                .GetBeatmapAsync(item.BeatmapID, (downloadCheckCancellation = new CancellationTokenSource()).Token)
+                .GetBeatmapAsync(currentItem.Beatmap.OnlineID, (downloadCheckCancellation = new CancellationTokenSource()).Token)
                 .ContinueWith(resolved => Schedule(() =>
                 {
                     var beatmapSet = resolved.GetResultSafely()?.BeatmapSet;

[peppy]

My test setup was joining a multiplayer game and hitting spectator 😄 .

I had something similar to what you had here before moving the code inside the spectate button but changed it for unknown reasons.

[smoogipoo]

Shouldn't this respond to changes of this setting? If it's initially disabled and the user enables it, it won't start the download.

[bdach]

seems to work