Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix clicking the beatmap import notification at the daily challenge screen exiting to main menu #29316

Closed
wants to merge 0 commits into from

Conversation

peppy
Copy link
Member

@peppy peppy commented Aug 7, 2024

Addresses #29303.

This also fixes an issue with presentation that I found while testing (the present would fail if you just deleted the beatmap you're testing re-import with). I attempted to write a test for this but give up after 20 minutes of trying. Getting the deletion in the same state is hard. Test diff below if anyone wants to give it further time:

diff --git a/osu.Game.Tests/Visual/Navigation/TestScenePresentBeatmap.cs b/osu.Game.Tests/Visual/Navigation/TestScenePresentBeatmap.cs
index c054792168..2570488425 100644
--- a/osu.Game.Tests/Visual/Navigation/TestScenePresentBeatmap.cs
+++ b/osu.Game.Tests/Visual/Navigation/TestScenePresentBeatmap.cs
@@ -1,8 +1,6 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
-#nullable disable
-
 using System;
 using System.Linq;
 using NUnit.Framework;
@@ -37,6 +35,22 @@ public void TestFromMainMenu()
             presentSecondDifficultyAndConfirm(secondImport, 3);
         }
 
+        [Test]
+        public void TestFromMainMenuWithDelete()
+        {
+            var firstImport = importBeatmap(1);
+            var secondImport = importBeatmap(3);
+
+            presentAndConfirm(firstImport);
+            returnToMenu();
+            presentAndConfirm(secondImport);
+            returnToMenu();
+
+            AddStep("delete first", () => Game.BeatmapManager.Delete(firstImport()));
+            var firstImportAgain = importBeatmap(1);
+            presentAndConfirm(firstImportAgain);
+        }
+
         [Test]
         public void TestFromMainMenuDifferentRuleset()
         {
@@ -133,9 +147,9 @@ private void returnToMenu()
             AddUntilStep("wait for menu", () => Game.ScreenStack.CurrentScreen is MainMenu);
         }
 
-        private Func<BeatmapSetInfo> importBeatmap(int i, RulesetInfo ruleset = null)
+        private Func<BeatmapSetInfo> importBeatmap(int i, RulesetInfo? ruleset = null)
         {
-            BeatmapSetInfo imported = null;
+            BeatmapSetInfo? imported = null;
             AddStep($"import beatmap {i}", () =>
             {
                 var metadata = new BeatmapMetadata
@@ -171,7 +185,7 @@ private Func<BeatmapSetInfo> importBeatmap(int i, RulesetInfo ruleset = null)
 
             AddAssert($"import {i} succeeded", () => imported != null);
 
-            return () => imported;
+            return () => imported!;
         }
 
         private void confirmBeatmapInSongSelect(Func<BeatmapSetInfo> getImport)

// If the import was for a different beatmap, pass the duty off to global handling.
if (beatmap.BeatmapSetInfo.OnlineID != playlistItem.Beatmap.BeatmapSet!.OnlineID)
{
game?.PresentBeatmap(beatmap.BeatmapSetInfo, b => b.ID == beatmap.BeatmapInfo.ID);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is infinitely looping, creating millions of PerformFromMenuRunner classes and whatnot other garbage.

@peppy
Copy link
Member Author

peppy commented Aug 7, 2024

recreated this at #29321. somehow my local git master started tracking this as a remote...

@smoogipoo can you post your review there again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants