From 8105d4854a53c48617ca7638a989a9442174e69a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 27 Jul 2022 13:30:38 +0900 Subject: [PATCH 1/7] Fix beatmap carousel not maintaining selection if currently selected beatmap is updated --- osu.Game/Screens/Select/BeatmapCarousel.cs | 32 ++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 75caa3c9a3..c6f2b61049 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -265,6 +265,38 @@ namespace osu.Game.Screens.Select foreach (int i in changes.InsertedIndices) UpdateBeatmapSet(sender[i].Detach()); + + if (changes.DeletedIndices.Length > 0) + { + // 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) + { + // If it is no longer valid, make the bold assumption that an updated version will be available in the modified indices. + // This relies on the full update operation being in a single transaction, so please don't change that. + foreach (int i in changes.NewModifiedIndices) + { + var beatmapSetInfo = sender[i]; + + foreach (var beatmapInfo in beatmapSetInfo.Beatmaps) + { + // Best effort matching. We can't use ID because in the update flow a new version will get its own GUID. + bool selectionMatches = + ((IBeatmapMetadataInfo)beatmapInfo.Metadata).Equals(SelectedBeatmapInfo.Metadata) + && beatmapInfo.DifficultyName == SelectedBeatmapInfo.DifficultyName; + + if (selectionMatches) + { + SelectBeatmap(beatmapInfo); + break; + } + } + } + } + } } private void beatmapsChanged(IRealmCollection sender, ChangeSet changes, Exception error) From 24d75612e222fe3e055d1f5fabf623727dc81727 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 27 Jul 2022 14:18:53 +0900 Subject: [PATCH 2/7] Always attempt to follow selection, even if difficulty name / metadata change --- osu.Game/Screens/Select/BeatmapCarousel.cs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index c6f2b61049..0430c15a1d 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -283,18 +283,21 @@ namespace osu.Game.Screens.Select foreach (var beatmapInfo in beatmapSetInfo.Beatmaps) { - // Best effort matching. We can't use ID because in the update flow a new version will get its own GUID. - bool selectionMatches = - ((IBeatmapMetadataInfo)beatmapInfo.Metadata).Equals(SelectedBeatmapInfo.Metadata) - && beatmapInfo.DifficultyName == SelectedBeatmapInfo.DifficultyName; + if (!((IBeatmapMetadataInfo)beatmapInfo.Metadata).Equals(SelectedBeatmapInfo.Metadata)) + continue; - if (selectionMatches) + // 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); - break; + 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(sender[changes.NewModifiedIndices.First()].Beatmaps.First()); } } } From e6a3659581e62b9c181d1c0ffe4f82a8988a8d05 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 27 Jul 2022 14:23:47 +0900 Subject: [PATCH 3/7] Guard against `NewModifiedIndices` being empty --- osu.Game/Screens/Select/BeatmapCarousel.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 0430c15a1d..e9419e7156 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -273,7 +273,7 @@ namespace osu.Game.Screens.Select // 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) + if (selectedSetMarkedDeleted && changes.NewModifiedIndices.Any()) { // If it is no longer valid, make the bold assumption that an updated version will be available in the modified indices. // This relies on the full update operation being in a single transaction, so please don't change that. From 9d2c2b71cf07a172a0f83e246e79e6b06822e29a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 16 Aug 2022 16:21:35 +0900 Subject: [PATCH 4/7] Change conditional to check for insertions in addition to modifications It is possible that the import process itself marks the previous beatmaps as deleted due to an overlap in metadata or otherwise. --- osu.Game/Screens/Select/BeatmapCarousel.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index e9419e7156..e9f676d32f 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -273,11 +273,13 @@ namespace osu.Game.Screens.Select // 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 && changes.NewModifiedIndices.Any()) + int[] modifiedAndInserted = changes.NewModifiedIndices.Concat(changes.InsertedIndices).ToArray(); + + if (selectedSetMarkedDeleted && modifiedAndInserted.Any()) { // If it is no longer valid, make the bold assumption that an updated version will be available in the modified indices. // This relies on the full update operation being in a single transaction, so please don't change that. - foreach (int i in changes.NewModifiedIndices) + foreach (int i in modifiedAndInserted) { var beatmapSetInfo = sender[i]; From 81c0a641b4f596ca4c9b0a7d9c19b97c389f2ee1 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 26 Aug 2022 14:51:08 +0300 Subject: [PATCH 5/7] Fix selection fallback path not updated to check inserted indices --- osu.Game/Screens/Select/BeatmapCarousel.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index e9f676d32f..0cbc17c67a 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -299,7 +299,7 @@ namespace osu.Game.Screens.Select // 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(sender[changes.NewModifiedIndices.First()].Beatmaps.First()); + SelectBeatmap(sender[modifiedAndInserted.First()].Beatmaps.First()); } } } From a3e595a9aac215987a6cb5ee5cad82cfad615a5c Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 26 Aug 2022 14:51:19 +0300 Subject: [PATCH 6/7] Update comment to include inserted indices --- osu.Game/Screens/Select/BeatmapCarousel.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 0cbc17c67a..0f130714f1 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -277,7 +277,7 @@ namespace osu.Game.Screens.Select if (selectedSetMarkedDeleted && modifiedAndInserted.Any()) { - // If it is no longer valid, make the bold assumption that an updated version will be available in the modified indices. + // 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 (int i in modifiedAndInserted) { From e8ae6840ea6da67cb206e55955c1040951378769 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 29 Aug 2022 15:23:34 +0900 Subject: [PATCH 7/7] Add test coverage of selection being retained --- .../SongSelect/TestScenePlaySongSelect.cs | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs index 3d8f496c9a..5db46e3097 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs @@ -6,15 +6,18 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Threading.Tasks; using NUnit.Framework; using osu.Framework.Allocation; using osu.Framework.Audio; using osu.Framework.Bindables; +using osu.Framework.Extensions; using osu.Framework.Extensions.IEnumerableExtensions; using osu.Framework.Graphics.Containers; using osu.Framework.Platform; using osu.Framework.Screens; using osu.Framework.Testing; +using osu.Framework.Utils; using osu.Game.Beatmaps; using osu.Game.Configuration; using osu.Game.Database; @@ -24,6 +27,7 @@ using osu.Game.Online.API.Requests.Responses; using osu.Game.Online.Chat; using osu.Game.Overlays; using osu.Game.Overlays.Mods; +using osu.Game.Overlays.Notifications; using osu.Game.Rulesets; using osu.Game.Rulesets.Mods; using osu.Game.Rulesets.Osu; @@ -413,6 +417,55 @@ namespace osu.Game.Tests.Visual.SongSelect AddUntilStep("no selection", () => songSelect.Carousel.SelectedBeatmapInfo == null); } + [Test] + public void TestSelectionRetainedOnBeatmapUpdate() + { + createSongSelect(); + changeRuleset(0); + + Live original = null!; + int originalOnlineSetID = 0; + + AddStep(@"Sort by artist", () => config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.Artist)); + + AddStep("import original", () => + { + original = manager.Import(new ImportTask(TestResources.GetQuickTestBeatmapForImport())).GetResultSafely(); + originalOnlineSetID = original!.Value.OnlineID; + }); + + // This will move the beatmap set to a different location in the carousel. + AddStep("Update original with bogus info", () => + { + original.PerformWrite(set => + { + foreach (var beatmap in set.Beatmaps) + { + beatmap.Metadata.Artist = "ZZZZZ"; + beatmap.OnlineID = 12804; + } + }); + }); + + AddRepeatStep("import other beatmaps", () => + { + var testBeatmapSetInfo = TestResources.CreateTestBeatmapSetInfo(); + + foreach (var beatmap in testBeatmapSetInfo.Beatmaps) + beatmap.Metadata.Artist = ((char)RNG.Next('A', 'Z')).ToString(); + + manager.Import(testBeatmapSetInfo); + }, 10); + + AddUntilStep("has selection", () => songSelect.Carousel.SelectedBeatmapInfo?.BeatmapSet?.OnlineID == originalOnlineSetID); + + Task> updateTask = null!; + AddStep("update beatmap", () => updateTask = manager.ImportAsUpdate(new ProgressNotification(), new ImportTask(TestResources.GetQuickTestBeatmapForImport()), original.Value)); + AddUntilStep("wait for update completion", () => updateTask.IsCompleted); + + AddUntilStep("retained selection", () => songSelect.Carousel.SelectedBeatmapInfo?.BeatmapSet?.OnlineID == originalOnlineSetID); + } + [Test] public void TestPresentNewRulesetNewBeatmap() {