diff --git a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs index 6581ce0323..4c6a5c93d9 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs @@ -1211,6 +1211,20 @@ namespace osu.Game.Tests.Visual.SongSelect AddAssert("0 matching shown", () => songSelect.ChildrenOfType().Single().InformationalText == "0 matches"); } + [Test] + [Solo] + public void TestHardDeleteHandledCorrectly() + { + createSongSelect(); + + addRulesetImportStep(0); + AddAssert("3 matching shown", () => songSelect.ChildrenOfType().Single().InformationalText == "3 matches"); + + AddStep("hard delete beatmap", () => Realm.Write(r => r.RemoveRange(r.All().Where(s => !s.Protected)))); + + AddUntilStep("0 matching shown", () => songSelect.ChildrenOfType().Single().InformationalText == "0 matches"); + } + [Test] public void TestDeleteHotkey() { diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 56e7c24985..9d61e2a977 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -197,9 +197,7 @@ namespace osu.Game.Screens.Select private CarouselRoot root; private IDisposable? subscriptionSets; - private IDisposable? subscriptionDeletedSets; private IDisposable? subscriptionBeatmaps; - private IDisposable? subscriptionHiddenBeatmaps; private readonly DrawablePool setPool = new DrawablePool(100); @@ -246,6 +244,9 @@ namespace osu.Game.Screens.Select if (!loadedTestBeatmaps) { + // This is performing an unnecessary second lookup on realm (in addition to the subscription), but for performance reasons + // we require it to be separate: the subscription's initial callback (with `ChangeSet` of `null`) will run on the update + // thread. If we attempt to detach beatmaps in this callback the game will fall over (it takes time). realm.Run(r => loadBeatmapSets(getBeatmapSets(r))); } } @@ -253,53 +254,21 @@ namespace osu.Game.Screens.Select [Resolved] private RealmAccess realm { get; set; } = null!; + /// + /// Track GUIDs of all sets in realm to allow handling deletions. + /// + private readonly List realmBeatmapSets = new List(); + protected override void LoadComplete() { base.LoadComplete(); subscriptionSets = realm.RegisterForNotifications(getBeatmapSets, beatmapSetsChanged); subscriptionBeatmaps = realm.RegisterForNotifications(r => r.All().Where(b => !b.Hidden), beatmapsChanged); - - // Can't use main subscriptions because we can't lookup deleted indices. - // https://github.com/realm/realm-dotnet/discussions/2634#discussioncomment-1605595. - subscriptionDeletedSets = realm.RegisterForNotifications(r => r.All().Where(s => s.DeletePending && !s.Protected), deletedBeatmapSetsChanged); - subscriptionHiddenBeatmaps = realm.RegisterForNotifications(r => r.All().Where(b => b.Hidden), beatmapsChanged); } - private void deletedBeatmapSetsChanged(IRealmCollection sender, ChangeSet? changes) - { - // If loading test beatmaps, avoid overwriting with realm subscription callbacks. - if (loadedTestBeatmaps) - return; - - if (changes == null) - return; - - 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); - - invalidateAfterChange(); - }); - } + private readonly HashSet setsRequiringUpdate = new HashSet(); + private readonly HashSet setsRequiringRemoval = new HashSet(); private void beatmapSetsChanged(IRealmCollection sender, ChangeSet? changes) { @@ -307,96 +276,91 @@ namespace osu.Game.Screens.Select if (loadedTestBeatmaps) return; - var setsRequiringUpdate = new HashSet(); - var setsRequiringRemoval = new HashSet(); - if (changes == null) { - // During initial population, we must manually account for the fact that our original query was done on an async thread. - // Since then, there may have been imports or deletions. - // Here we manually catch up on any changes. - var realmSets = new HashSet(); + realmBeatmapSets.Clear(); + realmBeatmapSets.AddRange(sender.Select(r => r.ID)); + setsRequiringRemoval.Clear(); + setsRequiringUpdate.Clear(); - for (int i = 0; i < sender.Count; i++) - realmSets.Add(sender[i].ID); - - foreach (var id in realmSets) - { - if (!root.BeatmapSetsByID.ContainsKey(id)) - setsRequiringUpdate.Add(realm.Realm.Find(id)!.Detach()); - } - - foreach (var id in root.BeatmapSetsByID.Keys) - { - if (!realmSets.Contains(id)) - setsRequiringRemoval.Add(id); - } + loadBeatmapSets(sender); } else { - foreach (int i in changes.NewModifiedIndices) - setsRequiringUpdate.Add(sender[i].Detach()); + foreach (int i in changes.DeletedIndices.OrderDescending()) + { + setsRequiringRemoval.Add(realmBeatmapSets[i]); + realmBeatmapSets.RemoveAt(i); + } foreach (int i in changes.InsertedIndices) + { + realmBeatmapSets.Insert(i, sender[i].ID); + setsRequiringUpdate.Add(sender[i].Detach()); + } + + foreach (int i in changes.NewModifiedIndices) setsRequiringUpdate.Add(sender[i].Detach()); } - // All local operations must be scheduled. - // - // If we don't schedule, beatmaps getting changed while song select is suspended (ie. last played being updated) - // will cause unexpected sounds and operations to occur in the background. - Schedule(() => + Scheduler.AddOnce(processBeatmapChanges); + } + + // All local operations must be scheduled. + // + // If we don't schedule, beatmaps getting changed while song select is suspended (ie. last played being updated) + // will cause unexpected sounds and operations to occur in the background. + private void processBeatmapChanges() + { + try { - try + foreach (var set in setsRequiringRemoval) removeBeatmapSet(set); + + foreach (var set in setsRequiringUpdate) updateBeatmapSet(set); + + if (setsRequiringRemoval.Count > 0 && SelectedBeatmapInfo != null) { - foreach (var set in setsRequiringRemoval) - removeBeatmapSet(set); + // If SelectedBeatmapInfo is non-null, the set should also be non-null. + Debug.Assert(SelectedBeatmapSet != null); - foreach (var set in setsRequiringUpdate) - updateBeatmapSet(set); + // To handle the beatmap update flow, attempt to track selection changes across delete-insert transactions. + // When an update occurs, the previous beatmap set is either soft or hard deleted. + // Check if the current selection was potentially deleted by re-querying its validity. + bool selectedSetMarkedDeleted = realm.Run(r => r.Find(SelectedBeatmapSet.ID)?.DeletePending != false); - if (changes?.DeletedIndices.Length > 0 && SelectedBeatmapInfo != null) + if (selectedSetMarkedDeleted && setsRequiringUpdate.Any()) { - // If SelectedBeatmapInfo is non-null, the set should also be non-null. - Debug.Assert(SelectedBeatmapSet != null); - - // To handle the beatmap update flow, attempt to track selection changes across delete-insert transactions. - // When an update occurs, the previous beatmap set is either soft or hard deleted. - // Check if the current selection was potentially deleted by re-querying its validity. - bool selectedSetMarkedDeleted = realm.Run(r => r.Find(SelectedBeatmapSet.ID)?.DeletePending != false); - - if (selectedSetMarkedDeleted && setsRequiringUpdate.Any()) + // If it is no longer valid, make the bold assumption that an updated version will be available in the modified/inserted indices. + // This relies on the full update operation being in a single transaction, so please don't change that. + foreach (var set in setsRequiringUpdate) { - // If it is no longer valid, make the bold assumption that an updated version will be available in the modified/inserted indices. - // This relies on the full update operation being in a single transaction, so please don't change that. - foreach (var set in setsRequiringUpdate) + foreach (var beatmapInfo in set.Beatmaps) { - foreach (var beatmapInfo in set.Beatmaps) - { - if (!((IBeatmapMetadataInfo)beatmapInfo.Metadata).Equals(SelectedBeatmapInfo.Metadata)) - continue; + if (!((IBeatmapMetadataInfo)beatmapInfo.Metadata).Equals(SelectedBeatmapInfo.Metadata)) continue; - // Best effort matching. We can't use ID because in the update flow a new version will get its own GUID. - if (beatmapInfo.DifficultyName == SelectedBeatmapInfo.DifficultyName) - { - SelectBeatmap(beatmapInfo); - return; - } + // Best effort matching. We can't use ID because in the update flow a new version will get its own GUID. + if (beatmapInfo.DifficultyName == SelectedBeatmapInfo.DifficultyName) + { + SelectBeatmap(beatmapInfo); + return; } } - - // If a direct selection couldn't be made, it's feasible that the difficulty name (or beatmap metadata) changed. - // Let's attempt to follow set-level selection anyway. - SelectBeatmap(setsRequiringUpdate.First().Beatmaps.First()); } + + // If a direct selection couldn't be made, it's feasible that the difficulty name (or beatmap metadata) changed. + // Let's attempt to follow set-level selection anyway. + SelectBeatmap(setsRequiringUpdate.First().Beatmaps.First()); } } - finally - { - BeatmapSetsLoaded = true; - invalidateAfterChange(); - } - }); + } + finally + { + BeatmapSetsLoaded = true; + invalidateAfterChange(); + } + + setsRequiringRemoval.Clear(); + setsRequiringUpdate.Clear(); } private void beatmapsChanged(IRealmCollection sender, ChangeSet? changes) @@ -1312,9 +1276,7 @@ namespace osu.Game.Screens.Select base.Dispose(isDisposing); subscriptionSets?.Dispose(); - subscriptionDeletedSets?.Dispose(); subscriptionBeatmaps?.Dispose(); - subscriptionHiddenBeatmaps?.Dispose(); } } }