Ensure adjacent selection when currently selected beatmap(set) is deleted

2026-05-18 12:40:18 +08:00 · 2025-07-10 17:41:35 +09:00
parent f1c8b1e1b2
commit 994bd4abfe
5 changed files with 293 additions and 9 deletions
@@ -5,6 +5,7 @@ using System;
 using System.Linq;
 using osu.Framework.Allocation;
 using osu.Framework.Audio;
+using osu.Framework.Extensions;
 using osu.Framework.Extensions.ObjectExtensions;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
@@ -145,6 +146,19 @@ namespace osu.Game.Tests.Visual.SongSelectV2
            AddUntilStep("wait for filtering", () => !Carousel.IsFiltering);
        }

+        protected void SortBy(SortMode mode) => AddStep($"sort by {mode.GetDescription().ToLowerInvariant()}", () => Config.SetValue(OsuSetting.SongSelectSortingMode, mode));
+
+        protected void GroupBy(GroupMode mode) => AddStep($"group by {mode.GetDescription().ToLowerInvariant()}", () => Config.SetValue(OsuSetting.SongSelectGroupMode, mode));
+
+        protected void SortAndGroupBy(SortMode sort, GroupMode group)
+        {
+            AddStep($"sort by {sort.GetDescription().ToLowerInvariant()} & group by {group.GetDescription().ToLowerInvariant()}", () =>
+            {
+                Config.SetValue(OsuSetting.SongSelectSortingMode, sort);
+                Config.SetValue(OsuSetting.SongSelectGroupMode, group);
+            });
+        }
+
        protected void ImportBeatmapForRuleset(int rulesetId)
        {
            int beatmapsCount = 0;
@@ -0,0 +1,198 @@
+// 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;
+using System.Collections.Generic;
+using System.Linq;
+using NUnit.Framework;
+using osu.Framework.Testing;
+using osu.Game.Beatmaps;
+using osu.Game.Screens.Select.Filter;
+using osu.Game.Screens.SelectV2;
+
+namespace osu.Game.Tests.Visual.SongSelectV2
+{
+    /// <summary>
+    /// The fallback behaviour guaranteed by SongSelect is that a random selection will happen in worst case scenario.
+    /// Every case we're testing here is expected to have a *custom behaviour* – engaging and overriding this random selection fallback.
+    ///
+    /// The scenarios we care abouts are:
+    /// - Beatmap set deleted (select closest valid beatmap post-deletion)
+    ///
+    /// We are working with 5 sets, each with 3 difficulties (all osu! ruleset).
+    /// </summary>
+    public partial class TestSceneSongSelectCurrentSelectionInvalidated : SongSelectTestScene
+    {
+        private BeatmapInfo? selectedBeatmap => (BeatmapInfo?)Carousel.CurrentSelection;
+        private BeatmapSetInfo? selectedBeatmapSet => selectedBeatmap?.BeatmapSet;
+
+        [SetUpSteps]
+        public override void SetUpSteps()
+        {
+            base.SetUpSteps();
+
+            for (int i = 0; i < 5; i++)
+                ImportBeatmapForRuleset(0);
+
+            LoadSongSelect();
+        }
+
+        /// <summary>
+        /// Make sure that deleting all sets doesn't hit some weird edge case / crash.
+        /// </summary>
+        [TestCase(SortMode.Title)]
+        [TestCase(SortMode.Artist)]
+        [TestCase(SortMode.Difficulty)]
+        public void TestDeleteAllSets(SortMode sortMode)
+        {
+            int filterCount = sortMode != SortMode.Title ? 2 : 1;
+
+            SortBy(sortMode);
+            waitForFiltering(filterCount);
+
+            BeatmapSetInfo deletedSet = null!;
+
+            for (int i = 0; i < 4; i++)
+            {
+                AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!));
+                waitForFiltering(filterCount + 1 + i);
+                selectionChangedFrom(() => deletedSet);
+            }
+
+            // The carousel still holds an invalid selection after the final deletion. Probably fine?
+            AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!));
+            AddUntilStep("wait for no global selection", () => Beatmap.IsDefault, () => Is.True);
+        }
+
+        [Test]
+        public void DifficultiesGrouped_DeleteSet_SelectsAdjacent()
+        {
+            SortAndGroupBy(SortMode.Difficulty, GroupMode.Difficulty);
+            waitForFiltering(2);
+
+            makePanelSelected<PanelGroupStarDifficulty>(2);
+            makePanelSelected<PanelBeatmapStandalone>(3);
+
+            // Deleting second-last, should select last
+            BeatmapSetInfo deletedSet = null!;
+            AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!));
+            waitForFiltering(3);
+
+            selectionChangedFrom(() => deletedSet);
+            assertPanelSelected<PanelBeatmapStandalone>(3);
+
+            // Deleting last, should select previous
+            AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!));
+            waitForFiltering(4);
+
+            selectionChangedFrom(() => deletedSet);
+            assertPanelSelected<PanelBeatmapStandalone>(2);
+        }
+
+        [TestCase(SortMode.Title)]
+        [TestCase(SortMode.Artist)]
+        public void SetsGrouped_DeleteSet_SelectsAdjacent(SortMode sortMode)
+        {
+            int filterCount = sortMode != SortMode.Title ? 2 : 1;
+
+            SortBy(sortMode);
+            waitForFiltering(filterCount);
+
+            makePanelSelected<PanelBeatmapSet>(3);
+
+            // Deleting second-last, should select last
+            BeatmapSetInfo deletedSet = null!;
+            AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!));
+            waitForFiltering(filterCount + 1);
+
+            selectionChangedFrom(() => deletedSet);
+            assertPanelSelected<PanelBeatmapSet>(3);
+            assertPanelSelected<PanelBeatmap>(0);
+
+            // Deleting last, should select previous
+            AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!));
+            waitForFiltering(filterCount + 2);
+
+            selectionChangedFrom(() => deletedSet);
+            assertPanelSelected<PanelBeatmapSet>(2);
+            assertPanelSelected<PanelBeatmap>(0);
+        }
+
+        // Same scenario as the test case above, but where selected difficulty before deletion is not first index in the expanded set.
+        // Basically ensures that the reselection is running `RequestRecommendedSelection` and not just relying on indices.
+        [TestCase(SortMode.Title)]
+        [TestCase(SortMode.Artist)]
+        public void SetsGrouped_DeleteSet_SelectsNextSetRecommendedDifficulty(SortMode sortMode)
+        {
+            int filterCount = sortMode != SortMode.Title ? 2 : 1;
+
+            SortBy(sortMode);
+            waitForFiltering(filterCount);
+
+            makePanelSelected<PanelBeatmapSet>(2);
+            makePanelSelected<PanelBeatmap>(2);
+
+            AddUntilStep("wait for beatmap to be selected", () => selectedBeatmapSet != null);
+
+            BeatmapSetInfo deletedSet = null!;
+            AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!));
+            waitForFiltering(++filterCount);
+
+            selectionChangedFrom(() => deletedSet);
+            assertPanelSelected<PanelBeatmapSet>(2);
+            assertPanelSelected<PanelBeatmap>(0);
+        }
+
+        private void waitForFiltering(int filterCount = 1)
+        {
+            AddUntilStep("wait for filter count", () => Carousel.FilterCount, () => Is.EqualTo(filterCount));
+            AddUntilStep("filtering finished", () => Carousel.IsFiltering, () => Is.False);
+        }
+
+        private void makePanelSelected<T>(int index)
+            where T : Panel
+        {
+            AddStep($"click panel at index {index} if not selected", () =>
+            {
+                var panel = allPanels<T>().ElementAt(index).ChildrenOfType<Panel>().Single();
+
+                // May have already been selected randomly. Don't click a second time or gameplay will start.
+                if (!panel.Selected.Value)
+                    panel.TriggerClick();
+            });
+
+            assertPanelSelected<T>(index);
+        }
+
+        private void selectionChangedFrom(Func<BeatmapSetInfo> deletedSet) =>
+            AddUntilStep("selection changed", () => selectedBeatmapSet, () => Is.Not.EqualTo(deletedSet()));
+
+        private void assertPanelSelected<T>(int index)
+            where T : Panel
+            => AddUntilStep($"selected panel at index {index}", getActivePanelIndex<T>, () => Is.EqualTo(index));
+
+        private int getActivePanelIndex<T>()
+            where T : Panel
+            => allPanels<T>().ToList().FindIndex(p =>
+            {
+                switch (p)
+                {
+                    case PanelBeatmapStandalone pb:
+                        return pb.Selected.Value;
+
+                    case PanelBeatmap pb:
+                        return pb.Selected.Value;
+
+                    case Panel pbs:
+                        return pbs.Expanded.Value;
+
+                    default:
+                        throw new InvalidOperationException();
+                }
+            });
+
+        private IEnumerable<T> allPanels<T>()
+            where T : Panel
+            => Carousel.ChildrenOfType<T>().Where(p => p.Item != null).OrderBy(p => p.Y);
+    }
+}
@@ -117,14 +117,14 @@ namespace osu.Game.Tests.Visual.SongSelectV2
            // TODO: old test has this step, but there doesn't seem to be any purpose for it.
            // AddUntilStep("random map selected", () => Beatmap.Value != defaultBeatmap);

-            AddStep(@"Sort by Artist", () => Config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.Artist));
-            AddStep(@"Sort by Title", () => Config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.Title));
-            AddStep(@"Sort by Author", () => Config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.Author));
-            AddStep(@"Sort by DateAdded", () => Config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.DateAdded));
-            AddStep(@"Sort by BPM", () => Config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.BPM));
-            AddStep(@"Sort by Length", () => Config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.Length));
-            AddStep(@"Sort by Difficulty", () => Config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.Difficulty));
-            AddStep(@"Sort by Source", () => Config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.Source));
+            SortBy(SortMode.Artist);
+            SortBy(SortMode.Title);
+            SortBy(SortMode.Author);
+            SortBy(SortMode.DateAdded);
+            SortBy(SortMode.BPM);
+            SortBy(SortMode.Length);
+            SortBy(SortMode.Difficulty);
+            SortBy(SortMode.Source);
        }

        [Test]
@@ -307,7 +307,7 @@ namespace osu.Game.Graphics.Carousel
        /// <summary>
        /// Retrieve a list of all <see cref="CarouselItem"/>s currently displayed.
        /// </summary>
-        public IReadOnlyCollection<CarouselItem>? GetCarouselItems() => carouselItems;
+        public IList<CarouselItem>? GetCarouselItems() => carouselItems;

        private List<CarouselItem>? carouselItems;

@@ -691,6 +691,11 @@ namespace osu.Game.Graphics.Carousel
        /// </summary>
        protected CarouselItem? CurrentSelectionItem => currentSelection.CarouselItem;

+        /// <summary>
+        /// The index in <see cref="GetCarouselItems"/> of the current selection, if available.
+        /// </summary>
+        protected int? CurrentSelectionIndex => currentSelection.Index;
+
        /// <summary>
        /// Becomes invalid when the current selection has changed and needs to be updated visually.
        /// </summary>
@@ -143,10 +143,77 @@ namespace osu.Game.Screens.SelectV2
                    break;

                case NotifyCollectionChangedAction.Remove:
+                    bool selectedSetDeleted = false;
+
                    foreach (var set in oldItems!)
                    {
                        foreach (var beatmap in set.Beatmaps)
+                        {
                            Items.RemoveAll(i => i is BeatmapInfo bi && beatmap.Equals(bi));
+                            selectedSetDeleted |= CheckModelEquality(CurrentSelection, beatmap);
+                        }
+                    }
+
+                    // After removing all items in this batch, we want to make an immediate reselection
+                    // based on adjacency to the previous selection if it was deleted.
+                    //
+                    // This needs to be done immediately to avoid song select making a random selection.
+                    // This needs to be done in this class because we need to know final display order.
+                    // This needs to be done with attention to detail of which beatmaps have not been deleted.
+                    if (selectedSetDeleted && CurrentSelectionIndex != null)
+                    {
+                        var items = GetCarouselItems()!;
+                        if (items.Count == 0)
+                            break;
+
+                        bool success = false;
+
+                        // Try selecting forwards first
+                        for (int i = CurrentSelectionIndex.Value + 1; i < items.Count; i++)
+                        {
+                            if (attemptSelection(items[i]))
+                            {
+                                success = true;
+                                break;
+                            }
+                        }
+
+                        if (success)
+                            break;
+
+                        // Then try backwards (we might be at the end of available items).
+                        for (int i = Math.Min(items.Count - 1, CurrentSelectionIndex.Value); i >= 0; i--)
+                        {
+                            if (attemptSelection(items[i]))
+                                break;
+                        }
+
+                        bool attemptSelection(CarouselItem item)
+                        {
+                            if (CheckValidForSetSelection(item))
+                            {
+                                if (item.Model is BeatmapInfo beatmapInfo)
+                                {
+                                    // check the new selection wasn't deleted above
+                                    if (!Items.Contains(beatmapInfo))
+                                        return false;
+
+                                    RequestSelection(beatmapInfo);
+                                    return true;
+                                }
+
+                                if (item.Model is BeatmapSetInfo beatmapSetInfo)
+                                {
+                                    if (oldItems.Contains(beatmapSetInfo))
+                                        return false;
+
+                                    RequestRecommendedSelection(beatmapSetInfo.Beatmaps);
+                                    return true;
+                                }
+                            }
+
+                            return false;
+                        }
                    }

                    break;