-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[wasm] Use timezone abbreviations as fallback if full names don't exist #45385
Conversation
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. |
@EgorBo Currently this doesn't work yet because |
<Compile Include="$(MSBuildThisFileDirectory)System\IO\DriveInfoInternal.Browser.cs" /> | ||
<Compile Include="$(MSBuildThisFileDirectory)System\IO\PersistedFiles.Browser.cs" /> | ||
<Compile Include="$(MSBuildThisFileDirectory)System\TimeZoneInfo.GetDisplayName.Invariant.cs" /> |
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.
Delete this file since it is no longer used? Also, TimeZoneInfo.GetDisplayName.cs
can be merged back into TimeZoneInfo.GetDisplayName.Unix.cs
.
This needs tests. And can you reference the issue, if any, that this is fixing? |
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.
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.
.. 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).
The code here isn't completely correct. Eg. for Newfoundland, the display name would be 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 |
<Compile Include="$(CommonPath)Interop\Interop.TimeZoneInfo.cs" Condition="'$(TargetsBrowser)' != 'true'"> | ||
<Compile Include="$(CommonPath)Interop\Interop.TimeZoneInfo.cs"> |
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.
why is this changed needed?
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.
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
@radical The regex in the original form is actually incorrect. |
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 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
The change makes sense, but are there any tests that check the daylight name on desktop? |
Only this one, so nothing comprehensive |
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.
Let's add tests for the non browser case too then
}; | ||
} | ||
|
||
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsBrowser))] |
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.
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?
Test failures unrelated to this change. |
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.
Nice!
|
||
[Theory] | ||
[MemberData(nameof(Platform_TimeZoneNamesTestData))] | ||
[PlatformSpecific(TestPlatforms.AnyUnix)] |
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.
Does windows have names for these extra timezones?
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.
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" }
{ 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" }, |
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.
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)
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.
How did the test pass with the wrong values?
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.
Are you referring to the last value in each test? They're supposed to be the DaylightNames.
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.
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.
@mattjohnsonpint if you can fix such issues in your PR would be great. You are touching the same area anyway. |
Will do. |
Thanks for following up on that, I should have caught it. |
Fixes: #45136
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.