tdf#159438: when there's no frame, close previous bookmark first

Commit 260d6cc6471444b4ef20ed6673831b0b12f96333 (INTEGRATION: CWS mtg2
(1.30.120); FILE MERGED, 2005-12-21) for #i58438# made sure to process
previously opened bookmarks that close at this node, before opening
the bookmarks that start here.

Commit 76a4305d1e90b6617054dd33036e64f005dbcf04 (sw: fix inconsistent
bookmark behavior around at-char/as-char anchored frames, 2017-12-21)
re-introduced the problem accidentally: it only intended to handle
case when there is an anchored frame here.

This change re-instates the correct order (close bookmarks first; then
process collapsed ones; then open new bookmarks) in case there's no
anchored frames here.

To avoid a problem when a non-collapsed bookmark starts and ends at
the same point (e.g., its text was deleted), it gets converted to
collapsed in lcl_FillBookmark. Thus, testRemoveBookmarkText was fixed
in sw/qa/extras/uiwriter/uiwriter4.cxx. There is no reason to keep
the separate -begin/-end elements, especially since on the following
open/save round-trip, it will become collapsed anyway.

Change-Id: Ib555a76f6f776001e12908a1299c24eebf591f6b
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/162743
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>
Signed-off-by: Xisco Fauli <xiscofauli@libreoffice.org>
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/162804

4 files changed, 177 insertions, 55 deletions

diff --git a/sw/qa/extras/odfexport/data/bookmark_order.fodt b/sw/qa/extras/odfexport/data/bookmark_order.fodt
new file mode 100644
index 000000000000..c7eac58758ff
--- /dev/null
+++ b/sw/qa/extras/odfexport/data/bookmark_order.fodt
@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<office:document xmlns:office="urn:oasis:names:tc:opendocument:xmlns:office:1.0" xmlns:text="urn:oasis:names:tc:opendocument:xmlns:text:1.0" office:version="1.3" office:mimetype="application/vnd.oasis.opendocument.text">
+ <office:body>
+  <office:text>
+   <text:p><text:bookmark-start text:name="bookmark1"/>foo<text:bookmark-end text:name="bookmark1"/><text:bookmark text:name="bookmark2"/><text:bookmark-start text:name="bookmark3"/>bar<text:bookmark-end text:name="bookmark3"/></text:p>
+  </office:text>
+ </office:body>
+</office:document>
\ No newline at end of file
diff --git a/sw/qa/extras/odfexport/odfexport2.cxx b/sw/qa/extras/odfexport/odfexport2.cxx
index 37e744e9c852..49836082907c 100644
--- a/sw/qa/extras/odfexport/odfexport2.cxx
+++ b/sw/qa/extras/odfexport/odfexport2.cxx
@@ -1247,6 +1247,102 @@ CPPUNIT_TEST_FIXTURE(Test, testTdf159382)
     }
 }
 
+CPPUNIT_TEST_FIXTURE(Test, testTdf159438)
+{
+    // Given a text with bookmarks, where an end of one bookmark is the position of another,
+    // and the start of a third
+    loadAndReload("bookmark_order.fodt");
+    auto xPara = getParagraph(1);
+
+    // Check that the order of runs is correct (bookmarks don't overlap)
+
+    {
+        auto run = getRun(xPara, 1);
+        CPPUNIT_ASSERT_EQUAL(u"Bookmark"_ustr, getProperty<OUString>(run, "TextPortionType"));
+        CPPUNIT_ASSERT_EQUAL(true, getProperty<bool>(run, "IsStart"));
+        CPPUNIT_ASSERT_EQUAL(false, getProperty<bool>(run, "IsCollapsed"));
+        auto named = getProperty<uno::Reference<container::XNamed>>(run, "Bookmark");
+        CPPUNIT_ASSERT_EQUAL(u"bookmark1"_ustr, named->getName());
+    }
+
+    {
+        auto run = getRun(xPara, 2);
+        CPPUNIT_ASSERT_EQUAL(u"Text"_ustr, getProperty<OUString>(run, "TextPortionType"));
+        CPPUNIT_ASSERT_EQUAL(u"foo"_ustr, run->getString());
+    }
+
+    {
+        auto run = getRun(xPara, 3);
+        CPPUNIT_ASSERT_EQUAL(u"Bookmark"_ustr, getProperty<OUString>(run, "TextPortionType"));
+        CPPUNIT_ASSERT_EQUAL(false, getProperty<bool>(run, "IsStart"));
+        CPPUNIT_ASSERT_EQUAL(false, getProperty<bool>(run, "IsCollapsed"));
+        auto named = getProperty<uno::Reference<container::XNamed>>(run, "Bookmark");
+        CPPUNIT_ASSERT_EQUAL(u"bookmark1"_ustr, named->getName());
+    }
+
+    {
+        auto run = getRun(xPara, 4);
+        CPPUNIT_ASSERT_EQUAL(u"Bookmark"_ustr, getProperty<OUString>(run, "TextPortionType"));
+        CPPUNIT_ASSERT_EQUAL(true, getProperty<bool>(run, "IsStart"));
+        CPPUNIT_ASSERT_EQUAL(true, getProperty<bool>(run, "IsCollapsed"));
+        auto named = getProperty<uno::Reference<container::XNamed>>(run, "Bookmark");
+        CPPUNIT_ASSERT_EQUAL(u"bookmark2"_ustr, named->getName());
+    }
+
+    {
+        auto run = getRun(xPara, 5);
+        CPPUNIT_ASSERT_EQUAL(u"Bookmark"_ustr, getProperty<OUString>(run, "TextPortionType"));
+        CPPUNIT_ASSERT_EQUAL(true, getProperty<bool>(run, "IsStart"));
+        CPPUNIT_ASSERT_EQUAL(false, getProperty<bool>(run, "IsCollapsed"));
+        auto named = getProperty<uno::Reference<container::XNamed>>(run, "Bookmark");
+        CPPUNIT_ASSERT_EQUAL(u"bookmark3"_ustr, named->getName());
+    }
+
+    {
+        auto run = getRun(xPara, 6);
+        CPPUNIT_ASSERT_EQUAL(u"Text"_ustr, getProperty<OUString>(run, "TextPortionType"));
+        CPPUNIT_ASSERT_EQUAL(u"bar"_ustr, run->getString());
+    }
+
+    {
+        auto run = getRun(xPara, 7);
+        CPPUNIT_ASSERT_EQUAL(u"Bookmark"_ustr, getProperty<OUString>(run, "TextPortionType"));
+        CPPUNIT_ASSERT_EQUAL(false, getProperty<bool>(run, "IsStart"));
+        CPPUNIT_ASSERT_EQUAL(false, getProperty<bool>(run, "IsCollapsed"));
+        auto named = getProperty<uno::Reference<container::XNamed>>(run, "Bookmark");
+        CPPUNIT_ASSERT_EQUAL(u"bookmark3"_ustr, named->getName());
+    }
+
+    // Test that the markup stays at save-and-reload
+    xmlDocUniquePtr pXmlDoc = parseExport("content.xml");
+
+    assertXPathNodeName(pXmlDoc, "//office:body/office:text/text:p/*[1]"_ostr,
+                        "bookmark-start"_ostr);
+    assertXPath(pXmlDoc, "//office:body/office:text/text:p/*[1]"_ostr, "name"_ostr,
+                u"bookmark1"_ustr);
+
+    // Without the fix in place, this would fail with
+    // - Expected: bookmark-end
+    // - Actual  : bookmark-start
+    // - In XPath '//office:body/office:text/text:p/*[2]' name of node is incorrect
+    assertXPathNodeName(pXmlDoc, "//office:body/office:text/text:p/*[2]"_ostr, "bookmark-end"_ostr);
+    assertXPath(pXmlDoc, "//office:body/office:text/text:p/*[2]"_ostr, "name"_ostr,
+                u"bookmark1"_ustr);
+
+    assertXPathNodeName(pXmlDoc, "//office:body/office:text/text:p/*[3]"_ostr, "bookmark"_ostr);
+    assertXPath(pXmlDoc, "//office:body/office:text/text:p/*[3]"_ostr, "name"_ostr,
+                u"bookmark2"_ustr);
+
+    assertXPathNodeName(pXmlDoc, "//office:body/office:text/text:p/*[4]"_ostr,
+                        "bookmark-start"_ostr);
+    assertXPath(pXmlDoc, "//office:body/office:text/text:p/*[4]"_ostr, "name"_ostr,
+                u"bookmark3"_ustr);
+
+    assertXPathNodeName(pXmlDoc, "//office:body/office:text/text:p/*[5]"_ostr, "bookmark-end"_ostr);
+    assertXPath(pXmlDoc, "//office:body/office:text/text:p/*[5]"_ostr, "name"_ostr,
+                u"bookmark3"_ustr);
+}
+
 CPPUNIT_PLUGIN_IMPLEMENT();
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sw/qa/extras/uiwriter/uiwriter4.cxx b/sw/qa/extras/uiwriter/uiwriter4.cxx
index c1f0be1757d6..98fcbcae2225 100644
--- a/sw/qa/extras/uiwriter/uiwriter4.cxx
+++ b/sw/qa/extras/uiwriter/uiwriter4.cxx
@@ -682,8 +682,7 @@ CPPUNIT_TEST_FIXTURE(SwUiWriterTest4, testBookmarkCollapsed)
 // 6. Hit Del, thus deleting "abc" (The bookmark "test" is still there).
 // 7. Save the document:
 //      <text:p text:style-name="Standard">
-//          <text:bookmark-start text:name="test"/>
-//          <text:bookmark-end text:name="test"/>
+//          <text:bookmark text:name="test"/>
 //          def
 //      </text:p>
 //
@@ -737,14 +736,10 @@ CPPUNIT_TEST_FIXTURE(SwUiWriterTest4, testRemoveBookmarkText)
     // load only content.xml from the resaved document
     xmlDocUniquePtr pXmlDoc = parseExport("content.xml");
 
-    constexpr OString aPath("/office:document-content/office:body/office:text/text:p"_ostr);
-
-    CPPUNIT_ASSERT_ASSERTION_FAIL(getXPathPosition(pXmlDoc, aPath, "bookmark")); // not found
-    const int pos2 = getXPathPosition(pXmlDoc, aPath, "bookmark-start");
-    const int pos3 = getXPathPosition(pXmlDoc, aPath, "bookmark-end");
-
-    CPPUNIT_ASSERT_EQUAL(0, pos2); // found, and it is first
-    CPPUNIT_ASSERT_EQUAL(1, pos3); // found, and it is second
+    // Bookmark without text becomes collapsed
+    assertXPath(pXmlDoc, "//office:body/office:text/text:p/text:bookmark"_ostr, 1);
+    assertXPath(pXmlDoc, "//office:body/office:text/text:p/text:bookmark-start"_ostr, 0);
+    assertXPath(pXmlDoc, "//office:body/office:text/text:p/text:bookmark-end"_ostr, 0);
 }
 
 // 1. Open a new writer document
diff --git a/sw/source/core/unocore/unoportenum.cxx b/sw/source/core/unocore/unoportenum.cxx
index 186b5c5b2354..494cec746865 100644
--- a/sw/source/core/unocore/unoportenum.cxx
+++ b/sw/source/core/unocore/unoportenum.cxx
@@ -84,7 +84,17 @@ static void lcl_CreatePortions(
 namespace
 {
     enum class BkmType {
-        Start, End, StartEnd
+        // The order is important: BookmarkCompareStruct::operator () depends on it.
+        // When different bookmarks' starts/ends appear at one position, by default (when there's no
+        // frames at the position - see lcl_ExportBookmark), first previous bookmarks close, then
+        // collapsed ones appear, then new bookmarks open.
+        End, StartEnd, Start
+    };
+
+    enum class ExportBookmarkPass
+    {
+        before_frames,
+        after_frames,
     };
 
     struct SwXBookmarkPortion_Impl
@@ -124,7 +134,8 @@ namespace
             // start of the 2nd bookmark BEFORE the end of the first bookmark
             // See bug #i58438# for more details. The below code is correct and
             // fixes both #i58438 and #i16896#
-            return r1->aPosition < r2->aPosition;
+            return std::make_pair(r1->aPosition, r1->nBkmType)
+                   < std::make_pair(r2->aPosition, r2->nBkmType);
         }
     };
     typedef std::multiset < SwXBookmarkPortion_ImplSharedPtr, BookmarkCompareStruct > SwXBookmarkPortion_ImplList;
@@ -132,13 +143,15 @@ namespace
     /// Inserts pBkmk to rBkmArr in case it starts or ends at rOwnNode
     void lcl_FillBookmark(sw::mark::IMark* const pBkmk, const SwNode& rOwnNode, SwDoc& rDoc, SwXBookmarkPortion_ImplList& rBkmArr)
     {
-        bool const hasOther = pBkmk->IsExpanded();
+        bool const isExpanded = pBkmk->IsExpanded();
         const SwPosition& rStartPos = pBkmk->GetMarkStart();
         const SwPosition& rEndPos = pBkmk->GetMarkEnd();
+        // A bookmark where the text was deleted becomes collapsed
+        bool const hasOther = isExpanded && rStartPos != rEndPos;
         bool const bStartPosInNode = rStartPos.GetNode() == rOwnNode;
         bool const bEndPosInNode = rEndPos.GetNode() == rOwnNode;
         sw::mark::CrossRefBookmark* const pCrossRefMark
-            = !hasOther && (bStartPosInNode || bEndPosInNode)
+            = !isExpanded && bStartPosInNode
                   ? dynamic_cast<sw::mark::CrossRefBookmark*>(pBkmk)
                   : nullptr;
 
@@ -560,15 +573,23 @@ lcl_CreateContentControlPortion(const css::uno::Reference<SwXText>& xParent,
  * Exports all bookmarks from rBkmArr into rPortions that have the same start
  * or end position as nIndex.
  *
- * @param rBkmArr the array of bookmarks. If bOnlyFrameStarts is true, then
- * this is only read, otherwise consumed entries are removed.
+ * @param rBkmArr the array of bookmarks.
  *
  * @param rFramePositions the list of positions where there is an at-char /
  * anchored frame.
+ * Collapsed (BkmType::StartEnd) bookmarks, as well as bookmarks that start/end
+ * at the frame anchor position, are considered as wrapping the frames, if any
+ * (i.e., starts are output before the frames; ends are output after frames).
+ * When there's no frame here, bookmarks are expected to not overlap (#i58438):
+ * first, non-collapsed bookmarks' ends are output; then collapsed bookmarks;
+ * then non-collapsed bookmarks' starts.
  *
- * @param bOnlyFrameStarts If true: export only the start of the bookmarks
- * which cover an at-char anchored frame. If false: export the end of the same
- * bookmarks and everything else.
+ * @param stage Case before_frames: if there is a frame at this index, output
+ * starts of both collapsed and non-collapsed bookmarks (remove non-collapsed
+ * starts from rBkmArr, convert collapsed ones to ends); if there's no frame,
+ * doesn't output anything.
+ * Case after_frames: outputs (and removes from rBkmArr) everything (left) at
+ * this index, in the order of occurrence in rBkmArr (see #i58438).
  */
 static void lcl_ExportBookmark(
     TextRangeList_t & rPortions,
@@ -577,54 +598,56 @@ static void lcl_ExportBookmark(
     SwXBookmarkPortion_ImplList& rBkmArr,
     const sal_Int32 nIndex,
     const o3tl::sorted_vector<sal_Int32>& rFramePositions,
-    bool bOnlyFrameStarts)
+    ExportBookmarkPass stage)
 {
     for ( SwXBookmarkPortion_ImplList::iterator aIter = rBkmArr.begin(), aEnd = rBkmArr.end(); aIter != aEnd; )
     {
         const SwXBookmarkPortion_ImplSharedPtr& pPtr = *aIter;
         if ( nIndex > pPtr->getIndex() )
         {
-            if (bOnlyFrameStarts)
-                ++aIter;
-            else
-                aIter = rBkmArr.erase(aIter);
+            assert(!"Some bookmarks were not consumed earlier");
             continue;
         }
         if ( nIndex < pPtr->getIndex() )
             break;
 
-        if ((BkmType::Start == pPtr->nBkmType && bOnlyFrameStarts) ||
-            (BkmType::StartEnd == pPtr->nBkmType))
+        if (stage == ExportBookmarkPass::before_frames)
         {
-            bool bFrameStart = rFramePositions.find(nIndex) != rFramePositions.end();
-            bool bEnd = pPtr->nBkmType == BkmType::StartEnd && bFrameStart && !bOnlyFrameStarts;
-            if (pPtr->nBkmType == BkmType::Start || bFrameStart || !bOnlyFrameStarts)
+            if (rFramePositions.find(nIndex) == rFramePositions.end()) // No frames at this index
+                break; // Do nothing; everything will be output at after_frames pass
+
+            if (pPtr->nBkmType == BkmType::End)
             {
-                // At this we create a text portion, due to one of these
-                // reasons:
-                // - this is the real start of a non-collapsed bookmark
-                // - this is the real position of a collapsed bookmark
-                // - this is the start or end (depending on bOnlyFrameStarts)
-                //   of a collapsed bookmark at the same position as an at-char
-                //   anchored frame
-                rtl::Reference<SwXTextPortion> pPortion =
-                    new SwXTextPortion(pUnoCursor, xParent, bEnd ? PORTION_BOOKMARK_END : PORTION_BOOKMARK_START);
-                rPortions.emplace_back(pPortion);
-                pPortion->SetBookmark(pPtr->xBookmark);
-                pPortion->SetCollapsed( BkmType::StartEnd == pPtr->nBkmType && !bFrameStart );
+                ++aIter;
+                continue; // Only consider BkmType::Start and BkmType::StartEnd in this pass
             }
         }
-        else if (BkmType::End == pPtr->nBkmType && !bOnlyFrameStarts)
-        {
-            rtl::Reference<SwXTextPortion> pPortion =
-                new SwXTextPortion(pUnoCursor, xParent, PORTION_BOOKMARK_END);
-            rPortions.emplace_back(pPortion);
-            pPortion->SetBookmark(pPtr->xBookmark);
-        }
+
+        // At this we create a text portion, due to one of these
+        // reasons:
+        // - this is the real start of a non-collapsed bookmark
+        // - this is the real end of a non-collapsed bookmark
+        // - this is the real position of a collapsed bookmark
+        // - this is the start or end of a collapsed bookmark at the same position as an at-char
+        //   anchored frame
+        const SwTextPortionType portionType
+            = pPtr->nBkmType == BkmType::End ? PORTION_BOOKMARK_END : PORTION_BOOKMARK_START;
+        const bool collapsed
+            = pPtr->nBkmType == BkmType::StartEnd && stage == ExportBookmarkPass::after_frames;
+
+        rtl::Reference<SwXTextPortion> pPortion = new SwXTextPortion(pUnoCursor, xParent, portionType);
+        rPortions.emplace_back(pPortion);
+        pPortion->SetBookmark(pPtr->xBookmark);
+        pPortion->SetCollapsed(collapsed);
 
         // next bookmark
-        if (bOnlyFrameStarts)
+        if (pPtr->nBkmType == BkmType::StartEnd && stage == ExportBookmarkPass::before_frames)
+        {
+            // This is a collapsed bookmark around a frame, and its start portion was just emitted;
+            // turn it into an end bookmark to process after_frames
+            pPtr->nBkmType = BkmType::End;
             ++aIter;
+        }
         else
             aIter = rBkmArr.erase(aIter);
     }
@@ -1168,13 +1191,12 @@ static void lcl_ExportBkmAndRedline(
     SwSoftPageBreakList& rBreakArr,
     const sal_Int32 nIndex,
     const o3tl::sorted_vector<sal_Int32>& rFramePositions,
-    bool bOnlyFrameBookmarkStarts)
+    ExportBookmarkPass stage)
 {
     if (!rBkmArr.empty())
-        lcl_ExportBookmark(rPortions, xParent, pUnoCursor, rBkmArr, nIndex, rFramePositions,
-                           bOnlyFrameBookmarkStarts);
+        lcl_ExportBookmark(rPortions, xParent, pUnoCursor, rBkmArr, nIndex, rFramePositions, stage);
 
-    if (bOnlyFrameBookmarkStarts)
+    if (stage == ExportBookmarkPass::before_frames)
         // Only exporting the start of some collapsed bookmarks: no export of
         // other arrays.
         return;
@@ -1401,7 +1423,7 @@ static void lcl_CreatePortions(
         // Then export start of collapsed bookmarks which "cover" at-char
         // anchored frames.
         lcl_ExportBkmAndRedline( *PortionStack.top().first, i_xParentText,
-            pUnoCursor, Bookmarks, Redlines, SoftPageBreaks, nCurrentIndex, aFramePositions, /*bOnlyFrameBookmarkStarts=*/true );
+            pUnoCursor, Bookmarks, Redlines, SoftPageBreaks, nCurrentIndex, aFramePositions, ExportBookmarkPass::before_frames );
 
         lcl_ExportAnnotationStarts(
             *PortionStack.top().first,
@@ -1419,7 +1441,7 @@ static void lcl_CreatePortions(
         // Export ends of the previously started collapsed bookmarks + all
         // other bookmarks, redlines, etc.
         lcl_ExportBkmAndRedline( *PortionStack.top().first, i_xParentText,
-            pUnoCursor, Bookmarks, Redlines, SoftPageBreaks, nCurrentIndex, aFramePositions, /*bOnlyFrameBookmarkStarts=*/false );
+            pUnoCursor, Bookmarks, Redlines, SoftPageBreaks, nCurrentIndex, aFramePositions, ExportBookmarkPass::after_frames );
 
         lcl_ExportAnnotationStarts(
             *PortionStack.top().first,