From 7c147e4e85b086d6fb400f1e292ab5317d7bcc99 Mon Sep 17 00:00:00 2001 From: Caio Lima Date: Tue, 10 Dec 2019 13:10:00 -0800 Subject: [PATCH] ICU-20741 Changing SimpleDateTimeFormat::subFormat to only include 1 field at the same position when there is a data fallback --- icu4c/source/i18n/smpdtfmt.cpp | 33 ++++++++---- icu4c/source/i18n/unicode/smpdtfmt.h | 1 + icu4c/source/test/intltest/dtfmttst.cpp | 53 ++++++++++--------- .../com/ibm/icu/text/ChineseDateFormat.java | 3 +- .../com/ibm/icu/text/SimpleDateFormat.java | 41 +++++++++----- .../icu/dev/test/format/DateFormatTest.java | 44 ++++++++------- 6 files changed, 106 insertions(+), 69 deletions(-) diff --git a/icu4c/source/i18n/smpdtfmt.cpp b/icu4c/source/i18n/smpdtfmt.cpp index 5fcbb5875b2..6246e8f270f 100644 --- a/icu4c/source/i18n/smpdtfmt.cpp +++ b/icu4c/source/i18n/smpdtfmt.cpp @@ -996,7 +996,8 @@ SimpleDateFormat::_format(Calendar& cal, UnicodeString& appendTo, // Use subFormat() to format a repeated pattern character // when a different pattern or non-pattern character is seen if (ch != prevCh && count > 0) { - subFormat(appendTo, prevCh, count, capitalizationContext, fieldNum++, handler, *workCal, status); + subFormat(appendTo, prevCh, count, capitalizationContext, fieldNum++, + prevCh, handler, *workCal, status); count = 0; } if (ch == QUOTE) { @@ -1023,7 +1024,8 @@ SimpleDateFormat::_format(Calendar& cal, UnicodeString& appendTo, // Format the last item in the pattern, if any if (count > 0) { - subFormat(appendTo, prevCh, count, capitalizationContext, fieldNum++, handler, *workCal, status); + subFormat(appendTo, prevCh, count, capitalizationContext, fieldNum++, + prevCh, handler, *workCal, status); } if (calClone != NULL) { @@ -1402,10 +1404,11 @@ SimpleDateFormat::processOverrideString(const Locale &locale, const UnicodeStrin //--------------------------------------------------------------------- void SimpleDateFormat::subFormat(UnicodeString &appendTo, - UChar ch, + char16_t ch, int32_t count, UDisplayContext capitalizationContext, int32_t fieldNum, + char16_t fieldToOutput, FieldPositionHandler& handler, Calendar& cal, UErrorCode& status) const @@ -1853,8 +1856,11 @@ SimpleDateFormat::subFormat(UnicodeString &appendTo, // In either case, fall back to am/pm. if (toAppend == NULL || toAppend->isBogus()) { // Reformat with identical arguments except ch, now changed to 'a'. - subFormat(appendTo, 0x61, count, capitalizationContext, fieldNum, - handler, cal, status); + // We are passing a different fieldToOutput because we want to add + // 'b' to field position. This makes this fallback stable when + // there is a data change on locales. + subFormat(appendTo, u'a', count, capitalizationContext, fieldNum, u'b', handler, cal, status); + return; } else { appendTo += *toAppend; } @@ -1874,9 +1880,11 @@ SimpleDateFormat::subFormat(UnicodeString &appendTo, if (ruleSet == NULL) { // Data doesn't exist for the locale we're looking for. // Falling back to am/pm. - subFormat(appendTo, 0x61, count, capitalizationContext, fieldNum, - handler, cal, status); - break; + // We are passing a different fieldToOutput because we want to add + // 'B' to field position. This makes this fallback stable when + // there is a data change on locales. + subFormat(appendTo, u'a', count, capitalizationContext, fieldNum, u'B', handler, cal, status); + return; } // Get current display time. @@ -1945,8 +1953,11 @@ SimpleDateFormat::subFormat(UnicodeString &appendTo, if (periodType == DayPeriodRules::DAYPERIOD_AM || periodType == DayPeriodRules::DAYPERIOD_PM || toAppend->isBogus()) { - subFormat(appendTo, 0x61, count, capitalizationContext, fieldNum, - handler, cal, status); + // We are passing a different fieldToOutput because we want to add + // 'B' to field position iterator. This makes this fallback stable when + // there is a data change on locales. + subFormat(appendTo, u'a', count, capitalizationContext, fieldNum, u'B', handler, cal, status); + return; } else { appendTo += *toAppend; @@ -1990,7 +2001,7 @@ SimpleDateFormat::subFormat(UnicodeString &appendTo, } #endif - handler.addAttribute(fgPatternIndexToDateFormatField[patternCharIndex], beginOffset, appendTo.length()); + handler.addAttribute(DateFormatSymbols::getPatternCharIndex(fieldToOutput), beginOffset, appendTo.length()); } //---------------------------------------------------------------------- diff --git a/icu4c/source/i18n/unicode/smpdtfmt.h b/icu4c/source/i18n/unicode/smpdtfmt.h index 79fa817d5af..b4b0e5fb390 100644 --- a/icu4c/source/i18n/unicode/smpdtfmt.h +++ b/icu4c/source/i18n/unicode/smpdtfmt.h @@ -1274,6 +1274,7 @@ private: int32_t count, UDisplayContext capitalizationContext, int32_t fieldNum, + char16_t fieldToOutput, FieldPositionHandler& handler, Calendar& cal, UErrorCode& status) const; // in case of illegal argument diff --git a/icu4c/source/test/intltest/dtfmttst.cpp b/icu4c/source/test/intltest/dtfmttst.cpp index 93aa95165e9..d4fca1c88d6 100644 --- a/icu4c/source/test/intltest/dtfmttst.cpp +++ b/icu4c/source/test/intltest/dtfmttst.cpp @@ -5579,34 +5579,39 @@ void DateFormatTest::Test20741_ABFields() { const char16_t timeZone[] = u"PST8PDT"; - UnicodeString skeleton = u"EEEEEBBBBB"; - int32_t count = 0; - const Locale* locales = Locale::getAvailableLocales(count); - for (int32_t i = 0; i < count; i++) { - if (quick && (i % 17) != 0) { continue; } + UnicodeString skeletons[] = {u"EEEEEBBBBB", u"EEEEEbbbbb"}; - const Locale locale = locales[i]; - LocalPointer gen(DateTimePatternGenerator::createInstance(locale, status)); - UnicodeString pattern = gen->getBestPattern(skeleton, status); + for (int32_t j = 0; j < 2; j++) { + UnicodeString skeleton = skeletons[j]; - SimpleDateFormat dateFormat(pattern, locale, status); - FieldPositionIterator fpositer; - UnicodeString result; - LocalPointer calendar(Calendar::createInstance(TimeZone::createTimeZone(timeZone), status)); - calendar->setTime(UDate(0), status); - dateFormat.format(*calendar, result, &fpositer, status); + int32_t count = 0; + const Locale* locales = Locale::getAvailableLocales(count); + for (int32_t i = 0; i < count; i++) { + if (quick && (i % 17) != 0) { continue; } - FieldPosition curFieldPosition; - FieldPosition lastFieldPosition; - lastFieldPosition.setBeginIndex(-1); - lastFieldPosition.setEndIndex(-1); - while(fpositer.next(curFieldPosition)) { - if (curFieldPosition.getBeginIndex() == lastFieldPosition.getBeginIndex() && curFieldPosition.getEndIndex() == lastFieldPosition.getEndIndex()) { - if (logKnownIssue("20741")) continue; - assertEquals("Different fields at same position", 'B', PATTERN_CHARS[lastFieldPosition.getField()]); + const Locale locale = locales[i]; + LocalPointer gen(DateTimePatternGenerator::createInstance(locale, status)); + UnicodeString pattern = gen->getBestPattern(skeleton, status); + + SimpleDateFormat dateFormat(pattern, locale, status); + FieldPositionIterator fpositer; + UnicodeString result; + LocalPointer calendar(Calendar::createInstance(TimeZone::createTimeZone(timeZone), status)); + calendar->setTime(UDate(0), status); + dateFormat.format(*calendar, result, &fpositer, status); + + FieldPosition curFieldPosition; + FieldPosition lastFieldPosition; + lastFieldPosition.setBeginIndex(-1); + lastFieldPosition.setEndIndex(-1); + while(fpositer.next(curFieldPosition)) { + assertFalse("Field missing on pattern", pattern.indexOf(PATTERN_CHARS[curFieldPosition.getField()]) == -1); + if (curFieldPosition.getBeginIndex() == lastFieldPosition.getBeginIndex() && curFieldPosition.getEndIndex() == lastFieldPosition.getEndIndex()) { + assertEquals("Different fields at same position", PATTERN_CHARS[curFieldPosition.getField()], PATTERN_CHARS[lastFieldPosition.getField()]); + } + + lastFieldPosition = curFieldPosition; } - - lastFieldPosition = curFieldPosition; } } } diff --git a/icu4j/main/classes/core/src/com/ibm/icu/text/ChineseDateFormat.java b/icu4j/main/classes/core/src/com/ibm/icu/text/ChineseDateFormat.java index 735fac6606d..a101504ddc5 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/text/ChineseDateFormat.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/text/ChineseDateFormat.java @@ -126,12 +126,13 @@ public class ChineseDateFormat extends SimpleDateFormat { char ch, int count, int beginOffset, int fieldNum, DisplayContext capitalizationContext, FieldPosition pos, + char patternCharToOutput, Calendar cal) { // Logic to handle 'G' for chinese calendar is moved into SimpleDateFormat, // and obsolete pattern char 'l' is now ignored in SimpleDateFormat, so we // just use its implementation - super.subFormat(buf, ch, count, beginOffset, fieldNum, capitalizationContext, pos, cal); + super.subFormat(buf, ch, count, beginOffset, fieldNum, capitalizationContext, pos, patternCharToOutput, cal); // The following is no longer an issue for this subclass... // TODO: add code to set FieldPosition for 'G' and 'l' fields. This diff --git a/icu4j/main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java b/icu4j/main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java index 090e9564cfa..733e5406285 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/text/SimpleDateFormat.java @@ -1395,10 +1395,10 @@ public class SimpleDateFormat extends DateFormat { } if (useFastFormat) { subFormat(toAppendTo, item.type, item.length, toAppendTo.length(), - i, capitalizationContext, pos, cal); + i, capitalizationContext, pos, item.type, cal); } else { toAppendTo.append(subFormat(item.type, item.length, toAppendTo.length(), - i, capitalizationContext, pos, cal)); + i, capitalizationContext, pos, item.type, cal)); } if (attributes != null) { // Check the sub format length @@ -1547,7 +1547,7 @@ public class SimpleDateFormat extends DateFormat { throws IllegalArgumentException { // Note: formatData is ignored - return subFormat(ch, count, beginOffset, 0, DisplayContext.CAPITALIZATION_NONE, pos, cal); + return subFormat(ch, count, beginOffset, 0, DisplayContext.CAPITALIZATION_NONE, pos, ch, cal); } /** @@ -1561,10 +1561,11 @@ public class SimpleDateFormat extends DateFormat { protected String subFormat(char ch, int count, int beginOffset, int fieldNum, DisplayContext capitalizationContext, FieldPosition pos, + char patternCharToOutput, Calendar cal) { StringBuffer buf = new StringBuffer(); - subFormat(buf, ch, count, beginOffset, fieldNum, capitalizationContext, pos, cal); + subFormat(buf, ch, count, beginOffset, fieldNum, capitalizationContext, pos, patternCharToOutput, cal); return buf.toString(); } @@ -1586,6 +1587,7 @@ public class SimpleDateFormat extends DateFormat { char ch, int count, int beginOffset, int fieldNum, DisplayContext capitalizationContext, FieldPosition pos, + char patternCharToOutput, Calendar cal) { final int maxIntCount = Integer.MAX_VALUE; @@ -1944,7 +1946,10 @@ public class SimpleDateFormat extends DateFormat { if (toAppend == null) { // Time isn't exactly midnight or noon (as displayed) or localized string doesn't // exist for requested period. Fall back to am/pm instead. - subFormat(buf, 'a', count, beginOffset, fieldNum, capitalizationContext, pos, cal); + // We are passing a different patternCharToOutput because we want to add + // 'b' to field position. This makes this fallback stable when + // there is a data change on locales. + subFormat(buf, 'a', count, beginOffset, fieldNum, capitalizationContext, pos, 'b', cal); } else { buf.append(toAppend); } @@ -1959,8 +1964,11 @@ public class SimpleDateFormat extends DateFormat { if (ruleSet == null) { // Data doesn't exist for the locale we're looking for. // Fall back to am/pm. - subFormat(buf, 'a', count, beginOffset, fieldNum, capitalizationContext, pos, cal); - break; + // We are passing a different patternCharToOutput because we want to add + // 'B' to field position. This makes this fallback stable when + // there is a data change on locales. + subFormat(buf, 'a', count, beginOffset, fieldNum, capitalizationContext, pos, 'B', cal); + return; } // Get current display time. @@ -2025,7 +2033,11 @@ public class SimpleDateFormat extends DateFormat { if (periodType == DayPeriodRules.DayPeriod.AM || periodType == DayPeriodRules.DayPeriod.PM || toAppend == null) { - subFormat(buf, 'a', count, beginOffset, fieldNum, capitalizationContext, pos, cal); + // We are passing a different patternCharToOutput because we want to add + // 'B' to field position. This makes this fallback stable when + // there is a data change on locales. + subFormat(buf, 'a', count, beginOffset, fieldNum, capitalizationContext, pos, 'B', cal); + return; } else { buf.append(toAppend); @@ -2089,12 +2101,13 @@ public class SimpleDateFormat extends DateFormat { } // Set the FieldPosition (for the first occurrence only) + int outputCharIndex = getIndexFromChar(patternCharToOutput); if (pos.getBeginIndex() == pos.getEndIndex()) { - if (pos.getField() == PATTERN_INDEX_TO_DATE_FORMAT_FIELD[patternCharIndex]) { + if (pos.getField() == PATTERN_INDEX_TO_DATE_FORMAT_FIELD[outputCharIndex]) { pos.setBeginIndex(beginOffset); pos.setEndIndex(beginOffset + buf.length() - bufstart); } else if (pos.getFieldAttribute() == - PATTERN_INDEX_TO_DATE_FORMAT_ATTRIBUTE[patternCharIndex]) { + PATTERN_INDEX_TO_DATE_FORMAT_ATTRIBUTE[outputCharIndex]) { pos.setBeginIndex(beginOffset); pos.setEndIndex(beginOffset + buf.length() - bufstart); } @@ -4368,10 +4381,10 @@ public class SimpleDateFormat extends DateFormat { PatternItem item = (PatternItem)items[i]; if (useFastFormat) { subFormat(appendTo, item.type, item.length, appendTo.length(), - i, capSetting, pos, fromCalendar); + i, capSetting, pos, item.type, fromCalendar); } else { appendTo.append(subFormat(item.type, item.length, appendTo.length(), - i, capSetting, pos, fromCalendar)); + i, capSetting, pos, item.type, fromCalendar)); } } } @@ -4386,10 +4399,10 @@ public class SimpleDateFormat extends DateFormat { PatternItem item = (PatternItem)items[i]; if (useFastFormat) { subFormat(appendTo, item.type, item.length, appendTo.length(), - i, capSetting, pos, toCalendar); + i, capSetting, pos, item.type, toCalendar); } else { appendTo.append(subFormat(item.type, item.length, appendTo.length(), - i, capSetting, pos, toCalendar)); + i, capSetting, pos, item.type, toCalendar)); } } } diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/DateFormatTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/DateFormatTest.java index 5b28a76d334..42b9ed33038 100644 --- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/DateFormatTest.java +++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/DateFormatTest.java @@ -5462,30 +5462,36 @@ public class DateFormatTest extends TestFmwk { @Test public void test20741_ABFields() { + String [] skeletons = {"EEEEEBBBBB", "EEEEEbbbbb"}; ULocale[] locales = ULocale.getAvailableLocales(); - for (int i = 0; i < locales.length; i++) { - ULocale locale = locales[i]; - if (isQuick() && (i % 17 != 0)) continue; + for (String skeleton : skeletons) { + for (int i = 0; i < locales.length; i++) { + ULocale locale = locales[i]; + if (isQuick() && (i % 17 != 0)) continue; - DateTimePatternGenerator gen = DateTimePatternGenerator.getInstance(locale); - String pattern = gen.getBestPattern("EEEEEBBBBB"); - SimpleDateFormat dateFormat = new SimpleDateFormat(pattern, locale); - Calendar calendar = Calendar.getInstance(TimeZone.getTimeZone("PST8PDT")); - calendar.setTime(new Date(0)); + DateTimePatternGenerator gen = DateTimePatternGenerator.getInstance(locale); + String pattern = gen.getBestPattern(skeleton); + SimpleDateFormat dateFormat = new SimpleDateFormat(pattern, locale); + Calendar calendar = Calendar.getInstance(TimeZone.getTimeZone("PST8PDT")); + calendar.setTime(new Date(0)); - FieldPosition pos_c = new FieldPosition(DateFormat.Field.DAY_OF_WEEK); - dateFormat.format(calendar, new StringBuffer(""), pos_c); - assertFalse("'Day of week' field was not found", pos_c.getBeginIndex() == 0 && pos_c.getEndIndex() == 0); + FieldPosition pos_c = new FieldPosition(DateFormat.Field.DAY_OF_WEEK); + dateFormat.format(calendar, new StringBuffer(""), pos_c); + assertFalse("'Day of week' field was not found", pos_c.getBeginIndex() == 0 && pos_c.getEndIndex() == 0); - FieldPosition pos_B = new FieldPosition(DateFormat.Field.FLEXIBLE_DAY_PERIOD); - dateFormat.format(calendar, new StringBuffer(""), pos_B); - assertFalse("'Flexible day period' field was not found", pos_B.getBeginIndex() == 0 && pos_B.getEndIndex() == 0); + if (skeleton.equals("EEEEEBBBBB")) { + FieldPosition pos_B = new FieldPosition(DateFormat.Field.FLEXIBLE_DAY_PERIOD); + dateFormat.format(calendar, new StringBuffer(""), pos_B); + assertFalse("'Flexible day period' field was not found", pos_B.getBeginIndex() == 0 && pos_B.getEndIndex() == 0); + } else { + FieldPosition pos_b = new FieldPosition(DateFormat.Field.AM_PM_MIDNIGHT_NOON); + dateFormat.format(calendar, new StringBuffer(""), pos_b); + assertFalse("'AM/PM/Midnight/Noon' field was not found", pos_b.getBeginIndex() == 0 && pos_b.getEndIndex() == 0); + } - FieldPosition pos_a = new FieldPosition(DateFormat.Field.AM_PM); - dateFormat.format(calendar, new StringBuffer(""), pos_a); - if (pos_B.getBeginIndex() == pos_a.getBeginIndex() && pos_B.getEndIndex() == pos_a.getEndIndex()) { - if (logKnownIssue("20741", "Format Fields reports same field position as \"a\" and \"B\"")) continue; - fail("Duplicated field found"); + FieldPosition pos_a = new FieldPosition(DateFormat.Field.AM_PM); + dateFormat.format(calendar, new StringBuffer(""), pos_a); + assertTrue("'AM/PM' field was found", pos_a.getBeginIndex() == 0 && pos_a.getEndIndex() == 0); } } }