-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Allow TimeZoneInfo display names to use any of the installed Windows languages #52992
Conversation
Tagging subscribers to this area: @tannergooding Issue Details
Since the data comes from the NLS resources installed with Windows language packs, we still can't support all languages on Windows. However, with this change we can now support any that are installed. If the Note, the unit test will only run if there are at least two language packs installed that have different root languages, as we need to compare that lang1's values are different than lang2's. I'm not sure if there are any machines in Helix set up like this, but the test does work on my local machine with English and French installed. Fixes #50310
|
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Win32.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Win32.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Win32.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Win32.cs
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mattjohnsonpint. LGTM
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs
Outdated
Show resolved
Hide resolved
Remaining failures appear unrelated. |
@mattjohnsonpint I have restarted the failed tests. |
I opened the issue #53110 to track the unrelated failure in the CI leg |
@tarekgh This failure was introduced by this PR. It is caused by interop definitions in WindowsUILanguageHelper. You may argue that the runtime should not assert and I agree, but that is not good enough justification for merging a change that will cause every PR to be red. |
Can you please explain further? I will fix asap. |
Sorry I overlooked the failure. @mattjohnsonpint is it ok to revert the changes and resubmit it with the fix? |
…Windows languages (dotnet#52992)" This reverts commit b54dbaf.
@@ -36,6 +36,9 @@ public sealed partial class TimeZoneInfo | |||
private const int MaxKeyLength = 255; | |||
private const string InvariantUtcStandardDisplayName = "Coordinated Universal Time"; | |||
|
|||
private static readonly Dictionary<string, string> s_FileMuiPathCache = new(); | |||
private static readonly TimeZoneInfo s_utcTimeZone = CreateUtcTimeZone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a TimeZoneInfo.cs file. This private static readonly TimeZoneInfo s_utcTimeZone = CreateUtcTimeZone();
is now declared in both TimeZoneInfo.Unix.cs and TimeZoneInfo.Win32.cs. Can this not be deduplicated into the shared TimeZoneInfo.cs file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one was duplicated intentionally, as the mui cache has to be initialized first, and there's no sequence guaranteed for static initialization across partial class files. The alternative was to initialize the mui cache in the main file with a #if TARGET_WINDOWS
around it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one was duplicated intentionally, as the mui cache has to be initialized first, and there's no sequence guaranteed for static initialization across partial class files.
Ok... then it's missing a comment explaining that ;-)
@@ -36,6 +36,9 @@ public sealed partial class TimeZoneInfo | |||
private const int MaxKeyLength = 255; | |||
private const string InvariantUtcStandardDisplayName = "Coordinated Universal Time"; | |||
|
|||
private static readonly Dictionary<string, string> s_FileMuiPathCache = new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: s_fileMuiPathCache
var lang = new string(language); | ||
if (string.Equals(lang, cultureInfo.Name, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need to allocate the string just to do this comparison. Presumably exact match is common and we'd want to delay the creation of lang until after this if block? This can instead be:
ReadOnlySpan<char> lang = MemoryMarshal.CreateReadOnlySpanFromNullTerminated(language);
if (lang.Equals(cultureInfo.Name, StringComparison.OrdinalIgnoreCase)
Then below if we need lang for the GetCultureInfo call, that can just be CultureInfo.GetCultureInfo(lang.ToString());
.
|
||
if (succeeded) | ||
{ | ||
fileMuiPath[Interop.Kernel32.MAX_PATH] = '\0'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line exists in multiple places here and below. It would be safer and simpler to just dedup that into a single line above just after the GetFileMUIPath call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's even needed. The API always returns a null terminated string. Tarek asked for it just to be safe. Note, the two length values from the API are useless - they are the buffer lengths, not the values lengths.
return new string(fileMuiPath); | ||
} | ||
|
||
// Shouldn't get here, as there's always at least one language installed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would ideally be a Debug.Fail.
TimeZoneInfo
on Windows previously was locked to using only the OS default UI language for itsDisplayName
,StandardName
andDaylightName
properties (contrast with Unix where it will use theCultureInfo.CurrentUICulture
).Since the data comes from the NLS resources installed with Windows language packs, we still can't support all languages on Windows. However, with this change we can now support any that are installed. If the
CurrentUICulture
matches one of the languages installed (e.g.fr-FR
=fr-FR
) it will use it. Otherwise, it will search for an installed language with the same parent language (e.g.fr-CA
≈fr-FR
) to use. Finally, it will fallback to the existing behavior of using the default language.Note, the unit test will only run if there are at least two languages installed that have different root languages, as we need to compare that lang1's values are different than lang2's. I'm not sure if there are any machines in Helix set up like this, but the test does work on my local machine with English and French installed.
Fixes #50310
@tarekgh