[composer] Avoid crash when using redo on multiframe (fix #11351)

Since multiframe changes can remove and create new frame items,
it's not safe to directly manipulate frame items in
QgsComposerItemCommand. Now, commands which apply to a frame always
fetch a reference to the correct frame item directly from the
parent multiframe. Also added unit tests for this crash.

Author: nyalldawson

python/core/composer/qgscomposeritemcommand.sip

@@ -25,7 +25,10 @@ class QgsComposerItemCommand: QUndoCommand
     /**Returns true if previous state and after state are valid and different*/
     bool containsChange() const;
 
-    const QgsComposerItem* item() const;
+    /**Returns the target item the command applies to.
+     * @returns target composer item
+    */
+    QgsComposerItem* item() const;
 
   protected:
     void saveState( QDomDocument& stateDoc ) const;

src/core/composer/qgscomposerhtml.cpp

@@ -481,7 +481,10 @@ bool QgsComposerHtml::writeXML( QDomElement& elem, QDomDocument & doc, bool igno
 
 bool QgsComposerHtml::readXML( const QDomElement& itemElem, const QDomDocument& doc, bool ignoreFrames )
 {
-  deleteFrames();
+  if ( !ignoreFrames )
+  {
+    deleteFrames();
+  }
 
   //first create the frames
   if ( !_readXML( itemElem, doc, ignoreFrames ) )

src/core/composer/qgscomposeritemcommand.cpp

@@ -17,11 +17,26 @@
 
 #include "qgscomposeritemcommand.h"
 #include "qgscomposeritem.h"
+#include "qgscomposerframe.h"
+#include "qgscomposermultiframe.h"
 #include "qgsproject.h"
+#include "qgslogger.h"
 
-QgsComposerItemCommand::QgsComposerItemCommand( QgsComposerItem* item, const QString& text, QUndoCommand* parent ):
-    QUndoCommand( text, parent ), mItem( item ), mFirstRun( true )
+QgsComposerItemCommand::QgsComposerItemCommand( QgsComposerItem* item, const QString& text, QUndoCommand* parent )
+    : QUndoCommand( text, parent )
+    , mItem( item )
+    , mMultiFrame( 0 )
+    , mFrameNumber( 0 )
+    , mFirstRun( true )
 {
+  //is item a frame?
+  QgsComposerFrame* frame = dynamic_cast<QgsComposerFrame*>( mItem );
+  if ( frame )
+  {
+    //store parent multiframe and frame index
+    mMultiFrame = frame->multiFrame();
+    mFrameNumber = mMultiFrame->frameIndex( frame );
+  }
 }
 
 QgsComposerItemCommand::~QgsComposerItemCommand()
@@ -48,6 +63,27 @@ bool QgsComposerItemCommand::containsChange() const
   return !( mPreviousState.isNull() || mAfterState.isNull() || mPreviousState.toString() == mAfterState.toString() );
 }
 
+QgsComposerItem* QgsComposerItemCommand::item() const
+{
+  QgsComposerItem* item = 0;
+  if ( mMultiFrame )
+  {
+    //item is a frame, so it needs to be handled differently
+    //in this case the target item is the matching frame number, as subsequent
+    //changes to the multiframe may have deleted mItem
+    if ( mMultiFrame->frameCount() > mFrameNumber )
+    {
+      item = mMultiFrame->frame( mFrameNumber );
+    }
+  }
+  else if ( mItem )
+  {
+    item = mItem;
+  }
+
+  return item;
+}
+
 void QgsComposerItemCommand::savePreviousState()
 {
   saveState( mPreviousState );
@@ -60,26 +96,38 @@ void QgsComposerItemCommand::saveAfterState()
 
 void QgsComposerItemCommand::saveState( QDomDocument& stateDoc ) const
 {
-  if ( mItem )
+  const QgsComposerItem* source = item();
+  if ( !source )
   {
-    stateDoc.clear();
-    QDomElement documentElement = stateDoc.createElement( "ComposerItemState" );
-    mItem->writeXML( documentElement, stateDoc );
-    stateDoc.appendChild( documentElement );
+    return;
   }
+
+  stateDoc.clear();
+  QDomElement documentElement = stateDoc.createElement( "ComposerItemState" );
+  source->writeXML( documentElement, stateDoc );
+  stateDoc.appendChild( documentElement );
 }
 
 void QgsComposerItemCommand::restoreState( QDomDocument& stateDoc ) const
 {
-  if ( mItem )
+  QgsComposerItem* destItem = item();
+  if ( !destItem )
   {
-    mItem->readXML( stateDoc.documentElement().firstChild().toElement(), stateDoc );
-    mItem->repaint();
-    QgsProject::instance()->dirty( true );
+    return;
   }
+
+  destItem->readXML( stateDoc.documentElement().firstChild().toElement(), stateDoc );
+  destItem->repaint();
+  QgsProject::instance()->dirty( true );
 }
 
-QgsComposerMergeCommand::QgsComposerMergeCommand( Context c, QgsComposerItem* item, const QString& text ): QgsComposerItemCommand( item, text ), mContext( c )
+//
+//QgsComposerMergeCommand
+//
+
+QgsComposerMergeCommand::QgsComposerMergeCommand( Context c, QgsComposerItem* item, const QString& text )
+    : QgsComposerItemCommand( item, text )
+    , mContext( c )
 {
 }
 
@@ -89,11 +137,18 @@ QgsComposerMergeCommand::~QgsComposerMergeCommand()
 
 bool QgsComposerMergeCommand::mergeWith( const QUndoCommand * command )
 {
+  QgsComposerItem* thisItem = item();
+  if ( !thisItem )
+  {
+    return false;
+  }
+
   const QgsComposerItemCommand* c = dynamic_cast<const QgsComposerItemCommand*>( command );
-  if ( !c || mItem != c->item() )
+  if ( !c || thisItem != c->item() )
   {
     return false;
   }
+
   mAfterState = c->afterState();
   return true;
 }

src/core/composer/qgscomposeritemcommand.h

@@ -22,6 +22,7 @@
 #include <QDomDocument>
 
 class QgsComposerItem;
+class QgsComposerMultiFrame;
 
 /**Undo command to undo/redo all composer item related changes*/
 class CORE_EXPORT QgsComposerItemCommand: public QUndoCommand
@@ -46,7 +47,10 @@ class CORE_EXPORT QgsComposerItemCommand: public QUndoCommand
     /**Returns true if previous state and after state are valid and different*/
     bool containsChange() const;
 
-    const QgsComposerItem* item() const { return mItem; }
+    /**Returns the target item the command applies to.
+     * @returns target composer item
+    */
+    QgsComposerItem *item() const;
 
   protected:
     /**Target item of the command*/
@@ -56,6 +60,11 @@ class CORE_EXPORT QgsComposerItemCommand: public QUndoCommand
     /**XML containing the state after executing the command*/
     QDomDocument mAfterState;
 
+    /**Parameters for frame items*/
+    /**Parent multiframe*/
+    QgsComposerMultiFrame* mMultiFrame;
+    int mFrameNumber;
+
     /**Flag to prevent the first redo() if the command is pushed to the undo stack*/
     bool mFirstRun;
 

src/core/composer/qgscomposermultiframe.cpp

@@ -307,7 +307,8 @@ void QgsComposerMultiFrame::removeFrame( int i, const bool removeEmptyPages )
   {
     mIsRecalculatingSize = true;
     int pageNumber = frameItem->page();
-    mComposition->removeComposerItem( frameItem );
+    //remove item, but don't create undo command
+    mComposition->removeComposerItem( frameItem, false );
     //if frame was the only item on the page, remove the page
     if ( removeEmptyPages && mComposition->pageIsEmpty( pageNumber ) )
     {
@@ -385,8 +386,10 @@ bool QgsComposerMultiFrame::_readXML( const QDomElement& itemElem, const QDomDoc
       QDomElement frameElem = frameList.at( i ).toElement();
       QgsComposerFrame* newFrame = new QgsComposerFrame( mComposition, this, 0, 0, 0, 0 );
       newFrame->readXML( frameElem, doc );
-      addFrame( newFrame );
+      addFrame( newFrame, false );
     }
+
+    //TODO - think there should be a recalculateFrameSizes() call here
   }
   return true;
 }

tests/src/core/testqgscomposermultiframe.cpp

@@ -34,6 +34,8 @@ class TestQgsComposerMultiFrame: public QObject
     void addFrame(); //test creating new frame inherits all properties of existing frame
     void frameIsEmpty(); //test if frame is empty works
     void addRemovePage(); //test if page is added and removed for RepeatUntilFinished mode
+    void undoRedo(); //test that combinations of frame/multiframe undo/redo don't crash
+    void undoRedoRemovedFrame(); //test that undo doesn't crash with removed frames
 
   private:
     QgsComposition* mComposition;
@@ -203,6 +205,148 @@ void TestQgsComposerMultiFrame::addRemovePage()
   delete htmlItem;
 }
 
+void TestQgsComposerMultiFrame::undoRedo()
+{
+  QgsComposerHtml* htmlItem = new QgsComposerHtml( mComposition, false );
+  QgsComposerFrame* frame1 = new QgsComposerFrame( mComposition, htmlItem, 0, 0, 100, 200 );
+  htmlItem->addFrame( frame1 );
+  htmlItem->setContentMode( QgsComposerHtml::ManualHtml );
+  htmlItem->setResizeMode( QgsComposerMultiFrame::RepeatUntilFinished );
+
+  //short content, so should fit in one frame
+  htmlItem->setHtml( QString( "<p>Test content</p>" ) );
+  htmlItem->loadHtml();
+
+  //do some combinations of undo/redo commands for both the frame and multiframe
+  //to try to trigger a crash
+  frame1->beginCommand( "move" );
+  frame1->setSceneRect( QRectF( 10, 10, 20, 20 ) );
+  frame1->endCommand();
+  frame1->beginCommand( "outline", QgsComposerMergeCommand::ItemOutlineWidth );
+  frame1->setFrameOutlineWidth( 4.0 );
+  frame1->endCommand();
+  frame1->beginCommand( "outline", QgsComposerMergeCommand::ItemOutlineWidth );
+  frame1->setFrameOutlineWidth( 7.0 );
+  frame1->endCommand();
+
+  //multiframe commands
+  mComposition->beginMultiFrameCommand( htmlItem, "maxbreak" );
+  htmlItem->setMaxBreakDistance( 100 );
+  mComposition->endMultiFrameCommand();
+
+  //another frame command
+  frame1->beginCommand( "bgcolor", QgsComposerMergeCommand::ItemTransparency );
+  frame1->setBackgroundColor( QColor( 255, 255, 0 ) );
+  frame1->endCommand();
+  frame1->beginCommand( "bgcolor", QgsComposerMergeCommand::ItemTransparency );
+  frame1->setBackgroundColor( QColor( 255, 0, 0 ) );
+  frame1->endCommand();
+
+  //undo changes
+
+  //frame bg
+  mComposition->undoStack()->undo();
+  //multiframe max break
+  mComposition->undoStack()->undo();
+  //frame outline width
+  mComposition->undoStack()->undo();
+  //frame move
+  mComposition->undoStack()->undo();
+
+  //check result
+  QCOMPARE( htmlItem->maxBreakDistance(), 10.0 );
+  QCOMPARE( htmlItem->frame( 0 )->frameOutlineWidth(), 0.3 );
+  QCOMPARE( htmlItem->frame( 0 )->pos(), QPointF( 0, 0 ) );
+  QCOMPARE( htmlItem->frame( 0 )->backgroundColor(), QColor( 255, 255, 255 ) );
+
+  //now redo
+
+  //frame move
+  mComposition->undoStack()->redo();
+  //frame outline width
+  mComposition->undoStack()->redo();
+  //multiframe max break
+  mComposition->undoStack()->redo();
+  //frame bg color
+  mComposition->undoStack()->redo();
+
+  //check result
+  QCOMPARE( htmlItem->maxBreakDistance(), 100.0 );
+  QCOMPARE( htmlItem->frame( 0 )->frameOutlineWidth(), 7.0 );
+  QCOMPARE( htmlItem->frame( 0 )->pos(), QPointF( 10, 10 ) );
+  QCOMPARE( htmlItem->frame( 0 )->backgroundColor(), QColor( 255, 0, 0 ) );
+
+  mComposition->removeMultiFrame( htmlItem );
+  delete htmlItem;
+}
+
+
+void TestQgsComposerMultiFrame::undoRedoRemovedFrame()
+{
+  QgsComposerHtml* htmlItem = new QgsComposerHtml( mComposition, false );
+  QgsComposerFrame* frame1 = new QgsComposerFrame( mComposition, htmlItem, 0, 0, 100, 200 );
+  htmlItem->addFrame( frame1 );
+  htmlItem->setContentMode( QgsComposerHtml::ManualHtml );
+  htmlItem->setResizeMode( QgsComposerMultiFrame::RepeatUntilFinished );
+
+  //long content, so should require multiple frames
+  htmlItem->setHtml( QString( "<p style=\"height: 2000px\">Test content</p>" ) );
+  htmlItem->loadHtml();
+
+  QVERIFY( htmlItem->frameCount() > 1 );
+
+  //do a command on the first frame
+  htmlItem->frame( 0 )->beginCommand( "outline", QgsComposerMergeCommand::ItemOutlineWidth );
+  htmlItem->frame( 0 )->setFrameOutlineWidth( 4.0 );
+  htmlItem->frame( 0 )->endCommand();
+  //do a command on the second frame
+  htmlItem->frame( 1 )->beginCommand( "outline", QgsComposerMergeCommand::ItemOutlineWidth );
+  htmlItem->frame( 1 )->setFrameOutlineWidth( 8.0 );
+  htmlItem->frame( 1 )->endCommand();
+
+  //do a multiframe command which removes extra frames
+  mComposition->beginMultiFrameCommand( htmlItem, "source" );
+  htmlItem->setHtml( QString( "<p style=\"height: 20px\">Test content</p>" ) );
+  mComposition->endMultiFrameCommand();
+
+  //wipes the second frame
+  htmlItem->loadHtml();
+
+  QCOMPARE( htmlItem->frameCount(), 1 );
+
+  //undo changes
+
+  //multiframe command
+  mComposition->undoStack()->undo();
+  //frame 2 command
+  mComposition->undoStack()->undo();
+  //frame 1 command
+  mComposition->undoStack()->undo();
+
+  //check result
+  QVERIFY( htmlItem->frameCount() > 1 );
+  QCOMPARE( htmlItem->frame( 0 )->frameOutlineWidth(), 0.3 );
+  QCOMPARE( htmlItem->frame( 1 )->frameOutlineWidth(), 0.3 );
+
+  //now redo
+
+  //frame 1 command
+  mComposition->undoStack()->redo();
+  //frame 2 command
+  mComposition->undoStack()->redo();
+
+  //check result
+  QVERIFY( htmlItem->frameCount() > 1 );
+  QCOMPARE( htmlItem->frame( 0 )->frameOutlineWidth(), 4.0 );
+  QCOMPARE( htmlItem->frame( 1 )->frameOutlineWidth(), 8.0 );
+
+  //multiframe command
+  mComposition->undoStack()->redo();
+  QCOMPARE( htmlItem->frameCount(), 1 );
+
+  mComposition->removeMultiFrame( htmlItem );
+  delete htmlItem;
+}
 
 QTEST_MAIN( TestQgsComposerMultiFrame )
 #include "moc_testqgscomposermultiframe.cxx"