Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Incorrect general long time pattern for some cultures on Android #103592

Closed
PopSlime opened this issue Jun 17, 2024 · 9 comments · Fixed by #103681
Closed

Incorrect general long time pattern for some cultures on Android #103592

PopSlime opened this issue Jun 17, 2024 · 9 comments · Fixed by #103681
Labels
area-System.Globalization os-android

Comments

@PopSlime
Copy link
Contributor

PopSlime commented Jun 17, 2024 •
edited
Loading

Description

On Android, some cultures have an incorrect general long time pattern, causing DateTime.ToString(IFormatProvider) to produce badly formatted strings.

Reproduction Steps

Consider the following code:

new DateTime(2000, 1, 1, 20, 0, 0).ToString(new CultureInfo("zh-TW"));

Expected behavior

A correctly formatted string (like 2000/1/1 下午8:00:00) is returned.

Actual behavior

A badly formatted string 2000/1/1 8:00:00 is returned, in which a 12-hour clock is used without an AM/PM designator.

Regression?

No response

Known Workarounds

No response

Configuration

  • .NET version: 8.0
  • Target operating system: Android
  • Architecture: x64, arm64
  • Android version: 13 (tested both on an emulator and a physical device)

Other information

No response
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jun 17, 2024
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

@PopSlime
Copy link
Contributor Author

I did some further investigations, this issue seems to be device-dependent. Just in case, this issue was reproduced with the system-images;android-33;google_apis;x86_64 system image.

@tarekgh
Copy link
Member

tarekgh commented Jun 17, 2024

CC @vitek-karas @mkhamoyan @ilonatommy

@vitek-karas
Copy link
Member

CC @matouskozak

@PopSlime
Copy link
Contributor Author

PopSlime commented Jun 18, 2024 •
edited
Loading

This issue seems to relate to this ConvertIcuTimeFormatString method.

runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.Icu.cs

Line 354 in b93061a

private static string ConvertIcuTimeFormatString(ReadOnlySpan<char> icuFormatString)

The current implementation does not cover the full Unicode Date Format Patterns. In the case of zh-TW (which expands to zh-Hant-TW and inherits zh-Hant,) the pattern symbol B is used [1], which is not covered in the aforementioned method.

Please consider supporting or partially supporting as many pattern symbols as possible just in case.

[1] https://github.com/unicode-org/cldr/blob/3147eda09f19de58463cc80d64248071531500a0/common/main/zh_Hant.xml#L4963

@PopSlime
Copy link
Contributor Author

Related PR: unicode-org/cldr#1297

@PopSlime
Copy link
Contributor Author

Affected CLDR version: 40 and above (https://cldr.unicode.org/index/downloads/cldr-40#h.mc372kkefa0z)

Affected Android version: 13 and above (https://developer.android.com/guide/topics/resources/internationalization#versioning-nougat)

@tarekgh
Copy link
Member

tarekgh commented Jun 18, 2024

@PopSlime thanks for the update. We should support the B conversion. Are you interested in submitting a PR for that?

CC @mkhamoyan

PopSlime added a commit to PopSlime/runtime that referenced this issue Jun 19, 2024
@PopSlime
Fix the ICU time format conversion logic
e61e7ab 
Revise the ICU time format conversion logic to support all unquoted literal texts and the `B` and `b` pattern symbols.

Fix dotnet#103592
@PopSlime PopSlime mentioned this issue Jun 19, 2024
Merged
@PopSlime
Copy link
Contributor Author

Hi, @tarekgh. I've submitted a PR for this issue but I'm not sure how to test it. Would you mind checking it out?

@PopSlime PopSlime mentioned this issue Jun 20, 2024
Closed
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Jun 21, 2024
rzikm pushed a commit to rzikm/dotnet-runtime that referenced this issue Jun 24, 2024
@PopSlime @rzikm
Fix the ICU time format conversion logic (dotnet#103681)
ecf7165 
* Fix the ICU time format conversion logic

Revise the ICU time format conversion logic to support all unquoted literal texts and the `B` and `b` pattern symbols.

Fix dotnet#103592

* Clarify literal texts in the conversion logic

* Add tests for verifying time patterns

Add tests verifying that all the short and long time patterns either use
a 24-hour clock or have an AM/PM designator.

* Fix literal single quote and literal backslash conversion

* Refactor the literal quote conversion logic

* Revise the test logic to ignore literal texts and check pattern redundancy

Modify the test logic so that it recognizes literal texts correctly, and
fails if 12-hour and 24-hour clocks are used at the same time.

* Revise the test logic to ensure all cultures are tested

* Add comments to clarify the backslash conversion

* Refactor the conversion logic

Simplify some logic and improve readability.

* Exclude bad ICU patterns from the tests

* Exclude the VerifyTimePatterns tests from hybrid globalization on browser

* Add missing usings

* Improve readability of the for-loops
@github-actions github-actions bot locked and limited conversation to collaborators Jul 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
area-System.Globalization os-android
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix the ICU time format conversion logic PopSlime/runtime
3 participants
@vitek-karas @tarekgh @PopSlime