Fix storing/reading multiframe items (TODO fix undo/redo support)

python/core/layout/qgslayoutframe.sip

@@ -37,8 +37,6 @@ Creates a new QgsLayoutFrame belonging to the specified ``layout``.
 
     virtual QIcon icon() const;
 
-    virtual QString uuid() const;
-
 
     virtual QString displayName() const;
 
@@ -127,6 +125,10 @@ Returns whether the frame is empty.
 
     virtual void drawBackground( QgsRenderContext &context );
 
+    virtual bool writePropertiesToElement( QDomElement &parentElement, QDomDocument &document, const QgsReadWriteContext &context ) const;
+
+    virtual bool readPropertiesFromElement( const QDomElement &itemElement, const QDomDocument &document, const QgsReadWriteContext &context );
+
 
 };
 

src/core/layout/qgslayout.cpp

@@ -777,8 +777,7 @@ QDomElement QgsLayout::writeXml( QDomDocument &document, const QgsReadWriteConte
   {
     if ( const QgsLayoutItem *item = dynamic_cast< const QgsLayoutItem *>( graphicsItem ) )
     {
-      if ( item->type() == QgsLayoutItemRegistry::LayoutPage
-           || item->type() == QgsLayoutItemRegistry::LayoutFrame )
+      if ( item->type() == QgsLayoutItemRegistry::LayoutPage )
         continue;
 
       item->writeXml( element, document, context );
@@ -937,42 +936,6 @@ QList< QgsLayoutItem * > QgsLayout::addItemsFromXml( const QDomElement &parentEl
       pageNumber = mPageCollection->pageNumberForPoint( *position );
     }
   }
-
-  const QDomNodeList layoutItemList = parentElement.childNodes();
-  for ( int i = 0; i < layoutItemList.size(); ++i )
-  {
-    const QDomElement currentItemElem = layoutItemList.at( i ).toElement();
-    if ( currentItemElem.nodeName() != QStringLiteral( "LayoutItem" ) )
-      continue;
-
-    const int itemType = currentItemElem.attribute( QStringLiteral( "type" ) ).toInt();
-    std::unique_ptr< QgsLayoutItem > item( QgsApplication::layoutItemRegistry()->createItem( itemType, this ) );
-    if ( !item )
-    {
-      // e.g. plugin based item which is no longer available
-      continue;
-    }
-
-    item->readXml( currentItemElem, document, context );
-    if ( position )
-    {
-      if ( pasteInPlace )
-      {
-        QgsLayoutPoint posOnPage = QgsLayoutPoint::decodePoint( currentItemElem.attribute( QStringLiteral( "positionOnPage" ) ) );
-        item->attemptMove( posOnPage, true, false, pageNumber );
-      }
-      else
-      {
-        item->attemptMoveBy( pasteShiftPos.x(), pasteShiftPos.y() );
-      }
-    }
-
-    QgsLayoutItem *layoutItem = item.get();
-    addLayoutItem( item.release() );
-    layoutItem->setZValue( layoutItem->zValue() + zOrderOffset );
-    newItems << layoutItem;
-  }
-
   // multiframes
 
   //TODO - fix this. pasting multiframe frame items has no effect
@@ -1008,6 +971,40 @@ QList< QgsLayoutItem * > QgsLayout::addItemsFromXml( const QDomElement &parentEl
     newMultiFrames << m;
   }
 
+  const QDomNodeList layoutItemList = parentElement.childNodes();
+  for ( int i = 0; i < layoutItemList.size(); ++i )
+  {
+    const QDomElement currentItemElem = layoutItemList.at( i ).toElement();
+    if ( currentItemElem.nodeName() != QStringLiteral( "LayoutItem" ) )
+      continue;
+
+    const int itemType = currentItemElem.attribute( QStringLiteral( "type" ) ).toInt();
+    std::unique_ptr< QgsLayoutItem > item( QgsApplication::layoutItemRegistry()->createItem( itemType, this ) );
+    if ( !item )
+    {
+      // e.g. plugin based item which is no longer available
+      continue;
+    }
+
+    item->readXml( currentItemElem, document, context );
+    if ( position )
+    {
+      if ( pasteInPlace )
+      {
+        QgsLayoutPoint posOnPage = QgsLayoutPoint::decodePoint( currentItemElem.attribute( QStringLiteral( "positionOnPage" ) ) );
+        item->attemptMove( posOnPage, true, false, pageNumber );
+      }
+      else
+      {
+        item->attemptMoveBy( pasteShiftPos.x(), pasteShiftPos.y() );
+      }
+    }
+
+    QgsLayoutItem *layoutItem = item.get();
+    addLayoutItem( item.release() );
+    layoutItem->setZValue( layoutItem->zValue() + zOrderOffset );
+    newItems << layoutItem;
+  }
 
   // we now allow items to "post-process", e.g. if they need to setup connections
   // to other items in the layout, which may not have existed at the time the

src/core/layout/qgslayoutframe.cpp

@@ -72,39 +72,6 @@ QgsLayoutSize QgsLayoutFrame::fixedSize() const
   return QgsLayoutSize( mMultiFrame->fixedFrameSize( frameIndex ), QgsUnitTypes::LayoutMillimeters );
 }
 
-#if 0// TODO - save/restore multiframe uuid!
-bool QgsLayoutFrame::writeXml( QDomElement &elem, QDomDocument &doc ) const
-{
-  QDomElement frameElem = doc.createElement( QStringLiteral( "ComposerFrame" ) );
-  frameElem.setAttribute( QStringLiteral( "sectionX" ), QString::number( mSection.x() ) );
-  frameElem.setAttribute( QStringLiteral( "sectionY" ), QString::number( mSection.y() ) );
-  frameElem.setAttribute( QStringLiteral( "sectionWidth" ), QString::number( mSection.width() ) );
-  frameElem.setAttribute( QStringLiteral( "sectionHeight" ), QString::number( mSection.height() ) );
-  frameElem.setAttribute( QStringLiteral( "hidePageIfEmpty" ), mHidePageIfEmpty );
-  frameElem.setAttribute( QStringLiteral( "hideBackgroundIfEmpty" ), mHideBackgroundIfEmpty );
-  elem.appendChild( frameElem );
-
-  return _writeXml( frameElem, doc );
-}
-
-bool QgsLayoutFrame::readXml( const QDomElement &itemElem, const QDomDocument &doc )
-{
-  double x = itemElem.attribute( QStringLiteral( "sectionX" ) ).toDouble();
-  double y = itemElem.attribute( QStringLiteral( "sectionY" ) ).toDouble();
-  double width = itemElem.attribute( QStringLiteral( "sectionWidth" ) ).toDouble();
-  double height = itemElem.attribute( QStringLiteral( "sectionHeight" ) ).toDouble();
-  mSection = QRectF( x, y, width, height );
-  mHidePageIfEmpty = itemElem.attribute( QStringLiteral( "hidePageIfEmpty" ), QStringLiteral( "0" ) ).toInt();
-  mHideBackgroundIfEmpty = itemElem.attribute( QStringLiteral( "hideBackgroundIfEmpty" ), QStringLiteral( "0" ) ).toInt();
-  QDomElement composerItem = itemElem.firstChildElement( QStringLiteral( "ComposerItem" ) );
-  if ( composerItem.isNull() )
-  {
-    return false;
-  }
-  return _readXml( composerItem, doc );
-}
-#endif
-
 int QgsLayoutFrame::type() const
 {
   return QgsLayoutItemRegistry::LayoutFrame;
@@ -118,14 +85,6 @@ QIcon QgsLayoutFrame::icon() const
     return QIcon();
 }
 
-QString QgsLayoutFrame::uuid() const
-{
-  if ( mMultiFrame )
-    return mMultiFrame->uuid() + ':' + mMultiFrame->frameIndex( const_cast< QgsLayoutFrame * >( this ) );
-  else
-    return QgsLayoutItem::uuid();
-}
-
 void QgsLayoutFrame::setHidePageIfEmpty( const bool hidePageIfEmpty )
 {
   mHidePageIfEmpty = hidePageIfEmpty;
@@ -168,10 +127,8 @@ QgsExpressionContext QgsLayoutFrame::createExpressionContext() const
   //start with multiframe's context
   QgsExpressionContext context = mMultiFrame->createExpressionContext();
 
-#if 0 //TODO
   //add frame's individual context
   context.appendScope( QgsExpressionContextUtils::layoutItemScope( this ) );
-#endif
 
   return context;
 }
@@ -226,6 +183,33 @@ void QgsLayoutFrame::drawBackground( QgsRenderContext &context )
   }
 }
 
+bool QgsLayoutFrame::writePropertiesToElement( QDomElement &parentElement, QDomDocument &, const QgsReadWriteContext & ) const
+{
+  parentElement.setAttribute( QStringLiteral( "multiFrame" ), mMultiFrameUuid );
+  parentElement.setAttribute( QStringLiteral( "sectionX" ), QString::number( mSection.x() ) );
+  parentElement.setAttribute( QStringLiteral( "sectionY" ), QString::number( mSection.y() ) );
+  parentElement.setAttribute( QStringLiteral( "sectionWidth" ), QString::number( mSection.width() ) );
+  parentElement.setAttribute( QStringLiteral( "sectionHeight" ), QString::number( mSection.height() ) );
+  parentElement.setAttribute( QStringLiteral( "hidePageIfEmpty" ), mHidePageIfEmpty );
+  parentElement.setAttribute( QStringLiteral( "hideBackgroundIfEmpty" ), mHideBackgroundIfEmpty );
+  return true;
+}
+
+bool QgsLayoutFrame::readPropertiesFromElement( const QDomElement &itemElem, const QDomDocument &, const QgsReadWriteContext & )
+{
+  double x = itemElem.attribute( QStringLiteral( "sectionX" ) ).toDouble();
+  double y = itemElem.attribute( QStringLiteral( "sectionY" ) ).toDouble();
+  double width = itemElem.attribute( QStringLiteral( "sectionWidth" ) ).toDouble();
+  double height = itemElem.attribute( QStringLiteral( "sectionHeight" ) ).toDouble();
+  mSection = QRectF( x, y, width, height );
+  mHidePageIfEmpty = itemElem.attribute( QStringLiteral( "hidePageIfEmpty" ), QStringLiteral( "0" ) ).toInt();
+  mHideBackgroundIfEmpty = itemElem.attribute( QStringLiteral( "hideBackgroundIfEmpty" ), QStringLiteral( "0" ) ).toInt();
+
+  mMultiFrameUuid = itemElem.attribute( QStringLiteral( "multiFrame" ) );
+  mMultiFrame = mLayout->multiFrameByUuid( mMultiFrameUuid );
+  return true;
+}
+
 #if 0 //TODO
 void QgsLayoutFrame::beginItemCommand( const QString &text )
 {

src/core/layout/qgslayoutframe.h

@@ -47,7 +47,6 @@ class CORE_EXPORT QgsLayoutFrame: public QgsLayoutItem
 
     int type() const override;
     QIcon icon() const override;
-    QString uuid() const override;
 
     //Overridden to allow multiframe to set display name
     QString displayName() const override;
@@ -72,8 +71,6 @@ class CORE_EXPORT QgsLayoutFrame: public QgsLayoutItem
 #if 0 //TODO
     void beginItemCommand( const QString &text ) override;
     void endItemCommand() override;
-    bool writeXml( QDomElement &elem, QDomDocument &doc ) const override;
-    bool readXml( const QDomElement &itemElem, const QDomDocument &doc ) override;
 #endif
 
     /**
@@ -124,6 +121,8 @@ class CORE_EXPORT QgsLayoutFrame: public QgsLayoutItem
     void draw( QgsRenderContext &context, const QStyleOptionGraphicsItem *itemStyle = nullptr ) override;
     void drawFrame( QgsRenderContext &context ) override;
     void drawBackground( QgsRenderContext &context ) override;
+    bool writePropertiesToElement( QDomElement &parentElement, QDomDocument &document, const QgsReadWriteContext &context ) const override;
+    bool readPropertiesFromElement( const QDomElement &itemElement, const QDomDocument &document, const QgsReadWriteContext &context ) override;
 
   private:
     QgsLayoutFrame() = delete;

src/core/layout/qgslayoutitemgroup.cpp

@@ -260,18 +260,12 @@ bool QgsLayoutItemGroup::readPropertiesFromElement( const QDomElement &itemEleme
 
 void QgsLayoutItemGroup::finalizeRestoreFromXml()
 {
-  QList<QgsLayoutItem *> items;
-  mLayout->layoutItems( items );
-
   for ( const QString &uuid : qgis::as_const( mItemUuids ) )
   {
-    for ( QgsLayoutItem *item : qgis::as_const( items ) )
+    QgsLayoutItem *item = mLayout->itemByUuid( uuid, true );
+    if ( item )
     {
-      if ( item && ( item->mUuid == uuid /* TODO || item->mTemplateUuid == uuid */ ) )
-      {
-        addItem( item );
-        break;
-      }
+      addItem( item );
     }
   }
 

src/core/layout/qgslayoutmultiframe.cpp

@@ -64,7 +64,7 @@ void QgsLayoutMultiFrame::addFrame( QgsLayoutFrame *frame, bool recalcFrameSizes
   {
     handleFrameRemoval( frame );
   } );
-  if ( mLayout )
+  if ( mLayout && !frame->scene() )
   {
     mLayout->addLayoutItem( frame );
   }
@@ -301,6 +301,14 @@ void QgsLayoutMultiFrame::cancelCommand()
 
 void QgsLayoutMultiFrame::finalizeRestoreFromXml()
 {
+  for ( const QString &uuid : qgis::as_const( mFrameUuids ) )
+  {
+    QgsLayoutItem *item = mLayout->itemByUuid( uuid, true );
+    if ( QgsLayoutFrame *frame = qobject_cast< QgsLayoutFrame * >( item ) )
+    {
+      addFrame( frame );
+    }
+  }
 }
 
 void QgsLayoutMultiFrame::refresh()
@@ -466,6 +474,16 @@ bool QgsLayoutMultiFrame::writeXml( QDomElement &parentElement, QDomDocument &do
   Q_UNUSED( ignoreFrames );
 #endif
 
+  for ( QgsLayoutFrame *frame : mFrameItems )
+  {
+    if ( !frame )
+      continue;
+
+    QDomElement childItem = doc.createElement( QStringLiteral( "childFrame" ) );
+    childItem.setAttribute( QStringLiteral( "uuid" ), frame->uuid() );
+    element.appendChild( childItem );
+  }
+
   writeObjectPropertiesToElement( element, doc, context );
   writePropertiesToElement( element, doc, context );
   parentElement.appendChild( element );
@@ -510,6 +528,18 @@ bool QgsLayoutMultiFrame::readXml( const QDomElement &element, const QDomDocumen
   Q_UNUSED( ignoreFrames );
 #endif
 
+  deleteFrames();
+  mFrameUuids.clear();
+  QDomNodeList elementNodes = element.elementsByTagName( QStringLiteral( "childFrame" ) );
+  for ( int i = 0; i < elementNodes.count(); ++i )
+  {
+    QDomNode elementNode = elementNodes.at( i );
+    if ( !elementNode.isElement() )
+      continue;
+
+    QString uuid = elementNode.toElement().attribute( QStringLiteral( "uuid" ) );
+    mFrameUuids << uuid;
+  }
 
   bool result = readPropertiesFromElement( element, doc, context );
 

src/core/layout/qgslayoutmultiframe.h

@@ -392,6 +392,8 @@ class CORE_EXPORT QgsLayoutMultiFrame: public QgsLayoutObject, public QgsLayoutU
     bool mBlockUpdates = false;
     bool mBlockUndoCommands = false;
 
+    QList< QString > mFrameUuids;
+
     //! Unique id
     QString mUuid;
     friend class QgsLayoutFrame;

tests/src/core/testqgslayout.cpp

@@ -65,7 +65,7 @@ class TestQgsLayout: public QObject
     void clone();
     void legendRestoredFromTemplate();
     void legendRestoredFromTemplateAutoUpdate();
-    // void attributeTableRestoredFromTemplate();
+    void attributeTableRestoredFromTemplate();
     void mapLayersRestoredFromTemplate();
     void mapLayersStyleOverrideRestoredFromTemplate();
     void atlasLayerRestoredFromTemplate();
@@ -1064,7 +1064,6 @@ void TestQgsLayout::legendRestoredFromTemplateAutoUpdate()
   QCOMPARE( model2->data( model->node2index( layerNode2 ), Qt::DisplayRole ).toString(), QString( "points" ) );
 }
 
-#if 0 //TODO
 void TestQgsLayout::attributeTableRestoredFromTemplate()
 {
   // load some layers
@@ -1112,7 +1111,6 @@ void TestQgsLayout::attributeTableRestoredFromTemplate()
 
   QCOMPARE( table2->vectorLayer(), layer3 );
 }
-#endif
 
 void TestQgsLayout::mapLayersRestoredFromTemplate()
 {