tdf#159815 DOCX import: fix redlined to-char image followed by inline SDT

The 2nd para of the bugdoc has a leading word, then a redlined anchored
shape, finally a redlined checkbox content control. The Writer import
result's content control starts at para start, while it should only
start after the anchored shape.

What happens is that writerfilter::dmapper::DomainMapper_Impl::PushSdt()
gets called to remember the SDT start, then
DomainMapper_Impl::applyToggleAttributes() deletes some content since
commit 8726cf692299ea262a7455adcf6ec25451c7869d (tdf#142700 DOCX: fix
lost track changes of images, 2021-07-08), which invalidates the SDT
start position, finally PopSdt gets called, but that fails because our
start position is no longer valid. This used to abort the import
process. Since commit 1b0f67018fa1d514ebca59e081efdd24c1d7811b (docx
import: correct redline content-controls, 2024-02-20), the import
finishes the but start of the SDT is incorrect: it's at the para start,
while it should start in the middle of the paragraph. The direct reason
for the invalidation is a call to SwXTextRange::Impl::Notify(), which is
from a content deletion from applyToggleAttributes().

Fix the problem by not deleting content when we're past PushSdt() and we
haven't called PopSdt(): extract the redlined anchored shape code into a
new DomainMapper_Impl::MergeAtContentImageRedlineWithNext() and call
that also in DomainMapper_Impl::PushSdt() early, so it won't be called
when we're in the middle of an SDT.

This way createTextCursorByRange() should not fail, so warn in case it
still fails in DomainMapper_Impl::PopSdt().

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

4 files changed, 88 insertions, 33 deletions

diff --git a/writerfilter/qa/cppunittests/dmapper/DomainMapper_Impl.cxx b/writerfilter/qa/cppunittests/dmapper/DomainMapper_Impl.cxx
index d0693746f24b..16aa5cbfb2df 100644
--- a/writerfilter/qa/cppunittests/dmapper/DomainMapper_Impl.cxx
+++ b/writerfilter/qa/cppunittests/dmapper/DomainMapper_Impl.cxx
@@ -366,6 +366,51 @@ CPPUNIT_TEST_FIXTURE(Test, testFloattableSectend)
     // i.e. the first table was lost.
     CPPUNIT_ASSERT_EQUAL(static_cast<sal_Int32>(2), xTables->getCount());
 }
+
+CPPUNIT_TEST_FIXTURE(Test, testRedlinedShapeThenSdt)
+{
+    // Given a file with a second paragraph where text is followed by a redline, then an SDT:
+    // When importing that document:
+    loadFromFile(u"redlined-shape-sdt.docx");
+
+    // Then make sure the content control doesn't start at para start:
+    uno::Reference<text::XTextDocument> xTextDocument(mxComponent, uno::UNO_QUERY);
+    uno::Reference<container::XEnumerationAccess> xParaEnumAccess(xTextDocument->getText(),
+                                                                  uno::UNO_QUERY);
+    uno::Reference<container::XEnumeration> xParaEnum = xParaEnumAccess->createEnumeration();
+    xParaEnum->nextElement();
+    uno::Reference<container::XEnumerationAccess> xPortionEnumAccess(xParaEnum->nextElement(),
+                                                                     uno::UNO_QUERY);
+    uno::Reference<container::XEnumeration> xPortionEnum = xPortionEnumAccess->createEnumeration();
+
+    uno::Reference<beans::XPropertySet> xPortion(xPortionEnum->nextElement(), uno::UNO_QUERY);
+    // Without the accompanying fix in place, this test would have failed with:
+    // - Expected: Text
+    // - Actual  : ContentControl
+    // i.e. the content control started at para start.
+    CPPUNIT_ASSERT_EQUAL(u"Text"_ustr,
+                         xPortion->getPropertyValue("TextPortionType").get<OUString>());
+    xPortion.set(xPortionEnum->nextElement(), uno::UNO_QUERY);
+    // Redline start+end pair, containing a pair of text portions with an anchored object in the
+    // middle.
+    CPPUNIT_ASSERT_EQUAL(u"Redline"_ustr,
+                         xPortion->getPropertyValue("TextPortionType").get<OUString>());
+    xPortion.set(xPortionEnum->nextElement(), uno::UNO_QUERY);
+    CPPUNIT_ASSERT_EQUAL(u"Text"_ustr,
+                         xPortion->getPropertyValue("TextPortionType").get<OUString>());
+    xPortion.set(xPortionEnum->nextElement(), uno::UNO_QUERY);
+    CPPUNIT_ASSERT_EQUAL(u"Frame"_ustr,
+                         xPortion->getPropertyValue("TextPortionType").get<OUString>());
+    xPortion.set(xPortionEnum->nextElement(), uno::UNO_QUERY);
+    CPPUNIT_ASSERT_EQUAL(u"Text"_ustr,
+                         xPortion->getPropertyValue("TextPortionType").get<OUString>());
+    xPortion.set(xPortionEnum->nextElement(), uno::UNO_QUERY);
+    CPPUNIT_ASSERT_EQUAL(u"Redline"_ustr,
+                         xPortion->getPropertyValue("TextPortionType").get<OUString>());
+    xPortion.set(xPortionEnum->nextElement(), uno::UNO_QUERY);
+    CPPUNIT_ASSERT_EQUAL(u"ContentControl"_ustr,
+                         xPortion->getPropertyValue("TextPortionType").get<OUString>());
+}
 }
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/writerfilter/qa/cppunittests/dmapper/data/redlined-shape-sdt.docx b/writerfilter/qa/cppunittests/dmapper/data/redlined-shape-sdt.docx
new file mode 100644
index 000000000000..ea7f4a5bd664
--- /dev/null
+++ b/writerfilter/qa/cppunittests/dmapper/data/redlined-shape-sdt.docx
diff --git a/writerfilter/source/dmapper/DomainMapper_Impl.cxx b/writerfilter/source/dmapper/DomainMapper_Impl.cxx
index d5c6d553ea11..f73504824ea0 100644
--- a/writerfilter/source/dmapper/DomainMapper_Impl.cxx
+++ b/writerfilter/source/dmapper/DomainMapper_Impl.cxx
@@ -984,6 +984,10 @@ void DomainMapper_Impl::PushSdt()
         return;
     }
 
+    // This may delete text, so call it early, before we would set our start position, which may be
+    // invalidated by a delete.
+    MergeAtContentImageRedlineWithNext(xTextAppend);
+
     uno::Reference<text::XText> xText = xTextAppend->getText();
     if (!xText.is())
     {
@@ -1022,6 +1026,7 @@ void DomainMapper_Impl::PopSdt()
     }
     catch (const uno::RuntimeException&)
     {
+        TOOLS_WARN_EXCEPTION("writerfilter", "DomainMapper_Impl::DomainMapper_Impl::PopSdt: createTextCursorByRange() failed");
         // We redline form controls and that gets us confused when
         // we process the SDT around the placeholder. What seems to
         // happen is we lose the text-range when we pop the SDT position.
@@ -3187,6 +3192,42 @@ void DomainMapper_Impl::applyToggleAttributes(const PropertyMapPtr& pPropertyMap
     }
 }
 
+void DomainMapper_Impl::MergeAtContentImageRedlineWithNext(const css::uno::Reference<css::text::XTextAppend>& xTextAppend)
+{
+    // remove workaround for change tracked images, if they are part of a redline,
+    // i.e. if the next run is a tracked change with the same type, author and date,
+    // as in the change tracking of the image.
+    if ( m_bRedlineImageInPreviousRun )
+    {
+        auto pCurrentRedline = m_aRedlines.top().size() > 0
+            ? m_aRedlines.top().back()
+            : GetTopContextOfType(CONTEXT_CHARACTER) &&
+            GetTopContextOfType(CONTEXT_CHARACTER)->Redlines().size() > 0
+            ? GetTopContextOfType(CONTEXT_CHARACTER)->Redlines().back()
+            : nullptr;
+        if ( m_previousRedline && pCurrentRedline &&
+                // same redline
+                (m_previousRedline->m_nToken & 0xffff) == (pCurrentRedline->m_nToken & 0xffff) &&
+                m_previousRedline->m_sAuthor == pCurrentRedline->m_sAuthor &&
+                m_previousRedline->m_sDate == pCurrentRedline->m_sDate )
+        {
+            uno::Reference< text::XTextCursor > xCursor = xTextAppend->getEnd()->getText( )->createTextCursor( );
+            assert(xCursor.is());
+            xCursor->gotoEnd(false);
+            xCursor->goLeft(2, true);
+            if ( xCursor->getString() == u"​​" )
+            {
+                xCursor->goRight(1, true);
+                xCursor->setString("");
+                xCursor->gotoEnd(false);
+                xCursor->goLeft(1, true);
+                xCursor->setString("");
+            }
+        }
+
+        m_bRedlineImageInPreviousRun = false;
+    }
+}
 
     void DomainMapper_Impl::appendTextPortion( const OUString& rString, const PropertyMapPtr& pPropertyMap )
 {
@@ -3220,39 +3261,7 @@ void DomainMapper_Impl::applyToggleAttributes(const PropertyMapPtr& pPropertyMap
                     rValue.Value <<= false;
             }
 
-        // remove workaround for change tracked images, if they are part of a redline,
-        // i.e. if the next run is a tracked change with the same type, author and date,
-        // as in the change tracking of the image.
-        if ( m_bRedlineImageInPreviousRun )
-        {
-            auto pCurrentRedline = m_aRedlines.top().size() > 0
-                    ? m_aRedlines.top().back()
-                    : GetTopContextOfType(CONTEXT_CHARACTER) &&
-                                GetTopContextOfType(CONTEXT_CHARACTER)->Redlines().size() > 0
-                        ? GetTopContextOfType(CONTEXT_CHARACTER)->Redlines().back()
-                        : nullptr;
-            if ( m_previousRedline && pCurrentRedline &&
-                   // same redline
-                   (m_previousRedline->m_nToken & 0xffff) == (pCurrentRedline->m_nToken & 0xffff) &&
-                    m_previousRedline->m_sAuthor == pCurrentRedline->m_sAuthor &&
-                    m_previousRedline->m_sDate == pCurrentRedline->m_sDate )
-            {
-                uno::Reference< text::XTextCursor > xCursor = xTextAppend->getEnd()->getText( )->createTextCursor( );
-                assert(xCursor.is());
-                xCursor->gotoEnd(false);
-                xCursor->goLeft(2, true);
-                if ( xCursor->getString() == u"​​" )
-                {
-                    xCursor->goRight(1, true);
-                    xCursor->setString("");
-                    xCursor->gotoEnd(false);
-                    xCursor->goLeft(1, true);
-                    xCursor->setString("");
-                }
-            }
-
-            m_bRedlineImageInPreviousRun = false;
-        }
+        MergeAtContentImageRedlineWithNext(xTextAppend);
 
         uno::Reference< text::XTextRange > xTextRange;
         if (m_aTextAppendStack.top().xInsertPosition.is())
diff --git a/writerfilter/source/dmapper/DomainMapper_Impl.hxx b/writerfilter/source/dmapper/DomainMapper_Impl.hxx
index 163c92c04d31..4f4f09e58b1f 100644
--- a/writerfilter/source/dmapper/DomainMapper_Impl.hxx
+++ b/writerfilter/source/dmapper/DomainMapper_Impl.hxx
@@ -804,6 +804,7 @@ public:
 
     void finishParagraph( const PropertyMapPtr& pPropertyMap, const bool bRemove = false, const bool bNoNumbering = false);
     void applyToggleAttributes( const PropertyMapPtr& pPropertyMap );
+    void MergeAtContentImageRedlineWithNext(const css::uno::Reference<css::text::XTextAppend>& xTextAppend);
     void appendTextPortion( const OUString& rString, const PropertyMapPtr& pPropertyMap );
     void appendTextContent(const css::uno::Reference<css::text::XTextContent>&, const css::uno::Sequence<css::beans::PropertyValue>&);
     void appendOLE( const OUString& rStreamName, const std::shared_ptr<OLEHandler>& pOleHandler );