sw floattable, delete UI: fix undo/redo

Pressing the unfloat context menu item for a frame resulted in 2 undo
actions and trying to undo both even crashed.

One problem was that no SwNode was left in the TextFrame and then it was
deleted; but undo created a TextFrame with an empty SwNode in it.

Fix this by inserting an empty text node at the end of the TextFrame and
only then moving content from it.

The other trouble was that in case the new text node has no matching
frame during MoveNodeRange(), then the fly frame will be deleted twice:
once during MoveNodeRange() (because it has become empty) and once more
during DelLayoutFormat(). This can be avoided by moving AppendTextNode()
to its own layout action. Finally also group the (now) 3 undo actions
together, because it was a single action on the UI.

Change-Id: I95ce0abbab79a293ecb209579c105f96d284a1c9
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/159730
Reviewed-by: Miklos Vajna <vmiklos@collabora.com>
Tested-by: Jenkins

2 files changed, 32 insertions, 2 deletions

diff --git a/sw/qa/core/frmedt/frmedt.cxx b/sw/qa/core/frmedt/frmedt.cxx
index 618fe7fb574f..b2a53e60db27 100644
--- a/sw/qa/core/frmedt/frmedt.cxx
+++ b/sw/qa/core/frmedt/frmedt.cxx
@@ -28,6 +28,7 @@
 #include <formatflysplit.hxx>
 #include <itabenum.hxx>
 #include <frmmgr.hxx>
+#include <UndoManager.hxx>
 
 /// Covers sw/source/core/frmedt/ fixes.
 class SwCoreFrmedtTest : public SwModelTestBase
@@ -204,6 +205,8 @@ CPPUNIT_TEST_FIXTURE(SwCoreFrmedtTest, testSplitFlyUnfloat)
     // Given a document with a floating table:
     createSwDoc();
     SwDoc* pDoc = getSwDocShell()->GetDoc();
+    CPPUNIT_ASSERT(pDoc->GetUndoManager().IsUndoEnabled());
+    pDoc->GetUndoManager().EnableUndo(false);
     sw::FrameFormats<sw::SpzFrameFormat*>& rFlyFormats = *pDoc->GetSpzFrameFormats();
     CPPUNIT_ASSERT(rFlyFormats.empty());
     // Insert a table:
@@ -226,6 +229,7 @@ CPPUNIT_TEST_FIXTURE(SwCoreFrmedtTest, testSplitFlyUnfloat)
     pWrtShell->EndAllAction();
     CPPUNIT_ASSERT(!rFlyFormats.empty());
     CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(1), pDoc->GetTableFrameFormatCount(/*bUsed=*/true));
+    pDoc->GetUndoManager().EnableUndo(true);
 
     // When marking that frame and unfloating it:
     selectShape(1);
@@ -236,6 +240,14 @@ CPPUNIT_TEST_FIXTURE(SwCoreFrmedtTest, testSplitFlyUnfloat)
     // have failed, the frame was not removed.
     CPPUNIT_ASSERT(rFlyFormats.empty());
     CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(1), pDoc->GetTableFrameFormatCount(/*bUsed=*/true));
+
+    // When undoing the conversion to inline:
+    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(1), pDoc->GetUndoManager().GetUndoActionCount());
+    pDoc->GetUndoManager().Undo();
+
+    // Then the undo stack had 2 undo actions and undo-all crashed.
+    CPPUNIT_ASSERT(!rFlyFormats.empty());
+    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(1), pDoc->GetTableFrameFormatCount(/*bUsed=*/true));
 }
 
 CPPUNIT_PLUGIN_IMPLEMENT();
diff --git a/sw/source/core/frmedt/fefly1.cxx b/sw/source/core/frmedt/fefly1.cxx
index d9a620122b07..9e2fedcc3827 100644
--- a/sw/source/core/frmedt/fefly1.cxx
+++ b/sw/source/core/frmedt/fefly1.cxx
@@ -31,6 +31,7 @@
 #include <com/sun/star/embed/XEmbeddedObject.hpp>
 #include <comphelper/types.hxx>
 #include <osl/diagnose.h>
+#include <comphelper/scopeguard.hxx>
 #include <fmtanchr.hxx>
 #include <fmtcntnt.hxx>
 #include <fmtornt.hxx>
@@ -275,6 +276,10 @@ void SwFEShell::SelectFlyFrame( SwFlyFrame& rFrame )
 
 void SwFEShell::UnfloatFlyFrame()
 {
+    GetIDocumentUndoRedo().StartUndo(SwUndoId::DELLAYFMT, nullptr);
+    comphelper::ScopeGuard g([this]
+                             { GetIDocumentUndoRedo().EndUndo(SwUndoId::DELLAYFMT, nullptr); });
+
     SwFlyFrame* pFly = GetSelectedFlyFrame();
     if (!pFly)
     {
@@ -295,7 +300,21 @@ void SwFEShell::UnfloatFlyFrame()
         return;
     }
 
-    SwNodeRange aRange(pFlyStart->GetNode(), SwNodeOffset(1), *pFlyEnd, SwNodeOffset(0));
+    // Create an empty paragraph after the table, so the frame's SwNodes section is non-empty after
+    // MoveNodeRange(). Undo would ensure it's non-empty and then node offsets won't match.
+    IDocumentContentOperations& rIDCO = GetDoc()->getIDocumentContentOperations();
+    {
+        SwNodeIndex aInsertIndex(*pFlyEnd);
+        --aInsertIndex;
+        SwPosition aInsertPos(aInsertIndex);
+        StartAllAction();
+        rIDCO.AppendTextNode(aInsertPos);
+        // Make sure that a layout frame is created for the node, so the fly frame is not deleted,
+        // during MoveNodeRange(), either.
+        EndAllAction();
+    }
+
+    SwNodeRange aRange(pFlyStart->GetNode(), SwNodeOffset(1), *pFlyEnd, SwNodeOffset(-1));
     const SwFormatAnchor& rAnchor = rFlyFormat.GetAnchor();
     SwNode* pAnchor = rAnchor.GetAnchorNode();
     if (!pAnchor)
@@ -305,7 +324,6 @@ void SwFEShell::UnfloatFlyFrame()
 
     // Move the content outside of the text frame.
     SwNodeIndex aInsertPos(*pAnchor);
-    IDocumentContentOperations& rIDCO = rFlyFormat.GetDoc()->getIDocumentContentOperations();
     rIDCO.MoveNodeRange(aRange, aInsertPos.GetNode(), SwMoveFlags::CREATEUNDOOBJ);
 
     // Remove the fly frame frame.