ICU-12973 CLDR Japanese Era data and tentative Japanese new era support #111

yumaoka · 2018-09-11T18:38:18Z

New internal API class EraRules designed for loading era data from CLDR and look up appropriate era for a given date.
EraRules can enable/disable tentative eras - disabled by default.
Updated JapaneseCalendar class to use EraRules to access Japanese era data.
Environment variable (or system property on Java) - ICU_ENABLE_TENTATIVE_ERA=true enables the tentative era rule and date format will produce placeholder era names (Qqqq/Q).

Note: Other calendar implementation classes may also use EraRules, but it may not add much values. For now, CLDR era data is utilized only by Japanese calendar.

Checklist

Issue filed at https://unicode-org.atlassian.net : ICU-12973
Update PR title to include Issue number
Issue accepted
Tests included
Documentation is changed or added

…from CLDR data in ICU4J.

Also added API comments.

yumaoka · 2018-09-11T18:40:25Z

#110 had merge conflict that cannot be resolved through PR. I closed #110 and created a new PR merging master to my branch before creating a PR, and created this one.

srl295

data LGTM

srl295

era rules in C LGTM

jefgen · 2018-09-11T20:44:11Z

icu4c/source/i18n/erarules.cpp

+
+EraRules* EraRules::createInstance(const char *calType, UBool includeTentativeEra, UErrorCode& status) {
+    if(U_FAILURE(status)) {
+        return NULL;


nit: Can we use nullptr since this is C++ file?

OK C++11 :) I'll update.

jefgen · 2018-09-11T20:46:37Z

icu4c/source/i18n/erarules.cpp

+    if(U_FAILURE(status)) {
+        return NULL;
+    }
+    UResourceBundle *rb = ures_openDirect(NULL, "supplementalData", &status);


nit: Could use LocalUResourceBundlePointer for this, so don't need to clean-up manually. :)

I see - will do.

jefgen · 2018-09-11T20:47:45Z

icu4c/source/i18n/erarules.cpp

+    int32_t firstTentativeIdx = MAX_INT32;
+
+    int32_t *startDates = (int32_t*)uprv_malloc(numEras * sizeof(int32_t));
+    if (startDates == NULL) {


jefgen · 2018-09-11T21:15:21Z

icu4c/source/i18n/japancal.cpp

+    // By default, such tentative era is disabled.
+
+    // 1. Environment variable ICU_ENABLE_TENTATIVE_ERA=true or false
+    // 2. Windows registry (TBD)


@yumaoka It might also be possible to check for environment variable in UWP as well.
Once this PR is in, perhaps I can also make another PR for this? (After the IUC 42 though).

I investigated quickly, but could not find it. I appreciate you can work on it.

- NULL to nullptr - use of LocalUResourceBundlePointer - TYPO "name" to "named" - env var checking stricmp() == 0

yumaoka · 2018-09-11T23:06:18Z

@jefgen I updated ICU4C implementation based on your comments. Also a few bug fixes. Please review it again. #Closed

srl295 · 2018-09-12T00:08:18Z

as of 3b01ed4 the tentative formatting works and is selectable. 👍

$ env ICU_ENABLE_TENTATIVE_ERA=false 'LC_ALL=ja@calendar=japanese' DYLD_LIBRARY_PATH=../../lib:../../stubdata:../../tools/ctestfw:$DYLD_LIBRARY_PATH  ./icudate -r 1559898950 -f
平成31年6月7日金曜日 2時15分50秒 アメリカ太平洋夏時間
srl@gozo:~/build/icu/samples/date
$ env ICU_ENABLE_TENTATIVE_ERA=true 'LC_ALL=ja@calendar=japanese' DYLD_LIBRARY_PATH=../../lib:../../stubdata:../../tools/ctestfw:$DYLD_LIBRARY_PATH  ./icudate -r 1559898950 -f
ＱＱ1年6月7日金曜日 2時15分50秒 アメリカ太平洋夏時間
srl@gozo:~/build/icu/samples/date
$ env ICU_ENABLE_TENTATIVE_ERA=true 'LC_ALL=ja@calendar=japanese' DYLD_LIBRARY_PATH=../../lib:../../stubdata:../../tools/ctestfw:$DYLD_LIBRARY_PATH  ./icudate  -f
平成30年9月11日火曜日 17時07分34秒 アメリカ太平洋夏時間
srl@gozo:~/build/icu/samples/date
$ env ICU_ENABLE_TENTATIVE_ERA=false 'LC_ALL=ja@calendar=japanese' DYLD_LIBRARY_PATH=../../lib:../../stubdata:../../tools/ctestfw:$DYLD_LIBRARY_PATH  ./icudate  -f
平成30年9月11日火曜日 17時07分38秒 アメリカ太平洋夏時間
``` #Closed

jefgen · 2018-09-12T00:22:34Z

icu4c/source/i18n/unicode/ucal.h

+ * <p>
+ * The Japanese calendar uses a combination of era name and year number.
+ * When an empror of Japan abdicates and a new emperor ascends the throne,
+ * a new era is declared and year number is reset to 1. Even the date of


nit: wording. Perhaps use this: "Even [if] the date of"

jefgen · 2018-09-12T00:28:57Z

icu4c/source/i18n/erarules.cpp

+    int32_t numEras = ures_getSize(rb.getAlias());
+    int32_t firstTentativeIdx = MAX_INT32;
+
+    int32_t *startDates = (int32_t*)uprv_malloc(numEras * sizeof(int32_t));


nit: could probably use LocalPointer instead of uprv_malloc. but I don't think this is necessary for now. I can change this later when adding UWP env support.

jefgen

LGTM. Thanks Yoshito.

Also, duplicate the comment in ucal.h to calendar.h

icu4c/source/i18n/unicode/calendar.h

srl295

lgtm with minor

icu4c/source/i18n/unicode/calendar.h

srl295 · 2018-09-12T17:14:41Z

icu4c/source/test/intltest/erarulestest.cpp

+}
+
+void EraRulesTest::testJapanese() {
+    const int32_t HEISEI = 235; // ICU4C does not define constants for eras


not ideal, but probably fine.

ICU4J exposes some constants for modern eras as int. Heisei is currently 235. ICU4J document says it might be changed in new releases. Markus pointed out it's not a good idea to change the value of these constants.

In this release, I considered that we clearly state these constant values won't change in future releases. But at the same time, I really think we should drop era data before Meiji. This would change the era indexes one more time.

I think it's not too late to find out how to expose some Japanese era constants after we decide what to do with historic eras.

srl295 · 2018-09-12T17:15:15Z

icu4c/source/test/intltest/erarulestest.cpp

+}
+
+void EraRulesTest::testAPIs() {
+    const char * calTypes[] = {


no other way to iterate on these types?

ICU4J has CalType enum. ICU4C has calendar type enum, but their names are defined in calendar.cpp (https://github.com/unicode-org/icu/blob/master/icu4c/source/i18n/calendar.cpp#L159) and has no API exposed to get calendar type name from enum or vise versa.

Filed https://unicode-org.atlassian.net/projects/ICU/issues/ICU-20134

srl295 · 2018-09-12T17:19:54Z

icu4j/main/classes/core/src/com/ibm/icu/util/JapaneseCalendar.java

@@ -634,6 +414,7 @@ protected int handleGetLimit(int field, int limitType) {
     * {@inheritDoc}
     * @stable ICU 3.8
     */
+    @Override
    public String getType() {
        return "japanese";


CalType.JAPANESE.name() ?

Right. If I remember correctly, getType() was implemented before CalType enum.
I did not touch this method for this time - eclipse kindly :) supplied @Override annotation automatically when saving JapaneseCalendar.java changes.

Other calendars have the same issue. I'd want to make consistent change with the new ticket https://unicode-org.atlassian.net/projects/ICU/issues/ICU-20134

jefgen · 2018-09-12T20:40:06Z

On Windows, with the Win32 version of ICU4C (UWP is yet to come) using a modified date program (Console is UTF-8 code-page).

>date.exe
平成30年9月12日 11:58:36
>date.exe -r 1559898950
平成31年6月7日 2:15:50
>set ICU_ENABLE_TENTATIVE_ERA=true
>date.exe
平成30年9月12日 13:11:26
>date.exe -r 1559898950
ＱＱ1年6月7日 2:15:50
``` #Closed

…rt (unicode-org#111) * Updated era data format in supplementalData. * Include tentative era names in data. Implemented Japanese era loaded from CLDR data in ICU4J. * ICU4C implementation, ICU4C refactoring. WIP. * VS project updates and some bug fixes Also added API comments. * Review feedback and bug fixes - NULL to nullptr - use of LocalUResourceBundlePointer - TYPO "name" to "named" - env var checking stricmp() == 0 * API comment correction based on feedback * Duplicate the comment in ucal.h to calendar.h * Fixed spelling errors in API comment

… the New Era. Manually cherry-picked from commit: 45cdda6 Original commit message: ICU-12973 CLDR Japanese Era data and tentative Japanese new era support (unicode-org#111) * Updated era data format in supplementalData. * Include tentative era names in data. Implemented Japanese era loaded from CLDR data in ICU4J. * ICU4C implementation, ICU4C refactoring. WIP. * VS project updates and some bug fixes Also added API comments. * Review feedback and bug fixes - NULL to nullptr - use of LocalUResourceBundlePointer - TYPO "name" to "named" - env var checking stricmp() == 0 * API comment correction based on feedback * Duplicate the comment in ucal.h to calendar.h * Fixed spelling errors in API comment

… the New Era. Manually cherry-picked from commit: 45cdda6 Original commit message: ICU-12973 CLDR Japanese Era data and tentative Japanese new era support (#111) * Updated era data format in supplementalData. * Include tentative era names in data. Implemented Japanese era loaded from CLDR data in ICU4J. * ICU4C implementation, ICU4C refactoring. WIP. * VS project updates and some bug fixes Also added API comments. * Review feedback and bug fixes - NULL to nullptr - use of LocalUResourceBundlePointer - TYPO "name" to "named" - env var checking stricmp() == 0 * API comment correction based on feedback * Duplicate the comment in ucal.h to calendar.h * Fixed spelling errors in API comment

yumaoka and others added 5 commits August 23, 2018 03:40

Updated era data format in supplementalData.

9342f53

Include tentative era names in data. Implemented Japanese era loaded …

dbf0f78

…from CLDR data in ICU4J.

ICU4C implementation, ICU4C refactoring. WIP.

8b83b30

VS project updates and some bug fixes

863fc5b

Also added API comments.

Merge the latest master changes and resolved merge conflict.

aac6add

yumaoka requested review from pedberg-icu, jefgen and srl295 September 11, 2018 18:38

yumaoka assigned srl295 Sep 11, 2018

srl295 reviewed Sep 11, 2018

View reviewed changes

jefgen reviewed Sep 11, 2018

View reviewed changes

Review feedback and bug fixes

3b01ed4

- NULL to nullptr - use of LocalUResourceBundlePointer - TYPO "name" to "named" - env var checking stricmp() == 0

jefgen reviewed Sep 12, 2018

View reviewed changes

jefgen previously approved these changes Sep 12, 2018

View reviewed changes

API comment correction based on feedback

852c439

Also, duplicate the comment in ucal.h to calendar.h

yumaoka dismissed jefgen’s stale review via 852c439 September 12, 2018 15:50

jefgen reviewed Sep 12, 2018

View reviewed changes

icu4c/source/i18n/unicode/calendar.h Outdated Show resolved Hide resolved

srl295 previously approved these changes Sep 12, 2018

View reviewed changes

Fixed spelling errors in API comment

3624b35

yumaoka dismissed srl295’s stale review via 3624b35 September 12, 2018 19:58

srl295 approved these changes Sep 12, 2018

View reviewed changes

yumaoka merged commit 61d446f into unicode-org:master Sep 12, 2018

yumaoka deleted the cldr-era2 branch September 12, 2018 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICU-12973 CLDR Japanese Era data and tentative Japanese new era support #111

ICU-12973 CLDR Japanese Era data and tentative Japanese new era support #111

yumaoka commented Sep 11, 2018

yumaoka commented Sep 11, 2018

srl295 left a comment

srl295 left a comment

jefgen Sep 11, 2018

yumaoka Sep 11, 2018

jefgen Sep 11, 2018

yumaoka Sep 11, 2018

jefgen Sep 11, 2018

jefgen Sep 11, 2018

yumaoka Sep 11, 2018

yumaoka commented Sep 11, 2018 •

edited by jefgen

srl295 commented Sep 12, 2018 •

edited by jefgen

jefgen Sep 12, 2018

jefgen Sep 12, 2018

yumaoka Sep 12, 2018

jefgen left a comment

srl295 left a comment

srl295 Sep 12, 2018

yumaoka Sep 12, 2018

srl295 Sep 12, 2018

yumaoka Sep 12, 2018

yumaoka Sep 12, 2018

srl295 Sep 12, 2018

yumaoka Sep 12, 2018 •

edited

yumaoka Sep 12, 2018

jefgen commented Sep 12, 2018 •

edited

ICU-12973 CLDR Japanese Era data and tentative Japanese new era support #111

ICU-12973 CLDR Japanese Era data and tentative Japanese new era support #111

Conversation

yumaoka commented Sep 11, 2018

Checklist

yumaoka commented Sep 11, 2018

srl295 left a comment

Choose a reason for hiding this comment

srl295 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yumaoka commented Sep 11, 2018 • edited by jefgen

srl295 commented Sep 12, 2018 • edited by jefgen

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefgen left a comment

Choose a reason for hiding this comment

srl295 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yumaoka Sep 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefgen commented Sep 12, 2018 • edited

yumaoka commented Sep 11, 2018 •

edited by jefgen

srl295 commented Sep 12, 2018 •

edited by jefgen

yumaoka Sep 12, 2018 •

edited

jefgen commented Sep 12, 2018 •

edited