sw/inc/doc.hxx | 2 - sw/inc/strings.hrc | 2 + sw/qa/core/doc/data/copy-bookmarks.docx |binary sw/qa/core/doc/doc.cxx | 43 +++++++++++++++++++++++++++ sw/source/core/doc/DocumentLayoutManager.cxx | 9 +++-- sw/source/core/doc/docbm.cxx | 4 +- sw/source/core/doc/doclay.cxx | 26 ++++++++++++++-- 7 files changed, 77 insertions(+), 9 deletions(-)
New commits: commit dde033a34bb89421caec92ffd47ade956ffbad2b Author: Miklos Vajna <[email protected]> AuthorDate: Fri Oct 14 14:28:41 2022 +0200 Commit: Mike Kaganski <[email protected]> CommitDate: Tue Dec 6 06:21:10 2022 +0000 sw: improve duplicated images in copied header/footer text DOCX import currently maps linked headers to 2 Writer headers, the second header has a copy of the first header's content. The image in the first header is named 'Picture 1', the copied header names the image as 'Image1'. This is similar to what commit 41403fbff8140ad0ca7cf8f81d37cddcfbd19197 (sw: improve duplicated bookmarks in copied header/footer text, 2022-10-13) fixed for bookmarks, what happens is that sw::DocumentLayoutManager::CopyLayoutFormat() clears the name of the image, and then these are filled in at the end of the import in one shot, to improve performance. The downside is that it's not possible for an API user to know which was the original image and which is the copy. Fix the problem by tweaking the in-header-footer && not-in-mail-merge case to generate a name like 'Picture 1 Copy 1': this is meant to preserve the lost connection between the original image and its copy, while maintaining performance for the mail merge and body text cases where we expect lots of images. In the long run it would probably make sense to rather support linked headers/footers in Writer core so we don't have to create such a copy in the first place. (cherry picked from commit 8d4f64427528f76afa4bf39a23edaa991850a50a) Conflicts: sw/qa/core/doc/doc.cxx sw/source/core/doc/DocumentLayoutManager.cxx Change-Id: I9679c0ce67131ed5c48eaecfcfd38abd1bcd3da4 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/143670 Tested-by: Jenkins Reviewed-by: Mike Kaganski <[email protected]> diff --git a/sw/inc/doc.hxx b/sw/inc/doc.hxx index 6c92c94ae760..581d06bfcef9 100644 --- a/sw/inc/doc.hxx +++ b/sw/inc/doc.hxx @@ -679,7 +679,7 @@ public: SwDBData const & GetDBData(); // Some helper functions - OUString GetUniqueGrfName() const; + OUString GetUniqueGrfName(std::u16string_view rPrefix = std::u16string_view()) const; OUString GetUniqueOLEName() const; OUString GetUniqueFrameName() const; OUString GetUniqueShapeName() const; diff --git a/sw/qa/core/doc/data/copy-bookmarks.docx b/sw/qa/core/doc/data/copy-bookmarks.docx index 3fb27b430a17..a9bedb487946 100644 Binary files a/sw/qa/core/doc/data/copy-bookmarks.docx and b/sw/qa/core/doc/data/copy-bookmarks.docx differ diff --git a/sw/qa/core/doc/doc.cxx b/sw/qa/core/doc/doc.cxx index f9d475216d2b..28fbe3feaaf8 100644 --- a/sw/qa/core/doc/doc.cxx +++ b/sw/qa/core/doc/doc.cxx @@ -394,6 +394,24 @@ CPPUNIT_TEST_FIXTURE(SwCoreDocTest, testCopyBookmarks) // - Actual : 2 // i.e. the 2nd header had a duplicated bookmark without "Copy" in its name. CPPUNIT_ASSERT_EQUAL(static_cast<sal_Int32>(1), nActual); + + // Also, when checking the # of non-copy images in the resulting doc model: + nActual = 0; + SwFrameFormats& rFrameFormats = *pDoc->GetSpzFrameFormats(); + for (size_t i = 0; i < rFrameFormats.size(); ++i) + { + if (rFrameFormats[i]->GetName().indexOf("Copy") == -1) + { + ++nActual; + } + } + + // Then make sure we have a single non-copy image, with no duplications: + // Without the accompanying fix in place, this test would have failed with: + // - Expected: 1 + // - Actual : 2 + // i.e. the 2nd header had a duplicated image without "Copy" in its name. + CPPUNIT_ASSERT_EQUAL(static_cast<sal_Int32>(1), nActual); } CPPUNIT_PLUGIN_IMPLEMENT(); diff --git a/sw/source/core/doc/DocumentLayoutManager.cxx b/sw/source/core/doc/DocumentLayoutManager.cxx index a03d5dc1d60d..87c75b3e246e 100644 --- a/sw/source/core/doc/DocumentLayoutManager.cxx +++ b/sw/source/core/doc/DocumentLayoutManager.cxx @@ -336,15 +336,16 @@ SwFrameFormat *DocumentLayoutManager::CopyLayoutFormat( // 2) anchored in a header/footer // 3) anchored (to paragraph?) bool bMayNotCopy = false; + const auto pCAnchor = rNewAnchor.GetContentAnchor(); + bool bInHeaderFooter = pCAnchor && m_rDoc.IsInHeaderFooter(pCAnchor->nNode); if(bDraw) { - const auto pCAnchor = rNewAnchor.GetContentAnchor(); bool bCheckControlLayer = false; rSource.CallSwClientNotify(sw::CheckDrawFrameFormatLayerHint(&bCheckControlLayer)); bMayNotCopy = bCheckControlLayer && ((RndStdIds::FLY_AT_PARA == rNewAnchor.GetAnchorId()) || (RndStdIds::FLY_AT_FLY == rNewAnchor.GetAnchorId()) || (RndStdIds::FLY_AT_CHAR == rNewAnchor.GetAnchorId())) && - pCAnchor && m_rDoc.IsInHeaderFooter(pCAnchor->nNode); + bInHeaderFooter; } // just return if we can't copy this @@ -395,7 +396,7 @@ SwFrameFormat *DocumentLayoutManager::CopyLayoutFormat( if( !m_rDoc.IsCopyIsMove() || &m_rDoc != pSrcDoc ) { - if( m_rDoc.IsInReading() || m_rDoc.IsInMailMerge() ) + if( (m_rDoc.IsInReading() && !bInHeaderFooter) || m_rDoc.IsInMailMerge() ) pDest->SetName( OUString() ); else { @@ -407,7 +408,7 @@ SwFrameFormat *DocumentLayoutManager::CopyLayoutFormat( if( m_rDoc.FindFlyByName( sOld, nNdTyp ) ) // found one switch( nNdTyp ) { - case SwNodeType::Grf: sOld = m_rDoc.GetUniqueGrfName(); break; + case SwNodeType::Grf: sOld = m_rDoc.GetUniqueGrfName(sOld); break; case SwNodeType::Ole: sOld = m_rDoc.GetUniqueOLEName(); break; default: sOld = m_rDoc.GetUniqueFrameName(); break; } diff --git a/sw/source/core/doc/doclay.cxx b/sw/source/core/doc/doclay.cxx index 7cf52cbb3ef7..b3275040a519 100644 --- a/sw/source/core/doc/doclay.cxx +++ b/sw/source/core/doc/doclay.cxx @@ -1323,7 +1323,7 @@ namespace } } -static OUString lcl_GetUniqueFlyName(const SwDoc& rDoc, TranslateId pDefStrId, sal_uInt16 eType) +static OUString lcl_GetUniqueFlyName(const SwDoc& rDoc, TranslateId pDefStrId, sal_uInt16 eType, std::u16string_view rPrefix = std::u16string_view(), SwNodeType nNdTyp = SwNodeType::NONE) { assert(eType >= RES_FMT_BEGIN && eType < RES_FMT_END); if (rDoc.IsInMailMerge()) @@ -1334,6 +1334,26 @@ static OUString lcl_GetUniqueFlyName(const SwDoc& rDoc, TranslateId pDefStrId, s return newName; } + if (!rPrefix.empty()) + { + // Generate a name that makes it possible to know this is a copy of which original name, + // e.g. 'Picture 1 Copy 1'. + assert(nNdTyp != SwNodeType::NONE); + sal_Int32 nCnt = 1; + OUString aPrefix = SwResId(STR_MARK_COPY).replaceFirst("%1", rPrefix); + OUString aTmp; + while(nCnt < SAL_MAX_INT32) + { + aTmp = aPrefix + OUString::number(nCnt); + ++nCnt; + if (!rDoc.FindFlyByName(aTmp, nNdTyp)) + { + break; + } + } + return aTmp; + } + OUString aName(SwResId(pDefStrId)); sal_Int32 nNmLen = aName.getLength(); @@ -1363,9 +1383,9 @@ static OUString lcl_GetUniqueFlyName(const SwDoc& rDoc, TranslateId pDefStrId, s return aName + OUString::number(nNum); } -OUString SwDoc::GetUniqueGrfName() const +OUString SwDoc::GetUniqueGrfName(std::u16string_view rPrefix) const { - return lcl_GetUniqueFlyName(*this, STR_GRAPHIC_DEFNAME, RES_FLYFRMFMT); + return lcl_GetUniqueFlyName(*this, STR_GRAPHIC_DEFNAME, RES_FLYFRMFMT, rPrefix, SwNodeType::Grf); } OUString SwDoc::GetUniqueOLEName() const commit 1515a03e8115267ad4c65f9a80eaa69827b5ef7c Author: Miklos Vajna <[email protected]> AuthorDate: Thu Oct 13 13:53:33 2022 +0200 Commit: Mike Kaganski <[email protected]> CommitDate: Tue Dec 6 06:21:03 2022 +0000 sw: improve duplicated bookmarks in copied header/footer text Bookmarks in copied text were renamed in a way that made it hard to differentiate between the original bookmark and the copy, e.g. "Bookmark 1" was renamed to "Bookmark 11". Bookmarks are supposed to have a unique name, so renaming makes sense, and it's probably better to do that compared to what commit 3ec224dcb15e0e663ba85077b8ea0e632f8f03f8 (DOCX import: avoid duplicated bookmarks in copied header/footer text, 2022-09-08) did to just omit them during copy. That solved the duplicated bookmarks, but if one had bookmarks around images to find them, now it doesn't work to find all such images. Fix the problem by going back to copying bookmarks, but copy "Bookmark 1" as "Bookmark 1 Copy 1" (and "Bookmark 1 Copy 2", etc), so API users can identify the original and the copied bookmarks. A similar problem is there for images as well, but that's not yet addressed here. (cherry picked from commit 41403fbff8140ad0ca7cf8f81d37cddcfbd19197) Conflicts: sw/qa/core/doc/doc.cxx sw/source/core/doc/docdesc.cxx sw/source/core/doc/docfmt.cxx Change-Id: Ic0689b05f790a99ff06523ff4836b1dd412af896 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/143669 Tested-by: Jenkins Reviewed-by: Mike Kaganski <[email protected]> diff --git a/sw/inc/IDocumentContentOperations.hxx b/sw/inc/IDocumentContentOperations.hxx index 7b88097cc780..bac97d685927 100644 --- a/sw/inc/IDocumentContentOperations.hxx +++ b/sw/inc/IDocumentContentOperations.hxx @@ -75,12 +75,11 @@ enum class SwCopyFlags CopyAll = (1<<0), ///< copy break attributes even when source is single node CheckPosInFly = (1<<1), ///< check if target position is in fly anchored at source range IsMoveToFly = (1<<2), ///< MakeFlyAndMove - SkipBookmarks = (1<<3), ///< skip bookmarks, but not other kind of marks // TODO: mbCopyIsMove? mbIsRedlineMove? }; namespace o3tl { - template<> struct typed_flags<SwCopyFlags> : is_typed_flags<SwCopyFlags, 0x0f> {}; + template<> struct typed_flags<SwCopyFlags> : is_typed_flags<SwCopyFlags, 0x07> {}; } enum class SwDeleteFlags diff --git a/sw/inc/strings.hrc b/sw/inc/strings.hrc index 130004c6f338..a05f211eac85 100644 --- a/sw/inc/strings.hrc +++ b/sw/inc/strings.hrc @@ -1437,6 +1437,8 @@ // in order to change %PRODUCTNAME at runtime is expensive, so limit doing that as much as possible. #define STR_A11Y_DESC_AUTO NC_("insertcaption|extended_tip|auto", "Opens the Caption dialog. It has the same information as the dialog you get by menu %PRODUCTNAME Writer - AutoCaption in the Options dialog box.") +#define STR_MARK_COPY NC_("STR_MARK_COPY", "%1 Copy ") + #endif /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/sw/qa/core/doc/data/copy-flag-skip-bookmarks.docx b/sw/qa/core/doc/data/copy-bookmarks.docx similarity index 100% rename from sw/qa/core/doc/data/copy-flag-skip-bookmarks.docx rename to sw/qa/core/doc/data/copy-bookmarks.docx diff --git a/sw/qa/core/doc/doc.cxx b/sw/qa/core/doc/doc.cxx index dcad2542c582..f9d475216d2b 100644 --- a/sw/qa/core/doc/doc.cxx +++ b/sw/qa/core/doc/doc.cxx @@ -371,19 +371,28 @@ CPPUNIT_TEST_FIXTURE(SwCoreDocTest, testBookmarkDeleteListeners) xBookmark->dispose(); } -CPPUNIT_TEST_FIXTURE(SwCoreDocTest, testCopyFlagSkipBookmarks) +CPPUNIT_TEST_FIXTURE(SwCoreDocTest, testCopyBookmarks) { // Given a document with a bookmark in a header that is linked later: - SwDoc* pDoc = createSwDoc(DATA_DIRECTORY, "copy-flag-skip-bookmarks.docx"); + SwDoc* pDoc = createSwDoc(DATA_DIRECTORY, "copy-bookmarks.docx"); - // When checking the # of bookmarks in the resulting doc model: - sal_Int32 nActual = pDoc->getIDocumentMarkAccess()->getAllMarksCount(); + // When checking the # of non-copy bookmarks in the resulting doc model: + sal_Int32 nActual = 0; + for (auto it = pDoc->getIDocumentMarkAccess()->getBookmarksBegin(); + it != pDoc->getIDocumentMarkAccess()->getBookmarksEnd(); ++it) + { + if ((*it)->GetName().indexOf("Copy") == -1) + { + ++nActual; + } + } + + // Then make sure we have a single non-copy bookmark, with no duplications: - // Then make sure we have a single bookmark, with no duplications: // Without the accompanying fix in place, this test would have failed with: // - Expected: 1 // - Actual : 2 - // i.e. the 2nd header had a duplicated bookmark. + // i.e. the 2nd header had a duplicated bookmark without "Copy" in its name. CPPUNIT_ASSERT_EQUAL(static_cast<sal_Int32>(1), nActual); } diff --git a/sw/source/core/doc/DocumentContentOperationsManager.cxx b/sw/source/core/doc/DocumentContentOperationsManager.cxx index 87094c87b4e7..962cea631e5f 100644 --- a/sw/source/core/doc/DocumentContentOperationsManager.cxx +++ b/sw/source/core/doc/DocumentContentOperationsManager.cxx @@ -230,7 +230,7 @@ namespace namespace sw { // TODO: use SaveBookmark (from DelBookmarks) - void CopyBookmarks(const SwPaM& rPam, const SwPosition& rCpyPam, SwCopyFlags eFlags) + void CopyBookmarks(const SwPaM& rPam, const SwPosition& rCpyPam) { const SwDoc& rSrcDoc = rPam.GetDoc(); SwDoc& rDestDoc = rCpyPam.GetDoc(); @@ -281,7 +281,6 @@ namespace sw // We have to count the "non-copied" nodes... SwNodeOffset nDelCount; SwNodeIndex aCorrIdx(InitDelCount(rPam, nDelCount)); - auto bSkipBookmarks = static_cast<bool>(eFlags & SwCopyFlags::SkipBookmarks); for(const sw::mark::IMark* const pMark : vMarksToCopy) { SwPaM aTmpPam(*pCpyStt); @@ -294,17 +293,10 @@ namespace sw lcl_SetCpyPos(pMark->GetOtherMarkPos(), rStt, *pCpyStt, *aTmpPam.GetMark(), nDelCount); } - IDocumentMarkAccess::MarkType eType = IDocumentMarkAccess::GetType(*pMark); - if (bSkipBookmarks && eType == IDocumentMarkAccess::MarkType::BOOKMARK) - { - // It was requested to skip bookmarks while copying. Do that, but continue to copy - // other kind of marks, like fieldmarks. - continue; - } ::sw::mark::IMark* const pNewMark = rDestDoc.getIDocumentMarkAccess()->makeMark( aTmpPam, pMark->GetName(), - eType, + IDocumentMarkAccess::GetType(*pMark), ::sw::mark::InsertMode::CopyText); // Explicitly try to get exactly the same name as in the source // because NavigatorReminders, DdeBookmarks etc. ignore the proposed name @@ -3698,7 +3690,7 @@ void DocumentContentOperationsManager::CopyWithFlyInFly( targetPos = pCopiedPaM->second; } - sw::CopyBookmarks(pCopiedPaM ? pCopiedPaM->first : aRgTmp, targetPos, flags); + sw::CopyBookmarks(pCopiedPaM ? pCopiedPaM->first : aRgTmp, targetPos); } if (rRg.aStart != rRg.aEnd) @@ -5345,7 +5337,7 @@ bool DocumentContentOperationsManager::CopyImplImpl(SwPaM& rPam, SwPosition& rPo // Also copy all bookmarks if( bCopyBookmarks && m_rDoc.getIDocumentMarkAccess()->getAllMarksCount() ) { - sw::CopyBookmarks(rPam, *pCopyPam->Start(), SwCopyFlags::Default); + sw::CopyBookmarks(rPam, *pCopyPam->Start()); } if( RedlineFlags::DeleteRedlines & eOld ) diff --git a/sw/source/core/doc/docbm.cxx b/sw/source/core/doc/docbm.cxx index 629cb4ccf633..d2ad4e111607 100644 --- a/sw/source/core/doc/docbm.cxx +++ b/sw/source/core/doc/docbm.cxx @@ -48,6 +48,7 @@ #include <libxml/xmlstring.h> #include <libxml/xmlwriter.h> #include <comphelper/lok.hxx> +#include <strings.hrc> constexpr OUStringLiteral S_ANNOTATION_BOOKMARK = u"____"; @@ -1732,9 +1733,10 @@ namespace sw::mark sal_Int32 nCnt = 1; MarkBasenameMapUniqueOffset_t::const_iterator aIter = m_aMarkBasenameMapUniqueOffset.find(rName); if(aIter != m_aMarkBasenameMapUniqueOffset.end()) nCnt = aIter->second; + OUString aPrefix = SwResId(STR_MARK_COPY).replaceFirst("%1", rName); while(nCnt < SAL_MAX_INT32) { - sTmp = rName + OUString::number(nCnt); + sTmp = aPrefix + OUString::number(nCnt); nCnt++; if (lcl_FindMarkByName(sTmp, m_vAllMarks.begin(), m_vAllMarks.end()) == m_vAllMarks.end()) { diff --git a/sw/source/core/doc/docdesc.cxx b/sw/source/core/doc/docdesc.cxx index d190633e4175..1ba9d86b480f 100644 --- a/sw/source/core/doc/docdesc.cxx +++ b/sw/source/core/doc/docdesc.cxx @@ -305,7 +305,7 @@ void SwDoc::CopyMasterHeader(const SwPageDesc &rChged, const SwFormatHeader &rHe GetDocumentContentOperationsManager().CopyFlyInFlyImpl(aRange, nullptr, aTmp); SwPaM const source(aRange.aStart, aRange.aEnd); SwPosition dest(aTmp); - sw::CopyBookmarks(source, dest, SwCopyFlags::Default); + sw::CopyBookmarks(source, dest); pFormat->SetFormatAttr( SwFormatContent( pSttNd ) ); rDescFrameFormat.SetFormatAttr( SwFormatHeader( pFormat ) ); } @@ -385,7 +385,7 @@ void SwDoc::CopyMasterFooter(const SwPageDesc &rChged, const SwFormatFooter &rFo GetDocumentContentOperationsManager().CopyFlyInFlyImpl(aRange, nullptr, aTmp); SwPaM const source(aRange.aStart, aRange.aEnd); SwPosition dest(aTmp); - sw::CopyBookmarks(source, dest, SwCopyFlags::Default); + sw::CopyBookmarks(source, dest); pFormat->SetFormatAttr( SwFormatContent( pSttNd ) ); rDescFrameFormat.SetFormatAttr( SwFormatFooter( pFormat ) ); } diff --git a/sw/source/core/doc/docfmt.cxx b/sw/source/core/doc/docfmt.cxx index 6846be79d347..dbf8fefeda4a 100644 --- a/sw/source/core/doc/docfmt.cxx +++ b/sw/source/core/doc/docfmt.cxx @@ -1410,7 +1410,7 @@ void SwDoc::CopyPageDescHeaderFooterImpl( bool bCpyHeader, // TODO: investigate calling CopyWithFlyInFly? SwPaM const source(aRg.aStart, aRg.aEnd); SwPosition dest(aTmpIdx); - sw::CopyBookmarks(source, dest, SwCopyFlags::Default); + sw::CopyBookmarks(source, dest); pNewFormat->SetFormatAttr( SwFormatContent( pSttNd )); } else diff --git a/sw/source/core/inc/DocumentContentOperationsManager.hxx b/sw/source/core/inc/DocumentContentOperationsManager.hxx index 4c16cd881f92..ce6dc7788a86 100644 --- a/sw/source/core/inc/DocumentContentOperationsManager.hxx +++ b/sw/source/core/inc/DocumentContentOperationsManager.hxx @@ -181,7 +181,7 @@ private: }; -void CopyBookmarks(const SwPaM& rPam, const SwPosition& rTarget, SwCopyFlags eFlags); +void CopyBookmarks(const SwPaM& rPam, const SwPosition& rTarget); void CalcBreaks(std::vector<std::pair<SwNodeOffset, sal_Int32>> & rBreaks, SwPaM const & rPam, bool const isOnlyFieldmarks = false); diff --git a/sw/source/core/unocore/unotext.cxx b/sw/source/core/unocore/unotext.cxx index 95f3911d1689..04803e0b9c35 100644 --- a/sw/source/core/unocore/unotext.cxx +++ b/sw/source/core/unocore/unotext.cxx @@ -86,7 +86,6 @@ public: const CursorType m_eType; SwDoc * m_pDoc; bool m_bIsValid; - bool m_bCopySkipsBookmarks; Impl( SwXText & rThis, SwDoc *const pDoc, const CursorType eType) @@ -95,7 +94,6 @@ public: , m_eType(eType) , m_pDoc(pDoc) , m_bIsValid(nullptr != pDoc) - , m_bCopySkipsBookmarks(false) { } @@ -1108,18 +1106,9 @@ SwXText::getPropertySetInfo() } void SAL_CALL -SwXText::setPropertyValue(const OUString& aPropertyName, - const uno::Any& aValue) +SwXText::setPropertyValue(const OUString& /*aPropertyName*/, + const uno::Any& /*aValue*/) { - if (aPropertyName == "CopySkipsBookmarks") - { - bool bValue{}; - if (aValue >>= bValue) - { - m_pImpl->m_bCopySkipsBookmarks = bValue; - } - return; - } throw lang::IllegalArgumentException(); } @@ -2399,12 +2388,7 @@ SwXText::copyText( // Explicitly request copy text mode, so // sw::DocumentContentOperationsManager::CopyFlyInFlyImpl() will copy shapes anchored to // us, even if we have only a single paragraph. - SwCopyFlags eFlags = SwCopyFlags::CheckPosInFly; - if (m_pImpl->m_bCopySkipsBookmarks) - { - eFlags |= SwCopyFlags::SkipBookmarks; - } - m_pImpl->m_pDoc->getIDocumentContentOperations().CopyRange(temp, rPos, eFlags); + m_pImpl->m_pDoc->getIDocumentContentOperations().CopyRange(temp, rPos, SwCopyFlags::CheckPosInFly); } if (!pFirstNode) { // the node at rPos was split; get rid of the first empty one so diff --git a/writerfilter/source/dmapper/PropertyMap.cxx b/writerfilter/source/dmapper/PropertyMap.cxx index 6f37269f73a6..1ae28759db33 100644 --- a/writerfilter/source/dmapper/PropertyMap.cxx +++ b/writerfilter/source/dmapper/PropertyMap.cxx @@ -873,18 +873,7 @@ void SectionPropertyMap::CopyHeaderFooterTextProperty( const uno::Reference< bea if ( xPrevStyle.is() ) xPrevTxt.set( xPrevStyle->getPropertyValue( sName ), uno::UNO_QUERY_THROW ); - uno::Reference<beans::XPropertySet> xTxtProps(xTxt, uno::UNO_QUERY); - if (xTxtProps.is()) - { - // Skip copying bookmarks, so the number of bookmarks in the source document and in the - // resulting doc model match. - xTxtProps->setPropertyValue("CopySkipsBookmarks", uno::Any(true)); - } xTxt->copyText( xPrevTxt ); - if (xTxtProps.is()) - { - xTxtProps->setPropertyValue("CopySkipsBookmarks", uno::Any(false)); - } } catch ( const uno::Exception& ) { commit 4b7ee51e5a12acbbf8fca086a4470d244a82c962 Author: Miklos Vajna <[email protected]> AuthorDate: Thu Sep 8 16:41:46 2022 +0200 Commit: Mike Kaganski <[email protected]> CommitDate: Tue Dec 6 06:20:56 2022 +0000 DOCX import: avoid duplicated bookmarks in copied header/footer text In case a DOCX document has 2 sections and the 2nd section has a linked header, writerfilter/ copies the header text from the 1st page style to the 2nd page style. This leads to duplicated bookmarks in the doc model, which causes an unexpected increase in bookmark count on export. This went wrong with 8885bdf2f564bb7b56181c50baf39ff99d551ec9 (tdf#128106 sw: copy bookmarks at start if whole node is copied, 2021-08-27), previously at least bookmarks at the start of the header were not duplicated, though that was also inconsistent, since other bookmarks were still duplicated. Interestingly the DOC import doesn't have this problem, because it first copies header text and only later imports the bookmarks. Fix the problem by introducing a new SwCopyFlags::SkipBookmarks flag, add UNO API to set this from writerfilter/ code and skip copying bookmarks (but not other marks) in sw::CopyBookmarks() accordingly. Copying other marks is still needed, because e.g. fieldmarks have a mark and a field, and it would be inconsistent to copy the field, but not the mark. Note that the text is still duplicated in header/footer, linked header/footer is a missing feature on the Writer side. (cherry picked from commit 3ec224dcb15e0e663ba85077b8ea0e632f8f03f8) Conflicts: sw/qa/core/doc/doc.cxx sw/source/core/doc/docdesc.cxx sw/source/core/doc/docfmt.cxx Change-Id: I40e18f231ef2c0d91ae9582621684ef5b6284904 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/139722 Tested-by: Jenkins Reviewed-by: Mike Kaganski <[email protected]> diff --git a/sw/inc/IDocumentContentOperations.hxx b/sw/inc/IDocumentContentOperations.hxx index bac97d685927..7b88097cc780 100644 --- a/sw/inc/IDocumentContentOperations.hxx +++ b/sw/inc/IDocumentContentOperations.hxx @@ -75,11 +75,12 @@ enum class SwCopyFlags CopyAll = (1<<0), ///< copy break attributes even when source is single node CheckPosInFly = (1<<1), ///< check if target position is in fly anchored at source range IsMoveToFly = (1<<2), ///< MakeFlyAndMove + SkipBookmarks = (1<<3), ///< skip bookmarks, but not other kind of marks // TODO: mbCopyIsMove? mbIsRedlineMove? }; namespace o3tl { - template<> struct typed_flags<SwCopyFlags> : is_typed_flags<SwCopyFlags, 0x07> {}; + template<> struct typed_flags<SwCopyFlags> : is_typed_flags<SwCopyFlags, 0x0f> {}; } enum class SwDeleteFlags diff --git a/sw/qa/core/doc/data/copy-flag-skip-bookmarks.docx b/sw/qa/core/doc/data/copy-flag-skip-bookmarks.docx new file mode 100644 index 000000000000..3fb27b430a17 Binary files /dev/null and b/sw/qa/core/doc/data/copy-flag-skip-bookmarks.docx differ diff --git a/sw/qa/core/doc/doc.cxx b/sw/qa/core/doc/doc.cxx index a9db78ba4871..dcad2542c582 100644 --- a/sw/qa/core/doc/doc.cxx +++ b/sw/qa/core/doc/doc.cxx @@ -371,6 +371,22 @@ CPPUNIT_TEST_FIXTURE(SwCoreDocTest, testBookmarkDeleteListeners) xBookmark->dispose(); } +CPPUNIT_TEST_FIXTURE(SwCoreDocTest, testCopyFlagSkipBookmarks) +{ + // Given a document with a bookmark in a header that is linked later: + SwDoc* pDoc = createSwDoc(DATA_DIRECTORY, "copy-flag-skip-bookmarks.docx"); + + // When checking the # of bookmarks in the resulting doc model: + sal_Int32 nActual = pDoc->getIDocumentMarkAccess()->getAllMarksCount(); + + // Then make sure we have a single bookmark, with no duplications: + // Without the accompanying fix in place, this test would have failed with: + // - Expected: 1 + // - Actual : 2 + // i.e. the 2nd header had a duplicated bookmark. + CPPUNIT_ASSERT_EQUAL(static_cast<sal_Int32>(1), nActual); +} + CPPUNIT_PLUGIN_IMPLEMENT(); /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/sw/source/core/doc/DocumentContentOperationsManager.cxx b/sw/source/core/doc/DocumentContentOperationsManager.cxx index 962cea631e5f..87094c87b4e7 100644 --- a/sw/source/core/doc/DocumentContentOperationsManager.cxx +++ b/sw/source/core/doc/DocumentContentOperationsManager.cxx @@ -230,7 +230,7 @@ namespace namespace sw { // TODO: use SaveBookmark (from DelBookmarks) - void CopyBookmarks(const SwPaM& rPam, const SwPosition& rCpyPam) + void CopyBookmarks(const SwPaM& rPam, const SwPosition& rCpyPam, SwCopyFlags eFlags) { const SwDoc& rSrcDoc = rPam.GetDoc(); SwDoc& rDestDoc = rCpyPam.GetDoc(); @@ -281,6 +281,7 @@ namespace sw // We have to count the "non-copied" nodes... SwNodeOffset nDelCount; SwNodeIndex aCorrIdx(InitDelCount(rPam, nDelCount)); + auto bSkipBookmarks = static_cast<bool>(eFlags & SwCopyFlags::SkipBookmarks); for(const sw::mark::IMark* const pMark : vMarksToCopy) { SwPaM aTmpPam(*pCpyStt); @@ -293,10 +294,17 @@ namespace sw lcl_SetCpyPos(pMark->GetOtherMarkPos(), rStt, *pCpyStt, *aTmpPam.GetMark(), nDelCount); } + IDocumentMarkAccess::MarkType eType = IDocumentMarkAccess::GetType(*pMark); + if (bSkipBookmarks && eType == IDocumentMarkAccess::MarkType::BOOKMARK) + { + // It was requested to skip bookmarks while copying. Do that, but continue to copy + // other kind of marks, like fieldmarks. + continue; + } ::sw::mark::IMark* const pNewMark = rDestDoc.getIDocumentMarkAccess()->makeMark( aTmpPam, pMark->GetName(), - IDocumentMarkAccess::GetType(*pMark), + eType, ::sw::mark::InsertMode::CopyText); // Explicitly try to get exactly the same name as in the source // because NavigatorReminders, DdeBookmarks etc. ignore the proposed name @@ -3690,7 +3698,7 @@ void DocumentContentOperationsManager::CopyWithFlyInFly( targetPos = pCopiedPaM->second; } - sw::CopyBookmarks(pCopiedPaM ? pCopiedPaM->first : aRgTmp, targetPos); + sw::CopyBookmarks(pCopiedPaM ? pCopiedPaM->first : aRgTmp, targetPos, flags); } if (rRg.aStart != rRg.aEnd) @@ -5337,7 +5345,7 @@ bool DocumentContentOperationsManager::CopyImplImpl(SwPaM& rPam, SwPosition& rPo // Also copy all bookmarks if( bCopyBookmarks && m_rDoc.getIDocumentMarkAccess()->getAllMarksCount() ) { - sw::CopyBookmarks(rPam, *pCopyPam->Start()); + sw::CopyBookmarks(rPam, *pCopyPam->Start(), SwCopyFlags::Default); } if( RedlineFlags::DeleteRedlines & eOld ) diff --git a/sw/source/core/doc/docdesc.cxx b/sw/source/core/doc/docdesc.cxx index 1ba9d86b480f..d190633e4175 100644 --- a/sw/source/core/doc/docdesc.cxx +++ b/sw/source/core/doc/docdesc.cxx @@ -305,7 +305,7 @@ void SwDoc::CopyMasterHeader(const SwPageDesc &rChged, const SwFormatHeader &rHe GetDocumentContentOperationsManager().CopyFlyInFlyImpl(aRange, nullptr, aTmp); SwPaM const source(aRange.aStart, aRange.aEnd); SwPosition dest(aTmp); - sw::CopyBookmarks(source, dest); + sw::CopyBookmarks(source, dest, SwCopyFlags::Default); pFormat->SetFormatAttr( SwFormatContent( pSttNd ) ); rDescFrameFormat.SetFormatAttr( SwFormatHeader( pFormat ) ); } @@ -385,7 +385,7 @@ void SwDoc::CopyMasterFooter(const SwPageDesc &rChged, const SwFormatFooter &rFo GetDocumentContentOperationsManager().CopyFlyInFlyImpl(aRange, nullptr, aTmp); SwPaM const source(aRange.aStart, aRange.aEnd); SwPosition dest(aTmp); - sw::CopyBookmarks(source, dest); + sw::CopyBookmarks(source, dest, SwCopyFlags::Default); pFormat->SetFormatAttr( SwFormatContent( pSttNd ) ); rDescFrameFormat.SetFormatAttr( SwFormatFooter( pFormat ) ); } diff --git a/sw/source/core/doc/docfmt.cxx b/sw/source/core/doc/docfmt.cxx index dbf8fefeda4a..6846be79d347 100644 --- a/sw/source/core/doc/docfmt.cxx +++ b/sw/source/core/doc/docfmt.cxx @@ -1410,7 +1410,7 @@ void SwDoc::CopyPageDescHeaderFooterImpl( bool bCpyHeader, // TODO: investigate calling CopyWithFlyInFly? SwPaM const source(aRg.aStart, aRg.aEnd); SwPosition dest(aTmpIdx); - sw::CopyBookmarks(source, dest); + sw::CopyBookmarks(source, dest, SwCopyFlags::Default); pNewFormat->SetFormatAttr( SwFormatContent( pSttNd )); } else diff --git a/sw/source/core/inc/DocumentContentOperationsManager.hxx b/sw/source/core/inc/DocumentContentOperationsManager.hxx index ce6dc7788a86..4c16cd881f92 100644 --- a/sw/source/core/inc/DocumentContentOperationsManager.hxx +++ b/sw/source/core/inc/DocumentContentOperationsManager.hxx @@ -181,7 +181,7 @@ private: }; -void CopyBookmarks(const SwPaM& rPam, const SwPosition& rTarget); +void CopyBookmarks(const SwPaM& rPam, const SwPosition& rTarget, SwCopyFlags eFlags); void CalcBreaks(std::vector<std::pair<SwNodeOffset, sal_Int32>> & rBreaks, SwPaM const & rPam, bool const isOnlyFieldmarks = false); diff --git a/sw/source/core/unocore/unotext.cxx b/sw/source/core/unocore/unotext.cxx index 04803e0b9c35..95f3911d1689 100644 --- a/sw/source/core/unocore/unotext.cxx +++ b/sw/source/core/unocore/unotext.cxx @@ -86,6 +86,7 @@ public: const CursorType m_eType; SwDoc * m_pDoc; bool m_bIsValid; + bool m_bCopySkipsBookmarks; Impl( SwXText & rThis, SwDoc *const pDoc, const CursorType eType) @@ -94,6 +95,7 @@ public: , m_eType(eType) , m_pDoc(pDoc) , m_bIsValid(nullptr != pDoc) + , m_bCopySkipsBookmarks(false) { } @@ -1106,9 +1108,18 @@ SwXText::getPropertySetInfo() } void SAL_CALL -SwXText::setPropertyValue(const OUString& /*aPropertyName*/, - const uno::Any& /*aValue*/) +SwXText::setPropertyValue(const OUString& aPropertyName, + const uno::Any& aValue) { + if (aPropertyName == "CopySkipsBookmarks") + { + bool bValue{}; + if (aValue >>= bValue) + { + m_pImpl->m_bCopySkipsBookmarks = bValue; + } + return; + } throw lang::IllegalArgumentException(); } @@ -2388,7 +2399,12 @@ SwXText::copyText( // Explicitly request copy text mode, so // sw::DocumentContentOperationsManager::CopyFlyInFlyImpl() will copy shapes anchored to // us, even if we have only a single paragraph. - m_pImpl->m_pDoc->getIDocumentContentOperations().CopyRange(temp, rPos, SwCopyFlags::CheckPosInFly); + SwCopyFlags eFlags = SwCopyFlags::CheckPosInFly; + if (m_pImpl->m_bCopySkipsBookmarks) + { + eFlags |= SwCopyFlags::SkipBookmarks; + } + m_pImpl->m_pDoc->getIDocumentContentOperations().CopyRange(temp, rPos, eFlags); } if (!pFirstNode) { // the node at rPos was split; get rid of the first empty one so diff --git a/writerfilter/source/dmapper/PropertyMap.cxx b/writerfilter/source/dmapper/PropertyMap.cxx index 1ae28759db33..6f37269f73a6 100644 --- a/writerfilter/source/dmapper/PropertyMap.cxx +++ b/writerfilter/source/dmapper/PropertyMap.cxx @@ -873,7 +873,18 @@ void SectionPropertyMap::CopyHeaderFooterTextProperty( const uno::Reference< bea if ( xPrevStyle.is() ) xPrevTxt.set( xPrevStyle->getPropertyValue( sName ), uno::UNO_QUERY_THROW ); + uno::Reference<beans::XPropertySet> xTxtProps(xTxt, uno::UNO_QUERY); + if (xTxtProps.is()) + { + // Skip copying bookmarks, so the number of bookmarks in the source document and in the + // resulting doc model match. + xTxtProps->setPropertyValue("CopySkipsBookmarks", uno::Any(true)); + } xTxt->copyText( xPrevTxt ); + if (xTxtProps.is()) + { + xTxtProps->setPropertyValue("CopySkipsBookmarks", uno::Any(false)); + } } catch ( const uno::Exception& ) {
