tdf#166319 sw interdependent redlines: handle ODF export of delete under format

The bugdoc has <del>AA<format>BB</format>CC</del> in it, DOCX import
works, but exporting to ODT turns the format-on-delete redline into a
plain "format" redline.

What happens is that various code only checks the m_eType of the first
SwRedlineData of an SwRangeRedline, so format-on-delete is handled as
format. But this is not correct: if the format-on-delete is to be
accepted, then it's just a small detail that there was a format on top
of it: the result should be text deleted from the document on accept,
not accepting the the format changes and leaving the text in the
document.

Fix the problem by:
1) Extending SwRangeRedline::Hide() and SwRangeRedline::Show(), so when
   the ODF export moves delete redlines to their own toplevel SwNode
   section, so it can refer to these ranges using a single SwStartNode
   index, this move happens for format-on-delete as well, not just deletes.
2) Then can extend SwXRedlinePortion::GetPropertyValue()'s
   lcl_GetSuccessorProperties() to produce an XText for the deleted
   range of a format-on-delete redline.
3) Finally consume this new XText in
   XMLRedlineExport::ExportChangeInfo().

Note that this way a format redline can now have its own XText (in case
it's on top of a delete), but we want to export the text only once, so
restrict the content export for a redline to the 'delete' type.

Change-Id: I91902a2ae4b464ace41717a196d19530be9d84d1
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/185551
Tested-by: Jenkins
Reviewed-by: Miklos Vajna <vmiklos@collabora.com>

schema/libreoffice/OpenDocument-v1.4+libreoffice-schema.rng

@@ -4135,4 +4135,15 @@ xmlns:loext="urn:org:documentfoundation:names:experimental:office:xmlns:loext:1.
         <rng:text/>
     </rng:element>
   </rng:define>
+
+  <!-- TODO(vmiklos) no proposal for insert/delete redline under another redline -->
+  <rng:define name="text-changed-region" combine="choice">
+    <rng:element name="text:changed-region">
+      <rng:ref name="text-changed-region-attr"/>
+      <rng:ref name="text-changed-region-content"/>
+      <rng:optional>
+        <rng:ref name="text-changed-region-content"/>
+      </rng:optional>
+    </rng:element>
+  </rng:define>
 </rng:grammar>

sw/source/core/doc/docredln.cxx

@@ -1585,6 +1585,24 @@ void SwRangeRedline::CallDisplayFunc(size_t nMyPos)
         ShowOriginal(0, nMyPos);
 }
 
+namespace
+{
+RedlineType GetRedlineTypeIgnoringAdditonalFormat(const SwRangeRedline& rRedline)
+{
+    RedlineType eType = rRedline.GetType();
+
+    if (eType == RedlineType::Format && rRedline.GetStackCount() > 1
+        && rRedline.GetType(1) == RedlineType::Delete)
+    {
+        // Consider format-on-delete the same as simple delete, so the range gets moved to the
+        // "Deleted Change Tracking content" toplevel section from body content during file save.
+        eType = RedlineType::Delete;
+    }
+
+    return eType;
+}
+}
+
 void SwRangeRedline::Show(sal_uInt16 nLoop, size_t nMyPos, bool bForced)
 {
     SwDoc& rDoc = GetDoc();
@@ -1606,7 +1624,7 @@ void SwRangeRedline::Show(sal_uInt16 nLoop, size_t nMyPos, bool bForced)
     rDoc.getIDocumentRedlineAccess().SetRedlineFlags_intern(eOld | RedlineFlags::Ignore);
     ::sw::UndoGuard const undoGuard(rDoc.GetIDocumentUndoRedo());
 
-    switch( GetType() )
+    switch (GetRedlineTypeIgnoringAdditonalFormat(*this))
     {
     case RedlineType::Insert:           // Content has been inserted
         m_bIsVisible = true;
@@ -1646,7 +1664,7 @@ void SwRangeRedline::Hide(sal_uInt16 nLoop, size_t nMyPos, bool /*bForced*/)
     rDoc.getIDocumentRedlineAccess().SetRedlineFlags_intern(eOld | RedlineFlags::Ignore);
     ::sw::UndoGuard const undoGuard(rDoc.GetIDocumentUndoRedo());
 
-    switch( GetType() )
+    switch (GetRedlineTypeIgnoringAdditonalFormat(*this))
     {
     case RedlineType::Insert:           // Content has been inserted
         m_bIsVisible = true;

sw/source/core/unocore/unoredline.cxx

@@ -176,6 +176,22 @@ static uno::Sequence<beans::PropertyValue> lcl_GetSuccessorProperties(const SwRa
     const SwRedlineData* pNext = rRedline.GetRedlineData().Next();
     if(pNext)
     {
+        uno::Reference<text::XText> xRedlineText;
+        if (pNext->GetType() == RedlineType::Delete)
+        {
+            // Something on delete: produce the XText for the underlying delete.
+            const SwNodeIndex* pNodeIdx = rRedline.GetContentIdx();
+            if (pNodeIdx
+                && (pNodeIdx->GetNode().EndOfSectionIndex() - pNodeIdx->GetNode().GetIndex())
+                       > SwNodeOffset(1))
+            {
+                // We have at least one node between the start and end node, create the
+                // SwXRedlineText.
+                SwDoc& rDoc = rRedline.GetDoc();
+                xRedlineText = new SwXRedlineText(&rDoc, *pNodeIdx);
+            }
+        }
+
         return
         {
             // GetAuthorString(n) walks the SwRedlineData* chain;
@@ -183,10 +199,11 @@ static uno::Sequence<beans::PropertyValue> lcl_GetSuccessorProperties(const SwRa
             comphelper::makePropertyValue(UNO_NAME_REDLINE_AUTHOR, rRedline.GetAuthorString(1)),
             comphelper::makePropertyValue(UNO_NAME_REDLINE_DATE_TIME, pNext->GetTimeStamp().GetUNODateTime()),
             comphelper::makePropertyValue(UNO_NAME_REDLINE_COMMENT, pNext->GetComment()),
-            comphelper::makePropertyValue(UNO_NAME_REDLINE_TYPE, SwRedlineTypeToOUString(pNext->GetType()))
+            comphelper::makePropertyValue(UNO_NAME_REDLINE_TYPE, SwRedlineTypeToOUString(pNext->GetType())),
+            comphelper::makePropertyValue(UNO_NAME_REDLINE_TEXT, xRedlineText)
         };
     }
-    return uno::Sequence<beans::PropertyValue>(4);
+    return uno::Sequence<beans::PropertyValue>(5);
 }
 
 uno::Any SwXRedlinePortion::getPropertyValue( const OUString& rPropertyName )

xmloff/qa/unit/text.cxx

@@ -1286,6 +1286,27 @@ CPPUNIT_TEST_FIXTURE(XmloffStyleTest, testNestedSpans)
     CPPUNIT_ASSERT_EQUAL(awt::FontWeight::BOLD, fWeight);
 }
 
+CPPUNIT_TEST_FIXTURE(XmloffStyleTest, testDeleteThenFormatOdtExport)
+{
+    // Given a document that has a delete redline, and then a format redline on top of it:
+    loadFromFile(u"del-then-format.docx");
+
+    // When exporting that to DOCX:
+    save(u"writer8"_ustr);
+
+    // Then make sure <text:changed-region> not only has the <text:format-change> child, but also an
+    // <text:deletion> one, with text:
+    xmlDocUniquePtr pXmlDoc = parseExport(u"content.xml"_ustr);
+    // Without the accompanying fix in place, this test would have failed with:
+    // - Expected: 1
+    // - Actual  : 0
+    // - XPath '//text:tracked-changes/text:changed-region[2]/text:deletion' number of nodes is incorrect
+    // i.e. <text:insertion> was written for a delete-then-format.
+    assertXPath(pXmlDoc, "//text:tracked-changes/text:changed-region[2]/text:deletion");
+    // Also the delete's text was in the body text.
+    assertXPath(pXmlDoc, "//text:tracked-changes/text:changed-region[2]/text:deletion/text:p");
+}
+
 CPPUNIT_PLUGIN_IMPLEMENT();
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */

xmloff/source/text/XMLRedlineExport.cxx

@@ -43,6 +43,7 @@
 #include <xmloff/xmlexp.hxx>
 #include <xmloff/xmluconv.hxx>
 #include <unotools/securityoptions.hxx>
+#include <comphelper/sequenceashashmap.hxx>
 
 
 using namespace ::com::sun::star;
@@ -361,7 +362,8 @@ void XMLRedlineExport::ExportChangedRegion(
         aAny = rPropSet->getPropertyValue(u"RedlineText"_ustr);
         Reference<XText> xText;
         aAny >>= xText;
-        if (xText.is())
+        // Avoid double export for format-on-delete: no write on format, only on delete.
+        if (xText.is() && sType == u"Delete")
         {
             rExport.GetTextParagraphExport()->exportText(xText);
             // default parameters: bProgress, bExportParagraph ???
@@ -380,15 +382,31 @@ void XMLRedlineExport::ExportChangedRegion(
     // process change info
     if (aSuccessorData.hasElements())
     {
-        // The only change that can be "undone" is an insertion -
-        // after all, you can't re-insert a deletion, but you can
-        // delete an insertion. This assumption is asserted in
-        // ExportChangeInfo(Sequence<PropertyValue>&).
+        // Look up the type of the change. In practice this is always insert or delete, other types
+        // can't have additional redlines on top of them.
+        OUString sType;
+        comphelper::SequenceAsHashMap aMap(aSuccessorData);
+        auto it = aMap.find("RedlineType");
+        if (it != aMap.end())
+        {
+            it->second >>= sType;
+        }
         SvXMLElementExport aSecondChangeElem(
-            rExport, XML_NAMESPACE_TEXT, XML_INSERTION,
+            rExport, XML_NAMESPACE_TEXT, ConvertTypeName(sType),
             true, true);
 
         ExportChangeInfo(aSuccessorData);
+        it = aMap.find("RedlineText");
+        if (it != aMap.end())
+        {
+            // Delete has its own content outside the body text: export it here.
+            uno::Reference<text::XText> xText;
+            it->second >>= xText;
+            if (xText.is())
+            {
+                rExport.GetTextParagraphExport()->exportText(xText);
+            }
+        }
     }
     // else: no hierarchical change
 }
@@ -529,10 +547,10 @@ void XMLRedlineExport::ExportChangeInfo(
         }
         else if( rVal.Name == "RedlineType" )
         {
-            // check if this is an insertion; cf. comment at calling location
+            // check if this has one of the expected types
             OUString sTmp;
             rVal.Value >>= sTmp;
-            DBG_ASSERT(sTmp == "Insert",
+            DBG_ASSERT(sTmp == "Insert" || sTmp == "Delete",
                        "hierarchical change must be insertion");
         }
         // else: unknown value -> ignore