Improve time zone display names on Unix #48931
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Tagging subscribers to this area: @tarekgh, @safern Issue DetailsThis PR tackles the problem described in #16232, addressing a long-unresolved TODO comment in the Unix implementation of setting the value for While its still true that ICU doesn't expose the generic name via the
In my implementation, each is retrieved separately in native code, then they are combined with the base UTC offset in managed code. They are combined in various ways to produce a final Even with access to these strings, there are several edge cases that have to be handled. Most are in the managed code in Below are the time zones before and after this change (Ubuntu 20.10, ICU 67.1-4, en-US). Observe from the "before" list, the problem of not being able to distinguish one time zone from another by looking only at the I also addressed a related issue, which was that the UTC time zone not using localized display names, but was fixed to the English "Coordinated Universal Time" on both Unix and Windows. It is now localized using the same locale as the other names (using the OS language on Windows, and using Before(click to expand)
After(click to expand)
The above lists were generated with this simple program: using System;
using System.Linq;
namespace ListTimeZones
{
class Program
{
static void Main(string[] args)
{
var timeZones = TimeZoneInfo.GetSystemTimeZones();
int col1Size = timeZones.Max(x => x.Id.Length);
int col2Size = timeZones.Max(x => x.DisplayName.Length);
Console.WriteLine("| " + "Id".PadRight(col1Size) + " | " + "Display Name".PadRight(col2Size) + " |");
Console.WriteLine("| " + new string('-', col1Size) + " | " + new string('-', col2Size) + " |");
foreach (TimeZoneInfo tzi in timeZones)
{
Console.WriteLine($"| {tzi.Id.PadRight(col1Size)} | {tzi.DisplayName.PadRight(col2Size)} |");
}
Console.WriteLine();
}
}
}
|
tarekgh
reviewed
Mar 2, 2021
src/libraries/Native/Unix/System.Globalization.Native/pal_timeZoneInfo.c
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Mar 2, 2021
src/libraries/Native/Unix/System.Globalization.Native/pal_timeZoneInfo.c
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Mar 2, 2021
src/libraries/Native/Unix/System.Globalization.Native/pal_timeZoneInfo.c
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Mar 2, 2021
src/libraries/Native/Unix/System.Globalization.Native/pal_timeZoneInfo.c
Outdated
Show resolved
Hide resolved
|
cc @tqiu8 |
tarekgh
reviewed
Mar 2, 2021
src/libraries/Native/Unix/System.Globalization.Native/pal_timeZoneInfo.c
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Mar 2, 2021
src/libraries/Native/Unix/System.Globalization.Native/pal_timeZoneInfo.c
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Mar 2, 2021
src/libraries/Native/Unix/System.Globalization.Native/pal_timeZoneInfo.c
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Mar 2, 2021
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Mar 2, 2021
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs
Outdated
Show resolved
Hide resolved
|
Just curious as I am not knowledgeable about timezones -- what would be the impact of the user picking the wrong one of the several Alaska time zones? They are all -9:00. I'm not sure which -8:00 I should pick for Seattle, even when they are marked Los Angeles and Vancouver. |
|
Looks like I have some platform-specific build failures to resolve also. |
@danmoseley - Indeed, that's a general issue with time zones - there are quite a lot of them. The reason has to do with historical accuracy. IANA time zones aim to be accurate from 1970 forward, thus any variation in local time between nearby locations since 1970 get a distinct time zone. For Alaska, the only two that matter today are For Pacific time, there are three. Vancouver for Canada, Los Angeles for the USA, and Tijuana for Mexico. They too have had some historical differences. Thus, the impact if a user choses the wrong time zone is that they get the changes in UTC offsets and daylight start/stop transition dates for the entire range of that time zone. If they are only working with "now" or nearby dates, that might not be noticed at all. If they are working with older dates, they could be off by an hour or more depending on the date in question. The same thing plays out with regard to any anticipated upcoming changes for future dates, if those changes are accounted for in their respective time zones. Since the only API we have presently is Incidentally, legacy Windows registry-based time zones are fewer in number only because originally Windows hadn't set any particular consistency date (like IANA's 1970). That changed in 2016 when we formalized some of Microsoft's time zone policies and set a consistency date of 2010. If they had aimed for 1970 and covering the world, there would be just as many as IANA. |
tarekgh
reviewed
Mar 2, 2021
|
@lewing could you please advise if referencing the extra ICU methods is fine with wasm and other mobile platforms? |
4 tasks
|
Are there any tests we can write for this new functionality? |
eerhardt
reviewed
Mar 2, 2021
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.GetDisplayName.Invariant.cs
Outdated
Show resolved
Hide resolved
eerhardt
reviewed
Mar 2, 2021
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs
Outdated
Show resolved
Hide resolved
eerhardt
reviewed
Mar 2, 2021
src/libraries/Native/Unix/System.Globalization.Native/pal_timeZoneInfo.c
Outdated
Show resolved
Hide resolved
Yes. I will add some. Thanks for calling that out. 👍 |
mattjohnsonpint
force-pushed
the
mattjoh/timezoneinfo-displayname-unix
branch
from
719c8d1
to
4200b75
Compare
It looks like this overlaps with #45385 for browser which is trying to get something to display when the display name doesn't exist in icu. From a quick scan my main concern is that we'd end up pulling in more unmanaged symbols (and size) to query for data that is unlikely to be in the icu data. @tqiu8 can you take a look? |
I thought that was already handled, but I guess not. The changes in #45385 look good to me. IMHO, it should go first and then I can rebase this on top of that and adjust. |
mattjohnsonpint
force-pushed
the
mattjoh/timezoneinfo-displayname-unix
branch
2 times, most recently
from
a1701e4
to
2fdfaea
Compare
tarekgh
reviewed
Mar 23, 2021
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Mar 23, 2021
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs
Outdated
Show resolved
Hide resolved
eerhardt
reviewed
Mar 23, 2021
safern
reviewed
Mar 23, 2021
src/libraries/Native/Unix/System.Globalization.Native/pal_timeZoneInfo.c
Show resolved
Hide resolved
tarekgh
reviewed
Mar 23, 2021
I am not getting why we have this condition? we are executing the method with remote executer any way and we force Invariant to the UI culture. This doesn't look right to condition it. I know this not your change but I think we can remove this condition and keep only the remote execution condition. Refers to: src/libraries/System.Runtime/tests/System/TimeZoneInfoTests.cs:2542 in 50d0fd2. [](commit_id = 50d0fd2, deletion_comment = False) |
tarekgh
reviewed
Mar 23, 2021
I think this is specifically about proper handling of The English condition is again about being locked to the OS language on Windows. |
tarekgh
approved these changes
Mar 23, 2021
|
Thanks @mattjohnsonpint for all work provided here. |
|
+1 thank you @mattjohnsonpint for this significant effort. |
|
Seems like this possibly caused a size regression in dotnet.wasm |
This was referenced Mar 25, 2021
spouliot
pushed a commit
to xamarin/xamarin-macios
that referenced
this pull request
Apr 19, 2021
spouliot
added a commit
to xamarin/xamarin-macios
that referenced
this pull request
Apr 20, 2021
* Update dependencies from https://github.com/dotnet/installer build 20210408.1 Microsoft.Dotnet.Sdk.Internal From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21208.1 * Update dependencies from https://github.com/dotnet/installer build 20210409.4 Microsoft.Dotnet.Sdk.Internal From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21209.4 * Update dependencies from https://github.com/dotnet/installer build 20210410.1 Microsoft.Dotnet.Sdk.Internal From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21210.1 * same P4 specific fix as ccb43cb but the ICU support was added based on P3 but merged after ^ * Update dependencies from https://github.com/dotnet/installer build 20210412.5 Microsoft.Dotnet.Sdk.Internal From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21212.5 * Update dependencies from https://github.com/dotnet/installer build 20210413.70 Microsoft.Dotnet.Sdk.Internal From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21213.70 * Update dependencies from https://github.com/dotnet/installer build 20210414.14 Microsoft.Dotnet.Sdk.Internal From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21214.14 * Update to new package names Thanks @pjcollins for the heads up #11175 (comment) * Update dependencies from https://github.com/dotnet/installer build 20210415.1 Microsoft.Dotnet.Sdk.Internal From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21215.1 * Fix build (path changed to include '.mono') * remove more '.mono' special case that are not needed anymore * Update dependencies from https://github.com/dotnet/installer build 20210415.12 Microsoft.Dotnet.Sdk.Internal From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21215.12 * Fix building apps (it now finds the native libs) Credits to @filipnavara filipnavara@8325f8d * Add back IsTrimmable (or nothing gets linked) * Update dependencies from https://github.com/dotnet/installer build 20210418.6 Microsoft.Dotnet.Sdk.Internal From Version 6.0.100-preview.3.21202.5 -> To Version 6.0.100-preview.4.21218.6 * Keep downloading the CoreCLR runtime packs. * [runtime] Adjust the build to link with the correct runtime library for CoreCLR. * [tests][monotouch-test] Ignore NSTimeZoneTest / All_28300 on dotnet as it hangs Introduced with dotnet/runtime#48931 Issue https://unicode-org.atlassian.net/browse/ICU-21591 PR unicode-org/icu#1699 * [dotnet][msbuild] Add more (missing) '\' Fix satellite/location assemblies and some unit tests Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Alex Soto <[email protected]> Co-authored-by: Manuel de la Pena <[email protected]> Co-authored-by: Sebastien Pouliot <[email protected]> Co-authored-by: Sebastien Pouliot <[email protected]> Co-authored-by: Rolf Bjarne Kvinge <[email protected]>
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR tackles the problem described in #16232, addressing a long-unresolved TODO comment in the Unix implementation of setting the value for
TimeZoneInfo.DisplayName.While its still true that ICU doesn't expose the generic name via the
ucal_getTimeZoneDisplayNamefunction, we can get at various forms of the generic name via theudat_formatfunction instead, by formatting the a datetime with patterns. The ICU date field symbol table gives several different patterns that are useful to us:vvvv- The generic non-location time zone name (ex: "Pacific Time")VVVV- The generic time zone location name (ex: "Los Angeles Time")VVV- The exemplar city associated with the time zone (ex: "Los Angeles")In my implementation, each is retrieved separately in native code, then they are combined with the base UTC offset in managed code. They are combined in various ways to produce a final
DisplayNamefor the time zone that is unique, discoverable, and intuitive when presenting a list.Even with access to these strings, there are several edge cases that have to be handled. Most are in the managed code in
TimeZoneInfo.Unix.cs, and one is handled in native code inpal_timeZoneInfo.c. They are documented in the code comments.Below are the time zones before and after this change (Ubuntu 20.10, ICU 67.1-4, en-US). Observe from the "before" list, the problem of not being able to distinguish one time zone from another by looking only at the
DisplayName. For example, there were 13 different time zones that had the same string(UTC-07:00) Mountain Standard Time. That was very problematic for an end-user to choose a time zone.I also addressed a related issue, which was that the UTC time zone not using localized display names, but was fixed to the English "Coordinated Universal Time" on both Unix and Windows. It is now localized using the same locale as the other names (using the OS language on Windows, and using
CurrentUICultureon Unix).Before
(click to expand)
After
(click to expand)
The above lists were generated with this simple program: