editeng: better lifecycle control for ContentNode in EditDoc

Don't use raw pointers, when it is possible to move the unique_ptr
around into another object that is responsible for the object's
ownership.
The ContentNode is either in a vector in the EditDoc class or it
is moved to the EditUndoDelContent class for the undo/redo action.
Those 2 classes are responsible for freeing the ContentNode.

Change-Id: I977d8e418947bb48781f23575d62420260025e57
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161480
Tested-by: Jenkins
Reviewed-by: Tomaž Vajngerl <quikee@gmail.com>

9 files changed, 143 insertions, 52 deletions

diff --git a/editeng/inc/editdoc.hxx b/editeng/inc/editdoc.hxx
index e9016989e840..974447bd806c 100644
--- a/editeng/inc/editdoc.hxx
+++ b/editeng/inc/editdoc.hxx
@@ -211,7 +211,7 @@ public:
     /// deletes
     void Remove(sal_Int32 nPos);
     /// does not delete
-    void Release(sal_Int32 nPos);
+    std::unique_ptr<ContentNode> Release(sal_Int32 nPos);
 
     static OUString GetSepStr( LineEnd eEnd );
 };
diff --git a/editeng/qa/unit/core-test.cxx b/editeng/qa/unit/core-test.cxx
index b5320f90e448..c17fccf1caef 100644
--- a/editeng/qa/unit/core-test.cxx
+++ b/editeng/qa/unit/core-test.cxx
@@ -113,6 +113,7 @@ public:
     void testTdf148148();
 
     void testSingleLine();
+    void testMoveParagraph();
 
     DECL_STATIC_LINK( Test, CalcFieldValueHdl, EditFieldInfo*, void );
 
@@ -139,6 +140,7 @@ public:
     CPPUNIT_TEST(testTdf147196);
     CPPUNIT_TEST(testTdf148148);
     CPPUNIT_TEST(testSingleLine);
+    CPPUNIT_TEST(testMoveParagraph);
     CPPUNIT_TEST_SUITE_END();
 
 private:
@@ -555,8 +557,7 @@ IMPL_STATIC_LINK( Test, CalcFieldValueHdl, EditFieldInfo*, pInfo, void )
 void Test::testHyperlinkCopyPaste()
 {
     // Create Outliner instance
-    Outliner aOutliner(mpItemPool.get(), OutlinerMode
-::TextObject);
+    Outliner aOutliner(mpItemPool.get(), OutlinerMode::TextObject);
     aOutliner.SetCalcFieldValueHdl( LINK( nullptr, Test, CalcFieldValueHdl ) );
 
     // Create EditEngine's instance
@@ -1984,6 +1985,94 @@ void Test::testSingleLine()
     CPPUNIT_ASSERT_EQUAL(sal_Int32(1), aEditEngine.GetLineCount(0));
 }
 
+void Test::testMoveParagraph()
+{
+    EditEngine aEditEngine(mpItemPool.get());
+    aEditEngine.SetPaperSize(Size(5000, 5000));
+    aEditEngine.SetText("Paragraph 1\nParagraph 2\nParagraph 3\nParagraph 4\nParagraph 5");
+
+    CPPUNIT_ASSERT_EQUAL(true, aEditEngine.IsFormatted());
+
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), aEditEngine.GetParagraphCount());
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 1"), aEditEngine.GetText(0));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 2"), aEditEngine.GetText(1));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 3"), aEditEngine.GetText(2));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 4"), aEditEngine.GetText(3));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 5"), aEditEngine.GetText(4));
+
+    aEditEngine.MoveParagraphs(Range(1, 1), 4); // Move paragraph 2 (index 1) -> to before index 4
+
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), aEditEngine.GetParagraphCount());
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 1"), aEditEngine.GetText(0));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 3"), aEditEngine.GetText(1));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 4"), aEditEngine.GetText(2));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 2"), aEditEngine.GetText(3));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 5"), aEditEngine.GetText(4));
+
+    aEditEngine.MoveParagraphs(Range(3, 3), 1); // Move paragraph 2 (index 3) -> to before index 1
+
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), aEditEngine.GetParagraphCount());
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 1"), aEditEngine.GetText(0));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 2"), aEditEngine.GetText(1));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 3"), aEditEngine.GetText(2));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 4"), aEditEngine.GetText(3));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 5"), aEditEngine.GetText(4));
+
+    aEditEngine.MoveParagraphs(Range(1, 2), 4); // Move paragraph 2, 3 -> to before index 4
+
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), aEditEngine.GetParagraphCount());
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 1"), aEditEngine.GetText(0));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 4"), aEditEngine.GetText(1));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 2"), aEditEngine.GetText(2));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 3"), aEditEngine.GetText(3));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 5"), aEditEngine.GetText(4));
+
+    aEditEngine.MoveParagraphs(Range(2, 3), 1); // Move paragraph 2, 3 -> to before index 2
+
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), aEditEngine.GetParagraphCount());
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 1"), aEditEngine.GetText(0));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 2"), aEditEngine.GetText(1));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 3"), aEditEngine.GetText(2));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 4"), aEditEngine.GetText(3));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 5"), aEditEngine.GetText(4));
+
+    aEditEngine.MoveParagraphs(Range(2, 4), 0); // Move paragraph 3, 4, 5 -> to before index 0
+
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), aEditEngine.GetParagraphCount());
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 3"), aEditEngine.GetText(0));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 4"), aEditEngine.GetText(1));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 5"), aEditEngine.GetText(2));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 1"), aEditEngine.GetText(3));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 2"), aEditEngine.GetText(4));
+
+    aEditEngine.MoveParagraphs(Range(0, 2), 5); // Move paragraph 3, 4, 5 -> to before index 0
+
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), aEditEngine.GetParagraphCount());
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 1"), aEditEngine.GetText(0));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 2"), aEditEngine.GetText(1));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 3"), aEditEngine.GetText(2));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 4"), aEditEngine.GetText(3));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 5"), aEditEngine.GetText(4));
+
+    aEditEngine.MoveParagraphs(Range(0, 0), 8); // Move paragraph 1 -> to before index 8 but 8 is out of bounds
+
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), aEditEngine.GetParagraphCount());
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 2"), aEditEngine.GetText(0));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 3"), aEditEngine.GetText(1));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 4"), aEditEngine.GetText(2));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 5"), aEditEngine.GetText(3));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 1"), aEditEngine.GetText(4));
+
+    aEditEngine.MoveParagraphs(Range(4, 4), 0); // Move paragraph 1 -> to before index 0
+
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), aEditEngine.GetParagraphCount());
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 1"), aEditEngine.GetText(0));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 2"), aEditEngine.GetText(1));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 3"), aEditEngine.GetText(2));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 4"), aEditEngine.GetText(3));
+    CPPUNIT_ASSERT_EQUAL(OUString("Paragraph 5"), aEditEngine.GetText(4));
+}
+
 CPPUNIT_TEST_SUITE_REGISTRATION(Test);
 
 }
diff --git a/editeng/source/editeng/editdoc.cxx b/editeng/source/editeng/editdoc.cxx
index c44c0b5a123c..07d61802c773 100644
--- a/editeng/source/editeng/editdoc.cxx
+++ b/editeng/source/editeng/editdoc.cxx
@@ -963,16 +963,17 @@ void EditDoc::Remove(sal_Int32 nPos)
     maContents.erase(maContents.begin() + nPos);
 }
 
-void EditDoc::Release(sal_Int32 nPos)
+std::unique_ptr<ContentNode> EditDoc::Release(sal_Int32 nPos)
 {
     if (nPos < 0 || o3tl::make_unsigned(nPos) >= maContents.size())
     {
         SAL_WARN( "editeng", "EditDoc::Release - out of bounds pos " << nPos);
-        return;
+        return nullptr;
     }
-    // coverity[leaked_storage] - this is on purpose, ownership should be transferred to undo/redo
-    (void)maContents[nPos].release();
+
+    std::unique_ptr<ContentNode> pNode = std::move(maContents[nPos]);
     maContents.erase(maContents.begin() + nPos);
+    return pNode;
 }
 
 sal_Int32 EditDoc::Count() const
diff --git a/editeng/source/editeng/editeng.cxx b/editeng/source/editeng/editeng.cxx
index 4dbb93ce2c94..c5db1b5ca9ab 100644
--- a/editeng/source/editeng/editeng.cxx
+++ b/editeng/source/editeng/editeng.cxx
@@ -733,9 +733,9 @@ void EditEngine::UpdateSelections()
     pImpEditEngine->UpdateSelections();
 }
 
-void EditEngine::InsertContent(ContentNode* pNode, sal_Int32 nPos)
+void EditEngine::InsertContent(std::unique_ptr<ContentNode> pNode, sal_Int32 nPos)
 {
-    pImpEditEngine->InsertContent(pNode, nPos);
+    pImpEditEngine->InsertContent(std::move(pNode), nPos);
 }
 
 EditPaM EditEngine::SplitContent(sal_Int32 nNode, sal_Int32 nSepPos)
diff --git a/editeng/source/editeng/editundo.cxx b/editeng/source/editeng/editundo.cxx
index 5854d1683e46..378e1f8f268e 100644
--- a/editeng/source/editeng/editundo.cxx
+++ b/editeng/source/editeng/editundo.cxx
@@ -157,25 +157,22 @@ ViewShellId EditUndo::GetViewShellId() const
     return mnViewShellId;
 }
 
-EditUndoDelContent::EditUndoDelContent(
-    EditEngine* pEE, ContentNode* pNode, sal_Int32 nPortion) :
-    EditUndo(EDITUNDO_DELCONTENT, pEE),
-    bDelObject(true),
-    nNode(nPortion),
-    pContentNode(pNode) {}
+EditUndoDelContent::EditUndoDelContent(EditEngine* pEE, std::unique_ptr<ContentNode> pNode, sal_Int32 nPortion)
+    : EditUndo(EDITUNDO_DELCONTENT, pEE)
+    , nNode(nPortion)
+    , mpContentNode(std::move(pNode))
+{}
 
 EditUndoDelContent::~EditUndoDelContent()
 {
-    if ( bDelObject )
-        delete pContentNode;
 }
 
 void EditUndoDelContent::Undo()
 {
     DBG_ASSERT( GetEditEngine()->GetActiveView(), "Undo/Redo: No Active View!" );
-    GetEditEngine()->InsertContent( pContentNode, nNode );
-    bDelObject = false; // belongs to the Engine again
-    EditSelection aSel( EditPaM( pContentNode, 0 ), EditPaM( pContentNode, pContentNode->Len() ) );
+    ContentNode* pNode = mpContentNode.get();
+    GetEditEngine()->InsertContent(std::move(mpContentNode), nNode);
+    EditSelection aSel(EditPaM(pNode, 0), EditPaM(pNode, pNode->Len()));
     GetEditEngine()->GetActiveView()->GetImpEditView()->SetEditSelection(aSel);
 }
 
@@ -187,27 +184,29 @@ void EditUndoDelContent::Redo()
 
     // pNode is no longer correct, if the paragraphs where merged
     // in between Undos
-    pContentNode = pEE->GetEditDoc().GetObject( nNode );
-    DBG_ASSERT( pContentNode, "EditUndoDelContent::Redo(): Node?!" );
+    ContentNode* pNode = pEE->GetEditDoc().GetObject(nNode);
+    DBG_ASSERT(pNode, "EditUndoDelContent::Redo(): Node?!");
 
     pEE->RemoveParaPortion(nNode);
 
     // Do not delete node, depends on the undo!
-    pEE->GetEditDoc().Release( nNode );
+    mpContentNode = pEE->GetEditDoc().Release(nNode);
+    assert(mpContentNode.get() == pNode);
+
     if (pEE->IsCallParaInsertedOrDeleted())
-        pEE->ParagraphDeleted( nNode );
+        pEE->ParagraphDeleted(nNode);
 
-    DeletedNodeInfo* pInf = new DeletedNodeInfo( pContentNode, nNode );
-    pEE->AppendDeletedNodeInfo(pInf);
+    DeletedNodeInfo* pDeletedNodeInfo = new DeletedNodeInfo(pNode, nNode);
+    pEE->AppendDeletedNodeInfo(pDeletedNodeInfo);
     pEE->UpdateSelections();
 
-    ContentNode* pN = ( nNode < pEE->GetEditDoc().Count() )
-        ? pEE->GetEditDoc().GetObject( nNode )
-        : pEE->GetEditDoc().GetObject( nNode-1 );
-    DBG_ASSERT( pN && ( pN != pContentNode ), "?! RemoveContent !? " );
-    EditPaM aPaM( pN, pN->Len() );
+    ContentNode* pCheckNode = (nNode < pEE->GetEditDoc().Count())
+        ? pEE->GetEditDoc().GetObject(nNode)
+        : pEE->GetEditDoc().GetObject(nNode - 1);
+
+    DBG_ASSERT(pCheckNode && pCheckNode != mpContentNode.get(), "?! RemoveContent !? ");
 
-    bDelObject = true;  // belongs to the Engine again
+    EditPaM aPaM(pCheckNode, pCheckNode->Len());
 
     pEE->GetActiveView()->GetImpEditView()->SetEditSelection( EditSelection( aPaM, aPaM ) );
 }
diff --git a/editeng/source/editeng/editundo.hxx b/editeng/source/editeng/editundo.hxx
index d08bc4810b62..d43ba9553a39 100644
--- a/editeng/source/editeng/editundo.hxx
+++ b/editeng/source/editeng/editundo.hxx
@@ -36,13 +36,10 @@ enum class TransliterationFlags;
 class EditUndoDelContent : public EditUndo
 {
 private:
-    bool            bDelObject;
-    sal_Int32       nNode;
-    ContentNode*    pContentNode;   // Points to the valid,
-                                    // undestroyed object!
-
+    sal_Int32 nNode;
+    std::unique_ptr<ContentNode> mpContentNode;   // Points to the valid, undestroyed object!
 public:
-    EditUndoDelContent(EditEngine* pEE, ContentNode* pNode, sal_Int32 nPortion);
+    EditUndoDelContent(EditEngine* pEE, std::unique_ptr<ContentNode> pNode, sal_Int32 nPortion);
     virtual ~EditUndoDelContent() override;
 
     virtual void    Undo() override;
diff --git a/editeng/source/editeng/impedit.hxx b/editeng/source/editeng/impedit.hxx
index 4b2000083106..7269c0e0b614 100644
--- a/editeng/source/editeng/impedit.hxx
+++ b/editeng/source/editeng/impedit.hxx
@@ -705,7 +705,7 @@ private:
 
     void                ImpFindKashidas( ContentNode* pNode, sal_Int32 nStart, sal_Int32 nEnd, std::vector<sal_Int32>& rArray );
 
-    void                InsertContent( ContentNode* pNode, sal_Int32 nPos );
+    void                InsertContent(std::unique_ptr<ContentNode> pNode, sal_Int32 nPos);
     EditPaM             SplitContent( sal_Int32 nNode, sal_Int32 nSepPos );
     EditPaM             ConnectContents( sal_Int32 nLeftNode, bool bBackward );
 
diff --git a/editeng/source/editeng/impedit2.cxx b/editeng/source/editeng/impedit2.cxx
index b7a1aca97e2f..94f4c6ccfa8f 100644
--- a/editeng/source/editeng/impedit2.cxx
+++ b/editeng/source/editeng/impedit2.cxx
@@ -2201,7 +2201,8 @@ EditSelection ImpEditEngine::ImpMoveParagraphs( Range aOldPositions, sal_Int32 n
     {
         // always aOldPositions.Min(), since Remove().
         std::unique_ptr<ParaPortion> pTmpPortion = GetParaPortions().Release(aOldPositions.Min());
-        maEditDoc.Release( aOldPositions.Min() );
+        auto pContentNode = maEditDoc.Release(aOldPositions.Min());
+        pContentNode.release();
         aTmpPortionList.Append(std::move(pTmpPortion));
     }
 
@@ -2499,15 +2500,15 @@ EditPaM ImpEditEngine::ImpDeleteSelection(const EditSelection& rCurSel)
 
 void ImpEditEngine::ImpRemoveParagraph( sal_Int32 nPara )
 {
-    ContentNode* pNode = maEditDoc.GetObject( nPara );
-    ContentNode* pNextNode = maEditDoc.GetObject( nPara+1 );
+    assert(maEditDoc.GetObject(nPara));
 
-    assert(pNode);
+    ContentNode* pNextNode = maEditDoc.GetObject( nPara+1 );
 
-    aDeletedNodes.push_back(std::make_unique<DeletedNodeInfo>( pNode, nPara ));
+    std::unique_ptr<ContentNode> pNode = maEditDoc.Release(nPara);
+    aDeletedNodes.push_back(std::make_unique<DeletedNodeInfo>(pNode.get(), nPara));
 
     // The node is managed by the undo and possibly destroyed!
-    maEditDoc.Release( nPara );
+
     GetParaPortions().Remove( nPara );
 
     if ( IsCallParaInsertedOrDeleted() )
@@ -2521,13 +2522,15 @@ void ImpEditEngine::ImpRemoveParagraph( sal_Int32 nPara )
     if ( pNextNode )
         ParaAttribsChanged( pNextNode );
 
-    if ( IsUndoEnabled() && !IsInUndo() )
-        InsertUndo(std::make_unique<EditUndoDelContent>(pEditEngine, pNode, nPara));
+    if (IsUndoEnabled() && !IsInUndo())
+    {
+        InsertUndo(std::make_unique<EditUndoDelContent>(pEditEngine, std::move(pNode), nPara));
+    }
     else
     {
         if ( pNode->GetStyleSheet() )
-            EndListening( *pNode->GetStyleSheet() );
-        delete pNode;
+            EndListening(*pNode->GetStyleSheet());
+        pNode.reset();
     }
 }
 
diff --git a/editeng/source/editeng/impedit3.cxx b/editeng/source/editeng/impedit3.cxx
index 67598d46db20..b97a9493a48e 100644
--- a/editeng/source/editeng/impedit3.cxx
+++ b/editeng/source/editeng/impedit3.cxx
@@ -4119,12 +4119,14 @@ void ImpEditEngine::Paint( ImpEditView* pView, const tools::Rectangle& rRect, Ou
     pView->DrawSelectionXOR(pView->GetEditSelection(), nullptr, &rTarget);
 }
 
-void ImpEditEngine::InsertContent( ContentNode* pNode, sal_Int32 nPos )
+void ImpEditEngine::InsertContent(std::unique_ptr<ContentNode> pNode, sal_Int32 nPos )
 {
     DBG_ASSERT( pNode, "NULL-Pointer in InsertContent! " );
     DBG_ASSERT( IsInUndo(), "InsertContent only for Undo()!" );
-    GetParaPortions().Insert(nPos, std::make_unique<ParaPortion>( pNode ));
-    maEditDoc.Insert(nPos, std::unique_ptr<ContentNode>(pNode));
+
+    GetParaPortions().Insert(nPos, std::make_unique<ParaPortion>(pNode.get()));
+    maEditDoc.Insert(nPos, std::move(pNode));
+
     if ( IsCallParaInsertedOrDeleted() )
         GetEditEnginePtr()->ParagraphInserted( nPos );
 }