-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[iOS][non-icu] HybridGlobalization implement japanese calendar data #92471
Conversation
Tagging subscribers to this area: @dotnet/area-system-globalization Issue DetailsImplement below methods for hybrid mode on Apple platforms.
Contributes to #80689 cc @SamMonoRT
|
/azp run runtime-ioslike |
Azure Pipelines successfully started running 1 pipeline(s). |
Tagging subscribers to 'os-ios': @steveisok, @akoeplinger, @kotlarmilos Issue DetailsImplement japanese calendar methods for hybrid mode on Apple platforms.
Contributes to #80689 cc @SamMonoRT
|
src/libraries/System.Private.CoreLib/src/System/Globalization/JapaneseCalendar.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/JapaneseCalendar.iOS.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/JapaneseCalendar.iOS.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/JapaneseCalendar.iOS.cs
Outdated
Show resolved
Hide resolved
/azp run runtime-ioslike |
Azure Pipelines successfully started running 1 pipeline(s). |
NSString *defaultCalendarIdentifier = [currentLocale calendarIdentifier]; | ||
int32_t calendarCount = MIN(calendarIdentifiers.count, calendarsCapacity); | ||
int32_t calendarIndex = 0; | ||
calendars[calendarIndex++] = GetCalendarId([defaultCalendarIdentifier UTF8String]); |
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.
Theres a possibility of this being UNINITIALIZED_VALUE
right? It doesn't seem like we should be setting this if its not a known calendar Id. Which highlights that there are more calendars in the calendarIdentifiers
list than in the known calendarIds in GetCalendarId
. Should we expand the coverage of GetCalendarId
and GetCalendarIdentifier
?
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 catch, I will update it.
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.
Updated the part related to UNINITIALIZED_VALUE
, but for expanding coverage in GetCalendarId
and GetCalendarIdentifier
, looks like it is not smth that we support in C# side with current implementation. Supported calendars are implemented in C# like for example https://github.com/mkhamoyan/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Globalization/JapaneseCalendar.cs . I guess I can open a ticket to see if we can implement more calendars that are supported by NSCalendarIdentifier
. What do you think?
else | ||
{ | ||
string[]? abbrevEraNames; | ||
if (!CalendarData.EnumCalendarInfo("ja", CalendarId.JAPAN, CalendarDataType.AbbrevEraNames, out abbrevEraNames)) |
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.
Could we still use out abbrevEnglishEraNames!
directly instead like in the non-apple block? So making this else block the same as the non-apple block
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.
Updated it.
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Looks good to me! Thanks!
Failures are not related. |
Implement japanese calendar methods for hybrid mode on Apple platforms.
Contributes to #80689
cc @SamMonoRT