Merge pull request #25949 from peppy/fix-song-select-extra-selections

Fix beatmap updates causing one extra carousel selection

osu.Game/Screens/Select/BeatmapCarousel.cs

@@ -455,30 +455,13 @@ namespace osu.Game.Screens.Select
 
         private void updateBeatmapSet(BeatmapSetInfo beatmapSet)
         {
-            Guid? previouslySelectedID = null;
-
             originalBeatmapSetsDetached.RemoveAll(set => set.ID == beatmapSet.ID);
             originalBeatmapSetsDetached.Add(beatmapSet.Detach());
 
-            // If the selected beatmap is about to be removed, store its ID so it can be re-selected if required
+            var newSets = new List<CarouselBeatmapSet>();
-            if (selectedBeatmapSet?.BeatmapSet.ID == beatmapSet.ID)
-                previouslySelectedID = selectedBeatmap?.BeatmapInfo.ID;
-
-            var removedSets = root.RemoveItemsByID(beatmapSet.ID);
-
-            foreach (var removedSet in removedSets)
-            {
-                // If we don't remove this here, it may remain in a hidden state until scrolled off screen.
-                // Doesn't really affect anything during actual user interaction, but makes testing annoying.
-                var removedDrawable = Scroll.FirstOrDefault(c => c.Item == removedSet);
-                if (removedDrawable != null)
-                    expirePanelImmediately(removedDrawable);
-            }
 
             if (beatmapsSplitOut)
             {
-                var newSets = new List<CarouselBeatmapSet>();
-
                 foreach (var beatmap in beatmapSet.Beatmaps)
                 {
                     var newSet = createCarouselSet(new BeatmapSetInfo(new[] { beatmap })
@@ -489,18 +472,7 @@ namespace osu.Game.Screens.Select
                     });
 
                     if (newSet != null)
-                    {
                         newSets.Add(newSet);
-                        root.AddItem(newSet);
-                    }
-                }
-
-                // check if we can/need to maintain our current selection.
-                if (previouslySelectedID != null)
-                {
-                    var toSelect = newSets.FirstOrDefault(s => s.Beatmaps.Any(b => b.BeatmapInfo.ID == previouslySelectedID))
-                                   ?? newSets.FirstOrDefault();
-                    select(toSelect);
                 }
             }
             else
@@ -508,13 +480,18 @@ namespace osu.Game.Screens.Select
                 var newSet = createCarouselSet(beatmapSet);
 
                 if (newSet != null)
-                {
+                    newSets.Add(newSet);
-                    root.AddItem(newSet);
+            }
 
-                    // check if we can/need to maintain our current selection.
+            var removedSets = root.ReplaceItem(beatmapSet, newSets);
-                    if (previouslySelectedID != null)
+
-                        select((CarouselItem?)newSet.Beatmaps.FirstOrDefault(b => b.BeatmapInfo.ID == previouslySelectedID) ?? newSet);
+            // If we don't remove these here, it may remain in a hidden state until scrolled off screen.
-                }
+            // Doesn't really affect anything during actual user interaction, but makes testing annoying.
+            foreach (var removedSet in removedSets)
+            {
+                var removedDrawable = Scroll.FirstOrDefault(c => c.Item == removedSet);
+                if (removedDrawable != null)
+                    expirePanelImmediately(removedDrawable);
             }
         }
 
@@ -1207,6 +1184,43 @@ namespace osu.Game.Screens.Select
                 base.AddItem(i);
             }
 
+            /// <summary>
+            /// A special method to handle replace operations (general for updating a beatmap).
+            /// Avoids event-driven selection flip-flopping during the remove/add process.
+            /// </summary>
+            /// <param name="oldItem">The beatmap set to be replaced.</param>
+            /// <param name="newItems">All new items to replace the removed beatmap set.</param>
+            /// <returns>All removed items, for any further processing.</returns>
+            public IEnumerable<CarouselBeatmapSet> ReplaceItem(BeatmapSetInfo oldItem, List<CarouselBeatmapSet> newItems)
+            {
+                var previousSelection = (LastSelected as CarouselBeatmapSet)?.Beatmaps
+                                                                            .FirstOrDefault(s => s.State.Value == CarouselItemState.Selected)
+                                                                            ?.BeatmapInfo;
+
+                bool wasSelected = previousSelection?.BeatmapSet?.ID == oldItem.ID;
+
+                // Without doing this, the removal of the old beatmap will cause carousel's eager selection
+                // logic to invoke, causing one unnecessary selection.
+                DisableSelection = true;
+                var removedSets = RemoveItemsByID(oldItem.ID);
+                DisableSelection = false;
+
+                foreach (var set in newItems)
+                    AddItem(set);
+
+                // Check if we can/need to maintain our current selection.
+                if (wasSelected)
+                {
+                    CarouselBeatmap? matchingBeatmap = newItems.SelectMany(s => s.Beatmaps)
+                                                               .FirstOrDefault(b => b.BeatmapInfo.ID == previousSelection?.ID);
+
+                    if (matchingBeatmap != null)
+                        matchingBeatmap.State.Value = CarouselItemState.Selected;
+                }
+
+                return removedSets;
+            }
+
             public IEnumerable<CarouselBeatmapSet> RemoveItemsByID(Guid beatmapSetID)
             {
                 if (BeatmapSetsByID.TryGetValue(beatmapSetID, out var carouselBeatmapSets))

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

@@ -36,13 +36,13 @@ namespace osu.Game.Screens.Select.Carousel
         /// items have been filtered. This bool will be true during the base <see cref="Filter(FilterCriteria)"/>
         /// operation.
         /// </summary>
-        private bool filteringItems;
+        protected bool DisableSelection;
 
         public override void Filter(FilterCriteria criteria)
         {
-            filteringItems = true;
+            DisableSelection = true;
             base.Filter(criteria);
-            filteringItems = false;
+            DisableSelection = false;
 
             attemptSelection();
         }
@@ -95,7 +95,7 @@ namespace osu.Game.Screens.Select.Carousel
 
         private void attemptSelection()
         {
-            if (filteringItems) return;
+            if (DisableSelection) return;
 
             // we only perform eager selection if we are a currently selected group.
             if (State.Value != CarouselItemState.Selected) return;