Fix crash with undo/redo and layout multiframes, restore test

python/core/layout/qgslayoutmultiframe.sip.in

@@ -176,26 +176,26 @@ Returns the resize mode for the multiframe.
 .. seealso:: :py:func:`setResizeMode`
 %End
 
-    bool writeXml( QDomElement &parentElement, QDomDocument &document, const QgsReadWriteContext &context, bool ignoreFrames = false ) const;
+    bool writeXml( QDomElement &parentElement, QDomDocument &document, const QgsReadWriteContext &context, bool includeFrames = false ) const;
 %Docstring
 Stores the multiframe state in a DOM element.
 
 :param parentElement: parent DOM element (e.g. 'Layout' element)
 :param document: DOM document
 :param context: read write context
-:param ignoreFrames: set to false to avoid writing state information about child frames into DOM
+:param includeFrames: set to true to write state information about child frames into DOM
 
 .. seealso:: :py:func:`readXml`
 %End
 
-    bool readXml( const QDomElement &itemElement, const QDomDocument &document, const QgsReadWriteContext &context, bool ignoreFrames = false );
+    bool readXml( const QDomElement &itemElement, const QDomDocument &document, const QgsReadWriteContext &context, bool includeFrames = false );
 %Docstring
 Sets the item state from a DOM element.
 
 :param itemElement: is the DOM node corresponding to item (e.g. 'LayoutItem' element)
 :param document: DOM document
 :param context: read write context
-:param ignoreFrames: set to false to avoid read state information about child frames from DOM
+:param includeFrames: set to true to read state information about child frames from DOM
 
 .. seealso:: :py:func:`writeXml`
 %End

src/app/layout/qgslayoutattributetablewidget.cpp

@@ -394,7 +394,7 @@ void QgsLayoutAttributeTableWidget::mBackgroundColorButton_colorChanged( const Q
 
 void QgsLayoutAttributeTableWidget::updateGuiElements()
 {
-  if ( !mTable )
+  if ( !mTable || !mFrame )
   {
     return;
   }

src/app/layout/qgslayouthtmlwidget.cpp

@@ -419,7 +419,7 @@ void QgsLayoutHtmlWidget::mAddFramePushButton_clicked()
 
 void QgsLayoutHtmlWidget::setGuiElementValues()
 {
-  if ( !mHtml )
+  if ( !mHtml || !mFrame )
   {
     return;
   }

src/core/layout/qgslayoutmultiframe.cpp

@@ -453,34 +453,26 @@ int QgsLayoutMultiFrame::frameIndex( QgsLayoutFrame *frame ) const
   return mFrameItems.indexOf( frame );
 }
 
-bool QgsLayoutMultiFrame::writeXml( QDomElement &parentElement, QDomDocument &doc, const QgsReadWriteContext &context, bool ignoreFrames ) const
+bool QgsLayoutMultiFrame::writeXml( QDomElement &parentElement, QDomDocument &doc, const QgsReadWriteContext &context, bool includeFrames ) const
 {
   QDomElement element = doc.createElement( QStringLiteral( "LayoutMultiFrame" ) );
   element.setAttribute( QStringLiteral( "resizeMode" ), mResizeMode );
   element.setAttribute( QStringLiteral( "uuid" ), mUuid );
   element.setAttribute( QStringLiteral( "type" ), type() );
 
-#if 0 //TODO
-
-  if ( !ignoreFrames )
-  {
-    QList<QgsComposerFrame *>::const_iterator frameIt = mFrameItems.constBegin();
-    for ( ; frameIt != mFrameItems.constEnd(); ++frameIt )
-    {
-      ( *frameIt )->writeXml( elem, doc );
-    }
-  }
-#else
-  Q_UNUSED( ignoreFrames );
-#endif
-
   for ( QgsLayoutFrame *frame : mFrameItems )
   {
     if ( !frame )
       continue;
 
     QDomElement childItem = doc.createElement( QStringLiteral( "childFrame" ) );
     childItem.setAttribute( QStringLiteral( "uuid" ), frame->uuid() );
+
+    if ( includeFrames )
+    {
+      frame->writeXml( childItem, doc, context );
+    }
+
     element.appendChild( childItem );
   }
 
@@ -490,7 +482,7 @@ bool QgsLayoutMultiFrame::writeXml( QDomElement &parentElement, QDomDocument &do
   return true;
 }
 
-bool QgsLayoutMultiFrame::readXml( const QDomElement &element, const QDomDocument &doc, const QgsReadWriteContext &context, bool ignoreFrames )
+bool QgsLayoutMultiFrame::readXml( const QDomElement &element, const QDomDocument &doc, const QgsReadWriteContext &context, bool includeFrames )
 {
   if ( element.nodeName() != QStringLiteral( "LayoutMultiFrame" ) )
   {
@@ -504,29 +496,6 @@ bool QgsLayoutMultiFrame::readXml( const QDomElement &element, const QDomDocumen
 
   mUuid = element.attribute( QStringLiteral( "uuid" ), QUuid::createUuid().toString() );
   mResizeMode = static_cast< ResizeMode >( element.attribute( QStringLiteral( "resizeMode" ), QStringLiteral( "0" ) ).toInt() );
-#if 0 //TODO
-  if ( !ignoreFrames )
-  {
-    deleteFrames();
-  }
-
-
-  if ( !ignoreFrames )
-  {
-    QDomNodeList frameList = itemElem.elementsByTagName( QStringLiteral( "ComposerFrame" ) );
-    for ( int i = 0; i < frameList.size(); ++i )
-    {
-      QDomElement frameElem = frameList.at( i ).toElement();
-      QgsComposerFrame *newFrame = new QgsComposerFrame( mComposition, this, 0, 0, 0, 0 );
-      newFrame->readXml( frameElem, doc );
-      addFrame( newFrame, false );
-    }
-
-    //TODO - think there should be a recalculateFrameSizes() call here
-  }
-#else
-  Q_UNUSED( ignoreFrames );
-#endif
 
   deleteFrames();
   mFrameUuids.clear();
@@ -537,8 +506,22 @@ bool QgsLayoutMultiFrame::readXml( const QDomElement &element, const QDomDocumen
     if ( !elementNode.isElement() )
       continue;
 
-    QString uuid = elementNode.toElement().attribute( QStringLiteral( "uuid" ) );
+    QDomElement frameElement = elementNode.toElement();
+
+    QString uuid = frameElement.attribute( QStringLiteral( "uuid" ) );
     mFrameUuids << uuid;
+
+    if ( includeFrames )
+    {
+      QDomNodeList frameNodes = frameElement.elementsByTagName( QStringLiteral( "LayoutItem" ) );
+      if ( !frameNodes.isEmpty() )
+      {
+        QDomElement frameItemElement = frameNodes.at( 0 ).toElement();
+        std::unique_ptr< QgsLayoutFrame > newFrame = qgis::make_unique< QgsLayoutFrame >( mLayout, this );
+        newFrame->readXml( frameItemElement, doc, context );
+        addFrame( newFrame.release(), false );
+      }
+    }
   }
 
   bool result = readPropertiesFromElement( element, doc, context );

src/core/layout/qgslayoutmultiframe.h

@@ -194,20 +194,20 @@ class CORE_EXPORT QgsLayoutMultiFrame: public QgsLayoutObject, public QgsLayoutU
      * \param parentElement parent DOM element (e.g. 'Layout' element)
      * \param document DOM document
      * \param context read write context
-     * \param ignoreFrames set to false to avoid writing state information about child frames into DOM
+     * \param includeFrames set to true to write state information about child frames into DOM
      * \see readXml()
      */
-    bool writeXml( QDomElement &parentElement, QDomDocument &document, const QgsReadWriteContext &context, bool ignoreFrames = false ) const;
+    bool writeXml( QDomElement &parentElement, QDomDocument &document, const QgsReadWriteContext &context, bool includeFrames = false ) const;
 
     /**
      * Sets the item state from a DOM element.
      * \param itemElement is the DOM node corresponding to item (e.g. 'LayoutItem' element)
      * \param document DOM document
      * \param context read write context
-     * \param ignoreFrames set to false to avoid read state information about child frames from DOM
+     * \param includeFrames set to true to read state information about child frames from DOM
      * \see writeXml()
      */
-    bool readXml( const QDomElement &itemElement, const QDomDocument &document, const QgsReadWriteContext &context, bool ignoreFrames = false );
+    bool readXml( const QDomElement &itemElement, const QDomDocument &document, const QgsReadWriteContext &context, bool includeFrames = false );
 
     /**
      * Returns a list of all child frames for this multiframe.

src/core/layout/qgslayoutmultiframeundocommand.cpp

@@ -56,7 +56,7 @@ void QgsLayoutMultiFrameUndoCommand::saveState( QDomDocument &stateDoc ) const
   QgsLayoutMultiFrame *item = mLayout->multiFrameByUuid( mFrameUuid );
   Q_ASSERT_X( item, "QgsLayoutMultiFrameUndoCommand::saveState", "could not retrieve item for saving state" );
 
-  item->writeXml( documentElement, stateDoc, QgsReadWriteContext() );
+  item->writeXml( documentElement, stateDoc, QgsReadWriteContext(), true );
   stateDoc.appendChild( documentElement );
 }
 
@@ -70,7 +70,7 @@ void QgsLayoutMultiFrameUndoCommand::restoreState( QDomDocument &stateDoc )
     item = recreateItem( mItemType, mLayout );
   }
 
-  item->readXml( stateDoc.documentElement().firstChild().toElement(), stateDoc, QgsReadWriteContext() );
+  item->readXml( stateDoc.documentElement().firstChild().toElement(), stateDoc, QgsReadWriteContext(), true );
   mLayout->project()->setDirty( true );
 }
 

tests/src/core/testqgslayoutmultiframe.cpp

@@ -44,11 +44,9 @@ class TestQgsLayoutMultiFrame : public QObject
     void displayName();
     void frameIsEmpty(); //test if frame is empty works
     void addRemovePage(); //test if page is added and removed for RepeatUntilFinished mode
-#if 0 //TODO
     void undoRedo(); //test that combinations of frame/multiframe undo/redo don't crash
     void undoRedoRemovedFrame(); //test that undo doesn't crash with removed frames
     void undoRedoRemovedFrame2();
-#endif
     void registry();
     void deleteFrame();
     void writeReadXml();
@@ -344,7 +342,7 @@ void TestQgsLayoutMultiFrame::addRemovePage()
   mLayout->removeMultiFrame( htmlItem );
   delete htmlItem;
 }
-#if 0//TODO
+
 void TestQgsLayoutMultiFrame::undoRedo()
 {
   QgsLayoutItemHtml *htmlItem = new QgsLayoutItemHtml( mLayout );
@@ -513,7 +511,7 @@ void TestQgsLayoutMultiFrame::undoRedoRemovedFrame2()
   htmlItem->addFrame( frame1 );
 
 }
-#endif
+
 void TestQgsLayoutMultiFrame::registry()
 {
   // test QgsLayoutItemRegistry