Fix undo/redo for groups

Author: nyalldawson

python/core/layout/qgslayoutitemgroup.sip

@@ -30,6 +30,14 @@ class QgsLayoutItemGroup: QgsLayoutItem
     virtual QString displayName() const;
 
 
+    static QgsLayoutItemGroup *create( QgsLayout *layout, const QVariantMap &settings ) /Factory/;
+%Docstring
+ Returns a new group item for the specified ``layout``.
+
+ The caller takes responsibility for deleting the returned object.
+ :rtype: QgsLayoutItemGroup
+%End
+
     void addItem( QgsLayoutItem *item /Transfer/ );
 %Docstring
  Adds an ``item`` to the group. Ownership of the item
@@ -56,6 +64,11 @@ class QgsLayoutItemGroup: QgsLayoutItem
     virtual void attemptResize( const QgsLayoutSize &size );
 
 
+    virtual bool writeXml( QDomElement &parentElement, QDomDocument &document, const QgsReadWriteContext &context ) const;
+
+    virtual bool readXml( const QDomElement &itemElement, const QDomDocument &document, const QgsReadWriteContext &context );
+
+
     virtual void paint( QPainter *painter, const QStyleOptionGraphicsItem *itemStyle, QWidget *pWidget );
 
 

src/core/CMakeLists.txt

@@ -366,6 +366,7 @@ SET(QGIS_CORE_SRCS
   layout/qgslayoutguidecollection.cpp
   layout/qgslayoutitem.cpp
   layout/qgslayoutitemgroup.cpp
+  layout/qgslayoutitemgroupundocommand.cpp
   layout/qgslayoutitemmap.cpp
   layout/qgslayoutitempage.cpp
   layout/qgslayoutitemregistry.cpp
@@ -716,6 +717,7 @@ SET(QGIS_CORE_MOC_HDRS
   layout/qgslayoutguidecollection.h
   layout/qgslayoutitem.h
   layout/qgslayoutitemgroup.h
+  layout/qgslayoutitemgroupundocommand.h
   layout/qgslayoutitemmap.h
   layout/qgslayoutitempage.h
   layout/qgslayoutitemregistry.h

src/core/layout/qgslayout.cpp

@@ -23,6 +23,7 @@
 #include "qgsproject.h"
 #include "qgslayoutitemundocommand.h"
 #include "qgslayoutitemgroup.h"
+#include "qgslayoutitemgroupundocommand.h"
 
 QgsLayout::QgsLayout( QgsProject *project )
   : mProject( project )
@@ -382,10 +383,16 @@ void QgsLayout::removeLayoutItem( QgsLayoutItem *item )
 {
   std::unique_ptr< QgsLayoutItemDeleteUndoCommand > deleteCommand;
   if ( !mBlockUndoCommands )
+  {
+    mUndoStack->beginMacro( tr( "Deleted item" ) );
     deleteCommand.reset( new QgsLayoutItemDeleteUndoCommand( item, tr( "Deleted item" ) ) );
+  }
   removeLayoutItemPrivate( item );
   if ( deleteCommand )
+  {
     mUndoStack->stack()->push( deleteCommand.release() );
+    mUndoStack->endMacro();
+  }
 }
 
 QgsLayoutUndoStack *QgsLayout::undoStack()
@@ -456,20 +463,17 @@ QgsLayoutItemGroup *QgsLayout::groupItems( const QList<QgsLayoutItem *> &items )
   }
   QgsLayoutItemGroup *returnGroup = itemGroup.get();
   addLayoutItem( itemGroup.release() );
-  mUndoStack->endMacro();
-
-#if 0
-  QgsGroupUngroupItemsCommand *c = new QgsGroupUngroupItemsCommand( QgsGroupUngroupItemsCommand::Grouped, itemGroup, this, tr( "Items grouped" ) );
-  connect( c, &QgsGroupUngroupItemsCommand::itemRemoved, this, &QgsComposition::itemRemoved );
-  connect( c, &QgsGroupUngroupItemsCommand::itemAdded, this, &QgsComposition::sendItemAddedSignal );
 
-  undoStack()->push( c );
+  std::unique_ptr< QgsLayoutItemGroupUndoCommand > c( new QgsLayoutItemGroupUndoCommand( QgsLayoutItemGroupUndoCommand::Grouped, returnGroup, this, tr( "Items grouped" ) ) );
+  mUndoStack->stack()->push( c.release() );
   mProject->setDirty( true );
-  //QgsDebugMsg( QString( "itemgroup after pushAddRemove has %1" ) .arg( itemGroup->items().size() ) );
 
+#if 0
   emit composerItemGroupAdded( itemGroup );
 #endif
 
+  mUndoStack->endMacro();
+
   return returnGroup;
 }
 
@@ -481,25 +485,21 @@ QList<QgsLayoutItem *> QgsLayout::ungroupItems( QgsLayoutItemGroup *group )
     return ungroupedItems;
   }
 
-#if 0 //TODO
-  // group ownership transferred to QgsGroupUngroupItemsCommand
+  mUndoStack->beginMacro( tr( "Ungrouped items" ) );
   // Call this before removing group items so it can keep note
   // of contents
-  QgsGroupUngroupItemsCommand *c = new QgsGroupUngroupItemsCommand( QgsGroupUngroupItemsCommand::Ungrouped, group, this, tr( "Items ungrouped" ) );
-  connect( c, &QgsGroupUngroupItemsCommand::itemRemoved, this, &QgsComposition::itemRemoved );
-  connect( c, &QgsGroupUngroupItemsCommand::itemAdded, this, &QgsComposition::sendItemAddedSignal );
+  std::unique_ptr< QgsLayoutItemGroupUndoCommand > c( new QgsLayoutItemGroupUndoCommand( QgsLayoutItemGroupUndoCommand::Ungrouped, group, this, tr( "Items ungrouped" ) ) );
+  mUndoStack->stack()->push( c.release() );
 
-  undoStack()->push( c );
   mProject->setDirty( true );
 
-#endif
-
   ungroupedItems = group->items();
   group->removeItems();
 
   removeLayoutItem( group );
+  mUndoStack->endMacro();
+
 #if 0 //TODO
-  // note: emits itemRemoved
   removeComposerItem( group, false, false );
 #endif
 
@@ -554,7 +554,7 @@ void QgsLayout::removeLayoutItemPrivate( QgsLayoutItem *item )
 #if 0 //TODO
   emit itemRemoved( item );
 #endif
-  item->deleteLater();
+  delete item;
 }
 
 void QgsLayout::updateZValues( const bool addUndoCommands )

src/core/layout/qgslayout.h

@@ -523,6 +523,7 @@ class CORE_EXPORT QgsLayout : public QGraphicsScene, public QgsExpressionContext
     friend class QgsLayoutItemDeleteUndoCommand;
     friend class QgsLayoutItemUndoCommand;
     friend class QgsLayoutUndoCommand;
+    friend class QgsLayoutItemGroupUndoCommand;
     friend class QgsLayoutModel;
 };
 

src/core/layout/qgslayoutitem.cpp

@@ -139,13 +139,20 @@ void QgsLayoutItem::setVisibility( const bool visible )
     return;
   }
 
+  std::unique_ptr< QgsAbstractLayoutUndoCommand > command;
   if ( !shouldBlockUndoCommands() )
-    mLayout->undoStack()->beginCommand( this, visible ? tr( "Item shown" ) : tr( "Item hidden" ) );
+  {
+    command.reset( createCommand( visible ? tr( "Item shown" ) : tr( "Item hidden" ), 0 ) );
+    command->saveBeforeState();
+  }
 
   QGraphicsItem::setVisible( visible );
 
-  if ( !shouldBlockUndoCommands() )
-    mLayout->undoStack()->endCommand();
+  if ( command )
+  {
+    command->saveAfterState();
+    mLayout->undoStack()->stack()->push( command.release() );
+  }
 
   //inform model that visibility has changed
   if ( mLayout )
@@ -729,6 +736,7 @@ bool QgsLayoutItem::writePropertiesToElement( QDomElement &element, QDomDocument
   element.setAttribute( QStringLiteral( "position" ), mItemPosition.encodePoint() );
   element.setAttribute( QStringLiteral( "size" ), mItemSize.encodeSize() );
   element.setAttribute( QStringLiteral( "rotation" ), QString::number( rotation() ) );
+  element.setAttribute( QStringLiteral( "groupUuid" ), mParentGroupUuid );
 
   element.setAttribute( "zValue", QString::number( zValue() ) );
   element.setAttribute( "visibility", isVisible() );
@@ -807,6 +815,15 @@ bool QgsLayoutItem::readPropertiesFromElement( const QDomElement &element, const
   attemptResize( QgsLayoutSize::decodeSize( element.attribute( QStringLiteral( "size" ) ) ) );
   setItemRotation( element.attribute( QStringLiteral( "rotation" ), QStringLiteral( "0" ) ).toDouble() );
 
+  mParentGroupUuid = element.attribute( QStringLiteral( "groupUuid" ) );
+  if ( !mParentGroupUuid.isEmpty() )
+  {
+    if ( QgsLayoutItemGroup *group = parentGroup() )
+    {
+      group->addItem( this );
+    }
+  }
+
   //TODO
   /*
   // temporary for groups imported from templates

src/core/layout/qgslayoutitemgroup.cpp

@@ -25,8 +25,6 @@ QgsLayoutItemGroup::QgsLayoutItemGroup( QgsLayout *layout )
 
 QgsLayoutItemGroup::~QgsLayoutItemGroup()
 {
-  if ( mLayout )
-    mLayout->undoStack()->beginMacro( tr( "Removed group" ) );
   //loop through group members and remove them from the scene
   for ( QgsLayoutItem *item : qgsAsConst( mItems ) )
   {
@@ -37,10 +35,8 @@ QgsLayoutItemGroup::~QgsLayoutItemGroup()
     if ( mLayout )
       mLayout->removeLayoutItem( item );
     else
-      item->deleteLater();
+      delete item;
   }
-  if ( mLayout )
-    mLayout->undoStack()->endMacro();
 }
 
 int QgsLayoutItemGroup::type() const
@@ -63,6 +59,11 @@ QString QgsLayoutItemGroup::displayName() const
   return tr( "<Group>" );
 }
 
+QgsLayoutItemGroup *QgsLayoutItemGroup::create( QgsLayout *layout, const QVariantMap & )
+{
+  return new QgsLayoutItemGroup( layout );
+}
+
 void QgsLayoutItemGroup::addItem( QgsLayoutItem *item )
 {
   if ( !item )
@@ -107,7 +108,7 @@ QList<QgsLayoutItem *> QgsLayoutItemGroup::items() const
 
 void QgsLayoutItemGroup::setVisibility( const bool visible )
 {
-  if ( mLayout )
+  if ( !shouldBlockUndoCommands() )
     mLayout->undoStack()->beginMacro( tr( "Set group visibility" ) );
   //also set visibility for all items within the group
   for ( QgsLayoutItem *item : qgsAsConst( mItems ) )
@@ -118,7 +119,7 @@ void QgsLayoutItemGroup::setVisibility( const bool visible )
   }
   //lastly set visibility for group item itself
   QgsLayoutItem::setVisibility( visible );
-  if ( mLayout )
+  if ( !shouldBlockUndoCommands() )
     mLayout->undoStack()->endMacro();
 }
 
@@ -127,7 +128,8 @@ void QgsLayoutItemGroup::attemptMove( const QgsLayoutPoint &point )
   if ( !mLayout )
     return;
 
-  mLayout->undoStack()->beginMacro( tr( "Moved group" ) );
+  if ( !shouldBlockUndoCommands() )
+    mLayout->undoStack()->beginMacro( tr( "Moved group" ) );
 
   QPointF scenePoint = mLayout->convertToLayoutUnits( point );
   double deltaX = scenePoint.x() - pos().x();
@@ -139,7 +141,12 @@ void QgsLayoutItemGroup::attemptMove( const QgsLayoutPoint &point )
     if ( !item )
       continue;
 
-    mLayout->undoStack()->beginCommand( item, QString() );
+    std::unique_ptr< QgsAbstractLayoutUndoCommand > command;
+    if ( !shouldBlockUndoCommands() )
+    {
+      command.reset( createCommand( QString(), 0 ) );
+      command->saveBeforeState();
+    }
 
     // need to convert delta from layout units -> item units
     QgsLayoutPoint itemPos = item->positionWithUnits();
@@ -148,11 +155,16 @@ void QgsLayoutItemGroup::attemptMove( const QgsLayoutPoint &point )
     itemPos.setY( itemPos.y() + deltaPos.y() );
     item->attemptMove( itemPos );
 
-    mLayout->undoStack()->endCommand();
+    if ( command )
+    {
+      command->saveAfterState();
+      mLayout->undoStack()->stack()->push( command.release() );
+    }
   }
   //lastly move group item itself
   QgsLayoutItem::attemptMove( point );
-  mLayout->undoStack()->endMacro();
+  if ( !shouldBlockUndoCommands() )
+    mLayout->undoStack()->endMacro();
   resetBoundingRect();
 }
 
@@ -161,7 +173,8 @@ void QgsLayoutItemGroup::attemptResize( const QgsLayoutSize &size )
   if ( !mLayout )
     return;
 
-  mLayout->undoStack()->beginMacro( tr( "Resized group" ) );
+  if ( !shouldBlockUndoCommands() )
+    mLayout->undoStack()->beginMacro( tr( "Resized group" ) );
 
   QRectF oldRect = rect();
   QSizeF newSizeLayoutUnits = mLayout->convertToLayoutUnits( size );
@@ -174,6 +187,13 @@ void QgsLayoutItemGroup::attemptResize( const QgsLayoutSize &size )
     if ( !item )
       continue;
 
+    std::unique_ptr< QgsAbstractLayoutUndoCommand > command;
+    if ( !shouldBlockUndoCommands() )
+    {
+      command.reset( createCommand( QString(), 0 ) );
+      command->saveBeforeState();
+    }
+
     QRectF itemRect = mapRectFromItem( item, item->rect() );
     QgsLayoutUtils::relativeResizeRect( itemRect, oldRect, newRect );
 
@@ -186,11 +206,76 @@ void QgsLayoutItemGroup::attemptResize( const QgsLayoutSize &size )
 
     QgsLayoutSize itemSize = mLayout->convertFromLayoutUnits( itemRect.size(), item->sizeWithUnits().units() );
     item->attemptResize( itemSize );
+
+    if ( command )
+    {
+      command->saveAfterState();
+      mLayout->undoStack()->stack()->push( command.release() );
+    }
   }
   QgsLayoutItem::attemptResize( size );
-  mLayout->undoStack()->endMacro();
+  if ( !shouldBlockUndoCommands() )
+    mLayout->undoStack()->endMacro();
+
+  resetBoundingRect();
+}
+
+bool QgsLayoutItemGroup::writeXml( QDomElement &parentElement, QDomDocument &document, const QgsReadWriteContext &context ) const
+{
+  QDomElement element = document.createElement( QStringLiteral( "LayoutItem" ) );
+  element.setAttribute( QStringLiteral( "type" ), stringType() );
+
+  writePropertiesToElement( element, document, context );
+
+  for ( QgsLayoutItem *item : mItems )
+  {
+    if ( !item )
+      continue;
+
+    QDomElement childItem = document.createElement( QStringLiteral( "ComposerItemGroupElement" ) );
+    childItem.setAttribute( QStringLiteral( "uuid" ), item->uuid() );
+    element.appendChild( childItem );
+  }
+
+  parentElement.appendChild( element );
+
+  return true;
+}
+
+bool QgsLayoutItemGroup::readXml( const QDomElement &itemElement, const QDomDocument &document, const QgsReadWriteContext &context )
+{
+  if ( itemElement.nodeName() != QStringLiteral( "LayoutItem" ) || itemElement.attribute( QStringLiteral( "type" ) ) != stringType() )
+  {
+    return false;
+  }
+
+  bool result = readPropertiesFromElement( itemElement, document, context );
+
+  QList<QgsLayoutItem *> items;
+  mLayout->layoutItems( items );
+
+  QDomNodeList elementNodes = itemElement.elementsByTagName( QStringLiteral( "ComposerItemGroupElement" ) );
+  for ( int i = 0; i < elementNodes.count(); ++i )
+  {
+    QDomNode elementNode = elementNodes.at( i );
+    if ( !elementNode.isElement() )
+      continue;
+
+    QString uuid = elementNode.toElement().attribute( QStringLiteral( "uuid" ) );
+
+    for ( QgsLayoutItem *item : qgsAsConst( items ) )
+    {
+      if ( item && ( item->mUuid == uuid /* TODO || item->mTemplateUuid == uuid */ ) )
+      {
+        addItem( item );
+        break;
+      }
+    }
+  }
 
   resetBoundingRect();
+
+  return result;
 }
 
 void QgsLayoutItemGroup::paint( QPainter *, const QStyleOptionGraphicsItem *, QWidget * )