sw/qa/extras/unowriter/data/bookmark_1.html | 4 ++
sw/qa/extras/unowriter/unowriter.cxx | 37 +++++++++++++++++++++
sw/source/core/doc/docbm.cxx | 48 ++++++++++++++++++++++------
sw/source/core/inc/MarkManager.hxx | 5 ++
sw/source/filter/basflt/shellio.cxx | 14 ++++----
5 files changed, 92 insertions(+), 16 deletions(-)
New commits:
commit 8015a6c9e6b2e67184fdea6c897ff8036a488ca5
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Thu May 15 17:45:55 2025 +0500
Commit: Mike Kaganski <mike.kagan...@collabora.com>
CommitDate: Fri May 16 13:00:15 2025 +0200
Related: tdf#165918 Avoid renaming pre-existing bookmarks
Thanks Michael Stahl, for pointing this:
> what happens when you first insert a bookmark, then insert a file that
> contains a bookmark with the same name at a place in the document before
> the pre-existing one - the SwHistoryBookmark uses the name of the bookmark
> to find it, and this loop would rename the pre-exsiting bookmark, so on
> undo of insert bookmark it can't be found?
( https://gerrit.libreoffice.org/c/core/+/185340/comment/274a3fba_aa363535/
)
This change avoids this problem by preparing a set of already existing names
at the start of the performance mode, and only checking the marks that were
added while in the performance mode.
Change-Id: Id4dc1809871fd4a998396d091f2f920ecd71b7d9
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/185363
Tested-by: Jenkins
Reviewed-by: Michael Stahl <michael.st...@allotropia.de>
Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com>
diff --git a/sw/qa/extras/unowriter/data/bookmark_1.html
b/sw/qa/extras/unowriter/data/bookmark_1.html
new file mode 100644
index 000000000000..225817dc6729
--- /dev/null
+++ b/sw/qa/extras/unowriter/data/bookmark_1.html
@@ -0,0 +1,4 @@
+<html>
+<body><a name="Bookmark 1">abc</a>
+</body>
+</html>
\ No newline at end of file
diff --git a/sw/qa/extras/unowriter/unowriter.cxx
b/sw/qa/extras/unowriter/unowriter.cxx
index 0e74fb83cdde..32dd4ead2097 100644
--- a/sw/qa/extras/unowriter/unowriter.cxx
+++ b/sw/qa/extras/unowriter/unowriter.cxx
@@ -16,6 +16,7 @@
#include <com/sun/star/frame/XDispatchProviderInterception.hpp>
#include <com/sun/star/frame/XDispatchProviderInterceptor.hpp>
#include <com/sun/star/table/XCellRange.hpp>
+#include <com/sun/star/text/ControlCharacter.hpp>
#include <com/sun/star/text/TextContentAnchorType.hpp>
#include <com/sun/star/text/AutoTextContainer.hpp>
#include <com/sun/star/text/VertOrientation.hpp>
@@ -1560,6 +1561,42 @@ CPPUNIT_TEST_FIXTURE(SwUnoWriter, testTdf164921)
}
}
+CPPUNIT_TEST_FIXTURE(SwUnoWriter, testMarkWithPreexistingNameInsertion)
+{
+ createSwDoc();
+
+ // Add a second paragraph, and create a bookmark there, with a specific
name
+ auto xTextDocument = mxComponent.queryThrow<text::XTextDocument>();
+ auto xText = xTextDocument->getText();
+ xText->insertControlCharacter(xText->getEnd(),
text::ControlCharacter::PARAGRAPH_BREAK, false);
+
+ auto xFac = mxComponent.queryThrow<lang::XMultiServiceFactory>();
+ auto xMark = xFac->createInstance(u"com.sun.star.text.Bookmark"_ustr);
+ auto xNamed = xMark.queryThrow<container::XNamed>();
+ xNamed->setName(u"Bookmark 1"_ustr);
+ xText->insertTextContent(xText->getEnd(),
xMark.queryThrow<text::XTextContent>(), false);
+
+ // Insert the content of a file, which has a bookmark with the same name,
before existing one
+ dispatchCommand(
+ mxComponent, u".uno:InsertDoc"_ustr,
+ { comphelper::makePropertyValue(u"Name"_ustr,
createFileURL(u"bookmark_1.html")) });
+
+ // The pre-existing bookmark's name must not change
+ // Before the fix, this would fail with "Actual : Bookmark 1 Copy 1"
+ CPPUNIT_ASSERT_EQUAL(u"Bookmark 1"_ustr, xNamed->getName());
+
+ auto xSupplier = mxComponent.queryThrow<text::XBookmarksSupplier>();
+ auto xBookmarks = xSupplier->getBookmarks();
+ auto names = xBookmarks->getElementNames();
+ CPPUNIT_ASSERT_EQUAL(sal_Int32(2), names.getLength());
+ // names[1] is the pre-existing bookmark
+ CPPUNIT_ASSERT_EQUAL(u"Bookmark 1"_ustr, names[1]);
+ // names[0] is the bookmark coming from the inserted file
+ OUString rest;
+ CPPUNIT_ASSERT(names[0].startsWith("Bookmark 1", &rest));
+ CPPUNIT_ASSERT(!rest.isEmpty()); // should be " Copy 1"
+}
+
} // end of anonymous namespace
CPPUNIT_PLUGIN_IMPLEMENT();
diff --git a/sw/source/core/doc/docbm.cxx b/sw/source/core/doc/docbm.cxx
index 4a17c6e198db..7a136dcceaf0 100644
--- a/sw/source/core/doc/docbm.cxx
+++ b/sw/source/core/doc/docbm.cxx
@@ -500,19 +500,37 @@ namespace sw::mark
, m_pLastActiveFieldmark(nullptr)
{ }
- void MarkManager::disableUniqueNameChecks() { m_bCheckUniqueNames = false;
}
+ // In the mode where the names are not checked, we need to avoid a case
where there was a
+ // bookmark, and a file is inserted at an earlier point, with the
same-name bookmark, causing
+ // a rename of the pre-existing bookmark. m_aUsedNames and
m_vUncheckedNameMarks are used for
+ // that. m_aUsedNames is pre-populated with the existing names (at the
moment when the mode
+ // started); and m_vUncheckedNameMarks stores only the marks needing the
checks.
+
+ void MarkManager::disableUniqueNameChecks()
+ {
+ if (!m_bCheckUniqueNames)
+ return; // nested call
+ m_bCheckUniqueNames = false;
+ assert(m_aUsedNames.empty());
+
+ // Populate the pre-existing already deduplicated names
+ for (auto& pMark : m_vAllMarks)
+ m_aUsedNames.insert(pMark->GetName().toString());
+ }
void MarkManager::enableUniqueNameChecks()
{
if (m_bCheckUniqueNames)
return;
- // Make sure that all names are unique
- std::unordered_set<OUString> usedNames;
+ // Make sure that all previously unchecked names are unique
for (auto& pMark : m_vAllMarks)
{
- assert(pMark);
- pMark->SetName(getUniqueMarkName(pMark->GetName(),
[&usedNames](const SwMarkName& n)
- { return
usedNames.insert(n.toString()).second; }));
+ if (!m_vUncheckedNameMarks.contains(pMark))
+ continue; // mark was added and removed while in the
performance mode
+ pMark->SetName(getUniqueMarkName(pMark->GetName(), [this](const
SwMarkName& n)
+ { return
m_aUsedNames.insert(n.toString()).second; }));
}
+ m_aUsedNames.clear();
+ m_vUncheckedNameMarks.clear();
m_bCheckUniqueNames = true;
}
@@ -632,10 +650,19 @@ namespace sw::mark
pMark->Swap();
// for performance reasons, we trust UnoMarks to have a (generated)
unique name
- if (eType != IDocumentMarkAccess::MarkType::UNO_BOOKMARK &&
m_bCheckUniqueNames)
- pMark->SetName(getUniqueMarkName(
- pMark->GetName(), [this](const SwMarkName& n)
- { return lcl_FindMarkByName(n, m_vAllMarks) ==
m_vAllMarks.end(); }));
+ if (eType != IDocumentMarkAccess::MarkType::UNO_BOOKMARK)
+ {
+ if (m_bCheckUniqueNames)
+ {
+ pMark->SetName(getUniqueMarkName(
+ pMark->GetName(), [this](const SwMarkName& n)
+ { return lcl_FindMarkByName(n, m_vAllMarks) ==
m_vAllMarks.end(); }));
+ }
+ else
+ {
+ m_vUncheckedNameMarks.insert(pMark.get());
+ }
+ }
// insert any dummy chars before inserting into sorted vectors
pMark->InitDoc(m_rDoc, eMode, pSepPos);
@@ -1343,6 +1370,7 @@ namespace sw::mark
m_vFieldmarks.clear();
m_vBookmarks.clear();
m_vAnnotationMarks.clear();
+ m_vUncheckedNameMarks.clear();
for (const auto & p : m_vAllMarks)
delete p;
m_vAllMarks.clear();
diff --git a/sw/source/core/inc/MarkManager.hxx
b/sw/source/core/inc/MarkManager.hxx
index f4ee394d3f5b..729958192a02 100644
--- a/sw/source/core/inc/MarkManager.hxx
+++ b/sw/source/core/inc/MarkManager.hxx
@@ -153,6 +153,11 @@ namespace sw::mark {
// container for all marks, this container owns the objects it
points to
container_t m_vAllMarks;
+ // container for all marks with possibly duplicating names
(m_bCheckUniqueNames mode)
+ std::unordered_set<sw::mark::MarkBase*> m_vUncheckedNameMarks;
+ // container for deduplicating names (m_bCheckUniqueNames mode)
+ std::unordered_set<OUString> m_aUsedNames;
+
// additional container for bookmarks
std::vector<sw::mark::Bookmark*> m_vBookmarks;
// additional container for fieldmarks
diff --git a/sw/source/filter/basflt/shellio.cxx
b/sw/source/filter/basflt/shellio.cxx
index abb516422b88..107176ec142c 100644
--- a/sw/source/filter/basflt/shellio.cxx
+++ b/sw/source/filter/basflt/shellio.cxx
@@ -18,6 +18,7 @@
*/
#include <hintids.hxx>
+#include <comphelper/scopeguard.hxx>
#include <osl/diagnose.h>
#include <tools/date.hxx>
#include <tools/time.hxx>
@@ -201,13 +202,14 @@ ErrCodeMsg SwReader::Read( const Reader& rOptions )
mxDoc->getIDocumentRedlineAccess().SetRedlineFlags_intern( eOld );
- // Preformance mode: import all bookmarks names as defined in the
document
- mxDoc->getIDocumentMarkAccess()->disableUniqueNameChecks();
-
- nError = po->Read( *mxDoc, msBaseURL, *pPam, maFileName );
+ {
+ // Preformance mode: import all bookmarks names as defined in the
document
+ mxDoc->getIDocumentMarkAccess()->disableUniqueNameChecks();
+ comphelper::ScopeGuard perfModeGuard(
+ [this]() {
mxDoc->getIDocumentMarkAccess()->enableUniqueNameChecks(); });
- // End performance mode: now make sure that all names are unique
- mxDoc->getIDocumentMarkAccess()->enableUniqueNameChecks();
+ nError = po->Read(*mxDoc, msBaseURL, *pPam, maFileName);
+ }
// an ODF document may contain redline mode in settings.xml; save it!
ePostReadRedlineFlags =
mxDoc->getIDocumentRedlineAccess().GetRedlineFlags();
