From 2350806b4cc8fbe23ee21288e5c75d16407133a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 22 Nov 2021 20:26:45 +0100 Subject: [PATCH 1/4] Add failing test case for number box stack overflow scenario --- .../Settings/TestSceneSettingsNumberBox.cs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 osu.Game.Tests/Visual/Settings/TestSceneSettingsNumberBox.cs diff --git a/osu.Game.Tests/Visual/Settings/TestSceneSettingsNumberBox.cs b/osu.Game.Tests/Visual/Settings/TestSceneSettingsNumberBox.cs new file mode 100644 index 0000000000..ffa1200f32 --- /dev/null +++ b/osu.Game.Tests/Visual/Settings/TestSceneSettingsNumberBox.cs @@ -0,0 +1,25 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System.Linq; +using NUnit.Framework; +using osu.Framework.Testing; +using osu.Game.Graphics.UserInterface; +using osu.Game.Overlays.Settings; + +namespace osu.Game.Tests.Visual.Settings +{ + public class TestSceneSettingsNumberBox : OsuTestScene + { + [Test] + public void TestLargeInteger() + { + SettingsNumberBox numberBox = null; + + AddStep("create number box", () => Child = numberBox = new SettingsNumberBox()); + + AddStep("set value to 1,000,000,000", () => numberBox.Current.Value = 1_000_000_000); + AddAssert("text box text is correct", () => numberBox.ChildrenOfType().Single().Current.Value == "1000000000"); + } + } +} From dced6a2e682a9902f43e1901e2ae1912bb637016 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 22 Nov 2021 20:39:44 +0100 Subject: [PATCH 2/4] Add extended test coverage for desired input handling --- .../Settings/TestSceneSettingsNumberBox.cs | 65 +++++++++++++++++-- 1 file changed, 59 insertions(+), 6 deletions(-) diff --git a/osu.Game.Tests/Visual/Settings/TestSceneSettingsNumberBox.cs b/osu.Game.Tests/Visual/Settings/TestSceneSettingsNumberBox.cs index ffa1200f32..c063e5526a 100644 --- a/osu.Game.Tests/Visual/Settings/TestSceneSettingsNumberBox.cs +++ b/osu.Game.Tests/Visual/Settings/TestSceneSettingsNumberBox.cs @@ -11,15 +11,68 @@ namespace osu.Game.Tests.Visual.Settings { public class TestSceneSettingsNumberBox : OsuTestScene { + private SettingsNumberBox numberBox; + private OsuTextBox textBox; + + [SetUpSteps] + public void SetUpSteps() + { + AddStep("create number box", () => Child = numberBox = new SettingsNumberBox()); + AddStep("get inner text box", () => textBox = numberBox.ChildrenOfType().Single()); + } + [Test] public void TestLargeInteger() { - SettingsNumberBox numberBox = null; - - AddStep("create number box", () => Child = numberBox = new SettingsNumberBox()); - - AddStep("set value to 1,000,000,000", () => numberBox.Current.Value = 1_000_000_000); - AddAssert("text box text is correct", () => numberBox.ChildrenOfType().Single().Current.Value == "1000000000"); + AddStep("set current to 1,000,000,000", () => numberBox.Current.Value = 1_000_000_000); + AddAssert("text box text is correct", () => textBox.Text == "1000000000"); } + + [Test] + public void TestUserInput() + { + inputText("42"); + currentValueIs(42); + currentTextIs("42"); + + inputText(string.Empty); + currentValueIs(null); + currentTextIs(string.Empty); + + inputText("555"); + currentValueIs(555); + currentTextIs("555"); + + inputText("-4444"); + // attempting to input the minus will raise an input error, the rest will pass through fine. + currentValueIs(4444); + currentTextIs("4444"); + + // checking the upper bound. + inputText(int.MaxValue.ToString()); + currentValueIs(int.MaxValue); + currentTextIs(int.MaxValue.ToString()); + + inputText((long)int.MaxValue + 1.ToString()); + currentValueIs(int.MaxValue); + currentTextIs(int.MaxValue.ToString()); + + inputText("0"); + currentValueIs(0); + currentTextIs("0"); + + // checking that leading zeroes are stripped. + inputText("00"); + currentValueIs(0); + currentTextIs("0"); + + inputText("01"); + currentValueIs(1); + currentTextIs("1"); + } + + private void inputText(string text) => AddStep($"set textbox text to {text}", () => textBox.Text = text); + private void currentValueIs(int? value) => AddAssert($"current value is {value?.ToString() ?? "null"}", () => numberBox.Current.Value == value); + private void currentTextIs(string value) => AddAssert($"current text is {value}", () => textBox.Text == value); } } From 4a9f080f3c0c227ff17ab5b4dc1744d4c433e354 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 22 Nov 2021 20:41:27 +0100 Subject: [PATCH 3/4] Accept full range of `int` in `SettingsNumberBox` This fixes stack overflow exceptions that would arise when a `Current.Value` of 1 billion or more was set on a `SettingsNumberBox`. The stack overflow was caused by the "maximum 9 digits" spec. If a value technically within `int` bounds, but larger than 1 billion (in the range [1,000,000,000; 2,147,483,647], to be more precise), a feedback loop between the setting control's `Current` and its inner text box's `Current` would occur, wherein the last digit would be trimmed and then re-appended again forevermore. To resolve, remove the offending spec and rely on `int.TryParse` entirely to be able to discern overflow range. Additionally, UX of the text box is slightly changed to notify when the `int` range is exceeded with a red flash. This behaviour would not have been possible to implement without recent framework-side fixes to text box (removal of text set scheduling). --- osu.Game/Overlays/Settings/SettingsNumberBox.cs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/osu.Game/Overlays/Settings/SettingsNumberBox.cs b/osu.Game/Overlays/Settings/SettingsNumberBox.cs index 545f1050b2..cbe9f7fc64 100644 --- a/osu.Game/Overlays/Settings/SettingsNumberBox.cs +++ b/osu.Game/Overlays/Settings/SettingsNumberBox.cs @@ -35,7 +35,6 @@ namespace osu.Game.Overlays.Settings { numberBox = new OutlinedNumberBox { - LengthLimit = 9, // limited to less than a value that could overflow int32 backing. Margin = new MarginPadding { Top = 5 }, RelativeSizeAxes = Axes.X, CommitOnFocusLost = true @@ -44,12 +43,19 @@ namespace osu.Game.Overlays.Settings numberBox.Current.BindValueChanged(e => { - int? value = null; + if (string.IsNullOrEmpty(e.NewValue)) + { + Current.Value = null; + return; + } if (int.TryParse(e.NewValue, out int intVal)) - value = intVal; + Current.Value = intVal; + else + numberBox.NotifyInputError(); - current.Value = value; + // trigger Current again to either restore the previous text box value, or to reformat the new value via .ToString(). + Current.TriggerChange(); }); Current.BindValueChanged(e => @@ -62,6 +68,8 @@ namespace osu.Game.Overlays.Settings private class OutlinedNumberBox : OutlinedTextBox { protected override bool CanAddCharacter(char character) => char.IsNumber(character); + + public new void NotifyInputError() => base.NotifyInputError(); } } } From b596a0204c5d4a48965812dbba173d3fa4729577 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 23 Nov 2021 06:43:17 +0100 Subject: [PATCH 4/4] Rewrite overflow test step for legibility --- .../Visual/Settings/TestSceneSettingsNumberBox.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Settings/TestSceneSettingsNumberBox.cs b/osu.Game.Tests/Visual/Settings/TestSceneSettingsNumberBox.cs index c063e5526a..334a814688 100644 --- a/osu.Game.Tests/Visual/Settings/TestSceneSettingsNumberBox.cs +++ b/osu.Game.Tests/Visual/Settings/TestSceneSettingsNumberBox.cs @@ -53,7 +53,7 @@ namespace osu.Game.Tests.Visual.Settings currentValueIs(int.MaxValue); currentTextIs(int.MaxValue.ToString()); - inputText((long)int.MaxValue + 1.ToString()); + inputText(smallestOverflowValue.ToString()); currentValueIs(int.MaxValue); currentTextIs(int.MaxValue.ToString()); @@ -74,5 +74,10 @@ namespace osu.Game.Tests.Visual.Settings private void inputText(string text) => AddStep($"set textbox text to {text}", () => textBox.Text = text); private void currentValueIs(int? value) => AddAssert($"current value is {value?.ToString() ?? "null"}", () => numberBox.Current.Value == value); private void currentTextIs(string value) => AddAssert($"current text is {value}", () => textBox.Text == value); + + /// + /// The smallest number that overflows . + /// + private static long smallestOverflowValue => 1L + int.MaxValue; } }