sw_redlinehide_2: fix frame updates for SwUndoRedlineDelete

There is a special case where the redline that is inserted doesn't have
a valid range and then it's split up inside AppendRedline(), in
SwRedlineTable::InsertWithValidRanges().  This happens for example
with a selection that includes a section start node but not its end node
(or vice versa).

This breaks the UpdateFramesForRemoveDeleteRedline() /
UpdateFramesForAddDeleteRedline() because they assume that the given
range is one SwRangeRedline; the result is duplicate text frames.

This could be worked around by iterating the SwRangeRedline in the given
PaM, but for the Remove case there is the additional complication that
the redlines are actually removed by the time the function is called.

So rework the implementation of DeleteAndJoinWithRedlineImpl()
to call the part of InsertWithValidRanges() that does the splitting
into multiple redlines (extracted into new sw::GetAllValidRanges())
and create multiple SwUndoRedlineDelete actions, each of which now
updates the frames properly.

Also clean up the horrible group-undo code to group before inserting,
instead of after-the-fact cleanup.

Change-Id: Ia279910e0c74edabe56b0c4cf87dbbad688179da

sw/inc/redline.hxx

@@ -340,6 +340,13 @@ class SW_DLLPUBLIC SwRedlineHint : public SfxHint
 {
 };
 
+
+namespace sw {
+
+std::vector<SwRangeRedline*> GetAllValidRanges(std::unique_ptr<SwRangeRedline> p);
+
+} // namespace sw
+
 #endif
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */

sw/source/core/doc/DocumentContentOperationsManager.cxx

@@ -3580,10 +3580,32 @@ bool DocumentContentOperationsManager::DeleteAndJoinWithRedlineImpl( SwPaM & rPa
 {
     assert(m_rDoc.getIDocumentRedlineAccess().IsRedlineOn());
 
-    SwUndoRedlineDelete* pUndo = nullptr;
     RedlineFlags eOld = m_rDoc.getIDocumentRedlineAccess().GetRedlineFlags();
     m_rDoc.GetDocumentRedlineManager().checkRedlining( eOld );
 
+    if (*rPam.GetPoint() == *rPam.GetMark())
+    {
+        return false; // do not add empty redlines
+    }
+
+    std::vector<SwRangeRedline*> redlines;
+    {
+        auto pRedline(o3tl::make_unique<SwRangeRedline>(nsRedlineType_t::REDLINE_DELETE, rPam));
+        if (pRedline->HasValidRange())
+        {
+            redlines.push_back(pRedline.release());
+        }
+        else // sigh ... why is such a selection even possible...
+        {    // split it up so we get one SwUndoRedlineDelete per inserted RL
+            redlines = GetAllValidRanges(std::move(pRedline));
+        }
+    }
+
+    if (redlines.empty())
+    {
+        return false;
+    }
+
     auto & rDMA(*m_rDoc.getIDocumentMarkAccess());
     std::vector<std::unique_ptr<SwUndo>> MarkUndos;
     for (auto iter = rDMA.getAnnotationMarksBegin();
@@ -3617,6 +3639,7 @@ bool DocumentContentOperationsManager::DeleteAndJoinWithRedlineImpl( SwPaM & rPa
     if (*rPam.GetPoint() != *rPam.GetMark())
         m_rDoc.getIDocumentRedlineAccess().AcceptRedlineParagraphFormatting(rPam);
 
+    std::vector<std::unique_ptr<SwUndoRedlineDelete>> undos;
     if (m_rDoc.GetIDocumentUndoRedo().DoesUndo())
     {
         /* please don't translate -- for cultural reasons this comment is protected
@@ -3624,47 +3647,61 @@ bool DocumentContentOperationsManager::DeleteAndJoinWithRedlineImpl( SwPaM & rPa
         //JP 06.01.98: MUSS noch optimiert werden!!!
         m_rDoc.getIDocumentRedlineAccess().SetRedlineFlags(
             RedlineFlags::On | RedlineFlags::ShowInsert | RedlineFlags::ShowDelete);
-        pUndo = new SwUndoRedlineDelete( rPam, SwUndoId::DELETE );
-        const SwRewriter aRewriter = pUndo->GetRewriter();
-        m_rDoc.GetIDocumentUndoRedo().StartUndo( SwUndoId::DELETE, &aRewriter );
-        for (auto& it : MarkUndos)
+        for (SwRangeRedline * pRedline : redlines)
+        {
+            assert(pRedline->HasValidRange());
+            undos.emplace_back(o3tl::make_unique<SwUndoRedlineDelete>(
+                        *pRedline, SwUndoId::DELETE));
+        }
+        const SwRewriter aRewriter = undos.front()->GetRewriter();
+        // can only group a single undo action
+        if (MarkUndos.empty() && undos.size() == 1
+            && m_rDoc.GetIDocumentUndoRedo().DoesGroupUndo())
         {
-            m_rDoc.GetIDocumentUndoRedo().AppendUndo(it.release());
+            SwUndo * const pLastUndo( m_rDoc.GetUndoManager().GetLastUndo() );
+            SwUndoRedlineDelete *const pUndoRedlineDel(dynamic_cast<SwUndoRedlineDelete*>(pLastUndo));
+            bool const bMerged = pUndoRedlineDel
+                && pUndoRedlineDel->CanGrouping(*undos.front());
+            if (!bMerged)
+            {
+                m_rDoc.GetIDocumentUndoRedo().AppendUndo(undos.front().release());
+            }
+            undos.clear(); // prevent unmatched EndUndo
+        }
+        else
+        {
+            m_rDoc.GetIDocumentUndoRedo().StartUndo(SwUndoId::DELETE, &aRewriter);
+            for (auto& it : MarkUndos)
+            {
+                m_rDoc.GetIDocumentUndoRedo().AppendUndo(it.release());
+            }
+            for (auto & it : undos)
+            {
+                m_rDoc.GetIDocumentUndoRedo().AppendUndo(it.release());
+            }
         }
-        m_rDoc.GetIDocumentUndoRedo().AppendUndo( pUndo );
     }
 
-    if (*rPam.GetPoint() != *rPam.GetMark())
-        m_rDoc.getIDocumentRedlineAccess().AppendRedline( new SwRangeRedline( nsRedlineType_t::REDLINE_DELETE, rPam ), true );
+    for (SwRangeRedline *const pRedline : redlines)
+    {
+        // note: 1. the pRedline can still be merged & deleted
+        //       2. the impl. can even DeleteAndJoin the range => no plain PaM
+        std::shared_ptr<SwUnoCursor> const pCursor(m_rDoc.CreateUnoCursor(*pRedline->GetMark()));
+        pCursor->SetMark();
+        *pCursor->GetPoint() = *pRedline->GetPoint();
+        m_rDoc.getIDocumentRedlineAccess().AppendRedline(pRedline, true);
+        // sw_redlinehide: 2 reasons why this is needed:
+        // 1. it's the first redline in node => RedlineDelText was sent but ignored
+        // 2. redline spans multiple nodes => must merge text frames
+        sw::UpdateFramesForAddDeleteRedline(*pCursor);
+    }
     m_rDoc.getIDocumentState().SetModified();
 
-    // sw_redlinehide: 2 reasons why this is needed:
-    // 1. it's the first redline in node => RedlineDelText was sent but ignored
-    // 2. redline spans multiple nodes => must merge text frames
-    sw::UpdateFramesForAddDeleteRedline(rPam);
-
-    if (pUndo)
+    if (m_rDoc.GetIDocumentUndoRedo().DoesUndo())
     {
-        m_rDoc.GetIDocumentUndoRedo().EndUndo( SwUndoId::EMPTY, nullptr );
-        // ??? why the hell is the AppendUndo not below the
-        // CanGrouping, so this hideous cleanup wouldn't be necessary?
-        // bah, this is redlining, probably changing this would break it...
-        if (m_rDoc.GetIDocumentUndoRedo().DoesGroupUndo())
+        if (!undos.empty())
         {
-            SwUndo * const pLastUndo( m_rDoc.GetUndoManager().GetLastUndo() );
-            SwUndoRedlineDelete *const pUndoRedlineDel( dynamic_cast<SwUndoRedlineDelete*>(pLastUndo) );
-            if (pUndoRedlineDel)
-            {
-                bool const bMerged = pUndoRedlineDel->CanGrouping( *pUndo );
-                if (bMerged)
-                {
-                    ::sw::UndoGuard const undoGuard( m_rDoc.GetIDocumentUndoRedo() );
-                    SwUndo const*const pDeleted = m_rDoc.GetUndoManager().RemoveLastUndo();
-                    OSL_ENSURE( pDeleted == pUndo, "DeleteAndJoinWithRedlineImpl: "
-                        "undo removed is not undo inserted?" );
-                    delete pDeleted;
-                }
-            }
+            m_rDoc.GetIDocumentUndoRedo().EndUndo(SwUndoId::EMPTY, nullptr);
         }
         //JP 06.01.98: MUSS noch optimiert werden!!!
         m_rDoc.getIDocumentRedlineAccess().SetRedlineFlags( eOld );

sw/source/core/doc/docredln.cxx

@@ -454,10 +454,12 @@ bool SwRedlineTable::Insert(SwRangeRedlinePtr& p, size_type& rP)
     return InsertWithValidRanges( p, &rP );
 }
 
-bool SwRedlineTable::InsertWithValidRanges(SwRangeRedlinePtr& p, size_type* pInsPos)
+namespace sw {
+
+std::vector<SwRangeRedline*> GetAllValidRanges(std::unique_ptr<SwRangeRedline> p)
 {
+    std::vector<SwRangeRedline*> ret;
     // Create valid "sub-ranges" from the Selection
-    bool bAnyIns = false;
     SwPosition* pStt = p->Start(),
               * pEnd = pStt == p->GetPoint() ? p->GetMark() : p->GetPoint();
     SwPosition aNewStt( *pStt );
@@ -474,7 +476,6 @@ bool SwRedlineTable::InsertWithValidRanges(SwRangeRedlinePtr& p, size_type* pIns
     }
 
     SwRangeRedline* pNew = nullptr;
-    size_type nInsPos;
 
     if( aNewStt < *pEnd )
         do {
@@ -542,14 +543,10 @@ bool SwRedlineTable::InsertWithValidRanges(SwRangeRedlinePtr& p, size_type* pIns
 #endif
 
             if( *pNew->GetPoint() != *pNew->GetMark() &&
-                pNew->HasValidRange() &&
-                Insert( pNew, nInsPos ) )
+                pNew->HasValidRange())
             {
-                pNew->CallDisplayFunc(nInsPos);
-                bAnyIns = true;
+                ret.push_back(pNew);
                 pNew = nullptr;
-                if( pInsPos && *pInsPos < nInsPos )
-                    *pInsPos = nInsPos;
             }
 
             if( aNewStt >= *pEnd ||
@@ -561,7 +558,31 @@ bool SwRedlineTable::InsertWithValidRanges(SwRangeRedlinePtr& p, size_type* pIns
         } while( aNewStt < *pEnd );
 
     delete pNew;
-    delete p;
+    p.reset();
+    return ret;
+}
+
+} // namespace sw
+
+bool SwRedlineTable::InsertWithValidRanges(SwRangeRedlinePtr& p, size_type* pInsPos)
+{
+    bool bAnyIns = false;
+    std::vector<SwRangeRedline*> const redlines(
+            GetAllValidRanges(std::unique_ptr<SwRangeRedline>(p)));
+    for (SwRangeRedline * pRedline : redlines)
+    {
+        assert(pRedline->HasValidRange());
+        size_type nInsPos;
+        if (Insert(pRedline, nInsPos))
+        {
+            pRedline->CallDisplayFunc(nInsPos);
+            bAnyIns = true;
+            if (pInsPos && *pInsPos < nInsPos)
+            {
+                *pInsPos = nInsPos;
+            }
+        }
+    }
     p = nullptr;
     return bAnyIns;
 }