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 01/20] 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 02/20] 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 03/20] 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 10bd7176e015f63e4bf2dfa829db66ce6d4a71f4 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Tue, 23 Nov 2021 14:06:06 +0900 Subject: [PATCH 04/20] Fix potential test failure in TestSceneMultiplayerReadyButton Couldn't exactly reproduce https://github.com/ppy/osu/runs/4294316800, but I found a similar issue via: ```diff diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs index 84b63a5733..29cac9b061 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs @@ -3,6 +3,7 @@ using System; using System.Linq; +using System.Threading; using System.Threading.Tasks; using NUnit.Framework; using osu.Framework.Allocation; @@ -81,6 +82,8 @@ private void load(GameHost host, AudioManager audio) await Client.ToggleReady(); + Thread.Sleep(1000); + readyClickOperation.Dispose(); }); } ``` --- .../Multiplayer/TestSceneMultiplayerReadyButton.cs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs index e9612bf55c..84b63a5733 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs @@ -9,6 +9,7 @@ using osu.Framework.Allocation; using osu.Framework.Audio; using osu.Framework.Bindables; using osu.Framework.Graphics; +using osu.Framework.Graphics.UserInterface; using osu.Framework.Platform; using osu.Framework.Testing; using osu.Framework.Utils; @@ -198,11 +199,15 @@ namespace osu.Game.Tests.Visual.Multiplayer }, users); } - private void addClickButtonStep() => AddStep("click button", () => + private void addClickButtonStep() { - InputManager.MoveMouseTo(button); - InputManager.Click(MouseButton.Left); - }); + AddUntilStep("wait for button to be ready", () => button.ChildrenOfType