From 30eab10619041d09310c15a8fe6994ff601bf09f Mon Sep 17 00:00:00 2001
From: Nyall Dawson <nyall.dawson@gmail.com>
Date: Tue, 10 Oct 2017 09:06:02 +1000
Subject: [PATCH] Fix handling of overlapping item commands

---
 src/core/layout/qgslayoutundostack.cpp | 18 +++++++++------
 src/core/layout/qgslayoutundostack.h   |  2 +-
 tests/src/core/testqgslayoutitem.cpp   | 31 ++++++++++++++++++++++++++
 3 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/src/core/layout/qgslayoutundostack.cpp b/src/core/layout/qgslayoutundostack.cpp
index b3bac2f1fed..8674d8b782c 100644
--- a/src/core/layout/qgslayoutundostack.cpp
+++ b/src/core/layout/qgslayoutundostack.cpp
@@ -44,19 +44,20 @@ void QgsLayoutUndoStack::beginCommand( QgsLayoutUndoObjectInterface *object, con
     return;
   }
 
-  mActiveCommand.reset( object->createCommand( commandText, id, nullptr ) );
-  mActiveCommand->saveBeforeState();
+  mActiveCommands.emplace_back( std::unique_ptr< QgsAbstractLayoutUndoCommand >( object->createCommand( commandText, id, nullptr ) ) );
+  mActiveCommands.back()->saveBeforeState();
 }
 
 void QgsLayoutUndoStack::endCommand()
 {
-  if ( !mActiveCommand )
+  if ( mActiveCommands.empty() )
     return;
 
-  mActiveCommand->saveAfterState();
-  if ( mActiveCommand->containsChange() ) //protect against empty commands
+  mActiveCommands.back()->saveAfterState();
+  if ( mActiveCommands.back()->containsChange() ) //protect against empty commands
   {
-    mUndoStack->push( mActiveCommand.release() );
+    mUndoStack->push( mActiveCommands.back().release() );
+    mActiveCommands.pop_back();
 
     mLayout->project()->setDirty( true );
   }
@@ -64,7 +65,10 @@ void QgsLayoutUndoStack::endCommand()
 
 void QgsLayoutUndoStack::cancelCommand()
 {
-  mActiveCommand.reset();
+  if ( mActiveCommands.empty() )
+    return;
+
+  mActiveCommands.pop_back();
 }
 
 QUndoStack *QgsLayoutUndoStack::stack()
diff --git a/src/core/layout/qgslayoutundostack.h b/src/core/layout/qgslayoutundostack.h
index 39fccff2c75..0ea917193a4 100644
--- a/src/core/layout/qgslayoutundostack.h
+++ b/src/core/layout/qgslayoutundostack.h
@@ -104,7 +104,7 @@ class CORE_EXPORT QgsLayoutUndoStack
 
     std::unique_ptr< QUndoStack > mUndoStack;
 
-    std::unique_ptr< QgsAbstractLayoutUndoCommand > mActiveCommand;
+    std::vector< std::unique_ptr< QgsAbstractLayoutUndoCommand > > mActiveCommands;
 
 #ifdef SIP_RUN
     QgsLayoutUndoStack( const QgsLayoutUndoStack &other );
diff --git a/tests/src/core/testqgslayoutitem.cpp b/tests/src/core/testqgslayoutitem.cpp
index e21868ea69f..2cd772af52c 100644
--- a/tests/src/core/testqgslayoutitem.cpp
+++ b/tests/src/core/testqgslayoutitem.cpp
@@ -149,6 +149,7 @@ class TestQgsLayoutItem: public QObject
     void writeReadXmlProperties();
     void undoRedo();
     void multiItemUndo();
+    void overlappingUndo();
 
   private:
 
@@ -1481,6 +1482,36 @@ void TestQgsLayoutItem::multiItemUndo()
   QCOMPARE( item->positionWithUnits(), QgsLayoutPoint( 10, 10 ) );
 }
 
+void TestQgsLayoutItem::overlappingUndo()
+{
+  QgsProject proj;
+  QgsLayout l( &proj );
+
+  QgsLayoutItemRectangularShape *item = new QgsLayoutItemRectangularShape( &l );
+  l.addLayoutItem( item );
+  item->attemptMove( QgsLayoutPoint( 10, 10 ) );
+  QgsLayoutItemRectangularShape *item2 = new QgsLayoutItemRectangularShape( &l );
+  l.addLayoutItem( item2 );
+  item2->attemptMove( QgsLayoutPoint( 20, 20 ) );
+
+  //commands overlap
+  l.undoStack()->beginCommand( item, tr( "Item moved" ), QgsLayoutItem::UndoIncrementalMove );
+  item->attemptMove( QgsLayoutPoint( 1, 1 ) );
+  l.undoStack()->beginCommand( item2, tr( "Item moved" ), QgsLayoutItem::UndoIncrementalMove );
+  item2->attemptMove( QgsLayoutPoint( 21, 21 ) );
+  l.undoStack()->endCommand();
+  l.undoStack()->endCommand();
+
+  // undo should remove item move
+  l.undoStack()->stack()->undo();
+  QCOMPARE( item2->positionWithUnits(), QgsLayoutPoint( 21, 21 ) );
+  QCOMPARE( item->positionWithUnits(), QgsLayoutPoint( 10, 10 ) );
+  l.undoStack()->stack()->undo();
+  QCOMPARE( item2->positionWithUnits(), QgsLayoutPoint( 20, 20 ) );
+  QCOMPARE( item->positionWithUnits(), QgsLayoutPoint( 10, 10 ) );
+
+}
+
 QgsLayoutItem *TestQgsLayoutItem::createCopyViaXml( QgsLayout *layout, QgsLayoutItem *original )
 {
   //save original item to xml