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 #29322

Merged
merged 3 commits into from
Aug 7, 2024

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 <[email protected]>. 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)

@peppy
Copy link
Member Author

peppy commented Aug 7, 2024

@smoogipoo sorry, once more..

// 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made a very big oversight while testing 😅. Pushed a fix.

@smoogipoo smoogipoo merged commit a06cb9a into ppy:master Aug 7, 2024
12 of 13 checks passed
@peppy peppy deleted the daily-challenge-import-improvement branch August 13, 2024 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants