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.

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

Fix the ICU time format conversion logic #103681

Merged
merged 13 commits into from
Jun 21, 2024

Conversation

PopSlime
Copy link
Contributor

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

Fix #103592

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

Fix dotnet#103592
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 19, 2024
@PopSlime
Copy link
Contributor Author

PopSlime commented Jun 19, 2024

I'm new to such a big and complex project and not sure how to test these changes, so this is a draft at the moment. Looking forward to help from anyone.

Example affected patterns:

ICU pattern Result (Old) Result (New)
a नि h:mm tt h:mm tt नि h:mm
ཆུ་ཚོད་h:mm:ss a h:mm:ss tt ཆུ་ཚོད་h:mm:ss tt
ཆུ་ཚོད་ h སྐར་མ་ mm a h mm tt ཆུ་ཚོད་ h སྐར་མ་ mm tt
Bh:mm:ss h:mm:ss tth:mm:ss

More information can be found in the fixing issue.

@mkhamoyan
Copy link
Contributor

@mkhamoyan
Copy link
Contributor

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@PopSlime
Copy link
Contributor Author

PopSlime commented Jun 19, 2024

@mkhamoyan I'm not sure how to write the test for this because the retrieved time format is device-/platform-dependent.

Instead of testing against some specific patterns, I'm considering adding a test that fails if a 12-hour clock is used without an AM/PM designator in any pattern in any culture. Also we need another test for the unquoted literal texts, which I have no clues how to do yet.

By the way, short time patterns are also affected by this PR.

@mkhamoyan
Copy link
Contributor

mkhamoyan commented Jun 19, 2024

@PopSlime I was thinking something like below (same test case for shorttime patterns under here) would validate the change.

[Theory]
[InlineData("zh-TW")]
[InlineData("en-US")]
[InlineData("fr-FR")]
public void LongTimePattern_ValidateAMPMDesignators(string cultureName)
{
    var cultureInfo = new CultureInfo(cultureName);
    var date = DateTime.Today + TimeSpan.FromDays(10) + TimeSpan.FromMinutes(10);
    string formattedDateTime = date.ToString("T", cultureInfo);
    
    Assert.True(formattedDateTime.Contains(cultureInfo.DateTimeFormat.AMDesignator));
}

This should be consistent for all platforms and devices.
Now we see issue only in some android devices because of different CLDR versions.

@PopSlime
Copy link
Contributor Author

@mkhamoyan Is it safe to assume those cultures use a 12-hour clock? fr-FR uses a 24-hour clock on my side.

@mkhamoyan
Copy link
Contributor

@mkhamoyan Is it safe to assume those cultures use a 12-hour clock? fr-FR uses a 24-hour clock on my side.

My bad, fr-FR shouldn't be there.

@PopSlime
Copy link
Contributor Author

How about this?

[Fact]
public void LongTimePattern_VerifyTimePatterns()
{
    foreach (var culture in CultureInfo.GetCultures(CultureTypes.AllCultures))
    {
        var pattern = culture.DateTimeFormat.LongTimePattern;
        var segments = pattern.Split('\'');
        bool use12Hour = false;
        bool useAMPM = false;
        for (var i = 0; i < segments.Length; i += 2)
        {
            var segment = segments[i];
            use12Hour |= segment.Contains('h', StringComparison.Ordinal);
            useAMPM |= segment.Contains('t', StringComparison.Ordinal);
        }
        Assert.True(!use12Hour || useAMPM, $"Bad time pattern for culture {culture.Name}: '{pattern}'");
    }
}

By the way, this fails on my PC (Windows) with mi: 'h:mm:ss' and mi-NZ: 'h:mm:ss'. Should we address this in another issue?

@mkhamoyan
Copy link
Contributor

How about this?

Looks good to me.

By the way, this fails on my PC (Windows) with mi: 'h:mm:ss' and mi-NZ: 'h:mm:ss'. Should we address this in another issue?

Yes , we should file an issue for that.
Let's push in the PR and check how it behaves on other platforms.

@tarekgh tarekgh self-requested a review June 19, 2024 16:06
Add tests verifying that all the short and long time patterns either use
a 24-hour clock or have an AM/PM designator.
@PopSlime
Copy link
Contributor Author

@dotnet-policy-service agree

@PopSlime PopSlime marked this pull request as ready for review June 19, 2024 17:31
@tarekgh
Copy link
Member

tarekgh commented Jun 19, 2024

By the way, this fails on my PC (Windows) with mi: 'h:mm:ss' and mi-NZ: 'h:mm:ss'. Should we address this in another issue?
Yes , we should file an issue for that.

I don't think we can fix that as the patterns are picked from Windows. I suggest you mark your new test methods with the attribute and see if this can help?

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsIcuGlobalization))]

@PopSlime
Copy link
Contributor Author

I'll do that, but I also want to see if similar issues are occurring on other platforms, so let's wait for these checks to be done.

@tarekgh
Copy link
Member

tarekgh commented Jun 19, 2024

this fails on my PC (Windows) with mi: 'h:mm:ss' and mi-NZ: 'h:mm:ss'.

I am trying on my machine, and I am not getting the same results you are getting.

mi-NZ .. Maori (New Zealand) .. h:mm tt .. h:mm:ss tt
mi .. Maori .. h:mm tt .. h:mm:ss tt

running code like:

            var ci = CultureInfo.GetCultureInfo("mi-NZ");
            Console.WriteLine($"{ci.Name} .. {ci.EnglishName} .. {ci.DateTimeFormat.ShortTimePattern} .. {ci.DateTimeFormat.LongTimePattern}");
            ci = CultureInfo.GetCultureInfo("mi");
            Console.WriteLine($"{ci.Name} .. {ci.EnglishName} .. {ci.DateTimeFormat.ShortTimePattern} .. {ci.DateTimeFormat.LongTimePattern}");

What Windows version do you have? I am wondering how you get this result?

@PopSlime
Copy link
Contributor Author

It seems that the version of the CLDR data installed on my PC is below 36. The time format for mi was fixed in unicode-org/cldr#104.

  • Operating system: Windows 10
  • Version number: 22H2
  • Internal version: 19045.4529

@tarekgh
Copy link
Member

tarekgh commented Jun 19, 2024

ok, then the attribute

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsIcuGlobalization))]
is not going to help. You may try using something like
if (PlatformDetection.WindowsVersion >= 10 || PlatformDetection.ICUVersion.Major >= 55 || PlatformDetection.IsHybridGlobalizationOnApplePlatform)
to avoid the failure.

I meant using check like if (PlatformDetection.ICUVersion.Major >= 68) .

@ilonatommy
Copy link
Member

@PopSlime the tests are still failing. For now, can you exclude running this test with hype globalization? or special case fr-CA when encountering it? @ilonatommy @mkhamoyan do you have any better suggestion? @PopSlime already logged issue to tracking fixing fr-CA formats.

Current behavior was expected and other HybridGlobalization tests are prepared for that. I am looking if we can provide a generic fix. For now this test can be blocked with PlatformDetection.IsNotHybridGlobalizationOnBrowser. The fix can get in in a separate PR

@mkhamoyan
Copy link
Contributor

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Contributor

@mkhamoyan mkhamoyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.
Let's wait for ios pipelines and then we can merge.

@mkhamoyan
Copy link
Contributor

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PopSlime
Copy link
Contributor Author

/azp run runtime

Copy link

Commenter does not have sufficient privileges for PR 103681 in repo dotnet/runtime

@ilonatommy
Copy link
Member

/azp run runtime

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tarekgh tarekgh merged commit 4b9a1b2 into dotnet:main Jun 21, 2024
159 of 167 checks passed
rzikm pushed a commit to rzikm/dotnet-runtime that referenced this pull request Jun 24, 2024
* 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.
Labels
area-System.Globalization community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect general long time pattern for some cultures on Android
4 participants