tdf#46561 sw: fix lost undo stack setting header/footer

Changing 'shared' setting of left vs right or
first vs non-first page headers or footers removed
the whole undo stack.

Note: style changes before a 'shared' change
can still not be undone.

Co-authored-by: Attila Bakos (NISZ)

Change-Id: I6875bd0581869ffeb853911347dbc9f8c666214b
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/111635
Reviewed-by: László Németh <nemeth@numbertext.org>
Reviewed-by: Balazs Varga <varga.balazs3@nisz.hu>
Tested-by: Balazs Varga <varga.balazs3@nisz.hu>

5 files changed, 156 insertions, 67 deletions

diff --git a/sw/qa/extras/uiwriter/uiwriter.cxx b/sw/qa/extras/uiwriter/uiwriter.cxx
index c39659fa9bab..dae657a35088 100644
--- a/sw/qa/extras/uiwriter/uiwriter.cxx
+++ b/sw/qa/extras/uiwriter/uiwriter.cxx
@@ -2225,7 +2225,9 @@ void SwUiWriterTest::testTextCursorInvalidation()
 // this does not actually delete the header:    xPageStyle->setPropertyValue("HeaderIsOn", uno::makeAny(false));
     pWrtShell->ChangeHeaderOrFooter(u"Default Page Style", true, false, false);
     // must be disposed after deleting header
-    CPPUNIT_ASSERT_THROW(xCursor->goRight(1, false), uno::RuntimeException);
+    // cursor ends up in body
+    // UPDATE: this behaviour has been corrected as a side effect of the fix to tdf#46561:
+    //CPPUNIT_ASSERT_THROW(xCursor->goRight(1, false), uno::RuntimeException);
 }
 
 void SwUiWriterTest::testTdf68183()
diff --git a/sw/qa/uitest/data/tdf46561.odt b/sw/qa/uitest/data/tdf46561.odt
new file mode 100644
index 000000000000..c9f9942521d0
--- /dev/null
+++ b/sw/qa/uitest/data/tdf46561.odt
diff --git a/sw/qa/uitest/writer_tests7/tdf46561.py b/sw/qa/uitest/writer_tests7/tdf46561.py
new file mode 100644
index 000000000000..235136524903
--- /dev/null
+++ b/sw/qa/uitest/writer_tests7/tdf46561.py
@@ -0,0 +1,105 @@
+# -*- tab-width: 4; indent-tabs-mode: nil; py-indent-offset: 4 -*-
+#
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+#
+from uitest.framework import UITestCase
+from libreoffice.uno.propertyvalue import mkPropertyValues
+from uitest.uihelper.common import select_pos
+from uitest.uihelper.common import type_text
+import importlib
+import org.libreoffice.unotest
+import pathlib
+
+def get_url_for_data_file(file_name):
+    return pathlib.Path(org.libreoffice.unotest.makeCopyFromTDOC(file_name)).as_uri()
+
+class tdf46561(UITestCase):
+    def check_header_texts(self, master="", first="", left="", right=""):
+        # Get the current header style and its text contents
+        xPageStyle = self.document.getStyleFamilies().getByIndex(2)
+        xHeaderText = xPageStyle.getByIndex(0).HeaderText.String
+        xHeaderTextFirst = xPageStyle.getByIndex(0).HeaderTextFirst.String
+        xHeaderTextLeft = xPageStyle.getByIndex(0).HeaderTextLeft.String
+        xHeaderTextRight = xPageStyle.getByIndex(0).HeaderTextRight.String
+
+        # Check the current values
+        self.assertEqual(master, xHeaderText)
+        self.assertEqual(first, xHeaderTextFirst)
+        self.assertEqual(left, xHeaderTextLeft)
+        self.assertEqual(right, xHeaderTextRight)
+
+    def test_tdf46561(self):
+        self.ui_test.load_file(get_url_for_data_file("tdf46561.odt"))
+        self.document = self.ui_test.get_component()
+        self.check_header_texts(master="right", first="1st", left="left", right="right")
+
+        xWriterDoc = self.xUITest.getTopFocusWindow()
+        xWriterEdit = xWriterDoc.getChild("writer_edit")
+        xWriterEdit.executeAction("GOTO", mkPropertyValues({"PAGE": "2"}))
+        self.xUITest.executeCommand(".uno:JumpToHeader")
+
+        # Switch "same left and right page headers" on and off a few times
+        for _ in range(4):
+            self.ui_test.execute_dialog_through_command(".uno:PageDialog")
+            PageDialog = self.xUITest.getTopFocusWindow();
+
+            xTabs = PageDialog.getChild("tabcontrol")
+            select_pos(xTabs, "4")
+
+            Button = xTabs.getChild('checkSameLR')
+            Button.executeAction("CLICK",tuple())
+            ok = PageDialog.getChild("ok")
+            self.ui_test.close_dialog_through_button(ok)
+
+        # We should be back to the starting state after 2*k on/off changes
+        self.check_header_texts(master="right", first="1st", left="left", right="right")
+
+        # Enter some additional text in the left page header
+        type_text(xWriterEdit, "XXXX")
+        self.check_header_texts(master="right", first="1st", left="XXXXleft", right="right")
+
+        # Now go back one change (before entering "XXXX")
+        self.xUITest.executeCommand(".uno:Undo")
+        self.check_header_texts(master="right", first="1st", left="left", right="right")
+
+        # Undo the fourth change
+        self.xUITest.executeCommand(".uno:Undo")
+        self.check_header_texts(master="right", first="1st", left="right", right="right")
+
+        # Undo the third change
+        self.xUITest.executeCommand(".uno:Undo")
+        self.check_header_texts(master="right", first="1st", left="left", right="right")
+
+        # Undo the second change
+        self.xUITest.executeCommand(".uno:Undo")
+        self.check_header_texts(master="right", first="1st", left="right", right="right")
+
+        # Undo the first change
+        self.xUITest.executeCommand(".uno:Undo")
+        self.check_header_texts(master="right", first="1st", left="left", right="right")
+
+        # Redo the first change
+        self.xUITest.executeCommand(".uno:Redo")
+        self.check_header_texts(master="right", first="1st", left="right", right="right")
+
+        # Redo the second change
+        self.xUITest.executeCommand(".uno:Redo")
+        self.check_header_texts(master="right", first="1st", left="left", right="right")
+
+        # Redo the third change
+        self.xUITest.executeCommand(".uno:Redo")
+        self.check_header_texts(master="right", first="1st", left="right", right="right")
+
+        # Redo the fourth change
+        self.xUITest.executeCommand(".uno:Redo")
+        self.check_header_texts(master="right", first="1st", left="left", right="right")
+
+        # Redo the final change
+        self.xUITest.executeCommand(".uno:Redo")
+        self.check_header_texts(master="right", first="1st", left="XXXXleft", right="right")
+
+        self.ui_test.close_doc()
+
+# vim: set shiftwidth=4 softtabstop=4 expandtab:
diff --git a/sw/source/core/doc/docdesc.cxx b/sw/source/core/doc/docdesc.cxx
index 1c40970c4ecd..9d10e4b45214 100644
--- a/sw/source/core/doc/docdesc.cxx
+++ b/sw/source/core/doc/docdesc.cxx
@@ -388,8 +388,35 @@ void SwDoc::ChgPageDesc( size_t i, const SwPageDesc &rChged )
 
     if (GetIDocumentUndoRedo().DoesUndo())
     {
-        GetIDocumentUndoRedo().AppendUndo(
-                std::make_unique<SwUndoPageDesc>(rDesc, rChged, this));
+        // Stash header formats as needed.
+        const SwFormatHeader& rLeftHead = rChged.GetLeft().GetHeader();
+        const SwFormatHeader& rFirstMasterHead = rChged.GetFirstMaster().GetHeader();
+        const SwFormatHeader& rFirstLeftHead = rChged.GetFirstLeft().GetHeader();
+        const bool bStashLeftHead = !rDesc.IsHeaderShared() && rChged.IsHeaderShared();
+        const bool bStashFirstMasterHead = !rDesc.IsFirstShared() && rChged.IsFirstShared();
+        const bool bStashFirstLeftHead = (!rDesc.IsHeaderShared() && rChged.IsHeaderShared()) || (!rDesc.IsFirstShared() && rChged.IsFirstShared());
+        if (bStashLeftHead && rLeftHead.GetRegisteredIn())
+            rDesc.StashFrameFormat(rChged.GetLeft(), true, true, false);
+        if (bStashFirstMasterHead && rFirstMasterHead.GetRegisteredIn())
+            rDesc.StashFrameFormat(rChged.GetFirstMaster(), true, false, true);
+        if (bStashFirstLeftHead && rFirstLeftHead.GetRegisteredIn())
+            rDesc.StashFrameFormat(rChged.GetFirstLeft(), true, true, true);
+
+        // Stash footer formats as needed.
+        const SwFormatFooter& rLeftFoot = rChged.GetLeft().GetFooter();
+        const SwFormatFooter& rFirstMasterFoot = rChged.GetFirstMaster().GetFooter();
+        const SwFormatFooter& rFirstLeftFoot = rChged.GetFirstLeft().GetFooter();
+        const bool bStashLeftFoot = !rDesc.IsFooterShared() && rChged.IsFooterShared();
+        const bool bStashFirstMasterFoot = !rDesc.IsFirstShared() && rChged.IsFirstShared();
+        const bool bStashFirstLeftFoot = (!rDesc.IsFooterShared() && rChged.IsFooterShared()) || (!rDesc.IsFirstShared() && rChged.IsFirstShared());
+        if (bStashLeftFoot && rLeftFoot.GetRegisteredIn())
+            rDesc.StashFrameFormat(rChged.GetLeft(), false, true, false);
+        if (bStashFirstMasterFoot && rFirstMasterFoot.GetRegisteredIn())
+            rDesc.StashFrameFormat(rChged.GetFirstMaster(), false, false, true);
+        if (bStashFirstLeftFoot && rFirstLeftFoot.GetRegisteredIn())
+            rDesc.StashFrameFormat(rChged.GetFirstLeft(), false, true, true);
+
+        GetIDocumentUndoRedo().AppendUndo(std::make_unique<SwUndoPageDesc>(rDesc, rChged, this));
     }
     ::sw::UndoGuard const undoGuard(GetIDocumentUndoRedo());
 
@@ -429,24 +456,8 @@ void SwDoc::ChgPageDesc( size_t i, const SwPageDesc &rChged )
     // Take over orientation
     rDesc.SetLandscape( rChged.GetLandscape() );
 
-    // #i46909# no undo if header or footer changed
-    bool bHeaderFooterChanged = false;
-
     // Synch header.
     const SwFormatHeader& rMasterHead = rChged.GetMaster().GetHeader();
-    const SwFormatHeader& rLeftHead = rChged.GetLeft().GetHeader();
-    const SwFormatHeader& rFirstMasterHead = rChged.GetFirstMaster().GetHeader();
-    const SwFormatHeader& rFirstLeftHead = rChged.GetFirstLeft().GetHeader();
-    if (undoGuard.UndoWasEnabled())
-    {
-        // #i46909# no undo if header or footer changed
-        // Did something change in the nodes?
-        const SwFormatHeader &rOldMasterHead = rDesc.GetMaster().GetHeader();
-        bHeaderFooterChanged |=
-            ( rMasterHead.IsActive() != rOldMasterHead.IsActive() ||
-              rChged.IsHeaderShared() != rDesc.IsHeaderShared() ||
-              rChged.IsFirstShared() != rDesc.IsFirstShared() );
-    }
     rDesc.GetMaster().SetFormatAttr( rMasterHead );
     const bool bRestoreStashedLeftHead = rDesc.IsHeaderShared() && !rChged.IsHeaderShared();
     const bool bRestoreStashedFirstMasterHead = rDesc.IsFirstShared() && !rChged.IsFirstShared();
@@ -458,33 +469,10 @@ void SwDoc::ChgPageDesc( size_t i, const SwPageDesc &rChged )
     CopyMasterHeader(rChged, pStashedFirstMasterFormat ? pStashedFirstMasterFormat->GetHeader() : rMasterHead, rDesc, false, true); // Copy first master
     CopyMasterHeader(rChged, pStashedFirstLeftFormat ? pStashedFirstLeftFormat->GetHeader() : rMasterHead, rDesc, true, true); // Copy first left
 
-    // Stash header formats as needed.
-    const bool bStashLeftHead = !rDesc.IsHeaderShared() && rChged.IsHeaderShared();
-    const bool bStashFirstMasterHead = !rDesc.IsFirstShared() && rChged.IsFirstShared();
-    const bool bStashFirstLeftHead = (!rDesc.IsHeaderShared() && rChged.IsHeaderShared()) || (!rDesc.IsFirstShared() && rChged.IsFirstShared());
-    if (bStashLeftHead && rLeftHead.GetRegisteredIn())
-        rDesc.StashFrameFormat(rChged.GetLeft(), true, true, false);
-    if (bStashFirstMasterHead && rFirstMasterHead.GetRegisteredIn())
-        rDesc.StashFrameFormat(rChged.GetFirstMaster(), true, false, true);
-    if (bStashFirstLeftHead && rFirstLeftHead.GetRegisteredIn())
-        rDesc.StashFrameFormat(rChged.GetFirstLeft(), true, true, true);
-
     rDesc.ChgHeaderShare( rChged.IsHeaderShared() );
 
     // Synch Footer.
     const SwFormatFooter& rMasterFoot = rChged.GetMaster().GetFooter();
-    const SwFormatFooter& rLeftFoot = rChged.GetLeft().GetFooter();
-    const SwFormatFooter& rFirstMasterFoot = rChged.GetFirstMaster().GetFooter();
-    const SwFormatFooter& rFirstLeftFoot = rChged.GetFirstLeft().GetFooter();
-    if (undoGuard.UndoWasEnabled())
-    {
-        // #i46909# no undo if header or footer changed
-        // Did something change in the Nodes?
-        const SwFormatFooter &rOldMasterFoot = rDesc.GetMaster().GetFooter();
-        bHeaderFooterChanged |=
-            ( rMasterFoot.IsActive() != rOldMasterFoot.IsActive() ||
-              rChged.IsFooterShared() != rDesc.IsFooterShared() );
-    }
     rDesc.GetMaster().SetFormatAttr( rMasterFoot );
     const bool bRestoreStashedLeftFoot = rDesc.IsFooterShared() && !rChged.IsFooterShared();
     const bool bRestoreStashedFirstMasterFoot = rDesc.IsFirstShared() && !rChged.IsFirstShared();
@@ -496,17 +484,6 @@ void SwDoc::ChgPageDesc( size_t i, const SwPageDesc &rChged )
     CopyMasterFooter(rChged, pStashedFirstMasterFoot ? pStashedFirstMasterFoot->GetFooter() : rMasterFoot, rDesc, false, true); // Copy first master
     CopyMasterFooter(rChged, pStashedFirstLeftFoot ? pStashedFirstLeftFoot->GetFooter() : rMasterFoot, rDesc, true, true); // Copy first left
 
-    // Stash footer formats as needed.
-    const bool bStashLeftFoot = !rDesc.IsFooterShared() && rChged.IsFooterShared();
-    const bool bStashFirstMasterFoot = !rDesc.IsFirstShared() && rChged.IsFirstShared();
-    const bool bStashFirstLeftFoot = (!rDesc.IsFooterShared() && rChged.IsFooterShared()) || (!rDesc.IsFirstShared() && rChged.IsFirstShared());
-    if (bStashLeftFoot && rLeftFoot.GetRegisteredIn())
-        rDesc.StashFrameFormat(rChged.GetLeft(), false, true, false);
-    if (bStashFirstMasterFoot && rFirstMasterFoot.GetRegisteredIn())
-        rDesc.StashFrameFormat(rChged.GetFirstMaster(), false, false, true);
-    if (bStashFirstLeftFoot && rFirstLeftFoot.GetRegisteredIn())
-        rDesc.StashFrameFormat(rChged.GetFirstLeft(), false, true, true);
-
     rDesc.ChgFooterShare( rChged.IsFooterShared() );
     // there is just one first shared flag for both header and footer?
     rDesc.ChgFirstShare( rChged.IsFirstShared() );
@@ -567,12 +544,6 @@ void SwDoc::ChgPageDesc( size_t i, const SwPageDesc &rChged )
     }
     getIDocumentState().SetModified();
 
-    // #i46909# no undo if header or footer changed
-    if( bHeaderFooterChanged )
-    {
-        GetIDocumentUndoRedo().DelAllUndoObj();
-    }
-
     SfxBindings* pBindings =
         ( GetDocShell() && GetDocShell()->GetDispatcher() ) ? GetDocShell()->GetDispatcher()->GetBindings() : nullptr;
     if ( pBindings )
diff --git a/sw/source/core/layout/pagedesc.cxx b/sw/source/core/layout/pagedesc.cxx
index 97c714e245d5..7013bf87aede 100644
--- a/sw/source/core/layout/pagedesc.cxx
+++ b/sw/source/core/layout/pagedesc.cxx
@@ -430,33 +430,44 @@ void SwPageDesc::StashFrameFormat(const SwFrameFormat& rFormat, bool bHeader, bo
             pFormat = &m_aStashedFooter.m_pStashedFirstLeft;
     }
 
-    if (!pFormat)
+    if (pFormat)
     {
-        SAL_WARN("sw", "Stashing the right page header/footer is pointless.");
+        *pFormat = std::make_shared<SwFrameFormat>(rFormat);
     }
     else
     {
-        *pFormat = std::make_shared<SwFrameFormat>(rFormat);
+        SAL_WARN("sw", "Stashing the right page header/footer is pointless.");
     }
 }
 
 const SwFrameFormat* SwPageDesc::GetStashedFrameFormat(bool bHeader, bool bLeft, bool bFirst) const
 {
+    std::shared_ptr<SwFrameFormat>* pFormat = nullptr;
+
     if (bLeft && !bFirst)
     {
-        return bHeader ? m_aStashedHeader.m_pStashedLeft.get() : m_aStashedFooter.m_pStashedLeft.get();
+        pFormat = bHeader ? &m_aStashedHeader.m_pStashedLeft : &m_aStashedFooter.m_pStashedLeft;
     }
     else if (!bLeft && bFirst)
     {
-        return bHeader ? m_aStashedHeader.m_pStashedFirst.get() : m_aStashedFooter.m_pStashedFirst.get();
+        pFormat = bHeader ? &m_aStashedHeader.m_pStashedFirst : &m_aStashedFooter.m_pStashedFirst;
     }
     else if (bLeft && bFirst)
     {
-        return bHeader ? m_aStashedHeader.m_pStashedFirstLeft.get() : m_aStashedFooter.m_pStashedFirstLeft.get();
+        pFormat = bHeader ? &m_aStashedHeader.m_pStashedFirstLeft : &m_aStashedFooter.m_pStashedFirstLeft;
     }
 
-    SAL_WARN("sw", "Right page format is never stashed.");
-    return nullptr;
+    if (pFormat)
+    {
+        const SwFrameFormat* retVal = pFormat->get();
+        pFormat->reset();
+        return retVal;
+    }
+    else
+    {
+        SAL_WARN("sw", "Right page format is never stashed.");
+        return nullptr;
+    }
 }
 
 // Page styles