Add format provider argument to IParseable.Parse()

[Neuheit]

Adds a method override for Bindable.Parse() to accept different CultureInfo values

Intended for #5176

Breaking changes

IParseable.Parse() has received an IFormatProvider argument

This facilitates better treatment of input as typed by end users, by respecting their regional number settings.

Existing usages of IParseable.Parse() should pass CultureInfo.InvariantCulture to preserve behaviour.

[peppy]

Calls to the new overload for bindables with custom overrides of Parse(object) will fail.

The virtual flag should probably be removed from the overload without the CultureInfo and those implementation should be moved to the more specific overload.

Alternatively, rather than adding an overload, a nullable CultureInfo parameter could be added to the original one, avoiding the silent failures mentioned above (because it was compile-time fail until fixed).

[peppy]

As proposed.

[Neuheit]

Calls to the new overload for bindables with custom overrides of Parse(object) will fail.

The virtual flag should probably be removed from the overload without the CultureInfo and those implementation should be moved to the more specific overload.

Alternatively, rather than adding an overload, a nullable CultureInfo parameter could be added to the original one, avoiding the silent failures mentioned above (because it was compile-time fail until fixed).

I tried that originally, but that would mean we would have to implement it in IParseable, which would make it required for the other bindables that don't have to do with strings, so i didnt think that would make sense. I can still do it that way if you like.

[peppy]

I think you might be misunderstanding the fail case. Here's a diff showing it failing:

diff --git a/osu.Framework.Tests/Bindables/BindableBoolTest.cs b/osu.Framework.Tests/Bindables/BindableBoolTest.cs
index dc7677fd2..7cf9991f3 100644
--- a/osu.Framework.Tests/Bindables/BindableBoolTest.cs
+++ b/osu.Framework.Tests/Bindables/BindableBoolTest.cs
@@ -1,6 +1,7 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System.Globalization;
 using NUnit.Framework;
 using osu.Framework.Bindables;
 
@@ -26,7 +27,7 @@ public void TestSet(bool value)
         public void TestParsingString(string value, bool expected)
         {
             var bindable = new BindableBool();
-            bindable.Parse(value);
+            bindable.Parse(value, CultureInfo.InvariantCulture);
 
             Assert.AreEqual(expected, bindable.Value);
         }

[Neuheit]

I ended up creating a new interface that's used to parse the base bindable types, since having cultureinfo on the list and dictionary types didn't make sense. I also added test cases for doubles and floats.

[peppy]

I'm not sure about a second interface. @ppy/team-client would want some other opinions here before moving forward.

[Susko3]

I think there should be only one interface, and it should match IFormattable.

The interface should be applied to IBindable and IBindable<T> as well.

[Susko3]

Needs to consider and test round-trip ToString + Parse.

[bdach]

Agree with one interface, the argument about IBindable{List,Dictionary} not doing anything with the culture info seems moot since the two already do not and cannot parse strings.

[peppy]

As a note, this is a breaking change and will likely require very fine combing on all projects ever (ie. anything that uses this in serialisation).

I'll go as far as saying this may require an envvar setting for the old behaviour to ease migration... will wait for more thoughts on this.

[Neuheit]

My original thought was that we keep it invariant by default and just override it in the game itself where the styling becomes bugged (e.g. in ppy/osu#18271.

[Susko3]

may require an envvar setting for the old behaviour to ease migration

I don't think this really works. If we update framework to the new behaviour, setting the envvar might break the framework.

What if, instead, IFormatProvider provider is made non-optional. This way, all existing code won't compile, so devs will have to think about whether they want current culture or invariant culture for Parse(). Could mention in the xmldoc remarks section to pass in either the Current or Invariant culture.

[bdach]

Tend to agree with non-optional, that's how this entire string-printing, parsing, locale, culture stuff should have been designed everywhere to begin with. All of this "default to ambient locale" stuff is just silent footguns. I was only proposing to have it behave the same way because of consistency.

[smoogipoo]

Agree with making non-optional as long as the changes required to update code to it aren't silly.

[peppy]

osu side changes with non-optional are pretty reasonable:

diff --git a/osu.Framework/Bindables/Bindable.cs b/osu.Framework/Bindables/Bindable.cs
index 28fc1e75c..a1ce5f5a8 100644
--- a/osu.Framework/Bindables/Bindable.cs
+++ b/osu.Framework/Bindables/Bindable.cs
@@ -249,7 +249,7 @@ private void addWeakReference(WeakReference<Bindable<T>> weakReference)
         /// </summary>
         /// <param name="input">The input which is to be parsed.</param>
         /// <param name="provider">An object that provides culture-specific formatting information about <paramref name="input"/>.</param>
-        public virtual void Parse([CanBeNull] object input, [CanBeNull] IFormatProvider provider = null)
+        public virtual void Parse([CanBeNull] object input, [CanBeNull] IFormatProvider provider)
         {
             switch (input)
             {
diff --git a/osu.Framework/Bindables/IParseable.cs b/osu.Framework/Bindables/IParseable.cs
index c91b00a3f..65970b97c 100644
--- a/osu.Framework/Bindables/IParseable.cs
+++ b/osu.Framework/Bindables/IParseable.cs
@@ -15,6 +15,6 @@ public interface IParseable
         /// </summary>
         /// <param name="input">The input which is to be parsed.</param>
         /// <param name="provider">An object that provides culture-specific formatting information about <paramref name="input"/>.</param>
-        void Parse(object? input, IFormatProvider? provider = null);
+        void Parse(object? input, IFormatProvider? provider);
     }
 }
diff --git a/osu.Framework/Configuration/IniConfigManager.cs b/osu.Framework/Configuration/IniConfigManager.cs
index 0b3470420..5c07aa73b 100644
--- a/osu.Framework/Configuration/IniConfigManager.cs
+++ b/osu.Framework/Configuration/IniConfigManager.cs
@@ -67,7 +67,7 @@ protected override void PerformLoad()
                                 if (!(b is IParseable parseable))
                                     throw new InvalidOperationException($"Bindable type {b.GetType().ReadableName()} is not {nameof(IParseable)}.");
 
-                                parseable.Parse(val);
+                                parseable.Parse(val, CultureInfo.InvariantCulture);
                             }
                             catch (Exception e)
                             {

so let's just go with that direction.

[bdach]

That doesn't look like a game-side diff. I applied this though:

diff --git a/osu.Game/Graphics/UserInterfaceV2/SliderWithTextBoxInput.cs b/osu.Game/Graphics/UserInterfaceV2/SliderWithTextBoxInput.cs
index 37ea2a3f96..e5ba7f61bf 100644
--- a/osu.Game/Graphics/UserInterfaceV2/SliderWithTextBoxInput.cs
+++ b/osu.Game/Graphics/UserInterfaceV2/SliderWithTextBoxInput.cs
@@ -121,7 +121,7 @@ private void tryUpdateSliderFromTextBox()
                         break;
 
                     default:
-                        slider.Current.Parse(textBox.Current.Value);
+                        slider.Current.Parse(textBox.Current.Value, CultureInfo.CurrentCulture);
                         break;
                 }
             }
diff --git a/osu.Game/Rulesets/Configuration/RulesetConfigManager.cs b/osu.Game/Rulesets/Configuration/RulesetConfigManager.cs
index 0eea1ff215..418dc3576f 100644
--- a/osu.Game/Rulesets/Configuration/RulesetConfigManager.cs
+++ b/osu.Game/Rulesets/Configuration/RulesetConfigManager.cs
@@ -84,7 +84,7 @@ protected override void AddBindable<TBindable>(TLookup lookup, Bindable<TBindabl
 
             if (setting != null)
             {
-                bindable.Parse(setting.Value);
+                bindable.Parse(setting.Value, CultureInfo.InvariantCulture);
             }
             else
             {
diff --git a/osu.Game/Rulesets/Mods/Mod.cs b/osu.Game/Rulesets/Mods/Mod.cs
index 775f6a0ed4..d635dccab1 100644
--- a/osu.Game/Rulesets/Mods/Mod.cs
+++ b/osu.Game/Rulesets/Mods/Mod.cs
@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Globalization;
 using System.Linq;
 using System.Reflection;
 using Newtonsoft.Json;
@@ -284,7 +285,7 @@ internal virtual void CopyAdjustedSetting(IBindable target, object source)
                 if (!(target is IParseable parseable))
                     throw new InvalidOperationException($"Bindable type {target.GetType().ReadableName()} is not {nameof(IParseable)}.");
 
-                parseable.Parse(source);
+                parseable.Parse(source, CultureInfo.InvariantCulture);
             }
         }
 
diff --git a/osu.Game/Screens/Edit/Timing/IndeterminateSliderWithTextBoxInput.cs b/osu.Game/Screens/Edit/Timing/IndeterminateSliderWithTextBoxInput.cs
index eabe9b9f64..151d469415 100644
--- a/osu.Game/Screens/Edit/Timing/IndeterminateSliderWithTextBoxInput.cs
+++ b/osu.Game/Screens/Edit/Timing/IndeterminateSliderWithTextBoxInput.cs
@@ -103,7 +103,7 @@ public IndeterminateSliderWithTextBoxInput(LocalisableString labelText, Bindable
                             break;
 
                         default:
-                            slider.Current.Parse(t.Text);
+                            slider.Current.Parse(t.Text, CultureInfo.CurrentCulture);
                             break;
                     }
                 }
diff --git a/osu.Game/Skinning/ISerialisableDrawable.cs b/osu.Game/Skinning/ISerialisableDrawable.cs
index 503b44c2dd..c9dcaca6d1 100644
--- a/osu.Game/Skinning/ISerialisableDrawable.cs
+++ b/osu.Game/Skinning/ISerialisableDrawable.cs
@@ -2,6 +2,7 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System;
+using System.Globalization;
 using osu.Framework.Bindables;
 using osu.Framework.Extensions.TypeExtensions;
 using osu.Framework.Graphics;
@@ -46,7 +47,7 @@ void CopyAdjustedSetting(IBindable target, object source)
                 if (!(target is IParseable parseable))
                     throw new InvalidOperationException($"Bindable type {target.GetType().ReadableName()} is not {nameof(IParseable)}.");
 
-                parseable.Parse(source);
+                parseable.Parse(source, CultureInfo.InvariantCulture);
             }
         }
     }
diff --git a/osu.Game/Skinning/LegacySkin.cs b/osu.Game/Skinning/LegacySkin.cs
index 2e91770919..9102231913 100644
--- a/osu.Game/Skinning/LegacySkin.cs
+++ b/osu.Game/Skinning/LegacySkin.cs
@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
+using System.Globalization;
 using System.IO;
 using System.Linq;
 using JetBrains.Annotations;
@@ -330,7 +331,7 @@ protected override void ParseConfigurationStream(Stream stream)
 
                     var bindable = new Bindable<TValue>();
                     if (val != null)
-                        bindable.Parse(val);
+                        bindable.Parse(val, CultureInfo.InvariantCulture);
                     return bindable;
                 }
             }

and it seems tolerable indeed.

[peppy]

I'm.. not sure how I attached the wrong diff 😅 .

[peppy]

Will wait for one more check here.

[Susko3]

Should be good to go after some small tweaks.