From f48ecca6f7e12c25d734e25d4c8fe8e5e6b5d2bd Mon Sep 17 00:00:00 2001 From: Shane Carr Date: Thu, 16 Nov 2017 23:25:16 +0000 Subject: [PATCH] ICU-13477 Tweak the DecimalFormat mapping function to correctly handle positive prefix and suffix overrides. X-SVN-Rev: 40672 --- .../com/ibm/icu/impl/number/AffixUtils.java | 3 +- .../ibm/icu/number/NumberPropertyMapper.java | 140 +++++++++++------- .../icu/dev/test/format/NumberFormatTest.java | 21 +++ 3 files changed, 110 insertions(+), 54 deletions(-) diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/AffixUtils.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/AffixUtils.java index a64c3d708cf..bf324514688 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/AffixUtils.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/AffixUtils.java @@ -225,8 +225,9 @@ public class AffixUtils { return output.length() - startLength; } - /** Version of {@link #escape} that returns a String. */ + /** Version of {@link #escape} that returns a String, or null if input is null. */ public static String escape(CharSequence input) { + if (input == null) return null; StringBuilder sb = new StringBuilder(); escape(input, sb); return sb.toString(); diff --git a/icu4j/main/classes/core/src/com/ibm/icu/number/NumberPropertyMapper.java b/icu4j/main/classes/core/src/com/ibm/icu/number/NumberPropertyMapper.java index 9d681b56c7f..5c6a08c0103 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/number/NumberPropertyMapper.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/number/NumberPropertyMapper.java @@ -86,15 +86,7 @@ final class NumberPropertyMapper { AffixPatternProvider affixProvider; if (properties.getCurrencyPluralInfo() == null) { - affixProvider = new PropertiesAffixPatternProvider( - properties.getPositivePrefix() != null ? AffixUtils.escape(properties.getPositivePrefix()) - : properties.getPositivePrefixPattern(), - properties.getPositiveSuffix() != null ? AffixUtils.escape(properties.getPositiveSuffix()) - : properties.getPositiveSuffixPattern(), - properties.getNegativePrefix() != null ? AffixUtils.escape(properties.getNegativePrefix()) - : properties.getNegativePrefixPattern(), - properties.getNegativeSuffix() != null ? AffixUtils.escape(properties.getNegativeSuffix()) - : properties.getNegativeSuffixPattern()); + affixProvider = new PropertiesAffixPatternProvider(properties); } else { affixProvider = new CurrencyPluralInfoAffixProvider(properties.getCurrencyPluralInfo()); } @@ -329,85 +321,127 @@ final class NumberPropertyMapper { } private static class PropertiesAffixPatternProvider implements AffixPatternProvider { - private final String posPrefixPattern; - private final String posSuffixPattern; - private final String negPrefixPattern; - private final String negSuffixPattern; - - public PropertiesAffixPatternProvider(String ppp, String psp, String npp, String nsp) { - if (ppp == null) - ppp = ""; - if (psp == null) - psp = ""; - if (npp == null && nsp != null) - npp = "-"; // TODO: This is a hack. - if (nsp == null && npp != null) - nsp = ""; - posPrefixPattern = ppp; - posSuffixPattern = psp; - negPrefixPattern = npp; - negSuffixPattern = nsp; + private final String posPrefix; + private final String posSuffix; + private final String negPrefix; + private final String negSuffix; + + public PropertiesAffixPatternProvider(DecimalFormatProperties properties) { + // There are two ways to set affixes in DecimalFormat: via the pattern string (applyPattern), and via the + // explicit setters (setPositivePrefix and friends). The way to resolve the settings is as follows: + // + // 1) If the explicit setting is present for the field, use it. + // 2) Otherwise, follows UTS 35 rules based on the pattern string. + // + // Importantly, the explicit setters affect only the one field they override. If you set the positive + // prefix, that should not affect the negative prefix. Since it is impossible for the user of this class + // to know whether the origin for a string was the override or the pattern, we have to say that we always + // have a negative subpattern and perform all resolution logic here. + + // Convenience: Extract the properties into local variables. + // Variables are named with three chars: [p/n][p/s][o/p] + // [p/n] => p for positive, n for negative + // [p/s] => p for prefix, s for suffix + // [o/p] => o for escaped custom override string, p for pattern string + String ppo = AffixUtils.escape(properties.getPositivePrefix()); + String pso = AffixUtils.escape(properties.getPositiveSuffix()); + String npo = AffixUtils.escape(properties.getNegativePrefix()); + String nso = AffixUtils.escape(properties.getNegativeSuffix()); + String ppp = properties.getPositivePrefixPattern(); + String psp = properties.getPositiveSuffixPattern(); + String npp = properties.getNegativePrefixPattern(); + String nsp = properties.getNegativeSuffixPattern(); + + if (ppo != null) { + posPrefix = ppo; + } else if (ppp != null) { + posPrefix = ppp; + } else { + // UTS 35: Default positive prefix is empty string. + posPrefix = ""; + } + + if (pso != null) { + posSuffix = pso; + } else if (psp != null) { + posSuffix = psp; + } else { + // UTS 35: Default positive suffix is empty string. + posSuffix = ""; + } + + if (npo != null) { + negPrefix = npo; + } else if (npp != null) { + negPrefix = npp; + } else { + // UTS 35: Default negative prefix is "-" with positive prefix. + // Important: We prepend the "-" to the pattern, not the override! + negPrefix = ppp == null ? "-" : "-" + ppp; + } + + if (nso != null) { + negSuffix = nso; + } else if (nsp != null) { + negSuffix = nsp; + } else { + // UTS 35: Default negative prefix is the positive prefix. + negSuffix = psp == null ? "" : psp; + } } @Override public char charAt(int flags, int i) { - boolean prefix = (flags & Flags.PREFIX) != 0; - boolean negative = (flags & Flags.NEGATIVE_SUBPATTERN) != 0; - if (prefix && negative) { - return negPrefixPattern.charAt(i); - } else if (prefix) { - return posPrefixPattern.charAt(i); - } else if (negative) { - return negSuffixPattern.charAt(i); - } else { - return posSuffixPattern.charAt(i); - } + return getStringForFlags(flags).charAt(i); } @Override public int length(int flags) { + return getStringForFlags(flags).length(); + } + + private String getStringForFlags(int flags) { boolean prefix = (flags & Flags.PREFIX) != 0; boolean negative = (flags & Flags.NEGATIVE_SUBPATTERN) != 0; if (prefix && negative) { - return negPrefixPattern.length(); + return negPrefix; } else if (prefix) { - return posPrefixPattern.length(); + return posPrefix; } else if (negative) { - return negSuffixPattern.length(); + return negSuffix; } else { - return posSuffixPattern.length(); + return posSuffix; } } @Override public boolean positiveHasPlusSign() { - return AffixUtils.containsType(posPrefixPattern, AffixUtils.TYPE_PLUS_SIGN) - || AffixUtils.containsType(posSuffixPattern, AffixUtils.TYPE_PLUS_SIGN); + return AffixUtils.containsType(posPrefix, AffixUtils.TYPE_PLUS_SIGN) + || AffixUtils.containsType(posSuffix, AffixUtils.TYPE_PLUS_SIGN); } @Override public boolean hasNegativeSubpattern() { - return negPrefixPattern != null; + // See comments in the constructor for more information on why this is always true. + return true; } @Override public boolean negativeHasMinusSign() { - return AffixUtils.containsType(negPrefixPattern, AffixUtils.TYPE_MINUS_SIGN) - || AffixUtils.containsType(negSuffixPattern, AffixUtils.TYPE_MINUS_SIGN); + return AffixUtils.containsType(negPrefix, AffixUtils.TYPE_MINUS_SIGN) + || AffixUtils.containsType(negSuffix, AffixUtils.TYPE_MINUS_SIGN); } @Override public boolean hasCurrencySign() { - return AffixUtils.hasCurrencySymbols(posPrefixPattern) || AffixUtils.hasCurrencySymbols(posSuffixPattern) - || AffixUtils.hasCurrencySymbols(negPrefixPattern) - || AffixUtils.hasCurrencySymbols(negSuffixPattern); + return AffixUtils.hasCurrencySymbols(posPrefix) || AffixUtils.hasCurrencySymbols(posSuffix) + || AffixUtils.hasCurrencySymbols(negPrefix) || AffixUtils.hasCurrencySymbols(negSuffix); } @Override public boolean containsSymbolType(int type) { - return AffixUtils.containsType(posPrefixPattern, type) || AffixUtils.containsType(posSuffixPattern, type) - || AffixUtils.containsType(negPrefixPattern, type) - || AffixUtils.containsType(negSuffixPattern, type); + return AffixUtils.containsType(posPrefix, type) || AffixUtils.containsType(posSuffix, type) + || AffixUtils.containsType(negPrefix, type) || AffixUtils.containsType(negSuffix, type); } } diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java index aabb7ce3ae0..3bb777eb641 100644 --- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java +++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java @@ -5320,6 +5320,13 @@ public class NumberFormatTest extends TestFmwk { 130, resultScientific.longValue()); } + @Test + public void Test13442() { + DecimalFormat df = new DecimalFormat(); + df.setGroupingUsed(false); + assertEquals("Grouping size should now be zero", 0, df.getGroupingSize()); + } + @Test public void testPercentZero() { DecimalFormat df = (DecimalFormat) NumberFormat.getPercentInstance(); @@ -5894,4 +5901,18 @@ public class NumberFormatTest extends TestFmwk { assertEquals("Narrow currency symbol for USD in en_CA is $", "$123.45", df.format(123.45)); } + + @Test + public void TestAffixOverrideBehavior() { + DecimalFormat df = (DecimalFormat) NumberFormat.getInstance(ULocale.ENGLISH); + expect2(df, 100, "100"); + expect2(df, -100, "-100"); + // This is not the right way to set an override plus sign, but we need to support it for compatibility. + df.setPositivePrefix("+"); + expect2(df, 100, "+100"); + expect2(df, -100, "-100"); // note: the positive prefix does not affect the negative prefix + df.applyPattern("a0"); + expect2(df, 100, "a100"); + expect2(df, -100, "-a100"); + } } -- 2.40.0