Refactor selection and activation handling

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.

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]

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]

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]

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<BeatmapPanel>().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();
 

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)

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;
         }
 

osu.Game/Screens/SelectV2/BeatmapSetPanel.cs

@@ -1,7 +1,6 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. 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

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();
+                }
+            }
         }
 
         /// <summary>
-        /// 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.
         /// </summary>
-        public void TryActivateSelection()
+        /// <param name="item"></param>
+        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;
 
         /// <summary>
-        /// Called when an item is "selected".
+        /// Called after an item becomes the <see cref="CurrentSelection"/>.
+        /// Should be used to handle any group expansion, item visibility changes, etc.
         /// </summary>
-        /// <returns>Whether the item should be selected.</returns>
-        protected virtual bool HandleItemSelected(object? model) => true;
+        protected virtual void HandleItemSelected(object? model) { }
 
         /// <summary>
-        /// Called when an item is "deselected".
+        /// Called when the <see cref="CurrentSelection"/> changes to a new selection.
+        /// Should be used to handle any group expansion, item visibility changes, etc.
         /// </summary>
-        protected virtual void HandleItemDeselected(object? model)
-        {
-        }
+        protected virtual void HandleItemDeselected(object? model) { }
 
         /// <summary>
-        /// Called when an item is "activated".
+        /// Called when an item is activated via user input (keyboard traversal or a mouse click).
         /// </summary>
         /// <remarks>
-        /// 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 <see cref="CurrentSelection"/>.
+        /// - Start gameplay on a beatmap difficulty if already selected.
         /// </remarks>
         /// <param name="item">The carousel item which was activated.</param>
-        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);

osu.Game/Screens/SelectV2/GroupPanel.cs

@@ -1,7 +1,6 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. 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