diff --git a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs index 6b53277964..518035fb82 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs @@ -464,7 +464,7 @@ namespace osu.Game.Tests.Visual.SongSelect manager.Import(testBeatmapSetInfo); }, 10); - AddUntilStep("has selection", () => songSelect!.Carousel.SelectedBeatmapInfo?.BeatmapSet?.OnlineID == originalOnlineSetID); + AddUntilStep("has selection", () => songSelect!.Carousel.SelectedBeatmapInfo?.BeatmapSet?.OnlineID, () => Is.EqualTo(originalOnlineSetID)); Task?> updateTask = null!; @@ -476,7 +476,7 @@ namespace osu.Game.Tests.Visual.SongSelect }); AddUntilStep("wait for update completion", () => updateTask.IsCompleted); - AddUntilStep("retained selection", () => songSelect!.Carousel.SelectedBeatmapInfo?.BeatmapSet?.OnlineID == originalOnlineSetID); + AddUntilStep("retained selection", () => songSelect!.Carousel.SelectedBeatmapInfo?.BeatmapSet?.OnlineID, () => Is.EqualTo(originalOnlineSetID)); } [Test] diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index eb47a7201a..fe7eee701f 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -64,7 +64,7 @@ namespace osu.Game.Screens.Select /// /// The total count of non-filtered beatmaps displayed. /// - public int CountDisplayed => beatmapSets.Where(s => !s.Filtered.Value).Sum(s => s.Beatmaps.Count(b => !b.Filtered.Value)); + public int CountDisplayed => beatmapSets.Where(s => !s.Filtered.Value).Sum(s => s.TotalItemsNotFiltered); /// /// The currently selected beatmap set. @@ -168,7 +168,10 @@ namespace osu.Game.Screens.Select applyActiveCriteria(false); if (loadedTestBeatmaps) - signalBeatmapsLoaded(); + { + invalidateAfterChange(); + BeatmapSetsLoaded = true; + } // Restore selection if (selectedBeatmapBefore != null && newRoot.BeatmapSetsByID.TryGetValue(selectedBeatmapBefore.BeatmapSet!.ID, out var newSelectionCandidates)) @@ -266,8 +269,30 @@ namespace osu.Game.Screens.Select 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); + + invalidateAfterChange(); + }); } private void beatmapSetsChanged(IRealmCollection sender, ChangeSet? changes) @@ -289,7 +314,7 @@ namespace osu.Game.Screens.Select foreach (var id in realmSets) { if (!root.BeatmapSetsByID.ContainsKey(id)) - UpdateBeatmapSet(realm.Realm.Find(id)!.Detach()); + updateBeatmapSet(realm.Realm.Find(id)!.Detach()); } foreach (var id in root.BeatmapSetsByID.Keys) @@ -298,15 +323,16 @@ namespace osu.Game.Screens.Select removeBeatmapSet(id); } - signalBeatmapsLoaded(); + invalidateAfterChange(); + BeatmapSetsLoaded = true; return; } foreach (int i in changes.NewModifiedIndices) - UpdateBeatmapSet(sender[i].Detach()); + updateBeatmapSet(sender[i].Detach()); foreach (int i in changes.InsertedIndices) - UpdateBeatmapSet(sender[i].Detach()); + updateBeatmapSet(sender[i].Detach()); if (changes.DeletedIndices.Length > 0 && SelectedBeatmapInfo != null) { @@ -316,7 +342,7 @@ namespace osu.Game.Screens.Select // 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; + bool selectedSetMarkedDeleted = sender.Realm.Find(SelectedBeatmapSet.ID)?.DeletePending != false; int[] modifiedAndInserted = changes.NewModifiedIndices.Concat(changes.InsertedIndices).ToArray(); @@ -347,6 +373,8 @@ namespace osu.Game.Screens.Select SelectBeatmap(sender[modifiedAndInserted.First()].Beatmaps.First()); } } + + invalidateAfterChange(); } private void beatmapsChanged(IRealmCollection sender, ChangeSet? changes) @@ -355,6 +383,8 @@ namespace osu.Game.Screens.Select if (changes == null) return; + bool changed = false; + foreach (int i in changes.InsertedIndices) { var beatmapInfo = sender[i]; @@ -367,17 +397,24 @@ namespace osu.Game.Screens.Select if (root.BeatmapSetsByID.TryGetValue(beatmapSet.ID, out var existingSets) && existingSets.SelectMany(s => s.Beatmaps).All(b => b.BeatmapInfo.ID != beatmapInfo.ID)) { - UpdateBeatmapSet(beatmapSet.Detach()); + updateBeatmapSet(beatmapSet.Detach()); + changed = true; } } + + if (changed) + invalidateAfterChange(); } private IQueryable getBeatmapSets(Realm realm) => realm.All().Where(s => !s.DeletePending && !s.Protected); - public void RemoveBeatmapSet(BeatmapSetInfo beatmapSet) => + public void RemoveBeatmapSet(BeatmapSetInfo beatmapSet) => Schedule(() => + { removeBeatmapSet(beatmapSet.ID); + invalidateAfterChange(); + }); - private void removeBeatmapSet(Guid beatmapSetID) => Schedule(() => + private void removeBeatmapSet(Guid beatmapSetID) { if (!root.BeatmapSetsByID.TryGetValue(beatmapSetID, out var existingSets)) return; @@ -392,16 +429,15 @@ namespace osu.Game.Screens.Select root.RemoveItem(set); } - - itemsCache.Invalidate(); - - if (!Scroll.UserScrolling) - ScrollToSelected(true); - - BeatmapSetsChanged?.Invoke(); - }); + } public void UpdateBeatmapSet(BeatmapSetInfo beatmapSet) => Schedule(() => + { + updateBeatmapSet(beatmapSet); + invalidateAfterChange(); + }); + + private void updateBeatmapSet(BeatmapSetInfo beatmapSet) { Guid? previouslySelectedID = null; @@ -464,14 +500,7 @@ namespace osu.Game.Screens.Select select((CarouselItem?)newSet.Beatmaps.FirstOrDefault(b => b.BeatmapInfo.ID == previouslySelectedID) ?? newSet); } } - - itemsCache.Invalidate(); - - if (!Scroll.UserScrolling) - ScrollToSelected(true); - - BeatmapSetsChanged?.Invoke(); - }); + } /// /// Selects a given beatmap on the carousel. @@ -748,15 +777,14 @@ namespace osu.Game.Screens.Select } } - private void signalBeatmapsLoaded() + private void invalidateAfterChange() { - if (!BeatmapSetsLoaded) - { - BeatmapSetsChanged?.Invoke(); - BeatmapSetsLoaded = true; - } - itemsCache.Invalidate(); + + if (!Scroll.UserScrolling) + ScrollToSelected(true); + + BeatmapSetsChanged?.Invoke(); } private float? scrollTarget; diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroup.cs b/osu.Game/Screens/Select/Carousel/CarouselGroup.cs index be841465bf..b2ca117cec 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroup.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroup.cs @@ -14,6 +14,8 @@ namespace osu.Game.Screens.Select.Carousel public IReadOnlyList Items => items; + public int TotalItemsNotFiltered { get; private set; } + private readonly List items = new List(); /// @@ -31,6 +33,9 @@ namespace osu.Game.Screens.Select.Carousel { items.Remove(i); + if (!i.Filtered.Value) + TotalItemsNotFiltered--; + // it's important we do the deselection after removing, so any further actions based on // State.ValueChanged make decisions post-removal. i.State.Value = CarouselItemState.Collapsed; @@ -55,6 +60,9 @@ namespace osu.Game.Screens.Select.Carousel // criteria may be null for initial population. the filtering will be applied post-add. items.Add(i); } + + if (!i.Filtered.Value) + TotalItemsNotFiltered++; } public CarouselGroup(List? items = null) @@ -84,7 +92,14 @@ namespace osu.Game.Screens.Select.Carousel { base.Filter(criteria); - items.ForEach(c => c.Filter(criteria)); + TotalItemsNotFiltered = 0; + + foreach (var c in items) + { + c.Filter(criteria); + if (!c.Filtered.Value) + TotalItemsNotFiltered++; + } // Sorting is expensive, so only perform if it's actually changed. if (lastCriteria?.RequiresSorting(criteria) != false) diff --git a/osu.Game/Screens/Select/SongSelect.cs b/osu.Game/Screens/Select/SongSelect.cs index dfea4e3794..d23a660ff6 100644 --- a/osu.Game/Screens/Select/SongSelect.cs +++ b/osu.Game/Screens/Select/SongSelect.cs @@ -162,7 +162,7 @@ namespace osu.Game.Screens.Select BleedBottom = Footer.HEIGHT, SelectionChanged = updateSelectedBeatmap, BeatmapSetsChanged = carouselBeatmapsLoaded, - FilterApplied = updateVisibleBeatmapCount, + FilterApplied = () => Scheduler.AddOnce(updateVisibleBeatmapCount), GetRecommendedBeatmap = s => recommender?.GetRecommendedBeatmap(s), }, c => carouselContainer.Child = c); @@ -843,7 +843,7 @@ namespace osu.Game.Screens.Select private void carouselBeatmapsLoaded() { bindBindables(); - updateVisibleBeatmapCount(); + Scheduler.AddOnce(updateVisibleBeatmapCount); Carousel.AllowSelection = true; @@ -877,7 +877,8 @@ namespace osu.Game.Screens.Select { // Intentionally not localised until we have proper support for this (see https://github.com/ppy/osu-framework/pull/4918 // but also in this case we want support for formatting a number within a string). - FilterControl.InformationalText = Carousel.CountDisplayed != 1 ? $"{Carousel.CountDisplayed:#,0} matches" : $"{Carousel.CountDisplayed:#,0} match"; + int carouselCountDisplayed = Carousel.CountDisplayed; + FilterControl.InformationalText = carouselCountDisplayed != 1 ? $"{carouselCountDisplayed:#,0} matches" : $"{carouselCountDisplayed:#,0} match"; } private bool boundLocalBindables;