-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Update settings to use new "form" style controls #36193
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
Conversation
|
I dunno about the Also, I'd expect to be able to click anywhere on the text in such header items to perform the toggle. |
2026-01-01.17.19.00.mp4This is weird. Might be best to fix separately but definitely needs to be changed. |
|
Maybe too late to say now, but I feel that the on vs. off contrast of switches is too subtle. This is particularly confusing for vertical switches. It is not clear if "up" is on or "down" is on. |
| public SettingsButtonV2() | ||
| { | ||
| RelativeSizeAxes = Axes.X; | ||
| Margin = new MarginPadding { Vertical = -1.5f }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike SettingsButton with -5, the vertical margin here is set to -1.5 instead. This is because the overall settings spacing specified in flow containers has been reduced from 14 to 7, and to preserve the button spacing with that change, -5 + 7 / 2 = -1.5.
| Anchor = Anchor.TopRight, | ||
| Origin = Anchor.TopRight | ||
| Origin = Anchor.TopRight, | ||
| Spacing = new Vector2(-6, 0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| content.AutoSizeDuration = 500; | ||
| content.AutoSizeDuration = 250; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The duration set here felt too slow for a snappy animation.
Before:
CleanShot.2026-01-01.at.06.45.52.mp4
After:
CleanShot.2026-01-01.at.06.45.32.mp4
| bool negativeBottomMargin = !handlerEnabled.Value || FlowContent.Count == 0; | ||
| HeaderContainer.TransformTo(nameof(Margin), new MarginPadding { Bottom = negativeBottomMargin ? -15 : 0 }, 300, Easing.OutQuint); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| windowModeDropdown.SetNoticeText(LayoutSettingsStrings.FullscreenMacOSNote, true); | ||
| windowModeDropdownNote.Value = new SettingsNote.Data(LayoutSettingsStrings.FullscreenMacOSNote, SettingsNote.Type.Critical); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've increased the level of this note from warning to critical as system-wide freezes may occur in this mode.
| windowModeDropdown.SetNoticeText(LayoutSettingsStrings.CheckingForFullscreenCapabilities, true); | ||
| windowModeDropdownNote.Value = new SettingsNote.Data(LayoutSettingsStrings.CheckingForFullscreenCapabilities, SettingsNote.Type.Informational); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've dropped down the level of this note as it's not really a "warning" yet. Informational makes most sense.
I've added this but I'm not 100% sure about the hover design and feedback there: CleanShot.2026-01-01.at.09.20.52.mp4 |
Sorry to +1 but I very much agree with this. I almost never see vertical switches in software, and think it's a suboptimal design because there is no established on or off position like you said. I personally expected "up" to be the on position because that's how most real-life switches work in my head. Wouldn't a horizontal switch look just as good here? |
|
Definitely need some word wrap for those, well spotted. |
|
The button text overlaps shown above will not be addressed in this PR / for this PR to proceed. It is too involved to be included here. |
|
-skin dropdown: doesn't show "type to search" anymore when active Registrazione.dello.schermo.2026-01-09.105028.mp4when closing a dropdown, the easing comes to a stop and then there's a final stutter that gets below elements into the correct place. Registrazione.dello.schermo.2026-01-09.105424.mp4 |
Addresses concerns in #36193 (comment) (excluding the last part, that is too involved and I can't imagine any workaround for it due to how strict the `Dropdown` structure is). Also adds truncation/padding to header label and search bar. Preview: https://github.com/user-attachments/assets/8885cb90-44dc-42ee-af21-cb33f7723e63
|
|
||
| scalingSettings.AutoSizeAxes = scalingMode.Value != ScalingMode.Off ? Axes.Y : Axes.None; | ||
| scalingSettings.ForEach(s => | ||
| scalingSettings.ForEach(item => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this to just store an array of the stuff we want to change (during construction) and address directly, rather than ForEach with comparisons and casts and stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following diff attempts to achieve the suggestion, but the end result doesn't quite feel like worth it just to not have Control exposed:
diff
diff --git a/osu.Game/Overlays/Settings/Sections/Graphics/LayoutSettings.cs b/osu.Game/Overlays/Settings/Sections/Graphics/LayoutSettings.cs
index 81a3029c2d..a04e103093 100644
--- a/osu.Game/Overlays/Settings/Sections/Graphics/LayoutSettings.cs
+++ b/osu.Game/Overlays/Settings/Sections/Graphics/LayoutSettings.cs
@@ -31,6 +31,8 @@ public partial class LayoutSettings : SettingsSubsection
private FillFlowContainer<SettingsItemV2> scalingSettings = null!;
+ private readonly List<FormSliderBar<float>> scalingSettingsControls = new List<FormSliderBar<float>>();
+
private readonly Bindable<Display> currentDisplay = new Bindable<Display>();
private Bindable<ScalingMode> scalingMode = null!;
@@ -60,7 +62,8 @@ public partial class LayoutSettings : SettingsSubsection
private FormDropdown<Display> displayDropdown = null!;
private FormDropdown<WindowMode> windowModeDropdown = null!;
- private FormSliderBar<float> dimSlider = null!;
+ private SettingsItemV2 dimSliderSetting = null!;
+ private FormSliderBar<float> dimSliderSettingControl = null!;
private readonly Bindable<SettingsNote.Data?> windowModeDropdownNote = new Bindable<SettingsNote.Data?>();
@@ -194,7 +197,7 @@ private void load(FrameworkConfigManager config, OsuConfigManager osuConfig, Gam
Current = scalingPositionX,
KeyboardStep = 0.01f,
DisplayAsPercentage = true,
- }.With(bindPreviewEvent))
+ }.With(setupScalingControl))
{
Keywords = new[] { "screen", "scaling" },
},
@@ -204,7 +207,7 @@ private void load(FrameworkConfigManager config, OsuConfigManager osuConfig, Gam
Current = scalingPositionY,
KeyboardStep = 0.01f,
DisplayAsPercentage = true,
- }.With(bindPreviewEvent))
+ }.With(setupScalingControl))
{
Keywords = new[] { "screen", "scaling" },
},
@@ -214,7 +217,7 @@ private void load(FrameworkConfigManager config, OsuConfigManager osuConfig, Gam
Current = scalingSizeX,
KeyboardStep = 0.01f,
DisplayAsPercentage = true,
- }.With(bindPreviewEvent))
+ }.With(setupScalingControl))
{
Keywords = new[] { "screen", "scaling" },
},
@@ -224,17 +227,17 @@ private void load(FrameworkConfigManager config, OsuConfigManager osuConfig, Gam
Current = scalingSizeY,
KeyboardStep = 0.01f,
DisplayAsPercentage = true,
- }.With(bindPreviewEvent))
+ }.With(setupScalingControl))
{
Keywords = new[] { "screen", "scaling" },
},
- new SettingsItemV2(dimSlider = new FormSliderBar<float>
+ dimSliderSetting = new SettingsItemV2(dimSliderSettingControl = new FormSliderBar<float>
{
Caption = GameplaySettingsStrings.BackgroundDim,
Current = scalingBackgroundDim,
KeyboardStep = 0.01f,
DisplayAsPercentage = true,
- }.With(bindPreviewEvent)),
+ }.With(setupScalingControl)),
}
},
};
@@ -242,6 +245,21 @@ private void load(FrameworkConfigManager config, OsuConfigManager osuConfig, Gam
fullscreenCapability.BindValueChanged(_ => Schedule(updateScreenModeWarning), true);
}
+ private void setupScalingControl(FormSliderBar<float> slider)
+ {
+ scalingSettingsControls.Add(slider);
+
+ slider.Current.ValueChanged += _ =>
+ {
+ switch (scalingMode.Value)
+ {
+ case ScalingMode.Gameplay:
+ showPreview();
+ break;
+ }
+ };
+ }
+
protected override void LoadComplete()
{
base.LoadComplete();
@@ -336,15 +354,16 @@ void updateScalingModeVisibility()
scalingSettings.AutoSizeAxes = scalingMode.Value != ScalingMode.Off ? Axes.Y : Axes.None;
scalingSettings.ForEach(item =>
{
- FormSliderBar<float> slider = (FormSliderBar<float>)item.Control;
-
- if (slider == dimSlider)
+ if (item == dimSliderSetting)
item.CanBeShown.Value = scalingMode.Value == ScalingMode.Everything || scalingMode.Value == ScalingMode.ExcludeOverlays;
else
- {
- slider.TransferValueOnCommit = scalingMode.Value == ScalingMode.Everything;
item.CanBeShown.Value = scalingMode.Value != ScalingMode.Off;
- }
+ });
+
+ scalingSettingsControls.ForEach(slider =>
+ {
+ if (slider != dimSliderSettingControl)
+ slider.TransferValueOnCommit = scalingMode.Value == ScalingMode.Everything;
});
}
}
@@ -412,19 +431,6 @@ private void updateScreenModeWarning()
}
}
- private void bindPreviewEvent(FormSliderBar<float> slider)
- {
- slider.Current.ValueChanged += _ =>
- {
- switch (scalingMode.Value)
- {
- case ScalingMode.Gameplay:
- showPreview();
- break;
- }
- };
- }
-
private Drawable? preview;
private void showPreview()
Particularly, it seems the suggestion's goal is to replace the highlighted ForEach loop with a simpler one that doesn't require pattern matching, but in reality the surrounding code changed from one loop into two loops instead (because there's a flag in SettingsItemV2 that's updated in this code path):
For now, I've at least omitted the first contextless commit and split out the LayoutSettings change into its own commit with the required structural change: 874e3ad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just going to go with what you already have at this point. I won't comment on my thoughts on the structure of things for now. Hopefully this is a one-off.
Should've been true by default, minor mistake from a previous PR.
Used for "confine mouse mode" dropdown tooltip text.
3186c8e to
ed091dc
Compare
bdach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
structural review only, scope of review limited to the high level components, I'm not going through the 60 other files to check if there's anything structural in there
I guess you could merge this as is if you really want to, nothing here is blocking per se
| [BackgroundDependencyLoader] | ||
| private void load() | ||
| { | ||
| HeaderContainer.Child = new ToggleableHeader(Header, IsToggleable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is dodgy as hell, the preferred solution would be to either have a CreateHeader() method in the base, or just remove the header from the base and have inheritors insert it themselves
| public sealed partial class SettingsItemV2 : CompositeDrawable, ISettingsItem, IConditionalFilterable | ||
| { | ||
| private readonly IFormControl control; | ||
| public readonly IFormControl Control; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how much of a stink to kick up about this one because it appears to be done for the sake of a single usage in LayoutSettings and could be obviated by trivial if a little annoying local workaround or even .ChildrenOfType<T>(). I guess I can let it slide but will watch closely nothing stupid happens further with this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a quick review of:
#36193 (comment)
#36193 (comment)
https://discord.com/channels/90072389919997952/1458132886946451497/1460540323011887238
and see if you agree with me turning a blind eye here or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I've seen those and as I mentioned above the possible workarounds here are wonky so I'm not going to insist on changes. can stay as is for now.
|
Oops didn't mean to push that commit here, will remove. |
933f9e1 to
cd433fe
Compare
Reported in #36193 (comment). Preview: https://github.com/user-attachments/assets/ac2b86cf-ad92-496b-b3dd-19ad47753a9b --------- Co-authored-by: Dean Herbert <[email protected]>






























The final piece, containing migration of all settings to use the new form controls, as well as minor changes within the settings overlay to complete the migration.
To avoid touching any irrelevant file, I've chosen to make and use separate constants and classes for new settings, as shown in 8956bbd. Once this PR is merged, a follow-up PR will migrate the remaining non-settings usages of
SettingsItemto the V2 counterpart, removeSettingsItemcompletely, and drop the V2 suffix from all existing components.Side design changes worthy of note: