From 11de4296210a9727b8f8dbad928409611b669e93 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 5 Feb 2025 18:59:29 +0900 Subject: [PATCH 01/10] Add support for grouping by artist --- osu.Game.Tests/Resources/TestResources.cs | 29 ++++++++++++++----- .../SongSelect/BeatmapCarouselV2TestScene.cs | 2 +- .../TestSceneBeatmapCarouselV2Basics.cs | 1 + osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 29 ++++++++++--------- .../SelectV2/BeatmapCarouselFilterGrouping.cs | 29 ++++++++++++++++++- 5 files changed, 68 insertions(+), 22 deletions(-) diff --git a/osu.Game.Tests/Resources/TestResources.cs b/osu.Game.Tests/Resources/TestResources.cs index e0572e604c..bf08097ffd 100644 --- a/osu.Game.Tests/Resources/TestResources.cs +++ b/osu.Game.Tests/Resources/TestResources.cs @@ -85,7 +85,8 @@ namespace osu.Game.Tests.Resources /// /// Number of difficulties. If null, a random number between 1 and 20 will be used. /// Rulesets to cycle through when creating difficulties. If null, osu! ruleset will be used. - public static BeatmapSetInfo CreateTestBeatmapSetInfo(int? difficultyCount = null, RulesetInfo[] rulesets = null) + /// Whether to randomise metadata to create a better distribution. + public static BeatmapSetInfo CreateTestBeatmapSetInfo(int? difficultyCount = null, RulesetInfo[] rulesets = null, bool randomiseMetadata = false) { int j = 0; @@ -95,13 +96,27 @@ namespace osu.Game.Tests.Resources int setId = GetNextTestID(); - var metadata = new BeatmapMetadata + char getRandomCharacter() { - // Create random metadata, then we can check if sorting works based on these - Artist = "Some Artist " + RNG.Next(0, 9), - Title = $"Some Song (set id {setId:000}) {Guid.NewGuid()}", - Author = { Username = "Some Guy " + RNG.Next(0, 9) }, - }; + const string chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz*"; + return chars[RNG.Next(chars.Length)]; + } + + var metadata = randomiseMetadata + ? new BeatmapMetadata + { + // Create random metadata, then we can check if sorting works based on these + Artist = $"{getRandomCharacter()}ome Artist " + RNG.Next(0, 9), + Title = $"{getRandomCharacter()}ome Song (set id {setId:000}) {Guid.NewGuid()}", + Author = { Username = $"{getRandomCharacter()}ome Guy " + RNG.Next(0, 9) }, + } + : new BeatmapMetadata + { + // Create random metadata, then we can check if sorting works based on these + Artist = "Some Artist " + RNG.Next(0, 9), + Title = $"Some Song (set id {setId:000}) {Guid.NewGuid()}", + Author = { Username = "Some Guy " + RNG.Next(0, 9) }, + }; Logger.Log($"🛠️ Generating beatmap set \"{metadata}\" for test consumption."); diff --git a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs index 72c9611fdb..f5ea959c51 100644 --- a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs +++ b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs @@ -193,7 +193,7 @@ namespace osu.Game.Tests.Visual.SongSelect protected void AddBeatmaps(int count, int? fixedDifficultiesPerSet = null) => AddStep($"add {count} beatmaps", () => { for (int i = 0; i < count; i++) - BeatmapSets.Add(TestResources.CreateTestBeatmapSetInfo(fixedDifficultiesPerSet ?? RNG.Next(1, 4))); + BeatmapSets.Add(TestResources.CreateTestBeatmapSetInfo(fixedDifficultiesPerSet ?? RNG.Next(1, 4), randomiseMetadata: true)); }); protected void RemoveAllBeatmaps() => AddStep("clear all beatmaps", () => BeatmapSets.Clear()); diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs index 8ffb51b995..a173920dc6 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs @@ -51,6 +51,7 @@ namespace osu.Game.Tests.Visual.SongSelect { AddBeatmaps(10); SortBy(new FilterCriteria { Group = GroupMode.Difficulty, Sort = SortMode.Difficulty }); + SortBy(new FilterCriteria { Group = GroupMode.Artist, Sort = SortMode.Artist }); SortBy(new FilterCriteria { Sort = SortMode.Artist }); } diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index 9f62780dda..e7311fbfbc 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -119,20 +119,12 @@ namespace osu.Game.Screens.SelectV2 return false; case BeatmapInfo beatmapInfo: + // Find any containing group. There should never be too many groups so iterating is efficient enough. + GroupDefinition? containingGroup = grouping.GroupItems.SingleOrDefault(kvp => kvp.Value.Any(i => ReferenceEquals(i.Model, beatmapInfo))).Key; - // If we have groups, we need to account for them. - if (Criteria.SplitOutDifficulties) - { - // Find the containing group. There should never be too many groups so iterating is efficient enough. - GroupDefinition? group = grouping.GroupItems.SingleOrDefault(kvp => kvp.Value.Any(i => ReferenceEquals(i.Model, beatmapInfo))).Key; - - if (group != null) - setExpandedGroup(group); - } - else - { - setExpandedSet(beatmapInfo); - } + if (containingGroup != null) + setExpandedGroup(containingGroup); + setExpandedSet(beatmapInfo); return true; } @@ -170,6 +162,7 @@ namespace osu.Game.Screens.SelectV2 { if (grouping.GroupItems.TryGetValue(group, out var items)) { + // First pass ignoring set groupings. foreach (var i in items) { if (i.Model is GroupDefinition) @@ -177,6 +170,16 @@ namespace osu.Game.Screens.SelectV2 else i.IsVisible = expanded; } + + // Second pass to hide set children when not meant to be displayed. + if (expanded) + { + foreach (var i in items) + { + if (i.Model is BeatmapSetInfo set) + setExpansionStateOfSetItems(set, i.IsExpanded); + } + } } } diff --git a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs index e4160cc0fa..d4e0a166ab 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs @@ -52,6 +52,33 @@ namespace osu.Game.Screens.SelectV2 newItems.AddRange(items); break; + case GroupMode.Artist: + groupSetsTogether = true; + char groupChar = (char)0; + + foreach (var item in items) + { + cancellationToken.ThrowIfCancellationRequested(); + + var b = (BeatmapInfo)item.Model; + + char beatmapFirstChar = char.ToUpperInvariant(b.Metadata.Artist[0]); + + if (beatmapFirstChar > groupChar) + { + groupChar = beatmapFirstChar; + var groupDefinition = new GroupDefinition($"{groupChar}"); + var groupItem = new CarouselItem(groupDefinition) { DrawHeight = GroupPanel.HEIGHT }; + + newItems.Add(groupItem); + groupItems[groupDefinition] = new HashSet { groupItem }; + } + + newItems.Add(item); + } + + break; + case GroupMode.Difficulty: groupSetsTogether = false; int starGroup = int.MinValue; @@ -91,7 +118,7 @@ namespace osu.Game.Screens.SelectV2 if (item.Model is BeatmapInfo beatmap) { - bool newBeatmapSet = lastItem == null || (lastItem.Model is BeatmapInfo lastBeatmap && lastBeatmap.BeatmapSet!.ID != beatmap.BeatmapSet!.ID); + bool newBeatmapSet = lastItem?.Model is not BeatmapInfo lastBeatmap || lastBeatmap.BeatmapSet!.ID != beatmap.BeatmapSet!.ID; if (newBeatmapSet) { From 0257b8c2ffd2dffa2b81fbf41ad88889db0ff14a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 6 Feb 2025 13:57:38 +0900 Subject: [PATCH 02/10] Move metadata randomisation local to usage --- osu.Game.Tests/Resources/TestResources.cs | 29 +++++------------- .../SongSelect/BeatmapCarouselV2TestScene.cs | 30 +++++++++++++++++-- .../TestSceneBeatmapCarouselV2Basics.cs | 3 +- 3 files changed, 37 insertions(+), 25 deletions(-) diff --git a/osu.Game.Tests/Resources/TestResources.cs b/osu.Game.Tests/Resources/TestResources.cs index bf08097ffd..e0572e604c 100644 --- a/osu.Game.Tests/Resources/TestResources.cs +++ b/osu.Game.Tests/Resources/TestResources.cs @@ -85,8 +85,7 @@ namespace osu.Game.Tests.Resources /// /// Number of difficulties. If null, a random number between 1 and 20 will be used. /// Rulesets to cycle through when creating difficulties. If null, osu! ruleset will be used. - /// Whether to randomise metadata to create a better distribution. - public static BeatmapSetInfo CreateTestBeatmapSetInfo(int? difficultyCount = null, RulesetInfo[] rulesets = null, bool randomiseMetadata = false) + public static BeatmapSetInfo CreateTestBeatmapSetInfo(int? difficultyCount = null, RulesetInfo[] rulesets = null) { int j = 0; @@ -96,27 +95,13 @@ namespace osu.Game.Tests.Resources int setId = GetNextTestID(); - char getRandomCharacter() + var metadata = new BeatmapMetadata { - const string chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz*"; - return chars[RNG.Next(chars.Length)]; - } - - var metadata = randomiseMetadata - ? new BeatmapMetadata - { - // Create random metadata, then we can check if sorting works based on these - Artist = $"{getRandomCharacter()}ome Artist " + RNG.Next(0, 9), - Title = $"{getRandomCharacter()}ome Song (set id {setId:000}) {Guid.NewGuid()}", - Author = { Username = $"{getRandomCharacter()}ome Guy " + RNG.Next(0, 9) }, - } - : new BeatmapMetadata - { - // Create random metadata, then we can check if sorting works based on these - Artist = "Some Artist " + RNG.Next(0, 9), - Title = $"Some Song (set id {setId:000}) {Guid.NewGuid()}", - Author = { Username = "Some Guy " + RNG.Next(0, 9) }, - }; + // Create random metadata, then we can check if sorting works based on these + Artist = "Some Artist " + RNG.Next(0, 9), + Title = $"Some Song (set id {setId:000}) {Guid.NewGuid()}", + Author = { Username = "Some Guy " + RNG.Next(0, 9) }, + }; Logger.Log($"🛠️ Generating beatmap set \"{metadata}\" for test consumption."); diff --git a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs index f5ea959c51..a55f79c42e 100644 --- a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs +++ b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs @@ -1,6 +1,7 @@ // 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.Linq; using NUnit.Framework; using osu.Framework.Allocation; @@ -190,12 +191,37 @@ namespace osu.Game.Tests.Visual.SongSelect /// /// The count of beatmap sets to add. /// If not null, the number of difficulties per set. If null, randomised difficulty count will be used. - protected void AddBeatmaps(int count, int? fixedDifficultiesPerSet = null) => AddStep($"add {count} beatmaps", () => + /// Whether to randomise the metadata to make groupings more uniform. + protected void AddBeatmaps(int count, int? fixedDifficultiesPerSet = null, bool randomMetadata = false) => AddStep($"add {count} beatmaps{(randomMetadata ? " with random data" : "")}", () => { for (int i = 0; i < count; i++) - BeatmapSets.Add(TestResources.CreateTestBeatmapSetInfo(fixedDifficultiesPerSet ?? RNG.Next(1, 4), randomiseMetadata: true)); + { + var beatmapSetInfo = TestResources.CreateTestBeatmapSetInfo(fixedDifficultiesPerSet ?? RNG.Next(1, 4)); + + if (randomMetadata) + { + var metadata = new BeatmapMetadata + { + // Create random metadata, then we can check if sorting works based on these + Artist = $"{getRandomCharacter()}ome Artist " + RNG.Next(0, 9), + Title = $"{getRandomCharacter()}ome Song (set id {beatmapSetInfo.OnlineID:000}) {Guid.NewGuid()}", + Author = { Username = $"{getRandomCharacter()}ome Guy " + RNG.Next(0, 9) }, + }; + + foreach (var beatmap in beatmapSetInfo.Beatmaps) + beatmap.Metadata = metadata.DeepClone(); + } + + BeatmapSets.Add(beatmapSetInfo); + } }); + private static char getRandomCharacter() + { + const string chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz*"; + return chars[RNG.Next(chars.Length)]; + } + protected void RemoveAllBeatmaps() => AddStep("clear all beatmaps", () => BeatmapSets.Clear()); protected void RemoveFirstBeatmap() => diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs index a173920dc6..41ceff3183 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs @@ -26,8 +26,9 @@ namespace osu.Game.Tests.Visual.SongSelect [Test] public void TestBasics() { - AddBeatmaps(1); AddBeatmaps(10); + AddBeatmaps(10, randomMetadata: true); + AddBeatmaps(1); RemoveFirstBeatmap(); RemoveAllBeatmaps(); } From 88ad87a78e36a7170d0ce05dd0a0a29433977f88 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 6 Feb 2025 14:30:15 +0900 Subject: [PATCH 03/10] Expose set grouping state --- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 2 +- .../SelectV2/BeatmapCarouselFilterGrouping.cs | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index e7311fbfbc..36e57c9067 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -140,7 +140,7 @@ namespace osu.Game.Screens.SelectV2 return true; case BeatmapInfo: - return Criteria.SplitOutDifficulties; + return !grouping.BeatmapSetsGroupedTogether; case GroupDefinition: return false; diff --git a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs index d4e0a166ab..29c534cbe2 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs @@ -14,6 +14,8 @@ namespace osu.Game.Screens.SelectV2 { public class BeatmapCarouselFilterGrouping : ICarouselFilter { + public bool BeatmapSetsGroupedTogether { get; private set; } + /// /// Beatmap sets contain difficulties as related panels. This dictionary holds the relationships between set-difficulties to allow expanding them on selection. /// @@ -36,8 +38,6 @@ namespace osu.Game.Screens.SelectV2 public async Task> Run(IEnumerable items, CancellationToken cancellationToken) => await Task.Run(() => { - bool groupSetsTogether; - setItems.Clear(); groupItems.Clear(); @@ -48,12 +48,12 @@ namespace osu.Game.Screens.SelectV2 switch (criteria.Group) { default: - groupSetsTogether = true; + BeatmapSetsGroupedTogether = true; newItems.AddRange(items); break; case GroupMode.Artist: - groupSetsTogether = true; + BeatmapSetsGroupedTogether = true; char groupChar = (char)0; foreach (var item in items) @@ -80,7 +80,7 @@ namespace osu.Game.Screens.SelectV2 break; case GroupMode.Difficulty: - groupSetsTogether = false; + BeatmapSetsGroupedTogether = false; int starGroup = int.MinValue; foreach (var item in items) @@ -108,7 +108,7 @@ namespace osu.Game.Screens.SelectV2 // Add set headers wherever required. CarouselItem? lastItem = null; - if (groupSetsTogether) + if (BeatmapSetsGroupedTogether) { for (int i = 0; i < newItems.Count; i++) { From 342a66b9e21e619c9192a4bd63bb2f32563c2e20 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 6 Feb 2025 14:39:11 +0900 Subject: [PATCH 04/10] Fix keyboard traversal on a collapsed group not working as intended --- osu.Game/Screens/SelectV2/Carousel.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/osu.Game/Screens/SelectV2/Carousel.cs b/osu.Game/Screens/SelectV2/Carousel.cs index 608ef207d9..6b7b1f3a9b 100644 --- a/osu.Game/Screens/SelectV2/Carousel.cs +++ b/osu.Game/Screens/SelectV2/Carousel.cs @@ -378,7 +378,7 @@ namespace osu.Game.Screens.SelectV2 { TryActivateSelection(); - // There's a chance this couldn't resolve, at which point continue with standard traversal. + // Is the selection actually changed, then we should not perform any further traversal. if (currentSelection.CarouselItem == currentKeyboardSelection.CarouselItem) return; } @@ -386,20 +386,20 @@ namespace osu.Game.Screens.SelectV2 int originalIndex; int newIndex; - if (currentSelection.Index == null) + if (currentKeyboardSelection.Index == null) { // If there's no current selection, start from either end of the full list. newIndex = originalIndex = direction > 0 ? carouselItems.Count - 1 : 0; } else { - newIndex = originalIndex = currentSelection.Index.Value; + newIndex = originalIndex = currentKeyboardSelection.Index.Value; // As a second special case, if we're group selecting backwards and the current selection isn't a group, // make sure to go back to the group header this item belongs to, so that the block below doesn't find it and stop too early. if (direction < 0) { - while (!CheckValidForGroupSelection(carouselItems[newIndex])) + while (newIndex > 0 && !CheckValidForGroupSelection(carouselItems[newIndex])) newIndex--; } } From bf377e081ad36ab88b1c7b6ef415bcb2db888bdd Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 6 Feb 2025 14:38:51 +0900 Subject: [PATCH 05/10] Reorganise tests to make more logical when manually testing --- .../SongSelect/BeatmapCarouselV2TestScene.cs | 23 ++--- ...asics.cs => TestSceneBeatmapCarouselV2.cs} | 93 ++++++------------- ...eneBeatmapCarouselV2DifficultyGrouping.cs} | 25 ++--- ...> TestSceneBeatmapCarouselV2NoGrouping.cs} | 12 ++- .../TestSceneBeatmapCarouselV2Scrolling.cs | 65 +++++++++++++ 5 files changed, 118 insertions(+), 100 deletions(-) rename osu.Game.Tests/Visual/SongSelect/{TestSceneBeatmapCarouselV2Basics.cs => TestSceneBeatmapCarouselV2.cs} (52%) rename osu.Game.Tests/Visual/SongSelect/{TestSceneBeatmapCarouselV2GroupSelection.cs => TestSceneBeatmapCarouselV2DifficultyGrouping.cs} (92%) rename osu.Game.Tests/Visual/SongSelect/{TestSceneBeatmapCarouselV2Selection.cs => TestSceneBeatmapCarouselV2NoGrouping.cs} (94%) create mode 100644 osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Scrolling.cs diff --git a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs index a55f79c42e..36226a13cc 100644 --- a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs +++ b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs @@ -17,7 +17,6 @@ using osu.Game.Database; using osu.Game.Graphics; using osu.Game.Graphics.Containers; using osu.Game.Screens.Select; -using osu.Game.Screens.Select.Filter; using osu.Game.Screens.SelectV2; using osu.Game.Tests.Beatmaps; using osu.Game.Tests.Resources; @@ -54,16 +53,6 @@ namespace osu.Game.Tests.Visual.SongSelect Scheduler.AddDelayed(updateStats, 100, true); } - [SetUpSteps] - public virtual void SetUpSteps() - { - RemoveAllBeatmaps(); - - CreateCarousel(); - - SortBy(new FilterCriteria { Sort = SortMode.Title }); - } - protected void CreateCarousel() { AddStep("create components", () => @@ -200,12 +189,14 @@ namespace osu.Game.Tests.Visual.SongSelect if (randomMetadata) { + char randomCharacter = getRandomCharacter(); + var metadata = new BeatmapMetadata { // Create random metadata, then we can check if sorting works based on these - Artist = $"{getRandomCharacter()}ome Artist " + RNG.Next(0, 9), - Title = $"{getRandomCharacter()}ome Song (set id {beatmapSetInfo.OnlineID:000}) {Guid.NewGuid()}", - Author = { Username = $"{getRandomCharacter()}ome Guy " + RNG.Next(0, 9) }, + Artist = $"{randomCharacter}ome Artist " + RNG.Next(0, 9), + Title = $"{randomCharacter}ome Song (set id {beatmapSetInfo.OnlineID:000}) {Guid.NewGuid()}", + Author = { Username = $"{randomCharacter}ome Guy " + RNG.Next(0, 9) }, }; foreach (var beatmap in beatmapSetInfo.Beatmaps) @@ -216,10 +207,12 @@ namespace osu.Game.Tests.Visual.SongSelect } }); + private static long randomCharPointer; + private static char getRandomCharacter() { const string chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz*"; - return chars[RNG.Next(chars.Length)]; + return chars[(int)((randomCharPointer++ / 2) % chars.Length)]; } protected void RemoveAllBeatmaps() => AddStep("clear all beatmaps", () => BeatmapSets.Clear()); diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2.cs similarity index 52% rename from osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs rename to osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2.cs index 41ceff3183..3c5cf16e92 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Basics.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2.cs @@ -2,102 +2,65 @@ // See the LICENCE file in the repository root for full licence text. using System.Collections.Generic; -using System.Linq; using System.Threading.Tasks; using NUnit.Framework; -using osu.Framework.Graphics.Primitives; -using osu.Framework.Testing; using osu.Framework.Utils; using osu.Game.Beatmaps; using osu.Game.Screens.Select; using osu.Game.Screens.Select.Filter; -using osu.Game.Screens.SelectV2; using osu.Game.Tests.Resources; namespace osu.Game.Tests.Visual.SongSelect { /// - /// Currently covers adding and removing of items and scrolling. - /// If we add more tests here, these two categories can likely be split out into separate scenes. + /// Covers common steps which can be used for manual testing. /// [TestFixture] - public partial class TestSceneBeatmapCarouselV2Basics : BeatmapCarouselV2TestScene + public partial class TestSceneBeatmapCarouselV2 : BeatmapCarouselV2TestScene { [Test] + [Explicit] public void TestBasics() { - AddBeatmaps(10); + CreateCarousel(); + RemoveAllBeatmaps(); + AddBeatmaps(10, randomMetadata: true); + AddBeatmaps(10); AddBeatmaps(1); + } + + [Test] + [Explicit] + public void TestSorting() + { + SortBy(new FilterCriteria { Sort = SortMode.Artist }); + SortBy(new FilterCriteria { Group = GroupMode.Difficulty, Sort = SortMode.Difficulty }); + SortBy(new FilterCriteria { Group = GroupMode.Artist, Sort = SortMode.Artist }); + } + + [Test] + [Explicit] + public void TestRemovals() + { RemoveFirstBeatmap(); RemoveAllBeatmaps(); } [Test] - public void TestOffScreenLoading() - { - AddStep("disable masking", () => Scroll.Masking = false); - AddStep("enable masking", () => Scroll.Masking = true); - } - - [Test] - public void TestAddRemoveOneByOne() + [Explicit] + public void TestAddRemoveRepeatedOps() { AddRepeatStep("add beatmaps", () => BeatmapSets.Add(TestResources.CreateTestBeatmapSetInfo(RNG.Next(1, 4))), 20); AddRepeatStep("remove beatmaps", () => BeatmapSets.RemoveAt(RNG.Next(0, BeatmapSets.Count)), 20); } [Test] - public void TestSorting() + [Explicit] + public void TestMasking() { - AddBeatmaps(10); - SortBy(new FilterCriteria { Group = GroupMode.Difficulty, Sort = SortMode.Difficulty }); - SortBy(new FilterCriteria { Group = GroupMode.Artist, Sort = SortMode.Artist }); - SortBy(new FilterCriteria { Sort = SortMode.Artist }); - } - - [Test] - public void TestScrollPositionMaintainedOnAddSecondSelected() - { - Quad positionBefore = default; - - AddBeatmaps(10); - WaitForDrawablePanels(); - - AddStep("select middle beatmap", () => Carousel.CurrentSelection = BeatmapSets.ElementAt(BeatmapSets.Count - 2)); - AddStep("scroll to selected item", () => Scroll.ScrollTo(Scroll.ChildrenOfType().Single(p => p.Selected.Value))); - - WaitForScrolling(); - - AddStep("save selected screen position", () => positionBefore = Carousel.ChildrenOfType().FirstOrDefault(p => p.Selected.Value)!.ScreenSpaceDrawQuad); - - RemoveFirstBeatmap(); - WaitForSorting(); - - AddAssert("select screen position unchanged", () => Carousel.ChildrenOfType().Single(p => p.Selected.Value).ScreenSpaceDrawQuad, - () => Is.EqualTo(positionBefore)); - } - - [Test] - public void TestScrollPositionMaintainedOnAddLastSelected() - { - Quad positionBefore = default; - - AddBeatmaps(10); - WaitForDrawablePanels(); - - AddStep("scroll to last item", () => Scroll.ScrollToEnd(false)); - - AddStep("select last beatmap", () => Carousel.CurrentSelection = BeatmapSets.Last()); - - WaitForScrolling(); - - AddStep("save selected screen position", () => positionBefore = Carousel.ChildrenOfType().FirstOrDefault(p => p.Selected.Value)!.ScreenSpaceDrawQuad); - - RemoveFirstBeatmap(); - WaitForSorting(); - AddAssert("select screen position unchanged", () => Carousel.ChildrenOfType().Single(p => p.Selected.Value).ScreenSpaceDrawQuad, - () => Is.EqualTo(positionBefore)); + AddStep("disable masking", () => Scroll.Masking = false); + AddStep("enable masking", () => Scroll.Masking = true); } [Test] diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2DifficultyGrouping.cs similarity index 92% rename from osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs rename to osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2DifficultyGrouping.cs index f4d97be5a5..e861d8bf30 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2GroupSelection.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2DifficultyGrouping.cs @@ -12,23 +12,22 @@ using osu.Game.Screens.SelectV2; namespace osu.Game.Tests.Visual.SongSelect { [TestFixture] - public partial class TestSceneBeatmapCarouselV2GroupSelection : BeatmapCarouselV2TestScene + public partial class TestSceneBeatmapCarouselV2DifficultyGrouping : BeatmapCarouselV2TestScene { - public override void SetUpSteps() + [SetUpSteps] + public void SetUpSteps() { RemoveAllBeatmaps(); - CreateCarousel(); - SortBy(new FilterCriteria { Group = GroupMode.Difficulty, Sort = SortMode.Difficulty }); + + AddBeatmaps(10, 3); + WaitForDrawablePanels(); } [Test] public void TestOpenCloseGroupWithNoSelectionMouse() { - AddBeatmaps(10, 5); - WaitForDrawablePanels(); - AddAssert("no beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); CheckNoSelection(); @@ -44,9 +43,6 @@ namespace osu.Game.Tests.Visual.SongSelect [Test] public void TestOpenCloseGroupWithNoSelectionKeyboard() { - AddBeatmaps(10, 5); - WaitForDrawablePanels(); - AddAssert("no beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); CheckNoSelection(); @@ -67,9 +63,6 @@ namespace osu.Game.Tests.Visual.SongSelect [Test] public void TestCarouselRemembersSelection() { - AddBeatmaps(10); - WaitForDrawablePanels(); - SelectNextGroup(); object? selection = null; @@ -107,9 +100,6 @@ namespace osu.Game.Tests.Visual.SongSelect [Test] public void TestGroupSelectionOnHeader() { - AddBeatmaps(10, 3); - WaitForDrawablePanels(); - SelectNextGroup(); WaitForGroupSelection(0, 0); @@ -121,9 +111,6 @@ namespace osu.Game.Tests.Visual.SongSelect [Test] public void TestKeyboardSelection() { - AddBeatmaps(10, 3); - WaitForDrawablePanels(); - SelectNextPanel(); SelectNextPanel(); SelectNextPanel(); diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Selection.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2NoGrouping.cs similarity index 94% rename from osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Selection.cs rename to osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2NoGrouping.cs index b087c252e4..82f35af0ec 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Selection.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2NoGrouping.cs @@ -5,14 +5,24 @@ using System.Linq; using NUnit.Framework; using osu.Framework.Testing; using osu.Game.Beatmaps; +using osu.Game.Screens.Select; +using osu.Game.Screens.Select.Filter; using osu.Game.Screens.SelectV2; using osuTK.Input; namespace osu.Game.Tests.Visual.SongSelect { [TestFixture] - public partial class TestSceneBeatmapCarouselV2Selection : BeatmapCarouselV2TestScene + public partial class TestSceneBeatmapCarouselV2NoGrouping : BeatmapCarouselV2TestScene { + [SetUpSteps] + public void SetUpSteps() + { + RemoveAllBeatmaps(); + CreateCarousel(); + SortBy(new FilterCriteria { Sort = SortMode.Title }); + } + /// /// Keyboard selection via up and down arrows doesn't actually change the selection until /// the select key is pressed. diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Scrolling.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Scrolling.cs new file mode 100644 index 0000000000..1d5d8e2a2d --- /dev/null +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Scrolling.cs @@ -0,0 +1,65 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System.Linq; +using NUnit.Framework; +using osu.Framework.Graphics.Primitives; +using osu.Framework.Testing; +using osu.Game.Screens.Select; +using osu.Game.Screens.SelectV2; + +namespace osu.Game.Tests.Visual.SongSelect +{ + [TestFixture] + public partial class TestSceneBeatmapCarouselV2Scrolling : BeatmapCarouselV2TestScene + { + [SetUpSteps] + public void SetUpSteps() + { + RemoveAllBeatmaps(); + CreateCarousel(); + SortBy(new FilterCriteria()); + + AddBeatmaps(10); + WaitForDrawablePanels(); + } + + [Test] + public void TestScrollPositionMaintainedOnAddSecondSelected() + { + Quad positionBefore = default; + + AddStep("select middle beatmap", () => Carousel.CurrentSelection = BeatmapSets.ElementAt(BeatmapSets.Count - 2)); + AddStep("scroll to selected item", () => Scroll.ScrollTo(Scroll.ChildrenOfType().Single(p => p.Selected.Value))); + + WaitForScrolling(); + + AddStep("save selected screen position", () => positionBefore = Carousel.ChildrenOfType().FirstOrDefault(p => p.Selected.Value)!.ScreenSpaceDrawQuad); + + RemoveFirstBeatmap(); + WaitForSorting(); + + AddAssert("select screen position unchanged", () => Carousel.ChildrenOfType().Single(p => p.Selected.Value).ScreenSpaceDrawQuad, + () => Is.EqualTo(positionBefore)); + } + + [Test] + public void TestScrollPositionMaintainedOnAddLastSelected() + { + Quad positionBefore = default; + + AddStep("scroll to last item", () => Scroll.ScrollToEnd(false)); + + AddStep("select last beatmap", () => Carousel.CurrentSelection = BeatmapSets.Last()); + + WaitForScrolling(); + + AddStep("save selected screen position", () => positionBefore = Carousel.ChildrenOfType().FirstOrDefault(p => p.Selected.Value)!.ScreenSpaceDrawQuad); + + RemoveFirstBeatmap(); + WaitForSorting(); + AddAssert("select screen position unchanged", () => Carousel.ChildrenOfType().Single(p => p.Selected.Value).ScreenSpaceDrawQuad, + () => Is.EqualTo(positionBefore)); + } + } +} From a25e1f4f9b3e9796905419b8aa310a356a3276e1 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 6 Feb 2025 15:13:17 +0900 Subject: [PATCH 06/10] Add test coverage of artist grouping --- ...estSceneBeatmapCarouselV2ArtistGrouping.cs | 170 ++++++++++++++++++ 1 file changed, 170 insertions(+) create mode 100644 osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2ArtistGrouping.cs diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2ArtistGrouping.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2ArtistGrouping.cs new file mode 100644 index 0000000000..c7ab9de5e5 --- /dev/null +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2ArtistGrouping.cs @@ -0,0 +1,170 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System.Linq; +using NUnit.Framework; +using osu.Framework.Testing; +using osu.Game.Beatmaps; +using osu.Game.Screens.Select; +using osu.Game.Screens.Select.Filter; +using osu.Game.Screens.SelectV2; + +namespace osu.Game.Tests.Visual.SongSelect +{ + [TestFixture] + public partial class TestSceneBeatmapCarouselV2ArtistGrouping : BeatmapCarouselV2TestScene + { + [SetUpSteps] + public void SetUpSteps() + { + RemoveAllBeatmaps(); + CreateCarousel(); + SortBy(new FilterCriteria { Group = GroupMode.Artist, Sort = SortMode.Artist }); + + AddBeatmaps(10, 3, true); + WaitForDrawablePanels(); + } + + [Test] + public void TestOpenCloseGroupWithNoSelectionMouse() + { + AddAssert("no beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); + AddUntilStep("no sets visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); + CheckNoSelection(); + + ClickVisiblePanel(0); + + AddUntilStep("some sets visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.GreaterThan(0)); + AddUntilStep("no beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); + CheckNoSelection(); + + ClickVisiblePanel(0); + + AddUntilStep("no sets visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); + AddUntilStep("no beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); + CheckNoSelection(); + } + + [Test] + public void TestOpenCloseGroupWithNoSelectionKeyboard() + { + AddAssert("no beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); + AddUntilStep("no sets visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); + CheckNoSelection(); + + SelectNextPanel(); + Select(); + + AddUntilStep("some sets visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.GreaterThan(0)); + AddUntilStep("no beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); + AddAssert("keyboard selected is expanded", () => getKeyboardSelectedPanel()?.Expanded.Value, () => Is.True); + CheckNoSelection(); + + Select(); + + AddUntilStep("no sets visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); + AddUntilStep("no beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); + AddAssert("keyboard selected is collapsed", () => getKeyboardSelectedPanel()?.Expanded.Value, () => Is.False); + CheckNoSelection(); + + GroupPanel? getKeyboardSelectedPanel() => Carousel.ChildrenOfType().SingleOrDefault(p => p.KeyboardSelected.Value); + } + + [Test] + public void TestCarouselRemembersSelection() + { + SelectNextGroup(); + + object? selection = null; + + AddStep("store drawable selection", () => selection = getSelectedPanel()?.Item?.Model); + + CheckHasSelection(); + AddAssert("drawable selection non-null", () => selection, () => Is.Not.Null); + AddAssert("drawable selection matches carousel selection", () => selection, () => Is.EqualTo(Carousel.CurrentSelection)); + + RemoveAllBeatmaps(); + AddUntilStep("no drawable selection", getSelectedPanel, () => Is.Null); + + AddBeatmaps(10); + WaitForDrawablePanels(); + + CheckHasSelection(); + AddAssert("no drawable selection", getSelectedPanel, () => Is.Null); + + AddStep("add previous selection", () => BeatmapSets.Add(((BeatmapInfo)selection!).BeatmapSet!)); + + AddAssert("selection matches original carousel selection", () => selection, () => Is.EqualTo(Carousel.CurrentSelection)); + AddUntilStep("drawable selection restored", () => getSelectedPanel()?.Item?.Model, () => Is.EqualTo(selection)); + AddAssert("carousel item is visible", () => getSelectedPanel()?.Item?.IsVisible, () => Is.True); + + ClickVisiblePanel(0); + AddUntilStep("carousel item not visible", getSelectedPanel, () => Is.Null); + + ClickVisiblePanel(0); + AddUntilStep("carousel item is visible", () => getSelectedPanel()?.Item?.IsVisible, () => Is.True); + + BeatmapPanel? getSelectedPanel() => Carousel.ChildrenOfType().SingleOrDefault(p => p.Selected.Value); + } + + [Test] + public void TestGroupSelectionOnHeader() + { + SelectNextGroup(); + WaitForGroupSelection(0, 1); + + SelectPrevPanel(); + SelectPrevGroup(); + WaitForGroupSelection(4, 5); + } + + [Test] + public void TestKeyboardSelection() + { + SelectNextPanel(); + SelectNextPanel(); + SelectNextPanel(); + SelectNextPanel(); + CheckNoSelection(); + + // open first group + Select(); + CheckNoSelection(); + AddUntilStep("some beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.GreaterThan(0)); + + SelectNextPanel(); + Select(); + WaitForGroupSelection(3, 1); + + SelectNextGroup(); + WaitForGroupSelection(3, 5); + + SelectNextGroup(); + WaitForGroupSelection(4, 1); + + SelectPrevGroup(); + WaitForGroupSelection(3, 5); + + SelectNextGroup(); + WaitForGroupSelection(4, 1); + + SelectNextGroup(); + WaitForGroupSelection(4, 5); + + SelectNextGroup(); + WaitForGroupSelection(0, 1); + + SelectNextPanel(); + SelectNextPanel(); + SelectNextPanel(); + SelectNextPanel(); + + SelectNextGroup(); + WaitForGroupSelection(0, 1); + + SelectNextPanel(); + SelectNextGroup(); + WaitForGroupSelection(1, 1); + } + } +} From 4026ca84f887979555d32484f32ec8f20f178c7d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 6 Feb 2025 15:41:57 +0900 Subject: [PATCH 07/10] Move selected retrieval functions to base class --- .../SongSelect/BeatmapCarouselV2TestScene.cs | 3 +++ ...estSceneBeatmapCarouselV2ArtistGrouping.cs | 22 +++++++---------- ...ceneBeatmapCarouselV2DifficultyGrouping.cs | 24 ++++++++----------- .../TestSceneBeatmapCarouselV2NoGrouping.cs | 14 ++++------- 4 files changed, 27 insertions(+), 36 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs index 36226a13cc..5ace306c7d 100644 --- a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs +++ b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs @@ -136,6 +136,9 @@ namespace osu.Game.Tests.Visual.SongSelect protected void CheckNoSelection() => AddAssert("has no selection", () => Carousel.CurrentSelection, () => Is.Null); protected void CheckHasSelection() => AddAssert("has selection", () => Carousel.CurrentSelection, () => Is.Not.Null); + protected BeatmapPanel? GetSelectedPanel() => Carousel.ChildrenOfType().SingleOrDefault(p => p.Selected.Value); + protected GroupPanel? GetKeyboardSelectedPanel() => Carousel.ChildrenOfType().SingleOrDefault(p => p.KeyboardSelected.Value); + protected void WaitForGroupSelection(int group, int panel) { AddUntilStep($"selected is group{group} panel{panel}", () => diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2ArtistGrouping.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2ArtistGrouping.cs index c7ab9de5e5..3c518fc7a6 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2ArtistGrouping.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2ArtistGrouping.cs @@ -57,17 +57,15 @@ namespace osu.Game.Tests.Visual.SongSelect AddUntilStep("some sets visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.GreaterThan(0)); AddUntilStep("no beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); - AddAssert("keyboard selected is expanded", () => getKeyboardSelectedPanel()?.Expanded.Value, () => Is.True); + AddAssert("keyboard selected is expanded", () => GetKeyboardSelectedPanel()?.Expanded.Value, () => Is.True); CheckNoSelection(); Select(); AddUntilStep("no sets visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); AddUntilStep("no beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); - AddAssert("keyboard selected is collapsed", () => getKeyboardSelectedPanel()?.Expanded.Value, () => Is.False); + AddAssert("keyboard selected is collapsed", () => GetKeyboardSelectedPanel()?.Expanded.Value, () => Is.False); CheckNoSelection(); - - GroupPanel? getKeyboardSelectedPanel() => Carousel.ChildrenOfType().SingleOrDefault(p => p.KeyboardSelected.Value); } [Test] @@ -77,34 +75,32 @@ namespace osu.Game.Tests.Visual.SongSelect object? selection = null; - AddStep("store drawable selection", () => selection = getSelectedPanel()?.Item?.Model); + AddStep("store drawable selection", () => selection = GetSelectedPanel()?.Item?.Model); CheckHasSelection(); AddAssert("drawable selection non-null", () => selection, () => Is.Not.Null); AddAssert("drawable selection matches carousel selection", () => selection, () => Is.EqualTo(Carousel.CurrentSelection)); RemoveAllBeatmaps(); - AddUntilStep("no drawable selection", getSelectedPanel, () => Is.Null); + AddUntilStep("no drawable selection", GetSelectedPanel, () => Is.Null); AddBeatmaps(10); WaitForDrawablePanels(); CheckHasSelection(); - AddAssert("no drawable selection", getSelectedPanel, () => Is.Null); + AddAssert("no drawable selection", GetSelectedPanel, () => Is.Null); AddStep("add previous selection", () => BeatmapSets.Add(((BeatmapInfo)selection!).BeatmapSet!)); AddAssert("selection matches original carousel selection", () => selection, () => Is.EqualTo(Carousel.CurrentSelection)); - AddUntilStep("drawable selection restored", () => getSelectedPanel()?.Item?.Model, () => Is.EqualTo(selection)); - AddAssert("carousel item is visible", () => getSelectedPanel()?.Item?.IsVisible, () => Is.True); + AddUntilStep("drawable selection restored", () => GetSelectedPanel()?.Item?.Model, () => Is.EqualTo(selection)); + AddAssert("carousel item is visible", () => GetSelectedPanel()?.Item?.IsVisible, () => Is.True); ClickVisiblePanel(0); - AddUntilStep("carousel item not visible", getSelectedPanel, () => Is.Null); + AddUntilStep("carousel item not visible", GetSelectedPanel, () => Is.Null); ClickVisiblePanel(0); - AddUntilStep("carousel item is visible", () => getSelectedPanel()?.Item?.IsVisible, () => Is.True); - - BeatmapPanel? getSelectedPanel() => Carousel.ChildrenOfType().SingleOrDefault(p => p.Selected.Value); + AddUntilStep("carousel item is visible", () => GetSelectedPanel()?.Item?.IsVisible, () => Is.True); } [Test] diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2DifficultyGrouping.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2DifficultyGrouping.cs index e861d8bf30..da3ef75487 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2DifficultyGrouping.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2DifficultyGrouping.cs @@ -49,15 +49,13 @@ namespace osu.Game.Tests.Visual.SongSelect SelectNextPanel(); Select(); AddUntilStep("some beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.GreaterThan(0)); - AddAssert("keyboard selected is expanded", () => getKeyboardSelectedPanel()?.Expanded.Value, () => Is.True); + AddAssert("keyboard selected is expanded", () => GetKeyboardSelectedPanel()?.Expanded.Value, () => Is.True); CheckNoSelection(); Select(); AddUntilStep("no beatmaps visible", () => Carousel.ChildrenOfType().Count(p => p.Alpha > 0), () => Is.Zero); - AddAssert("keyboard selected is collapsed", () => getKeyboardSelectedPanel()?.Expanded.Value, () => Is.False); + AddAssert("keyboard selected is collapsed", () => GetKeyboardSelectedPanel()?.Expanded.Value, () => Is.False); CheckNoSelection(); - - GroupPanel? getKeyboardSelectedPanel() => Carousel.ChildrenOfType().SingleOrDefault(p => p.KeyboardSelected.Value); } [Test] @@ -67,34 +65,32 @@ namespace osu.Game.Tests.Visual.SongSelect object? selection = null; - AddStep("store drawable selection", () => selection = getSelectedPanel()?.Item?.Model); + AddStep("store drawable selection", () => selection = GetSelectedPanel()?.Item?.Model); CheckHasSelection(); AddAssert("drawable selection non-null", () => selection, () => Is.Not.Null); AddAssert("drawable selection matches carousel selection", () => selection, () => Is.EqualTo(Carousel.CurrentSelection)); RemoveAllBeatmaps(); - AddUntilStep("no drawable selection", getSelectedPanel, () => Is.Null); + AddUntilStep("no drawable selection", GetSelectedPanel, () => Is.Null); AddBeatmaps(10); WaitForDrawablePanels(); CheckHasSelection(); - AddAssert("no drawable selection", getSelectedPanel, () => Is.Null); + AddAssert("no drawable selection", GetSelectedPanel, () => Is.Null); AddStep("add previous selection", () => BeatmapSets.Add(((BeatmapInfo)selection!).BeatmapSet!)); AddAssert("selection matches original carousel selection", () => selection, () => Is.EqualTo(Carousel.CurrentSelection)); - AddUntilStep("drawable selection restored", () => getSelectedPanel()?.Item?.Model, () => Is.EqualTo(selection)); - AddAssert("carousel item is visible", () => getSelectedPanel()?.Item?.IsVisible, () => Is.True); + AddUntilStep("drawable selection restored", () => GetSelectedPanel()?.Item?.Model, () => Is.EqualTo(selection)); + AddAssert("carousel item is visible", () => GetSelectedPanel()?.Item?.IsVisible, () => Is.True); ClickVisiblePanel(0); - AddUntilStep("carousel item not visible", getSelectedPanel, () => Is.Null); + AddUntilStep("carousel item not visible", GetSelectedPanel, () => Is.Null); ClickVisiblePanel(0); - AddUntilStep("carousel item is visible", () => getSelectedPanel()?.Item?.IsVisible, () => Is.True); - - BeatmapPanel? getSelectedPanel() => Carousel.ChildrenOfType().SingleOrDefault(p => p.Selected.Value); + AddUntilStep("carousel item is visible", () => GetSelectedPanel()?.Item?.IsVisible, () => Is.True); } [Test] @@ -105,7 +101,7 @@ namespace osu.Game.Tests.Visual.SongSelect SelectPrevPanel(); SelectPrevGroup(); - WaitForGroupSelection(2, 9); + WaitForGroupSelection(0, 0); } [Test] diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2NoGrouping.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2NoGrouping.cs index 82f35af0ec..56bc7790bf 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2NoGrouping.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2NoGrouping.cs @@ -1,13 +1,11 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -using System.Linq; using NUnit.Framework; using osu.Framework.Testing; using osu.Game.Beatmaps; using osu.Game.Screens.Select; using osu.Game.Screens.Select.Filter; -using osu.Game.Screens.SelectV2; using osuTK.Input; namespace osu.Game.Tests.Visual.SongSelect @@ -87,28 +85,26 @@ namespace osu.Game.Tests.Visual.SongSelect object? selection = null; - AddStep("store drawable selection", () => selection = getSelectedPanel()?.Item?.Model); + AddStep("store drawable selection", () => selection = GetSelectedPanel()?.Item?.Model); CheckHasSelection(); AddAssert("drawable selection non-null", () => selection, () => Is.Not.Null); AddAssert("drawable selection matches carousel selection", () => selection, () => Is.EqualTo(Carousel.CurrentSelection)); RemoveAllBeatmaps(); - AddUntilStep("no drawable selection", getSelectedPanel, () => Is.Null); + AddUntilStep("no drawable selection", GetSelectedPanel, () => Is.Null); AddBeatmaps(10); WaitForDrawablePanels(); CheckHasSelection(); - AddAssert("no drawable selection", getSelectedPanel, () => Is.Null); + AddAssert("no drawable selection", GetSelectedPanel, () => Is.Null); AddStep("add previous selection", () => BeatmapSets.Add(((BeatmapInfo)selection!).BeatmapSet!)); AddAssert("selection matches original carousel selection", () => selection, () => Is.EqualTo(Carousel.CurrentSelection)); - AddUntilStep("drawable selection restored", () => getSelectedPanel()?.Item?.Model, () => Is.EqualTo(selection)); - AddAssert("carousel item is visible", () => getSelectedPanel()?.Item?.IsVisible, () => Is.True); - - BeatmapPanel? getSelectedPanel() => Carousel.ChildrenOfType().SingleOrDefault(p => p.Selected.Value); + AddUntilStep("drawable selection restored", () => GetSelectedPanel()?.Item?.Model, () => Is.EqualTo(selection)); + AddAssert("carousel item is visible", () => GetSelectedPanel()?.Item?.IsVisible, () => Is.True); } [Test] From 024fbde0fd723a721eba48279085e0539bec0dde Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 6 Feb 2025 16:21:18 +0900 Subject: [PATCH 08/10] Refactor selection and activation handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I had a bit of a struggle getting header traversal logic to work well. The constraints I had in place were a bit weird: - Group panels should toggle or potentially fall into the prev/next group - Set panels should just traverse around them The current method of using `CheckValidForGroupSelection` return type for traversal did not mesh with the above two cases. Just trust me on this one since it's quite hard to explain in words. After some re-thinking, I've gone with a simpler approach with one important change to UX: Now when group traversing with a beatmap set header currently keyboard focused, the first operation will be to reset keyboard selection to the selected beatmap, rather than traverse. I find this non-offensive – at most it means a user will need to press their group traversal key one extra time. I've also changed group headers to always toggle expansion when doing group traversal with them selected. To make all this work, the meaning of `Activation` has changed somewhat. It is now the primary path for carousel implementations to change selection of an item. It is what the `Drawable` panels call when they are clicked. Selection changes are not performed implicitly by `Carousel` – an implementation should decide when it actually wants to change the selection, usually in `HandleItemActivated`. Having less things mutating `CurrentSelection` is better in my eyes, as we see this variable as only being mutated internally when utmost required (ie the user has requested the change). With this change, `CurrentSelection` can no longer become of a non-`T` type (in the beatmap carousel implementation at least). This might pave a path forward for making `CurrentSelection` typed, but that comes with a few other concerns so I'll look at that as a follow-up. --- ...estSceneBeatmapCarouselV2ArtistGrouping.cs | 13 ++- ...ceneBeatmapCarouselV2DifficultyGrouping.cs | 9 ++ .../TestSceneBeatmapCarouselV2NoGrouping.cs | 6 +- .../TestSceneBeatmapCarouselV2Scrolling.cs | 4 +- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 33 ++++--- osu.Game/Screens/SelectV2/BeatmapPanel.cs | 8 +- osu.Game/Screens/SelectV2/BeatmapSetPanel.cs | 5 +- osu.Game/Screens/SelectV2/Carousel.cs | 98 +++++++++---------- osu.Game/Screens/SelectV2/GroupPanel.cs | 5 +- 9 files changed, 98 insertions(+), 83 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2ArtistGrouping.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2ArtistGrouping.cs index 3c518fc7a6..d3eeee151a 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2ArtistGrouping.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2ArtistGrouping.cs @@ -110,8 +110,19 @@ namespace osu.Game.Tests.Visual.SongSelect WaitForGroupSelection(0, 1); SelectPrevPanel(); + SelectPrevPanel(); + + AddAssert("keyboard selected panel is expanded", () => GetKeyboardSelectedPanel()?.Expanded.Value, () => Is.True); + SelectPrevGroup(); - WaitForGroupSelection(4, 5); + + WaitForGroupSelection(0, 1); + AddAssert("keyboard selected panel is contracted", () => GetKeyboardSelectedPanel()?.Expanded.Value, () => Is.False); + + SelectPrevGroup(); + + WaitForGroupSelection(0, 1); + AddAssert("keyboard selected panel is expanded", () => GetKeyboardSelectedPanel()?.Expanded.Value, () => Is.True); } [Test] diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2DifficultyGrouping.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2DifficultyGrouping.cs index da3ef75487..151f1f5fec 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2DifficultyGrouping.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2DifficultyGrouping.cs @@ -100,8 +100,17 @@ namespace osu.Game.Tests.Visual.SongSelect WaitForGroupSelection(0, 0); SelectPrevPanel(); + AddAssert("keyboard selected panel is expanded", () => GetKeyboardSelectedPanel()?.Expanded.Value, () => Is.True); + SelectPrevGroup(); + WaitForGroupSelection(0, 0); + AddAssert("keyboard selected panel is contracted", () => GetKeyboardSelectedPanel()?.Expanded.Value, () => Is.False); + + SelectPrevGroup(); + + WaitForGroupSelection(0, 0); + AddAssert("keyboard selected panel is expanded", () => GetKeyboardSelectedPanel()?.Expanded.Value, () => Is.True); } [Test] diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2NoGrouping.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2NoGrouping.cs index 56bc7790bf..34bdd1265d 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2NoGrouping.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2NoGrouping.cs @@ -147,7 +147,11 @@ namespace osu.Game.Tests.Visual.SongSelect SelectPrevPanel(); SelectPrevGroup(); - WaitForSelection(0, 0); + WaitForSelection(1, 0); + + SelectPrevPanel(); + SelectNextGroup(); + WaitForSelection(1, 0); } [Test] diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Scrolling.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Scrolling.cs index 1d5d8e2a2d..ee6c11595a 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Scrolling.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2Scrolling.cs @@ -29,7 +29,7 @@ namespace osu.Game.Tests.Visual.SongSelect { Quad positionBefore = default; - AddStep("select middle beatmap", () => Carousel.CurrentSelection = BeatmapSets.ElementAt(BeatmapSets.Count - 2)); + AddStep("select middle beatmap", () => Carousel.CurrentSelection = BeatmapSets.ElementAt(BeatmapSets.Count - 2).Beatmaps.First()); AddStep("scroll to selected item", () => Scroll.ScrollTo(Scroll.ChildrenOfType().Single(p => p.Selected.Value))); WaitForScrolling(); @@ -50,7 +50,7 @@ namespace osu.Game.Tests.Visual.SongSelect AddStep("scroll to last item", () => Scroll.ScrollToEnd(false)); - AddStep("select last beatmap", () => Carousel.CurrentSelection = BeatmapSets.Last()); + AddStep("select last beatmap", () => Carousel.CurrentSelection = BeatmapSets.Last().Beatmaps.Last()); WaitForScrolling(); diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index 36e57c9067..6032989ad0 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -95,11 +95,9 @@ namespace osu.Game.Screens.SelectV2 private GroupDefinition? lastSelectedGroup; private BeatmapInfo? lastSelectedBeatmap; - protected override bool HandleItemSelected(object? model) + protected override void HandleItemActivated(CarouselItem item) { - base.HandleItemSelected(model); - - switch (model) + switch (item.Model) { case GroupDefinition group: // Special case – collapsing an open group. @@ -107,16 +105,32 @@ namespace osu.Game.Screens.SelectV2 { setExpansionStateOfGroup(lastSelectedGroup, false); lastSelectedGroup = null; - return false; + return; } setExpandedGroup(group); - return false; + return; case BeatmapSetInfo setInfo: // Selecting a set isn't valid – let's re-select the first difficulty. CurrentSelection = setInfo.Beatmaps.First(); - return false; + return; + + case BeatmapInfo beatmapInfo: + CurrentSelection = beatmapInfo; + return; + } + } + + protected override void HandleItemSelected(object? model) + { + base.HandleItemSelected(model); + + switch (model) + { + case BeatmapSetInfo: + case GroupDefinition: + throw new InvalidOperationException("Groups should never become selected"); case BeatmapInfo beatmapInfo: // Find any containing group. There should never be too many groups so iterating is efficient enough. @@ -125,11 +139,8 @@ namespace osu.Game.Screens.SelectV2 if (containingGroup != null) setExpandedGroup(containingGroup); setExpandedSet(beatmapInfo); - - return true; + break; } - - return true; } protected override bool CheckValidForGroupSelection(CarouselItem item) diff --git a/osu.Game/Screens/SelectV2/BeatmapPanel.cs b/osu.Game/Screens/SelectV2/BeatmapPanel.cs index 3edfd4203b..9280e1c2c1 100644 --- a/osu.Game/Screens/SelectV2/BeatmapPanel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapPanel.cs @@ -86,13 +86,7 @@ namespace osu.Game.Screens.SelectV2 protected override bool OnClick(ClickEvent e) { - if (carousel.CurrentSelection != Item!.Model) - { - carousel.CurrentSelection = Item!.Model; - return true; - } - - carousel.TryActivateSelection(); + carousel.Activate(Item!); return true; } diff --git a/osu.Game/Screens/SelectV2/BeatmapSetPanel.cs b/osu.Game/Screens/SelectV2/BeatmapSetPanel.cs index 79ffe0f68a..f6c9324077 100644 --- a/osu.Game/Screens/SelectV2/BeatmapSetPanel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapSetPanel.cs @@ -1,7 +1,6 @@ // 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.Diagnostics; using osu.Framework.Allocation; using osu.Framework.Bindables; @@ -83,7 +82,7 @@ namespace osu.Game.Screens.SelectV2 protected override bool OnClick(ClickEvent e) { - carousel.CurrentSelection = Item!.Model; + carousel.Activate(Item!); return true; } @@ -98,8 +97,6 @@ namespace osu.Game.Screens.SelectV2 public void Activated() { - // sets should never be activated. - throw new InvalidOperationException(); } #endregion diff --git a/osu.Game/Screens/SelectV2/Carousel.cs b/osu.Game/Screens/SelectV2/Carousel.cs index 6b7b1f3a9b..603a792847 100644 --- a/osu.Game/Screens/SelectV2/Carousel.cs +++ b/osu.Game/Screens/SelectV2/Carousel.cs @@ -94,26 +94,39 @@ namespace osu.Game.Screens.SelectV2 public object? CurrentSelection { get => currentSelection.Model; - set => setSelection(value); + set + { + if (currentSelection.Model != value) + { + HandleItemSelected(value); + + if (currentSelection.Model != null) + HandleItemDeselected(currentSelection.Model); + + currentKeyboardSelection = new Selection(value); + currentSelection = currentKeyboardSelection; + selectionValid.Invalidate(); + } + else if (currentKeyboardSelection.Model != value) + { + // Even if the current selection matches, let's ensure the keyboard selection is reset + // to the newly selected object. This matches user expectations (for now). + currentKeyboardSelection = currentSelection; + selectionValid.Invalidate(); + } + } } /// - /// Activate the current selection, if a selection exists and matches keyboard selection. - /// If keyboard selection does not match selection, this will transfer the selection on first invocation. + /// Activate the specified item. /// - public void TryActivateSelection() + /// + public void Activate(CarouselItem item) { - if (currentSelection.CarouselItem != currentKeyboardSelection.CarouselItem) - { - CurrentSelection = currentKeyboardSelection.Model; - return; - } + (GetMaterialisedDrawableForItem(item) as ICarouselPanel)?.Activated(); + HandleItemActivated(item); - if (currentSelection.CarouselItem != null) - { - (GetMaterialisedDrawableForItem(currentSelection.CarouselItem) as ICarouselPanel)?.Activated(); - HandleItemActivated(currentSelection.CarouselItem); - } + selectionValid.Invalidate(); } #endregion @@ -176,30 +189,28 @@ namespace osu.Game.Screens.SelectV2 protected virtual bool CheckValidForGroupSelection(CarouselItem item) => true; /// - /// Called when an item is "selected". + /// Called after an item becomes the . + /// Should be used to handle any group expansion, item visibility changes, etc. /// - /// Whether the item should be selected. - protected virtual bool HandleItemSelected(object? model) => true; + protected virtual void HandleItemSelected(object? model) { } /// - /// Called when an item is "deselected". + /// Called when the changes to a new selection. + /// Should be used to handle any group expansion, item visibility changes, etc. /// - protected virtual void HandleItemDeselected(object? model) - { - } + protected virtual void HandleItemDeselected(object? model) { } /// - /// Called when an item is "activated". + /// Called when an item is activated via user input (keyboard traversal or a mouse click). /// /// - /// An activated item should for instance: - /// - Open or close a folder - /// - Start gameplay on a beatmap difficulty. + /// An activated item should decide to perform an action, such as: + /// - Change its expanded state (and show / hide children items). + /// - Set the item to the . + /// - Start gameplay on a beatmap difficulty if already selected. /// /// The carousel item which was activated. - protected virtual void HandleItemActivated(CarouselItem item) - { - } + protected virtual void HandleItemActivated(CarouselItem item) { } #endregion @@ -305,7 +316,8 @@ namespace osu.Game.Screens.SelectV2 switch (e.Action) { case GlobalAction.Select: - TryActivateSelection(); + if (currentKeyboardSelection.CarouselItem != null) + Activate(currentKeyboardSelection.CarouselItem); return true; case GlobalAction.SelectNext: @@ -374,13 +386,10 @@ namespace osu.Game.Screens.SelectV2 // If the user has a different keyboard selection and requests // group selection, first transfer the keyboard selection to actual selection. - if (currentSelection.CarouselItem != currentKeyboardSelection.CarouselItem) + if (currentKeyboardSelection.CarouselItem != null && currentSelection.CarouselItem != currentKeyboardSelection.CarouselItem) { - TryActivateSelection(); - - // Is the selection actually changed, then we should not perform any further traversal. - if (currentSelection.CarouselItem == currentKeyboardSelection.CarouselItem) - return; + Activate(currentKeyboardSelection.CarouselItem); + return; } int originalIndex; @@ -413,7 +422,7 @@ namespace osu.Game.Screens.SelectV2 if (CheckValidForGroupSelection(newItem)) { - setSelection(newItem.Model); + HandleItemActivated(newItem); return; } } while (newIndex != originalIndex); @@ -428,23 +437,6 @@ namespace osu.Game.Screens.SelectV2 private Selection currentKeyboardSelection = new Selection(); private Selection currentSelection = new Selection(); - private void setSelection(object? model) - { - if (currentSelection.Model == model) - return; - - if (HandleItemSelected(model)) - { - if (currentSelection.Model != null) - HandleItemDeselected(currentSelection.Model); - - currentKeyboardSelection = new Selection(model); - currentSelection = currentKeyboardSelection; - } - - selectionValid.Invalidate(); - } - private void setKeyboardSelection(object? model) { currentKeyboardSelection = new Selection(model); diff --git a/osu.Game/Screens/SelectV2/GroupPanel.cs b/osu.Game/Screens/SelectV2/GroupPanel.cs index 7ed256ca6a..e10521f63e 100644 --- a/osu.Game/Screens/SelectV2/GroupPanel.cs +++ b/osu.Game/Screens/SelectV2/GroupPanel.cs @@ -1,7 +1,6 @@ // 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.Diagnostics; using osu.Framework.Allocation; using osu.Framework.Bindables; @@ -96,7 +95,7 @@ namespace osu.Game.Screens.SelectV2 protected override bool OnClick(ClickEvent e) { - carousel.CurrentSelection = Item!.Model; + carousel.Activate(Item!); return true; } @@ -111,8 +110,6 @@ namespace osu.Game.Screens.SelectV2 public void Activated() { - // sets should never be activated. - throw new InvalidOperationException(); } #endregion From bff686f01289f68ec8b12de2bb62107ddb49d76a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 6 Feb 2025 17:09:58 +0900 Subject: [PATCH 09/10] Avoid double iteration when updating group states --- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 47 +++++++++++++++----- 1 file changed, 35 insertions(+), 12 deletions(-) diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index 6032989ad0..4126889892 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -173,22 +173,45 @@ namespace osu.Game.Screens.SelectV2 { if (grouping.GroupItems.TryGetValue(group, out var items)) { - // First pass ignoring set groupings. - foreach (var i in items) - { - if (i.Model is GroupDefinition) - i.IsExpanded = expanded; - else - i.IsVisible = expanded; - } - - // Second pass to hide set children when not meant to be displayed. if (expanded) { foreach (var i in items) { - if (i.Model is BeatmapSetInfo set) - setExpansionStateOfSetItems(set, i.IsExpanded); + switch (i.Model) + { + case GroupDefinition: + i.IsExpanded = true; + break; + + case BeatmapSetInfo set: + // Case where there are set headers, header should be visible + // and items should use the set's expanded state. + i.IsVisible = true; + setExpansionStateOfSetItems(set, i.IsExpanded); + break; + + default: + // Case where there are no set headers, all items should be visible. + if (!grouping.BeatmapSetsGroupedTogether) + i.IsVisible = true; + break; + } + } + } + else + { + foreach (var i in items) + { + switch (i.Model) + { + case GroupDefinition: + i.IsExpanded = false; + break; + + default: + i.IsVisible = false; + break; + } } } } From cb42ef95c57cf6f86c66dd882962b1532401d823 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 6 Feb 2025 17:48:42 +0900 Subject: [PATCH 10/10] Add invalidation on draw size change in beatmap carousel v2 Matching old implementation. --- osu.Game/Screens/SelectV2/Carousel.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/osu.Game/Screens/SelectV2/Carousel.cs b/osu.Game/Screens/SelectV2/Carousel.cs index 1fd2f0a9b0..4248641a43 100644 --- a/osu.Game/Screens/SelectV2/Carousel.cs +++ b/osu.Game/Screens/SelectV2/Carousel.cs @@ -16,6 +16,7 @@ using osu.Framework.Graphics.Cursor; using osu.Framework.Graphics.Pooling; using osu.Framework.Input.Bindings; using osu.Framework.Input.Events; +using osu.Framework.Layout; using osu.Framework.Logging; using osu.Framework.Utils; using osu.Game.Graphics.Containers; @@ -678,6 +679,15 @@ namespace osu.Game.Screens.SelectV2 carouselPanel.Expanded.Value = false; } + protected override bool OnInvalidate(Invalidation invalidation, InvalidationSource source) + { + // handles the vertical size of the carousel changing (ie. on window resize when aspect ratio has changed). + if (invalidation.HasFlag(Invalidation.DrawSize)) + selectionValid.Invalidate(); + + return base.OnInvalidate(invalidation, source); + } + #endregion #region Internal helper classes