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