From 86c3e3e987740745ddebed4680dc1e3ff380ffa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 4 Oct 2024 13:59:04 +0200 Subject: [PATCH] Replace `FormSliderBar.Instantaneous` with `TransferValueOnCommit` Rather than control the propagation of the value between the slider and the textbox, add a property that controls the propagation of the value between the bindables inside the form control to external bindables. This will help alleviate issues where the external bindable update incurs overheads due to having heavy change callbacks attached. --- .../UserInterface/TestSceneFormControls.cs | 15 +--- .../UserInterface/TestSceneFormSliderBar.cs | 63 ++++++++++++++ .../Graphics/UserInterfaceV2/FormSliderBar.cs | 82 ++++++++++++------- 3 files changed, 116 insertions(+), 44 deletions(-) create mode 100644 osu.Game.Tests/Visual/UserInterface/TestSceneFormSliderBar.cs diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneFormControls.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneFormControls.cs index 518c3fc69329..c6fd65b9731e 100644 --- a/osu.Game.Tests/Visual/UserInterface/TestSceneFormControls.cs +++ b/osu.Game.Tests/Visual/UserInterface/TestSceneFormControls.cs @@ -72,7 +72,7 @@ public TestSceneFormControls() }, new FormSliderBar { - Caption = "Instantaneous slider", + Caption = "Slider", Current = new BindableFloat { MinValue = 0, @@ -82,19 +82,6 @@ public TestSceneFormControls() }, TabbableContentContainer = this, }, - new FormSliderBar - { - Caption = "Non-instantaneous slider", - Current = new BindableFloat - { - MinValue = 0, - MaxValue = 10, - Value = 5, - Precision = 0.1f, - }, - Instantaneous = false, - TabbableContentContainer = this, - }, new FormEnumDropdown { Caption = EditorSetupStrings.EnableCountdown, diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneFormSliderBar.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneFormSliderBar.cs new file mode 100644 index 000000000000..97835a993d60 --- /dev/null +++ b/osu.Game.Tests/Visual/UserInterface/TestSceneFormSliderBar.cs @@ -0,0 +1,63 @@ +// 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.Allocation; +using osu.Framework.Bindables; +using osu.Framework.Extensions.ObjectExtensions; +using osu.Framework.Graphics; +using osu.Framework.Graphics.Containers; +using osu.Game.Graphics.Sprites; +using osu.Game.Graphics.UserInterfaceV2; +using osu.Game.Overlays; +using osuTK; + +namespace osu.Game.Tests.Visual.UserInterface +{ + public partial class TestSceneFormSliderBar : OsuTestScene + { + [Cached] + private OverlayColourProvider colourProvider = new OverlayColourProvider(OverlayColourScheme.Aquamarine); + + [Test] + public void TestTransferValueOnCommit() + { + OsuSpriteText text; + FormSliderBar slider = null!; + + AddStep("create content", () => + { + Child = new FillFlowContainer + { + RelativeSizeAxes = Axes.Both, + Anchor = Anchor.Centre, + Origin = Anchor.Centre, + Width = 0.5f, + Direction = FillDirection.Vertical, + Spacing = new Vector2(10), + Children = new Drawable[] + { + text = new OsuSpriteText(), + slider = new FormSliderBar + { + Caption = "Slider", + Current = new BindableFloat + { + MinValue = 0, + MaxValue = 10, + Precision = 0.1f, + Default = 5f, + } + }, + } + }; + slider.Current.BindValueChanged(_ => text.Text = $"Current value is: {slider.Current.Value}", true); + }); + AddToggleStep("toggle transfer value on commit", b => + { + if (slider.IsNotNull()) + slider.TransferValueOnCommit = b; + }); + } + } +} diff --git a/osu.Game/Graphics/UserInterfaceV2/FormSliderBar.cs b/osu.Game/Graphics/UserInterfaceV2/FormSliderBar.cs index a29e33a42146..da28437eee2c 100644 --- a/osu.Game/Graphics/UserInterfaceV2/FormSliderBar.cs +++ b/osu.Game/Graphics/UserInterfaceV2/FormSliderBar.cs @@ -28,26 +28,22 @@ public partial class FormSliderBar : CompositeDrawable, IHasCurrentValue public Bindable Current { get => current.Current; - set => current.Current = value; + set + { + current.Current = value; + currentNumberInstantaneous.Default = current.Default; + } } - private bool instantaneous = true; + private readonly BindableNumberWithCurrent current = new BindableNumberWithCurrent(); + + private readonly BindableNumber currentNumberInstantaneous = new BindableNumber(); /// - /// Whether changes to the slider should instantaneously transfer to the text box (and vice versa). - /// If , the transfer will happen on text box commit (explicit, or implicit via focus loss), or on slider drag end. + /// Whether changes to the value should instantaneously transfer to outside bindables. + /// If , the transfer will happen on text box commit (explicit, or implicit via focus loss), or on slider commit. /// - public bool Instantaneous - { - get => instantaneous; - set - { - instantaneous = value; - - if (slider.IsNotNull()) - slider.TransferValueOnCommit = !instantaneous; - } - } + public bool TransferValueOnCommit { get; set; } private CompositeDrawable? tabbableContentContainer; @@ -62,8 +58,6 @@ public CompositeDrawable? TabbableContentContainer } } - private readonly BindableNumberWithCurrent current = new BindableNumberWithCurrent(); - /// /// Caption describing this slider bar, displayed on top of the controls. /// @@ -147,8 +141,8 @@ private void load(OsuColour colours, OsuGame? game) Origin = Anchor.CentreRight, RelativeSizeAxes = Axes.X, Width = 0.5f, - Current = Current, - TransferValueOnCommit = !instantaneous, + Current = currentNumberInstantaneous, + OnCommit = () => current.Value = currentNumberInstantaneous.Value, } }, }, @@ -170,9 +164,28 @@ protected override void LoadComplete() slider.IsDragging.BindValueChanged(_ => updateState()); + current.ValueChanged += e => currentNumberInstantaneous.Value = e.NewValue; + current.MinValueChanged += v => currentNumberInstantaneous.MinValue = v; + current.MaxValueChanged += v => currentNumberInstantaneous.MaxValue = v; + current.PrecisionChanged += v => currentNumberInstantaneous.Precision = v; + current.DisabledChanged += disabled => + { + if (disabled) + { + // revert any changes before disabling to make sure we are in a consistent state. + currentNumberInstantaneous.Value = current.Value; + } + + currentNumberInstantaneous.Disabled = disabled; + }; + + current.CopyTo(currentNumberInstantaneous); currentLanguage.BindValueChanged(_ => Schedule(updateValueDisplay)); - current.BindValueChanged(_ => + currentNumberInstantaneous.BindValueChanged(e => { + if (!TransferValueOnCommit) + current.Value = e.NewValue; + updateState(); updateValueDisplay(); }, true); @@ -182,17 +195,15 @@ protected override void LoadComplete() private void textChanged(ValueChangedEvent change) { - if (!instantaneous) return; - tryUpdateSliderFromTextBox(); } private void textCommitted(TextBox t, bool isNew) { tryUpdateSliderFromTextBox(); - // If the attempted update above failed, restore text box to match the slider. - Current.TriggerChange(); + currentNumberInstantaneous.TriggerChange(); + current.Value = currentNumberInstantaneous.Value; flashLayer.Colour = ColourInfo.GradientVertical(colourProvider.Dark2.Opacity(0), colourProvider.Dark2); flashLayer.FadeOutFromOne(800, Easing.OutQuint); @@ -204,7 +215,7 @@ private void tryUpdateSliderFromTextBox() try { - switch (Current) + switch (currentNumberInstantaneous) { case Bindable bindableInt: bindableInt.Value = int.Parse(textBox.Current.Value); @@ -215,7 +226,7 @@ private void tryUpdateSliderFromTextBox() break; default: - Current.Parse(textBox.Current.Value, CultureInfo.CurrentCulture); + currentNumberInstantaneous.Parse(textBox.Current.Value, CultureInfo.CurrentCulture); break; } } @@ -250,9 +261,9 @@ private void updateState() { textBox.Alpha = 1; - background.Colour = Current.Disabled ? colourProvider.Background4 : colourProvider.Background5; - caption.Colour = Current.Disabled ? colourProvider.Foreground1 : colourProvider.Content2; - textBox.Colour = Current.Disabled ? colourProvider.Foreground1 : colourProvider.Content1; + background.Colour = currentNumberInstantaneous.Disabled ? colourProvider.Background4 : colourProvider.Background5; + caption.Colour = currentNumberInstantaneous.Disabled ? colourProvider.Foreground1 : colourProvider.Content2; + textBox.Colour = currentNumberInstantaneous.Disabled ? colourProvider.Foreground1 : colourProvider.Content1; BorderThickness = IsHovered || textBox.Focused.Value || slider.IsDragging.Value ? 2 : 0; BorderColour = textBox.Focused.Value ? colourProvider.Highlight1 : colourProvider.Light4; @@ -269,12 +280,13 @@ private void updateValueDisplay() { if (updatingFromTextBox) return; - textBox.Text = slider.GetDisplayableValue(Current.Value).ToString(); + textBox.Text = slider.GetDisplayableValue(currentNumberInstantaneous.Value).ToString(); } private partial class Slider : OsuSliderBar { public BindableBool IsDragging { get; set; } = new BindableBool(); + public Action? OnCommit { get; set; } private Box leftBox = null!; private Box rightBox = null!; @@ -381,6 +393,16 @@ protected override void UpdateValue(float value) { nub.MoveToX(value, 200, Easing.OutPow10); } + + protected override bool Commit() + { + bool result = base.Commit(); + + if (result) + OnCommit?.Invoke(); + + return result; + } } } }