From 994bd4abfeb2a9b3eb8b5a8f0551c8a706148dcb Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 10 Jul 2025 17:41:35 +0900 Subject: [PATCH] Ensure adjacent selection when currently selected beatmap(set) is deleted --- .../SongSelectV2/SongSelectTestScene.cs | 14 ++ ...neSongSelectCurrentSelectionInvalidated.cs | 198 ++++++++++++++++++ .../TestSceneSongSelectFiltering.cs | 16 +- osu.Game/Graphics/Carousel/Carousel.cs | 7 +- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 67 ++++++ 5 files changed, 293 insertions(+), 9 deletions(-) create mode 100644 osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectCurrentSelectionInvalidated.cs diff --git a/osu.Game.Tests/Visual/SongSelectV2/SongSelectTestScene.cs b/osu.Game.Tests/Visual/SongSelectV2/SongSelectTestScene.cs index c704f21fa4..8eb132dce7 100644 --- a/osu.Game.Tests/Visual/SongSelectV2/SongSelectTestScene.cs +++ b/osu.Game.Tests/Visual/SongSelectV2/SongSelectTestScene.cs @@ -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; diff --git a/osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectCurrentSelectionInvalidated.cs b/osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectCurrentSelectionInvalidated.cs new file mode 100644 index 0000000000..6bb7e0f375 --- /dev/null +++ b/osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectCurrentSelectionInvalidated.cs @@ -0,0 +1,198 @@ +// Copyright (c) ppy Pty Ltd . 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 +{ + /// + /// 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). + /// + 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(); + } + + /// + /// Make sure that deleting all sets doesn't hit some weird edge case / crash. + /// + [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(2); + makePanelSelected(3); + + // Deleting second-last, should select last + BeatmapSetInfo deletedSet = null!; + AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!)); + waitForFiltering(3); + + selectionChangedFrom(() => deletedSet); + assertPanelSelected(3); + + // Deleting last, should select previous + AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!)); + waitForFiltering(4); + + selectionChangedFrom(() => deletedSet); + assertPanelSelected(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(3); + + // Deleting second-last, should select last + BeatmapSetInfo deletedSet = null!; + AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!)); + waitForFiltering(filterCount + 1); + + selectionChangedFrom(() => deletedSet); + assertPanelSelected(3); + assertPanelSelected(0); + + // Deleting last, should select previous + AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!)); + waitForFiltering(filterCount + 2); + + selectionChangedFrom(() => deletedSet); + assertPanelSelected(2); + assertPanelSelected(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(2); + makePanelSelected(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(2); + assertPanelSelected(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(int index) + where T : Panel + { + AddStep($"click panel at index {index} if not selected", () => + { + var panel = allPanels().ElementAt(index).ChildrenOfType().Single(); + + // May have already been selected randomly. Don't click a second time or gameplay will start. + if (!panel.Selected.Value) + panel.TriggerClick(); + }); + + assertPanelSelected(index); + } + + private void selectionChangedFrom(Func deletedSet) => + AddUntilStep("selection changed", () => selectedBeatmapSet, () => Is.Not.EqualTo(deletedSet())); + + private void assertPanelSelected(int index) + where T : Panel + => AddUntilStep($"selected panel at index {index}", getActivePanelIndex, () => Is.EqualTo(index)); + + private int getActivePanelIndex() + where T : Panel + => allPanels().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 allPanels() + where T : Panel + => Carousel.ChildrenOfType().Where(p => p.Item != null).OrderBy(p => p.Y); + } +} diff --git a/osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectFiltering.cs b/osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectFiltering.cs index 19fccdf94d..838a294e53 100644 --- a/osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectFiltering.cs +++ b/osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectFiltering.cs @@ -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] diff --git a/osu.Game/Graphics/Carousel/Carousel.cs b/osu.Game/Graphics/Carousel/Carousel.cs index b0e2ad428e..eaf075cd83 100644 --- a/osu.Game/Graphics/Carousel/Carousel.cs +++ b/osu.Game/Graphics/Carousel/Carousel.cs @@ -307,7 +307,7 @@ namespace osu.Game.Graphics.Carousel /// /// Retrieve a list of all s currently displayed. /// - public IReadOnlyCollection? GetCarouselItems() => carouselItems; + public IList? GetCarouselItems() => carouselItems; private List? carouselItems; @@ -691,6 +691,11 @@ namespace osu.Game.Graphics.Carousel /// protected CarouselItem? CurrentSelectionItem => currentSelection.CarouselItem; + /// + /// The index in of the current selection, if available. + /// + protected int? CurrentSelectionIndex => currentSelection.Index; + /// /// Becomes invalid when the current selection has changed and needs to be updated visually. /// diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index 1b8d8b506d..eb360a1f60 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -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;