From 967babdc5f19967bb8a5ce7c76fbb15a820b1b6d Mon Sep 17 00:00:00 2001 From: Rich Gillam Date: Thu, 15 Aug 2024 18:52:36 -0700 Subject: [PATCH] ICU-22669 Fix circular dependency in DateTimePatternGenerator::addICUPatterns(). --- icu4c/source/i18n/dtptngen.cpp | 85 ++++++++++++------- icu4c/source/test/cintltst/udatpg_test.c | 10 +++ icu4c/source/test/intltest/dtptngts.cpp | 28 +++++- icu4c/source/test/intltest/dtptngts.h | 1 + .../icu/text/DateTimePatternGenerator.java | 41 ++++++--- .../test/format/DateTimeGeneratorTest.java | 10 +++ 6 files changed, 130 insertions(+), 45 deletions(-) diff --git a/icu4c/source/i18n/dtptngen.cpp b/icu4c/source/i18n/dtptngen.cpp index 3207291f34a..976e8d28b30 100644 --- a/icu4c/source/i18n/dtptngen.cpp +++ b/icu4c/source/i18n/dtptngen.cpp @@ -803,38 +803,58 @@ DateTimePatternGenerator::staticGetBaseSkeleton( void DateTimePatternGenerator::addICUPatterns(const Locale& locale, UErrorCode& status) { - if (U_FAILURE(status)) { return; } - UnicodeString dfPattern; - UnicodeString conflictingString; - DateFormat* df; + if (U_FAILURE(status)) { + return; + } + + LocalUResourceBundlePointer rb(ures_open(nullptr, locale.getBaseName(), &status)); + CharString calendarTypeToUse; // to be filled in with the type to use, if all goes well + getCalendarTypeToUse(locale, calendarTypeToUse, status); - // Load with ICU patterns - for (int32_t i=DateFormat::kFull; i<=DateFormat::kShort; i++) { - DateFormat::EStyle style = static_cast(i); - df = DateFormat::createDateInstance(style, locale); - SimpleDateFormat* sdf; - if (df != nullptr && (sdf = dynamic_cast(df)) != nullptr) { - sdf->toPattern(dfPattern); - addPattern(dfPattern, false, conflictingString, status); + // HACK to get around the fact that the old SimpleDateFormat code (actually, Calendar::getCalendarTypeForLocale() ) + // returns "gregorian" for ja_JP_TRADITIONAL instead of "japanese" + if (uprv_strcmp(locale.getBaseName(), "ja_JP_TRADITIONAL") == 0) { + calendarTypeToUse.clear().append("gregorian", status); + } + + if (U_FAILURE(status)) { + return; + } + + // TODO: See ICU-22867 + CharString patternResourcePath; + patternResourcePath.append(DT_DateTimeCalendarTag, status) + .append('/', status) + .append(calendarTypeToUse, status) + .append('/', status) + .append(DT_DateTimePatternsTag, status); + + LocalUResourceBundlePointer dateTimePatterns(ures_getByKeyWithFallback(rb.getAlias(), patternResourcePath.data(), + nullptr, &status)); + if (ures_getType(dateTimePatterns.getAlias()) != URES_ARRAY || ures_getSize(dateTimePatterns.getAlias()) < 8) { + status = U_INVALID_FORMAT_ERROR; + return; + } + + for (int32_t i = 0; U_SUCCESS(status) && i < DateFormat::kDateTime; i++) { + LocalUResourceBundlePointer patternRes(ures_getByIndex(dateTimePatterns.getAlias(), i, nullptr, &status)); + UnicodeString pattern; + switch (ures_getType(patternRes.getAlias())) { + case URES_STRING: + pattern = ures_getUnicodeString(patternRes.getAlias(), &status); + break; + case URES_ARRAY: + pattern = ures_getUnicodeStringByIndex(patternRes.getAlias(), 0, &status); + break; + default: + status = U_INVALID_FORMAT_ERROR; + return; } - // TODO Maybe we should return an error when the date format isn't simple. - delete df; - if (U_FAILURE(status)) { return; } - - df = DateFormat::createTimeInstance(style, locale); - if (df != nullptr && (sdf = dynamic_cast(df)) != nullptr) { - sdf->toPattern(dfPattern); - addPattern(dfPattern, false, conflictingString, status); - - // TODO: C++ and Java are inconsistent (see #12568). - // C++ uses MEDIUM, but Java uses SHORT. - if ( i==DateFormat::kShort && !dfPattern.isEmpty() ) { - consumeShortTimePattern(dfPattern, status); - } + + if (U_SUCCESS(status)) { + UnicodeString conflictingPattern; + addPatternWithSkeleton(pattern, nullptr, false, conflictingPattern, status); } - // TODO Maybe we should return an error when the date format isn't simple. - delete df; - if (U_FAILURE(status)) { return; } } } @@ -905,7 +925,12 @@ DateTimePatternGenerator::getCalendarTypeToUse(const Locale& locale, CharString& &localStatus); localeWithCalendarKey[ULOC_LOCALE_IDENTIFIER_CAPACITY-1] = 0; // ensure null termination // now get the calendar key value from that locale - destination = ulocimp_getKeywordValue(localeWithCalendarKey, "calendar", localStatus); + // (the call to ures_getFunctionalEquivalent() above might fail, and if it does, localeWithCalendarKey + // won't contain a `calendar` keyword. If this happens, the line below will stomp on `destination`, + // so we have to check the return code before overwriting `destination`.) + if (U_SUCCESS(localStatus)) { + destination = ulocimp_getKeywordValue(localeWithCalendarKey, "calendar", localStatus); + } // If the input locale was invalid, don't fail with missing resource error, instead // continue with default of Gregorian. if (U_FAILURE(localStatus) && localStatus != U_MISSING_RESOURCE_ERROR) { diff --git a/icu4c/source/test/cintltst/udatpg_test.c b/icu4c/source/test/cintltst/udatpg_test.c index 7f01d5d0b94..5475c195426 100644 --- a/icu4c/source/test/cintltst/udatpg_test.c +++ b/icu4c/source/test/cintltst/udatpg_test.c @@ -424,6 +424,16 @@ static void TestOptions(void) { { "da", skel_Hmm, UDATPG_MATCH_HOUR_FIELD_LENGTH, patn_Hpmm }, { "da", skel_HHmm, UDATPG_MATCH_HOUR_FIELD_LENGTH, patn_HHpmm }, { "da", skel_hhmm, UDATPG_MATCH_HOUR_FIELD_LENGTH, patn_hhpmm_a }, + + // tests for ICU-22669 + { "zh_TW", u"jjm", UDATPG_MATCH_NO_OPTIONS, u"ah:mm" }, + { "zh_TW", u"jjm", UDATPG_MATCH_ALL_FIELDS_LENGTH, u"ahh:mm" }, + { "zh_TW", u"jjms", UDATPG_MATCH_NO_OPTIONS, u"ah:mm:ss" }, + { "zh_TW", u"jjms", UDATPG_MATCH_ALL_FIELDS_LENGTH, u"ahh:mm:ss" }, + { "zh_TW@hours=h23", u"jjm", UDATPG_MATCH_NO_OPTIONS, u"HH:mm" }, + { "zh_TW@hours=h23", u"jjm", UDATPG_MATCH_ALL_FIELDS_LENGTH, u"HH:mm" }, // (without the fix, we get "HH:m" here) + { "zh_TW@hours=h23", u"jjms", UDATPG_MATCH_NO_OPTIONS, u"HH:mm:ss" }, + { "zh_TW@hours=h23", u"jjms", UDATPG_MATCH_ALL_FIELDS_LENGTH, u"HH:mm:ss" }, }; int count = UPRV_LENGTHOF(testData); diff --git a/icu4c/source/test/intltest/dtptngts.cpp b/icu4c/source/test/intltest/dtptngts.cpp index 2ea24458258..c743d8159ba 100644 --- a/icu4c/source/test/intltest/dtptngts.cpp +++ b/icu4c/source/test/intltest/dtptngts.cpp @@ -47,7 +47,8 @@ void IntlTestDateTimePatternGeneratorAPI::runIndexedTest( int32_t index, UBool e TESTCASE(11, test_jConsistencyOddLocales); TESTCASE(12, testBestPattern); TESTCASE(13, testDateTimePatterns); - TESTCASE(14, testRegionOverride); + TESTCASE(14, testISO8601); + TESTCASE(15, testRegionOverride); default: name = ""; break; } } @@ -1774,6 +1775,31 @@ void IntlTestDateTimePatternGeneratorAPI::testDateTimePatterns() { } } +// Test for ICU-21839: Make sure ISO8601 patterns/symbols are inherited from Gregorian +void IntlTestDateTimePatternGeneratorAPI::testISO8601() { + const char* localeIDs[] = { + "de-AT-u-ca-iso8601", + "de-CH-u-ca-iso8601", + }; + const UChar* skeleton = u"jms"; + + for (int32_t i = 0; i < UPRV_LENGTHOF(localeIDs); i++) { + UErrorCode err = U_ZERO_ERROR; + LocalPointer dtpg(DateTimePatternGenerator::createInstance(Locale::forLanguageTag(localeIDs[i], err), err)); + UnicodeString pattern = dtpg->getBestPattern(skeleton, err); + if (pattern.indexOf(UnicodeString(u"├")) >= 0 || pattern.indexOf(UnicodeString(u"Minute")) >= 0) { + errln(UnicodeString("ERROR: locale ") + localeIDs[i] + UnicodeString(", skeleton ") + skeleton + UnicodeString(", bad pattern: ") + pattern); + } + + LocalPointer df(DateFormat::createTimeInstance(DateFormat::MEDIUM, Locale::forLanguageTag(localeIDs[i], err))); + UnicodeString format; + df->format(ucal_getNow(), format, err); + if (format.indexOf(UnicodeString(u"├")) >= 0 || format.indexOf(UnicodeString(u"Minute")) >= 0) { + errln(UnicodeString("ERROR: locale ") + localeIDs[i] + UnicodeString(", MEDIUM, bad format: ") + format); + } + } +} + void IntlTestDateTimePatternGeneratorAPI::testRegionOverride() { const struct TestCase { const char* locale; diff --git a/icu4c/source/test/intltest/dtptngts.h b/icu4c/source/test/intltest/dtptngts.h index 61699865ed8..123c580149d 100644 --- a/icu4c/source/test/intltest/dtptngts.h +++ b/icu4c/source/test/intltest/dtptngts.h @@ -41,6 +41,7 @@ private: void test_jConsistencyOddLocales(); void testBestPattern(); void testDateTimePatterns(); + void testISO8601(); void testRegionOverride(); enum { kNumDateTimePatterns = 4 }; diff --git a/icu4j/main/core/src/main/java/com/ibm/icu/text/DateTimePatternGenerator.java b/icu4j/main/core/src/main/java/com/ibm/icu/text/DateTimePatternGenerator.java index d66b478b0ed..769076ccc6e 100644 --- a/icu4j/main/core/src/main/java/com/ibm/icu/text/DateTimePatternGenerator.java +++ b/icu4j/main/core/src/main/java/com/ibm/icu/text/DateTimePatternGenerator.java @@ -27,13 +27,7 @@ import java.util.Set; import java.util.TreeMap; import java.util.TreeSet; -import com.ibm.icu.impl.ICUCache; -import com.ibm.icu.impl.ICUData; -import com.ibm.icu.impl.ICUResourceBundle; -import com.ibm.icu.impl.PatternTokenizer; -import com.ibm.icu.impl.SimpleCache; -import com.ibm.icu.impl.SimpleFormatterImpl; -import com.ibm.icu.impl.UResource; +import com.ibm.icu.impl.*; import com.ibm.icu.util.Calendar; import com.ibm.icu.util.Freezable; import com.ibm.icu.util.ICUCloneNotSupportedException; @@ -173,15 +167,34 @@ public class DateTimePatternGenerator implements Freezable