Fix digit grouping: m_grouping_top doesn't mean what I thought it did

I'd previously understood CLDR's minimumGroupingDigits to mean the
most significant group must have that many digits. It turns out to
mean only that the first grouping separator doesn't get added unless
the more significant group has this many. Once we have one separator,
more can be added that do isolate a single digit.

In the process, I discover some of the prior arithmetic is incorrect;
it is now fixed. Added some basic testing, amended some existing
tests. In the process, fixed naming of some double validator tests.

Pick-to: 6.6 6.5
Fixes: QTBUG-115740
Change-Id: Ia6ce011ba72e72428b015ca22b97d815ebf751b2
Reviewed-by: Ievgenii Meshcheriakov <ievgenii.meshcheriakov@qt.io>

5 files changed, 119 insertions, 15 deletions

diff --git a/src/corelib/text/qlocale.cpp b/src/corelib/text/qlocale.cpp
index c7ecddb484..7a56501a1a 100644
--- a/src/corelib/text/qlocale.cpp
+++ b/src/corelib/text/qlocale.cpp
@@ -3675,7 +3675,7 @@ QString QLocaleData::doubleToString(double d, int precision, DoubleForm form,
                 int bias = 2 + minExponentDigits;
                 // Decimal form may get grouping separators inserted:
                 if (groupDigits && decpt >= m_grouping_top + m_grouping_least)
-                    bias -= (decpt - m_grouping_top - m_grouping_least) / m_grouping_higher + 1;
+                    bias -= (decpt - m_grouping_least) / m_grouping_higher + 1;
                 // X = decpt - 1 needs two digits if decpt > 10:
                 if (decpt > 10 && minExponentDigits == 1)
                     ++bias;
@@ -3764,7 +3764,7 @@ QString QLocaleData::decimalForm(QString &&digits, int decpt, int precision,
         qsizetype i = decpt - m_grouping_least;
         if (i >= m_grouping_top) {
             digits.insert(i * digitWidth, group);
-            while ((i -= m_grouping_higher) >= m_grouping_top)
+            while ((i -= m_grouping_higher) > 0)
                 digits.insert(i * digitWidth, group);
         }
     }
@@ -3872,7 +3872,7 @@ QString QLocaleData::applyIntegerFormatting(QString &&numStr, bool negative, int
         if (i >= m_grouping_top) {
             numStr.insert(i * digitWidth, group);
             ++usedWidth;
-            while ((i -= m_grouping_higher) >= m_grouping_top) {
+            while ((i -= m_grouping_higher) > 0) {
                 numStr.insert(i * digitWidth, group);
                 ++usedWidth;
             }
@@ -4191,7 +4191,7 @@ bool QLocaleData::numberToCLocale(QStringView s, QLocale::NumberOptions number_o
                 if (last_separator_idx == -1) {
                     // Check distance from the beginning of the digits:
                     if (start_of_digits_idx == -1 || m_grouping_top > digitsInGroup
-                        || digitsInGroup >= m_grouping_higher + m_grouping_top) {
+                        || digitsInGroup >= m_grouping_least + m_grouping_top) {
                         return false;
                     }
                 } else {
diff --git a/src/corelib/text/qlocale_p.h b/src/corelib/text/qlocale_p.h
index 4dcfb0329b..f8814c4417 100644
--- a/src/corelib/text/qlocale_p.h
+++ b/src/corelib/text/qlocale_p.h
@@ -486,7 +486,7 @@ public:
     quint8 m_first_day_of_week : 3;
     quint8 m_weekend_start : 3;
     quint8 m_weekend_end : 3;
-    quint8 m_grouping_top : 2; // Must have this many before the first grouping separator
+    quint8 m_grouping_top : 2; // Don't group until more significant group has this many digits.
     quint8 m_grouping_higher : 3; // Number of digits between grouping separators
     quint8 m_grouping_least : 3; // Number of digits after last grouping separator (before decimal).
 };
diff --git a/tests/auto/corelib/text/qlocale/tst_qlocale.cpp b/tests/auto/corelib/text/qlocale/tst_qlocale.cpp
index 04fae3898b..ad491b6854 100644
--- a/tests/auto/corelib/text/qlocale/tst_qlocale.cpp
+++ b/tests/auto/corelib/text/qlocale/tst_qlocale.cpp
@@ -134,6 +134,8 @@ private slots:
     void systemLocaleDayAndMonthNames();
 #endif
 
+    void numberGrouping_data();
+    void numberGrouping();
     void numberGroupingIndia();
     void numberFormatChakma();
 
@@ -1229,7 +1231,9 @@ void tst_QLocale::doubleToString_data()
     QTest::newRow("de_DE 1245678900 g -")  << QString("de_DE") << QString("1.245.678.900") << 12456789e2 << 'g' << shortest;
     QTest::newRow("de_DE 12456789100 g -") << QString("de_DE") << QString("12.456.789.100") << 124567891e2 << 'g' << shortest;
     QTest::newRow("de_DE 12456789000 g -") << QString("de_DE") << QString("1,2456789E+10")  << 12456789e3 << 'g' << shortest;
-    QTest::newRow("de_DE 120000 g -")  << QString("de_DE") << QString("120.000") << 12e4 << 'g' << shortest;
+    QTest::newRow("de_DE 12000 g -")
+        << QString("de_DE") << QString("12.000") << 12e3 << 'g' << shortest;
+    // 12e4 has "120.000" and "1.2E+05" of equal length; which shortest picks is unspecified.
     QTest::newRow("de_DE 1200000 g -") << QString("de_DE") << QString("1,2E+06") << 12e5 << 'g' << shortest;
     QTest::newRow("de_DE 1000 g -")  << QString("de_DE") << QString("1.000") << 1e3 << 'g' << shortest;
     QTest::newRow("de_DE 10000 g -") << QString("de_DE") << QString("1E+04") << 1e4 << 'g' << shortest;
@@ -4056,6 +4060,90 @@ void tst_QLocale::systemLocaleDayAndMonthNames()
 
 #endif // QT_NO_SYSTEMLOCALE
 
+void tst_QLocale::numberGrouping_data()
+{
+    QTest::addColumn<QLocale>("locale");
+    QTest::addColumn<int>("number");
+    QTest::addColumn<QString>("string");
+    // Number options set here are expected to be default, but set for the
+    // avoidance of uncertainty or susceptibility to changed defaults.
+
+    QLocale c(QLocale::C); // English-style, without separators.
+    c.setNumberOptions(c.numberOptions() | QLocale::OmitGroupSeparator);
+    QTest::newRow("c:1") << c << 1 << u"1"_s;
+    QTest::newRow("c:12") << c << 12 << u"12"_s;
+    QTest::newRow("c:123") << c << 123 << u"123"_s;
+    QTest::newRow("c:1234") << c << 1234 << u"1234"_s;
+    QTest::newRow("c:12345") << c << 12345 << u"12345"_s;
+    QTest::newRow("c:123456") << c << 123456 << u"123456"_s;
+    QTest::newRow("c:1234567") << c << 1234567 << u"1234567"_s;
+    QTest::newRow("c:12345678") << c << 12345678 << u"12345678"_s;
+    QTest::newRow("c:123456789") << c << 123456789 << u"123456789"_s;
+    QTest::newRow("c:1234567890") << c << 1234567890 << u"1234567890"_s;
+
+    QLocale en(QLocale::English); // English-style, with separators:
+    en.setNumberOptions(en.numberOptions() & ~QLocale::OmitGroupSeparator);
+    QTest::newRow("en:1") << en << 1 << u"1"_s;
+    QTest::newRow("en:12") << en << 12 << u"12"_s;
+    QTest::newRow("en:123") << en << 123 << u"123"_s;
+    QTest::newRow("en:1,234") << en << 1234 << u"1,234"_s;
+    QTest::newRow("en:12,345") << en << 12345 << u"12,345"_s;
+    QTest::newRow("en:123,456") << en << 123456 << u"123,456"_s;
+    QTest::newRow("en:1,234,567") << en << 1234567 << u"1,234,567"_s;
+    QTest::newRow("en:12,345,678") << en << 12345678 << u"12,345,678"_s;
+    QTest::newRow("en:123,456,789") << en << 123456789 << u"123,456,789"_s;
+    QTest::newRow("en:1,234,567,890") << en << 1234567890 << u"1,234,567,890"_s;
+
+    QLocale es(QLocale::Spanish); // Spanish-style, with separators
+    es.setNumberOptions(es.numberOptions() & ~QLocale::OmitGroupSeparator);
+    QTest::newRow("es:1") << es << 1 << u"1"_s;
+    QTest::newRow("es:12") << es << 12 << u"12"_s;
+    QTest::newRow("es:123") << es << 123 << u"123"_s;
+    // First split doesn't happen unless first group has at least two digits:
+    QTest::newRow("es:1234") << es << 1234 << u"1234"_s;
+    QTest::newRow("es:12.345") << es << 12345 << u"12.345"_s;
+    QTest::newRow("es:123.456") << es << 123456 << u"123.456"_s;
+    // Later splits aren't limited to two digits (QTBUG-115740):
+    QTest::newRow("es:1.234.567") << es << 1234567 << u"1.234.567"_s;
+    QTest::newRow("es:12.345.678") << es << 12345678 << u"12.345.678"_s;
+    QTest::newRow("es:123.456.789") << es << 123456789 << u"123.456.789"_s;
+    QTest::newRow("es:1.234.567.890") << es << 1234567890 << u"1.234.567.890"_s;
+
+    QLocale hi(QLocale::Hindi, QLocale::India);
+    hi.setNumberOptions(hi.numberOptions() & ~QLocale::OmitGroupSeparator);
+    QTest::newRow("hi:1") << hi << 1 << u"1"_s;
+    QTest::newRow("hi:12") << hi << 12 << u"12"_s;
+    QTest::newRow("hi:123") << hi << 123 << u"123"_s;
+    QTest::newRow("hi:1,234") << hi << 1234 << u"1,234"_s;
+    QTest::newRow("hi:12,345") << hi << 12345 << u"12,345"_s;
+    QTest::newRow("hi:1,23,456") << hi << 123456 << u"1,23,456"_s;
+    QTest::newRow("hi:12,34,567") << hi << 1234567 << u"12,34,567"_s;
+    QTest::newRow("hi:1,23,45,678") << hi << 12345678 << u"1,23,45,678"_s;
+    QTest::newRow("hi:12,34,56,789") << hi << 123456789 << u"12,34,56,789"_s;
+    QTest::newRow("hi:1,23,45,67,890") << hi << 1234567890 << u"1,23,45,67,890"_s;
+}
+
+void tst_QLocale::numberGrouping()
+{
+    QFETCH(const QLocale, locale);
+    QFETCH(const int, number);
+    QFETCH(const QString, string);
+
+    QCOMPARE(locale.toString(number), string);
+    QLocale sys = QLocale::system();
+    if (sys.language() == locale.language()
+            && sys.script() == locale.script()
+            && sys.territory() == locale.territory()) {
+        sys.setNumberOptions(locale.numberOptions());
+
+        QCOMPARE(sys.toString(number), string);
+        if (QLocale() == sys) { // This normally should be the case.
+            QCOMPARE(u"%L1"_s.arg(number), string);
+            QCOMPARE(u"%L1"_s.arg(double(number), 0, 'f', 0), string);
+        }
+    }
+}
+
 void tst_QLocale::numberGroupingIndia()
 {
     const QLocale indian(QLocale::Hindi, QLocale::India);
diff --git a/tests/auto/gui/util/qdoublevalidator/tst_qdoublevalidator.cpp b/tests/auto/gui/util/qdoublevalidator/tst_qdoublevalidator.cpp
index 859c198595..b4c3b45420 100644
--- a/tests/auto/gui/util/qdoublevalidator/tst_qdoublevalidator.cpp
+++ b/tests/auto/gui/util/qdoublevalidator/tst_qdoublevalidator.cpp
@@ -417,7 +417,7 @@ void tst_QDoubleValidator::fixup_data()
     QTest::newRow("C standard with invalid digit grouping")
             << "C" << QDoubleValidator::StandardNotation << -1 << "1,234,5.678"
             << "12345.678";
-    QTest::newRow("C standard with invalid number of decimals")
+    QTest::newRow("C standard with invalid group size")
             << "C" << QDoubleValidator::StandardNotation << 2 << "-12,34.678"
             << "-1234.68";
     QTest::newRow("C standard truncate decimals")
@@ -446,7 +446,7 @@ void tst_QDoubleValidator::fixup_data()
     QTest::newRow("C scientific with invalid digit grouping")
             << "C" << QDoubleValidator::ScientificNotation << -1 << "12,34.98765e2"
             << "1.23498765e+05";
-    QTest::newRow("C scientific with invalid number of decimals")
+    QTest::newRow("C scientific with invalid group size")
             << "C" << QDoubleValidator::ScientificNotation << 2 << "-12,34.98765e2"
             << "-1.23e+05";
     QTest::newRow("C scientific truncate decimals")
@@ -486,7 +486,7 @@ void tst_QDoubleValidator::fixup_data()
     QTest::newRow("en standard with invalid digit grouping")
             << "en" << QDoubleValidator::StandardNotation << -1 << "-1,234,5.678"
             << "-12,345.678";
-    QTest::newRow("en standard with invalid number of decimals")
+    QTest::newRow("en standard with invalid group size")
             << "en" << QDoubleValidator::StandardNotation << 2 << "12,34.678"
             << "1,234.68";
     QTest::newRow("en standard no fractional part")
@@ -502,7 +502,7 @@ void tst_QDoubleValidator::fixup_data()
     QTest::newRow("en scientific with invalid digit grouping")
             << "en" << QDoubleValidator::ScientificNotation << -1 << "-12,34.98765e2"
             << "-1.23498765E+05";
-    QTest::newRow("en scientific with invalid number of decimals")
+    QTest::newRow("en scientific with invalid group size")
             << "en" << QDoubleValidator::ScientificNotation << 2 << "12,34.98765e2"
             << "1.23E+05";
     QTest::newRow("en scientific no fractional part")
@@ -529,7 +529,7 @@ void tst_QDoubleValidator::fixup_data()
     QTest::newRow("de standard with invalid digit grouping")
             << "de" << QDoubleValidator::StandardNotation << -1 << "1.234.5,678"
             << "12.345,678";
-    QTest::newRow("de standard with invalid number of decimals")
+    QTest::newRow("de standard with invalid group size")
             << "de" << QDoubleValidator::StandardNotation << 2 << "-12.34,678"
             << "-1.234,68";
     QTest::newRow("de standard no fractional part")
@@ -544,7 +544,7 @@ void tst_QDoubleValidator::fixup_data()
     QTest::newRow("de scientific with invalid digit grouping")
             << "de" << QDoubleValidator::ScientificNotation << -1 << "12.34,98765e2"
             << "1,23498765E+05";
-    QTest::newRow("de scientific with invalid number of decimals")
+    QTest::newRow("de scientific with invalid group size")
             << "de" << QDoubleValidator::ScientificNotation << 2 << "-12.34,98765e2"
             << "-1,23E+05";
     QTest::newRow("de scientific no fractional part")
@@ -560,6 +560,22 @@ void tst_QDoubleValidator::fixup_data()
             << "de" << QDoubleValidator::ScientificNotation << -1 << "-12.34"
             << "-1,234E+03";
 
+    // es locale uses ',' as decimal point and '.' as grouping separator.
+    // It doesn't apply grouping unless the the next-to-least significant group
+    // has more than one digit in it.
+    QTest::newRow("es standard no digit grouping")
+            << "es" << QDoubleValidator::StandardNotation << -1 << "1234,567" << "1234,567";
+    QTest::newRow("es standard with digit grouping")
+            << "es" << QDoubleValidator::StandardNotation << -1 << "-12.345,678" << "-12.345,678";
+    QTest::newRow("es standard with invalid group size")
+            << "es" << QDoubleValidator::StandardNotation << -1 << "1.234.5,678" << "12.345,678";
+    QTest::newRow("es standard with invalid digit grouping")
+            << "es" << QDoubleValidator::StandardNotation << 2 << "-1.234,678" << "-1234,68";
+    QTest::newRow("es standard big with invalid digit grouping")
+            << "es" << QDoubleValidator::StandardNotation << 2 << "-1234.678,9" << "-1.234.678,9";
+    QTest::newRow("es standard no fractional part")
+            << "es" << QDoubleValidator::StandardNotation << -1 << "12.34" << "1234";
+
     // hi locale uses '.' as decimal point and ',' as grouping separator.
     // The rightmost group is of three digits, all the others contain two
     // digits.
diff --git a/tests/auto/gui/util/qintvalidator/tst_qintvalidator.cpp b/tests/auto/gui/util/qintvalidator/tst_qintvalidator.cpp
index 756b1a9c01..e940423165 100644
--- a/tests/auto/gui/util/qintvalidator/tst_qintvalidator.cpp
+++ b/tests/auto/gui/util/qintvalidator/tst_qintvalidator.cpp
@@ -308,9 +308,9 @@ void tst_QIntValidator::fixup_data()
     // Normally the groups contain three digits, but the leftmost group should
     // have at least two digits.
     QTest::newRow("es no digit grouping 1000") << "es" << "1000" << "1000";
-    QTest::newRow("es no digit grouping 10000") << "es" << "10000" << "10.000";
-    QTest::newRow("es with digit grouping") << "es" << "1000.000" << "1000.000";
-    QTest::newRow("es invalid digit grouping") << "es" << "1.000.000" << "1000.000";
+    QTest::newRow("es with digit grouping 10000") << "es" << "10000" << "10.000";
+    QTest::newRow("es with digit grouping million") << "es" << "1.000.000" << "1.000.000";
+    QTest::newRow("es invalid digit grouping") << "es" << "1000.000" << "1.000.000";
 }
 
 QTEST_APPLESS_MAIN(tst_QIntValidator)