diff --git a/osu.Game.Tests/Visual/TestCaseBeatmapCarousel.cs b/osu.Game.Tests/Visual/TestCaseBeatmapCarousel.cs index f7f2108f33..6163daaf39 100644 --- a/osu.Game.Tests/Visual/TestCaseBeatmapCarousel.cs +++ b/osu.Game.Tests/Visual/TestCaseBeatmapCarousel.cs @@ -13,6 +13,7 @@ using osu.Game.Beatmaps; using osu.Game.Configuration; using osu.Game.Screens.Select; using osu.Game.Screens.Select.Carousel; +using osu.Game.Screens.Select.Filter; namespace osu.Game.Tests.Visual { @@ -35,6 +36,9 @@ namespace osu.Game.Tests.Visual typeof(DrawableCarouselBeatmapSet), }; + + private readonly Stack selectedSets = new Stack(); + [BackgroundDependencyLoader] private void load() { @@ -54,34 +58,68 @@ namespace osu.Game.Tests.Visual }; }); - void checkSelected(int set, int diff) => - AddAssert($"selected is set{set} diff{diff}", () => - carousel.SelectedBeatmap == carousel.BeatmapSets.Skip(set - 1).First().Beatmaps.Skip(diff - 1).First()); - - void setSelected(int set, int diff) => - AddStep($"select set{set} diff{diff}", () => - carousel.SelectBeatmap(carousel.BeatmapSets.Skip(set - 1).First().Beatmaps.Skip(diff - 1).First())); - - void advanceSelection(bool diff, int direction = 1, int count = 1) - { - if (count == 1) - AddStep($"select {(direction > 0 ? "next" : "prev")} {(diff ? "diff" : "set")}", () => - carousel.SelectNext(direction, !diff)); - else - { - AddRepeatStep($"select {(direction > 0 ? "next" : "prev")} {(diff ? "diff" : "set")}", () => - carousel.SelectNext(direction, !diff), count); - } - } - - void checkVisibleItemCount(bool diff, int count) => - AddAssert($"{count} {(diff ? "diff" : "set")} visible", () => - carousel.Items.Count(s => (diff ? s.Item is CarouselBeatmap : s.Item is CarouselBeatmapSet) && s.Item.Visible) == count); - AddUntilStep(() => carousel.BeatmapSets.Any(), "Wait for load"); - // test traversal + testTraversal(); + testFiltering(); + testRandom(); + testAddRemove(); + testSorting(); + } + private void ensureRandomFetchSuccess() => + AddAssert("ensure prev random fetch worked", () => selectedSets.Peek() == carousel.SelectedBeatmapSet); + + private void checkSelected(int set, int diff) => + AddAssert($"selected is set{set} diff{diff}", () => + carousel.SelectedBeatmap == carousel.BeatmapSets.Skip(set - 1).First().Beatmaps.Skip(diff - 1).First()); + + private void setSelected(int set, int diff) => + AddStep($"select set{set} diff{diff}", () => + carousel.SelectBeatmap(carousel.BeatmapSets.Skip(set - 1).First().Beatmaps.Skip(diff - 1).First())); + + private void advanceSelection(bool diff, int direction = 1, int count = 1) + { + if (count == 1) + AddStep($"select {(direction > 0 ? "next" : "prev")} {(diff ? "diff" : "set")}", () => + carousel.SelectNext(direction, !diff)); + else + { + AddRepeatStep($"select {(direction > 0 ? "next" : "prev")} {(diff ? "diff" : "set")}", () => + carousel.SelectNext(direction, !diff), count); + } + } + + private void checkVisibleItemCount(bool diff, int count) => + AddAssert($"{count} {(diff ? "diff" : "set")} visible", () => + carousel.Items.Count(s => (diff ? s.Item is CarouselBeatmap : s.Item is CarouselBeatmapSet) && s.Item.Visible) == count); + + private void nextRandom() => + AddStep("select random next", () => + { + carousel.RandomAlgorithm.Value = RandomSelectAlgorithm.RandomPermutation; + + if (!selectedSets.Any() && carousel.SelectedBeatmap != null) + selectedSets.Push(carousel.SelectedBeatmapSet); + + carousel.SelectNextRandom(); + selectedSets.Push(carousel.SelectedBeatmapSet); + }); + + private void ensureRandomDidntRepeat() => + AddAssert("ensure no repeats", () => selectedSets.Distinct().Count() == selectedSets.Count); + + private void prevRandom() => AddStep("select random last", () => + { + carousel.SelectPreviousRandom(); + selectedSets.Pop(); + }); + + /// + /// Test keyboard traversal + /// + private void testTraversal() + { advanceSelection(direction: 1, diff: false); checkSelected(1, 1); @@ -100,8 +138,14 @@ namespace osu.Game.Tests.Visual advanceSelection(direction: -1, diff: true); checkSelected(4, 3); + } - // test basic filtering + /// + /// Test filtering + /// + private void testFiltering() + { + // basic filtering AddStep("Filter", () => carousel.Filter(new FilterCriteria { SearchText = "set #3" }, false)); checkVisibleItemCount(diff: false, count: 1); @@ -123,35 +167,13 @@ namespace osu.Game.Tests.Visual checkSelected(1, 1); AddStep("Un-filter", () => carousel.Filter(new FilterCriteria(), false)); checkSelected(1, 1); + } - // test random non-repeating algorithm - - Stack selectedSets = new Stack(); - - void nextRandom() => - AddStep("select random next", () => - { - carousel.RandomAlgorithm.Value = RandomSelectAlgorithm.RandomPermutation; - - if (!selectedSets.Any() && carousel.SelectedBeatmap != null) - selectedSets.Push(carousel.SelectedBeatmapSet); - - carousel.SelectNextRandom(); - selectedSets.Push(carousel.SelectedBeatmapSet); - }); - - void ensureRandomDidntRepeat() => - AddAssert("ensure no repeats", () => selectedSets.Distinct().Count() == selectedSets.Count); - - void prevRandom() => AddStep("select random last", () => - { - carousel.SelectPreviousRandom(); - selectedSets.Pop(); - }); - - void ensureRandomFetchSuccess() => - AddAssert("ensure prev random fetch worked", () => selectedSets.Peek() == carousel.SelectedBeatmapSet); - + /// + /// Test random non-repeating algorithm + /// + private void testRandom() + { setSelected(1, 1); nextRandom(); @@ -173,21 +195,37 @@ namespace osu.Game.Tests.Visual nextRandom(); AddAssert("ensure repeat", () => selectedSets.Contains(carousel.SelectedBeatmapSet)); + } - // test adding and removing - + /// + /// Test adding and removing beatmap sets + /// + private void testAddRemove() + { AddStep("Add new set #5", () => carousel.UpdateBeatmapSet(createTestBeatmapSet(5))); AddStep("Add new set #6", () => carousel.UpdateBeatmapSet(createTestBeatmapSet(6))); checkVisibleItemCount(false, 6); - AddStep("Remove set #4", () => carousel.RemoveBeatmapSet(createTestBeatmapSet(4))); + AddStep("Remove set #5", () => carousel.RemoveBeatmapSet(createTestBeatmapSet(5))); checkVisibleItemCount(false, 5); - + AddStep("Remove set #6", () => carousel.RemoveBeatmapSet(createTestBeatmapSet(6))); } + /// + /// Test sorting + /// + private void testSorting() + { + AddStep("Sort by author", () => carousel.Filter(new FilterCriteria { Sort = SortMode.Author }, false)); + AddAssert("Check yyyyy is at bottom", () => carousel.BeatmapSets.Last().Metadata.AuthorString == "yyyyy"); + AddStep("Sort by title", () => carousel.Filter(new FilterCriteria { Sort = SortMode.Title }, false)); + AddAssert("Check #4 is at bottom", () => carousel.BeatmapSets.Last().Metadata.Title.EndsWith("#4")); + } + + private BeatmapSetInfo createTestBeatmapSet(int i) { return new BeatmapSetInfo @@ -201,7 +239,7 @@ namespace osu.Game.Tests.Visual // Create random metadata, then we can check if sorting works based on these Artist = "peppy", Title = "test set #" + i, - AuthorString = "peppy", + AuthorString = string.Concat(Enumerable.Repeat((char)('z' - i), 5)) }, Beatmaps = new List(new[] { diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index bdfb772704..e51ce85546 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -17,6 +17,7 @@ using osu.Framework.Allocation; using osu.Framework.Caching; using osu.Framework.Threading; using osu.Framework.Configuration; +using osu.Framework.Extensions.IEnumerableExtensions; using osu.Game.Beatmaps; using osu.Game.Graphics.Containers; using osu.Game.Graphics.Cursor; @@ -52,32 +53,26 @@ namespace osu.Game.Screens.Select public override bool HandleInput => AllowSelection; - private readonly List beatmapSets = new List(); + private IEnumerable beatmapSets => root.Children?.OfType() ?? new CarouselBeatmapSet[] { }; public IEnumerable BeatmapSets { get { return beatmapSets.Select(g => g.BeatmapSet); } set { - Schedule(() => - { - scrollableContent.Clear(false); - Items.Clear(); - beatmapSets.Clear(); - yPositionsCache.Invalidate(); - }); - List newSets = null; + CarouselGroup newRoot = new CarouselGroup(); + Task.Run(() => { - newSets = value.Select(createCarouselSet).Where(g => g != null).ToList(); - newSets.ForEach(g => g.Filter(criteria)); + value.Select(createCarouselSet).Where(g => g != null).ForEach(newRoot.AddChild); + newRoot.Filter(criteria); }).ContinueWith(t => { Schedule(() => { - beatmapSets.AddRange(newSets); + root = newRoot; updateItems(); }); }); @@ -91,8 +86,7 @@ namespace osu.Game.Screens.Select { scrollableContent.Clear(false); - root = new CarouselGroup(beatmapSets.OfType().ToList()); - Items = root.Drawables.Value.ToList(); + Items = root.Drawables.ToList(); yPositionsCache.Invalidate(); BeatmapSetsChanged?.Invoke(); @@ -125,13 +119,12 @@ namespace osu.Game.Screens.Select public void RemoveBeatmapSet(BeatmapSetInfo beatmapSet) { - var existingSet = beatmapSets.Find(b => b.BeatmapSet.ID == beatmapSet.ID); + var existingSet = beatmapSets.FirstOrDefault(b => b.BeatmapSet.ID == beatmapSet.ID); if (existingSet == null) return; - beatmapSets.Remove(existingSet); - + root.RemoveChild(existingSet); updateItems(); if (existingSet.State == CarouselItemState.Selected) @@ -140,39 +133,29 @@ namespace osu.Game.Screens.Select public void UpdateBeatmapSet(BeatmapSetInfo beatmapSet) { - CarouselBeatmapSet existingSet = beatmapSets.Find(b => b.BeatmapSet.ID == beatmapSet.ID); + CarouselBeatmapSet existingSet = beatmapSets.FirstOrDefault(b => b.BeatmapSet.ID == beatmapSet.ID); bool hadSelection = existingSet?.State?.Value == CarouselItemState.Selected; var newSet = createCarouselSet(beatmapSet); - int index = beatmapSets.IndexOf(existingSet); - if (index >= 0) - beatmapSets.RemoveAt(index); + if (existingSet != null) + root.RemoveChild(existingSet); - if (newSet != null) + if (newSet == null) { - if (index >= 0) - beatmapSets.Insert(index, newSet); - else - beatmapSets.Add(newSet); + updateItems(); + SelectNext(); + return; } - if (hadSelection && newSet == null) - SelectNext(); + root.AddChild(newSet); - Filter(null, false); + Filter(debounce: false); //check if we can/need to maintain our current selection. - if (hadSelection && newSet != null) - { - var newSelection = newSet.Beatmaps.Find(b => b.Beatmap.ID == selectedBeatmap?.Beatmap.ID); - - if (newSelection == null && selectedBeatmap != null) - newSelection = newSet.Beatmaps[Math.Min(newSet.Beatmaps.Count - 1, existingSet.Beatmaps.IndexOf(selectedBeatmap))]; - - select(newSelection); - } + if (hadSelection) + select((CarouselItem)newSet.Beatmaps.FirstOrDefault(b => b.Beatmap.ID == selectedBeatmap?.Beatmap.ID) ?? newSet); updateItems(); } @@ -212,7 +195,7 @@ namespace osu.Game.Screens.Select return; } - int originalIndex = Items.IndexOf(selectedBeatmap?.Drawables.Value.First()); + int originalIndex = Items.IndexOf(selectedBeatmap?.Drawables.First()); int currentIndex = originalIndex; // local function to increment the index in the required direction, wrapping over extremities. @@ -241,7 +224,7 @@ namespace osu.Game.Screens.Select public void SelectNextRandom() { - if (beatmapSets.Count == 0) + if (!beatmapSets.Any()) return; var visible = getVisibleSets().ToList(); @@ -275,7 +258,7 @@ namespace osu.Game.Screens.Select else set = visible.ElementAt(RNG.Next(visible.Count)); - select(set.Beatmaps[RNG.Next(set.Beatmaps.Count)]); + select(set.Beatmaps.Skip(RNG.Next(set.Beatmaps.Count())).FirstOrDefault()); } public void SelectPreviousRandom() @@ -303,7 +286,7 @@ namespace osu.Game.Screens.Select public void FlushPendingFilters() { if (FilterTask?.Completed == false) - Filter(null, false); + Filter(debounce: false); } public void Filter(FilterCriteria newCriteria = null, bool debounce = true) @@ -318,9 +301,8 @@ namespace osu.Game.Screens.Select var lastSet = selectedBeatmapSet; var lastBeatmap = selectedBeatmap; - beatmapSets.ForEach(s => s.Filter(criteria)); - - yPositionsCache.Invalidate(); + root.Filter(criteria); + updateItems(); if (selectedBeatmap?.Filtered == false) select(selectedBeatmap); @@ -337,6 +319,8 @@ namespace osu.Game.Screens.Select } else SelectNext(); + + ScrollToSelected(false); }; FilterTask?.Cancel(); diff --git a/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs b/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs index 655579269f..a0d91cf23b 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs @@ -4,6 +4,7 @@ using System; using System.Linq; using osu.Game.Beatmaps; +using osu.Game.Screens.Select.Filter; namespace osu.Game.Screens.Select.Carousel { @@ -32,5 +33,21 @@ namespace osu.Game.Screens.Select.Carousel Filtered.Value = !match; } + + public override int CompareTo(FilterCriteria criteria, CarouselItem other) + { + if (!(other is CarouselBeatmap otherBeatmap)) + return base.CompareTo(criteria, other); + + switch (criteria.Sort) + { + default: + case SortMode.Difficulty: + var ruleset = Beatmap.RulesetID.CompareTo(otherBeatmap.Beatmap.RulesetID); + if (ruleset != 0) return ruleset; + + return Beatmap.StarDifficulty.CompareTo(otherBeatmap.Beatmap.StarDifficulty); + } + } } } diff --git a/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs b/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs index d0ccda0e28..655ed2057d 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs @@ -4,52 +4,53 @@ using System; using System.Collections.Generic; using System.Linq; +using osu.Framework.Extensions.IEnumerableExtensions; using osu.Game.Beatmaps; +using osu.Game.Screens.Select.Filter; namespace osu.Game.Screens.Select.Carousel { public class CarouselBeatmapSet : CarouselGroupEagerSelect { - public readonly List Beatmaps; + public IEnumerable Beatmaps => InternalChildren.OfType(); public BeatmapSetInfo BeatmapSet; public CarouselBeatmapSet(BeatmapSetInfo beatmapSet) { - if (beatmapSet == null) throw new ArgumentNullException(nameof(beatmapSet)); + BeatmapSet = beatmapSet ?? throw new ArgumentNullException(nameof(beatmapSet)); - BeatmapSet = beatmapSet; - - Children = Beatmaps = beatmapSet.Beatmaps - .Where(b => !b.Hidden) - .OrderBy(b => b.RulesetID).ThenBy(b => b.StarDifficulty) - .Select(b => new CarouselBeatmap(b)) - .ToList(); + beatmapSet.Beatmaps + .Where(b => !b.Hidden) + .Select(b => new CarouselBeatmap(b)) + .ForEach(AddChild); } protected override DrawableCarouselItem CreateDrawableRepresentation() => new DrawableCarouselBeatmapSet(this); - public override void Filter(FilterCriteria criteria) + public override int CompareTo(FilterCriteria criteria, CarouselItem other) { - base.Filter(criteria); - Filtered.Value = Children.All(i => i.Filtered); + if (!(other is CarouselBeatmapSet otherSet)) + return base.CompareTo(criteria, other); - /*switch (criteria.Sort) + switch (criteria.Sort) { default: case SortMode.Artist: - groups.Sort((x, y) => string.Compare(x.BeatmapSet.Metadata.Artist, y.BeatmapSet.Metadata.Artist, StringComparison.InvariantCultureIgnoreCase)); - break; + return string.Compare(BeatmapSet.Metadata.Artist, otherSet.BeatmapSet.Metadata.Artist, StringComparison.InvariantCultureIgnoreCase); case SortMode.Title: - groups.Sort((x, y) => string.Compare(x.BeatmapSet.Metadata.Title, y.BeatmapSet.Metadata.Title, StringComparison.InvariantCultureIgnoreCase)); - break; + return string.Compare(BeatmapSet.Metadata.Title, otherSet.BeatmapSet.Metadata.Title, StringComparison.InvariantCultureIgnoreCase); case SortMode.Author: - groups.Sort((x, y) => string.Compare(x.BeatmapSet.Metadata.Author.Username, y.BeatmapSet.Metadata.Author.Username, StringComparison.InvariantCultureIgnoreCase)); - break; + return string.Compare(BeatmapSet.Metadata.Author.Username, otherSet.BeatmapSet.Metadata.Author.Username, StringComparison.InvariantCultureIgnoreCase); case SortMode.Difficulty: - groups.Sort((x, y) => x.BeatmapSet.MaxStarDifficulty.CompareTo(y.BeatmapSet.MaxStarDifficulty)); - break; - }*/ + return BeatmapSet.MaxStarDifficulty.CompareTo(otherSet.BeatmapSet.MaxStarDifficulty); + } + } + + public override void Filter(FilterCriteria criteria) + { + base.Filter(criteria); + Filtered.Value = InternalChildren.All(i => i.Filtered); } } } diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroup.cs b/osu.Game/Screens/Select/Carousel/CarouselGroup.cs index 597fed7a34..695a8e1bc4 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroup.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroup.cs @@ -3,7 +3,6 @@ using System.Collections.Generic; using osu.Framework.Configuration; -using osu.Framework.Extensions.IEnumerableExtensions; namespace osu.Game.Screens.Select.Carousel { @@ -18,19 +17,15 @@ namespace osu.Game.Screens.Select.Carousel protected override DrawableCarouselItem CreateDrawableRepresentation() => null; - protected override IEnumerable Children + public override void AddChild(CarouselItem i) { - get { return base.Children; } - set - { - base.Children = value; - value.ForEach(i => i.State.ValueChanged += v => itemStateChanged(i, v)); - } + i.State.ValueChanged += v => itemStateChanged(i, v); + base.AddChild(i); } public CarouselGroup(List items = null) { - if (items != null) Children = items; + if (items != null) InternalChildren = items; } private void itemStateChanged(CarouselItem item, CarouselItemState value) @@ -40,7 +35,7 @@ namespace osu.Game.Screens.Select.Carousel // ensure we are the only item selected if (value == CarouselItemState.Selected) { - foreach (var b in Children) + foreach (var b in InternalChildren) { if (item == b) continue; b.State.Value = CarouselItemState.NotSelected; diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs index 351f0ed55b..b9312882a1 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs @@ -16,11 +16,11 @@ namespace osu.Game.Screens.Select.Carousel { if (v == CarouselItemState.Selected) { - foreach (var c in Children.Where(c => c.State.Value == CarouselItemState.Hidden)) + foreach (var c in InternalChildren.Where(c => c.State.Value == CarouselItemState.Hidden)) c.State.Value = CarouselItemState.NotSelected; - if (Children.Any(c => c.Visible) && Children.All(c => c.State != CarouselItemState.Selected)) - Children.First(c => c.Visible).State.Value = CarouselItemState.Selected; + if (InternalChildren.Any(c => c.Visible) && InternalChildren.All(c => c.State != CarouselItemState.Selected)) + InternalChildren.First(c => c.Visible).State.Value = CarouselItemState.Selected; } }; } diff --git a/osu.Game/Screens/Select/Carousel/CarouselItem.cs b/osu.Game/Screens/Select/Carousel/CarouselItem.cs index 68148814e0..7b0202cd87 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselItem.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselItem.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; using osu.Framework.Configuration; -using osu.Framework.Extensions.IEnumerableExtensions; namespace osu.Game.Screens.Select.Carousel { @@ -14,45 +13,62 @@ namespace osu.Game.Screens.Select.Carousel public readonly Bindable State = new Bindable(CarouselItemState.NotSelected); - protected virtual IEnumerable Children { get; set; } + public IReadOnlyList Children => InternalChildren; + + protected List InternalChildren { get; set; } public bool Visible => State.Value != CarouselItemState.Hidden && !Filtered.Value; - public readonly Lazy> Drawables; - - protected CarouselItem() + public IEnumerable Drawables { - Drawables = new Lazy>(() => + get { List items = new List(); - var self = CreateDrawableRepresentation(); + var self = drawableRepresentation.Value; if (self != null) items.Add(self); - if (Children != null) - foreach (var c in Children) - items.AddRange(c.Drawables.Value); + if (InternalChildren != null) + foreach (var c in InternalChildren) + items.AddRange(c.Drawables); return items; - }); + } + } + + public virtual void AddChild(CarouselItem i) => (InternalChildren ?? (InternalChildren = new List())).Add(i); + + public virtual void RemoveChild(CarouselItem i) => InternalChildren?.Remove(i); + + protected CarouselItem() + { + drawableRepresentation = new Lazy(CreateDrawableRepresentation); State.ValueChanged += v => { - if (Children == null) return; + if (InternalChildren == null) return; switch (v) { case CarouselItemState.Hidden: case CarouselItemState.NotSelected: - Children.ForEach(c => c.State.Value = CarouselItemState.Hidden); + InternalChildren.ForEach(c => c.State.Value = CarouselItemState.Hidden); break; } }; } + private readonly Lazy drawableRepresentation; + protected abstract DrawableCarouselItem CreateDrawableRepresentation(); - public virtual void Filter(FilterCriteria criteria) => Children?.ForEach(c => c.Filter(criteria)); + public virtual void Filter(FilterCriteria criteria) + { + InternalChildren?.Sort((x, y) => x.CompareTo(criteria, y)); + InternalChildren?.ForEach(c => c.Filter(criteria)); + } + + public virtual int CompareTo(FilterCriteria criteria, CarouselItem other) => GetHashCode().CompareTo(other.GetHashCode()); } public enum CarouselItemState