tdf#155823: Improve the check if the list id is not required

The implementation introduced in commit 8f48f91009caa86d896f247059874242ed18bf39
(SwNumRule::HasContinueList) was a bit naive: it assumed that maTextNodeList is
sorted (it is not, and so, valid cases to avoid the id were missed); it assumed
that a given list can only consist of items of a single numbering style, and so
only tested the list of nodes referenced from maTextNodeList of given SwNumRule.
I.e., this implementation targeted a special case of a list style fully covering
a single continuous list.

This skipped ids for list items with list styles, in which maTextNodeList passed
the check in HasContinueList, but which were followed by items with a different
list style, continuing the same list. This constellation outputs continue-list
attribute in the following items (see XMLTextParagraphExport::exportListChange),
which references the skipped id. The resulting ODF is an invalid XML (an xml:id
is missing that is referenced), and also does not allow to continue such a list.

The change tries to fix this, using a list of nodes in XMLTextParagraphExport,
and analyzing if the list of the current paragraph has a continuation that needs
to reference this list id. Two new hidden properties introduced in SwXParagraph
and SwXTextDocument: "ODFExport_NodeIndex" and "ODFExport_ListNodes", resp. They
allow to pipe the data to the export. The previous special casing of property
state for "ListId", used in SwNumRule::HasContinueList, is removed together with
the mentioned function.

The intention is to have a logic allowing to detect 100% cases where the list id
is required, and where it's not required.

A related unit test for tdf#149668 was fixed to not rely on the mentioned ListId
property state workaround, and moved from sw/qa/core/unocore to xmloff/qa/unit.

Change-Id: If6a6ac7a3dfe0b2ea143229678a603875153eedb
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/153044
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/153065
Reviewed-by: Miklos Vajna <vmiklos@collabora.com>

5 files changed, 287 insertions, 28 deletions

diff --git a/xmloff/qa/unit/data/differentListStylesInOneList.fodt b/xmloff/qa/unit/data/differentListStylesInOneList.fodt
new file mode 100644
index 000000000000..5f90135fbb23
--- /dev/null
+++ b/xmloff/qa/unit/data/differentListStylesInOneList.fodt
@@ -0,0 +1,47 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<office:document xmlns:office="urn:oasis:names:tc:opendocument:xmlns:office:1.0" xmlns:fo="urn:oasis:names:tc:opendocument:xmlns:xsl-fo-compatible:1.0" xmlns:style="urn:oasis:names:tc:opendocument:xmlns:style:1.0" xmlns:text="urn:oasis:names:tc:opendocument:xmlns:text:1.0" office:version="1.3" office:mimetype="application/vnd.oasis.opendocument.text">
+ <office:styles>
+  <text:list-style style:name="ListStyleOne">
+   <text:list-level-style-number text:level="1" style:num-suffix="." style:num-format="1">
+    <style:list-level-properties text:list-level-position-and-space-mode="label-alignment">
+     <style:list-level-label-alignment text:label-followed-by="listtab" fo:text-indent="-1cm" fo:margin-left="1cm"/>
+    </style:list-level-properties>
+   </text:list-level-style-number>
+  </text:list-style>
+  <text:list-style style:name="ListStyleAnother">
+   <text:list-level-style-number text:level="1" style:num-suffix="." style:num-format="1">
+    <style:list-level-properties text:list-level-position-and-space-mode="label-alignment">
+     <style:list-level-label-alignment text:label-followed-by="listtab" fo:text-indent="-1cm" fo:margin-left="1cm"/>
+    </style:list-level-properties>
+   </text:list-level-style-number>
+  </text:list-style>
+ </office:styles>
+ <office:body>
+  <office:text>
+   <text:list xml:id="list1" text:style-name="ListStyleOne">
+    <text:list-item>
+     <text:p>Item1 (ListStyleOne)</text:p>
+    </text:list-item>
+   </text:list>
+   <text:p/>
+   <text:list xml:id="list2" text:continue-numbering="true" text:style-name="ListStyleOne">
+    <text:list-item>
+     <text:p>Item2 (ListStyleOne)</text:p>
+    </text:list-item>
+   </text:list>
+   <text:p/>
+   <text:list xml:id="list3" text:continue-list="list2" text:style-name="ListStyleAnother">
+    <text:list-item>
+     <text:p>Item3 (ListStyleAnother)</text:p>
+    </text:list-item>
+   </text:list>
+   <text:p/>
+   <text:list xml:id="list4" text:continue-list="list3" text:style-name="ListStyleOne">
+    <text:list-item>
+     <text:p>Item4 (ListStyleOne)</text:p>
+    </text:list-item>
+   </text:list>
+  </office:text>
+ </office:body>
+</office:document>
\ No newline at end of file
diff --git a/xmloff/qa/unit/text.cxx b/xmloff/qa/unit/text.cxx
index 58a34996b6b2..ab8a28448701 100644
--- a/xmloff/qa/unit/text.cxx
+++ b/xmloff/qa/unit/text.cxx
@@ -13,6 +13,7 @@
 #include <com/sun/star/beans/PropertyValues.hpp>
 #include <com/sun/star/frame/XStorable.hpp>
 #include <com/sun/star/text/XTextDocument.hpp>
+#include <com/sun/star/text/ControlCharacter.hpp>
 #include <com/sun/star/text/BibliographyDataType.hpp>
 #include <com/sun/star/text/TextContentAnchorType.hpp>
 #include <com/sun/star/style/XStyleFamiliesSupplier.hpp>
@@ -203,6 +204,110 @@ CPPUNIT_TEST_FIXTURE(XmloffStyleTest, testListId)
     assertXPathNoAttribute(pXmlDoc, "//text:list", "id");
 }
 
+CPPUNIT_TEST_FIXTURE(XmloffStyleTest, testListId2)
+{
+    // tdf#155823 Given a document with a list consisting of items having different list styles:
+    loadFromURL(u"differentListStylesInOneList.fodt");
+
+    uno::Reference<text::XTextDocument> xTextDocument(mxComponent, uno::UNO_QUERY_THROW);
+    uno::Reference<container::XEnumerationAccess> xParaEnumAccess(xTextDocument->getText(),
+                                                                  uno::UNO_QUERY_THROW);
+    auto xParaEnum(xParaEnumAccess->createEnumeration());
+
+    uno::Reference<beans::XPropertySet> xPara(xParaEnum->nextElement(), uno::UNO_QUERY_THROW);
+    auto aActual(xPara->getPropertyValue("ListLabelString").get<OUString>());
+    CPPUNIT_ASSERT_EQUAL(OUString("1."), aActual);
+    xParaEnum->nextElement(); // Skip empty intermediate paragraph
+    xPara.set(xParaEnum->nextElement(), uno::UNO_QUERY);
+    aActual = xPara->getPropertyValue("ListLabelString").get<OUString>();
+    CPPUNIT_ASSERT_EQUAL(OUString("2."), aActual);
+    xParaEnum->nextElement(); // Skip empty intermediate paragraph
+    xPara.set(xParaEnum->nextElement(), uno::UNO_QUERY);
+    aActual = xPara->getPropertyValue("ListLabelString").get<OUString>();
+    CPPUNIT_ASSERT_EQUAL(OUString("3."), aActual);
+    xParaEnum->nextElement(); // Skip empty intermediate paragraph
+    xPara.set(xParaEnum->nextElement(), uno::UNO_QUERY);
+    aActual = xPara->getPropertyValue("ListLabelString").get<OUString>();
+    CPPUNIT_ASSERT_EQUAL(OUString("4."), aActual);
+
+    // When storing that document as ODF:
+    // Without the fix in place, automatic validation would fail with:
+    // Error: "list123456789012345" is referenced by an IDREF, but not defined.
+    saveAndReload("writer8");
+
+    xTextDocument.set(mxComponent, uno::UNO_QUERY_THROW);
+    xParaEnumAccess.set(xTextDocument->getText(), uno::UNO_QUERY_THROW);
+    xParaEnum.set(xParaEnumAccess->createEnumeration());
+
+    xPara.set(xParaEnum->nextElement(), uno::UNO_QUERY);
+    aActual = xPara->getPropertyValue("ListLabelString").get<OUString>();
+    CPPUNIT_ASSERT_EQUAL(OUString("1."), aActual);
+    xParaEnum->nextElement(); // Skip empty intermediate paragraph
+    xPara.set(xParaEnum->nextElement(), uno::UNO_QUERY);
+    aActual = xPara->getPropertyValue("ListLabelString").get<OUString>();
+    CPPUNIT_ASSERT_EQUAL(OUString("2."), aActual);
+    xParaEnum->nextElement(); // Skip empty intermediate paragraph
+    xPara.set(xParaEnum->nextElement(), uno::UNO_QUERY);
+    aActual = xPara->getPropertyValue("ListLabelString").get<OUString>();
+    CPPUNIT_ASSERT_EQUAL(OUString("3."), aActual);
+    xParaEnum->nextElement(); // Skip empty intermediate paragraph
+
+    // Check that the last item number is correct
+
+    xPara.set(xParaEnum->nextElement(), uno::UNO_QUERY);
+    aActual = xPara->getPropertyValue("ListLabelString").get<OUString>();
+    // Without the fix in place, this would fail with:
+    // - Expected: 4.
+    // - Actual  : 1.
+    // i.e. the numbering was not continued.
+    CPPUNIT_ASSERT_EQUAL(OUString("4."), aActual);
+
+    // Then make sure that required xml:id="..." attributes is written when the style changes:
+    xmlDocUniquePtr pXmlDoc = parseExport("content.xml");
+    CPPUNIT_ASSERT(pXmlDoc);
+    // Without the fix in place, this would fail,
+    // i.e. xml:id="..." was omitted, even though it was needed for the next item.
+    OUString id
+        = getXPath(pXmlDoc, "/office:document-content/office:body/office:text/text:list[3]", "id");
+    CPPUNIT_ASSERT(!id.isEmpty());
+    assertXPath(pXmlDoc, "/office:document-content/office:body/office:text/text:list[4]",
+                "continue-list", id);
+}
+
+CPPUNIT_TEST_FIXTURE(XmloffStyleTest, testListIdState)
+{
+    // tdf#149668: given a document with 3 paragraphs: an outer numbering on para 1 & 3, an inner
+    // numbering on para 2:
+    mxComponent = loadFromDesktop("private:factory/swriter");
+    uno::Reference<text::XTextDocument> xTextDocument(mxComponent, uno::UNO_QUERY_THROW);
+    auto xText(xTextDocument->getText());
+    xText->insertControlCharacter(xText->getEnd(), css::text::ControlCharacter::PARAGRAPH_BREAK,
+                                  false);
+    xText->insertControlCharacter(xText->getEnd(), css::text::ControlCharacter::PARAGRAPH_BREAK,
+                                  false);
+
+    uno::Reference<container::XEnumerationAccess> paraEnumAccess(xText, uno::UNO_QUERY_THROW);
+    auto paraEnum(paraEnumAccess->createEnumeration());
+    uno::Reference<beans::XPropertySet> xParaProps(paraEnum->nextElement(), uno::UNO_QUERY_THROW);
+    xParaProps->setPropertyValue("NumberingStyleName", css::uno::Any(OUString("Numbering ABC")));
+    xParaProps.set(paraEnum->nextElement(), uno::UNO_QUERY_THROW);
+    xParaProps->setPropertyValue("NumberingStyleName", css::uno::Any(OUString("Numbering 123")));
+    xParaProps.set(paraEnum->nextElement(), uno::UNO_QUERY_THROW);
+    xParaProps->setPropertyValue("NumberingStyleName", css::uno::Any(OUString("Numbering ABC")));
+
+    // When storing that document as ODF:
+    save("writer8");
+    xmlDocUniquePtr pXmlDoc = parseExport("content.xml");
+
+    // Make sure that xml:id="..." gets written for para 1, as it'll be continued in para 3.
+    // Without the accompanying fix in place, this test would have failed,
+    // i.e. para 1 didn't write an xml:id="..." but para 3 referred to it using continue-list="...",
+    // which is inconsistent.
+    OUString id
+        = getXPath(pXmlDoc, "/office:document-content/office:body/office:text/text:list[1]", "id");
+    CPPUNIT_ASSERT(!id.isEmpty());
+}
+
 CPPUNIT_TEST_FIXTURE(XmloffStyleTest, testClearingBreakExport)
 {
     // Given a document with a clearing break:
diff --git a/xmloff/source/text/XMLTextNumRuleInfo.cxx b/xmloff/source/text/XMLTextNumRuleInfo.cxx
index 062b92879ee2..5f9a5e2b7a54 100644
--- a/xmloff/source/text/XMLTextNumRuleInfo.cxx
+++ b/xmloff/source/text/XMLTextNumRuleInfo.cxx
@@ -54,14 +54,14 @@ void XMLTextNumRuleInfo::Set(
         const css::uno::Reference < css::text::XTextContent > & xTextContent,
         const bool bOutlineStyleAsNormalListStyle,
         const XMLTextListAutoStylePool& rListAutoPool,
-        const bool bExportTextNumberElement )
+        const bool bExportTextNumberElement,
+        const bool bListIdIsDefault )
 {
     Reset();
     // Written OpenDocument file format doesn't fit to the created text document (#i69627#)
     mbOutlineStyleAsNormalListStyle = bOutlineStyleAsNormalListStyle;
 
     Reference< XPropertySet > xPropSet( xTextContent, UNO_QUERY );
-    Reference<XPropertyState> xPropState(xTextContent, UNO_QUERY);
     Reference< XPropertySetInfo > xPropSetInfo = xPropSet->getPropertySetInfo();
 
     // check if this paragraph supports a numbering
@@ -138,14 +138,10 @@ void XMLTextNumRuleInfo::Set(
         if( xPropSetInfo->hasPropertyByName( "ListId" ) )
         {
             xPropSet->getPropertyValue( "ListId" ) >>= msListId;
-
-            if (xPropState.is())
-            {
-                mbListIdIsDefault
-                    = xPropState->getPropertyState("ListId") == PropertyState_DEFAULT_VALUE;
-            }
         }
 
+        mbListIdIsDefault = bListIdIsDefault;
+
         mbContinueingPreviousSubTree = false;
         if( xPropSetInfo->hasPropertyByName( "ContinueingPreviousSubTree" ) )
         {
diff --git a/xmloff/source/text/XMLTextNumRuleInfo.hxx b/xmloff/source/text/XMLTextNumRuleInfo.hxx
index adb405411164..7cbc3cf8d4fb 100644
--- a/xmloff/source/text/XMLTextNumRuleInfo.hxx
+++ b/xmloff/source/text/XMLTextNumRuleInfo.hxx
@@ -66,9 +66,10 @@ public:
     inline XMLTextNumRuleInfo& operator=( const XMLTextNumRuleInfo& rInfo );
 
     void Set( const css::uno::Reference < css::text::XTextContent > & rTextContent,
-              const bool bOutlineStyleAsNormalListStyle,
+              bool bOutlineStyleAsNormalListStyle,
               const XMLTextListAutoStylePool& rListAutoPool,
-              const bool bExportTextNumberElement );
+              bool bExportTextNumberElement,
+              bool bListIdIsDefault );
     inline void Reset();
 
     const OUString& GetNumRulesName() const
diff --git a/xmloff/source/text/txtparae.cxx b/xmloff/source/text/txtparae.cxx
index d8a622bdd888..b2f12b0635e9 100644
--- a/xmloff/source/text/txtparae.cxx
+++ b/xmloff/source/text/txtparae.cxx
@@ -997,7 +997,7 @@ void XMLTextParagraphExport::exportListChange(
     // end a list
     if ( rPrevInfo.GetLevel() > 0 )
     {
-        sal_Int16 nListLevelsToBeClosed = 0;
+        sal_uInt32 nListLevelsToBeClosed = 0; // unsigned larger type to safely multiply and compare
         if ( !rNextInfo.BelongsToSameList( rPrevInfo ) ||
              rNextInfo.GetLevel() <= 0 )
         {
@@ -1007,13 +1007,11 @@ void XMLTextParagraphExport::exportListChange(
         else if ( rPrevInfo.GetLevel() > rNextInfo.GetLevel() )
         {
             // close corresponding sub lists
-            SAL_WARN_IF( rNextInfo.GetLevel() <= 0, "xmloff",
-                        "<rPrevInfo.GetLevel() > 0> not hold. Serious defect." );
             nListLevelsToBeClosed = rPrevInfo.GetLevel() - rNextInfo.GetLevel();
         }
 
         if ( nListLevelsToBeClosed > 0 &&
-             maListElements.size() >= sal::static_int_cast< sal_uInt32 >( 2 * nListLevelsToBeClosed ) )
+             maListElements.size() >= 2 * nListLevelsToBeClosed )
         {
             do {
                 for(size_t j = 0; j < 2; ++j)
@@ -1031,11 +1029,6 @@ void XMLTextParagraphExport::exportListChange(
         }
     }
 
-    const bool bExportODF =
-                bool( GetExport().getExportFlags() & SvXMLExportFlags::OASIS );
-    const SvtSaveOptions::ODFSaneDefaultVersion eODFDefaultVersion =
-                                    GetExport().getSaneDefaultVersion();
-
     // start a new list
     if ( rNextInfo.GetLevel() > 0 )
     {
@@ -1051,8 +1044,6 @@ void XMLTextParagraphExport::exportListChange(
         else if ( rNextInfo.GetLevel() > rPrevInfo.GetLevel() )
         {
             // open corresponding sub lists
-            SAL_WARN_IF( rPrevInfo.GetLevel() <= 0, "xmloff",
-                        "<rPrevInfo.GetLevel() > 0> not hold. Serious defect." );
             nListLevelsToBeOpened = rNextInfo.GetLevel() - rPrevInfo.GetLevel();
         }
 
@@ -1074,8 +1065,7 @@ void XMLTextParagraphExport::exportListChange(
                 {
                     if ( !mpTextListsHelper->IsListProcessed( sListId ) )
                     {
-                        if ( bExportODF &&
-                            eODFDefaultVersion >= SvtSaveOptions::ODFSVER_012 &&
+                        if ( ExportListId() &&
                              !sListId.isEmpty() && !rNextInfo.IsListIdDefault() )
                         {
                             /* Property text:id at element <text:list> has to be
@@ -1093,8 +1083,7 @@ void XMLTextParagraphExport::exportListChange(
                     {
                         const OUString sNewListId(
                                         mpTextListsHelper->GenerateNewListId() );
-                        if ( bExportODF &&
-                            eODFDefaultVersion >= SvtSaveOptions::ODFSVER_012 &&
+                        if ( ExportListId() &&
                              !sListId.isEmpty() && !rNextInfo.IsListIdDefault() )
                         {
                             /* Property text:id at element <text:list> has to be
@@ -1124,8 +1113,7 @@ void XMLTextParagraphExport::exportListChange(
                         }
                         else
                         {
-                            if ( bExportODF &&
-                                eODFDefaultVersion >= SvtSaveOptions::ODFSVER_012 &&
+                            if ( ExportListId() &&
                                  !sListId.isEmpty() )
                             {
                                 GetExport().AddAttribute( XML_NAMESPACE_TEXT,
@@ -1808,6 +1796,127 @@ void XMLTextParagraphExport::exportText(
         pRedlineExport->ExportStartOrEndRedline( xPropertySet, false );
 }
 
+bool XMLTextParagraphExport::ExportListId() const
+{
+    return (GetExport().getExportFlags() & SvXMLExportFlags::OASIS)
+           && GetExport().getSaneDefaultVersion() >= SvtSaveOptions::ODFSVER_012;
+}
+
+struct XMLTextParagraphExport::DocumentListNodes
+{
+    struct NodeData
+    {
+        sal_Int32 index; // see SwNode::GetIndex and SwNodeOffset
+        sal_uInt64 style_id; // actually a pointer to NumRule
+        OUString list_id;
+        bool isRestart;
+    };
+    std::vector<NodeData> docListNodes;
+    DocumentListNodes(const css::uno::Reference<css::frame::XModel>& xModel)
+    {
+        // Sequence of nodes, each of them represented by four-element sequence,
+        // corresponding to NodeData members
+        css::uno::Sequence<css::uno::Sequence<css::uno::Any>> nodes;
+        if (uno::Reference<beans::XPropertySet> xPropSet{ xModel, uno::UNO_QUERY })
+        {
+            try
+            {
+                // See SwXTextDocument::getPropertyValue
+                xPropSet->getPropertyValue("ODFExport_ListNodes") >>= nodes;
+            }
+            catch (css::beans::UnknownPropertyException&)
+            {
+                // That's absolutely fine!
+            }
+        }
+
+        docListNodes.reserve(nodes.getLength());
+        for (const auto& node : nodes)
+        {
+            assert(node.getLength() == 4);
+            docListNodes.push_back({ node[0].get<sal_Int32>(), node[1].get<sal_uInt64>(),
+                                     node[2].get<OUString>(), node[3].get<bool>() });
+        }
+
+        std::sort(docListNodes.begin(), docListNodes.end(),
+                  [](const NodeData& lhs, const NodeData& rhs) { return lhs.index < rhs.index; });
+    }
+    bool ShouldSkipListId(const Reference<XTextContent>& xTextContent) const
+    {
+        if (docListNodes.empty())
+            return false;
+
+        if (uno::Reference<beans::XPropertySet> xPropSet{ xTextContent, uno::UNO_QUERY })
+        {
+            sal_Int32 index = 0;
+            try
+            {
+                // See SwXParagraph::Impl::GetPropertyValues_Impl
+                xPropSet->getPropertyValue("ODFExport_NodeIndex") >>= index;
+            }
+            catch (css::beans::UnknownPropertyException&)
+            {
+                // That's absolutely fine!
+                return false;
+            }
+
+            auto it = std::lower_bound(docListNodes.begin(), docListNodes.end(), index,
+                                       [](const NodeData& lhs, sal_Int32 rhs)
+                                       { return lhs.index < rhs; });
+            if (it == docListNodes.end() || it->index != index)
+                return false;
+
+            // We need to write the id, when there will be continuation of the list either with
+            // a different list style, or after another list.
+
+            for (auto next = it + 1; next != docListNodes.end(); ++next)
+            {
+                if (it->list_id != next->list_id)
+                {
+                    // List changed. We will have to refer to this id, only if there will
+                    // appear a continuation of this list
+                    return std::find_if(next + 1, docListNodes.end(),
+                                        [list_id = it->list_id](const NodeData& data)
+                                        { return data.list_id == list_id; })
+                           == docListNodes.end();
+                }
+
+                if (it->style_id != next->style_id)
+                {
+                    // Same list, new style -> this "next" will refer to the id, no skipping
+                    return false;
+                }
+                if (it->index + 1 != next->index)
+                {
+                    // we have a gap before the next node with the same list and style,
+                    // with no other lists in between. There will be a continuation;
+                    // in case of restart, there will be a reference to the id;
+                    // otherwise, there will be simple 'text:continue-numbering="true"'.
+                    return !next->isRestart;
+                }
+                it = next; // walk through adjacent nodes of the same list
+            }
+            // all nodes were adjacent and of the same list and style -> no continuation, skip id
+            return true;
+        }
+
+        return false;
+    }
+};
+
+bool XMLTextParagraphExport::ShouldSkipListId(const Reference<XTextContent>& xTextContent)
+{
+    if (!mpDocumentListNodes)
+    {
+        if (ExportListId())
+            mpDocumentListNodes.reset(new DocumentListNodes(GetExport().GetModel()));
+        else
+            mpDocumentListNodes.reset(new DocumentListNodes({}));
+    }
+
+    return mpDocumentListNodes->ShouldSkipListId(xTextContent);
+}
+
 void XMLTextParagraphExport::exportTextContentEnumeration(
         const Reference < XEnumeration > & rContEnum,
         bool bAutoStyles,
@@ -1866,7 +1975,8 @@ void XMLTextParagraphExport::exportTextContentEnumeration(
                 aNextNumInfo.Set( xTxtCntnt,
                                   GetExport().writeOutlineStyleAsNormalListStyle(),
                                   GetListAutoStylePool(),
-                                  GetExport().exportTextNumberElement() );
+                                  GetExport().exportTextNumberElement(),
+                                  ShouldSkipListId(xTxtCntnt) );
 
                 exportListAndSectionChange( xCurrentTextSection, aPropSetHelper,
                                             TEXT_SECTION, xTxtCntnt,