Allow to undo/redo composer grouping/ungrouping

strk · strk · commit a211c982cfe6 · 2016-06-17T13:41:56.000+02:00
Fixes #11371 (crash on ungrouping after moving the group) and more undo/redo related issues. Enable pending test for the crash (now passing) and add many more undo/redo related ones (including signals testing). Includes a new QgsGroupUngroupItemsCommand class and its SIP bindings.
diff --git a/python/core/composer/qgscomposition.sip b/python/core/composer/qgscomposition.sip
@@ -874,6 +874,8 @@ class QgsComposition : QGraphicsScene
     void composerPolylineAdded( QgsComposerPolyline* polyline );
     /** Is emitted when a new composer html has been added to the view*/
     void composerHtmlFrameAdded( QgsComposerHtml* html, QgsComposerFrame* frame );
+    /** Is emitted when a new item group has been added to the view*/
+    void composerItemGroupAdded( QgsComposerItemGroup* group );
     /** Is emitted when new composer label has been added to the view*/
     void composerLabelAdded( QgsComposerLabel* label );
     /** Is emitted when new composer map has been added to the view*/
diff --git a/python/core/composer/qgsgroupungroupitemscommand.sip b/python/core/composer/qgsgroupungroupitemscommand.sip
@@ -0,0 +1,43 @@
+/** A composer command class for grouping / ungrouping composer items.
+ *
+ * If mState == Ungrouped, the command owns the group item
+ */
+class QgsGroupUngroupItemsCommand: QObject, QUndoCommand
+{
+%TypeHeaderCode
+#include "qgsgroupungroupitemscommand.h"
+%End
+
+  public:
+
+    /** Command kind, and state */
+    enum State
+    {
+      Grouped = 0,
+      Ungrouped
+    };
+
+    /** Create a group or ungroup command
+     *
+     * @param s command kind (@see State)
+     * @param item the group item being created or ungrouped
+     * @param c the composition including this group
+     * @param text command label
+     * @param parent parent command, if any
+     *
+     */
+    QgsGroupUngroupItemsCommand( State s, QgsComposerItemGroup* item, QgsComposition* c, const QString& text, QUndoCommand* parent = nullptr );
+    ~QgsGroupUngroupItemsCommand();
+
+    void redo();
+    void undo();
+
+  signals:
+    /** Signals addition of an item (the group) */
+    void itemAdded( QgsComposerItem* item );
+    /** Signals removal of an item (the group) */
+    void itemRemoved( QgsComposerItem* item );
+
+};
+
+
diff --git a/python/core/core.sip b/python/core/core.sip
@@ -168,6 +168,7 @@
 %Include auth/qgsauthmethod.sip
 
 %Include composer/qgsaddremoveitemcommand.sip
+%Include composer/qgsgroupungroupitemscommand.sip
 %Include composer/qgsaddremovemultiframecommand.sip
 %Include composer/qgsatlascomposition.sip
 %Include composer/qgscomposerarrow.sip
diff --git a/src/core/CMakeLists.txt b/src/core/CMakeLists.txt
@@ -268,6 +268,7 @@ SET(QGIS_CORE_SRCS
   composer/qgscomposerutils.cpp
   composer/qgscomposition.cpp
   composer/qgsdoubleboxscalebarstyle.cpp
+  composer/qgsgroupungroupitemscommand.cpp
   composer/qgslegendmodel.cpp
   composer/qgsnumericscalebarstyle.cpp
   composer/qgspaperitem.cpp
@@ -535,6 +536,7 @@ SET(QGIS_CORE_MOC_HDRS
   composer/qgscomposertablev2.h
   composer/qgscomposertexttable.h
   composer/qgscomposition.h
+  composer/qgsgroupungroupitemscommand.h
   composer/qgslegendmodel.h
   composer/qgspaperitem.h
 
diff --git a/src/core/composer/qgsaddremoveitemcommand.cpp b/src/core/composer/qgsaddremoveitemcommand.cpp
@@ -36,6 +36,7 @@ QgsAddRemoveItemCommand::~QgsAddRemoveItemCommand()
 
 void QgsAddRemoveItemCommand::redo()
 {
+  QUndoCommand::redo(); // call redo() on all childs
   if ( mFirstRun )
   {
     mFirstRun = false;
@@ -46,6 +47,7 @@ void QgsAddRemoveItemCommand::redo()
 
 void QgsAddRemoveItemCommand::undo()
 {
+  QUndoCommand::undo(); // call undo() on all childs, in reverse order
   if ( mFirstRun )
   {
     mFirstRun = false;
@@ -58,6 +60,7 @@ void QgsAddRemoveItemCommand::switchState()
 {
   if ( mState == Added )
   {
+    // Remove
     if ( mComposition )
     {
       mComposition->itemsModel()->setItemRemoved( mItem );
@@ -68,6 +71,7 @@ void QgsAddRemoveItemCommand::switchState()
   }
   else //Removed
   {
+    // Add
     if ( mComposition )
     {
       mComposition->itemsModel()->setItemRestored( mItem );
diff --git a/src/core/composer/qgscomposeritemgroup.cpp b/src/core/composer/qgscomposeritemgroup.cpp
@@ -36,7 +36,7 @@ QgsComposerItemGroup::~QgsComposerItemGroup()
   //loop through group members and remove them from the scene
   Q_FOREACH ( QgsComposerItem* item, mItems )
   {
-    if ( !item )
+    if ( !item || item->isRemoved() )
       continue;
 
     //inform model that we are about to remove an item from the scene
diff --git a/src/core/composer/qgscomposition.cpp b/src/core/composer/qgscomposition.cpp
@@ -35,6 +35,7 @@
 #include "qgscomposerattributetablev2.h"
 #include "qgsaddremovemultiframecommand.h"
 #include "qgscomposermultiframecommand.h"
+#include "qgsgroupungroupitemscommand.h"
 #include "qgspaintenginehack.h"
 #include "qgspaperitem.h"
 #include "qgsproject.h"
@@ -1589,6 +1590,10 @@ void QgsComposition::addItemsFromXML( const QDomElement& elem, const QDomDocumen
     QgsComposerItemGroup *newGroup = new QgsComposerItemGroup( this );
     newGroup->readXML( groupElem, doc );
     addItem( newGroup );
+    if ( addUndoCommands )
+    {
+      pushAddRemoveCommand( newGroup, tr( "Group added" ) );
+    }
   }
 
   //Since this function adds items grouped by type, and each item is added to end of
@@ -2014,14 +2019,28 @@ QgsComposerItemGroup *QgsComposition::groupItems( QList<QgsComposerItem *> items
   }
 
   QgsComposerItemGroup* itemGroup = new QgsComposerItemGroup( this );
+  QgsDebugMsg( QString( "itemgroup created with %1 items (%2 to be added)" ) .arg( itemGroup->items().size() ).arg( items.size() ) );
 
   QList<QgsComposerItem*>::iterator itemIter = items.begin();
   for ( ; itemIter != items.end(); ++itemIter )
   {
     itemGroup->addItem( *itemIter );
+    QgsDebugMsg( QString( "itemgroup now has %1" )
+                 .arg( itemGroup->items().size() ) );
   }
 
   addItem( itemGroup );
+
+  QgsGroupUngroupItemsCommand* c = new QgsGroupUngroupItemsCommand( QgsGroupUngroupItemsCommand::Grouped, itemGroup, this, tr( "Items grouped" ) );
+  QObject::connect( c, SIGNAL( itemRemoved( QgsComposerItem* ) ), this, SIGNAL( itemRemoved( QgsComposerItem* ) ) );
+  QObject::connect( c, SIGNAL( itemAdded( QgsComposerItem* ) ), this, SLOT( sendItemAddedSignal( QgsComposerItem* ) ) );
+
+  undoStack()->push( c );
+  QgsProject::instance()->setDirty( true );
+  //QgsDebugMsg( QString( "itemgroup after pushAddRemove has %1" ) .arg( itemGroup->items().size() ) );
+
+  emit composerItemGroupAdded( itemGroup );
+
   return itemGroup;
 }
 
@@ -2033,6 +2052,17 @@ QList<QgsComposerItem *> QgsComposition::ungroupItems( QgsComposerItemGroup* gro
     return ungroupedItems;
   }
 
+  // group ownership transferred to QgsGroupUngroupItemsCommand
+  // Call this before removing group items so it can keep note
+  // of contents
+  QgsGroupUngroupItemsCommand* c = new QgsGroupUngroupItemsCommand( QgsGroupUngroupItemsCommand::Ungrouped, group, this, tr( "Items ungrouped" ) );
+  QObject::connect( c, SIGNAL( itemRemoved( QgsComposerItem* ) ), this, SIGNAL( itemRemoved( QgsComposerItem* ) ) );
+  QObject::connect( c, SIGNAL( itemAdded( QgsComposerItem* ) ), this, SLOT( sendItemAddedSignal( QgsComposerItem* ) ) );
+
+  undoStack()->push( c );
+  QgsProject::instance()->setDirty( true );
+
+
   QSet<QgsComposerItem*> groupedItems = group->items();
   QSet<QgsComposerItem*>::iterator itemIt = groupedItems.begin();
   for ( ; itemIt != groupedItems.end(); ++itemIt )
@@ -2041,10 +2071,9 @@ QList<QgsComposerItem *> QgsComposition::ungroupItems( QgsComposerItemGroup* gro
   }
 
   group->removeItems();
-  removeComposerItem( group, false, false );
 
-  emit itemRemoved( group );
-  delete( group );
+  // note: emits itemRemoved
+  removeComposerItem( group, false, false );
 
   return ungroupedItems;
 }
@@ -2636,6 +2665,7 @@ void QgsComposition::addComposerTableFrame( QgsComposerAttributeTableV2 *table,
   emit composerTableFrameAdded( table, frame );
 }
 
+/* public */
 void QgsComposition::removeComposerItem( QgsComposerItem* item, const bool createCommand, const bool removeGroupItems )
 {
   QgsComposerMap* map = dynamic_cast<QgsComposerMap *>( item );
@@ -2644,25 +2674,36 @@ void QgsComposition::removeComposerItem( QgsComposerItem* item, const bool creat
   {
     mItemsModel->setItemRemoved( item );
     removeItem( item );
+    emit itemRemoved( item );
+
+    QgsDebugMsg( QString( "removeComposerItem called, createCommand:%1 removeGroupItems:%2" )
+                 .arg( createCommand ).arg( removeGroupItems ) );
 
     QgsComposerItemGroup* itemGroup = dynamic_cast<QgsComposerItemGroup*>( item );
     if ( itemGroup && removeGroupItems )
     {
-      //add add/remove item command for every item in the group
-      QUndoCommand* parentCommand = new QUndoCommand( tr( "Remove item group" ) );
+      QgsDebugMsg( QString( "itemGroup && removeGroupItems" ) );
 
+      // Takes ownership of itemGroup
+      QgsAddRemoveItemCommand* parentCommand = new QgsAddRemoveItemCommand(
+        QgsAddRemoveItemCommand::Removed, itemGroup, this,
+        tr( "Remove item group" ) );
+      connectAddRemoveCommandSignals( parentCommand );
+
+      //add add/remove item command for every item in the group
       QSet<QgsComposerItem*> groupedItems = itemGroup->items();
+      QgsDebugMsg( QString( "itemGroup contains %1 items" ) .arg( groupedItems.size() ) );
       QSet<QgsComposerItem*>::iterator it = groupedItems.begin();
       for ( ; it != groupedItems.end(); ++it )
       {
+        mItemsModel->setItemRemoved( *it );
+        removeItem( *it );
         QgsAddRemoveItemCommand* subcommand = new QgsAddRemoveItemCommand( QgsAddRemoveItemCommand::Removed, *it, this, "", parentCommand );
         connectAddRemoveCommandSignals( subcommand );
         emit itemRemoved( *it );
       }
 
       undoStack()->push( parentCommand );
-      emit itemRemoved( itemGroup );
-      delete itemGroup;
     }
     else
     {
@@ -2674,19 +2715,13 @@ void QgsComposition::removeComposerItem( QgsComposerItem* item, const bool creat
         {
           multiFrame = static_cast<QgsComposerFrame*>( item )->multiFrame();
           item->beginItemCommand( tr( "Frame deleted" ) );
-          emit itemRemoved( item );
           item->endItemCommand();
         }
         else
         {
-          emit itemRemoved( item );
           pushAddRemoveCommand( item, tr( "Item deleted" ), QgsAddRemoveItemCommand::Removed );
         }
       }
-      else
-      {
-        emit itemRemoved( item );
-      }
 
       //check if there are frames left. If not, remove the multi frame
       if ( frameItem && multiFrame )
@@ -2823,6 +2858,11 @@ void QgsComposition::sendItemAddedSignal( QgsComposerItem* item )
     emit selectedItemChanged( frame );
     return;
   }
+  QgsComposerItemGroup* group = dynamic_cast<QgsComposerItemGroup*>( item );
+  if ( group )
+  {
+    emit composerItemGroupAdded( group );
+  }
 }
 
 void QgsComposition::updatePaperItems()
diff --git a/src/core/composer/qgscomposition.h b/src/core/composer/qgscomposition.h
@@ -1119,6 +1119,8 @@ class CORE_EXPORT QgsComposition : public QGraphicsScene
     void composerPolylineAdded( QgsComposerPolyline* polyline );
     /** Is emitted when a new composer html has been added to the view*/
     void composerHtmlFrameAdded( QgsComposerHtml* html, QgsComposerFrame* frame );
+    /** Is emitted when a new item group has been added to the view*/
+    void composerItemGroupAdded( QgsComposerItemGroup* group );
     /** Is emitted when new composer label has been added to the view*/
     void composerLabelAdded( QgsComposerLabel* label );
     /** Is emitted when new composer map has been added to the view*/
diff --git a/src/core/composer/qgsgroupungroupitemscommand.cpp b/src/core/composer/qgsgroupungroupitemscommand.cpp
@@ -0,0 +1,96 @@
+/***************************************************************************
+                          qgsgroupungroupitemscommand.cpp
+                          ---------------------------
+    begin                : 2016-06-09
+    copyright            : (C) 2016 by Sandro Santilli
+    email                : strk at kbt dot io
+***************************************************************************/
+
+/***************************************************************************
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *
+ *   it under the terms of the GNU General Public License as published by  *
+ *   the Free Software Foundation; either version 2 of the License, or     *
+ *   (at your option) any later version.                                   *
+ *                                                                         *
+ ***************************************************************************/
+
+#include "qgsgroupungroupitemscommand.h"
+#include "qgscomposeritem.h"
+#include "qgscomposeritemgroup.h"
+#include "qgscomposition.h"
+#include "qgsproject.h"
+#include "qgscomposermodel.h"
+#include "qgslogger.h"
+
+QgsGroupUngroupItemsCommand::QgsGroupUngroupItemsCommand( State s, QgsComposerItemGroup* item, QgsComposition* c, const QString& text, QUndoCommand* parent ):
+    QUndoCommand( text, parent ), mGroup( item ), mComposition( c ), mState( s ), mFirstRun( true )
+{
+  mItems = mGroup->items();
+}
+
+QgsGroupUngroupItemsCommand::~QgsGroupUngroupItemsCommand()
+{
+  if ( mState == Ungrouped )
+  {
+    //command class stores the item if ungrouped from the composition
+    delete mGroup;
+  }
+}
+
+void QgsGroupUngroupItemsCommand::redo()
+{
+  if ( mFirstRun )
+  {
+    mFirstRun = false;
+    return;
+  }
+  switchState();
+}
+
+void QgsGroupUngroupItemsCommand::undo()
+{
+  if ( mFirstRun )
+  {
+    mFirstRun = false;
+    return;
+  }
+  switchState();
+}
+
+void QgsGroupUngroupItemsCommand::switchState()
+{
+  if ( mState == Grouped )
+  {
+    // ungroup
+    if ( mComposition )
+    {
+      // This is probably redundant
+      mComposition->itemsModel()->setItemRemoved( mGroup );
+      mComposition->removeItem( mGroup );
+    }
+    mGroup->removeItems();
+    emit itemRemoved( mGroup );
+    mState = Ungrouped;
+  }
+  else //Ungrouped
+  {
+    // group
+    if ( mComposition )
+    {
+      //delete mGroup; mGroup = new QgsComposerItemGroup( mCompoiser );
+      QSet<QgsComposerItem*>::iterator itemIter = mItems.begin();
+      for ( ; itemIter != mItems.end(); ++itemIter )
+      {
+        mGroup->addItem( *itemIter );
+        QgsDebugMsg( QString( "itemgroup now has %1" ) .arg( mGroup->items().size() ) );
+      }
+      // Add the group
+      mComposition->itemsModel()->setItemRestored( mGroup );
+      mComposition->addItem( mGroup );
+    }
+    mState = Grouped;
+    emit itemAdded( mGroup );
+  }
+  QgsProject::instance()->setDirty( true );
+}
diff --git a/src/core/composer/qgsgroupungroupitemscommand.h b/src/core/composer/qgsgroupungroupitemscommand.h
diff --git a/tests/src/core/testqgscomposergroup.cpp b/tests/src/core/testqgscomposergroup.cpp

Original file line number	Diff line number	Diff line change
`@@ -36,6 +36,7 @@ QgsAddRemoveItemCommand::~QgsAddRemoveItemCommand()`
`36`	`36`
`37`	`37`	`void QgsAddRemoveItemCommand::redo()`
`38`	`38`	`{`
	`39`	`+ QUndoCommand::redo(); // call redo() on all childs`
`39`	`40`	`if ( mFirstRun )`
`40`	`41`	`{`
`41`	`42`	`mFirstRun = false;`
`@@ -46,6 +47,7 @@ void QgsAddRemoveItemCommand::redo()`
`46`	`47`
`47`	`48`	`void QgsAddRemoveItemCommand::undo()`
`48`	`49`	`{`
	`50`	`+ QUndoCommand::undo(); // call undo() on all childs, in reverse order`
`49`	`51`	`if ( mFirstRun )`
`50`	`52`	`{`
`51`	`53`	`mFirstRun = false;`
`@@ -58,6 +60,7 @@ void QgsAddRemoveItemCommand::switchState()`
`58`	`60`	`{`
`59`	`61`	`if ( mState == Added )`
`60`	`62`	`{`
	`63`	`+ // Remove`
`61`	`64`	`if ( mComposition )`
`62`	`65`	`{`
`63`	`66`	`mComposition->itemsModel()->setItemRemoved( mItem );`
`@@ -68,6 +71,7 @@ void QgsAddRemoveItemCommand::switchState()`
`68`	`71`	`}`
`69`	`72`	`else //Removed`
`70`	`73`	`{`
	`74`	`+ // Add`
`71`	`75`	`if ( mComposition )`
`72`	`76`	`{`
`73`	`77`	`mComposition->itemsModel()->setItemRestored( mItem );`
Original file line number	Diff line number	Diff line change
`@@ -36,7 +36,7 @@ QgsComposerItemGroup::~QgsComposerItemGroup()`
`36`	`36`	`//loop through group members and remove them from the scene`
`37`	`37`	`Q_FOREACH ( QgsComposerItem* item, mItems )`
`38`	`38`	`{`
`39`		`- if ( !item )`
	`39`	`+ if ( !item \|\| item->isRemoved() )`
`40`	`40`	`continue;`
`41`	`41`
`42`	`42`	`//inform model that we are about to remove an item from the scene`