From 1171d44ad972573af689ee469c752f32168017a2 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Wed, 15 Jun 2022 03:34:08 +0300 Subject: [PATCH 1/7] Add failing test case --- .../Navigation/TestSceneFirstRunGame.cs | 46 +++++++++++++++++++ osu.Game/OsuGame.cs | 8 ++-- osu.Game/Tests/Visual/OsuGameTestScene.cs | 2 + 3 files changed, 52 insertions(+), 4 deletions(-) create mode 100644 osu.Game.Tests/Visual/Navigation/TestSceneFirstRunGame.cs diff --git a/osu.Game.Tests/Visual/Navigation/TestSceneFirstRunGame.cs b/osu.Game.Tests/Visual/Navigation/TestSceneFirstRunGame.cs new file mode 100644 index 0000000000..37d7faf1fb --- /dev/null +++ b/osu.Game.Tests/Visual/Navigation/TestSceneFirstRunGame.cs @@ -0,0 +1,46 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using NUnit.Framework; +using osu.Framework.Graphics.Containers; +using osu.Framework.Platform; +using osu.Game.Configuration; +using osu.Game.Online.API; +using osu.Game.Overlays.Notifications; + +namespace osu.Game.Tests.Visual.Navigation +{ + [System.ComponentModel.Description("game with first-run setup overlay")] + public class TestSceneFirstRunGame : OsuGameTestScene + { + public override void SetUpSteps() + { + base.SetUpSteps(); + + AddUntilStep("Wait for first-run setup", () => Game.FirstRunOverlay.State.Value == Visibility.Visible); + } + + [Test] + public void TestImportantNotificationDoesntInterruptSetup() + { + AddStep("post important notification", () => Game.Notifications.Post(new SimpleNotification { Text = "Important notification" })); + AddAssert("first-run setup still visible", () => Game.FirstRunOverlay.State.Value == Visibility.Visible); + } + + protected override TestOsuGame CreateTestGame() => new FirstRunGame(LocalStorage, API); + + private class FirstRunGame : TestOsuGame + { + public FirstRunGame(Storage storage, IAPIProvider api, string[] args = null) + : base(storage, api, args) + { + } + + protected override void LoadComplete() + { + base.LoadComplete(); + LocalConfig.SetValue(OsuSetting.ShowFirstRunSetup, true); + } + } + } +} diff --git a/osu.Game/OsuGame.cs b/osu.Game/OsuGame.cs index 1f9a1ce938..200f7613a1 100644 --- a/osu.Game/OsuGame.cs +++ b/osu.Game/OsuGame.cs @@ -153,7 +153,7 @@ namespace osu.Game protected SettingsOverlay Settings; - private FirstRunSetupOverlay firstRunOverlay; + protected FirstRunSetupOverlay FirstRunOverlay { get; private set; } private VolumeOverlay volume; @@ -845,7 +845,7 @@ namespace osu.Game loadComponentSingleFile(CreateUpdateManager(), Add, true); // overlay elements - loadComponentSingleFile(firstRunOverlay = new FirstRunSetupOverlay(), overlayContent.Add, true); + loadComponentSingleFile(FirstRunOverlay = new FirstRunSetupOverlay(), overlayContent.Add, true); loadComponentSingleFile(new ManageCollectionsDialog(), overlayContent.Add, true); loadComponentSingleFile(beatmapListing = new BeatmapListingOverlay(), overlayContent.Add, true); loadComponentSingleFile(dashboard = new DashboardOverlay(), overlayContent.Add, true); @@ -896,7 +896,7 @@ namespace osu.Game Add(new MusicKeyBindingHandler()); // side overlays which cancel each other. - var singleDisplaySideOverlays = new OverlayContainer[] { Settings, Notifications, firstRunOverlay }; + var singleDisplaySideOverlays = new OverlayContainer[] { Settings, Notifications, FirstRunOverlay }; foreach (var overlay in singleDisplaySideOverlays) { @@ -921,7 +921,7 @@ namespace osu.Game } // ensure only one of these overlays are open at once. - var singleDisplayOverlays = new OverlayContainer[] { firstRunOverlay, chatOverlay, news, dashboard, beatmapListing, changelogOverlay, rankingsOverlay, wikiOverlay }; + var singleDisplayOverlays = new OverlayContainer[] { FirstRunOverlay, chatOverlay, news, dashboard, beatmapListing, changelogOverlay, rankingsOverlay, wikiOverlay }; foreach (var overlay in singleDisplayOverlays) { diff --git a/osu.Game/Tests/Visual/OsuGameTestScene.cs b/osu.Game/Tests/Visual/OsuGameTestScene.cs index 6e4adb4d4c..5995b30b60 100644 --- a/osu.Game/Tests/Visual/OsuGameTestScene.cs +++ b/osu.Game/Tests/Visual/OsuGameTestScene.cs @@ -132,6 +132,8 @@ namespace osu.Game.Tests.Visual public new NotificationOverlay Notifications => base.Notifications; + public new FirstRunSetupOverlay FirstRunOverlay => base.FirstRunOverlay; + public new MusicController MusicController => base.MusicController; public new OsuConfigManager LocalConfig => base.LocalConfig; From ddeee09a5193fd79135540b26c54e81a14590e70 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Wed, 15 Jun 2022 03:36:38 +0300 Subject: [PATCH 2/7] Fix important notifications interrupting first-run setup --- osu.Game/Overlays/NotificationOverlay.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/osu.Game/Overlays/NotificationOverlay.cs b/osu.Game/Overlays/NotificationOverlay.cs index f1ed5c4ba6..e4ee2fe049 100644 --- a/osu.Game/Overlays/NotificationOverlay.cs +++ b/osu.Game/Overlays/NotificationOverlay.cs @@ -35,6 +35,9 @@ namespace osu.Game.Overlays [Resolved] private AudioManager audio { get; set; } + [Resolved(canBeNull: true)] + private FirstRunSetupOverlay firstRunSetup { get; set; } + [BackgroundDependencyLoader] private void load() { @@ -130,7 +133,9 @@ namespace osu.Game.Overlays var section = sections.Children.FirstOrDefault(s => s.AcceptTypes.Any(accept => accept.IsAssignableFrom(ourType))); section?.Add(notification, notification.DisplayOnTop ? -runningDepth : runningDepth); - if (notification.IsImportant) + // we don't want important notifications interrupting user on first-run setup. + // (this can happen when importing beatmaps inside setup, which posts import notifications) + if (notification.IsImportant && firstRunSetup?.State.Value != Visibility.Visible) Show(); updateCounts(); From 026bad7fc49bdbd116910514c1c9836eaba30d13 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Wed, 15 Jun 2022 18:11:28 +0300 Subject: [PATCH 3/7] Use notification processing mode logic instead --- osu.Game/Overlays/NotificationOverlay.cs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/osu.Game/Overlays/NotificationOverlay.cs b/osu.Game/Overlays/NotificationOverlay.cs index e4ee2fe049..b772dffa73 100644 --- a/osu.Game/Overlays/NotificationOverlay.cs +++ b/osu.Game/Overlays/NotificationOverlay.cs @@ -38,6 +38,8 @@ namespace osu.Game.Overlays [Resolved(canBeNull: true)] private FirstRunSetupOverlay firstRunSetup { get; set; } + private IBindable firstRunSetupState; + [BackgroundDependencyLoader] private void load() { @@ -84,7 +86,7 @@ namespace osu.Game.Overlays private void updateProcessingMode() { - bool enabled = OverlayActivationMode.Value == OverlayActivation.All || State.Value == Visibility.Visible; + bool enabled = (OverlayActivationMode.Value == OverlayActivation.All && firstRunSetupState?.Value != Visibility.Visible) || State.Value == Visibility.Visible; notificationsEnabler?.Cancel(); @@ -100,6 +102,10 @@ namespace osu.Game.Overlays base.LoadComplete(); State.ValueChanged += _ => updateProcessingMode(); + + firstRunSetupState = firstRunSetup.State.GetBoundCopy(); + firstRunSetupState.ValueChanged += _ => updateProcessingMode(); + OverlayActivationMode.BindValueChanged(_ => updateProcessingMode(), true); } @@ -133,9 +139,7 @@ namespace osu.Game.Overlays var section = sections.Children.FirstOrDefault(s => s.AcceptTypes.Any(accept => accept.IsAssignableFrom(ourType))); section?.Add(notification, notification.DisplayOnTop ? -runningDepth : runningDepth); - // we don't want important notifications interrupting user on first-run setup. - // (this can happen when importing beatmaps inside setup, which posts import notifications) - if (notification.IsImportant && firstRunSetup?.State.Value != Visibility.Visible) + if (notification.IsImportant) Show(); updateCounts(); From 387c54c25219aea13c8473880823b51e5201ff1f Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Wed, 15 Jun 2022 18:11:46 +0300 Subject: [PATCH 4/7] Ensure notification is displayed after first-run setup is hidden --- .../Visual/Navigation/TestSceneFirstRunGame.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/osu.Game.Tests/Visual/Navigation/TestSceneFirstRunGame.cs b/osu.Game.Tests/Visual/Navigation/TestSceneFirstRunGame.cs index 37d7faf1fb..18c6e84950 100644 --- a/osu.Game.Tests/Visual/Navigation/TestSceneFirstRunGame.cs +++ b/osu.Game.Tests/Visual/Navigation/TestSceneFirstRunGame.cs @@ -24,7 +24,17 @@ namespace osu.Game.Tests.Visual.Navigation public void TestImportantNotificationDoesntInterruptSetup() { AddStep("post important notification", () => Game.Notifications.Post(new SimpleNotification { Text = "Important notification" })); + AddAssert("no notification posted", () => Game.Notifications.UnreadCount.Value == 0); AddAssert("first-run setup still visible", () => Game.FirstRunOverlay.State.Value == Visibility.Visible); + + AddUntilStep("finish first-run setup", () => + { + Game.FirstRunOverlay.NextButton.TriggerClick(); + return Game.FirstRunOverlay.State.Value == Visibility.Hidden; + }); + AddWaitStep("wait for post delay", 5); + AddAssert("notifications shown", () => Game.Notifications.State.Value == Visibility.Visible); + AddAssert("notification posted", () => Game.Notifications.UnreadCount.Value == 1); } protected override TestOsuGame CreateTestGame() => new FirstRunGame(LocalStorage, API); From e7dcbddbeb4eb4febf48c79578c3d2595b27e765 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Wed, 15 Jun 2022 18:53:49 +0300 Subject: [PATCH 5/7] Fix potential null reference --- osu.Game/Overlays/NotificationOverlay.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/osu.Game/Overlays/NotificationOverlay.cs b/osu.Game/Overlays/NotificationOverlay.cs index b772dffa73..1fe446454c 100644 --- a/osu.Game/Overlays/NotificationOverlay.cs +++ b/osu.Game/Overlays/NotificationOverlay.cs @@ -2,6 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using System.Linq; +using JetBrains.Annotations; using osu.Framework.Extensions.IEnumerableExtensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; @@ -36,8 +37,10 @@ namespace osu.Game.Overlays private AudioManager audio { get; set; } [Resolved(canBeNull: true)] + [CanBeNull] private FirstRunSetupOverlay firstRunSetup { get; set; } + [CanBeNull] private IBindable firstRunSetupState; [BackgroundDependencyLoader] @@ -103,8 +106,10 @@ namespace osu.Game.Overlays State.ValueChanged += _ => updateProcessingMode(); - firstRunSetupState = firstRunSetup.State.GetBoundCopy(); - firstRunSetupState.ValueChanged += _ => updateProcessingMode(); + firstRunSetupState = firstRunSetup?.State.GetBoundCopy(); + + if (firstRunSetupState != null) + firstRunSetupState.ValueChanged += _ => updateProcessingMode(); OverlayActivationMode.BindValueChanged(_ => updateProcessingMode(), true); } From 6b30ee095041719fbeed4149e4e8f4c0325fc78b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 16 Jun 2022 17:26:43 +0900 Subject: [PATCH 6/7] Tidy up DI and binding logic --- osu.Game/Overlays/NotificationOverlay.cs | 25 +++++++++--------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/osu.Game/Overlays/NotificationOverlay.cs b/osu.Game/Overlays/NotificationOverlay.cs index 1fe446454c..d50768f43e 100644 --- a/osu.Game/Overlays/NotificationOverlay.cs +++ b/osu.Game/Overlays/NotificationOverlay.cs @@ -36,15 +36,10 @@ namespace osu.Game.Overlays [Resolved] private AudioManager audio { get; set; } - [Resolved(canBeNull: true)] - [CanBeNull] - private FirstRunSetupOverlay firstRunSetup { get; set; } + private readonly IBindable firstRunSetupVisibility = new Bindable(); - [CanBeNull] - private IBindable firstRunSetupState; - - [BackgroundDependencyLoader] - private void load() + [BackgroundDependencyLoader(true)] + private void load([CanBeNull] FirstRunSetupOverlay firstRunSetup) { X = WIDTH; Width = WIDTH; @@ -83,13 +78,16 @@ namespace osu.Game.Overlays } } }; + + if (firstRunSetup != null) + firstRunSetupVisibility.BindTo(firstRunSetup.State); } private ScheduledDelegate notificationsEnabler; private void updateProcessingMode() { - bool enabled = (OverlayActivationMode.Value == OverlayActivation.All && firstRunSetupState?.Value != Visibility.Visible) || State.Value == Visibility.Visible; + bool enabled = (OverlayActivationMode.Value == OverlayActivation.All && firstRunSetupVisibility?.Value != Visibility.Visible) || State.Value == Visibility.Visible; notificationsEnabler?.Cancel(); @@ -104,13 +102,8 @@ namespace osu.Game.Overlays { base.LoadComplete(); - State.ValueChanged += _ => updateProcessingMode(); - - firstRunSetupState = firstRunSetup?.State.GetBoundCopy(); - - if (firstRunSetupState != null) - firstRunSetupState.ValueChanged += _ => updateProcessingMode(); - + State.BindValueChanged(_ => updateProcessingMode()); + firstRunSetupVisibility.BindValueChanged(_ => updateProcessingMode()); OverlayActivationMode.BindValueChanged(_ => updateProcessingMode(), true); } From 251923c1067ab2e5e43c1ea8b4a819dff2187b37 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 16 Jun 2022 17:28:08 +0900 Subject: [PATCH 7/7] Convert `NotificationOverlay` to NRT --- osu.Game/Overlays/NotificationOverlay.cs | 27 ++++++++++++------------ 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/osu.Game/Overlays/NotificationOverlay.cs b/osu.Game/Overlays/NotificationOverlay.cs index d50768f43e..d83839fa2c 100644 --- a/osu.Game/Overlays/NotificationOverlay.cs +++ b/osu.Game/Overlays/NotificationOverlay.cs @@ -1,21 +1,22 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +#nullable enable + using System.Linq; -using JetBrains.Annotations; -using osu.Framework.Extensions.IEnumerableExtensions; -using osu.Framework.Graphics; -using osu.Framework.Graphics.Containers; -using osu.Game.Overlays.Notifications; -using osu.Framework.Graphics.Shapes; -using osu.Game.Graphics.Containers; using osu.Framework.Allocation; using osu.Framework.Audio; using osu.Framework.Bindables; +using osu.Framework.Extensions.IEnumerableExtensions; +using osu.Framework.Graphics; +using osu.Framework.Graphics.Containers; +using osu.Framework.Graphics.Shapes; using osu.Framework.Localisation; using osu.Framework.Logging; using osu.Framework.Threading; using osu.Game.Graphics; +using osu.Game.Graphics.Containers; +using osu.Game.Overlays.Notifications; using osu.Game.Resources.Localisation.Web; using NotificationsStrings = osu.Game.Localisation.NotificationsStrings; @@ -31,15 +32,15 @@ namespace osu.Game.Overlays public const float TRANSITION_LENGTH = 600; - private FlowContainer sections; + private FlowContainer sections = null!; [Resolved] - private AudioManager audio { get; set; } + private AudioManager audio { get; set; } = null!; private readonly IBindable firstRunSetupVisibility = new Bindable(); - [BackgroundDependencyLoader(true)] - private void load([CanBeNull] FirstRunSetupOverlay firstRunSetup) + [BackgroundDependencyLoader] + private void load(FirstRunSetupOverlay? firstRunSetup) { X = WIDTH; Width = WIDTH; @@ -83,11 +84,11 @@ namespace osu.Game.Overlays firstRunSetupVisibility.BindTo(firstRunSetup.State); } - private ScheduledDelegate notificationsEnabler; + private ScheduledDelegate? notificationsEnabler; private void updateProcessingMode() { - bool enabled = (OverlayActivationMode.Value == OverlayActivation.All && firstRunSetupVisibility?.Value != Visibility.Visible) || State.Value == Visibility.Visible; + bool enabled = (OverlayActivationMode.Value == OverlayActivation.All && firstRunSetupVisibility.Value != Visibility.Visible) || State.Value == Visibility.Visible; notificationsEnabler?.Cancel();