ICU-21392 Fix Windows Time Zone detection when more than one possible time zone. #1465
Conversation
d2efa64 to
9b5b9c1
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
FIWW, I think that unit testing is a good idea -- but it's somewhat tricky as this function directly calls platform APIs like the registry and such. If we had something like Detours to support mocking the Win32 platform APIs directly, then it would be easier. But I don't know if we want to use another library just to test one function. FYI: @FrankYFTang: I've file a follow-up bug to think some more about how to better test this code: |
|
Thanks for the reviews! |
f9bc0a0
9b5b9c1 to
f9bc0a0
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
f9bc0a0 to
d3a989c
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
|
CC @mj1856 |
|
It seems we see new problem coming in related to this when we start to roll out chrome. Which version of Windows have you test your code on? Would it work on older version of Windows? https://bugs.chromium.org/p/chromium/issues/detail?id=1168528 |
Yes, it should work on older versions of Windows as well. FWIW, I tested the change on the following versions:
I can't repo the issue anymore with the fix. I'll add a comment to the Cr bug, but I think we'll probably need more info in order to understand what might be the issue. |
|
Actually, it seems that editing the comment does not remove the link on GitHub... I'm going to delete the comment instead. Update: And it seems that deleting the comment doesn't remove the link either. :( In any case, here's the updated comment:
Just to add an update on this PR: |
This change cherry-picks the upstream fix for the issue of the Windows Time Zone offset being wrong when Automatic DST setting is OFF in the Control Panel. ICU-21465 Windows Time Zone offset is wrong when Automatic DST setting is OFF https://unicode-org.atlassian.net/browse/ICU-21465 Cherry-picked from upstream commit: c6934ff7476200fc22abdf4300c94e2a3460c793 The upstream PR into master is here: unicode-org/icu#1465 The upstream PR into maint/maint-68 is here: unicode-org/icu#1543
…65) This change cherry-picks the upstream fix for the issue of the Windows Time Zone offset being wrong when Automatic DST setting is OFF in the Control Panel. ICU-21465 Windows Time Zone offset is wrong when Automatic DST setting is OFF https://unicode-org.atlassian.net/browse/ICU-21465 Cherry-picked from upstream commit: c6934ff7476200fc22abdf4300c94e2a3460c793 The upstream PR into master is here: unicode-org/icu#1465 The upstream PR into maint/maint-68 is here: unicode-org/icu#1543 Detailed Description: This fixes a bug in the changes for ICU-13845, for handling when the "Automatic DST" setting is OFF in the Control Panel. It turns out that the offset value for the calculated time zone when the "Automatic DST" setting is turn off is wrong. The sign for the time zone offset is flipped. (It's positive when it should be negative, and vice-versa). I think this was perhaps due to confusion about the value for the UTC offset, how POSIX style offsets are handled, and what is reported by the Win32 API `GetDynamicTimeZoneInformation`. It seems that this issue was actually present in the original PR for ICU-13845 and it was missed in the reviews on the other PR for the ticket.
This PR is a fix for the regression caused by the change in PR #1362.
The list of possible time zones returned is a space-delimited list in some cases.
The change in PR #1362 was only stopping at the null-terminator. Instead we need to stop at the first space.
I missed this when reviewing the changes in PR #1297.
The issue is due to the removal of the check here on this line which was stopping at the null or space:
https://github.com/unicode-org/icu/pull/1297/files#diff-04dfe6b718cf7d1421df2b6ff3aa89d0b95a7e2d468f5eb323e3698557a3a936L106
Checklist