writerfilter/qa/cppunittests/dmapper/DomainMapper_Impl.cxx | 45 +++++ writerfilter/qa/cppunittests/dmapper/data/redlined-shape-sdt.docx |binary writerfilter/source/dmapper/DomainMapper_Impl.cxx | 76 +++++----- writerfilter/source/dmapper/DomainMapper_Impl.hxx | 1 4 files changed, 89 insertions(+), 33 deletions(-)
New commits: commit 6b725cd782a8048929af2a136dc37f7cc0b7b8f3 Author: Miklos Vajna <[email protected]> AuthorDate: Wed Feb 21 10:22:12 2024 +0100 Commit: Caolán McNamara <[email protected]> CommitDate: Thu Feb 22 11:48:03 2024 +0100 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(). Conflicts: writerfilter/source/dmapper/DomainMapper_Impl.cxx writerfilter/source/dmapper/DomainMapper_Impl.hxx (cherry picked from commit 833abb4a197561c34ec59cceb9d7d8a46f6b17ce) Change-Id: Ic4198804a92088ec268203d44c0da2d6997754b7 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/163716 Tested-by: Jenkins CollaboraOffice <[email protected]> Tested-by: Caolán McNamara <[email protected]> Reviewed-by: Caolán McNamara <[email protected]> diff --git a/writerfilter/qa/cppunittests/dmapper/DomainMapper_Impl.cxx b/writerfilter/qa/cppunittests/dmapper/DomainMapper_Impl.cxx index 61ad254279ee..dc99e6ae2168 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: + loadFromURL(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(OUString("Text"), + 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(OUString("Redline"), + xPortion->getPropertyValue("TextPortionType").get<OUString>()); + xPortion.set(xPortionEnum->nextElement(), uno::UNO_QUERY); + CPPUNIT_ASSERT_EQUAL(OUString("Text"), + xPortion->getPropertyValue("TextPortionType").get<OUString>()); + xPortion.set(xPortionEnum->nextElement(), uno::UNO_QUERY); + CPPUNIT_ASSERT_EQUAL(OUString("Frame"), + xPortion->getPropertyValue("TextPortionType").get<OUString>()); + xPortion.set(xPortionEnum->nextElement(), uno::UNO_QUERY); + CPPUNIT_ASSERT_EQUAL(OUString("Text"), + xPortion->getPropertyValue("TextPortionType").get<OUString>()); + xPortion.set(xPortionEnum->nextElement(), uno::UNO_QUERY); + CPPUNIT_ASSERT_EQUAL(OUString("Redline"), + xPortion->getPropertyValue("TextPortionType").get<OUString>()); + xPortion.set(xPortionEnum->nextElement(), uno::UNO_QUERY); + CPPUNIT_ASSERT_EQUAL(OUString("ContentControl"), + 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 Binary files /dev/null and b/writerfilter/qa/cppunittests/dmapper/data/redlined-shape-sdt.docx differ diff --git a/writerfilter/source/dmapper/DomainMapper_Impl.cxx b/writerfilter/source/dmapper/DomainMapper_Impl.cxx index f735f26141a9..a4f3a4877f3a 100644 --- a/writerfilter/source/dmapper/DomainMapper_Impl.cxx +++ b/writerfilter/source/dmapper/DomainMapper_Impl.cxx @@ -982,6 +982,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()) { @@ -1020,6 +1024,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. @@ -2874,6 +2879,43 @@ void DomainMapper_Impl::finishParagraph( const PropertyMapPtr& pPropertyMap, con } +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 ) { if (m_bDiscardHeaderFooter) @@ -2901,39 +2943,7 @@ void DomainMapper_Impl::appendTextPortion( const OUString& rString, const Proper 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 bf1de04c05e2..3fcd02048251 100644 --- a/writerfilter/source/dmapper/DomainMapper_Impl.hxx +++ b/writerfilter/source/dmapper/DomainMapper_Impl.hxx @@ -775,6 +775,7 @@ public: bool isParaSdtEndDeferred() const; void finishParagraph( const PropertyMapPtr& pPropertyMap, const bool bRemove = false, const bool bNoNumbering = false); + 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 );
