Merge branch 'master' into editorfix

osu.Game.Tests/Visual/Menus/TestSceneMusicActionHandling.cs

@@ -79,5 +79,114 @@ namespace osu.Game.Tests.Visual.Menus
                 trackChangeQueue.Peek().changeDirection == TrackChangeDirection.Next);
             AddAssert("track actually changed", () => !trackChangeQueue.First().working.BeatmapInfo.Equals(trackChangeQueue.Last().working.BeatmapInfo));
         }
+
+        [Test]
+        public void TestShuffleBackwards()
+        {
+            Queue<(IWorkingBeatmap working, TrackChangeDirection changeDirection)> trackChangeQueue = null!;
+
+            AddStep("enable shuffle", () => Game.MusicController.Shuffle.Value = true);
+
+            // ensure we have at least two beatmaps available to identify the direction the music controller navigated to.
+            AddRepeatStep("import beatmap", () => Game.BeatmapManager.Import(TestResources.CreateTestBeatmapSetInfo()), 5);
+            AddStep("ensure nonzero track duration", () => Game.Realm.Write(r =>
+            {
+                // this was already supposed to be non-zero (see innards of `TestResources.CreateTestBeatmapSetInfo()`),
+                // but the non-zero value is being overwritten *to* zero by `BeatmapUpdater`.
+                // do a bit of a hack to change it back again - otherwise tracks are going to switch instantly and we won't be able to assert anything sane anymore.
+                foreach (var beatmap in r.All<BeatmapInfo>().Where(b => b.Length == 0))
+                    beatmap.Length = 60_000;
+            }));
+
+            AddStep("bind to track change", () =>
+            {
+                trackChangeQueue = new Queue<(IWorkingBeatmap, TrackChangeDirection)>();
+                Game.MusicController.TrackChanged += (working, changeDirection) => trackChangeQueue.Enqueue((working, changeDirection));
+            });
+
+            AddStep("seek track to 6 second", () => Game.MusicController.SeekTo(6000));
+            AddUntilStep("wait for current time to update", () => Game.MusicController.CurrentTrack.CurrentTime > 5000);
+
+            AddStep("press previous", () => globalActionContainer.TriggerPressed(GlobalAction.MusicPrev));
+            AddAssert("no track change", () => trackChangeQueue.Count == 0);
+            AddUntilStep("track restarted", () => Game.MusicController.CurrentTrack.CurrentTime < 5000);
+
+            AddStep("press previous", () => globalActionContainer.TriggerPressed(GlobalAction.MusicPrev));
+            AddUntilStep("track changed", () => trackChangeQueue.Count == 1);
+
+            AddStep("press previous", () => globalActionContainer.TriggerPressed(GlobalAction.MusicPrev));
+            AddUntilStep("track changed", () => trackChangeQueue.Count == 2);
+
+            AddStep("press next", () => globalActionContainer.TriggerPressed(GlobalAction.MusicNext));
+            AddUntilStep("track changed", () =>
+                trackChangeQueue.Count == 3 && !trackChangeQueue.ElementAt(1).working.BeatmapInfo.Equals(trackChangeQueue.Last().working.BeatmapInfo));
+        }
+
+        [Test]
+        public void TestShuffleForwards()
+        {
+            Queue<(IWorkingBeatmap working, TrackChangeDirection changeDirection)> trackChangeQueue = null!;
+
+            AddStep("enable shuffle", () => Game.MusicController.Shuffle.Value = true);
+
+            // ensure we have at least two beatmaps available to identify the direction the music controller navigated to.
+            AddRepeatStep("import beatmap", () => Game.BeatmapManager.Import(TestResources.CreateTestBeatmapSetInfo()), 5);
+            AddStep("ensure nonzero track duration", () => Game.Realm.Write(r =>
+            {
+                // this was already supposed to be non-zero (see innards of `TestResources.CreateTestBeatmapSetInfo()`),
+                // but the non-zero value is being overwritten *to* zero by `BeatmapUpdater`.
+                // do a bit of a hack to change it back again - otherwise tracks are going to switch instantly and we won't be able to assert anything sane anymore.
+                foreach (var beatmap in r.All<BeatmapInfo>().Where(b => b.Length == 0))
+                    beatmap.Length = 60_000;
+            }));
+
+            AddStep("bind to track change", () =>
+            {
+                trackChangeQueue = new Queue<(IWorkingBeatmap, TrackChangeDirection)>();
+                Game.MusicController.TrackChanged += (working, changeDirection) => trackChangeQueue.Enqueue((working, changeDirection));
+            });
+
+            AddStep("press next", () => globalActionContainer.TriggerPressed(GlobalAction.MusicNext));
+            AddUntilStep("track changed", () => trackChangeQueue.Count == 1);
+
+            AddStep("press next", () => globalActionContainer.TriggerPressed(GlobalAction.MusicNext));
+            AddUntilStep("track changed", () => trackChangeQueue.Count == 2);
+
+            AddStep("press previous", () => globalActionContainer.TriggerPressed(GlobalAction.MusicPrev));
+            AddUntilStep("track changed", () =>
+                trackChangeQueue.Count == 3 && !trackChangeQueue.ElementAt(1).working.BeatmapInfo.Equals(trackChangeQueue.Last().working.BeatmapInfo));
+        }
+
+        [Test]
+        public void TestShuffleBackAndForth()
+        {
+            Queue<(IWorkingBeatmap working, TrackChangeDirection changeDirection)> trackChangeQueue = null!;
+
+            AddStep("enable shuffle", () => Game.MusicController.Shuffle.Value = true);
+
+            // ensure we have at least two beatmaps available to identify the direction the music controller navigated to.
+            AddRepeatStep("import beatmap", () => Game.BeatmapManager.Import(TestResources.CreateTestBeatmapSetInfo()), 5);
+            AddStep("ensure nonzero track duration", () => Game.Realm.Write(r =>
+            {
+                // this was already supposed to be non-zero (see innards of `TestResources.CreateTestBeatmapSetInfo()`),
+                // but the non-zero value is being overwritten *to* zero by `BeatmapUpdater`.
+                // do a bit of a hack to change it back again - otherwise tracks are going to switch instantly and we won't be able to assert anything sane anymore.
+                foreach (var beatmap in r.All<BeatmapInfo>().Where(b => b.Length == 0))
+                    beatmap.Length = 60_000;
+            }));
+
+            AddStep("bind to track change", () =>
+            {
+                trackChangeQueue = new Queue<(IWorkingBeatmap, TrackChangeDirection)>();
+                Game.MusicController.TrackChanged += (working, changeDirection) => trackChangeQueue.Enqueue((working, changeDirection));
+            });
+
+            AddStep("press next", () => globalActionContainer.TriggerPressed(GlobalAction.MusicNext));
+            AddUntilStep("track changed", () => trackChangeQueue.Count == 1);
+
+            AddStep("press previous", () => globalActionContainer.TriggerPressed(GlobalAction.MusicPrev));
+            AddUntilStep("track changed", () =>
+                trackChangeQueue.Count == 2 && !trackChangeQueue.First().working.BeatmapInfo.Equals(trackChangeQueue.Last().working.BeatmapInfo));
+        }
     }
 }

osu.Game/Overlays/MusicController.cs

@@ -74,6 +74,7 @@ namespace osu.Game.Overlays
         private readonly Bindable<RandomSelectAlgorithm> randomSelectAlgorithm = new Bindable<RandomSelectAlgorithm>();
         private readonly List<Live<BeatmapSetInfo>> previousRandomSets = new List<Live<BeatmapSetInfo>>();
         private int randomHistoryDirection;
+        private int lastRandomTrackDirection;
 
         [BackgroundDependencyLoader]
         private void load(AudioManager audio, OsuConfigManager configManager)
@@ -370,6 +371,8 @@ namespace osu.Game.Overlays
         }
 
         private Live<BeatmapSetInfo>? getNextRandom(int direction, bool allowProtectedTracks)
+        {
+            try
             {
                 Live<BeatmapSetInfo> result;
 
@@ -385,11 +388,22 @@ namespace osu.Game.Overlays
                 if (randomHistoryDirection * direction < 0)
                 {
                     Debug.Assert(Math.Abs(randomHistoryDirection) == previousRandomSets.Count);
-                result = previousRandomSets[^1];
+
+                    // if the user has been shuffling backwards and now going forwards (or vice versa),
+                    // the topmost item from history needs to be discarded because it's the *current* track.
+                    if (direction * lastRandomTrackDirection < 0)
+                    {
                         previousRandomSets.RemoveAt(previousRandomSets.Count - 1);
                         randomHistoryDirection += direction;
+                    }
+
+                    if (previousRandomSets.Count > 0)
+                    {
+                        result = previousRandomSets[^1];
+                        previousRandomSets.RemoveAt(previousRandomSets.Count - 1);
                         return result;
                     }
+                }
 
                 // if the early-return above didn't cover it, it means that we have no history to fall back on
                 // and need to actually choose something random.
@@ -417,9 +431,14 @@ namespace osu.Game.Overlays
                 }
 
                 previousRandomSets.Add(result);
-            randomHistoryDirection += direction;
                 return result;
             }
+            finally
+            {
+                randomHistoryDirection += direction;
+                lastRandomTrackDirection = direction;
+            }
+        }
 
         private void restartTrack()
         {

osu.Game/Screens/Edit/Compose/Components/SelectionBox.cs

@@ -150,13 +150,25 @@ namespace osu.Game.Screens.Edit.Compose.Components
             switch (e.Key)
             {
                 case Key.G:
-                    return CanReverse && reverseButton?.TriggerClick() == true;
+                    if (!CanReverse || reverseButton == null)
+                        return false;
+
+                    reverseButton.TriggerAction();
+                    return true;
 
                 case Key.Comma:
-                    return canRotate.Value && rotateCounterClockwiseButton?.TriggerClick() == true;
+                    if (!canRotate.Value || rotateCounterClockwiseButton == null)
+                        return false;
+
+                    rotateCounterClockwiseButton.TriggerAction();
+                    return true;
 
                 case Key.Period:
-                    return canRotate.Value && rotateClockwiseButton?.TriggerClick() == true;
+                    if (!canRotate.Value || rotateClockwiseButton == null)
+                        return false;
+
+                    rotateClockwiseButton.TriggerAction();
+                    return true;
             }
 
             return base.OnKeyDown(e);
@@ -285,7 +297,7 @@ namespace osu.Game.Screens.Edit.Compose.Components
                 Action = action
             };
 
-            button.OperationStarted += freezeButtonPosition;
+            button.Clicked += freezeButtonPosition;
             button.HoverLost += unfreezeButtonPosition;
 
             button.OperationStarted += operationStarted;

osu.Game/Screens/Edit/Compose/Components/SelectionBoxButton.cs

@@ -21,6 +21,8 @@ namespace osu.Game.Screens.Edit.Compose.Components
 
         public Action? Action;
 
+        public event Action? Clicked;
+
         public event Action? HoverLost;
 
         public SelectionBoxButton(IconUsage iconUsage, string tooltip)
@@ -49,11 +51,10 @@ namespace osu.Game.Screens.Edit.Compose.Components
 
         protected override bool OnClick(ClickEvent e)
         {
-            Circle.FlashColour(Colours.GrayF, 300);
+            Clicked?.Invoke();
+
+            TriggerAction();
 
-            TriggerOperationStarted();
-            Action?.Invoke();
-            TriggerOperationEnded();
             return true;
         }
 
@@ -71,5 +72,14 @@ namespace osu.Game.Screens.Edit.Compose.Components
         }
 
         public LocalisableString TooltipText { get; }
+
+        public void TriggerAction()
+        {
+            Circle.FlashColour(Colours.GrayF, 300);
+
+            TriggerOperationStarted();
+            Action?.Invoke();
+            TriggerOperationEnded();
+        }
     }
 }