Merge pull request #19399 from frenzibyte/carousel-order-stability

Maintain sort stability by using carousel item ID as a fallback

osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs

@@ -522,15 +522,15 @@ namespace osu.Game.Tests.Visual.SongSelect
         {
             var sets = new List<BeatmapSetInfo>();
 
-            for (int i = 0; i < 20; i++)
+            for (int i = 0; i < 10; i++)
             {
                 var set = TestResources.CreateTestBeatmapSetInfo();
 
                 // only need to set the first as they are a shared reference.
                 var beatmap = set.Beatmaps.First();
 
-                beatmap.Metadata.Artist = "same artist";
+                beatmap.Metadata.Artist = $"artist {i / 2}";
-                beatmap.Metadata.Title = "same title";
+                beatmap.Metadata.Title = $"title {9 - i}";
 
                 sets.Add(set);
             }
@@ -540,10 +540,13 @@ namespace osu.Game.Tests.Visual.SongSelect
             loadBeatmaps(sets);
 
             AddStep("Sort by artist", () => carousel.Filter(new FilterCriteria { Sort = SortMode.Artist }, false));
-            AddAssert("Items remain in original order", () => carousel.BeatmapSets.Select((set, index) => set.OnlineID == index + idOffset).All(b => b));
+            AddAssert("Items remain in original order", () => carousel.BeatmapSets.Select((set, index) => set.OnlineID == idOffset + index).All(b => b));
 
             AddStep("Sort by title", () => carousel.Filter(new FilterCriteria { Sort = SortMode.Title }, false));
-            AddAssert("Items remain in original order", () => carousel.BeatmapSets.Select((set, index) => set.OnlineID == index + idOffset).All(b => b));
+            AddAssert("Items are in reverse order", () => carousel.BeatmapSets.Select((set, index) => set.OnlineID == idOffset + sets.Count - index - 1).All(b => b));
+
+            AddStep("Sort by artist", () => carousel.Filter(new FilterCriteria { Sort = SortMode.Artist }, false));
+            AddAssert("Items reset to original order", () => carousel.BeatmapSets.Select((set, index) => set.OnlineID == idOffset + index).All(b => b));
         }
 
         [Test]

osu.Game/Screens/Select/Carousel/CarouselGroup.cs

@@ -7,7 +7,7 @@ using System.Linq;
 namespace osu.Game.Screens.Select.Carousel
 {
     /// <summary>
-    /// A group which ensures only one child is selected.
+    /// A group which ensures only one item is selected.
     /// </summary>
     public class CarouselGroup : CarouselItem
     {
@@ -18,13 +18,15 @@ namespace osu.Game.Screens.Select.Carousel
         private List<CarouselItem> items = new List<CarouselItem>();
 
         /// <summary>
-        /// Used to assign a monotonically increasing ID to children as they are added. This member is
+        /// Used to assign a monotonically increasing ID to items as they are added. This member is
-        /// incremented whenever a child is added.
+        /// incremented whenever an item is added.
         /// </summary>
-        private ulong currentChildID;
+        private ulong currentItemID;
 
         private Comparer<CarouselItem>? criteriaComparer;
 
+        private static readonly Comparer<CarouselItem> item_id_comparer = Comparer<CarouselItem>.Create((x, y) => x.ItemID.CompareTo(y.ItemID));
+
         private FilterCriteria? lastCriteria;
 
         protected int GetIndexOfItem(CarouselItem lastSelected) => items.IndexOf(lastSelected);
@@ -41,7 +43,7 @@ namespace osu.Game.Screens.Select.Carousel
         public virtual void AddItem(CarouselItem i)
         {
             i.State.ValueChanged += state => ChildItemStateChanged(i, state.NewValue);
-            i.ChildID = ++currentChildID;
+            i.ItemID = ++currentItemID;
 
             if (lastCriteria != null)
             {
@@ -90,7 +92,7 @@ namespace osu.Game.Screens.Select.Carousel
 
             // IEnumerable<T>.OrderBy() is used instead of List<T>.Sort() to ensure sorting stability
             criteriaComparer = Comparer<CarouselItem>.Create((x, y) => x.CompareTo(criteria, y));
-            items = items.OrderBy(c => c, criteriaComparer).ToList();
+            items = items.OrderBy(c => c, criteriaComparer).ThenBy(c => c, item_id_comparer).ToList();
 
             lastCriteria = criteria;
         }

osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs

@@ -10,7 +10,7 @@ using System.Linq;
 namespace osu.Game.Screens.Select.Carousel
 {
     /// <summary>
-    /// A group which ensures at least one child is selected (if the group itself is selected).
+    /// A group which ensures at least one item is selected (if the group itself is selected).
     /// </summary>
     public class CarouselGroupEagerSelect : CarouselGroup
     {
@@ -35,16 +35,16 @@ namespace osu.Game.Screens.Select.Carousel
 
         /// <summary>
         /// To avoid overhead during filter operations, we don't attempt any selections until after all
-        /// children have been filtered. This bool will be true during the base <see cref="Filter(FilterCriteria)"/>
+        /// items have been filtered. This bool will be true during the base <see cref="Filter(FilterCriteria)"/>
         /// operation.
         /// </summary>
-        private bool filteringChildren;
+        private bool filteringItems;
 
         public override void Filter(FilterCriteria criteria)
         {
-            filteringChildren = true;
+            filteringItems = true;
             base.Filter(criteria);
-            filteringChildren = false;
+            filteringItems = false;
 
             attemptSelection();
         }
@@ -97,12 +97,12 @@ namespace osu.Game.Screens.Select.Carousel
 
         private void attemptSelection()
         {
-            if (filteringChildren) return;
+            if (filteringItems) return;
 
             // we only perform eager selection if we are a currently selected group.
             if (State.Value != CarouselItemState.Selected) return;
 
-            // we only perform eager selection if none of our children are in a selected state already.
+            // we only perform eager selection if none of our items are in a selected state already.
             if (Items.Any(i => i.State.Value == CarouselItemState.Selected)) return;
 
             PerformSelection();

osu.Game/Screens/Select/Carousel/CarouselItem.cs

@@ -38,7 +38,7 @@ namespace osu.Game.Screens.Select.Carousel
         /// <summary>
         /// Used as a default sort method for <see cref="CarouselItem"/>s of differing types.
         /// </summary>
-        internal ulong ChildID;
+        internal ulong ItemID;
 
         /// <summary>
         /// Create a fresh drawable version of this item.
@@ -49,7 +49,7 @@ namespace osu.Game.Screens.Select.Carousel
         {
         }
 
-        public virtual int CompareTo(FilterCriteria criteria, CarouselItem other) => ChildID.CompareTo(other.ChildID);
+        public virtual int CompareTo(FilterCriteria criteria, CarouselItem other) => ItemID.CompareTo(other.ItemID);
 
         public int CompareTo(CarouselItem other) => CarouselYPosition.CompareTo(other.CarouselYPosition);
     }