sw: fix copying of bookmarks in CopyRange

If the copied range starts with a not fully selected paragraph, the
bookmarks that are copied will be created on the wrong paragraphs,
on the node after the correct one.

This also happens when hinding the redlines, and causes asserts from
attempting to create CrossRefBookmarks on table nodes on WW8 export of
fdo66302-1.odt and fdo66312-1.odt.

Change-Id: Id576be3e38a89527d967f02b39d9aabbf6368354
(cherry picked from commit c95ba3ef2613e9d5abd2f19ab2432c7bc1a40fe7)
Reviewed-on: https://gerrit.libreoffice.org/19104
Reviewed-by: Miklos Vajna <vmiklos@collabora.co.uk>
Tested-by: Miklos Vajna <vmiklos@collabora.co.uk>

3 files changed, 94 insertions, 8 deletions

diff --git a/sw/qa/extras/uiwriter/uiwriter.cxx b/sw/qa/extras/uiwriter/uiwriter.cxx
index 006cb469d9fd..364439b7ba56 100644
--- a/sw/qa/extras/uiwriter/uiwriter.cxx
+++ b/sw/qa/extras/uiwriter/uiwriter.cxx
@@ -67,6 +67,7 @@ public:
     //EDITING: undo search&replace corrupt text when searching backward
     void testReplaceBackward();
     void testRedlineFrame();
+    void testBookmarkCopy();
     void testFdo69893();
     void testFdo70807();
     void testImportRTF();
@@ -108,6 +109,7 @@ public:
     CPPUNIT_TEST(testReplaceForward);
     CPPUNIT_TEST(testReplaceBackward);
     CPPUNIT_TEST(testRedlineFrame);
+    CPPUNIT_TEST(testBookmarkCopy);
     CPPUNIT_TEST(testFdo69893);
     CPPUNIT_TEST(testFdo70807);
     CPPUNIT_TEST(testImportRTF);
@@ -221,6 +223,61 @@ void SwUiWriterTest::testRedlineFrame()
     CPPUNIT_ASSERT_EQUAL(sal_Int32(1), xDrawPage->getCount());
 }
 
+void SwUiWriterTest::testBookmarkCopy()
+{
+    SwDoc * pDoc(createDoc());
+
+    // add text and bookmark
+    IDocumentMarkAccess & rIDMA(*pDoc->getIDocumentMarkAccess());
+    IDocumentContentOperations & rIDCO(pDoc->getIDocumentContentOperations());
+    SwNodeIndex aIdx(pDoc->GetNodes().GetEndOfContent(), -1);
+    SwCursor aPaM(SwPosition(aIdx), nullptr, false);
+    rIDCO.InsertString(aPaM, OUString("foo"));
+    rIDCO.SplitNode(*aPaM.GetPoint(), false);
+    rIDCO.InsertString(aPaM, OUString("bar"));
+    aPaM.SetMark();
+    aPaM.MovePara(GetfnParaCurr(), GetfnParaStart());
+    rIDMA.makeMark(aPaM, OUString("Mark"), IDocumentMarkAccess::MarkType::BOOKMARK);
+    aPaM.Exchange();
+    aPaM.DeleteMark();
+    rIDCO.SplitNode(*aPaM.GetPoint(), false);
+    rIDCO.InsertString(aPaM, OUString("baz"));
+
+    // copy range
+    rIDCO.SplitNode(*aPaM.GetPoint(), false);
+    SwPosition target(*aPaM.GetPoint());
+    aPaM.Move(fnMoveBackward, fnGoContent);
+    aPaM.SetMark();
+    aPaM.SttEndDoc(true/*start*/);
+    aPaM.Move(fnMoveForward, fnGoContent); // partially select 1st para
+
+    rIDCO.CopyRange(aPaM, target, /*bCopyAll=*/false, /*bCheckPos=*/true);
+
+    // check bookmark was copied to correct position
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(2), rIDMA.getBookmarksCount());
+    for (auto it(rIDMA.getBookmarksBegin()); it != rIDMA.getBookmarksEnd(); ++it)
+    {
+        OUString markText(SwPaM((*it)->GetMarkPos(), (*it)->GetOtherMarkPos()).GetText());
+        CPPUNIT_ASSERT_EQUAL(OUString("bar"), markText);
+    }
+
+    // copy 2nd time, such that bCanMoveBack is false in CopyImpl
+    SwPaM aCopyPaM(*aPaM.GetMark(), *aPaM.GetPoint());
+    aPaM.SttEndDoc(true/*start*/);
+    rIDCO.SplitNode(*aPaM.GetPoint(), false);
+    aPaM.SttEndDoc(true/*start*/);
+
+    rIDCO.CopyRange(aCopyPaM, *aPaM.GetPoint(), /*bCopyAll=*/false, /*bCheckPos=*/true);
+
+    // check bookmark was copied to correct position
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(3), rIDMA.getBookmarksCount());
+    for (auto it(rIDMA.getBookmarksBegin()); it != rIDMA.getBookmarksEnd(); ++it)
+    {
+        OUString markText(SwPaM((*it)->GetMarkPos(), (*it)->GetOtherMarkPos()).GetText());
+        CPPUNIT_ASSERT_EQUAL(OUString("bar"), markText);
+    }
+}
+
 void SwUiWriterTest::testFdo75110()
 {
     SwDoc* pDoc = createDoc("fdo75110.odt");
diff --git a/sw/source/core/doc/DocumentContentOperationsManager.cxx b/sw/source/core/doc/DocumentContentOperationsManager.cxx
index 1f387f4d7d26..77228f05caa3 100644
--- a/sw/source/core/doc/DocumentContentOperationsManager.cxx
+++ b/sw/source/core/doc/DocumentContentOperationsManager.cxx
@@ -3129,11 +3129,14 @@ void DocumentContentOperationsManager::CopyWithFlyInFly(
     const SwNodeRange& rRg,
     const sal_Int32 nEndContentIndex,
     const SwNodeIndex& rInsPos,
-    const SwPaM* pCopiedPaM,
+    const std::pair<const SwPaM&, const SwPosition&>* pCopiedPaM /*and real insert pos*/,
     const bool bMakeNewFrms,
     const bool bDelRedlines,
     const bool bCopyFlyAtFly ) const
 {
+    assert(!pCopiedPaM || pCopiedPaM->first.End()->nContent == nEndContentIndex);
+    assert(!pCopiedPaM || pCopiedPaM->first.End()->nNode == rRg.aEnd);
+
     SwDoc* pDest = rInsPos.GetNode().GetDoc();
 
     _SaveRedlEndPosForRestore aRedlRest( rInsPos, 0 );
@@ -3176,14 +3179,24 @@ void DocumentContentOperationsManager::CopyWithFlyInFly(
     SwNodeRange aCpyRange( aSavePos, rInsPos );
 
     // Also copy all bookmarks
+    // guess this must be done before the _DelDummyNodes below as that
+    // deletes nodes so would mess up the index arithmetic
     if( m_rDoc.getIDocumentMarkAccess()->getAllMarksCount() )
     {
         SwPaM aRgTmp( rRg.aStart, rRg.aEnd );
-        SwPaM aCpyTmp( aCpyRange.aStart, aCpyRange.aEnd );
+        SwPaM aCpyPaM(aCpyRange.aStart, aCpyRange.aEnd);
+        if (pCopiedPaM && rRg.aStart != pCopiedPaM->first.Start()->nNode)
+        {
+            // there is 1 (partially selected, maybe) paragraph before
+            assert(SwNodeIndex(rRg.aStart, -1) == pCopiedPaM->first.Start()->nNode);
+            // only use the passed in target SwPosition if the source PaM point
+            // is on a different node; if it was the same node then the target
+            // position was likely moved along by the copy operation and now
+            // points to the end of the range!
+            *aCpyPaM.GetPoint() = pCopiedPaM->second;
+        }
 
-        lcl_CopyBookmarks(
-            pCopiedPaM != NULL ? *pCopiedPaM : aRgTmp,
-            aCpyTmp );
+        lcl_CopyBookmarks((pCopiedPaM) ? pCopiedPaM->first : aRgTmp, aCpyPaM);
     }
 
     if( bDelRedlines && ( nsRedlineMode_t::REDLINE_DELETE_REDLINES & pDest->getIDocumentRedlineAccess().GetRedlineMode() ))
@@ -3192,6 +3205,12 @@ void DocumentContentOperationsManager::CopyWithFlyInFly(
     pDest->GetNodes()._DelDummyNodes( aCpyRange );
 }
 
+// TODO: there is a limitation here in that it's not possible to pass a start
+// content index - which means that at-character anchored frames inside
+// partial 1st paragraph of redline is not copied.
+// But the DelFlyInRange() that is called from DelCopyOfSection() does not
+// delete it either, and it also does not delete those on partial last para of
+// redline, so copying those is supressed here too ...
 void DocumentContentOperationsManager::CopyFlyInFlyImpl(
     const SwNodeRange& rRg,
     const sal_Int32 nEndContentIndex,
@@ -4388,16 +4407,26 @@ bool DocumentContentOperationsManager::CopyImpl( SwPaM& rPam, SwPosition& rPos,
                     pDestTextNd->ResetAttr( RES_PAGEDESC );
             }
 
+            SwPosition startPos(SwNodeIndex(pCopyPam->GetPoint()->nNode, +1),
+                SwIndex(SwNodeIndex(pCopyPam->GetPoint()->nNode, +1).GetNode().GetContentNode()));
+            if (bCanMoveBack)
+            {   // pCopyPam is actually 1 before the copy range so move it fwd
+                SwPaM temp(*pCopyPam->GetPoint());
+                temp.Move(fnMoveForward, fnGoContent);
+                startPos = *temp.GetPoint();
+            }
+            assert(startPos.nNode.GetNode().IsContentNode());
+            std::pair<SwPaM const&, SwPosition const&> tmp(rPam, startPos);
             if( aInsPos == pEnd->nNode )
             {
                 SwNodeIndex aSaveIdx( aInsPos, -1 );
-                CopyWithFlyInFly( aRg, 0,aInsPos, &rPam, bMakeNewFrms, false );
+                CopyWithFlyInFly( aRg, 0, aInsPos, &tmp, bMakeNewFrms, false );
                 ++aSaveIdx;
                 pEnd->nNode = aSaveIdx;
                 pEnd->nContent.Assign( aSaveIdx.GetNode().GetTextNode(), 0 );
             }
             else
-                CopyWithFlyInFly( aRg, pEnd->nContent.GetIndex(), aInsPos, &rPam, bMakeNewFrms, false );
+                CopyWithFlyInFly( aRg, pEnd->nContent.GetIndex(), aInsPos, &tmp, bMakeNewFrms, false );
 
             bCopyBookmarks = false;
 
diff --git a/sw/source/core/inc/DocumentContentOperationsManager.hxx b/sw/source/core/inc/DocumentContentOperationsManager.hxx
index 44560fc14840..b6753a582385 100644
--- a/sw/source/core/inc/DocumentContentOperationsManager.hxx
+++ b/sw/source/core/inc/DocumentContentOperationsManager.hxx
@@ -103,7 +103,7 @@ public:
     void CopyWithFlyInFly( const SwNodeRange& rRg,
                             const sal_Int32 nEndContentIndex,
                             const SwNodeIndex& rInsPos,
-                            const SwPaM* pCopiedPaM = NULL,
+                            const std::pair<const SwPaM&, const SwPosition&> * pCopiedPaM = nullptr,
                             bool bMakeNewFrms = true,
                             bool bDelRedlines = true,
                             bool bCopyFlyAtFly = false ) const;