Fix crash when deleting multiframe item child frames

qgis · Jan 7, 2018 · 1b96926 · 1b96926
1 parent 3f414e2
commit 1b96926
Show file tree

Hide file tree

Showing 7 changed files with 45 additions and 19 deletions.
diff --git a/python/core/layout/qgslayoutframe.sip b/python/core/layout/qgslayoutframe.sip
@@ -43,6 +43,9 @@ Creates a new QgsLayoutFrame belonging to the specified ``layout``.
     virtual QString displayName() const;
 
 
+    virtual void cleanup();
+
+
     void setContentSection( const QRectF &section );
 %Docstring
 Sets the visible part of the multiframe's content which is visible within

diff --git a/python/core/layout/qgslayoutmultiframe.sip b/python/core/layout/qgslayoutmultiframe.sip
@@ -383,20 +383,6 @@ in finalizeRestoreFromXml(), not readPropertiesFromElement().
 
 
 
-  protected slots:
-
-    void handlePageChange();
-%Docstring
-Adapts to changed number of layout pages if resize type is RepeatOnEveryPage.
-%End
-
-    void handleFrameRemoval();
-%Docstring
-Called when a frame is removed. Updates frame list and recalculates
-content of remaining frames.
-%End
-
-
 };
 
 

diff --git a/src/core/layout/qgslayoutframe.cpp b/src/core/layout/qgslayoutframe.cpp
@@ -192,6 +192,14 @@ QString QgsLayoutFrame::displayName() const
   return tr( "<Frame>" );
 }
 
+void QgsLayoutFrame::cleanup()
+{
+  if ( mMultiFrame )
+    mMultiFrame->handleFrameRemoval( this );
+
+  QgsLayoutItem::cleanup();
+}
+
 void QgsLayoutFrame::draw( QgsRenderContext &context, const QStyleOptionGraphicsItem *itemStyle )
 {
   if ( mMultiFrame )

diff --git a/src/core/layout/qgslayoutframe.h b/src/core/layout/qgslayoutframe.h
@@ -52,6 +52,8 @@ class CORE_EXPORT QgsLayoutFrame: public QgsLayoutItem
     //Overridden to allow multiframe to set display name
     QString displayName() const override;
 
+    void cleanup() override;
+
     /**
      * Sets the visible part of the multiframe's content which is visible within
      * this frame (relative to the total multiframe extent in layout units).

diff --git a/src/core/layout/qgslayoutmultiframe.cpp b/src/core/layout/qgslayoutmultiframe.cpp
@@ -60,7 +60,10 @@ void QgsLayoutMultiFrame::addFrame( QgsLayoutFrame *frame, bool recalcFrameSizes
   mFrameItems.push_back( frame );
   frame->mMultiFrame = this;
   connect( frame, &QgsLayoutItem::sizePositionChanged, this, &QgsLayoutMultiFrame::recalculateFrameSizes );
-  connect( frame, &QgsLayoutFrame::destroyed, this, &QgsLayoutMultiFrame::handleFrameRemoval );
+  connect( frame, &QgsLayoutFrame::destroyed, this, [this, frame ]
+  {
+    handleFrameRemoval( frame );
+  } );
   if ( mLayout )
   {
     mLayout->addLayoutItem( frame );
@@ -306,12 +309,11 @@ void QgsLayoutMultiFrame::refresh()
   refreshDataDefinedProperty();
 }
 
-void QgsLayoutMultiFrame::handleFrameRemoval()
+void QgsLayoutMultiFrame::handleFrameRemoval( QgsLayoutFrame *frame )
 {
   if ( mBlockUpdates )
     return;
 
-  QgsLayoutFrame *frame = qobject_cast<QgsLayoutFrame *>( sender() );
   if ( !frame )
   {
     return;

diff --git a/src/core/layout/qgslayoutmultiframe.h b/src/core/layout/qgslayoutmultiframe.h
@@ -370,7 +370,7 @@ class CORE_EXPORT QgsLayoutMultiFrame: public QgsLayoutObject, public QgsLayoutU
 
     ResizeMode mResizeMode = UseExistingFrames;
 
-  protected slots:
+  private slots:
 
     /**
      * Adapts to changed number of layout pages if resize type is RepeatOnEveryPage.
@@ -381,7 +381,7 @@ class CORE_EXPORT QgsLayoutMultiFrame: public QgsLayoutObject, public QgsLayoutU
      * Called when a frame is removed. Updates frame list and recalculates
      * content of remaining frames.
      */
-    void handleFrameRemoval();
+    void handleFrameRemoval( QgsLayoutFrame *frame );
 
 
   private:
@@ -394,6 +394,7 @@ class CORE_EXPORT QgsLayoutMultiFrame: public QgsLayoutObject, public QgsLayoutU
 
     //! Unique id
     QString mUuid;
+    friend class QgsLayoutFrame;
 };
 
 

diff --git a/tests/src/core/testqgslayoutmultiframe.cpp b/tests/src/core/testqgslayoutmultiframe.cpp
@@ -47,6 +47,7 @@ class TestQgsLayoutMultiFrame : public QObject
     void undoRedoRemovedFrame(); //test that undo doesn't crash with removed frames
     void undoRedoRemovedFrame2();
     void registry();
+    void deleteFrame();
 
   private:
     QgsLayout *mLayout = nullptr;
@@ -556,5 +557,28 @@ void TestQgsLayoutMultiFrame::registry()
   QVERIFY( props.isEmpty() );
 }
 
+void TestQgsLayoutMultiFrame::deleteFrame()
+{
+  QgsLayout l( QgsProject::instance() );
+  l.initializeDefaults();
+
+  QgsLayoutItemHtml *htmlItem = new QgsLayoutItemHtml( &l );
+  QgsLayoutFrame *frame1 = new QgsLayoutFrame( &l, htmlItem );
+  frame1->attemptSetSceneRect( QRectF( 0, 0, 100, 200 ) );
+  htmlItem->addFrame( frame1 );
+  QgsLayoutFrame *frame2 = new QgsLayoutFrame( &l, htmlItem );
+  frame2->attemptSetSceneRect( QRectF( 0, 0, 100, 200 ) );
+  htmlItem->addFrame( frame2 );
+
+  QCOMPARE( htmlItem->frameCount(), 2 );
+  QCOMPARE( htmlItem->frames(), QList< QgsLayoutFrame * >() << frame1 << frame2 );
+  l.removeLayoutItem( frame1 );
+  QCOMPARE( htmlItem->frameCount(), 1 );
+  QCOMPARE( htmlItem->frames(), QList< QgsLayoutFrame * >() << frame2 );
+  l.removeLayoutItem( frame2 );
+  QCOMPARE( htmlItem->frameCount(), 0 );
+  QVERIFY( htmlItem->frames().empty() );
+}
+
 QGSTEST_MAIN( TestQgsLayoutMultiFrame )
 #include "testqgslayoutmultiframe.moc"