[Core] Fixes conversion binding a DateTime to a string if the locale is not the US #3700

paymicro · 2018-09-04T12:05:57Z

Description of Change

Fixed converting the date without regard to the installed locale.
You can install the locale using the following code:

Thread.CurrentThread.CurrentCulture = new System.Globalization.CultureInfo("eu-AU");

Issues Resolved

fixes Bound DateTimes Display in en-US Format Not Local Format #2049

API Changes

None

Platforms Affected

Core (all platforms)

Behavioral/Visual Changes

Simple types correctly converts to a string from binding context.

Before/After Screenshots

Before

After

Testing Procedure

run UItest 2049
click on button "Change locale"
check result
click on button "Change locale" ...

PR Checklist

Has automated tests
Rebased on top of the target branch at time of PR
Changes adhere to coding standard

…is not the US

jassmith · 2018-09-05T18:04:06Z

build --uitests

samhouts

ConvertIsCultureInvariant unit test is failing

StephaneDelcroix · 2018-09-05T20:37:10Z

I will look at this, but I think it's not a matter of using the current locale while converting (there are good reasons why we do not) and more a matter of string formatting.

jassmith · 2018-09-06T17:19:40Z

build --uitests

samhouts · 2018-09-06T21:03:17Z

Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue2049.cs

+			public static Action<decimal> DecimalChange;
+		}
+
+		void UpdateDescLabel() => descLabel.Text = $"{_instuctions}{Thread.CurrentThread.CurrentCulture.DisplayName}";


When I run this on Android, I don't see the current culture on the page.

samhouts · 2018-09-06T21:03:30Z

Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue2049.cs

+		DateTime _testDate = DateTime.ParseExact("2077-12-31T13:55:56", "yyyy-MM-ddTHH:mm:ss", CultureInfo.InvariantCulture);
+		int _localeIndex = 0;
+		string[] _localeIds = new[] { "en-US", "pt-PT", "ru-RU", "en-AU", "zh-Hans" };
+		string _instuctions = $"When you change the locale, the date format must change.{Environment.NewLine}Current locale: ";


_instuctions >> _instructions

kingces95

Looks like a feature request to me; The existing behavior is expected. We have code that explicitly checks that we're using the invariant culture. Now, that's not to say we should stick with that behavior but to change it would be a feature and would have to be opt-in unless we decide it won't break anyone.

The new behavior, allowing culture specific conversions, should be applied to all renderers and so would require a horizontal audit of all renderers as well as tests for all newly culturally aware renderers.

kingces95 · 2018-09-06T23:31:04Z

Xamarin.Forms.Core.UnitTests/BindingUnitTests.cs

+			// people write numbers using the separator accepted in their locale
+			var ds = System.Threading.Thread.CurrentThread.CurrentCulture.NumberFormat.NumberDecimalSeparator;
+			vm.Text = $"0{ds}7";
+			Assert.That(slider.Value, Is.EqualTo(0.7));


This test explicitly expects the invariant culture is used when displaying the slider value. This would lead me to believe the original developer thought about Culture and decided to go with the invariant culture everywhere. So this PR is less of a "fix" and more of a feature request; The current behavior is "expected" from the perspective of the original developer. As a feature request, the work item would be to review all renderers and re-establish a new expected behavior wrt culture -- which is to say that it should respect whatever culture the main thread is using.

kingces95 · 2018-09-06T23:35:57Z

Xamarin.Forms.Core/BindableProperty.cs

@@ -317,7 +318,7 @@ internal bool TryConvert(ref object value)
 			TypeConverter typeConverterTo;
 			if (SimpleConvertTypes.TryGetValue(valueType, out convertableTo) && Array.IndexOf(convertableTo, type) != -1)
 			{
-				value = Convert.ChangeType(value, type);
+				value = Convert.ChangeType(value, type, System.Globalization.CultureInfo.CurrentCulture);


With any luck the feature "Use UI Thread Culture When Preforming Conversions" would only require changes in this function. Who knows, maybe this is the only spot! However, right below, we have a ConvertFromInvariantString so we'd have to go and understand if that also needs to be updated to take the UI culture.

Below seems to be a mistake, because the ToString method must use current culture. Therefore, we can't converted from an invariant culture. But there are known types that do not depend on localization and the mistake is not manifested.

kingces95 · 2018-09-06T23:41:04Z

Xamarin.Forms.Core/BindingExpression.cs

+				{
+					// see: https://bugzilla.xamarin.com/show_bug.cgi?id=32871
+					// do not canonicalize "*.[.]"; "1." should not update bound BindableProperty
+					if (stringValue.EndsWith("."))


This logic should probably be updated to test if the string value ends with the CurrentCulture decimal separator instead of the hard coded dot. Even if we stick with invariant culture we should replace the hardcoded dot with the NumberDecimalSeparator.

kingces95 · 2018-09-06T23:42:26Z

Xamarin.Forms.Core/BindingExpression.cs

+						throw new FormatException();
+
+					// to change locale if stringValue contains decimal separator for that locale
+					if (stringValue.IndexOf(CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator) != -1)


This should be removed; We shouldn't infer a culture by testing for culture specific values in the string. If we decide to allow non-invariant culture then we'll simply get the culture from the UI thread.

StephaneDelcroix

I'm with @kingces95 on this one. This has been discussed in the past, and decided to stick with the current behavior.

One of the risk I recalled by doing automagic conversion was the potentiality of corrupting one's string-based data model (think json).

@jassmith Please weight in, and decide to close or not.

jassmith · 2018-09-07T18:48:56Z

I agree with Chris here as well, we should be doing conversions with CurrentCulture. I haven't given this a full review but I thought this was supposed to simply make conversions work with comma cultures when one is active.

paymicro · 2018-09-10T09:19:11Z

I leave old "expected" behavior by default, nothing to break. To use current locale for a specific property, you also must enable it. Ex:. Label.TextProperty.UseCurrentCulture = true.

StephaneDelcroix · 2018-09-11T07:04:42Z

@paymicro Thanks for this feature. We appreciate the amount of time you spent on it, and understand your will and energy for making Xamarin.Forms easier to use for everyone.

Unfortunately, we need more time to think about this and evaluate all the consequences of those changes, and a pull-request is not the right place to discuss those.

So, I'm closing this PR. It's not a no, it's a not now.

Feel free to write a spec for your proposed change and we'll happily discuss it in an issue.

Pavel Yakovlev (Akvelon Inc) added 2 commits September 4, 2018 14:44

[Core] Fixes conversion binding a DateTime to a string if the locale …

819af34

…is not the US

added support for changing the decimal value

11da699

jassmith requested review from samhouts and kingces95 September 4, 2018 17:53

samhouts suggested changes Sep 5, 2018

View reviewed changes

StephaneDelcroix self-requested a review September 5, 2018 20:35

StephaneDelcroix self-assigned this Sep 5, 2018

Pavel Yakovlev added 3 commits September 6, 2018 16:58

fix getting decimal values from invariant string with non US locale

ccab2d4

minor refactoring UItest

97f9e34

Improved unit test

8e8d883

samhouts approved these changes Sep 6, 2018

View reviewed changes

kingces95 suggested changes Sep 6, 2018

View reviewed changes

StephaneDelcroix suggested changes Sep 7, 2018

View reviewed changes

addressing comments

1a22bb5

StephaneDelcroix closed this Sep 11, 2018

samhouts added this to the 3.2.0 milestone Sep 11, 2018

samhouts removed this from the 3.2.0 milestone Sep 12, 2018

samhouts added e/9 🕘 9 i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often m/high impact ⬛ labels Nov 2, 2018

samhouts mentioned this pull request May 10, 2019

Bound DateTimes Display in en-US Format Not Local Format #2049

Open

samhouts added the a/l10n label Jun 6, 2019

Crunch91 mentioned this pull request Jul 2, 2019

[Bug] Xamarin.Forms.Entry/Label does not diyplay decimal separator when binding a float to it. #6069

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Fixes conversion binding a DateTime to a string if the locale is not the US #3700

[Core] Fixes conversion binding a DateTime to a string if the locale is not the US #3700

paymicro commented Sep 4, 2018

jassmith commented Sep 5, 2018

samhouts left a comment

StephaneDelcroix commented Sep 5, 2018

jassmith commented Sep 6, 2018

samhouts Sep 6, 2018

samhouts Sep 6, 2018

kingces95 left a comment

kingces95 Sep 6, 2018

kingces95 Sep 6, 2018

paymicro Sep 10, 2018

kingces95 Sep 6, 2018

kingces95 Sep 6, 2018

StephaneDelcroix left a comment

jassmith commented Sep 7, 2018

paymicro commented Sep 10, 2018

StephaneDelcroix commented Sep 11, 2018

[Core] Fixes conversion binding a DateTime to a string if the locale is not the US #3700

[Core] Fixes conversion binding a DateTime to a string if the locale is not the US #3700

Conversation

paymicro commented Sep 4, 2018

Description of Change

Issues Resolved

API Changes

Platforms Affected

Behavioral/Visual Changes

Before/After Screenshots

Testing Procedure

PR Checklist

jassmith commented Sep 5, 2018

samhouts left a comment

Choose a reason for hiding this comment

StephaneDelcroix commented Sep 5, 2018

jassmith commented Sep 6, 2018

samhouts Sep 6, 2018

Choose a reason for hiding this comment

samhouts Sep 6, 2018

Choose a reason for hiding this comment

kingces95 left a comment

Choose a reason for hiding this comment

kingces95 Sep 6, 2018

Choose a reason for hiding this comment

kingces95 Sep 6, 2018

Choose a reason for hiding this comment

paymicro Sep 10, 2018

Choose a reason for hiding this comment

kingces95 Sep 6, 2018

Choose a reason for hiding this comment

kingces95 Sep 6, 2018

Choose a reason for hiding this comment

StephaneDelcroix left a comment

Choose a reason for hiding this comment

jassmith commented Sep 7, 2018

paymicro commented Sep 10, 2018

StephaneDelcroix commented Sep 11, 2018