Merge pull request #19783 from bdach/mod-select/presets-dont-open-customisation

Fix selecting preset containing Difficulty Adjust automatically opening customisation panel

osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs

@@ -1,14 +1,13 @@
 // 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.Collections.Generic;
 using System.Linq;
 using NUnit.Framework;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
+using osu.Framework.Extensions.ObjectExtensions;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
 using osu.Framework.Testing;
@@ -29,10 +28,18 @@ namespace osu.Game.Tests.Visual.UserInterface
     [TestFixture]
     public class TestSceneModSelectOverlay : OsuManualInputManagerTestScene
     {
-        [Resolved]
+        protected override bool UseFreshStoragePerRun => true;
-        private RulesetStore rulesetStore { get; set; }
 
-        private UserModSelectOverlay modSelectOverlay;
+        private RulesetStore rulesetStore = null!;
+
+        private TestModSelectOverlay modSelectOverlay = null!;
+
+        [BackgroundDependencyLoader]
+        private void load()
+        {
+            Dependencies.Cache(rulesetStore = new RealmRulesetStore(Realm));
+            Dependencies.Cache(Realm);
+        }
 
         [SetUpSteps]
         public void SetUpSteps()
@@ -40,11 +47,31 @@ namespace osu.Game.Tests.Visual.UserInterface
             AddStep("clear contents", Clear);
             AddStep("reset ruleset", () => Ruleset.Value = rulesetStore.GetRuleset(0));
             AddStep("reset mods", () => SelectedMods.SetDefault());
+            AddStep("set up presets", () =>
+            {
+                Realm.Write(r =>
+                {
+                    r.RemoveAll<ModPreset>();
+                    r.Add(new ModPreset
+                    {
+                        Name = "AR0",
+                        Description = "Too... many... circles...",
+                        Ruleset = r.Find<RulesetInfo>(OsuRuleset.SHORT_NAME),
+                        Mods = new[]
+                        {
+                            new OsuModDifficultyAdjust
+                            {
+                                ApproachRate = { Value = 0 }
+                            }
+                        }
+                    });
+                });
+            });
         }
 
         private void createScreen()
         {
-            AddStep("create screen", () => Child = modSelectOverlay = new UserModSelectOverlay
+            AddStep("create screen", () => Child = modSelectOverlay = new TestModSelectOverlay
             {
                 RelativeSizeAxes = Axes.Both,
                 State = { Value = Visibility.Visible },
@@ -137,7 +164,7 @@ namespace osu.Game.Tests.Visual.UserInterface
 
             AddUntilStep("any column dimmed", () => this.ChildrenOfType<ModColumn>().Any(column => !column.Active.Value));
 
-            ModSelectColumn lastColumn = null;
+            ModSelectColumn lastColumn = null!;
 
             AddAssert("last column dimmed", () => !this.ChildrenOfType<ModColumn>().Last().Active.Value);
             AddStep("request scroll to last column", () =>
@@ -165,18 +192,22 @@ namespace osu.Game.Tests.Visual.UserInterface
             AddStep("select customisable mod", () => SelectedMods.Value = new[] { new OsuModDoubleTime() });
             assertCustomisationToggleState(disabled: false, active: false);
 
-            AddStep("select mod requiring configuration", () => SelectedMods.Value = new[] { new OsuModDifficultyAdjust() });
+            AddStep("select mod requiring configuration externally", () => SelectedMods.Value = new[] { new OsuModDifficultyAdjust() });
+            assertCustomisationToggleState(disabled: false, active: false);
+
+            AddStep("reset mods", () => SelectedMods.SetDefault());
+            AddStep("select difficulty adjust via panel", () => getPanelForMod(typeof(OsuModDifficultyAdjust)).TriggerClick());
             assertCustomisationToggleState(disabled: false, active: true);
 
             AddStep("dismiss mod customisation via toggle", () =>
             {
-                InputManager.MoveMouseTo(modSelectOverlay.ChildrenOfType<ShearedToggleButton>().Single());
+                InputManager.MoveMouseTo(modSelectOverlay.CustomisationButton);
                 InputManager.Click(MouseButton.Left);
             });
             assertCustomisationToggleState(disabled: false, active: false);
 
             AddStep("reset mods", () => SelectedMods.SetDefault());
-            AddStep("select mod requiring configuration", () => SelectedMods.Value = new[] { new OsuModDifficultyAdjust() });
+            AddStep("select difficulty adjust via panel", () => getPanelForMod(typeof(OsuModDifficultyAdjust)).TriggerClick());
             assertCustomisationToggleState(disabled: false, active: true);
 
             AddStep("dismiss mod customisation via keyboard", () => InputManager.Key(Key.Escape));
@@ -188,11 +219,18 @@ namespace osu.Game.Tests.Visual.UserInterface
             AddStep("select mod without configuration", () => SelectedMods.Value = new[] { new OsuModAutoplay() });
             assertCustomisationToggleState(disabled: true, active: false);
 
-            AddStep("select mod requiring configuration", () => SelectedMods.Value = new[] { new OsuModDifficultyAdjust() });
+            AddStep("select difficulty adjust via panel", () => getPanelForMod(typeof(OsuModDifficultyAdjust)).TriggerClick());
             assertCustomisationToggleState(disabled: false, active: true);
 
             AddStep("select mod without configuration", () => SelectedMods.Value = new[] { new OsuModAutoplay() });
             assertCustomisationToggleState(disabled: true, active: false); // config was dismissed without explicit user action.
+
+            AddStep("select mod preset with mod requiring configuration", () =>
+            {
+                InputManager.MoveMouseTo(this.ChildrenOfType<ModPresetPanel>().First());
+                InputManager.Click(MouseButton.Left);
+            });
+            assertCustomisationToggleState(disabled: false, active: false);
         }
 
         [Test]
@@ -201,7 +239,7 @@ namespace osu.Game.Tests.Visual.UserInterface
             createScreen();
             assertCustomisationToggleState(disabled: true, active: false);
 
-            AddStep("select mod requiring configuration", () => SelectedMods.Value = new[] { new OsuModDifficultyAdjust() });
+            AddStep("select difficulty adjust via panel", () => getPanelForMod(typeof(OsuModDifficultyAdjust)).TriggerClick());
             assertCustomisationToggleState(disabled: false, active: true);
 
             AddStep("move mouse to settings area", () => InputManager.MoveMouseTo(this.ChildrenOfType<ModSettingsArea>().Single()));
@@ -224,11 +262,11 @@ namespace osu.Game.Tests.Visual.UserInterface
         [Test]
         public void TestSettingsNotCrossPolluting()
         {
-            Bindable<IReadOnlyList<Mod>> selectedMods2 = null;
+            Bindable<IReadOnlyList<Mod>> selectedMods2 = null!;
-            ModSelectOverlay modSelectOverlay2 = null;
+            ModSelectOverlay modSelectOverlay2 = null!;
 
             createScreen();
-            AddStep("select diff adjust", () => SelectedMods.Value = new Mod[] { new OsuModDifficultyAdjust() });
+            AddStep("select difficulty adjust via panel", () => getPanelForMod(typeof(OsuModDifficultyAdjust)).TriggerClick());
 
             AddStep("set setting", () => modSelectOverlay.ChildrenOfType<OsuSliderBar<float>>().First().Current.Value = 8);
 
@@ -353,7 +391,7 @@ namespace osu.Game.Tests.Visual.UserInterface
         public void TestExternallySetModIsReplacedByOverlayInstance()
         {
             Mod external = new OsuModDoubleTime();
-            Mod overlayButtonMod = null;
+            Mod overlayButtonMod = null!;
 
             createScreen();
             changeRuleset(0);
@@ -458,14 +496,14 @@ namespace osu.Game.Tests.Visual.UserInterface
             createScreen();
             changeRuleset(0);
 
-            AddStep("select difficulty adjust", () => SelectedMods.Value = new Mod[] { new OsuModDifficultyAdjust() });
+            AddStep("select difficulty adjust via panel", () => getPanelForMod(typeof(OsuModDifficultyAdjust)).TriggerClick());
             assertCustomisationToggleState(disabled: false, active: true);
-            AddAssert("back button disabled", () => !this.ChildrenOfType<ShearedButton>().First().Enabled.Value);
+            AddAssert("back button disabled", () => !modSelectOverlay.BackButton.Enabled.Value);
 
             AddStep("dismiss customisation area", () => InputManager.Key(Key.Escape));
             AddStep("click back button", () =>
             {
-                InputManager.MoveMouseTo(this.ChildrenOfType<ShearedButton>().First());
+                InputManager.MoveMouseTo(modSelectOverlay.BackButton);
                 InputManager.Click(MouseButton.Left);
             });
             AddAssert("mod select hidden", () => modSelectOverlay.State.Value == Visibility.Hidden);
@@ -474,7 +512,7 @@ namespace osu.Game.Tests.Visual.UserInterface
         [Test]
         public void TestColumnHiding()
         {
-            AddStep("create screen", () => Child = modSelectOverlay = new UserModSelectOverlay
+            AddStep("create screen", () => Child = modSelectOverlay = new TestModSelectOverlay
             {
                 RelativeSizeAxes = Axes.Both,
                 State = { Value = Visibility.Visible },
@@ -527,15 +565,21 @@ namespace osu.Game.Tests.Visual.UserInterface
 
         private void assertCustomisationToggleState(bool disabled, bool active)
         {
-            ShearedToggleButton getToggle() => modSelectOverlay.ChildrenOfType<ShearedToggleButton>().Single();
+            AddAssert($"customisation toggle is {(disabled ? "" : "not ")}disabled", () => modSelectOverlay.CustomisationButton.AsNonNull().Active.Disabled == disabled);
-
+            AddAssert($"customisation toggle is {(active ? "" : "not ")}active", () => modSelectOverlay.CustomisationButton.AsNonNull().Active.Value == active);
-            AddAssert($"customisation toggle is {(disabled ? "" : "not ")}disabled", () => getToggle().Active.Disabled == disabled);
-            AddAssert($"customisation toggle is {(active ? "" : "not ")}active", () => getToggle().Active.Value == active);
         }
 
         private ModPanel getPanelForMod(Type modType)
             => modSelectOverlay.ChildrenOfType<ModPanel>().Single(panel => panel.Mod.GetType() == modType);
 
+        private class TestModSelectOverlay : UserModSelectOverlay
+        {
+            protected override bool ShowPresets => true;
+
+            public new ShearedButton BackButton => base.BackButton;
+            public new ShearedToggleButton? CustomisationButton => base.CustomisationButton;
+        }
+
         private class TestUnimplementedMod : Mod
         {
             public override string Name => "Unimplemented mod";

osu.Game/Overlays/Mods/ModColumn.cs

@@ -66,7 +66,10 @@ namespace osu.Game.Overlays.Mods
         private IModHotkeyHandler hotkeyHandler = null!;
 
         private Task? latestLoadTask;
-        internal bool ItemsLoaded => latestLoadTask?.IsCompleted == true;
+        private ICollection<ModPanel>? latestLoadedPanels;
+        internal bool ItemsLoaded => latestLoadTask?.IsCompleted == true && latestLoadedPanels?.All(panel => panel.Parent != null) == true;
+
+        public override bool IsPresent => base.IsPresent || Scheduler.HasPendingTasks;
 
         public ModColumn(ModType modType, bool allowIncompatibleSelection)
         {
@@ -130,7 +133,8 @@ namespace osu.Game.Overlays.Mods
         {
             cancellationTokenSource?.Cancel();
 
-            var panels = availableMods.Select(mod => CreateModPanel(mod).With(panel => panel.Shear = Vector2.Zero));
+            var panels = availableMods.Select(mod => CreateModPanel(mod).With(panel => panel.Shear = Vector2.Zero)).ToArray();
+            latestLoadedPanels = panels;
 
             latestLoadTask = LoadComponentsAsync(panels, loaded =>
             {

osu.Game/Overlays/Mods/ModPanel.cs

@@ -57,6 +57,18 @@ namespace osu.Game.Overlays.Mods
             Filtered.BindValueChanged(_ => updateFilterState(), true);
         }
 
+        protected override void Select()
+        {
+            modState.PendingConfiguration = Mod.RequiresConfiguration;
+            Active.Value = true;
+        }
+
+        protected override void Deselect()
+        {
+            modState.PendingConfiguration = false;
+            Active.Value = false;
+        }
+
         #region Filtering support
 
         private void updateFilterState()

osu.Game/Overlays/Mods/ModPresetPanel.cs

@@ -37,8 +37,6 @@ namespace osu.Game.Overlays.Mods
 
             Title = preset.Value.Name;
             Description = preset.Value.Description;
-
-            Action = toggleRequestedByUser;
         }
 
         [BackgroundDependencyLoader]
@@ -54,15 +52,19 @@ namespace osu.Game.Overlays.Mods
             selectedMods.BindValueChanged(_ => selectedModsChanged(), true);
         }
 
-        private void toggleRequestedByUser()
+        protected override void Select()
+        {
+            // if the preset is not active at the point of the user click, then set the mods using the preset directly, discarding any previous selections,
+            // which will also have the side effect of activating the preset (see `updateActiveState()`).
+            selectedMods.Value = Preset.Value.Mods.ToArray();
+        }
+
+        protected override void Deselect()
         {
-            // if the preset is not active at the point of the user click, then set the mods using the preset directly, discarding any previous selections.
             // if the preset is active when the user has clicked it, then it means that the set of active mods is exactly equal to the set of mods in the preset
             // (there are no other active mods than what the preset specifies, and the mod settings match exactly).
             // therefore it's safe to just clear selected mods, since it will have the effect of toggling the preset off.
-            selectedMods.Value = !Active.Value
+            selectedMods.Value = Array.Empty<Mod>();
-                ? Preset.Value.Mods.ToArray()
-                : Array.Empty<Mod>();
         }
 
         private void selectedModsChanged()

osu.Game/Overlays/Mods/ModSelectOverlay.cs

@@ -87,7 +87,7 @@ namespace osu.Game.Overlays.Mods
         {
             if (AllowCustomisation)
             {
-                yield return customisationButton = new ShearedToggleButton(BUTTON_WIDTH)
+                yield return CustomisationButton = new ShearedToggleButton(BUTTON_WIDTH)
                 {
                     Text = ModSelectOverlayStrings.ModCustomisation,
                     Active = { BindTarget = customisationVisible }
@@ -107,11 +107,11 @@ namespace osu.Game.Overlays.Mods
         private ColumnScrollContainer columnScroll = null!;
         private ColumnFlowContainer columnFlow = null!;
         private FillFlowContainer<ShearedButton> footerButtonFlow = null!;
-        private ShearedButton backButton = null!;
 
         private DifficultyMultiplierDisplay? multiplierDisplay;
 
-        private ShearedToggleButton? customisationButton;
+        protected ShearedButton BackButton { get; private set; } = null!;
+        protected ShearedToggleButton? CustomisationButton { get; private set; }
 
         private Sample? columnAppearSample;
 
@@ -214,7 +214,7 @@ namespace osu.Game.Overlays.Mods
                     Horizontal = 70
                 },
                 Spacing = new Vector2(10),
-                ChildrenEnumerable = CreateFooterButtons().Prepend(backButton = new ShearedButton(BUTTON_WIDTH)
+                ChildrenEnumerable = CreateFooterButtons().Prepend(BackButton = new ShearedButton(BUTTON_WIDTH)
                 {
                     Text = CommonStrings.Back,
                     Action = Hide,
@@ -247,8 +247,8 @@ namespace osu.Game.Overlays.Mods
                 modSettingChangeTracker?.Dispose();
 
                 updateMultiplier();
-                updateCustomisation(val);
                 updateFromExternalSelection();
+                updateCustomisation();
 
                 if (AllowCustomisation)
                 {
@@ -356,25 +356,26 @@ namespace osu.Game.Overlays.Mods
             multiplierDisplay.Current.Value = multiplier;
         }
 
-        private void updateCustomisation(ValueChangedEvent<IReadOnlyList<Mod>> valueChangedEvent)
+        private void updateCustomisation()
         {
-            if (customisationButton == null)
+            if (CustomisationButton == null)
                 return;
 
-            bool anyCustomisableMod = false;
+            bool anyCustomisableModActive = false;
-            bool anyModWithRequiredCustomisationAdded = false;
+            bool anyModPendingConfiguration = false;
 
-            foreach (var mod in SelectedMods.Value)
+            foreach (var modState in allAvailableMods)
             {
-                anyCustomisableMod |= mod.GetSettingsSourceProperties().Any();
+                anyCustomisableModActive |= modState.Active.Value && modState.Mod.GetSettingsSourceProperties().Any();
-                anyModWithRequiredCustomisationAdded |= valueChangedEvent.OldValue.All(m => m.GetType() != mod.GetType()) && mod.RequiresConfiguration;
+                anyModPendingConfiguration |= modState.PendingConfiguration;
+                modState.PendingConfiguration = false;
             }
 
-            if (anyCustomisableMod)
+            if (anyCustomisableModActive)
             {
                 customisationVisible.Disabled = false;
 
-                if (anyModWithRequiredCustomisationAdded && !customisationVisible.Value)
+                if (anyModPendingConfiguration && !customisationVisible.Value)
                     customisationVisible.Value = true;
             }
             else
@@ -394,7 +395,7 @@ namespace osu.Game.Overlays.Mods
 
             foreach (var button in footerButtonFlow)
             {
-                if (button != customisationButton)
+                if (button != CustomisationButton)
                     button.Enabled.Value = !customisationVisible.Value;
             }
 
@@ -587,14 +588,14 @@ namespace osu.Game.Overlays.Mods
             {
                 if (customisationVisible.Value)
                 {
-                    Debug.Assert(customisationButton != null);
+                    Debug.Assert(CustomisationButton != null);
-                    customisationButton.TriggerClick();
+                    CustomisationButton.TriggerClick();
 
                     if (!immediate)
                         return;
                 }
 
-                backButton.TriggerClick();
+                BackButton.TriggerClick();
             }
         }
 
@@ -708,7 +709,18 @@ namespace osu.Game.Overlays.Mods
                 FinishTransforms();
             }
 
-            protected override bool RequiresChildrenUpdate => base.RequiresChildrenUpdate || (Column as ModColumn)?.SelectionAnimationRunning == true;
+            protected override bool RequiresChildrenUpdate
+            {
+                get
+                {
+                    bool result = base.RequiresChildrenUpdate;
+
+                    if (Column is ModColumn modColumn)
+                        result |= !modColumn.ItemsLoaded || modColumn.SelectionAnimationRunning;
+
+                    return result;
+                }
+            }
 
             private void updateState()
             {

osu.Game/Overlays/Mods/ModSelectPanel.cs

@@ -143,9 +143,25 @@ namespace osu.Game.Overlays.Mods
                 }
             };
 
-            Action = () => Active.Toggle();
+            Action = () =>
+            {
+                if (!Active.Value)
+                    Select();
+                else
+                    Deselect();
+            };
         }
 
+        /// <summary>
+        /// Performs all actions necessary to select this <see cref="ModSelectPanel"/>.
+        /// </summary>
+        protected abstract void Select();
+
+        /// <summary>
+        /// Performs all actions necessary to deselect this <see cref="ModSelectPanel"/>.
+        /// </summary>
+        protected abstract void Deselect();
+
         [BackgroundDependencyLoader]
         private void load(AudioManager audio, ISamplePlaybackDisabler? samplePlaybackDisabler)
         {

osu.Game/Overlays/Mods/ModState.cs

@@ -24,6 +24,13 @@ namespace osu.Game.Overlays.Mods
         /// </summary>
         public BindableBool Active { get; } = new BindableBool();
 
+        /// <summary>
+        /// Whether the mod requires further customisation.
+        /// This flag is read by the <see cref="ModSelectOverlay"/> to determine if the customisation panel should be opened after a mod change
+        /// and cleared after reading.
+        /// </summary>
+        public bool PendingConfiguration { get; set; }
+
         /// <summary>
         /// Whether the mod is currently filtered out due to not matching imposed criteria.
         /// </summary>

osu.Game/Screens/Select/SongSelect.cs

@@ -928,7 +928,7 @@ namespace osu.Game.Screens.Select
             }
         }
 
-        private class SoloModSelectOverlay : UserModSelectOverlay
+        internal class SoloModSelectOverlay : UserModSelectOverlay
         {
             protected override bool ShowPresets => true;
         }