IANA To/From Windows Ids Conversion APIs #51093
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @tarekgh, @safern Issue DetailsFixes #49407
|
|
@marek-safar @eerhardt @lewing could you please advise what is wrong here? This is browser/AOT/Mono error. do we need to add something to Mono's runtime or should I exclude it? |
|
Browser configurations do not contain the time zone data and hence the Interop file is also excluded: This was already mentioned in one of the issues related to time zone names. |
See This falls into the same category as #50210. We need to make sure we aren't regressing size on Blazor WASM when we are making new calls to ICU. |
We'd be happy to support this optionally but the size impact is large enough that it we probably can't do it by default. Just referencing the symbols pulled in 40k of compressed runtime size in #50210 and @tqiu8 can weigh in with the actual zone data sizes but if memory serves even just "en" was huge. |
|
@eerhardt I have addressed the feedback. do you have any more comments or we are good to go? |
|
@lewing - not sure exactly what changes would be required for packaging, but this feature would only need the |
|
Oh wait - it would also need to be able to resolve aliases. In CLDR, that's done in |
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/Native/Unix/System.Globalization.Native/pal_timeZoneInfo.c
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.FullGlobalizationData.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.FullGlobalizationData.cs
Show resolved
Hide resolved
| GetDisplayName(UtcId, Interop.Globalization.TimeZoneDisplayNameType.Standard, uiCulture.Name, ref standardDisplayName); | ||
|
|
||
| // Final safety check. Don't allow null or abbreviations | ||
| if (standardDisplayName == null || standardDisplayName == "GMT" || standardDisplayName == "UTC") |
There was a problem hiding this comment.
I see in some place such culture sensitive string comparisons. Make sense to use ordinal?
(Below I see if (windowsId.Equals("utc", StringComparison.OrdinalIgnoreCase)))
| { | ||
| // Try to fallback using FallbackCultureName just in case we can make it work. | ||
| result = Interop.CallStringMethod( | ||
| (buffer, locale, id, type) => |
There was a problem hiding this comment.
Can we set static?
| (buffer, locale, id, type) => | |
| static (buffer, locale, id, type) => |
|
|
||
| string? timeZoneDisplayName; | ||
| bool result = Interop.CallStringMethod( | ||
| (buffer, locale, id, type) => |
There was a problem hiding this comment.
Can we set static?
| (buffer, locale, id, type) => | |
| static (buffer, locale, id, type) => |
| } | ||
|
|
||
| // Helper function that retrieves various forms of time zone display names from ICU | ||
| private static unsafe void GetDisplayName(string timeZoneId, Interop.Globalization.TimeZoneDisplayNameType nameType, string uiCulture, ref string? displayName) |
There was a problem hiding this comment.
I wonder why we use ref string? displayName. Can it be "out":
| private static unsafe void GetDisplayName(string timeZoneId, Interop.Globalization.TimeZoneDisplayNameType nameType, string uiCulture, ref string? displayName) | |
| private static unsafe void GetDisplayName(string timeZoneId, Interop.Globalization.TimeZoneDisplayNameType nameType, string uiCulture, out string? displayName) |
|
|
||
| // See if we should include the exemplar city name. | ||
| string exemplarCityName = GetExemplarCityName(timeZoneId, uiCulture.Name); | ||
| if (uiCulture.CompareInfo.IndexOf(genericName, exemplarCityName, CompareOptions.IgnoreCase | CompareOptions.IgnoreNonSpace) >= 0 && genericLocationName != null) |
There was a problem hiding this comment.
Maybe swap conditions:
| if (uiCulture.CompareInfo.IndexOf(genericName, exemplarCityName, CompareOptions.IgnoreCase | CompareOptions.IgnoreNonSpace) >= 0 && genericLocationName != null) | |
| if (genericLocationName != null && uiCulture.CompareInfo.IndexOf(genericName, exemplarCityName, CompareOptions.IgnoreCase | CompareOptions.IgnoreNonSpace) >= 0) |
| } | ||
|
|
||
| windowsId = null; |
There was a problem hiding this comment.
All such initializations from all code paths in the method could be removed and one could be added on top of the method.
| } | ||
|
|
||
| windowsId = null; | ||
| return false; |
There was a problem hiding this comment.
To continue previous comment we could make the code more readable:
| return false; | |
| if (allocate && length > 0) | |
| { | |
| windowsId = new string(buffer, 0, length); | |
| } | |
| return length > 0; |
| { | ||
| // When an exemplar city is already part of the generic name, | ||
| // there's no need to repeat it again so just use the generic name. | ||
|
|
||
| // *** Example (fr-FR) *** | ||
| // id = "Australia/Lord_Howe" | ||
| // baseOffsetText = "(UTC+10:30)" | ||
| // standardName = "heure normale de Lord Howe" | ||
| // genericName = "heure de Lord Howe" | ||
| // genericLocationName = "heure : Lord Howe" | ||
| // exemplarCityName = "Lord Howe" | ||
| // displayName = "(UTC+10:30) heure de Lord Howe" | ||
|
|
||
| displayName = $"{baseOffsetText} {genericName}"; | ||
| } | ||
| else | ||
| char* buffer = stackalloc char[100]; | ||
| int length = Interop.Globalization.WindowsIdToIanaId(windowsId, region, buffer, 100); | ||
| if (length > 0) | ||
| { | ||
| // Finally, use the generic name and the exemplar city together. | ||
| // This provides an intuitive name and still disambiguates. | ||
|
|
||
| // *** Example (en-US) *** | ||
| // id = "Europe/Rome" | ||
| // baseOffsetText = "(UTC+01:00)" | ||
| // standardName = "Central European Standard Time" | ||
| // genericName = "Central European Time" | ||
| // genericLocationName = "Italy Time" | ||
| // exemplarCityName = "Rome" | ||
| // displayName = "(UTC+01:00) Central European Time (Rome)" | ||
|
|
||
| displayName = $"{baseOffsetText} {genericName} ({exemplarCityName})"; | ||
| ianaId = allocate ? new string(buffer, 0, length) : null; | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| // Helper function that gets an exmplar city name either from ICU or from the IANA time zone ID itself | ||
| private static string GetExemplarCityName(string timeZoneId, string uiCultureName) | ||
| { | ||
| // First try to get the name through the localization data. | ||
| string? exemplarCityName = null; | ||
| GetDisplayName(timeZoneId, Interop.Globalization.TimeZoneDisplayNameType.ExemplarCity, uiCultureName, ref exemplarCityName); | ||
| if (!string.IsNullOrEmpty(exemplarCityName)) | ||
| return exemplarCityName; | ||
|
|
||
| // Support for getting exemplar city names was added in ICU 51. | ||
| // We may have an older version. For example, in Helix we test on RHEL 7.5 which uses ICU 50.1.2. | ||
| // We'll fallback to using an English name generated from the time zone ID. | ||
| int i = timeZoneId.LastIndexOf('/'); | ||
| return timeZoneId.Substring(i + 1).Replace('_', ' '); | ||
| ianaId = null; | ||
| return false; |
There was a problem hiding this comment.
The same. ianaId = null could be on top of the method and:
if (allocate && length > 0)
{
ianaId = new string(buffer, 0, length);
}
return length > 0;
|
Oops! ^-) Already merged. |
ghost
locked as resolved and limited conversation to collaborators
Fixes #49407