sw/qa/extras/ooxmlexport/data/section_break_after_table.docx |binary sw/qa/extras/ooxmlexport/ooxmlexport20.cxx | 19 + sw/qa/extras/ooxmlexport/ooxmlexport8.cxx | 2 writerfilter/source/dmapper/DomainMapper.cxx | 13 - writerfilter/source/dmapper/DomainMapper_Impl.cxx | 137 ++++++++--- writerfilter/source/dmapper/DomainMapper_Impl.hxx | 3 6 files changed, 122 insertions(+), 52 deletions(-)
New commits: commit 62e8b576b287111bd0dc79e71334f77dcf909938 Author: Mike Kaganski <mike.kagan...@collabora.com> AuthorDate: Mon Dec 25 15:32:41 2023 +0600 Commit: Xisco Fauli <xiscofa...@libreoffice.org> CommitDate: Wed Jan 3 10:26:58 2024 +0100 tdf#158855: Make sure to not add extra paragraph after a table in a section Simplify the condition for last paragraph removal in lcl_utext, and clean up the complications piled after commit 2e8aad6d45c53d554ccaf26de998ede708cfc289 (fdo#39056 fdo#75431 Section Properties if section starts with table, 2014-04-24). This makes sure that the trailing extra paragraph gets properly removed, even when there was a dummy paragraph added for a table in section. Because of the removed check, import used to avoid the removal of the extra paragraph immediately following a table (when the table needed to be the last element in the section); hence, in DomainMapper_Impl::appendTextSectionAfter, a last section's table was followed by two paragraphs (one that is expected to stay after the created section, and one before that, that was not removed after the table). xCursor had ininitally both paragraphs in the selection after the forward goto* calls; and then, gotoLeft excluded only the last paragraph from the selection. After the change, in such a case, there is only one paragraph in the end: the one that should stay after the created section. But calling gotoLeft then would have to put the end of selection to a text node inside the table; since the selection that starts before the table can't end inside the table, the call resulted in the whole table excluded from the selection. This requires to add a paragraph here, to allow the previous behavior of cursor; and then the last paragraph in section is destroyed. The code makes sure that the trailing paragraph kept after the creation of the section has all the properties applied to the old last paragraph. testFdo53985 used to check for total 9 paragraphs in the document; while 3 of those were introduced exactly by the bug fixed here; they are absent in Word. The test is fixed; its logic is unchanged, and it still correctly tests that a round-trip doesn't increase number of paragraphs. Change-Id: Ibf6349ba7906dac185c759eb3b58c1849ee5064a Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161275 Tested-by: Jenkins Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com> Signed-off-by: Xisco Fauli <xiscofa...@libreoffice.org> Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161300 diff --git a/sw/qa/extras/ooxmlexport/data/section_break_after_table.docx b/sw/qa/extras/ooxmlexport/data/section_break_after_table.docx new file mode 100644 index 000000000000..9f8e4c687bbb Binary files /dev/null and b/sw/qa/extras/ooxmlexport/data/section_break_after_table.docx differ diff --git a/sw/qa/extras/ooxmlexport/ooxmlexport20.cxx b/sw/qa/extras/ooxmlexport/ooxmlexport20.cxx index 4a4e6e087b68..639aea237618 100644 --- a/sw/qa/extras/ooxmlexport/ooxmlexport20.cxx +++ b/sw/qa/extras/ooxmlexport/ooxmlexport20.cxx @@ -1022,6 +1022,25 @@ CPPUNIT_TEST_FIXTURE(Test, testtdf158044) assertXPath(pXmlDoc, "/w:document/w:body/w:p[8]/w:r[4]/w:rPr[1]/w:shadow[1]"_ostr); } +CPPUNIT_TEST_FIXTURE(Test, testTdf158855) +{ + // Given a table immediately followed by a section break + load(createFileURL(u"section_break_after_table.docx")); + + // Check that the import doesn't produce an extra empty paragraph before a page break + CPPUNIT_ASSERT_EQUAL(2, getPages()); // was 3 + CPPUNIT_ASSERT_EQUAL(2, getParagraphs()); // was 3 + uno::Reference<text::XTextTable>(getParagraphOrTable(1), uno::UNO_QUERY_THROW); + getParagraph(2, u"Next page"_ustr); // was empty, with the 3rd being "Next page" + + saveAndReload(mpFilter); + + CPPUNIT_ASSERT_EQUAL(2, getPages()); + CPPUNIT_ASSERT_EQUAL(2, getParagraphs()); + uno::Reference<text::XTextTable>(getParagraphOrTable(1), uno::UNO_QUERY_THROW); + getParagraph(2, u"Next page"_ustr); +} + CPPUNIT_PLUGIN_IMPLEMENT(); /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/sw/qa/extras/ooxmlexport/ooxmlexport8.cxx b/sw/qa/extras/ooxmlexport/ooxmlexport8.cxx index 158b8f61d873..10fd87ebec2c 100644 --- a/sw/qa/extras/ooxmlexport/ooxmlexport8.cxx +++ b/sw/qa/extras/ooxmlexport/ooxmlexport8.cxx @@ -756,7 +756,7 @@ DECLARE_OOXMLEXPORT_TEST(testFdo53985, "fdo53985.docx") CPPUNIT_ASSERT_EQUAL_MESSAGE("Section3 is protected", false, getProperty<bool>(xSect, "IsProtected")); // This was increasing by 3 every round-trip - an extra paragraph after each table in sections - CPPUNIT_ASSERT_EQUAL(9, getParagraphs()); + CPPUNIT_ASSERT_EQUAL(6, getParagraphs()); } DECLARE_OOXMLEXPORT_TEST(testFdo59638, "fdo59638.docx") diff --git a/writerfilter/source/dmapper/DomainMapper.cxx b/writerfilter/source/dmapper/DomainMapper.cxx index e18819401417..7e81cfd430c2 100644 --- a/writerfilter/source/dmapper/DomainMapper.cxx +++ b/writerfilter/source/dmapper/DomainMapper.cxx @@ -3897,10 +3897,7 @@ void DomainMapper::lcl_text(const sal_uInt8 * data_, size_t len) case 0x0c: //page break // page breaks aren't supported in footnotes and endnotes if (!m_pImpl->IsInFootOrEndnote()) - { m_pImpl->deferBreak(PAGE_BREAK); - m_pImpl->SetIsDummyParaAddedForTableInSectionPage(false); - } return; case 0x0e: //column break m_pImpl->deferBreak(COLUMN_BREAK); @@ -4403,7 +4400,6 @@ void DomainMapper::lcl_utext(const sal_Unicode *const data_, size_t len) && !bIsColumnBreak && !m_pImpl->GetIsLastSectionGroup() // testContSectionPageBreak && !m_pImpl->GetParaHadField() - && (!m_pImpl->GetIsDummyParaAddedForTableInSectionPage()) && !m_pImpl->GetIsPreviousParagraphFramed() && !m_pImpl->HasTopAnchoredObjects() && !m_pImpl->IsParaWithInlineObject()); @@ -4424,15 +4420,6 @@ void DomainMapper::lcl_utext(const sal_Unicode *const data_, size_t len) if (bRemove) m_pImpl->RemoveLastParagraph(); - // When the table is closed and the section's initial dummy paragraph has been processed - // then any following sectPr paragraph in the section must be eligible for removal. - if (!bRemove && m_pImpl->GetIsDummyParaAddedForTableInSectionPage() && !IsInTable() - && !m_pImpl->GetFootnoteContext() && !m_pImpl->IsInComments() && !IsInHeaderFooter() - && !IsInShape()) - { - m_pImpl->SetIsDummyParaAddedForTableInSectionPage(false); - } - m_pImpl->SetParaSectpr(false); } else diff --git a/writerfilter/source/dmapper/DomainMapper_Impl.cxx b/writerfilter/source/dmapper/DomainMapper_Impl.cxx index 29de4e1b73af..11d58a3b2031 100644 --- a/writerfilter/source/dmapper/DomainMapper_Impl.cxx +++ b/writerfilter/source/dmapper/DomainMapper_Impl.cxx @@ -29,6 +29,7 @@ #include "TagLogger.hxx" #include <com/sun/star/uno/XComponentContext.hpp> #include <com/sun/star/graphic/XGraphic.hpp> +#include <com/sun/star/beans/PropertyAttribute.hpp> #include <com/sun/star/beans/XPropertyState.hpp> #include <com/sun/star/container/XNamed.hpp> #include <com/sun/star/document/PrinterIndependentLayout.hpp> @@ -380,7 +381,6 @@ DomainMapper_Impl::DomainMapper_Impl( m_bIsFirstParaInSection( true ), m_bIsFirstParaInSectionAfterRedline( true ), m_bDummyParaAddedForTableInSection( false ), - m_bDummyParaAddedForTableInSectionPage( false ), m_bTextFrameInserted(false), m_bIsPreviousParagraphFramed( false ), m_bIsLastParaInSection( false ), @@ -979,12 +979,6 @@ void DomainMapper_Impl::SetIsFirstParagraphInShape(bool bIsFirst) void DomainMapper_Impl::SetIsDummyParaAddedForTableInSection( bool bIsAdded ) { m_bDummyParaAddedForTableInSection = bIsAdded; - m_bDummyParaAddedForTableInSectionPage = bIsAdded; -} - -void DomainMapper_Impl::SetIsDummyParaAddedForTableInSectionPage( bool bIsAdded ) -{ - m_bDummyParaAddedForTableInSectionPage = bIsAdded; } @@ -3519,6 +3513,86 @@ void DomainMapper_Impl::adjustLastPara(sal_Int8 nAlign) pLastPara->Insert(PROP_PARA_ADJUST, uno::Any(nAlign), true); } +static void checkAndAddPropVal(const OUString& prop, const css::uno::Any& val, + std::vector<OUString>& props, std::vector<css::uno::Any>& values) +{ + // Avoid well-known reasons for exceptions when setting property values + if (!val.hasValue()) + return; + if (prop == "CharAutoStyleName" || prop == "ParaAutoStyleName") + return; + if (prop == "CharStyleName" || prop == "DropCapCharStyleName") + if (OUString val_string; (val >>= val_string) && val_string.isEmpty()) + return; + + props.push_back(prop); + values.push_back(val); +} + +static void copyAllProps(const css::uno::Reference<css::uno::XInterface>& from, + const css::uno::Reference<css::uno::XInterface>& to) +{ + css::uno::Reference<css::beans::XPropertySet> xFromProps(from, css::uno::UNO_QUERY_THROW); + css::uno::Reference<css::beans::XPropertySetInfo> xFromInfo(xFromProps->getPropertySetInfo(), + css::uno::UNO_SET_THROW); + css::uno::Sequence<css::beans::Property> rawProps(xFromInfo->getProperties()); + std::vector<OUString> props; + props.reserve(rawProps.getLength()); + for (const auto& prop : rawProps) + if ((prop.Attributes & css::beans::PropertyAttribute::READONLY) == 0) + props.push_back(prop.Name); + std::vector<css::uno::Any> values; + values.reserve(props.size()); + if (css::uno::Reference<css::beans::XMultiPropertySet> xFromMulti{ xFromProps, + css::uno::UNO_QUERY }) + { + const auto propsSeq = comphelper::containerToSequence(props); + const auto valuesSeq = xFromMulti->getPropertyValues(propsSeq); + assert(propsSeq.getLength() == valuesSeq.getLength()); + props.clear(); + for (size_t i = 0; i < propsSeq.size(); ++i) + checkAndAddPropVal(propsSeq[i], valuesSeq[i], props, values); + } + else + { + std::vector<OUString> filtered_props; + filtered_props.reserve(props.size()); + for (const auto& prop : props) + checkAndAddPropVal(prop, xFromProps->getPropertyValue(prop), filtered_props, values); + filtered_props.swap(props); + } + assert(props.size() == values.size()); + + css::uno::Reference<css::beans::XPropertySet> xToProps(to, css::uno::UNO_QUERY_THROW); + if (css::uno::Reference<css::beans::XMultiPropertySet> xToMulti{ xToProps, + css::uno::UNO_QUERY }) + { + try + { + xToMulti->setPropertyValues(comphelper::containerToSequence(props), + comphelper::containerToSequence(values)); + return; + } + catch (css::uno::Exception&) + { + DBG_UNHANDLED_EXCEPTION("writerfilter.dmapper"); + } + // Fallback to property-by-property iteration + } + + for (size_t i = 0; i < props.size(); ++i) + { + try + { + xToProps->setPropertyValue(props[i], values[i]); + } + catch (css::uno::Exception&) + { + DBG_UNHANDLED_EXCEPTION("writerfilter.dmapper"); + } + } +} + uno::Reference< beans::XPropertySet > DomainMapper_Impl::appendTextSectionAfter( uno::Reference< text::XTextRange > const & xBefore ) { @@ -3538,40 +3612,33 @@ uno::Reference< beans::XPropertySet > DomainMapper_Impl::appendTextSectionAfter( xCursor->gotoRange( m_aTextAppendStack.top().xInsertPosition, true ); else xCursor->gotoEnd( true ); - //the paragraph after this new section is already inserted - xCursor->goLeft(1, true); - css::uno::Reference<css::text::XTextRange> xTextRange(xCursor, css::uno::UNO_QUERY_THROW); + // The paragraph after this new section is already inserted. The previous node may be a + // table; then trying to go left would skip the whole table. Split the trailing + // paragraph; let the section span over the first of the two resulting paragraphs; + // destroy the last section's paragraph afterwards. + css::uno::Reference<css::text::XTextRange> xEndPara = xCursor->getEnd(); + xTextAppend->insertControlCharacter( + xEndPara, css::text::ControlCharacter::PARAGRAPH_BREAK, false); + css::uno::Reference<css::text::XTextRange> xNewPara = xCursor->getEnd(); + xCursor->gotoPreviousParagraph(true); + xEndPara = xCursor->getEnd(); + // xEndPara may already have properties (like page break); make sure to apply them + // to the newly appended paragraph, which will be kept in the end. + copyAllProps(xEndPara, xNewPara); - if (css::uno::Reference<css::text::XDocumentIndexesSupplier> xIndexSupplier{ - GetTextDocument(), css::uno::UNO_QUERY }) + uno::Reference< text::XTextContent > xSection( m_xTextFactory->createInstance("com.sun.star.text.TextSection"), uno::UNO_QUERY_THROW ); + xSection->attach(xCursor); + + // Remove the extra paragraph (last inside the section) + if (uno::Reference<container::XEnumerationAccess> xEA{ xEndPara, uno::UNO_QUERY }) { - css::uno::Reference<css::text::XTextRangeCompare> xCompare( - xTextAppend, css::uno::UNO_QUERY); - const auto xIndexAccess = xIndexSupplier->getDocumentIndexes(); - for (sal_Int32 i = xIndexAccess->getCount(); i > 0; --i) + if (uno::Reference<lang::XComponent> xParagraph{ + xEA->createEnumeration()->nextElement(), uno::UNO_QUERY }) { - if (css::uno::Reference<css::text::XDocumentIndex> xIndex{ - xIndexAccess->getByIndex(i - 1), css::uno::UNO_QUERY }) - { - const auto xIndexTextRange = xIndex->getAnchor(); - if (xCompare->compareRegionStarts(xTextRange, xIndexTextRange) == 0 - && xCompare->compareRegionEnds(xTextRange, xIndexTextRange) == 0) - { - // The boundaries coincide with an index: trying to attach a section - // to the range will insert the section inside the index. goRight will - // extend the range outside of the index, so that created section will - // be around it. Alternatively we could return index section itself - // instead : xRet.set(xIndex, uno::UNO_QUERY) - to set its properties, - // like columns/fill. - xCursor->goRight(1, true); - break; - } - } + xParagraph->dispose(); } } - uno::Reference< text::XTextContent > xSection( m_xTextFactory->createInstance("com.sun.star.text.TextSection"), uno::UNO_QUERY_THROW ); - xSection->attach( xTextRange ); xRet.set(xSection, uno::UNO_QUERY ); } catch(const uno::Exception&) diff --git a/writerfilter/source/dmapper/DomainMapper_Impl.hxx b/writerfilter/source/dmapper/DomainMapper_Impl.hxx index 32f50fec6aa3..fbca4aa1f3f0 100644 --- a/writerfilter/source/dmapper/DomainMapper_Impl.hxx +++ b/writerfilter/source/dmapper/DomainMapper_Impl.hxx @@ -589,7 +589,6 @@ private: bool m_bIsFirstParaInSectionAfterRedline; bool m_bIsFirstParaInShape = false; bool m_bDummyParaAddedForTableInSection; - bool m_bDummyParaAddedForTableInSectionPage; bool m_bTextFrameInserted; bool m_bIsPreviousParagraphFramed; bool m_bIsLastParaInSection; @@ -720,8 +719,6 @@ public: bool GetIsFirstParagraphInShape() const { return m_bIsFirstParaInShape; } void SetIsDummyParaAddedForTableInSection( bool bIsAdded ); bool GetIsDummyParaAddedForTableInSection() const { return m_bDummyParaAddedForTableInSection;} - void SetIsDummyParaAddedForTableInSectionPage(bool bIsAdded); - bool GetIsDummyParaAddedForTableInSectionPage() const { return m_bDummyParaAddedForTableInSectionPage; } /// Track if a textframe has been inserted into this section void SetIsTextFrameInserted( bool bIsInserted );