ICU-20568 Improve negative measure handling for mixed units
See #1379
diff --git a/icu4c/source/i18n/number_longnames.cpp b/icu4c/source/i18n/number_longnames.cpp
index 97a5973..8b33992 100644
--- a/icu4c/source/i18n/number_longnames.cpp
+++ b/icu4c/source/i18n/number_longnames.cpp
@@ -455,6 +455,8 @@
status = U_UNSUPPORTED_ERROR;
return µs.helpers.emptyWeakModifier;
}
+ // If we don't have at least one mixedMeasure, the LongNameHandler would be
+ // sufficient and we shouldn't be running MixedUnitLongNameHandler code:
U_ASSERT(micros.mixedMeasuresCount > 0);
// mixedMeasures does not contain the last value:
U_ASSERT(fMixedUnitCount == micros.mixedMeasuresCount + 1);
@@ -486,6 +488,11 @@
for (int32_t i = 0; i < micros.mixedMeasuresCount; i++) {
DecimalQuantity fdec;
fdec.setToLong(micros.mixedMeasures[i]);
+ if (i > 0 && fdec.isNegative()) {
+ // If numbers are negative, only the first number needs to have its
+ // negative sign formatted.
+ fdec.negate();
+ }
StandardPlural::Form pluralForm = utils::getStandardPlural(rules, fdec);
UnicodeString simpleFormat =
@@ -499,6 +506,13 @@
// TODO(icu-units#67): fix field positions
}
+ // Reiterated: we have at least one mixedMeasure:
+ U_ASSERT(micros.mixedMeasuresCount > 0);
+ // Thus if negative, a negative has already been formatted:
+ if (quantity.isNegative()) {
+ quantity.negate();
+ }
+
UnicodeString *finalSimpleFormats = &fMixedUnitData[(fMixedUnitCount - 1) * ARRAY_LENGTH];
StandardPlural::Form finalPlural = utils::getPluralSafe(micros.rounder, rules, quantity, status);
UnicodeString finalSimpleFormat = getWithPlural(finalSimpleFormats, finalPlural, status);
diff --git a/icu4c/source/i18n/units_complexconverter.cpp b/icu4c/source/i18n/units_complexconverter.cpp
index 9775b6a..27f835e 100644
--- a/icu4c/source/i18n/units_complexconverter.cpp
+++ b/icu4c/source/i18n/units_complexconverter.cpp
@@ -110,9 +110,13 @@
MaybeStackVector<Measure> ComplexUnitsConverter::convert(double quantity,
icu::number::impl::RoundingImpl *rounder,
UErrorCode &status) const {
- // TODO(icu-units#63): test negative numbers!
// TODO(hugovdm): return an error for "foot-and-foot"?
MaybeStackVector<Measure> result;
+ int sign = 1;
+ if (quantity < 0) {
+ quantity *= -1;
+ sign = -1;
+ }
// For N converters:
// - the first converter converts from the input unit to the largest unit,
@@ -190,7 +194,7 @@
// Package values into Measure instances in result:
for (int i = 0, n = unitConverters_.length(); i < n; ++i) {
if (i < n - 1) {
- Formattable formattableQuantity(intValues[i]);
+ Formattable formattableQuantity(intValues[i] * sign);
// Measure takes ownership of the MeasureUnit*
MeasureUnit *type = new MeasureUnit(units_[i]->copy(status).build(status));
if (result.emplaceBackAndCheckErrorCode(status, formattableQuantity, type, status) ==
@@ -204,7 +208,7 @@
}
} else { // LAST ELEMENT
// Add the last element, not an integer:
- Formattable formattableQuantity(quantity);
+ Formattable formattableQuantity(quantity * sign);
// Measure takes ownership of the MeasureUnit*
MeasureUnit *type = new MeasureUnit(units_[i]->copy(status).build(status));
if (result.emplaceBackAndCheckErrorCode(status, formattableQuantity, type, status) ==
diff --git a/icu4c/source/i18n/units_router.cpp b/icu4c/source/i18n/units_router.cpp
index 9489a8d..3158718 100644
--- a/icu4c/source/i18n/units_router.cpp
+++ b/icu4c/source/i18n/units_router.cpp
@@ -15,6 +15,7 @@
#include "unicode/measure.h"
#include "units_data.h"
#include "units_router.h"
+#include <cmath>
U_NAMESPACE_BEGIN
namespace units {
@@ -96,7 +97,7 @@
const ConverterPreference *converterPreference = nullptr;
for (int32_t i = 0, n = converterPreferences_.length(); i < n; i++) {
converterPreference = converterPreferences_[i];
- if (converterPreference->converter.greaterThanOrEqual(quantity * (1 + DBL_EPSILON),
+ if (converterPreference->converter.greaterThanOrEqual(std::abs(quantity) * (1 + DBL_EPSILON),
converterPreference->limit)) {
break;
}
diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp
index ca2170d..43323fb 100644
--- a/icu4c/source/test/intltest/numbertest_api.cpp
+++ b/icu4c/source/test/intltest/numbertest_api.cpp
@@ -764,6 +764,24 @@
Locale("en-US"),
1.9999,
u"2 feet, 0 inches");
+
+ assertFormatSingle(
+ u"Negative numbers: temperature",
+ u"measure-unit/temperature-celsius",
+ u"unit/celsius",
+ NumberFormatter::with().unit(MeasureUnit::forIdentifier("celsius", status)),
+ Locale("nl-NL"),
+ -6.5,
+ u"-6,5\u00B0C");
+
+ assertFormatSingle(
+ u"Negative numbers: time",
+ nullptr, // submitting after TODO(icu-units#35) is fixed: fill in skeleton!
+ u"unit/hour-and-minute-and-second",
+ NumberFormatter::with().unit(MeasureUnit::forIdentifier("hour-and-minute-and-second", status)),
+ Locale("de-DE"),
+ -1.24,
+ u"-1 Std., 14 Min. und 24 Sek.");
}
void NumberFormatterApiTest::unitCompoundMeasure() {
@@ -863,6 +881,16 @@
2.4,
"2.4 m/s/s");
+ assertFormatSingle(
+ u"Negative numbers: acceleration",
+ u"measure-unit/acceleration-meter-per-square-second",
+ // TODO: when other PRs are merged, try: u"unit/meter-per-second-second" instead:
+ u"measure-unit/acceleration-meter-per-square-second",
+ NumberFormatter::with().unit(MeasureUnit::forIdentifier("meter-per-pow2-second", status)),
+ Locale("af-ZA"),
+ -9.81,
+ u"-9,81 m/s\u00B2");
+
// Testing the rejection of invalid specifications
// If .unit() is not given a built-in type, .perUnit() is not allowed
@@ -1135,6 +1163,15 @@
u"0E0 square centimetres");
assertFormatSingle(
+ u"Negative numbers: minute-and-second",
+ u"measure-unit/duration-second usage/media",
+ u"unit/second usage/media",
+ NumberFormatter::with().unit(SECOND).usage("media"),
+ Locale("nl-NL"),
+ -77.7,
+ u"-1 min, 18 sec");
+
+ assertFormatSingle(
u"Rounding Mode propagates: rounding down",
u"usage/road measure-unit/length-centimeter rounding-mode-floor",
u"usage/road unit/centimeter rounding-mode-floor",
diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/MixedUnitLongNameHandler.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/MixedUnitLongNameHandler.java
index 3e22ed0..d1b7965 100644
--- a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/MixedUnitLongNameHandler.java
+++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/MixedUnitLongNameHandler.java
@@ -141,6 +141,8 @@
* unit.
*/
private Modifier getMixedUnitModifier(DecimalQuantity quantity, MicroProps micros) {
+ // If we don't have at least one mixedMeasure, the LongNameHandler would be
+ // sufficient and we shouldn't be running MixedUnitLongNameHandler code:
if (micros.mixedMeasures.size() == 0) {
assert false : "Mixed unit: we must have more than one unit value";
throw new UnsupportedOperationException();
@@ -168,6 +170,11 @@
for (int i = 0; i < micros.mixedMeasures.size(); i++) {
DecimalQuantity fdec = new DecimalQuantity_DualStorageBCD(micros.mixedMeasures.get(i).getNumber());
+ if (i > 0 && fdec.isNegative()) {
+ // If numbers are negative, only the first number needs to have its
+ // negative sign formatted.
+ fdec.negate();
+ }
StandardPlural pluralForm = fdec.getStandardPlural(rules);
String simpleFormat = LongNameHandler.getWithPlural(this.fMixedUnitData.get(i), pluralForm);
@@ -179,6 +186,13 @@
// TODO(icu-units#67): fix field positions
}
+ // Reiterated: we have at least one mixedMeasure:
+ assert micros.mixedMeasures.size() > 0;
+ // Thus if negative, a negative has already been formatted:
+ if (quantity.isNegative()) {
+ quantity.negate();
+ }
+
String[] finalSimpleFormats = this.fMixedUnitData.get(this.fMixedUnitData.size() - 1);
StandardPlural finalPlural = RoundingUtils.getPluralSafe(micros.rounder, rules, quantity);
String finalSimpleFormat = LongNameHandler.getWithPlural(finalSimpleFormats, finalPlural);
diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/ComplexUnitsConverter.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/ComplexUnitsConverter.java
index 2570c90..96e8810 100644
--- a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/ComplexUnitsConverter.java
+++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/ComplexUnitsConverter.java
@@ -105,6 +105,11 @@
*/
public List<Measure> convert(BigDecimal quantity, Precision rounder) {
List<Measure> result = new ArrayList<>(unitConverters_.size());
+ BigDecimal sign = BigDecimal.ONE;
+ if (quantity.compareTo(BigDecimal.ZERO) < 0) {
+ quantity = quantity.abs();
+ sign = sign.negate();
+ }
// For N converters:
// - the first converter converts from the input unit to the largest
@@ -179,9 +184,9 @@
// Package values into Measure instances in result:
for (int i = 0, n = unitConverters_.size(); i < n; ++i) {
if (i < n - 1) {
- result.add(new Measure(intValues.get(i), units_.get(i).build()));
+ result.add(new Measure(intValues.get(i).multiply(sign), units_.get(i).build()));
} else {
- result.add(new Measure(quantity, units_.get(i).build()));
+ result.add(new Measure(quantity.multiply(sign), units_.get(i).build()));
}
}
diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsRouter.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsRouter.java
index 3874f09..d344f3e 100644
--- a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsRouter.java
+++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsRouter.java
@@ -86,7 +86,8 @@
ConverterPreference converterPreference = null;
for (ConverterPreference itr : converterPreferences_) {
converterPreference = itr;
- if (converterPreference.converter.greaterThanOrEqual(quantity, converterPreference.limit)) {
+ if (converterPreference.converter.greaterThanOrEqual(quantity.abs(),
+ converterPreference.limit)) {
break;
}
}
diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java
index 22bd114..89afcf3 100644
--- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java
+++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java
@@ -726,6 +726,24 @@
new ULocale("en-US"),
1.9999,
"2 feet, 0 inches");
+
+ assertFormatSingle(
+ "Negative numbers: temperature",
+ "measure-unit/temperature-celsius",
+ "unit/celsius",
+ NumberFormatter.with().unit(MeasureUnit.forIdentifier("celsius")),
+ new ULocale("nl-NL"),
+ -6.5,
+ "-6,5\u00B0C");
+
+ assertFormatSingle(
+ "Negative numbers: time",
+ null, // submitting after TODO(icu-units#35) is fixed: fill in skeleton!
+ "unit/hour-and-minute-and-second",
+ NumberFormatter.with().unit(MeasureUnit.forIdentifier("hour-and-minute-and-second")),
+ new ULocale("de-DE"),
+ -1.24,
+ "-1 Std., 14 Min. und 24 Sek.");
}
@Test
@@ -824,6 +842,16 @@
2.4,
"2.4 m/s/s");
+ assertFormatSingle(
+ "Negative numbers: acceleration",
+ "measure-unit/acceleration-meter-per-square-second",
+ // TODO: when other PRs are merged, try: u"unit/meter-per-second-second" instead:
+ "measure-unit/acceleration-meter-per-square-second",
+ NumberFormatter.with().unit(MeasureUnit.forIdentifier("meter-per-pow2-second")),
+ new ULocale("af-ZA"),
+ -9.81,
+ "-9,81 m/s\u00B2");
+
// Testing the rejection of invalid specifications
// If .unit() is not given a built-in type, .perUnit() is not allowed
@@ -1092,6 +1120,15 @@
"0E0 square centimetres");
assertFormatSingle(
+ "Negative numbers: minute-and-second",
+ "measure-unit/duration-second usage/media",
+ "unit/second usage/media",
+ NumberFormatter.with().unit(MeasureUnit.SECOND).usage("media"),
+ new ULocale("nl-NL"),
+ -77.7,
+ "-1 min, 18 sec");
+
+ assertFormatSingle(
"Rounding Mode propagates: rounding down",
"usage/road measure-unit/length-centimeter rounding-mode-floor",
"usage/road unit/centimeter rounding-mode-floor",
