comphelper: fix natural string compare function + reorganize tests

Natural string compare function doesn't take into account that
the string can start with a number and in this case it treats it
like a conventional string compare. This change takes this case
into account.

This change also refactores the tests for NaturalStringSorter
class. The previous tet used a mock XBreakIterator and XCollator
implementations to test the functionallity. This is not needed as
we can just use a real one instead, which makes the test more
real as it actually uses a real implementation instead of a mock
implementation, which could differ. This change removes the mock
XCollator and XBreakIterator implementations and moves the test
into a new file - NaturalStringSortTest.cxx

The test is also extended with the new use case where the string
starts with a number.

Change-Id: I32ea055f914c2947e4d979093b32f56170a61102
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/134540
Tested-by: Jenkins
Reviewed-by: Tomaž Vajngerl <quikee@gmail.com>

4 files changed, 129 insertions, 198 deletions

diff --git a/comphelper/CppunitTest_comphelper_test.mk b/comphelper/CppunitTest_comphelper_test.mk
index d530f4577806..17de701aca26 100644
--- a/comphelper/CppunitTest_comphelper_test.mk
+++ b/comphelper/CppunitTest_comphelper_test.mk
@@ -11,6 +11,7 @@ $(eval $(call gb_CppunitTest_CppunitTest,comphelper_test))
 
 $(eval $(call gb_CppunitTest_add_exception_objects,comphelper_test, \
     comphelper/qa/string/test_string \
+    comphelper/qa/string/NaturalStringSortTest \
     comphelper/qa/container/testifcontainer \
     comphelper/qa/container/testifcontainer3 \
     comphelper/qa/unit/test_hash \
@@ -21,6 +22,7 @@ $(eval $(call gb_CppunitTest_add_exception_objects,comphelper_test, \
     comphelper/qa/unit/test_traceevent \
 ))
 
+$(eval $(call gb_CppunitTest_use_ure,comphelper_test))
 $(eval $(call gb_CppunitTest_use_sdk_api,comphelper_test))
 
 $(eval $(call gb_CppunitTest_use_libraries,comphelper_test, \
@@ -28,6 +30,7 @@ $(eval $(call gb_CppunitTest_use_libraries,comphelper_test, \
     cppuhelper \
     cppu \
     sal \
+    unotest \
 ))
 
 ifeq ($(TLS),NSS)
@@ -37,4 +40,8 @@ $(eval $(call gb_CppunitTest_use_externals,comphelper_test,\
 ))
 endif
 
+$(eval $(call gb_CppunitTest_use_components,comphelper_test,\
+	i18npool/util/i18npool \
+))
+
 # vim: set noet sw=4 ts=4:
diff --git a/comphelper/qa/string/NaturalStringSortTest.cxx b/comphelper/qa/string/NaturalStringSortTest.cxx
new file mode 100644
index 000000000000..bfdcaff6e13c
--- /dev/null
+++ b/comphelper/qa/string/NaturalStringSortTest.cxx
@@ -0,0 +1,95 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ *
+ * This file incorporates work covered by the following license notice:
+ *
+ *   Licensed to the Apache Software Foundation (ASF) under one or more
+ *   contributor license agreements. See the NOTICE file distributed
+ *   with this work for additional information regarding copyright
+ *   ownership. The ASF licenses this file to you under the Apache
+ *   License, Version 2.0 (the "License"); you may not use this file
+ *   except in compliance with the License. You may obtain a copy of
+ *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
+ */
+
+#include <comphelper/string.hxx>
+#include <comphelper/processfactory.hxx>
+#include <cppuhelper/implbase.hxx>
+#include <com/sun/star/i18n/CharType.hpp>
+#include <com/sun/star/i18n/XBreakIterator.hpp>
+#include <com/sun/star/i18n/XCollator.hpp>
+
+#include <unotest/bootstrapfixturebase.hxx>
+#include <cppunit/TestAssert.h>
+#include <cppunit/TestFixture.h>
+#include <cppunit/extensions/HelperMacros.h>
+#include <cppunit/plugin/TestPlugIn.h>
+#include <rtl/ustring.hxx>
+
+using namespace css;
+
+namespace
+{
+class TestStringNaturalCompare : public test::BootstrapFixtureBase
+{
+public:
+    void testNatural()
+    {
+        lang::Locale aLocale;
+        aLocale.Language = "en";
+        aLocale.Country = "US";
+
+        comphelper::string::NaturalStringSorter aSorter(comphelper::getProcessComponentContext(),
+                                                        aLocale);
+
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+0), aSorter.compare("ABC", "ABC"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+1), aSorter.compare("ABC", "abc"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(-1), aSorter.compare("abc", "ABC"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+1), aSorter.compare("alongstring", "alongerstring"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(-1), aSorter.compare("alongerstring", "alongstring"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(-1), aSorter.compare("Heading 9", "Heading 10"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+1), aSorter.compare("Heading 10", "Heading 9"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(-1), aSorter.compare("July, the 4th", "July, the 10th"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+1), aSorter.compare("July, the 10th", "July, the 4th"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(-1), aSorter.compare("abc08", "abc010"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+1), aSorter.compare("abc010", "abc08"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+0), aSorter.compare("apple10apple", "apple10apple"));
+
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+1), aSorter.compare("KA1", "KA0"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+0), aSorter.compare("KA1", "KA1"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(-1), aSorter.compare("KA1", "KA2"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+1), aSorter.compare("KA50", "KA5"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(-1), aSorter.compare("KA50", "KA100"));
+
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+1), aSorter.compare("1", "0"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+0), aSorter.compare("1", "1"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(-1), aSorter.compare("1", "2"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+1), aSorter.compare("11", "1"));
+
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(-1), aSorter.compare("50", "100"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(-1), aSorter.compare("0", "100000"));
+
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(-1), aSorter.compare("0", "A"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+1), aSorter.compare("A", "0"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+1), aSorter.compare("A", "99"));
+
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+1), aSorter.compare("00ABC2", "00ABC1"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(-1), aSorter.compare("00ABC1", "00ABC2"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(+1), aSorter.compare("00ABC11", "00ABC2"));
+        CPPUNIT_ASSERT_EQUAL(sal_Int32(-1), aSorter.compare("00ABC2", "00ABC11"));
+    }
+
+    CPPUNIT_TEST_SUITE(TestStringNaturalCompare);
+    CPPUNIT_TEST(testNatural);
+    CPPUNIT_TEST_SUITE_END();
+};
+
+CPPUNIT_TEST_SUITE_REGISTRATION(TestStringNaturalCompare);
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/comphelper/qa/string/test_string.cxx b/comphelper/qa/string/test_string.cxx
index 5d3132756ad5..0a9850ed920f 100644
--- a/comphelper/qa/string/test_string.cxx
+++ b/comphelper/qa/string/test_string.cxx
@@ -35,7 +35,6 @@ namespace {
 class TestString: public CppUnit::TestFixture
 {
 public:
-    void testNatural();
     void testStripStart();
     void testStripEnd();
     void testStrip();
@@ -48,7 +47,6 @@ public:
     void testRemoveAny();
 
     CPPUNIT_TEST_SUITE(TestString);
-    CPPUNIT_TEST(testNatural);
     CPPUNIT_TEST(testStripStart);
     CPPUNIT_TEST(testStripEnd);
     CPPUNIT_TEST(testStrip);
@@ -82,192 +80,6 @@ void TestString::testIsdigitAsciiString()
     CPPUNIT_ASSERT_EQUAL(true, comphelper::string::isdigitAsciiString(""));
 }
 
-using namespace ::com::sun::star;
-
-class testCollator : public cppu::WeakImplHelper< i18n::XCollator >
-{
-public:
-    virtual sal_Int32 SAL_CALL compareSubstring(
-        const OUString& str1, sal_Int32 off1, sal_Int32 len1,
-        const OUString& str2, sal_Int32 off2, sal_Int32 len2) override
-    {
-        return str1.copy(off1, len1).compareTo(str2.subView(off2, len2));
-    }
-    virtual sal_Int32 SAL_CALL compareString(
-        const OUString& str1,
-        const OUString& str2) override
-    {
-        return str1.compareTo(str2);
-    }
-    virtual sal_Int32 SAL_CALL loadDefaultCollator(const lang::Locale&, sal_Int32) override {return 0;}
-    virtual sal_Int32 SAL_CALL loadCollatorAlgorithm(const OUString&,
-        const lang::Locale&, sal_Int32) override {return 0;}
-    virtual void SAL_CALL loadCollatorAlgorithmWithEndUserOption(const OUString&,
-        const lang::Locale&, const uno::Sequence< sal_Int32 >&) override {}
-    virtual uno::Sequence< OUString > SAL_CALL listCollatorAlgorithms(const lang::Locale&) override
-    {
-        return uno::Sequence< OUString >();
-    }
-    virtual uno::Sequence< sal_Int32 > SAL_CALL listCollatorOptions(const OUString&) override
-    {
-        return uno::Sequence< sal_Int32 >();
-    }
-};
-
-#define IS_DIGIT(CHAR) (((CHAR) >= 48) && ((CHAR <= 57)))
-
-class testBreakIterator : public cppu::WeakImplHelper< i18n::XBreakIterator >
-{
-public:
-    virtual sal_Int32 SAL_CALL nextCharacters( const OUString&, sal_Int32,
-        const lang::Locale&, sal_Int16, sal_Int32, sal_Int32& ) override {return -1;}
-    virtual sal_Int32 SAL_CALL previousCharacters( const OUString&, sal_Int32,
-        const lang::Locale&, sal_Int16, sal_Int32, sal_Int32& ) override {return -1;}
-
-    virtual i18n::Boundary SAL_CALL previousWord( const OUString&, sal_Int32,
-        const lang::Locale&, sal_Int16) override
-        { return i18n::Boundary(); }
-    virtual i18n::Boundary SAL_CALL nextWord( const OUString&, sal_Int32,
-        const lang::Locale&, sal_Int16) override
-        { return i18n::Boundary(); }
-    virtual i18n::Boundary SAL_CALL getWordBoundary( const OUString&, sal_Int32,
-        const lang::Locale&, sal_Int16, sal_Bool ) override
-        { return i18n::Boundary(); }
-
-    virtual sal_Bool SAL_CALL isBeginWord( const OUString&, sal_Int32,
-        const lang::Locale&, sal_Int16 ) override
-        { return false; }
-    virtual sal_Bool SAL_CALL isEndWord( const OUString&, sal_Int32,
-        const lang::Locale& , sal_Int16 ) override
-        { return false; }
-    virtual sal_Int16 SAL_CALL getWordType( const OUString&, sal_Int32,
-        const lang::Locale& ) override
-        { return 0; }
-
-    virtual sal_Int32 SAL_CALL beginOfSentence( const OUString&, sal_Int32,
-        const lang::Locale& ) override
-        { return 0; }
-    virtual sal_Int32 SAL_CALL endOfSentence( const OUString& rText, sal_Int32,
-        const lang::Locale& ) override
-        { return rText.getLength(); }
-
-    virtual i18n::LineBreakResults SAL_CALL getLineBreak( const OUString&, sal_Int32,
-        const lang::Locale&, sal_Int32,
-        const i18n::LineBreakHyphenationOptions&,
-        const i18n::LineBreakUserOptions&) override
-    {
-        return i18n::LineBreakResults();
-    }
-
-    virtual sal_Int16 SAL_CALL getScriptType( const OUString&, sal_Int32 ) override { return -1; }
-    virtual sal_Int32 SAL_CALL beginOfScript( const OUString&, sal_Int32,
-        sal_Int16 ) override { return -1; }
-    virtual sal_Int32 SAL_CALL endOfScript( const OUString&, sal_Int32,
-        sal_Int16 ) override { return -1; }
-    virtual sal_Int32 SAL_CALL previousScript( const OUString&, sal_Int32,
-        sal_Int16 ) override { return -1; }
-    virtual sal_Int32 SAL_CALL nextScript( const OUString&, sal_Int32,
-        sal_Int16 ) override { return -1; }
-
-    virtual sal_Int32 SAL_CALL beginOfCharBlock( const OUString&, sal_Int32,
-        const lang::Locale&, sal_Int16 ) override { return -1; }
-    virtual sal_Int32 SAL_CALL endOfCharBlock( const OUString& rText, sal_Int32 nStartPos,
-        const lang::Locale&, sal_Int16 CharType ) override
-    {
-        const sal_Unicode *pStr = rText.getStr()+nStartPos;
-        for (sal_Int32 nI = nStartPos; nI < rText.getLength(); ++nI)
-        {
-            if (CharType == i18n::CharType::DECIMAL_DIGIT_NUMBER && !IS_DIGIT(*pStr))
-                return nI;
-            else if (CharType != i18n::CharType::DECIMAL_DIGIT_NUMBER && IS_DIGIT(*pStr))
-                return nI;
-            ++pStr;
-        }
-        return -1;
-    }
-    virtual sal_Int32 SAL_CALL previousCharBlock( const OUString&, sal_Int32,
-        const lang::Locale&, sal_Int16 ) override { return -1; }
-    virtual sal_Int32 SAL_CALL nextCharBlock( const OUString& rText, sal_Int32 nStartPos,
-        const lang::Locale&, sal_Int16 CharType ) override
-    {
-        const sal_Unicode *pStr = rText.getStr()+nStartPos;
-        for (sal_Int32 nI = nStartPos; nI < rText.getLength(); ++nI)
-        {
-            if (CharType == i18n::CharType::DECIMAL_DIGIT_NUMBER && IS_DIGIT(*pStr))
-                return nI;
-            else if (CharType != i18n::CharType::DECIMAL_DIGIT_NUMBER && !IS_DIGIT(*pStr))
-                return nI;
-            ++pStr;
-        }
-        return -1;
-    }
-};
-
-void TestString::testNatural()
-{
-    using namespace comphelper::string;
-
-    uno::Reference< i18n::XCollator > xCollator(new testCollator);
-    uno::Reference< i18n::XBreakIterator > xBI(new testBreakIterator);
-
-// --- Some generic tests to ensure we do not alter original behavior
-// outside what we want
-    CPPUNIT_ASSERT_EQUAL(
-        static_cast<sal_Int32>(0), compareNatural("ABC", "ABC", xCollator, xBI, lang::Locale())
-    );
-    // Case sensitivity
-    CPPUNIT_ASSERT(
-        compareNatural("ABC", "abc", xCollator, xBI, lang::Locale()) < 0
-    );
-    // Reverse
-    CPPUNIT_ASSERT(
-        compareNatural("abc", "ABC", xCollator, xBI, lang::Locale()) > 0
-    );
-    // First shorter
-    CPPUNIT_ASSERT(
-        compareNatural("alongstring", "alongerstring", xCollator, xBI, lang::Locale()) > 0
-    );
-    // Second shorter
-    CPPUNIT_ASSERT(
-        compareNatural("alongerstring", "alongstring", xCollator, xBI, lang::Locale()) < 0
-    );
-// -- Here we go on natural order, each one is followed by classic compare and the reverse comparison
-    // That's why we originally made the patch
-    CPPUNIT_ASSERT(
-        compareNatural("Heading 9", "Heading 10", xCollator, xBI, lang::Locale()) < 0
-    );
-    // Original behavior
-    CPPUNIT_ASSERT(
-        OUString("Heading 9").compareTo(u"Heading 10") > 0
-    );
-    CPPUNIT_ASSERT(
-        compareNatural("Heading 10", "Heading 9", xCollator, xBI, lang::Locale()) > 0
-    );
-    // Harder
-    CPPUNIT_ASSERT(
-        compareNatural("July, the 4th", "July, the 10th", xCollator, xBI, lang::Locale()) < 0
-    );
-    CPPUNIT_ASSERT(
-        OUString("July, the 4th").compareTo(u"July, the 10th") > 0
-    );
-    CPPUNIT_ASSERT(
-        compareNatural("July, the 10th", "July, the 4th", xCollator, xBI, lang::Locale()) > 0
-    );
-    // Hardest
-    CPPUNIT_ASSERT(
-        compareNatural("abc08", "abc010", xCollator, xBI, lang::Locale()) < 0
-    );
-    CPPUNIT_ASSERT(
-        OUString("abc08").compareTo(u"abc010") > 0
-    );
-    CPPUNIT_ASSERT(
-        compareNatural("abc010", "abc08", xCollator, xBI, lang::Locale()) > 0
-    );
-    CPPUNIT_ASSERT_EQUAL(
-        static_cast<sal_Int32>(0), compareNatural("apple10apple", "apple10apple", xCollator, xBI, lang::Locale())
-    );
-}
-
 void TestString::testStripStart()
 {
     OString aIn("abc");
diff --git a/comphelper/source/misc/string.cxx b/comphelper/source/misc/string.cxx
index a11a305c5daf..8daeb377482a 100644
--- a/comphelper/source/misc/string.cxx
+++ b/comphelper/source/misc/string.cxx
@@ -425,33 +425,50 @@ sal_Int32 compareNatural( const OUString & rLHS, const OUString & rRHS,
     sal_Int32 nLHSFirstDigitPos = 0;
     sal_Int32 nRHSFirstDigitPos = 0;
 
+    // Check if the string starts with a digit
+    sal_Int32 nStartsDigitLHS = rBI->endOfCharBlock(rLHS, nLHSFirstDigitPos, rLocale, i18n::CharType::DECIMAL_DIGIT_NUMBER);
+    sal_Int32 nStartsDigitRHS = rBI->endOfCharBlock(rRHS, nRHSFirstDigitPos, rLocale, i18n::CharType::DECIMAL_DIGIT_NUMBER);
+
+    if (nStartsDigitLHS > 0 && nStartsDigitRHS > 0)
+    {
+        sal_uInt32 nLHS = comphelper::string::decimalStringToNumber(rLHS, 0, nStartsDigitLHS);
+        sal_uInt32 nRHS = comphelper::string::decimalStringToNumber(rRHS, 0, nStartsDigitRHS);
+
+        if (nLHS != nRHS)
+            return nLHS < nRHS ? -1 : 1;
+        nLHSLastNonDigitPos = nStartsDigitLHS;
+        nRHSLastNonDigitPos = nStartsDigitRHS;
+    }
+    else if (nStartsDigitLHS > 0)
+        return -1;
+    else if (nStartsDigitRHS > 0)
+        return 1;
+
     while (nLHSFirstDigitPos < rLHS.getLength() || nRHSFirstDigitPos < rRHS.getLength())
     {
         sal_Int32 nLHSChunkLen;
         sal_Int32 nRHSChunkLen;
 
         //Compare non digit block as normal strings
-        nLHSFirstDigitPos = rBI->nextCharBlock(rLHS, nLHSLastNonDigitPos,
-            rLocale, i18n::CharType::DECIMAL_DIGIT_NUMBER);
-        nRHSFirstDigitPos = rBI->nextCharBlock(rRHS, nRHSLastNonDigitPos,
-            rLocale, i18n::CharType::DECIMAL_DIGIT_NUMBER);
+        nLHSFirstDigitPos = rBI->nextCharBlock(rLHS, nLHSLastNonDigitPos, rLocale, i18n::CharType::DECIMAL_DIGIT_NUMBER);
+        nRHSFirstDigitPos = rBI->nextCharBlock(rRHS, nRHSLastNonDigitPos, rLocale, i18n::CharType::DECIMAL_DIGIT_NUMBER);
+
         if (nLHSFirstDigitPos == -1)
             nLHSFirstDigitPos = rLHS.getLength();
+
         if (nRHSFirstDigitPos == -1)
             nRHSFirstDigitPos = rRHS.getLength();
+
         nLHSChunkLen = nLHSFirstDigitPos - nLHSLastNonDigitPos;
         nRHSChunkLen = nRHSFirstDigitPos - nRHSLastNonDigitPos;
 
-        nRet = rCollator->compareSubstring(rLHS, nLHSLastNonDigitPos,
-            nLHSChunkLen, rRHS, nRHSLastNonDigitPos, nRHSChunkLen);
+        nRet = rCollator->compareSubstring(rLHS, nLHSLastNonDigitPos, nLHSChunkLen, rRHS, nRHSLastNonDigitPos, nRHSChunkLen);
         if (nRet != 0)
             break;
 
         //Compare digit block as one number vs another
-        nLHSLastNonDigitPos = rBI->endOfCharBlock(rLHS, nLHSFirstDigitPos,
-            rLocale, i18n::CharType::DECIMAL_DIGIT_NUMBER);
-        nRHSLastNonDigitPos = rBI->endOfCharBlock(rRHS, nRHSFirstDigitPos,
-            rLocale, i18n::CharType::DECIMAL_DIGIT_NUMBER);
+        nLHSLastNonDigitPos = rBI->endOfCharBlock(rLHS, nLHSFirstDigitPos, rLocale, i18n::CharType::DECIMAL_DIGIT_NUMBER);
+        nRHSLastNonDigitPos = rBI->endOfCharBlock(rRHS, nRHSFirstDigitPos, rLocale, i18n::CharType::DECIMAL_DIGIT_NUMBER);
         if (nLHSLastNonDigitPos == -1)
             nLHSLastNonDigitPos = rLHS.getLength();
         if (nRHSLastNonDigitPos == -1)