Actually fix realm selection retention regression

ppy · Dec 19, 2023 · 8f5d21d · 8f5d21d
1 parent 7e9c1b2
commit 8f5d21d
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 6 deletions.
diff --git a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs
@@ -464,8 +464,6 @@ public void TestSelectionRetainedOnBeatmapUpdate()
                 manager.Import(testBeatmapSetInfo);
             }, 10);
 
-            AddStep("Force realm refresh", () => Realm.Run(r => r.Refresh()));
-
             AddUntilStep("has selection", () => songSelect!.Carousel.SelectedBeatmapInfo?.BeatmapSet?.OnlineID, () => Is.EqualTo(originalOnlineSetID));
 
             Task<Live<BeatmapSetInfo>?> updateTask = null!;
@@ -478,8 +476,6 @@ public void TestSelectionRetainedOnBeatmapUpdate()
             });
             AddUntilStep("wait for update completion", () => updateTask.IsCompleted);
 
-            AddStep("Force realm refresh", () => Realm.Run(r => r.Refresh()));
-
             AddUntilStep("retained selection", () => songSelect!.Carousel.SelectedBeatmapInfo?.BeatmapSet?.OnlineID, () => Is.EqualTo(originalOnlineSetID));
         }
 

diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs
@@ -269,8 +269,28 @@ private void deletedBeatmapSetsChanged(IRealmCollection<BeatmapSetInfo> sender,
             if (changes == null)
                 return;
 
-            foreach (int i in changes.InsertedIndices)
-                removeBeatmapSet(sender[i].ID);
+            var removeableSets = changes.InsertedIndices.Select(i => sender[i].ID).ToHashSet();
+
+            // This schedule is required to retain selection of beatmaps over an ImportAsUpdate operation.
+            // This is covered by TestPlaySongSelect.TestSelectionRetainedOnBeatmapUpdate.
+            //
+            // In short, we have specialised logic in `beatmapSetsChanged` (directly below) to infer that an
+            // update operation has occurred. For this to work, we need to confirm the `DeletePending` flag
+            // of the current selection.
+            //
+            // If we don't schedule the following code, it is possible for the `deleteBeatmapSetsChanged` handler
+            // to be invoked before the `beatmapSetsChanged` handler (realm call order seems non-deterministic)
+            // which will lead to the currently selected beatmap changing via `CarouselGroupEagerSelect`.
+            //
+            // We need a better path forward here. A few ideas:
+            // - Avoid the necessity of having realm subscriptions on deleted/hidden items, maybe by storing all guids in realm
+            //   to a local list so we can better look them up on receiving `DeletedIndices`.
+            // - Add a new property on `BeatmapSetInfo` to link to the pre-update set, and use that to handle the update case.
+            Schedule(() =>
+            {
+                foreach (var set in removeableSets)
+                    removeBeatmapSet(set);
+            });
         }
 
         private void beatmapSetsChanged(IRealmCollection<BeatmapSetInfo> sender, ChangeSet? changes)