[wasm] Use timezone abbreviations as fallback if full names don't exist #45385

tqiu8 · 2020-11-30T21:30:21Z

Uses Timezone abbreviations as a fallback if full name does not exist in the ICU. Added 2 browser tests for daylight name and standard names.

Dotnet-GitSync-Bot · 2020-11-30T21:30:25Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

tqiu8 · 2020-11-30T21:31:56Z

@EgorBo Currently this doesn't work yet because ucal_getTimeZoneDisplayName from the icu doesn't return anything. Is it possible that the timezone display names have been filtered out? All standard, generic, and display names are being returned as empty even though the result code is successful.

jkotas · 2020-12-01T00:04:34Z

src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems

    <Compile Include="$(MSBuildThisFileDirectory)System\IO\DriveInfoInternal.Browser.cs" />
    <Compile Include="$(MSBuildThisFileDirectory)System\IO\PersistedFiles.Browser.cs" />
-    <Compile Include="$(MSBuildThisFileDirectory)System\TimeZoneInfo.GetDisplayName.Invariant.cs" />


Delete this file since it is no longer used? Also, TimeZoneInfo.GetDisplayName.cs can be merged back into TimeZoneInfo.GetDisplayName.Unix.cs.

radical · 2020-12-01T15:00:27Z

This needs tests. And can you reference the issue, if any, that this is fixing?

lewing

It sounds like the invariant version was added when we didn't have full icu working. If browser is the only one using it we should go ahead and remove it. If you add a test we can verify it on desktop and compare to the wasm results.

… fixed regex

.. to abbreviations. [This code](https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.GetDisplayName.cs#L29-L40) seems to return `true` result, even though `timeZoneDisplayName` is null/empty. So, in such a case don't set the out var and let the abbrev get used as the fallback (like it already says in the comment).

radical · 2021-01-13T00:50:22Z

The code here isn't completely correct. Eg. for Newfoundland, the display name would be "(UTC-03:30) ", instead of "(UTC-03:30) NST".

I have reworked the tests a bit too. The final diff would be master...radical:rf-tz-displayname-fix , with my additional commit - radical@1805c6d

radical · 2021-01-12T21:36:03Z

src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems

-    <Compile Include="$(CommonPath)Interop\Interop.TimeZoneInfo.cs" Condition="'$(TargetsBrowser)' != 'true'">
+    <Compile Include="$(CommonPath)Interop\Interop.TimeZoneInfo.cs">


why is this changed needed?

Because we aren't using TimeZoneInfo.GetDisplayName.Invariant.cs anymore (will also remove that file in this PR) so Interop.Globalization is not defined for Browser

tqiu8 · 2021-01-13T01:00:58Z

@radical The regex in the original form is actually incorrect. "^\(UTC(\+|-)[0-9]{2}:[0-9]{2}\) \S.*" is looking for a space after (UTC-03:30). Even though ((UTC-03:30) may not contain any more text afterwards.

radical · 2021-01-13T01:05:54Z

@radical The regex in the original form is actually incorrect. "^\(UTC(\+|-)[0-9]{2}:[0-9]{2}\) \S.*" is looking for a space after (UTC-03:30). Even though ((UTC-03:30) may not contain any more text afterwards.

That was the case because the change in this PR set the display name (when falling back on abbreviation) after that string was constructed, so it ends up with nothing after the (UTC+03:30) - https://github.com/dotnet/runtime/pull/45385/files#diff-08facbdd1840bc8f2298563a79fe4565de02c874d72d7ac944890f8c4d200659R87-R93

See my changes, and the test: https://github.com/radical/runtime/blob/rf-tz-displayname-fix/src/libraries/System.Runtime/tests/System/TimeZoneInfoTests.cs#L90-L96

.. these have content after that space.

[wasm] If standard, or daylight names are not available, then fallback

lewing · 2021-02-18T04:29:06Z

The change makes sense, but are there any tests that check the daylight name on desktop?

tqiu8 · 2021-02-18T15:49:20Z

The change makes sense, but are there any tests that check the daylight name on desktop?

Only this one, so nothing comprehensive

lewing

Let's add tests for the non browser case too then

lewing · 2021-02-24T20:50:55Z

src/libraries/System.Runtime/tests/System/TimeZoneInfoTests.cs

+            };
+        }
+
+        [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsBrowser))]


Instead of checking PlatformDetection.IsBrowser here can you make a Platform_TimeZoneNamesTestData that returns different data for IsBrowser so that we are testing both cases and verify that we haven't regressed the normal case?

tqiu8 · 2021-03-04T18:00:10Z

Test failures unrelated to this change.

lewing

Nice!

lewing · 2021-03-05T01:13:20Z

src/libraries/System.Runtime/tests/System/TimeZoneInfoTests.cs

+
+        [Theory]
+        [MemberData(nameof(Platform_TimeZoneNamesTestData))]
+        [PlatformSpecific(TestPlatforms.AnyUnix)]


Does windows have names for these extra timezones?

Yes, assuming the OS platform language (not necessarily CurrentUICulture) is English, they would be:

{ TimeZoneInfo.FindSystemTimeZoneById(s_strPacific), "(UTC-08:00) Pacific Time (US & Canada)", "Pacific Standard Time", "Pacific Daylight Time" }, { TimeZoneInfo.FindSystemTimeZoneById(s_strSydney), "(UTC+10:00) Canberra, Melbourne, Sydney", "AUS Eastern Standard Time", "AUS Eastern Daylight Time" }, { TimeZoneInfo.FindSystemTimeZoneById(s_strPerth), "(UTC+08:00) Perth", "W. Australia Standard Time", "W. Australia Daylight Time" }, { TimeZoneInfo.FindSystemTimeZoneById(s_strIran), "(UTC+03:30) Tehran", "Iran Standard Time", "Iran Daylight Time" }, { s_NewfoundlandTz, "(UTC-03:30) Newfoundland", "Newfoundland Standard Time", "Newfoundland Daylight Time" }, { s_catamarcaTz, "(UTC-03:00) City of Buenos Aires", "Argentina Standard Time", "Argentina Daylight Time" }

mattjohnsonpint · 2021-03-05T16:50:17Z

src/libraries/System.Runtime/tests/System/TimeZoneInfoTests.cs

+                    { TimeZoneInfo.FindSystemTimeZoneById(s_strPacific), "(UTC-08:00) Pacific Standard Time", "Pacific Standard Time", "Pacific Daylight Time" },
+                    { TimeZoneInfo.FindSystemTimeZoneById(s_strSydney), "(UTC+10:00) Australian Eastern Standard Time", "Australian Eastern Standard Time", "Australian Eastern Daylight Time" },
+                    { TimeZoneInfo.FindSystemTimeZoneById(s_strPerth), "(UTC+08:00) Australian Western Standard Time", "Australian Western Standard Time", "Australian Western Daylight Time" },
+                    { TimeZoneInfo.FindSystemTimeZoneById(s_strIran), "(UTC+03:30) +0330", "+0330", "+0430" },


Shouldn't the last three be the full names from ICU?

{ TimeZoneInfo.FindSystemTimeZoneById(s_strIran), "(UTC+03:30) Iran Standard Time", "Iran Standard Time", "(UTC+03:30) Iran Standard Time" }, { s_NewfoundlandTz, "(UTC-03:30) Newfoundland Standard Time", "Newfoundland Standard Time", "(UTC-03:30) Newfoundland Standard Time" }, { s_catamarcaTz, "(UTC-03:00) Argentina Standard Time", "Argentina Standard Time", "(UTC-03:00) Argentina Standard Time" }

(Sorry I didn't see this earlier)

How did the test pass with the wrong values?

Are you referring to the last value in each test? They're supposed to be the DaylightNames.

No, I meant that in the non-browser version, the last three rows were not using the ICU names. I believe its due to the if (PlatformDetection.IsBrowser) in the test - this part of the test isn't being run. Per our offline discussion, I'll fix that up in my PR.

tarekgh · 2021-03-05T17:40:04Z

@mattjohnsonpint if you can fix such issues in your PR would be great. You are touching the same area anyway.

mattjohnsonpint · 2021-03-05T18:16:14Z

Will do.

lewing · 2021-03-05T20:25:25Z

Thanks for following up on that, I should have caught it.

use unix ver of GetDisplayName instead of invariant display name

eac0385

tqiu8 changed the title ~~use unix ver of GetDisplayName instead of invariant display name~~ [wasm] Use unix ver of GetDisplayName instead of invariant display name Nov 30, 2020

jkotas reviewed Dec 1, 2020

View reviewed changes

radical added the arch-wasm WebAssembly architecture label Dec 17, 2020

lewing reviewed Jan 6, 2021

View reviewed changes

tqiu8 added 4 commits January 6, 2021 14:33

Merge branch 'master' into tz-displayname-fix

a040a7e

Add tests for Daylight Names

429689e

Merge branch 'master' into tz-displayname-fix

93c0f35

Use tz abbreviations as fallback for browser cases, added test cases,…

9b4230a

… fixed regex

tqiu8 marked this pull request as ready for review January 12, 2021 19:59

tqiu8 changed the title ~~[wasm] Use unix ver of GetDisplayName instead of invariant display name~~ [wasm] Use timezone abbreviations as fallback if full names don't exist Jan 12, 2021

tqiu8 requested review from radical and EgorBo January 12, 2021 20:02

tqiu8 and others added 2 commits January 12, 2021 15:56

Move abbrev fields to TimeZoneInfo.Unix.cs

33ff298

radical reviewed Jan 13, 2021

View reviewed changes

Merge pull request #4 from radical/rf-tz-displayname-fix

c0d5599

[wasm] If standard, or daylight names are not available, then fallback

radical requested review from lewing and jkotas January 13, 2021 02:27

lewing requested a review from tarekgh January 21, 2021 20:59

lewing reviewed Feb 24, 2021

View reviewed changes

Base automatically changed from master to main March 1, 2021 09:07

lewing mentioned this pull request Mar 2, 2021

Improve time zone display names on Unix #48931

Merged

tqiu8 added 2 commits March 3, 2021 14:00

add additional platform tests

8478bee

edit displayname test

3524f6a

tqiu8 requested review from lewing and radical March 4, 2021 18:00

lewing approved these changes Mar 5, 2021

View reviewed changes

lewing merged commit 5d6c8aa into dotnet:main Mar 5, 2021

mattjohnsonpint reviewed Mar 5, 2021

View reviewed changes

ghost locked as resolved and limited conversation to collaborators Apr 4, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wasm] Use timezone abbreviations as fallback if full names don't exist #45385

[wasm] Use timezone abbreviations as fallback if full names don't exist #45385

tqiu8 commented Nov 30, 2020 •

edited

Loading

Dotnet-GitSync-Bot commented Nov 30, 2020

tqiu8 commented Nov 30, 2020

jkotas Dec 1, 2020

radical commented Dec 1, 2020

lewing left a comment

radical commented Jan 13, 2021

radical Jan 12, 2021

tqiu8 Jan 13, 2021

tqiu8 commented Jan 13, 2021

radical commented Jan 13, 2021

lewing commented Feb 18, 2021

tqiu8 commented Feb 18, 2021

lewing left a comment

lewing Feb 24, 2021

tqiu8 commented Mar 4, 2021

lewing left a comment

lewing Mar 5, 2021

mattjohnsonpint Mar 5, 2021 •

edited

Loading

mattjohnsonpint Mar 5, 2021 •

edited

Loading

mattjohnsonpint Mar 5, 2021

tqiu8 Mar 5, 2021

mattjohnsonpint Mar 5, 2021

tarekgh commented Mar 5, 2021

mattjohnsonpint commented Mar 5, 2021

lewing commented Mar 5, 2021

		<Compile Include="$(CommonPath)Interop\Interop.TimeZoneInfo.cs" Condition="'$(TargetsBrowser)' != 'true'">
		<Compile Include="$(CommonPath)Interop\Interop.TimeZoneInfo.cs">

[wasm] Use timezone abbreviations as fallback if full names don't exist #45385

[wasm] Use timezone abbreviations as fallback if full names don't exist #45385

Conversation

tqiu8 commented Nov 30, 2020 • edited Loading

Dotnet-GitSync-Bot commented Nov 30, 2020

tqiu8 commented Nov 30, 2020

Choose a reason for hiding this comment

radical commented Dec 1, 2020

lewing left a comment

Choose a reason for hiding this comment

radical commented Jan 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqiu8 commented Jan 13, 2021

radical commented Jan 13, 2021

lewing commented Feb 18, 2021

tqiu8 commented Feb 18, 2021

lewing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqiu8 commented Mar 4, 2021

lewing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattjohnsonpint Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

mattjohnsonpint Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh commented Mar 5, 2021

mattjohnsonpint commented Mar 5, 2021

lewing commented Mar 5, 2021

tqiu8 commented Nov 30, 2020 •

edited

Loading

mattjohnsonpint Mar 5, 2021 •

edited

Loading

mattjohnsonpint Mar 5, 2021 •

edited

Loading