diff --git a/osu.Game.Tests/Visual/TestCaseBeatmapCarousel.cs b/osu.Game.Tests/Visual/TestCaseBeatmapCarousel.cs index ccf9ecb7f9..973ea71cbf 100644 --- a/osu.Game.Tests/Visual/TestCaseBeatmapCarousel.cs +++ b/osu.Game.Tests/Visual/TestCaseBeatmapCarousel.cs @@ -74,9 +74,14 @@ namespace osu.Game.Tests.Visual 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 checkSelected(int set, int? diff = null) => + AddAssert($"selected is set{set}{(diff.HasValue ? $" diff{diff.Value}" : "")}", () => + { + if (diff != null) + return carousel.SelectedBeatmap == carousel.BeatmapSets.Skip(set - 1).First().Beatmaps.Skip(diff.Value - 1).First(); + + return carousel.BeatmapSets.Skip(set - 1).First().Beatmaps.Contains(carousel.SelectedBeatmap); + }); private void setSelected(int set, int diff) => AddStep($"select set{set} diff{diff}", () => @@ -95,7 +100,7 @@ namespace osu.Game.Tests.Visual } private void checkVisibleItemCount(bool diff, int count) => - AddAssert($"{count} {(diff ? "diff" : "set")} visible", () => + AddAssert($"{count} {(diff ? "diffs" : "sets")} visible", () => carousel.Items.Count(s => (diff ? s.Item is CarouselBeatmap : s.Item is CarouselBeatmapSet) && s.Item.Visible) == count); private void nextRandom() => @@ -138,8 +143,9 @@ namespace osu.Game.Tests.Visual advanceSelection(diff: false); advanceSelection(diff: false); - checkSelected(1, 1); + checkSelected(1, 2); + advanceSelection(direction: -1, diff: true); advanceSelection(direction: -1, diff: true); checkSelected(set_count, 3); } @@ -234,7 +240,7 @@ namespace osu.Game.Tests.Visual checkVisibleItemCount(false, set_count); - checkSelected(set_count, 1); + checkSelected(set_count, 2); } /// diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index fab5ce473a..e37040b3de 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -74,6 +74,7 @@ namespace osu.Game.Screens.Select { root = newRoot; updateItems(); + BeatmapSetsChanged?.Invoke(); }); }); } @@ -85,11 +86,11 @@ namespace osu.Game.Screens.Select private void updateItems() { scrollableContent.Clear(false); - Items = root.Drawables.ToList(); - yPositionsCache.Invalidate(); - BeatmapSetsChanged?.Invoke(); + + if (root.Children == null || root.Children.All(c => c.Filtered)) + SelectionChanged?.Invoke(null); } private readonly List yPositions = new List(); @@ -126,9 +127,6 @@ namespace osu.Game.Screens.Select root.RemoveChild(existingSet); updateItems(); - - if (existingSet.State == CarouselItemState.Selected) - SelectNext(); } public void UpdateBeatmapSet(BeatmapSetInfo beatmapSet) @@ -188,13 +186,6 @@ namespace osu.Game.Screens.Select /// Whether to skip individual difficulties and only increment over full groups. 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.Filtered)) - { - SelectionChanged?.Invoke(null); - return; - } - int originalIndex = Items.IndexOf(selectedBeatmap?.Drawables.First()); int currentIndex = originalIndex; @@ -214,13 +205,14 @@ namespace osu.Game.Screens.Select select(beatmap); return; case CarouselBeatmapSet set: + if (!skipDifficulties) continue; select(set); return; } } } - private IEnumerable getVisibleSets() => beatmapSets.Where(select => select.Visible); + private IEnumerable getVisibleSets() => beatmapSets.Where(select => !select.Filtered); public void SelectNextRandom() { @@ -298,28 +290,9 @@ namespace osu.Game.Screens.Select { FilterTask = null; - var lastSet = selectedBeatmapSet; - var lastBeatmap = selectedBeatmap; - root.Filter(criteria); updateItems(); - if (selectedBeatmap != null && !selectedBeatmap.Filtered) - select(selectedBeatmap); - else if (lastBeatmap != null && !lastSet.Filtered) - { - var searchable = lastSet.Beatmaps.AsEnumerable(); - - // search forwards first then backwards if nothing found. - var nextAvailable = - searchable.SkipWhile(b => b != lastBeatmap).FirstOrDefault(b => !b.Filtered) ?? - searchable.Reverse().SkipWhile(b => b != lastBeatmap).FirstOrDefault(b => !b.Filtered); - - select(nextAvailable); - } - else - SelectNext(); - ScrollToSelected(false); }; diff --git a/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs b/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs index a0d91cf23b..cd3fefe8e6 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs @@ -49,5 +49,7 @@ namespace osu.Game.Screens.Select.Carousel return Beatmap.StarDifficulty.CompareTo(otherBeatmap.Beatmap.StarDifficulty); } } + + public override string ToString() => Beatmap.ToString(); } } diff --git a/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs b/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs index 655ed2057d..885595fc51 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs @@ -52,5 +52,7 @@ namespace osu.Game.Screens.Select.Carousel base.Filter(criteria); Filtered.Value = InternalChildren.All(i => i.Filtered); } + + public override string ToString() => BeatmapSet.ToString(); } } diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroup.cs b/osu.Game/Screens/Select/Carousel/CarouselGroup.cs index 151e73f45f..98aa7cd5c5 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroup.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroup.cs @@ -2,7 +2,6 @@ // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE using System.Collections.Generic; -using osu.Framework.Configuration; namespace osu.Game.Screens.Select.Carousel { @@ -13,13 +12,11 @@ namespace osu.Game.Screens.Select.Carousel { private readonly List items; - public readonly Bindable Selected = new Bindable(); - protected override DrawableCarouselItem CreateDrawableRepresentation() => null; public override void AddChild(CarouselItem i) { - i.State.ValueChanged += v => ItemStateChanged(i, v); + i.State.ValueChanged += v => ChildItemStateChanged(i, v); base.AddChild(i); } @@ -28,7 +25,7 @@ namespace osu.Game.Screens.Select.Carousel if (items != null) InternalChildren = items; } - protected virtual void ItemStateChanged(CarouselItem item, CarouselItemState value) + protected virtual void ChildItemStateChanged(CarouselItem item, CarouselItemState value) { // todo: check state of selected item. @@ -42,7 +39,6 @@ namespace osu.Game.Screens.Select.Carousel } State.Value = CarouselItemState.Selected; - Selected.Value = item; } } } diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs index 0a3c7b40ba..4fca0c69b6 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs @@ -15,31 +15,47 @@ namespace osu.Game.Screens.Select.Carousel State.ValueChanged += v => { if (v == CarouselItemState.Selected) - { - foreach (var c in InternalChildren.Where(c => c.State.Value == CarouselItemState.Hidden)) - c.State.Value = CarouselItemState.NotSelected; - - if (InternalChildren.Any(c => c.Visible) && InternalChildren.All(c => c.State != CarouselItemState.Selected)) - InternalChildren.First(c => c.Visible).State.Value = CarouselItemState.Selected; - } + attemptSelection(); }; } - protected override void ItemStateChanged(CarouselItem item, CarouselItemState value) - { - base.ItemStateChanged(item, value); + private int lastSelectedIndex; - if (value == CarouselItemState.NotSelected) + public override void Filter(FilterCriteria criteria) + { + base.Filter(criteria); + attemptSelection(); + } + + protected override void ChildItemStateChanged(CarouselItem item, CarouselItemState value) + { + base.ChildItemStateChanged(item, value); + + switch (value) { - if (Children.All(i => i.State != CarouselItemState.Selected)) - { - var first = Children.FirstOrDefault(i => !i.Filtered); - if (first != null) - { - first.State.Value = CarouselItemState.Selected; - } - } + case CarouselItemState.Selected: + lastSelectedIndex = InternalChildren.IndexOf(item); + break; + case CarouselItemState.NotSelected: + attemptSelection(); + break; } } + + private void attemptSelection() + { + // we only perform eager selection if we are a currently selected group. + if (State != CarouselItemState.Selected) return; + + // we only perform eager selection if none of our children are in a selected state already. + if (Children.Any(i => i.State == CarouselItemState.Selected)) return; + + CarouselItem nextToSelect = + Children.Skip(lastSelectedIndex).FirstOrDefault(i => !i.Filtered) ?? + Children.Reverse().Skip(InternalChildren.Count - lastSelectedIndex).FirstOrDefault(i => !i.Filtered); + + if (nextToSelect != null) + nextToSelect.State.Value = CarouselItemState.Selected; + } } } diff --git a/osu.Game/Screens/Select/Carousel/CarouselItem.cs b/osu.Game/Screens/Select/Carousel/CarouselItem.cs index fba7e4321c..91fe600dc7 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselItem.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselItem.cs @@ -18,7 +18,10 @@ namespace osu.Game.Screens.Select.Carousel protected List InternalChildren { get; set; } - public bool Visible => State.Value != CarouselItemState.Hidden && !Filtered.Value; + /// + /// This item is not in a hidden state. + /// + public bool Visible => State.Value != CarouselItemState.Hidden && !Filtered; public IEnumerable Drawables { @@ -39,7 +42,14 @@ namespace osu.Game.Screens.Select.Carousel public virtual void AddChild(CarouselItem i) => (InternalChildren ?? (InternalChildren = new List())).Add(i); - public virtual void RemoveChild(CarouselItem i) => InternalChildren?.Remove(i); + public virtual void RemoveChild(CarouselItem i) + { + InternalChildren?.Remove(i); + + // it's important we do the deselection after removing, so any further actions based on + // State.ValueChanged make decisions post-removal. + if (i.State.Value == CarouselItemState.Selected) i.State.Value = CarouselItemState.NotSelected; + } protected CarouselItem() { @@ -47,9 +57,9 @@ namespace osu.Game.Screens.Select.Carousel State.ValueChanged += v => { - if (InternalChildren == null) return; + Logger.Log($"State of {this} changed to {v}"); - Logger.Log($"State changed to {v}"); + if (InternalChildren == null) return; switch (v) { @@ -57,6 +67,12 @@ namespace osu.Game.Screens.Select.Carousel case CarouselItemState.NotSelected: InternalChildren.ForEach(c => c.State.Value = CarouselItemState.Hidden); break; + case CarouselItemState.Selected: + InternalChildren.ForEach(c => + { + if (c.State == CarouselItemState.Hidden) c.State.Value = CarouselItemState.NotSelected; + }); + break; } };