CultureInfo should independent from local computer settings (Lombiq Technologies: OCORE-86)

[hishamco]

Fixes #11228

[hishamco]

Seems I need to rename LocalizationOptions to avoid naming collision

[sebastienros]

Will review it later

[hishamco]

@sebastienros I just use CultureSettings enum to make clear, but I never mind to replace it with a boolean value, one more thing I just moved CultureLocalizationOptions into OC.Localization.Abstractions all the options that we are using are placed in their modules, but while this kind of option could be use in any localization implementation I think it's fine to place it in OC.Localization.Abstractions rather than OC.Abstractions. Another thing I suggest to use the system culture instead of user-selected culture by default, similar to what CultureInfo is using

If sound looks good I will remove everything I added related to ILocalizationService

[jtkech]

@hishamco

Take care, about CultureInfo the user is not an OC user but an user at the machine level, that's why we were talking about System settings.

There are 2 ctors CultureInfo(string name) and CultureInfo(string name, bool useUserOverride), the first one is the same as using CultureInfo(name, useUserOverride: true), so if we assume that the default is when using the first ctor, the default is then useUserOverride = true.

So the 2 defined values of the current enum Default, User is confusing because of the above. Also, I have nothing against using enum but not when it is to retrieve a value to be used as a bool.

So to not be confused with the "user" we could name this option field UseSystemSettings whose default would be true, as I remember @sebastienros suggested IgnoreSystemSettings but whose default value would be false and we would need to pass useUserOverride = !option.IgnoreSystemSettings.

Not sure we need to bind this IOptions to the configuration (e.g. from appsettins.json), if so and if we bind this field individually, if we use UseSystemSettings we would need to pass to the GetValue() a default value of true, if we bind the whole IOptions class (as we do many time) we just need to specify in the IOptions class a default value of true for this field.

Or if we use IgnoreSystemSettings, as said above, we would just need to pass useUserOverride = !option.IgnoreSystemSettings

Maybe better to name this IOptions CultureOptions and define it in OC.Abstractions, where we define CultureScope, for example this IOptions would be used each time we use CultureScope.

[hishamco]

Take care, about CultureInfo the user is not an OC user but an user at the machine level, that's why we were talking about System settings.

Sure

There are 2 ctors CultureInfo(string name) and CultureInfo(string name, bool useUserOverride), the first one is the same as using CultureInfo(name, useUserOverride: true), so if we assume that the default is when using the first ctor, the default is then useUserOverride = true.

Oops, seem I made a mistake here, I presumed it's a default culture by default

Or if we use IgnoreSystemSettings, as said above, we would just need to pass useUserOverride = !option.IgnoreSystemSettings

I never mind with this option as I mentioned above

Maybe better to name this IOptions CultureOptions and define it in OC.Abstractions, where we define CultureScope, for example this IOptions would be used each time we use CultureScope.

It was there before, but while all the modules adding the options in module project and CultureScope is using CultureOptions that's why I move it into OC.Localization.Abstractions

[jtkech]

@hishamco

@jtkech seems we forgot to apply the same settings in SetDefaultCulture()

Yes as I mentioned here #11228 (comment)

one more thing that I was did it right but you confused me ;) may be because the naming

Just saw that you now use new CultureInfo(culture, ignoreSystemSettings) which is wrong as by default when just using new CultureInfo(culture) it doesn't ignore system settings, so if it was not working it would mean that something else is wrong. But never sure at 100%, will look at it tonight.

[hishamco]

Yes as I mentioned here #11228 (comment)

May be I didn't notice that, coz all the time I'm trying to focus on the PR comment, sorry for that

Just saw that you now use new CultureInfo(culture, ignoreSystemSettings) which is wrong as by default when just using

It's not wrong because the default value of ignoreSystemSettings is false which means means don't use user override, Am I right?

just using new CultureInfo(culture) it doesn't ignore system settings,

Because the useUserOverride is true, in our case if ignoreSystemSettings is true it should ignore the default settings

https://referencesource.microsoft.com/#mscorlib/system/globalization/cultureinfo.cs,325

[jtkech]

Yes I know https://github.com/dotnet/runtime/blob/5481c4bfe6ced9000c05692163a019739c912677/src/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.cs#L158-L162

Because the useUserOverride is true, in our case if ignoreSystemSettings is true it should ignore the default settings

Because the useUserOverride is true by default, meaning it will use system settings, and because ignoreSystemSettings is false by default (if not configured/overridden), we should use new CultureInfo(culture, useUserOverride: !ignoreSystemSettings), so that it works as before if the option value is not configured (e.g. in appsettings) and not overridden by code (e.g. in a s.Configure<>()).

So if IgnoreSystemSettings is configured/overridden to true (which is not the default) the result will be new CultureInfo(culture, useUserOverride: false) which is right as here we want to ignore system settings, so we don't want to use user override (meaning we don't want to use system settings).

[hishamco]

Nooo, you are right Jean, I just confused by the CultureInfo docs

so we don't want to use user override (meaning we don't want to use system settings).

That's the point, again sorry for inconvience and ignore my above comments

[hishamco]

Believe it or not the IgnoreSystemSettings is confusing especially we you read the CultureInfo(string, bool) docs ;)

[hishamco]

I need to do a final quick test again

[jtkech]

Believe it or not the IgnoreSystemSettings is confusing especially we you read the CultureInfo(string, bool) docs ;)

Yes I agree ;)

[hishamco]

Everythings looks as expected

[Piedone]

Nice work!

[hishamco]

FYI I created a PR in aspnet repo while I'm working on this feature, finally it got merge. I might need to open a draft PR to change the API with the one that will be built-in in .NET 8.0

dotnet/aspnetcore#44282

[Piedone]

Great!

[alafi]

I believe this PR introduced this issue #12610

[hishamco]

Thanks @alafi I will check the related issue