Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect the default locale name during startup on Apple platforms #68706

Merged
merged 8 commits into from
May 5, 2022

Conversation

steveisok
Copy link
Member

This change adds a function to lookup the current NSLocale and extract the language + country code to load into ICU by default. Previously, we would defer to uloc_getDefault in ICU, which would return a value we would ignore (en_US_POSIX) and result in falling back to invariant mode.

Fixes #68321

@ghost
Copy link

ghost commented Apr 29, 2022

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

This change adds a function to lookup the current NSLocale and extract the language + country code to load into ICU by default. Previously, we would defer to uloc_getDefault in ICU, which would return a value we would ignore (en_US_POSIX) and result in falling back to invariant mode.

Fixes #68321

Author: steveisok
Assignees: -
Labels:

area-System.Globalization

Milestone: -

@steveisok steveisok requested a review from akoeplinger April 29, 2022 16:54
This change adds a function to lookup the current NSLocale and extract the language + country code to load into ICU by default.  Previously, we would defer to uloc_getDefault in ICU, which would return a value we would ignore (en_US_POSIX) and result in falling back to invariant mode.

Fixes dotnet#68321
@steveisok steveisok force-pushed the default-culture-ios branch from 7ee4f55 to 2f780a4 Compare April 29, 2022 16:55
@steveisok steveisok requested a review from rolfbjarne April 29, 2022 16:56
@steveisok
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

return strdup([localeName UTF8String]);
}

if ([currentLocale.languageCode length] > 0 && [currentLocale.countryCode length] > 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currentLocale

do you know if possible having a collation parts too? I mean ICU can have a locale name like:

de_DE@collation=phoneb which will be translated to .NET name as de-DE_phoneb.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, I would suggest getting localeIdentifier then normalize it to the .NET culture name form.

@steveisok
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveisok
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveisok
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -33,6 +33,14 @@ public void CurrentCulture()
}
}

[Fact]
[PlatformSpecific(TestPlatforms.OSX | TestPlatforms.iOS | TestPlatforms.MacCatalyst | TestPlatforms.tvOS)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not going to pass on windows and Linux too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to fail on Linux and Windows. On Windows it is unlikely the users will set the user locale to Invariant but it is possible. For Linux, some distros set the locale to "C" which we replace it with Invariant. That means it is likely to fail on Linux.

@steveisok
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveisok
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveisok
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@maryamariyan maryamariyan added the os-mac-os-x macOS aka OSX label May 3, 2022
@steveisok steveisok force-pushed the default-culture-ios branch from b08d2af to 5f367d5 Compare May 3, 2022 21:46
@steveisok
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveisok
Copy link
Member Author

@tarekgh can you please give this another review?

@steveisok steveisok force-pushed the default-culture-ios branch from 8200a40 to 28cd339 Compare May 5, 2022 17:41
Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @steveisok. LGTM!

@steveisok
Copy link
Member Author

Failure is unrelated #68932

@steveisok steveisok merged commit 45887d1 into dotnet:main May 5, 2022
@steveisok steveisok deleted the default-culture-ios branch May 5, 2022 21:13
@steveisok
Copy link
Member Author

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2022

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/2278294189

@ghost ghost locked as resolved and limited conversation to collaborators Jun 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

macOS / Mac Catalyst / iOS: CurrentThread.Current[UI]Culture is always the invariant culture
6 participants