From fc0c9f76bd8ef2a9c5250d1977782ddb9916b678 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 21 Jul 2022 16:24:46 +0900 Subject: [PATCH] Fix `UpdateBeatmapSetButton` intermittent test failure Carousel would only expire items when off-screen. This meant that for a case (like a test) where items are generally always on-screen, `UpdateBeatmapSet` calls would result in panels remaining hidden but not cleaned up. --- .../TestSceneUpdateBeatmapSetButton.cs | 2 ++ osu.Game/Screens/Select/BeatmapCarousel.cs | 27 ++++++++++++++----- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneUpdateBeatmapSetButton.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneUpdateBeatmapSetButton.cs index a95f145897..70786c93e7 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneUpdateBeatmapSetButton.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneUpdateBeatmapSetButton.cs @@ -71,6 +71,7 @@ namespace osu.Game.Tests.Visual.SongSelect carousel.UpdateBeatmapSet(testBeatmapSetInfo); }); + AddUntilStep("only one set visible", () => carousel.ChildrenOfType().Count() == 1); AddUntilStep("update button visible", () => getUpdateButton() != null); AddStep("click button", () => getUpdateButton()?.TriggerClick()); @@ -120,6 +121,7 @@ namespace osu.Game.Tests.Visual.SongSelect carousel.UpdateBeatmapSet(testBeatmapSetInfo); }); + AddUntilStep("only one set visible", () => carousel.ChildrenOfType().Count() == 1); AddUntilStep("update button visible", () => getUpdateButton() != null); AddStep("click button", () => getUpdateButton()?.TriggerClick()); diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 04d77bfb0e..75caa3c9a3 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -316,8 +316,13 @@ namespace osu.Game.Screens.Select previouslySelectedID = selectedBeatmap?.BeatmapInfo.ID; var newSet = createCarouselSet(beatmapSet); + var removedSet = root.RemoveChild(beatmapSet.ID); - root.RemoveChild(beatmapSet.ID); + // 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 (newSet != null) { @@ -696,11 +701,7 @@ namespace osu.Game.Screens.Select // panel loaded as drawable but not required by visible range. // remove but only if too far off-screen if (panel.Y + panel.DrawHeight < visibleUpperBound - distance_offscreen_before_unload || panel.Y > visibleBottomBound + distance_offscreen_before_unload) - { - // may want a fade effect here (could be seen if a huge change happens, like a set with 20 difficulties becomes selected). - panel.ClearTransforms(); - panel.Expire(); - } + expirePanelImmediately(panel); } // Add those items within the previously found index range that should be displayed. @@ -730,6 +731,13 @@ namespace osu.Game.Screens.Select } } + private static void expirePanelImmediately(DrawableCarouselItem panel) + { + // may want a fade effect here (could be seen if a huge change happens, like a set with 20 difficulties becomes selected). + panel.ClearTransforms(); + panel.Expire(); + } + private readonly CarouselBoundsItem carouselBoundsItem = new CarouselBoundsItem(); private (int firstIndex, int lastIndex) getDisplayRange() @@ -972,10 +980,15 @@ namespace osu.Game.Screens.Select base.AddItem(i); } - public void RemoveChild(Guid beatmapSetID) + public CarouselBeatmapSet RemoveChild(Guid beatmapSetID) { if (BeatmapSetsByID.TryGetValue(beatmapSetID, out var carouselBeatmapSet)) + { RemoveItem(carouselBeatmapSet); + return carouselBeatmapSet; + } + + return null; } public override void RemoveItem(CarouselItem i)