Implement new settings item component

[frenzibyte]

• Depends on Add osu!-style undo icon osu-resources#398

First step towards #36040, and a solution to a long-standing issue with the code quality of the existing SettingsItem and its entire hierarchy of subclasses.

Opening notes:

• This PR only implements the new component, it does not use it anywhere in the game yet.
• Adjustments to the form control visuals and UX will be done in a separate PR.

New version

The new version SettingsItemV2 utilizes composition (as proposed by @bdach and @peppy) to wrap the UI control in a component that offers a revert-to-default button and filtering functionality. The class is sealed given its design.

Example usage:

new SettingsItemV2(new FormSliderBar<float>
{
    Caption = "Some slider setting",
    HintText = "Some slider setting hint",
    Current = someBindable,
    KeyboardStep = 0.1f
})
{
    Note = { BindTarget = someNote },
    Keywords = new[] { "some", "slider", "setting" },
    //ShowDefaultRevertButton = true,
    //HasClassicDefault = true,
};

...

someNote.Value = new SettingsNote.Data("some note", SettingsNote.Type.Warning);
someNote.Value = null;

Drawbacks

There are few drawbacks worth mentioning from this approach, one which has already been discussed, and another I kept until opening this PR so it's easier to discuss it code-wise.

SettingsItemV2 cannot be aware of any bindable

This necessitated the presence of a non-generic IFormControl interface, implemented across all form controls, exposing basic bindable interaction API for SettingsItemV2 to utilize.

This has been discussed already and we've agreed to push forward with the interface above. I'm hoping for a day where o!f bindables directly support non-generic interaction, but I don't expect that happening any time soon due to the core idea of IBindable interfaces being readonly (making a function like SetDefault violate rules if placed there).

SettingsItemV2 cannot store "classic" default values

Due to SettingsItemV2 not being generic and not support applying arbitrary values to the underlying wrapped control, this feature is impossible to implement within SettingsItemV2.

My initial approach was to take this feature completely away from SettingsItemV2 and into where each settings item is instantiated (i.e. SettingsSubsection). However, part of this feature is filtering all settings based on those who have a classic default value set, therefore SettingsItemV2 must have a HasClassicDefault flag present in order to feed it to its filter terms.

I've sat around for a bit thinking about any way to approach this, but no bright ideas came to mind. So I've bit the bullet and kept the flag, expecting each settings declaration in SettingsSubsection to set the flag, then override a new ApplyClassicDefaults method to overwrite all relevant settings in the subsection to their classic defaults.

Need feedback on this. I would ideally want an approach which takes HasClassicDefault away from SettingsItemV2 altogether.

This has been addressed by the suggestion in #36055 (comment).

Sample implementation

Lastly, to give a full idea of how SettingsItemV2 would transform the coding structure of existing settings, below is the diff of LayoutSettings alone, the heaviest settings subsection I found code-wise:

Sample implementation

diff --git a/osu.Game/Overlays/Settings/Sections/Graphics/LayoutSettings.cs b/osu.Game/Overlays/Settings/Sections/Graphics/LayoutSettings.cs
index cdc4f328c3..df6a1e76b0 100644
--- a/osu.Game/Overlays/Settings/Sections/Graphics/LayoutSettings.cs
+++ b/osu.Game/Overlays/Settings/Sections/Graphics/LayoutSettings.cs
@@ -18,7 +18,7 @@
 using osu.Framework.Platform.Windows;
 using osu.Game.Configuration;
 using osu.Game.Graphics.Containers;
-using osu.Game.Graphics.UserInterface;
+using osu.Game.Graphics.UserInterfaceV2;
 using osu.Game.Localisation;
 using osuTK;
 using osuTK.Graphics;
@@ -29,8 +29,8 @@ public partial class LayoutSettings : SettingsSubsection
     {
         protected override LocalisableString Header => GraphicsSettingsStrings.LayoutHeader;
 
-        private FillFlowContainer<SettingsSlider<float>> scalingSettings = null!;
-        private SettingsSlider<float> dimSlider = null!;
+        private FillFlowContainer<SettingsItemV2> scalingSettings = null!;
+        private SettingsItemV2 dimSliderSetting = null!;
 
         private readonly Bindable<Display> currentDisplay = new Bindable<Display>();
 
@@ -51,12 +51,17 @@ public partial class LayoutSettings : SettingsSubsection
 
         private IWindow? window;
 
-        private SettingsDropdown<Size> resolutionFullscreenDropdown = null!;
-        private SettingsDropdown<Size> resolutionWindowedDropdown = null!;
-        private SettingsDropdown<Display> displayDropdown = null!;
-        private SettingsDropdown<WindowMode> windowModeDropdown = null!;
-        private SettingsCheckbox minimiseOnFocusLossCheckbox = null!;
-        private SettingsCheckbox safeAreaConsiderationsCheckbox = null!;
+        private readonly BindableBool resolutionFullscreenCanBeShown = new BindableBool(true);
+        private readonly BindableBool resolutionWindowedCanBeShown = new BindableBool(true);
+        private readonly BindableBool displayDropdownCanBeShown = new BindableBool(true);
+        private readonly BindableBool minimiseOnFocusLossCanBeShown = new BindableBool(true);
+        private readonly BindableBool safeAreaConsiderationsCanBeShown = new BindableBool(true);
+
+        private FormDropdown<Size> resolutionWindowedDropdown = null!;
+        private FormDropdown<Display> displayDropdown = null!;
+        private FormDropdown<WindowMode> windowModeDropdown = null!;
+
+        private readonly Bindable<SettingsNote.Data?> windowModeDropdownNote = new Bindable<SettingsNote.Data?>();
 
         private Bindable<double> windowedPositionX = null!;
         private Bindable<double> windowedPositionY = null!;
@@ -98,105 +103,141 @@ private void load(FrameworkConfigManager config, OsuConfigManager osuConfig, Gam
 
             Children = new Drawable[]
             {
-                windowModeDropdown = new SettingsDropdown<WindowMode>
+                new SettingsItemV2(windowModeDropdown = new FormDropdown<WindowMode>
                 {
-                    LabelText = GraphicsSettingsStrings.ScreenMode,
+                    Caption = GraphicsSettingsStrings.ScreenMode,
                     Items = window?.SupportedWindowModes,
-                    CanBeShown = { Value = window?.SupportedWindowModes.Count() > 1 },
                     Current = config.GetBindable<WindowMode>(FrameworkSetting.WindowMode),
+                })
+                {
+                    CanBeShown = { Value = window?.SupportedWindowModes.Count() > 1 },
+                    Note = { BindTarget = windowModeDropdownNote },
                 },
-                displayDropdown = new DisplaySettingsDropdown
+                new SettingsItemV2(displayDropdown = new DisplayDropdown
                 {
-                    LabelText = GraphicsSettingsStrings.Display,
+                    Caption = GraphicsSettingsStrings.Display,
                     Items = window?.Displays,
                     Current = currentDisplay,
+                })
+                {
+                    CanBeShown = { BindTarget = displayDropdownCanBeShown }
                 },
-                resolutionFullscreenDropdown = new ResolutionSettingsDropdown
+                new SettingsItemV2(new ResolutionDropdown
                 {
-                    LabelText = GraphicsSettingsStrings.Resolution,
-                    ShowsDefaultIndicator = false,
+                    Caption = GraphicsSettingsStrings.Resolution,
                     ItemSource = resolutionsFullscreen,
                     Current = sizeFullscreen
+                })
+                {
+                    CanBeShown = { BindTarget = resolutionFullscreenCanBeShown },
+                    ShowDefaultRevertButton = false,
                 },
-                resolutionWindowedDropdown = new ResolutionSettingsDropdown
+                new SettingsItemV2(resolutionWindowedDropdown = new ResolutionDropdown
                 {
-                    LabelText = GraphicsSettingsStrings.Resolution,
-                    ShowsDefaultIndicator = false,
+                    Caption = GraphicsSettingsStrings.Resolution,
                     ItemSource = resolutionsWindowed,
                     Current = windowedResolution
+                })
+                {
+                    CanBeShown = { BindTarget = resolutionWindowedCanBeShown },
+                    ShowDefaultRevertButton = false,
                 },
-                minimiseOnFocusLossCheckbox = new SettingsCheckbox
+                new SettingsItemV2(new FormCheckBox
                 {
-                    LabelText = GraphicsSettingsStrings.MinimiseOnFocusLoss,
+                    Caption = GraphicsSettingsStrings.MinimiseOnFocusLoss,
                     Current = config.GetBindable<bool>(FrameworkSetting.MinimiseOnFocusLossInFullscreen),
+                })
+                {
+                    CanBeShown = { BindTarget = minimiseOnFocusLossCanBeShown },
                     Keywords = new[] { "alt-tab", "minimize", "focus", "hide" },
                 },
-                safeAreaConsiderationsCheckbox = new SettingsCheckbox
+                new SettingsItemV2(new FormCheckBox
                 {
-                    LabelText = GraphicsSettingsStrings.ShrinkGameToSafeArea,
+                    Caption = GraphicsSettingsStrings.ShrinkGameToSafeArea,
                     Current = osuConfig.GetBindable<bool>(OsuSetting.SafeAreaConsiderations),
+                })
+                {
+                    CanBeShown = { BindTarget = safeAreaConsiderationsCanBeShown },
                 },
-                new SettingsSlider<float, UIScaleSlider>
+                new SettingsItemV2(new FormSliderBar<float>
                 {
-                    LabelText = GraphicsSettingsStrings.UIScaling,
+                    Caption = GraphicsSettingsStrings.UIScaling,
                     TransferValueOnCommit = true,
                     Current = osuConfig.GetBindable<float>(OsuSetting.UIScale),
                     KeyboardStep = 0.01f,
+                    // LabelFormat = v => $@"{v:0.##}x",
+                })
+                {
                     Keywords = new[] { "scale", "letterbox" },
                 },
-                new SettingsEnumDropdown<ScalingMode>
+                new SettingsItemV2(new FormEnumDropdown<ScalingMode>
                 {
-                    LabelText = GraphicsSettingsStrings.ScreenScaling,
+                    Caption = GraphicsSettingsStrings.ScreenScaling,
                     Current = osuConfig.GetBindable<ScalingMode>(OsuSetting.Scaling),
+                })
+                {
                     Keywords = new[] { "scale", "letterbox" },
                 },
-                scalingSettings = new FillFlowContainer<SettingsSlider<float>>
+                scalingSettings = new FillFlowContainer<SettingsItemV2>
                 {
                     Direction = FillDirection.Vertical,
                     RelativeSizeAxes = Axes.X,
                     AutoSizeAxes = Axes.Y,
                     Masking = true,
+                    Spacing = new Vector2(0, SettingsSection.ITEM_SPACING),
                     Children = new[]
                     {
-                        new SettingsSlider<float>
+                        new SettingsItemV2(new FormSliderBar<float>
                         {
-                            LabelText = GraphicsSettingsStrings.HorizontalPosition,
-                            Keywords = new[] { "screen", "scaling" },
+                            Caption = GraphicsSettingsStrings.HorizontalPosition,
                             Current = scalingPositionX,
                             KeyboardStep = 0.01f,
-                            DisplayAsPercentage = true
-                        },
-                        new SettingsSlider<float>
+                            // DisplayAsPercentage = true,
+                            TransferValueOnCommit = scalingMode.Value == ScalingMode.Everything,
+                        }.With(bindPreviewEvent))
                         {
-                            LabelText = GraphicsSettingsStrings.VerticalPosition,
                             Keywords = new[] { "screen", "scaling" },
+                        },
+                        new SettingsItemV2(new FormSliderBar<float>
+                        {
+                            Caption = GraphicsSettingsStrings.VerticalPosition,
                             Current = scalingPositionY,
                             KeyboardStep = 0.01f,
-                            DisplayAsPercentage = true
-                        },
-                        new SettingsSlider<float>
+                            // DisplayAsPercentage = true,
+                            TransferValueOnCommit = scalingMode.Value == ScalingMode.Everything,
+                        }.With(bindPreviewEvent))
                         {
-                            LabelText = GraphicsSettingsStrings.HorizontalScale,
                             Keywords = new[] { "screen", "scaling" },
+                        },
+                        new SettingsItemV2(new FormSliderBar<float>
+                        {
+                            Caption = GraphicsSettingsStrings.HorizontalScale,
                             Current = scalingSizeX,
                             KeyboardStep = 0.01f,
-                            DisplayAsPercentage = true
-                        },
-                        new SettingsSlider<float>
+                            // DisplayAsPercentage = true,
+                            TransferValueOnCommit = scalingMode.Value == ScalingMode.Everything,
+                        }.With(bindPreviewEvent))
                         {
-                            LabelText = GraphicsSettingsStrings.VerticalScale,
                             Keywords = new[] { "screen", "scaling" },
+                        },
+                        new SettingsItemV2(new FormSliderBar<float>
+                        {
+                            Caption = GraphicsSettingsStrings.VerticalScale,
                             Current = scalingSizeY,
                             KeyboardStep = 0.01f,
-                            DisplayAsPercentage = true
+                            // DisplayAsPercentage = true,
+                            TransferValueOnCommit = scalingMode.Value == ScalingMode.Everything,
+                        }.With(bindPreviewEvent))
+                        {
+                            Keywords = new[] { "screen", "scaling" },
                         },
-                        dimSlider = new SettingsSlider<float>
+                        dimSliderSetting = new SettingsItemV2(new FormSliderBar<float>
                         {
-                            LabelText = GameplaySettingsStrings.BackgroundDim,
+                            Caption = GameplaySettingsStrings.BackgroundDim,
                             Current = scalingBackgroundDim,
                             KeyboardStep = 0.01f,
-                            DisplayAsPercentage = true,
-                        },
+                            // DisplayAsPercentage = true,
+                        }.With(bindPreviewEvent)),
                     }
                 },
             };
@@ -208,8 +249,6 @@ protected override void LoadComplete()
         {
             base.LoadComplete();
 
-            scalingSettings.ForEach(s => bindPreviewEvent(s.Current));
-
             windowModeDropdown.Current.BindValueChanged(_ =>
             {
                 updateDisplaySettingsVisibility();
@@ -300,15 +339,10 @@ void updateScalingModeVisibility()
                 scalingSettings.AutoSizeAxes = scalingMode.Value != ScalingMode.Off ? Axes.Y : Axes.None;
                 scalingSettings.ForEach(s =>
                 {
-                    if (s == dimSlider)
-                    {
+                    if (s == dimSliderSetting)
                         s.CanBeShown.Value = scalingMode.Value == ScalingMode.Everything || scalingMode.Value == ScalingMode.ExcludeOverlays;
-                    }
                     else
-                    {
-                        s.TransferValueOnCommit = scalingMode.Value == ScalingMode.Everything;
                         s.CanBeShown.Value = scalingMode.Value != ScalingMode.Off;
-                    }
                 });
             }
         }
@@ -325,12 +359,12 @@ private void onDisplaysChanged(IEnumerable<Display> displays)
 
         private void updateDisplaySettingsVisibility()
         {
-            resolutionFullscreenDropdown.CanBeShown.Value = windowModeDropdown.Current.Value == WindowMode.Fullscreen && resolutionsFullscreen.Count > 1;
-            resolutionWindowedDropdown.CanBeShown.Value = windowModeDropdown.Current.Value == WindowMode.Windowed && resolutionsWindowed.Count > 1;
+            resolutionFullscreenCanBeShown.Value = windowModeDropdown.Current.Value == WindowMode.Fullscreen && resolutionsFullscreen.Count > 1;
+            resolutionWindowedCanBeShown.Value = windowModeDropdown.Current.Value == WindowMode.Windowed && resolutionsWindowed.Count > 1;
 
-            displayDropdown.CanBeShown.Value = displayDropdown.Items.Count() > 1;
-            minimiseOnFocusLossCheckbox.CanBeShown.Value = RuntimeInfo.IsDesktop && windowModeDropdown.Current.Value == WindowMode.Fullscreen;
-            safeAreaConsiderationsCheckbox.CanBeShown.Value = host.Window?.SafeAreaPadding.Value.Total != Vector2.Zero;
+            displayDropdownCanBeShown.Value = displayDropdown.Items.Count() > 1;
+            minimiseOnFocusLossCanBeShown.Value = RuntimeInfo.IsDesktop && windowModeDropdown.Current.Value == WindowMode.Fullscreen;
+            safeAreaConsiderationsCanBeShown.Value = host.Window?.SafeAreaPadding.Value.Total != Vector2.Zero;
         }
 
         private void updateScreenModeWarning()
@@ -339,16 +373,16 @@ private void updateScreenModeWarning()
             if (RuntimeInfo.OS == RuntimeInfo.Platform.macOS && !FrameworkEnvironment.UseSDL3)
             {
                 if (windowModeDropdown.Current.Value == WindowMode.Fullscreen)
-                    windowModeDropdown.SetNoticeText(LayoutSettingsStrings.FullscreenMacOSNote, true);
+                    windowModeDropdownNote.Value = new SettingsNote.Data(LayoutSettingsStrings.FullscreenMacOSNote, SettingsNote.Type.Warning);
                 else
-                    windowModeDropdown.ClearNoticeText();
+                    windowModeDropdownNote.Value = null;
 
                 return;
             }
 
             if (windowModeDropdown.Current.Value != WindowMode.Fullscreen)
             {
-                windowModeDropdown.SetNoticeText(GraphicsSettingsStrings.NotFullscreenNote, true);
+                windowModeDropdownNote.Value = new SettingsNote.Data(GraphicsSettingsStrings.NotFullscreenNote, SettingsNote.Type.Warning);
                 return;
             }
 
@@ -357,28 +391,28 @@ private void updateScreenModeWarning()
                 switch (fullscreenCapability.Value)
                 {
                     case FullscreenCapability.Unknown:
-                        windowModeDropdown.SetNoticeText(LayoutSettingsStrings.CheckingForFullscreenCapabilities, true);
+                        windowModeDropdownNote.Value = new SettingsNote.Data(LayoutSettingsStrings.CheckingForFullscreenCapabilities, SettingsNote.Type.Warning);
                         break;
 
                     case FullscreenCapability.Capable:
-                        windowModeDropdown.SetNoticeText(LayoutSettingsStrings.OsuIsRunningExclusiveFullscreen);
+                        windowModeDropdownNote.Value = new SettingsNote.Data(LayoutSettingsStrings.OsuIsRunningExclusiveFullscreen, SettingsNote.Type.Informational);
                         break;
 
                     case FullscreenCapability.Incapable:
-                        windowModeDropdown.SetNoticeText(LayoutSettingsStrings.UnableToRunExclusiveFullscreen, true);
+                        windowModeDropdownNote.Value = new SettingsNote.Data(LayoutSettingsStrings.UnableToRunExclusiveFullscreen, SettingsNote.Type.Warning);
                         break;
                 }
             }
             else
             {
                 // We can only detect exclusive fullscreen status on windows currently.
-                windowModeDropdown.ClearNoticeText();
+                windowModeDropdownNote.Value = null;
             }
         }
 
-        private void bindPreviewEvent(Bindable<float> bindable)
+        private void bindPreviewEvent(FormSliderBar<float> slider)
         {
-            bindable.ValueChanged += _ =>
+            slider.Current.ValueChanged += _ =>
             {
                 switch (scalingMode.Value)
                 {
@@ -421,37 +455,22 @@ public ScalingPreview()
             }
         }
 
-        private partial class UIScaleSlider : RoundedSliderBar<float>
+        private partial class DisplayDropdown : FormDropdown<Display>
         {
-            public override LocalisableString TooltipText => base.TooltipText + "x";
-        }
-
-        private partial class DisplaySettingsDropdown : SettingsDropdown<Display>
-        {
-            protected override OsuDropdown<Display> CreateDropdown() => new DisplaySettingsDropdownControl();
-
-            private partial class DisplaySettingsDropdownControl : DropdownControl
+            protected override LocalisableString GenerateItemText(Display item)
             {
-                protected override LocalisableString GenerateItemText(Display item)
-                {
-                    return $"{item.Index}: {item.Name} ({item.Bounds.Width}x{item.Bounds.Height})";
-                }
+                return $"{item.Index}: {item.Name} ({item.Bounds.Width}x{item.Bounds.Height})";
             }
         }
 
-        private partial class ResolutionSettingsDropdown : SettingsDropdown<Size>
+        private partial class ResolutionDropdown : FormDropdown<Size>
         {
-            protected override OsuDropdown<Size> CreateDropdown() => new ResolutionDropdownControl();
-
-            private partial class ResolutionDropdownControl : DropdownControl
+            protected override LocalisableString GenerateItemText(Size item)
             {
-                protected override LocalisableString GenerateItemText(Size item)
-                {
-                    if (item == new Size(9999, 9999))
-                        return CommonStrings.Default;
+                if (item == new Size(9999, 9999))
+                    return CommonStrings.Default;
 
-                    return $"{item.Width}x{item.Height}";
-                }
+                return $"{item.Width}x{item.Height}";
             }
         }
 

[bdach]

My initial approach was to take this feature completely away from SettingsItemV2 and into where each settings item is instantiated (i.e. SettingsSubsection). However, part of this feature is filtering all settings based on those who have a classic default value set, therefore SettingsItemV2 must have a HasClassicDefault flag present in order to feed it to its filter terms.

I've sat around for a bit thinking about any way to approach this, but no bright ideas came to mind. So I've bit the bullet and kept the flag, expecting each settings declaration in SettingsSubsection to set the flag, then override a new ApplyClassicDefaults method to overwrite all relevant settings in the subsection to their classic defaults.

Can SettingsItemV2 not have a Func? ApplyClassicDefault { set; } property?

• if the value is not null then there is a classic default, therefore obviating HasClassicDefault existing separately
• you would declaratively set the classic default as required where needed

SettingsItem.ClassicDefault doesn't even have a get; accessor so I can't immediately see why this wouldn't work.

It'd be a solution that: is plainly declarative, resides next to the control you'd expect it to reside in (and not its subsection for weird reasons), involves no virtual magic.

[frenzibyte]

Can SettingsItemV2 not have a Func? ApplyClassicDefault { set; } property?

• if the value is not null then there is a classic default, therefore obviating HasClassicDefault existing separately
• you would declaratively set the classic default as required where needed

SettingsItem.ClassicDefault doesn't even have a get; accessor so I can't immediately see why this wouldn't work.

It'd be a solution that: is plainly declarative, resides next to the control you'd expect it to reside in (and not its subsection for weird reasons), involves no virtual magic.

Nice idea, seems to work perfectly in my mind as well. Have applied.

[peppy]

Visually I'm pretty happy with how things are looking. I think you should add a tooltip to the revert button "Revert to default".

[bdach]

passing remark, no action required: was hoping to see something less of an utter hack job to replace this but alright

[frenzibyte]

Had same thoughts but looking at what ScreenBehaviour faces I think it's just fine the way it is. If anything, maybe we could have a SearchPredicate function in SearchContainer which, rather than filtering by a search term, it filters by giving each Drawable to that predicate and returning true/false to filter. But I'll leave that for another day.

[bdach]

seems good enough structure-wise, will leave visuals to be judged by @peppy