From fe414b942f69230c473398b8ee2e199c06449a84 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 20 Jul 2021 15:44:32 +0900 Subject: [PATCH 1/3] Ensure online play subscreen is loaded before forwarding `OnExiting` Closes https://github.com/ppy/osu-framework/issues/4619 (actually not a framework issue; the framework correctly guards against this scenario, see https://github.com/ppy/osu-framework/blob/4e29504384bb78bbccc0d492de58124f17270eb2/osu.Framework/Screens/ScreenStack.cs#L277). Added the assertions to be very explicit about the nested stack's state at this point. Both of those events can only be triggered if the stack has a loaded screen (as far as I can tell), making this check unnecessary in those cases. --- osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs b/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs index ceee002c6e..3aef05c26e 100644 --- a/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs +++ b/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs @@ -1,6 +1,7 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using System.Diagnostics; using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Framework.Extensions.Color4Extensions; @@ -222,7 +223,9 @@ namespace osu.Game.Screens.OnlinePlay this.FadeIn(250); this.ScaleTo(1, 250, Easing.OutSine); - screenStack.CurrentScreen?.OnResuming(last); + Debug.Assert(screenStack.CurrentScreen != null); + screenStack.CurrentScreen.OnResuming(last); + base.OnResuming(last); UpdatePollingRate(isIdle.Value); @@ -233,14 +236,16 @@ namespace osu.Game.Screens.OnlinePlay this.ScaleTo(1.1f, 250, Easing.InSine); this.FadeOut(250); - screenStack.CurrentScreen?.OnSuspending(next); + Debug.Assert(screenStack.CurrentScreen != null); + screenStack.CurrentScreen.OnSuspending(next); UpdatePollingRate(isIdle.Value); } public override bool OnExiting(IScreen next) { - if (screenStack.CurrentScreen?.OnExiting(next) == true) + var subScreen = screenStack.CurrentScreen as Drawable; + if (subScreen?.IsLoaded == true && screenStack.CurrentScreen.OnExiting(next)) return true; RoomManager.PartRoom(); From 4113eae6ade3cc1d55819d0d19cde3fda4458877 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 20 Jul 2021 16:23:48 +0900 Subject: [PATCH 2/3] Add test coverage of fail scenario --- .../Multiplayer/TestSceneMultiplayer.cs | 33 +++++++++++++++++-- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs index 92f9c5733f..f39455dc85 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs @@ -104,6 +104,36 @@ namespace osu.Game.Tests.Visual.Multiplayer }); } + [Test] + public void TestExitMidJoin() + { + Room room = null; + + AddStep("create room", () => + { + room = new Room + { + Name = { Value = "Test Room" }, + Playlist = + { + new PlaylistItem + { + Beatmap = { Value = beatmaps.GetWorkingBeatmap(importedSet.Beatmaps.First(b => b.RulesetID == 0)).BeatmapInfo }, + Ruleset = { Value = new OsuRuleset().RulesetInfo }, + } + } + }; + }); + + AddStep("refresh rooms", () => multiplayerScreen.RoomManager.Filter.Value = new FilterCriteria()); + AddStep("select room", () => InputManager.Key(Key.Down)); + AddStep("join room and immediately exit", () => + { + multiplayerScreen.ChildrenOfType().Single().Open(room); + Schedule(() => Stack.CurrentScreen.Exit()); + }); + } + [Test] public void TestJoinRoomWithoutPassword() { @@ -179,9 +209,6 @@ namespace osu.Game.Tests.Visual.Multiplayer AddUntilStep("password prompt appeared", () => (passwordEntryPopover = InputManager.ChildrenOfType().FirstOrDefault()) != null); AddStep("enter password in text box", () => passwordEntryPopover.ChildrenOfType().First().Text = "password"); AddStep("press join room button", () => passwordEntryPopover.ChildrenOfType().First().Click()); - - AddUntilStep("wait for room open", () => this.ChildrenOfType().FirstOrDefault()?.IsLoaded == true); - AddUntilStep("wait for join", () => client.Room != null); } [Test] From 26cc4af87c03fe513f4b5d31e27fb61db7245519 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 20 Jul 2021 16:44:51 +0900 Subject: [PATCH 3/3] Revert unintended changes --- osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs index f39455dc85..36dd9c2de3 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs @@ -209,6 +209,9 @@ namespace osu.Game.Tests.Visual.Multiplayer AddUntilStep("password prompt appeared", () => (passwordEntryPopover = InputManager.ChildrenOfType().FirstOrDefault()) != null); AddStep("enter password in text box", () => passwordEntryPopover.ChildrenOfType().First().Text = "password"); AddStep("press join room button", () => passwordEntryPopover.ChildrenOfType().First().Click()); + + AddUntilStep("wait for room open", () => this.ChildrenOfType().FirstOrDefault()?.IsLoaded == true); + AddUntilStep("wait for join", () => client.Room != null); } [Test]