tdf#148389 sw: fix Delete Undo/Redo of bookmark positions

The main problem is that in SwUndoSaveContent::DelContentIndex() if the
selection start/end is equal to the bookmark start/end, the bookmark is
not deleted and no SwHistoryBookmark is created, hence on Undo the
bookmark positions are not restored.

Since deleting bookmarks in more situations might cause user complaints,
let's just extend the creation of SwHistoryBookmark to these cases,
which means we need to take care both here and in
SwHistoryBookmark::SetInDoc() that there is now a situation where all
bookmark positions are saved and restored but the bookmark still exists
in the document because it wasn't deleted.

The next problem is that using Backspace/Delete keys sets the
ArtificialSelection flag which is stored in SwUndoDelete, but when used
multiple times the SwUndoDelete::CanGrouping() extends an existing
SwUndoDelete, and if it previously would not delete a bookmark, the
extended range might fully contain the bookmark and thus delete it on
Redo, so check if there are saved bookmark positions and prevent
grouping in that case.

Another problem is then that SwUndoDelete::RedoImpl() deletes the
bookmark anyway, as already indicated with a FIXME comment.

This can be prevented by passing the now-existing m_DeleteFlags into
DelBookmarks() from DeleteRangeImplImpl().

Change-Id: Id5eb1a58927aaa6e7e8b75be82d7f854d8057cfc
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/159875
Tested-by: Jenkins
Reviewed-by: Michael Stahl <michael.stahl@allotropia.de>
(cherry picked from commit 57974af130e7421da6b07589d4a63a754b757ad6)

10 files changed, 212 insertions, 36 deletions

diff --git a/sw/inc/IDocumentMarkAccess.hxx b/sw/inc/IDocumentMarkAccess.hxx
index c88e51077005..b8d7262120b9 100644
--- a/sw/inc/IDocumentMarkAccess.hxx
+++ b/sw/inc/IDocumentMarkAccess.hxx
@@ -229,7 +229,8 @@ class IDocumentMarkAccess
             const SwNodeIndex& rEnd,
             std::vector< ::sw::mark::SaveBookmark>* pSaveBkmk, // Ugly: SaveBookmark is core-internal
             const SwIndex* pSttIdx,
-            const SwIndex* pEndIdx) =0;
+            const SwIndex* pEndIdx,
+            bool isReplace) = 0;
 
         /** Deletes a mark.
 
diff --git a/sw/qa/extras/uiwriter/uiwriter.cxx b/sw/qa/extras/uiwriter/uiwriter.cxx
index 6a08e69a3d06..cdb6a3634552 100644
--- a/sw/qa/extras/uiwriter/uiwriter.cxx
+++ b/sw/qa/extras/uiwriter/uiwriter.cxx
@@ -1865,6 +1865,150 @@ void SwUiWriterTest::testBookmarkUndo()
     CPPUNIT_ASSERT_EQUAL(sal_Int32(0), pMarkAccess->getAllMarksCount());
 }
 
+CPPUNIT_TEST_FIXTURE(SwUiWriterTest, testTdf148389_Left)
+{
+    SwDoc* const pDoc = createDoc();
+    SwWrtShell* pWrtShell = pDoc->GetDocShell()->GetWrtShell();
+    CPPUNIT_ASSERT(pWrtShell);
+    pWrtShell->Insert("foo bar baz");
+    pWrtShell->Left(CRSR_SKIP_CHARS, /*bSelect=*/false, 4, /*bBasicCall=*/false);
+    pWrtShell->Left(CRSR_SKIP_CHARS, /*bSelect=*/true, 3, /*bBasicCall=*/false);
+    IDocumentMarkAccess* const pMarkAccess = pDoc->getIDocumentMarkAccess();
+
+    auto pMark = pMarkAccess->makeMark(*pWrtShell->GetCursor(), "Mark",
+        IDocumentMarkAccess::MarkType::BOOKMARK, ::sw::mark::InsertMode::New);
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    pWrtShell->Right(CRSR_SKIP_CHARS, /*bSelect=*/false, 4, /*bBasicCall=*/false);
+    pWrtShell->DelLeft();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(7), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->DelLeft();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(6), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->DelLeft();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->DelLeft();
+    // historically it wasn't deleted if empty, not sure if it should be
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(4), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(4), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->Undo();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    // Undo re-creates the mark...
+    pMark = *pMarkAccess->getAllMarksBegin();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(4), pMark->GetMarkPos().nContent.GetIndex());
+    // the problem was that the end position was not restored
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->Undo();
+    // this undo is no longer grouped, to prevent Redo deleting bookmark
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    // Undo re-creates the mark...
+    pMark = *pMarkAccess->getAllMarksBegin();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(4), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(7), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->Redo();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(4), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->Redo();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(4), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(4), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->Undo();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    // Undo re-creates the mark...
+    pMark = *pMarkAccess->getAllMarksBegin();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(4), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->Undo();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    // Undo re-creates the mark...
+    pMark = *pMarkAccess->getAllMarksBegin();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(4), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(7), pMark->GetOtherMarkPos().nContent.GetIndex());
+}
+
+CPPUNIT_TEST_FIXTURE(SwUiWriterTest, testTdf148389_Right)
+{
+    SwDoc* const pDoc = createDoc();
+    SwWrtShell* pWrtShell = pDoc->GetDocShell()->GetWrtShell();
+    CPPUNIT_ASSERT(pWrtShell);
+    pWrtShell->Insert("foo bar baz");
+    pWrtShell->Left(CRSR_SKIP_CHARS, /*bSelect=*/false, 4, /*bBasicCall=*/false);
+    pWrtShell->Left(CRSR_SKIP_CHARS, /*bSelect=*/true, 3, /*bBasicCall=*/false);
+    IDocumentMarkAccess* const pMarkAccess = pDoc->getIDocumentMarkAccess();
+
+    auto pMark = pMarkAccess->makeMark(*pWrtShell->GetCursor(), "Mark",
+        IDocumentMarkAccess::MarkType::BOOKMARK, ::sw::mark::InsertMode::New);
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    pWrtShell->Left(CRSR_SKIP_CHARS, /*bSelect=*/false, 2, /*bBasicCall=*/false);
+    pWrtShell->DelRight();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(3), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(6), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->DelRight();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(2), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->DelRight();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(2), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(4), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->DelRight();
+    // historically it wasn't deleted if empty, not sure if it should be
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(2), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(3), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->Undo();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    // Undo re-creates the mark...
+    pMark = *pMarkAccess->getAllMarksBegin();
+    // the problem was that the start position was not restored
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(2), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->Undo();
+    // this undo is no longer grouped, to prevent Redo deleting bookmark
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    // Undo re-creates the mark...
+    pMark = *pMarkAccess->getAllMarksBegin();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(3), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(6), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->Undo();
+    // this undo is no longer grouped, to prevent Redo deleting bookmark
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    // Undo re-creates the mark...
+    pMark = *pMarkAccess->getAllMarksBegin();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(4), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(7), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->Redo();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(3), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(6), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->Redo();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(2), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->Redo();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(2), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(3), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->Undo();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    // Undo re-creates the mark...
+    pMark = *pMarkAccess->getAllMarksBegin();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(2), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(5), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->Undo();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    // Undo re-creates the mark...
+    pMark = *pMarkAccess->getAllMarksBegin();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(3), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(6), pMark->GetOtherMarkPos().nContent.GetIndex());
+    pWrtShell->Undo();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(1), pMarkAccess->getAllMarksCount());
+    // Undo re-creates the mark...
+    pMark = *pMarkAccess->getAllMarksBegin();
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(4), pMark->GetMarkPos().nContent.GetIndex());
+    CPPUNIT_ASSERT_EQUAL(sal_Int32(7), pMark->GetOtherMarkPos().nContent.GetIndex());
+}
+
 static void lcl_setWeight(SwWrtShell* pWrtShell, FontWeight aWeight)
 {
     SvxWeightItem aWeightItem(aWeight, EE_CHAR_WEIGHT);
diff --git a/sw/source/core/doc/DocumentContentOperationsManager.cxx b/sw/source/core/doc/DocumentContentOperationsManager.cxx
index d90a40b60ea6..1722a52dc312 100644
--- a/sw/source/core/doc/DocumentContentOperationsManager.cxx
+++ b/sw/source/core/doc/DocumentContentOperationsManager.cxx
@@ -4196,7 +4196,8 @@ bool DocumentContentOperationsManager::DeleteRangeImplImpl(SwPaM & rPam, SwDelet
         pEnd->nNode,
         nullptr,
         &pStt->nContent,
-        &pEnd->nContent);
+        &pEnd->nContent,
+        bool(flags & SwDeleteFlags::ArtificialSelection));
 
     SwNodeIndex aSttIdx( pStt->nNode );
     SwContentNode * pCNd = aSttIdx.GetNode().GetContentNode();
diff --git a/sw/source/core/doc/docbm.cxx b/sw/source/core/doc/docbm.cxx
index 347ed827694d..19d70d112e8b 100644
--- a/sw/source/core/doc/docbm.cxx
+++ b/sw/source/core/doc/docbm.cxx
@@ -1066,7 +1066,8 @@ namespace sw { namespace mark
             const SwNodeIndex& rEnd,
             std::vector<SaveBookmark>* pSaveBkmk,
             const SwIndex* pSttIdx,
-            const SwIndex* pEndIdx )
+            const SwIndex* pEndIdx,
+            bool const isReplace)
     {
         std::vector<const_iterator_t> vMarksToDelete;
         bool bIsSortingNeeded = false;
@@ -1089,7 +1090,7 @@ namespace sw { namespace mark
 
             bool bIsPosInRange(false);
             bool bIsOtherPosInRange(false);
-            bool const bDeleteMark = isDeleteMark(pMark, false, rStt, rEnd, pSttIdx, pEndIdx, bIsPosInRange, bIsOtherPosInRange);
+            bool const bDeleteMark = isDeleteMark(pMark, isReplace, rStt, rEnd, pSttIdx, pEndIdx, bIsPosInRange, bIsOtherPosInRange);
 
             if ( bIsPosInRange
                  && ( bIsOtherPosInRange
@@ -1806,7 +1807,8 @@ void DelBookmarks(
     const SwNodeIndex& rEnd,
     std::vector<SaveBookmark> * pSaveBkmk,
     const SwIndex* pSttIdx,
-    const SwIndex* pEndIdx)
+    const SwIndex* pEndIdx,
+    bool const isReplace)
 {
     // illegal range ??
     if(rStt.GetIndex() > rEnd.GetIndex()
@@ -1814,7 +1816,7 @@ void DelBookmarks(
         return;
     SwDoc* const pDoc = rStt.GetNode().GetDoc();
 
-    pDoc->getIDocumentMarkAccess()->deleteMarks(rStt, rEnd, pSaveBkmk, pSttIdx, pEndIdx);
+    pDoc->getIDocumentMarkAccess()->deleteMarks(rStt, rEnd, pSaveBkmk, pSttIdx, pEndIdx, isReplace);
 
     // Copy all Redlines which are in the move area into an array
     // which holds all position information as offset.
diff --git a/sw/source/core/inc/MarkManager.hxx b/sw/source/core/inc/MarkManager.hxx
index 1efb30f3e121..840d492c2b35 100644
--- a/sw/source/core/inc/MarkManager.hxx
+++ b/sw/source/core/inc/MarkManager.hxx
@@ -63,7 +63,7 @@ namespace sw {
             virtual void correctMarksAbsolute(const SwNodeIndex& rOldNode, const SwPosition& rNewPos, const sal_Int32 nOffset) override;
             virtual void correctMarksRelative(const SwNodeIndex& rOldNode, const SwPosition& rNewPos, const sal_Int32 nOffset) override;
 
-            virtual void deleteMarks(const SwNodeIndex& rStt, const SwNodeIndex& rEnd, std::vector< ::sw::mark::SaveBookmark>* pSaveBkmk, const SwIndex* pSttIdx, const SwIndex* pEndIdx) override;
+            virtual void deleteMarks(const SwNodeIndex& rStt, const SwNodeIndex& rEnd, std::vector< ::sw::mark::SaveBookmark>* pSaveBkmk, const SwIndex* pSttIdx, const SwIndex* pEndIdx, bool isReplace) override;
 
             // deleters
             virtual std::unique_ptr<ILazyDeleter>
diff --git a/sw/source/core/inc/mvsave.hxx b/sw/source/core/inc/mvsave.hxx
index 5b84b5e10819..14b9ee6f6be5 100644
--- a/sw/source/core/inc/mvsave.hxx
+++ b/sw/source/core/inc/mvsave.hxx
@@ -95,7 +95,8 @@ void DelBookmarks(const SwNodeIndex& rStt,
     const SwNodeIndex& rEnd,
     std::vector< ::sw::mark::SaveBookmark> * SaveBkmk =nullptr,
     const SwIndex* pSttIdx =nullptr,
-    const SwIndex* pEndIdx =nullptr);
+    const SwIndex* pEndIdx = nullptr,
+    bool isReplace = false);
 
 /** data structure to temporarily hold fly anchor positions relative to some
  *  location. */
diff --git a/sw/source/core/undo/rolbck.cxx b/sw/source/core/undo/rolbck.cxx
index 50ff24aa4d98..ce36b4a1c37c 100644
--- a/sw/source/core/undo/rolbck.cxx
+++ b/sw/source/core/undo/rolbck.cxx
@@ -599,44 +599,37 @@ void SwHistoryBookmark::SetInDoc( SwDoc* pDoc, bool )
     std::unique_ptr<SwPaM> pPam;
     ::sw::mark::IMark* pMark = nullptr;
 
+    // now the situation is that m_bSavePos and m_bSaveOtherPos don't determine
+    // whether the mark was deleted
+    if (auto const iter = pMarkAccess->findMark(m_aName); iter != pMarkAccess->getAllMarksEnd())
+    {
+        pMark = *iter;
+    }
     if(m_bSavePos)
     {
         SwContentNode* const pContentNd = rNds[m_nNode]->GetContentNode();
-        OSL_ENSURE(pContentNd,
-            "<SwHistoryBookmark::SetInDoc(..)>"
-            " - wrong node for a mark");
-
-        // #111660# don't crash when nNode1 doesn't point to content node.
-        if(pContentNd)
-            pPam.reset(new SwPaM(*pContentNd, m_nContent));
+        assert(pContentNd);
+        pPam.reset(new SwPaM(*pContentNd, m_nContent));
     }
     else
     {
-        pMark = *pMarkAccess->findMark(m_aName);
+        assert(pMark);
         pPam.reset(new SwPaM(pMark->GetMarkPos()));
     }
+    assert(pPam);
 
     if(m_bSaveOtherPos)
     {
         SwContentNode* const pContentNd = rNds[m_nOtherNode]->GetContentNode();
-        OSL_ENSURE(pContentNd,
-            "<SwHistoryBookmark::SetInDoc(..)>"
-            " - wrong node for a mark");
-
-        if (pPam != nullptr && pContentNd)
-        {
-            pPam->SetMark();
-            pPam->GetMark()->nNode = m_nOtherNode;
-            pPam->GetMark()->nContent.Assign(pContentNd, m_nOtherContent);
-        }
+        assert(pContentNd);
+        pPam->SetMark();
+        pPam->GetMark()->nNode = m_nOtherNode;
+        pPam->GetMark()->nContent.Assign(pContentNd, m_nOtherContent);
     }
     else if(m_bHadOtherPos)
     {
-        if(!pMark)
-            pMark = *pMarkAccess->findMark(m_aName);
-        OSL_ENSURE(pMark->IsExpanded(),
-            "<SwHistoryBookmark::SetInDoc(..)>"
-            " - missing pos on old mark");
+        assert(pMark);
+        assert(pMark->IsExpanded());
         pPam->SetMark();
         *pPam->GetMark() = pMark->GetOtherMarkPos();
     }
diff --git a/sw/source/core/undo/undel.cxx b/sw/source/core/undo/undel.cxx
index bee925546d75..dc24ce96e2fd 100644
--- a/sw/source/core/undo/undel.cxx
+++ b/sw/source/core/undo/undel.cxx
@@ -547,6 +547,7 @@ bool SwUndoDelete::CanGrouping( SwDoc* pDoc, const SwPaM& rDelPam )
         if( m_bGroup && !m_bBackSp ) return false;
         m_bBackSp = true;
     }
+    // note: compare m_nSttContent here because the text isn't there any more!
     else if( pStt->nContent == m_nSttContent )
     {
         if( m_bGroup && m_bBackSp ) return false;
@@ -575,6 +576,30 @@ bool SwUndoDelete::CanGrouping( SwDoc* pDoc, const SwPaM& rDelPam )
         return false;
     }
 
+    if ((m_DeleteFlags & SwDeleteFlags::ArtificialSelection) && m_pHistory)
+    {
+        IDocumentMarkAccess const& rIDMA(*pDoc->getIDocumentMarkAccess());
+        for (auto i = m_pHistory->Count(); 0 < i; )
+        {
+            --i;
+            SwHistoryHint const*const pHistory((*m_pHistory)[i]);
+            if (pHistory->Which() == HSTRY_BOOKMARK)
+            {
+                SwHistoryBookmark const*const pHistoryBM(
+                        static_cast<SwHistoryBookmark const*>(pHistory));
+                auto const ppMark(rIDMA.findMark(pHistoryBM->GetName()));
+                if (ppMark != rIDMA.getAllMarksEnd()
+                    && (m_bBackSp
+                            ? ((**ppMark).GetMarkPos() == *pStt)
+                            : ((**ppMark).IsExpanded()
+                                && (**ppMark).GetOtherMarkPos() == *pEnd)))
+                {   // prevent grouping that would delete this mark on Redo()
+                    return false;
+                }
+            }
+        }
+    }
+
     {
         SwRedlineSaveDatas aTmpSav;
         const bool bSaved = FillSaveData( rDelPam, aTmpSav, false );
@@ -1304,7 +1329,6 @@ void SwUndoDelete::RedoImpl(::sw::UndoRedoContext & rContext)
         rDoc.getIDocumentContentOperations().DelFullPara( rPam );
     }
     else
-        // FIXME: this ends up calling DeleteBookmarks() on the entire rPam which deletes too many!
         rDoc.getIDocumentContentOperations().DeleteAndJoin(rPam, m_DeleteFlags);
 }
 
diff --git a/sw/source/core/undo/undobj.cxx b/sw/source/core/undo/undobj.cxx
index b1ed15e3124d..8ff858013b72 100644
--- a/sw/source/core/undo/undobj.cxx
+++ b/sw/source/core/undo/undobj.cxx
@@ -1061,6 +1061,7 @@ void SwUndoSaveContent::DelContentIndex( const SwPosition& rMark,
                 // #i81002#
                 bool bSavePos = false;
                 bool bSaveOtherPos = false;
+                bool bDelete = false;
                 const ::sw::mark::IMark *const pBkmk = pMarkAccess->getAllMarksBegin()[n];
                 auto const type(IDocumentMarkAccess::GetType(*pBkmk));
 
@@ -1077,6 +1078,7 @@ void SwUndoSaveContent::DelContentIndex( const SwPosition& rMark,
                     {
                         bSaveOtherPos = true;
                     }
+                    bDelete = bSavePos && bSaveOtherPos;
                 }
                 else
                 {
@@ -1117,8 +1119,16 @@ void SwUndoSaveContent::DelContentIndex( const SwPosition& rMark,
                         {
                             if( bMaybe )
                                 bSavePos = true;
-                            bSaveOtherPos = true;
+                            bDelete = true;
                         }
+                        if (bDelete || pBkmk->GetOtherMarkPos() == *pEnd)
+                        {
+                            bSaveOtherPos = true; // tdf#148389 always undo if at end
+                        }
+                    }
+                    if (!bSavePos && bMaybe && pBkmk->IsExpanded() && *pStt == pBkmk->GetMarkPos())
+                    {
+                        bSavePos = true; // tdf#148389 always undo if at start
                     }
 
                     if ( !bSavePos && !bSaveOtherPos
@@ -1157,6 +1167,7 @@ void SwUndoSaveContent::DelContentIndex( const SwPosition& rMark,
                         {
                             bSavePos = true;
                             bSaveOtherPos = pBkmk->IsExpanded(); //tdf#90138, only save the other pos if there is one
+                            bDelete = true;
                         }
                     }
                 }
@@ -1170,8 +1181,7 @@ void SwUndoSaveContent::DelContentIndex( const SwPosition& rMark,
                         m_pHistory->Add( *pBkmk, bSavePos, bSaveOtherPos );
                     }
                     if ( bSavePos
-                         && ( bSaveOtherPos
-                              || !pBkmk->IsExpanded() ) )
+                         && (bDelete || !pBkmk->IsExpanded()))
                     {
                         pMarkAccess->deleteMark(pMarkAccess->getAllMarksBegin()+n, false);
                         n--;
diff --git a/sw/source/filter/html/htmltab.cxx b/sw/source/filter/html/htmltab.cxx
index 670ed3ae7aeb..8a7c2c8bbd72 100644
--- a/sw/source/filter/html/htmltab.cxx
+++ b/sw/source/filter/html/htmltab.cxx
@@ -4925,7 +4925,7 @@ void SwHTMLParser::ClearFootnotesMarksInRange(const SwNodeIndex& rMkNdIdx, const
 
     //ofz#9733 drop bookmarks in this range
     IDocumentMarkAccess* const pMarkAccess = pDoc->getIDocumentMarkAccess();
-    pMarkAccess->deleteMarks(rMkNdIdx, SwNodeIndex(rPtNdIdx, 1), nullptr, nullptr, nullptr);
+    pMarkAccess->deleteMarks(rMkNdIdx, SwNodeIndex(rPtNdIdx, 1), nullptr, nullptr, nullptr, false);
 
     SwFrameFormats& rTable = *pDoc->GetSpzFrameFormats();
     for ( auto i = rTable.size(); i; )