ICU-21351 Don't coalesce adjacent list formatter fields in ICU4J

diff --git a/icu4c/source/i18n/formattedval_sbimpl.cpp b/icu4c/source/i18n/formattedval_sbimpl.cpp
index 84c2d00666c2be7ead5dec85ddcfb966dd3e1616..64601f436ba01cf7e3673e6525dc6f472fc03de0 100644 (file)
--- a/icu4c/source/i18n/formattedval_sbimpl.cpp
+++ b/icu4c/source/i18n/formattedval_sbimpl.cpp
@@ -170,7 +170,10 @@ bool FormattedValueStringBuilderImpl::nextPositionImpl(ConstrainedFieldPosition&
             auto elementField = fString.getFieldPtr()[i-1];
             if (elementField == Field(UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD)
                     && cfpos.matchesField(elementField.getCategory(), elementField.getField())
-                    && (cfpos.getLimit() < i - fString.fZero || cfpos.getCategory() != elementField.getCategory())) {
+                    && (
+                        cfpos.getLimit() < i - fString.fZero
+                        || cfpos.getCategory() != elementField.getCategory()
+                        || cfpos.getField() != elementField.getField())) {
                 int64_t si = cfpos.getInt64IterationContext() - 1;
                 cfpos.setState(
                     elementField.getCategory(),

diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/FormattedValueStringBuilderImpl.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/FormattedValueStringBuilderImpl.java
index 43f0c4df1762ff5b8c03705b38c8cd8b3879c1ef..938eb13e2daaea09839db125cae46c6e325f2404 100644 (file)
--- a/icu4j/main/classes/core/src/com/ibm/icu/impl/FormattedValueStringBuilderImpl.java
+++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/FormattedValueStringBuilderImpl.java
@@ -34,6 +34,7 @@ public class FormattedValueStringBuilderImpl {
         public UFormat.SpanField spanField;
         public Field normalField;
         public Object value;
+        public int length;
     }
 
     /**
@@ -142,12 +143,6 @@ public class FormattedValueStringBuilderImpl {
             if (currField != null) {
                 if (currField != _field) {
                     int end = i - self.zero;
-                    // Handle span fields; don't trim them
-                    if (currField instanceof SpanFieldPlaceholder) {
-                        boolean handleResult = handleSpan(currField, cfpos, fieldStart, end);
-                        assert handleResult;
-                        return true;
-                    }
                     // Grouping separators can be whitespace; don't throw them out!
                     if (isTrimmable(currField)) {
                         end = trimBack(self, end);
@@ -188,6 +183,7 @@ public class FormattedValueStringBuilderImpl {
                     && (i - self.zero > cfpos.getLimit() || cfpos.getField() != numericField)
                     && isNumericField(self.fields[i - 1])
                     && !isNumericField(_field)) {
+                // Re-wind to the beginning of the field and then emit it
                 int j = i - 1;
                 for (; j >= self.zero && isNumericField(self.fields[j]); j--) {}
                 cfpos.setState(numericField, null, j - self.zero + 1, i - self.zero);
@@ -196,9 +192,11 @@ public class FormattedValueStringBuilderImpl {
             // Special case: emit normalField if we are pointing at the end of spanField.
             if (i > self.zero
                     && self.fields[i-1] instanceof SpanFieldPlaceholder) {
-                int j = i - 1;
-                for (; j >= self.zero && self.fields[j] == self.fields[i-1]; j--) {}
-                if (handleSpan(self.fields[i-1], cfpos, j - self.zero + 1, i - self.zero)) {
+                SpanFieldPlaceholder ph = (SpanFieldPlaceholder) self.fields[i-1];
+                if (cfpos.matchesField(ph.normalField, null)
+                        && (cfpos.getLimit() < i - self.zero
+                            || cfpos.getField() != ph.normalField)) {
+                    cfpos.setState(ph.normalField, null, i - self.zero - ph.length, i - self.zero);
                     return true;
                 }
             }
@@ -214,9 +212,15 @@ public class FormattedValueStringBuilderImpl {
             // Case 3a: SpanField placeholder
             if (_field instanceof SpanFieldPlaceholder) {
                 SpanFieldPlaceholder ph = (SpanFieldPlaceholder) _field;
-                if (cfpos.matchesField(ph.normalField, null) || cfpos.matchesField(ph.spanField, ph.value)) {
+                if (cfpos.matchesField(ph.spanField, ph.value)) {
                     fieldStart = i - self.zero;
-                    currField = _field;
+                    int end = fieldStart + ph.length;
+                    cfpos.setState(ph.spanField, ph.value, fieldStart, end);
+                    return true;
+                } else {
+                    // Failed to match; jump ahead
+                    i += ph.length - 1;
+                    continue;
                 }
             }
             // Case 3b: No SpanField
@@ -258,19 +262,4 @@ public class FormattedValueStringBuilderImpl {
         return StaticUnicodeSets.get(StaticUnicodeSets.Key.DEFAULT_IGNORABLES)
                 .span(self, start, UnicodeSet.SpanCondition.CONTAINED);
     }
-
-    private static boolean handleSpan(Object field, ConstrainedFieldPosition cfpos, int start, int limit) {
-        SpanFieldPlaceholder ph = (SpanFieldPlaceholder) field;
-        if (cfpos.matchesField(ph.spanField, ph.value)
-                && cfpos.getLimit() < limit) {
-            cfpos.setState(ph.spanField, ph.value, start, limit);
-            return true;
-        }
-        if (cfpos.matchesField(ph.normalField, null)
-                && (cfpos.getLimit() < limit || cfpos.getField() != ph.normalField)) {
-            cfpos.setState(ph.normalField, null, start, limit);
-            return true;
-        }
-        return false;
-    }
 }

diff --git a/icu4j/main/classes/core/src/com/ibm/icu/text/ListFormatter.java b/icu4j/main/classes/core/src/com/ibm/icu/text/ListFormatter.java
index 10503d45ea31325109f3b2bc7c666ac90e6b69c5..e746e8f707adae1b40f6d3bef3c2b782295bc965 100644 (file)
--- a/icu4j/main/classes/core/src/com/ibm/icu/text/ListFormatter.java
+++ b/icu4j/main/classes/core/src/com/ibm/icu/text/ListFormatter.java
@@ -724,14 +724,16 @@ final public class ListFormatter {
         }
 
         private void appendElement(Object element, int position) {
+            String elementString = element.toString();
             if (needsFields) {
                 SpanFieldPlaceholder field = new SpanFieldPlaceholder();
                 field.spanField = SpanField.LIST_SPAN;
                 field.normalField = Field.ELEMENT;
                 field.value = position;
-                string.append(element.toString(), field);
+                field.length = elementString.length();
+                string.append(elementString, field);
             } else {
-                string.append(element.toString(), null);
+                string.append(elementString, null);
             }
         }
 

diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/FormattedValueTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/FormattedValueTest.java
index 51a39f47ed2a3bb2fda95b9b608799c9741cc83c..227abdd86e474b3741d10445ff0a0817dfa41412 100644 (file)
--- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/FormattedValueTest.java
+++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/FormattedValueTest.java
@@ -148,6 +148,11 @@ public class FormattedValueTest {
 
     public static void checkFormattedValue(String message, FormattedValue fv, String expectedString,
             Object[][] expectedFieldPositions) {
+        checkFormattedValue(message, fv, expectedString, expectedFieldPositions, false);
+    }
+
+    public static void checkFormattedValue(String message, FormattedValue fv, String expectedString,
+            Object[][] expectedFieldPositions, boolean skipAttributedCharacterIterator) {
         // Calculate some initial expected values
         int stringLength = fv.toString().length();
         HashSet<Format.Field> uniqueFields = new HashSet<>();
@@ -169,6 +174,12 @@ public class FormattedValueTest {
         assertEquals(baseMessage + "Iterator should have length of string output", stringLength, fpi.getEndIndex());
         int i = 0;
         for (char c = fpi.first(); c != AttributedCharacterIterator.DONE; c = fpi.next(), i++) {
+            // Strings with adjacent fields cannot be disambiguated using AttributedCharacterIterator,
+            // so skip this part of the test for strings with adjacent fields.
+            if (skipAttributedCharacterIterator) {
+                i = stringLength;
+                break;
+            }
             Set<AttributedCharacterIterator.Attribute> currentAttributes = fpi.getAttributes().keySet();
             int attributesRemaining = currentAttributes.size();
             for (Object[] cas : expectedFieldPositions) {
@@ -181,22 +192,22 @@ public class FormattedValueTest {
                     continue;
                 }
 
-                assertTrue(baseMessage + "Character at " + i + " should have field " + expectedField,
+                assertTrue(baseMessage + "A Character at " + i + " should have field " + expectedField,
                         currentAttributes.contains(expectedField));
-                assertTrue(baseMessage + "Field " + expectedField + " should be a known attribute",
+                assertTrue(baseMessage + "A Field " + expectedField + " should be a known attribute",
                         allAttributes.contains(expectedField));
                 int actualBeginIndex = fpi.getRunStart(expectedField);
                 int actualEndIndex = fpi.getRunLimit(expectedField);
                 Object actualValue = fpi.getAttribute(expectedField);
-                assertEquals(baseMessage + expectedField + " begin @" + i, expectedBeginIndex, actualBeginIndex);
-                assertEquals(baseMessage + expectedField + " end @" + i, expectedEndIndex, actualEndIndex);
-                assertEquals(baseMessage + expectedField + " value @" + i, expectedValue, actualValue);
+                assertEquals(baseMessage + expectedField + " A begin @" + i, expectedBeginIndex, actualBeginIndex);
+                assertEquals(baseMessage + expectedField + " A end @" + i, expectedEndIndex, actualEndIndex);
+                assertEquals(baseMessage + expectedField + " A value @" + i, expectedValue, actualValue);
                 attributesRemaining--;
             }
-            assertEquals(baseMessage + "Should have looked at every field: " + i + ": " + currentAttributes,
+            assertEquals(baseMessage + "A Should have looked at every field: " + i + ": " + currentAttributes,
                     0, attributesRemaining);
         }
-        assertEquals(baseMessage + "Should have looked at every character", stringLength, i);
+        assertEquals(baseMessage + "A Should have looked at every character", stringLength, i);
 
         // Check nextPosition over all fields
         ConstrainedFieldPosition cfpos = new ConstrainedFieldPosition();
@@ -207,16 +218,16 @@ public class FormattedValueTest {
             int expectedStart = (Integer) cas[1];
             int expectedLimit = (Integer) cas[2];
             Object expectedValue = cas.length == 4 ? cas[3] : null;
-            assertEquals(baseMessage + "field " + i, expectedField, cfpos.getField());
-            assertEquals(baseMessage + "start " + i, expectedStart, cfpos.getStart());
-            assertEquals(baseMessage + "limit " + i, expectedLimit, cfpos.getLimit());
-            assertEquals(baseMessage + "value " + i, expectedValue, cfpos.getFieldValue());
+            assertEquals(baseMessage + "B field " + i, expectedField, cfpos.getField());
+            assertEquals(baseMessage + "B start " + i, expectedStart, cfpos.getStart());
+            assertEquals(baseMessage + "B limit " + i, expectedLimit, cfpos.getLimit());
+            assertEquals(baseMessage + "B value " + i, expectedValue, cfpos.getFieldValue());
             i++;
         }
         boolean afterLoopResult = fv.nextPosition(cfpos);
-        assertFalse(baseMessage + "after loop: " + cfpos, afterLoopResult);
+        assertFalse(baseMessage + "B after loop: " + cfpos, afterLoopResult);
         afterLoopResult = fv.nextPosition(cfpos);
-        assertFalse(baseMessage + "after loop again: " + cfpos, afterLoopResult);
+        assertFalse(baseMessage + "B after loop again: " + cfpos, afterLoopResult);
 
         // Check nextPosition constrained over each class one at a time
         for (Class<?> classConstraint : uniqueFieldClasses) {
@@ -232,22 +243,22 @@ public class FormattedValueTest {
                 int expectedStart = (Integer) cas[1];
                 int expectedLimit = (Integer) cas[2];
                 Object expectedValue = cas.length == 4 ? cas[3] : null;
-                assertEquals(baseMessage + "field " + i, expectedField, cfpos.getField());
-                assertEquals(baseMessage + "start " + i, expectedStart, cfpos.getStart());
-                assertEquals(baseMessage + "limit " + i, expectedLimit, cfpos.getLimit());
-                assertEquals(baseMessage + "value " + i, expectedValue, cfpos.getFieldValue());
+                assertEquals(baseMessage + "C field " + i, expectedField, cfpos.getField());
+                assertEquals(baseMessage + "C start " + i, expectedStart, cfpos.getStart());
+                assertEquals(baseMessage + "C limit " + i, expectedLimit, cfpos.getLimit());
+                assertEquals(baseMessage + "C value " + i, expectedValue, cfpos.getFieldValue());
                 i++;
             }
             afterLoopResult = fv.nextPosition(cfpos);
-            assertFalse(baseMessage + "after loop: " + cfpos, afterLoopResult);
+            assertFalse(baseMessage + "C after loop: " + cfpos, afterLoopResult);
             afterLoopResult = fv.nextPosition(cfpos);
-            assertFalse(baseMessage + "after loop again: " + cfpos, afterLoopResult);
+            assertFalse(baseMessage + "C after loop again: " + cfpos, afterLoopResult);
         }
 
         // Check nextPosition constrained over an unrelated class
         cfpos.reset();
         cfpos.constrainClass(HashSet.class);
-        assertFalse(baseMessage + "unrelated class", fv.nextPosition(cfpos));
+        assertFalse(baseMessage + "C unrelated class", fv.nextPosition(cfpos));
 
         // Check nextPosition constrained over each field one at a time
         for (Format.Field field : uniqueFields) {
@@ -263,16 +274,16 @@ public class FormattedValueTest {
                 int expectedStart = (Integer) cas[1];
                 int expectedLimit = (Integer) cas[2];
                 Object expectedValue = cas.length == 4 ? cas[3] : null;
-                assertEquals(baseMessage + "field " + i, expectedField, cfpos.getField());
-                assertEquals(baseMessage + "start " + i, expectedStart, cfpos.getStart());
-                assertEquals(baseMessage + "limit " + i, expectedLimit, cfpos.getLimit());
-                assertEquals(baseMessage + "value " + i, expectedValue, cfpos.getFieldValue());
+                assertEquals(baseMessage + "D field " + i, expectedField, cfpos.getField());
+                assertEquals(baseMessage + "D start " + i, expectedStart, cfpos.getStart());
+                assertEquals(baseMessage + "D limit " + i, expectedLimit, cfpos.getLimit());
+                assertEquals(baseMessage + "D value " + i, expectedValue, cfpos.getFieldValue());
                 i++;
             }
             afterLoopResult = fv.nextPosition(cfpos);
-            assertFalse(baseMessage + "after loop: " + cfpos, afterLoopResult);
+            assertFalse(baseMessage + "D after loop: " + cfpos, afterLoopResult);
             afterLoopResult = fv.nextPosition(cfpos);
-            assertFalse(baseMessage + "after loop again: " + cfpos, afterLoopResult);
+            assertFalse(baseMessage + "D after loop again: " + cfpos, afterLoopResult);
         }
     }
 

diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/ListFormatterTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/ListFormatterTest.java
index 92977524bc864d72546f6b41837a298ca73805b0..9407bd5f85a65bc4f8a1b149d5c1b67841fbaf62 100644 (file)
--- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/ListFormatterTest.java
+++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/ListFormatterTest.java
@@ -262,15 +262,15 @@ public class ListFormatterTest extends TestFmwk {
                 {ListFormatter.Field.ELEMENT, 2, 9},
                 {ListFormatter.SpanField.LIST_SPAN, 9, 12, 2},
                 {ListFormatter.Field.ELEMENT, 9, 12}};
-            if (!logKnownIssue("21351", "Java still coalesces adjacent elements")) {
-                FormattedValueTest.checkFormattedValue(
-                    message,
-                    result,
-                    expectedString,
-                    expectedFieldPositions);
-            }
+            FormattedValueTest.checkFormattedValue(
+                message,
+                result,
+                expectedString,
+                expectedFieldPositions,
+                // Adjacent fields: skip AttributedCharacterIterator test
+                true);
         }
-    
+
         {
             ListFormatter fmt = ListFormatter.getInstance(ULocale.ENGLISH, Type.UNITS, Width.SHORT);
             String message = "ICU-21383 Long list";