From a8a2c233a004adf866e155fd0b95c701109475c9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sat, 16 Dec 2017 00:33:09 +0900 Subject: [PATCH] Add tests for (and fix) removal of last item in carousel --- .../Visual/TestCaseBeatmapCarousel.cs | 40 +++++++++++++++++-- osu.Game/Screens/Select/BeatmapCarousel.cs | 8 ++-- .../Screens/Select/Carousel/CarouselGroup.cs | 4 +- .../Carousel/CarouselGroupEagerSelect.cs | 17 ++++++++ .../Screens/Select/Carousel/CarouselItem.cs | 9 +++++ 5 files changed, 69 insertions(+), 9 deletions(-) diff --git a/osu.Game.Tests/Visual/TestCaseBeatmapCarousel.cs b/osu.Game.Tests/Visual/TestCaseBeatmapCarousel.cs index 12e6e292f6..8c8ec8a670 100644 --- a/osu.Game.Tests/Visual/TestCaseBeatmapCarousel.cs +++ b/osu.Game.Tests/Visual/TestCaseBeatmapCarousel.cs @@ -39,7 +39,9 @@ namespace osu.Game.Tests.Visual private readonly Stack selectedSets = new Stack(); - private const int set_count = 1000; + private BeatmapInfo currentSelection; + + private const int set_count = 5; [BackgroundDependencyLoader] private void load() @@ -54,6 +56,8 @@ namespace osu.Game.Tests.Visual for (int i = 1; i <= set_count; i++) beatmapSets.Add(createTestBeatmapSet(i)); + carousel.SelectionChanged = s => currentSelection = s; + AddStep("Load Beatmaps", () => { carousel.BeatmapSets = beatmapSets; }); AddUntilStep(() => carousel.BeatmapSets.Any(), "Wait for load"); @@ -63,6 +67,8 @@ namespace osu.Game.Tests.Visual testRandom(); testAddRemove(); testSorting(); + + testRemoveAll(); } private void ensureRandomFetchSuccess() => @@ -145,6 +151,8 @@ namespace osu.Game.Tests.Visual { // basic filtering + setSelected(1, 1); + AddStep("Filter", () => carousel.Filter(new FilterCriteria { SearchText = "set #3!" }, false)); checkVisibleItemCount(diff: false, count: 1); checkVisibleItemCount(diff: true, count: 3); @@ -163,8 +171,19 @@ namespace osu.Game.Tests.Visual setSelected(1, 2); AddStep("Filter some difficulties", () => carousel.Filter(new FilterCriteria { SearchText = "Normal" }, false)); checkSelected(1, 1); + AddStep("Un-filter", () => carousel.Filter(new FilterCriteria(), false)); checkSelected(1, 1); + + AddStep("Filter all", () => carousel.Filter(new FilterCriteria { SearchText = "Dingo" }, false)); + + checkVisibleItemCount(false, 0); + checkVisibleItemCount(true, 0); + AddAssert("Selection is null", () => currentSelection == null); + + AddStep("Un-filter", () => carousel.Filter(new FilterCriteria(), false)); + + AddAssert("Selection is non-null", () => currentSelection != null); } /// @@ -225,6 +244,21 @@ namespace osu.Game.Tests.Visual AddAssert($"Check #{set_count} is at bottom", () => carousel.BeatmapSets.Last().Metadata.Title.EndsWith($"#{set_count}!")); } + private void testRemoveAll() + { + setSelected(2, 1); + + AddAssert("Selection is non-null", () => currentSelection != null); + + AddUntilStep(() => + { + carousel.RemoveBeatmapSet(carousel.BeatmapSets.Last()); + return !carousel.BeatmapSets.Any(); + }, "Remove all"); + + AddAssert("Selection is null", () => currentSelection == null); + } + private BeatmapSetInfo createTestBeatmapSet(int i) { @@ -237,9 +271,9 @@ namespace osu.Game.Tests.Visual { OnlineBeatmapSetID = i, // Create random metadata, then we can check if sorting works based on these - Artist = $"peppy{i.ToString().PadLeft(6,'0')}", + Artist = $"peppy{i.ToString().PadLeft(6, '0')}", Title = $"test set #{i}!", - AuthorString = string.Concat(Enumerable.Repeat((char)('z' - Math.Min(25,i - 1)), 5)) + AuthorString = string.Concat(Enumerable.Repeat((char)('z' - Math.Min(25, i - 1)), 5)) }, Beatmaps = new List(new[] { diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index b0b5c932ba..fab5ce473a 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -62,7 +62,7 @@ namespace osu.Game.Screens.Select { List newSets = null; - CarouselGroup newRoot = new CarouselGroup(); + CarouselGroup newRoot = new CarouselGroupEagerSelect(); Task.Run(() => { @@ -102,7 +102,7 @@ namespace osu.Game.Screens.Select private readonly Stack randomSelectedBeatmaps = new Stack(); protected List Items = new List(); - private CarouselGroup root = new CarouselGroup(); + private CarouselGroup root = new CarouselGroupEagerSelect(); public BeatmapCarousel() { @@ -189,7 +189,7 @@ namespace osu.Game.Screens.Select public void SelectNext(int direction = 1, bool skipDifficulties = true) { // todo: we may want to refactor and remove this as an optimisation in the future. - if (beatmapSets.All(g => !g.Visible)) + if (beatmapSets.All(g => g.Filtered)) { SelectionChanged?.Invoke(null); return; @@ -304,7 +304,7 @@ namespace osu.Game.Screens.Select root.Filter(criteria); updateItems(); - if (selectedBeatmap?.Filtered == false) + if (selectedBeatmap != null && !selectedBeatmap.Filtered) select(selectedBeatmap); else if (lastBeatmap != null && !lastSet.Filtered) { diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroup.cs b/osu.Game/Screens/Select/Carousel/CarouselGroup.cs index 695a8e1bc4..151e73f45f 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroup.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroup.cs @@ -19,7 +19,7 @@ namespace osu.Game.Screens.Select.Carousel public override void AddChild(CarouselItem i) { - i.State.ValueChanged += v => itemStateChanged(i, v); + i.State.ValueChanged += v => ItemStateChanged(i, v); base.AddChild(i); } @@ -28,7 +28,7 @@ namespace osu.Game.Screens.Select.Carousel if (items != null) InternalChildren = items; } - private void itemStateChanged(CarouselItem item, CarouselItemState value) + protected virtual void ItemStateChanged(CarouselItem item, CarouselItemState value) { // todo: check state of selected item. diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs index b9312882a1..0a3c7b40ba 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs @@ -24,5 +24,22 @@ namespace osu.Game.Screens.Select.Carousel } }; } + + protected override void ItemStateChanged(CarouselItem item, CarouselItemState value) + { + base.ItemStateChanged(item, value); + + if (value == CarouselItemState.NotSelected) + { + if (Children.All(i => i.State != CarouselItemState.Selected)) + { + var first = Children.FirstOrDefault(i => !i.Filtered); + if (first != null) + { + first.State.Value = CarouselItemState.Selected; + } + } + } + } } } diff --git a/osu.Game/Screens/Select/Carousel/CarouselItem.cs b/osu.Game/Screens/Select/Carousel/CarouselItem.cs index 7b0202cd87..fba7e4321c 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselItem.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselItem.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using osu.Framework.Configuration; +using osu.Framework.Logging; namespace osu.Game.Screens.Select.Carousel { @@ -48,6 +49,8 @@ namespace osu.Game.Screens.Select.Carousel { if (InternalChildren == null) return; + Logger.Log($"State changed to {v}"); + switch (v) { case CarouselItemState.Hidden: @@ -56,6 +59,12 @@ namespace osu.Game.Screens.Select.Carousel break; } }; + + Filtered.ValueChanged += v => + { + if (v && State == CarouselItemState.Selected) + State.Value = CarouselItemState.NotSelected; + }; } private readonly Lazy drawableRepresentation;