sw, out of order undo: allow multiple actions from other views

Previously we assumed that the action to be executed is always exactly
the top of the undo stack minus 1 element.

Extend this, so that in case an other view appends two or more elements
to the undo stack, we still find our undo action. Obviously only do this
if all those undo actions are independent from us.

This requires replacing the swap in svl/ with a move-out + move a range
down + move in construct.

(cherry picked from commit 39f231360013e944a8713248359662b9f282d902)

Conflicts:
sw/source/core/undo/docundo.cxx

Change-Id: Ic12d32d6eb5e77618d99eddb4fa096802f32d655
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/125102
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>

5 files changed, 90 insertions, 15 deletions

diff --git a/svl/source/undo/undo.cxx b/svl/source/undo/undo.cxx
index 5dca45082fce..6bb7bcd21013 100644
--- a/svl/source/undo/undo.cxx
+++ b/svl/source/undo/undo.cxx
@@ -688,13 +688,21 @@ bool SfxUndoManager::ImplUndo( SfxUndoContext* i_contextOrNull )
         return false;
     }
 
-    if (i_contextOrNull && i_contextOrNull->GetUndoOffset() == 1)
+    if (i_contextOrNull && i_contextOrNull->GetUndoOffset() > 0)
     {
-        if (m_xData->pActUndoArray->nCurUndoAction >= 2)
+        size_t nCurrent = m_xData->pActUndoArray->nCurUndoAction;
+        size_t nOffset = i_contextOrNull->GetUndoOffset();
+        if (nCurrent >= nOffset + 1)
         {
-            std::swap(
-                m_xData->pActUndoArray->maUndoActions[m_xData->pActUndoArray->nCurUndoAction - 1],
-                m_xData->pActUndoArray->maUndoActions[m_xData->pActUndoArray->nCurUndoAction - 2]);
+            // Move out the action we want to execute.
+            MarkedUndoAction aAction
+                = std::move(m_xData->pActUndoArray->maUndoActions[nCurrent - 1 - nOffset]);
+            // Move the actions after aAction down by one.
+            std::move(m_xData->pActUndoArray->maUndoActions.data() + nCurrent - nOffset,
+                      m_xData->pActUndoArray->maUndoActions.data() + nCurrent,
+                      m_xData->pActUndoArray->maUndoActions.data() + nCurrent - nOffset - 1);
+            // Move aAction to the top.
+            m_xData->pActUndoArray->maUndoActions[nCurrent - 1] = std::move(aAction);
         }
     }
 
diff --git a/sw/qa/extras/tiledrendering/tiledrendering.cxx b/sw/qa/extras/tiledrendering/tiledrendering.cxx
index 9b64b64c78e8..ffc81dbc3d63 100644
--- a/sw/qa/extras/tiledrendering/tiledrendering.cxx
+++ b/sw/qa/extras/tiledrendering/tiledrendering.cxx
@@ -109,6 +109,7 @@ public:
     void testUndoLimiting();
     void testUndoReordering();
     void testUndoReorderingRedo();
+    void testUndoReorderingMulti();
     void testUndoShapeLimiting();
     void testUndoDispatch();
     void testUndoRepairDispatch();
@@ -191,6 +192,7 @@ public:
     CPPUNIT_TEST(testUndoLimiting);
     CPPUNIT_TEST(testUndoReordering);
     CPPUNIT_TEST(testUndoReorderingRedo);
+    CPPUNIT_TEST(testUndoReorderingMulti);
     CPPUNIT_TEST(testUndoShapeLimiting);
     CPPUNIT_TEST(testUndoDispatch);
     CPPUNIT_TEST(testUndoRepairDispatch);
@@ -1402,6 +1404,56 @@ void SwTiledRenderingTest::testUndoReorderingRedo()
     SfxViewShell::Current()->setLibreOfficeKitViewCallback(nullptr);
 }
 
+void SwTiledRenderingTest::testUndoReorderingMulti()
+{
+    // Create two views and a document of 2 paragraphs.
+    SwXTextDocument* pXTextDocument = createDoc();
+    SwWrtShell* pWrtShell1 = pXTextDocument->GetDocShell()->GetWrtShell();
+    int nView1 = SfxLokHelper::getView();
+    int nView2 = SfxLokHelper::createView();
+    pXTextDocument->initializeForTiledRendering(uno::Sequence<beans::PropertyValue>());
+    SwWrtShell* pWrtShell2 = pXTextDocument->GetDocShell()->GetWrtShell();
+    pWrtShell2->SplitNode();
+    SfxLokHelper::setView(nView1);
+    pWrtShell1->SttEndDoc(/*bStt=*/true);
+    SwTextNode* pTextNode1 = pWrtShell1->GetCursor()->GetNode().GetTextNode();
+    // View 1 types into the first paragraph.
+    pXTextDocument->postKeyEvent(LOK_KEYEVENT_KEYINPUT, 'a', 0);
+    pXTextDocument->postKeyEvent(LOK_KEYEVENT_KEYUP, 'a', 0);
+    Scheduler::ProcessEventsToIdle();
+    SfxLokHelper::setView(nView2);
+    pWrtShell2->SttEndDoc(/*bStt=*/false);
+    SwTextNode* pTextNode2 = pWrtShell2->GetCursor()->GetNode().GetTextNode();
+    // View 2 types into the second paragraph, twice.
+    pXTextDocument->postKeyEvent(LOK_KEYEVENT_KEYINPUT, 'x', 0);
+    pXTextDocument->postKeyEvent(LOK_KEYEVENT_KEYUP, 'x', 0);
+    Scheduler::ProcessEventsToIdle();
+    // Go to the start of the paragraph, to avoid grouping.
+    pWrtShell2->SttPara();
+    pXTextDocument->postKeyEvent(LOK_KEYEVENT_KEYINPUT, 'y', 0);
+    pXTextDocument->postKeyEvent(LOK_KEYEVENT_KEYUP, 'y', 0);
+    Scheduler::ProcessEventsToIdle();
+    CPPUNIT_ASSERT_EQUAL(OUString("a"), pTextNode1->GetText());
+    CPPUNIT_ASSERT_EQUAL(OUString("yx"), pTextNode2->GetText());
+
+    // When view 1 presses undo:
+    SfxLokHelper::setView(nView1);
+    dispatchCommand(mxComponent, ".uno:Undo", {});
+    Scheduler::ProcessEventsToIdle();
+
+    // Then make sure view 1's undo action is invoked, out of order:
+    // Without the accompanying fix in place, this test would have failed with:
+    // - Expression: pTextNode1->GetText().isEmpty()
+    // i.e. out of order undo was not executed, the first paragrph was still "a".
+    CPPUNIT_ASSERT(pTextNode1->GetText().isEmpty());
+    // The top 2 undo actions are not invoked, as they belong to view 2.
+    CPPUNIT_ASSERT_EQUAL(OUString("yx"), pTextNode2->GetText());
+    SfxLokHelper::setView(nView1);
+    SfxViewShell::Current()->setLibreOfficeKitViewCallback(nullptr);
+    SfxLokHelper::setView(nView2);
+    SfxViewShell::Current()->setLibreOfficeKitViewCallback(nullptr);
+}
+
 void SwTiledRenderingTest::testUndoShapeLimiting()
 {
     // Load a document and create a view.
diff --git a/sw/source/core/inc/UndoManager.hxx b/sw/source/core/inc/UndoManager.hxx
index 93126322f809..9cc019595f8e 100644
--- a/sw/source/core/inc/UndoManager.hxx
+++ b/sw/source/core/inc/UndoManager.hxx
@@ -98,9 +98,9 @@ public:
 
     /**
      * Checks if the topmost undo action owned by pView is independent from the topmost action undo
-     * action.
+     * action. Sets rOffset to the offset of that independent undo action on success.
      */
-    bool IsViewUndoActionIndependent(const SwView* pView) const;
+    bool IsViewUndoActionIndependent(const SwView* pView, sal_uInt16& rOffset) const;
 
 protected:
     virtual void EmptyActionsChanged() override;
diff --git a/sw/source/core/undo/docundo.cxx b/sw/source/core/undo/docundo.cxx
index 16266e6a4f85..3370e7298ed1 100644
--- a/sw/source/core/undo/docundo.cxx
+++ b/sw/source/core/undo/docundo.cxx
@@ -39,6 +39,7 @@
 
 #include <sfx2/viewfrm.hxx>
 #include <sfx2/bindings.hxx>
+#include <o3tl/temporary.hxx>
 
 #include <UndoInsert.hxx>
 
@@ -357,7 +358,7 @@ UndoManager::EndUndo(SwUndoId eUndoId, SwRewriter const*const pRewriter)
  * Checks if the topmost undo action owned by pView is independent from the topmost action undo
  * action.
  */
-bool UndoManager::IsViewUndoActionIndependent(const SwView* pView) const
+bool UndoManager::IsViewUndoActionIndependent(const SwView* pView, sal_uInt16& rOffset) const
 {
     if (GetUndoActionCount() <= 1)
     {
@@ -377,10 +378,16 @@ bool UndoManager::IsViewUndoActionIndependent(const SwView* pView) const
 
     // Earlier undo action that belongs to the view, but is not the top one.
     const SfxUndoAction* pViewAction = nullptr;
-    const SfxUndoAction* pAction = GetUndoAction(1);
-    if (pAction->GetViewShellId() == nViewId)
+    size_t nOffset = 0;
+    for (size_t i = 0; i < GetUndoActionCount(); ++i)
     {
-        pViewAction = pAction;
+        const SfxUndoAction* pAction = GetUndoAction(i);
+        if (pAction->GetViewShellId() == nViewId)
+        {
+            pViewAction = pAction;
+            nOffset = i;
+            break;
+        }
     }
 
     if (!pViewAction)
@@ -420,7 +427,13 @@ bool UndoManager::IsViewUndoActionIndependent(const SwView* pView) const
         }
     }
 
-    return rViewInsert.IsIndependent(rTopInsert);
+    if (!rViewInsert.IsIndependent(rTopInsert))
+    {
+        return false;
+    }
+
+    rOffset = nOffset;
+    return true;
 }
 
 bool
@@ -441,7 +454,8 @@ UndoManager::GetLastUndoInfo(
         // If another view created the undo action, prevent undoing it from this view.
         ViewShellId nViewShellId = pView ? pView->GetViewShellId() : m_pDocShell->GetView()->GetViewShellId();
         // Unless we know that the other view's undo action is independent from us.
-        if (pAction->GetViewShellId() != nViewShellId && !IsViewUndoActionIndependent(pView))
+        if (pAction->GetViewShellId() != nViewShellId
+            && !IsViewUndoActionIndependent(pView, o3tl::temporary(sal_uInt16())))
         {
             if (o_pId)
             {
diff --git a/sw/source/uibase/shells/basesh.cxx b/sw/source/uibase/shells/basesh.cxx
index ed6dd8883b57..9a44aea45a2c 100644
--- a/sw/source/uibase/shells/basesh.cxx
+++ b/sw/source/uibase/shells/basesh.cxx
@@ -563,12 +563,13 @@ void SwBaseShell::ExecUndo(SfxRequest &rReq)
                     const SfxUndoAction* pAction = rManager.GetUndoAction();
                     SwView& rSwView = rWrtShell.GetView();
                     ViewShellId nViewShellId = rSwView.GetViewShellId();
+                    sal_uInt16 nOffset = 0;
                     if (pAction->GetViewShellId() != nViewShellId
-                        && rManager.IsViewUndoActionIndependent(&rSwView))
+                        && rManager.IsViewUndoActionIndependent(&rSwView, nOffset))
                     {
                         // Execute the undo with an offset: don't undo the top action, but an
                         // earlier one, since it's independent and that belongs to our view.
-                        nUndoOffset = 1;
+                        nUndoOffset += nOffset;
                     }
                 }