Fix music sometimes restarting twice if exiting song select with no beatmap selected

[peppy]

Closes #23849.

Don't mind the second commit, just wanted to touch it up along the way to make this kind of case make sense:

[bdach]

I'm not sure where the failure vector is here or what this is trying to do exactly. Mind explaining this further? Is this something to do with the beatmap/ruleset debounce in song select?

[peppy]

If the current beatmap is dummy (no selection), the global track can potentially still be the previous track.

In this code song select was telling a potentially arbitrary global track to play regardless of checking whether a selection is current. Then when returning to menu the same thing would happen there:

[frenzibyte]

I imagined ensurePlayingSelected is being called when exiting song select, followed by main menu calling EnsurePlayingSomething on resume, thus causing the double-restart, and the guard in this PR is preventing the first call from happening. But turns out the story actually thickens...

To begin with:

• Only when the current track is dummy, EnsurePlayingSomething will try playing the next track.
• In song select, the current track gets played on repeat, but once exiting back to main menu, the Looping flag is disabled and MusicController is signalled to play the next track once the current one reaches completion.

With the two points above in mind, what happens in master is the following:

• Once song select is entered with no beatmaps, or once a filter is set to hide all beatmaps, DummyWorkingBeatmap is selected and ensurePlayingSelected is called, and finally the dummy/virtual track is running in loop.
• Once exiting song select, the dummy/virtual track is no longer in loop.
• Once main menu is resumed, EnsurePlayingSomething is called, and since the virtual track is the current one and is playing, then the method will play the first track available in the game (i.e. "triangles" or something).
• Once dummy/virtual track reaches completion, MusicController.onTrackCompleted is fired and another track is now played (precisely the second beatmap track available in the game).

And finally that's how MusicController restarts track twice when exiting song select.

This sounds like whenever MusicController switches to a new track, it should unsubscribe from OnCompleted on the previous track...

...and from a quick discord discussion, turns out we already have existing code to not switch track when the current playing track doesn't match the one that's just ended:

osu/osu.Game/Overlays/MusicController.cs

Lines 356 to 358 in 7bc8908

	if (!CurrentTrack.Looping && !beatmap.Disabled)
	NextTrack();

but from looking at the usage, it turns out current is not properly captured and therefore the condition of the guard above will pretty much always return true:

osu/osu.Game/Overlays/MusicController.cs

Line 347 in 7bc8908

queuedTrack.Completed += () => onTrackCompleted(current);

Something along the lines of this diff fixes the original issue:

diff --git a/osu.Game/Overlays/MusicController.cs b/osu.Game/Overlays/MusicController.cs
index 1ad5a8c08b..0d175a624c 100644
--- a/osu.Game/Overlays/MusicController.cs
+++ b/osu.Game/Overlays/MusicController.cs
@@ -316,6 +316,8 @@ private void changeTrack()
             var queuedTrack = getQueuedTrack();
 
             var lastTrack = CurrentTrack;
+            lastTrack.Completed -= onTrackCompleted;
+
             CurrentTrack = queuedTrack;
 
             // At this point we may potentially be in an async context from tests. This is extremely dangerous but we have to make do for now.
@@ -344,16 +346,12 @@ private DrawableTrack getQueuedTrack()
             // Important to keep this in its own method to avoid inadvertently capturing unnecessary variables in the callback.
             // Can lead to leaks.
             var queuedTrack = new DrawableTrack(current.LoadTrack());
-            queuedTrack.Completed += () => onTrackCompleted(current);
+            queuedTrack.Completed += onTrackCompleted;
             return queuedTrack;
         }
 
-        private void onTrackCompleted(WorkingBeatmap workingBeatmap)
+        private void onTrackCompleted()
         {
-            // the source of track completion is the audio thread, so the beatmap may have changed before firing.
-            if (current != workingBeatmap)
-                return;
-
             if (!CurrentTrack.Looping && !beatmap.Disabled)
                 NextTrack();
         }
diff --git a/osu.Game/Screens/Select/SongSelect.cs b/osu.Game/Screens/Select/SongSelect.cs
index 01c38667b1..4d6a5398c5 100644
--- a/osu.Game/Screens/Select/SongSelect.cs
+++ b/osu.Game/Screens/Select/SongSelect.cs
@@ -814,9 +814,6 @@ private void ensurePlayingSelected()
             if (!ControlGlobalMusic)
                 return;
 
-            if (Beatmap.Value is DummyWorkingBeatmap)
-                return;
-
             ITrack track = music.CurrentTrack;
 
             bool isNewTrack = !lastTrack.TryGetTarget(out var last) || last != track;

[frenzibyte]

See above.

[peppy]

Your alternative fix looks sound. I've applied it.