Fix crashes when rapidly deleting/undeleting objects

Also fix some leaks
qgis · Dec 6, 2017 · ef9e013 · ef9e013
1 parent acb956a
commit ef9e013
Show file tree

Hide file tree

Showing 27 changed files with 86 additions and 46 deletions.
diff --git a/python/core/layout/qgslayoutitem.sip b/python/core/layout/qgslayoutitem.sip
@@ -185,6 +185,12 @@ class QgsLayoutItem : QgsLayoutObject, QGraphicsRectItem, QgsLayoutUndoObjectInt
 
     ~QgsLayoutItem();
 
+    virtual void cleanup();
+%Docstring
+ Called just before a batch of items are deleted, allowing them to run cleanup
+ tasks.
+%End
+
     virtual int type() const;
 
 %Docstring

diff --git a/python/core/layout/qgslayoutitemgroup.sip b/python/core/layout/qgslayoutitemgroup.sip
@@ -24,7 +24,9 @@ class QgsLayoutItemGroup: QgsLayoutItem
 %Docstring
  Constructor for QgsLayoutItemGroup, belonging to the specified ``layout``.
 %End
-    ~QgsLayoutItemGroup();
+
+    virtual void cleanup();
+
 
     virtual int type() const;
 

diff --git a/src/app/layout/qgslayoutattributetablewidget.h b/src/app/layout/qgslayoutattributetablewidget.h
@@ -35,8 +35,8 @@ class QgsLayoutAttributeTableWidget: public QgsLayoutItemBaseWidget, private Ui:
     bool setNewItem( QgsLayoutItem *item ) override;
 
   private:
-    QgsLayoutItemAttributeTable *mTable = nullptr;
-    QgsLayoutFrame *mFrame = nullptr;
+    QPointer< QgsLayoutItemAttributeTable > mTable;
+    QPointer< QgsLayoutFrame > mFrame;
     QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
 
     //! Blocks / unblocks the signals of all GUI elements

diff --git a/src/app/layout/qgslayouthtmlwidget.h b/src/app/layout/qgslayouthtmlwidget.h
@@ -68,8 +68,8 @@ class QgsLayoutHtmlWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLayout
 
     void blockSignals( bool block );
 
-    QgsLayoutItemHtml *mHtml = nullptr;
-    QgsLayoutFrame *mFrame = nullptr;
+    QPointer< QgsLayoutItemHtml > mHtml;
+    QPointer< QgsLayoutFrame > mFrame;
     QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
 
     QgsCodeEditorHTML *mHtmlEditor = nullptr;

diff --git a/src/app/layout/qgslayoutlabelwidget.h b/src/app/layout/qgslayoutlabelwidget.h
@@ -20,8 +20,7 @@
 
 #include "ui_qgslayoutlabelwidgetbase.h"
 #include "qgslayoutitemwidget.h"
-
-class QgsLayoutItemLabel;
+#include "qgslayoutitemlabel.h"
 
 /**
  * \ingroup app
@@ -55,7 +54,7 @@ class QgsLayoutLabelWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLayou
     void justifyClicked();
 
   private:
-    QgsLayoutItemLabel *mLabel = nullptr;
+    QPointer< QgsLayoutItemLabel > mLabel = nullptr;
 
     QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
 

diff --git a/src/app/layout/qgslayoutlegendwidget.h b/src/app/layout/qgslayoutlegendwidget.h
@@ -20,12 +20,10 @@
 
 #include "ui_qgslayoutlegendwidgetbase.h"
 #include "qgslayoutitemwidget.h"
+#include "qgslayoutitemlegend.h"
 #include <QWidget>
 #include <QItemDelegate>
 
-class QgsLayoutItemLegend;
-
-
 /**
  * \ingroup app
  * A widget for setting properties relating to a layout legend.
@@ -111,7 +109,7 @@ class QgsLayoutLegendWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLayo
     QgsLayoutLegendWidget() = delete;
     void blockAllSignals( bool b );
 
-    QgsLayoutItemLegend *mLegend = nullptr;
+    QPointer< QgsLayoutItemLegend > mLegend;
 
     QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
 };

diff --git a/src/app/layout/qgslayoutmapgridwidget.h b/src/app/layout/qgslayoutmapgridwidget.h
@@ -106,8 +106,8 @@ class QgsLayoutMapGridWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLay
     void markerSymbolChanged();
 
   private:
-    QgsLayoutItemMap *mMap = nullptr;
-    QgsLayoutItemMapGrid *mMapGrid = nullptr;
+    QPointer< QgsLayoutItemMap > mMap;
+    QPointer< QgsLayoutItemMapGrid > mMapGrid;
 
     //! Blocks / unblocks the signals of all GUI elements
     void blockAllSignals( bool b );

diff --git a/src/app/layout/qgslayoutmapwidget.h b/src/app/layout/qgslayoutmapwidget.h
@@ -115,7 +115,7 @@ class QgsLayoutMapWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLayoutM
     void mapCrsChanged( const QgsCoordinateReferenceSystem &crs );
     void overviewSymbolChanged();
   private:
-    QgsLayoutItemMap *mMapItem = nullptr;
+    QPointer< QgsLayoutItemMap > mMapItem;
     QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
 
     //! Sets extent of composer map from line edits

diff --git a/src/app/layout/qgslayoutpicturewidget.h b/src/app/layout/qgslayoutpicturewidget.h
@@ -78,7 +78,7 @@ class QgsLayoutPictureWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLay
     void mNorthTypeComboBox_currentIndexChanged( int index );
 
   private:
-    QgsLayoutItemPicture *mPicture = nullptr;
+    QPointer< QgsLayoutItemPicture > mPicture;
     QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
 
 

diff --git a/src/app/layout/qgslayoutpolygonwidget.h b/src/app/layout/qgslayoutpolygonwidget.h
@@ -35,7 +35,7 @@ class QgsLayoutPolygonWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLay
     bool setNewItem( QgsLayoutItem *item ) override;
 
   private:
-    QgsLayoutItemPolygon *mPolygon = nullptr;
+    QPointer< QgsLayoutItemPolygon > mPolygon;
     QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
 
     //! Sets the GUI elements to the currentValues of mComposerShape

diff --git a/src/app/layout/qgslayoutpolylinewidget.h b/src/app/layout/qgslayoutpolylinewidget.h
@@ -35,7 +35,7 @@ class QgsLayoutPolylineWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLa
     bool setNewItem( QgsLayoutItem *item ) override;
 
   private:
-    QgsLayoutItemPolyline *mPolyline = nullptr;
+    QPointer< QgsLayoutItemPolyline > mPolyline;
     QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
 
     void enableStartSvgInputElements( bool enable );

diff --git a/src/app/layout/qgslayoutscalebarwidget.h b/src/app/layout/qgslayoutscalebarwidget.h
@@ -67,7 +67,7 @@ class QgsLayoutScaleBarWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLa
     void fontChanged();
 
   private:
-    QgsLayoutItemScaleBar *mScalebar = nullptr;
+    QPointer< QgsLayoutItemScaleBar > mScalebar;
     QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
 
     QButtonGroup mSegmentSizeRadioGroup;

diff --git a/src/app/layout/qgslayoutshapewidget.h b/src/app/layout/qgslayoutshapewidget.h
@@ -38,7 +38,7 @@ class QgsLayoutShapeWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLayou
 
 
   private:
-    QgsLayoutItemShape *mShape = nullptr;
+    QPointer< QgsLayoutItemShape > mShape;
     QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
 
     //! Blocks / unblocks the signal of all GUI elements

diff --git a/src/core/layout/qgslayout.cpp b/src/core/layout/qgslayout.cpp
@@ -634,7 +634,8 @@ void QgsLayout::removeLayoutItemPrivate( QgsLayoutItem *item )
 #if 0 //TODO
   emit itemRemoved( item );
 #endif
-  delete item;
+  item->cleanup();
+  item->deleteLater();
 }
 
 void QgsLayout::deleteAndRemoveMultiFrames()
@@ -759,17 +760,17 @@ QList< QgsLayoutItem * > QgsLayout::addItemsFromXml( const QDomElement &parentEl
     item->readXml( currentItemElem, document, context );
     if ( position )
     {
-#if 0 //TODO
       if ( pasteInPlacePt )
       {
+#if 0 //TODO
         item->setItemPosition( newLabel->pos().x(), std::fmod( newLabel->pos().y(), ( paperHeight() + spaceBetweenPages() ) ) );
         item->move( pasteInPlacePt->x(), pasteInPlacePt->y() );
+#endif
       }
       else
       {
-        item->move( pasteShiftPos.x(), pasteShiftPos.y() );
+        item->attemptMoveBy( pasteShiftPos.x(), pasteShiftPos.y() );
       }
-#endif
     }
 
     QgsLayoutItem *layoutItem = item.get();
@@ -807,6 +808,8 @@ QList< QgsLayoutItem * > QgsLayout::addItemsFromXml( const QDomElement &parentEl
     {
       QgsLayoutItemFrame * frame = mf->frame( frameIdx );
       frame->setZValue( frame->zValue() + zOrderOffset );
+
+      // also need to shift frames according to position/pasteInPlacePt
     }*/
     newMultiFrames << m;
   }

diff --git a/src/core/layout/qgslayoutitem.cpp b/src/core/layout/qgslayoutitem.cpp
@@ -78,6 +78,11 @@ QgsLayoutItem::QgsLayoutItem( QgsLayout *layout, bool manageZValue )
 }
 
 QgsLayoutItem::~QgsLayoutItem()
+{
+  cleanup();
+}
+
+void QgsLayoutItem::cleanup()
 {
   if ( mLayout && mLayoutManagesZValue )
   {

diff --git a/src/core/layout/qgslayoutitem.h b/src/core/layout/qgslayoutitem.h
@@ -218,6 +218,12 @@ class CORE_EXPORT QgsLayoutItem : public QgsLayoutObject, public QGraphicsRectIt
 
     ~QgsLayoutItem();
 
+    /**
+     * Called just before a batch of items are deleted, allowing them to run cleanup
+     * tasks.
+     */
+    virtual void cleanup();
+
     /**
      * Return unique graphics item type identifier.
      *

diff --git a/src/core/layout/qgslayoutitemgroup.cpp b/src/core/layout/qgslayoutitemgroup.cpp
@@ -23,7 +23,7 @@ QgsLayoutItemGroup::QgsLayoutItemGroup( QgsLayout *layout )
   : QgsLayoutItem( layout )
 {}
 
-QgsLayoutItemGroup::~QgsLayoutItemGroup()
+void QgsLayoutItemGroup::cleanup()
 {
   //loop through group members and remove them from the scene
   for ( QgsLayoutItem *item : qgis::as_const( mItems ) )
@@ -35,8 +35,13 @@ QgsLayoutItemGroup::~QgsLayoutItemGroup()
     if ( mLayout )
       mLayout->removeLayoutItem( item );
     else
-      delete item;
+    {
+      item->cleanup();
+      item->deleteLater();
+    }
   }
+  mItems.clear();
+  QgsLayoutItem::cleanup();
 }
 
 int QgsLayoutItemGroup::type() const

diff --git a/src/core/layout/qgslayoutitemgroup.h b/src/core/layout/qgslayoutitemgroup.h
@@ -35,7 +35,8 @@ class CORE_EXPORT QgsLayoutItemGroup: public QgsLayoutItem
      * Constructor for QgsLayoutItemGroup, belonging to the specified \a layout.
      */
     explicit QgsLayoutItemGroup( QgsLayout *layout );
-    ~QgsLayoutItemGroup();
+
+    void cleanup() override;
 
     int type() const override;
     QString displayName() const override;

diff --git a/src/core/layout/qgslayoutitemregistry.cpp b/src/core/layout/qgslayoutitemregistry.cpp
@@ -41,6 +41,7 @@ QgsLayoutItemRegistry::QgsLayoutItemRegistry( QObject *parent )
 QgsLayoutItemRegistry::~QgsLayoutItemRegistry()
 {
   qDeleteAll( mMetadata );
+  qDeleteAll( mMultiFrameMetadata );
 }
 
 bool QgsLayoutItemRegistry::populate()

diff --git a/src/core/layout/qgslayoutitemundocommand.cpp b/src/core/layout/qgslayoutitemundocommand.cpp
@@ -120,8 +120,10 @@ void QgsLayoutItemDeleteUndoCommand::redo()
   QgsLayoutItem *item = layout()->itemByUuid( itemUuid() );
   //Q_ASSERT_X( item, "QgsLayoutItemDeleteUndoCommand::redo", "could not find item to re-delete!" );
 
+  layout()->undoStack()->blockCommands( true );
   if ( item )
     layout()->removeLayoutItemPrivate( item );
+  layout()->undoStack()->blockCommands( false );
 }
 
 QgsLayoutItemAddItemCommand::QgsLayoutItemAddItemCommand( QgsLayoutItem *item, const QString &text, int id, QUndoCommand *parent )

diff --git a/src/gui/layout/qgslayoutview.cpp b/src/gui/layout/qgslayoutview.cpp
@@ -55,7 +55,6 @@ QgsLayoutView::QgsLayoutView( QWidget *parent )
   mSpacePanTool = new QgsLayoutViewToolTemporaryKeyPan( this );
   mMidMouseButtonPanTool = new QgsLayoutViewToolTemporaryMousePan( this );
   mSpaceZoomTool = new QgsLayoutViewToolTemporaryKeyZoom( this );
-  mSnapMarker = new QgsLayoutViewSnapMarker();
 
   mPreviewEffect = new QgsPreviewEffect( this );
   viewport()->setGraphicsEffect( mPreviewEffect );
@@ -131,7 +130,8 @@ void QgsLayoutView::setTool( QgsLayoutViewTool *tool )
     disconnect( mTool, &QgsLayoutViewTool::itemFocused, this, &QgsLayoutView::itemFocused );
   }
 
-  mSnapMarker->hide();
+  if ( mSnapMarker )
+    mSnapMarker->hide();
   if ( mHorizontalSnapLine )
     mHorizontalSnapLine->hide();
   if ( mVerticalSnapLine )
@@ -785,7 +785,8 @@ void QgsLayoutView::ungroupSelectedItems()
 
 void QgsLayoutView::mousePressEvent( QMouseEvent *event )
 {
-  mSnapMarker->setVisible( false );
+  if ( mSnapMarker )
+    mSnapMarker->setVisible( false );
 
   if ( mTool )
   {
@@ -849,11 +850,16 @@ void QgsLayoutView::mouseMoveEvent( QMouseEvent *event )
       if ( me->isSnapped() )
       {
         cursorPos = me->snappedPoint();
-        mSnapMarker->setPos( me->snappedPoint() );
-        mSnapMarker->setVisible( true );
+        if ( mSnapMarker )
+        {
+          mSnapMarker->setPos( me->snappedPoint() );
+          mSnapMarker->setVisible( true );
+        }
       }
-      else
+      else if ( mSnapMarker )
+      {
         mSnapMarker->setVisible( false );
+      }
     }
     mTool->layoutMoveEvent( me.get() );
     event->setAccepted( me->isAccepted() );

diff --git a/tests/src/core/testqgslayoutitemgroup.cpp b/tests/src/core/testqgslayoutitemgroup.cpp
@@ -80,6 +80,7 @@ void TestQgsLayoutItemGroup::initTestCase()
 
 void TestQgsLayoutItemGroup::cleanupTestCase()
 {
+  QgsApplication::exitQgis();
 }
 
 void TestQgsLayoutItemGroup::init()
@@ -185,6 +186,9 @@ void TestQgsLayoutItemGroup::createGroup()
   QVERIFY( item->isGroupMember() );
   QCOMPARE( item->parentGroup(), group );
   QCOMPARE( item2->parentGroup(), group );
+
+  delete item;
+  delete item2;
 }
 
 void TestQgsLayoutItemGroup::ungroup()

diff --git a/tests/src/core/testqgslayoutmap.cpp b/tests/src/core/testqgslayoutmap.cpp
@@ -351,10 +351,10 @@ void TestQgsLayoutMap::dataDefinedLayers()
   QCOMPARE( result.count(), 1 );
   QCOMPARE( result.at( 0 ), mPointsLayer );
   mComposition->atlasComposition().setEnabled( false );
-  delete atlasLayer;
-
 #endif
 
+  delete atlasLayer;
+
   //render test
   map->dataDefinedProperties().setProperty( QgsLayoutObject::MapLayers, QgsProperty::fromExpression(
         QStringLiteral( "'%1|%2'" ).arg( mPolysLayer->name(), mPointsLayer->name() ) ) );

diff --git a/tests/src/core/testqgslayoutshapes.cpp b/tests/src/core/testqgslayoutshapes.cpp
@@ -226,7 +226,7 @@ void TestQgsLayoutShapes::readWriteXml()
 {
   QgsProject p;
   QgsLayout l( &p );
-  QgsLayoutItemShape *shape = new QgsLayoutItemShape( &l );
+  std::unique_ptr< QgsLayoutItemShape > shape = qgis::make_unique< QgsLayoutItemShape >( &l );
   shape->setShapeType( QgsLayoutItemShape::Triangle );
   QgsSimpleFillSymbolLayer *simpleFill = new QgsSimpleFillSymbolLayer();
   QgsFillSymbol *fillSymbol = new QgsFillSymbol();
@@ -235,6 +235,7 @@ void TestQgsLayoutShapes::readWriteXml()
   simpleFill->setStrokeColor( Qt::yellow );
   simpleFill->setStrokeWidth( 6 );
   shape->setSymbol( fillSymbol );
+  delete fillSymbol;
 
   //save original item to xml
   QDomImplementation DomImplementation;
@@ -247,7 +248,7 @@ void TestQgsLayoutShapes::readWriteXml()
   shape->writeXml( rootNode, doc, QgsReadWriteContext() );
 
   //create new item and restore settings from xml
-  QgsLayoutItemShape *copy = new QgsLayoutItemShape( &l );
+  std::unique_ptr< QgsLayoutItemShape > copy = qgis::make_unique< QgsLayoutItemShape >( &l );
   QVERIFY( copy->readXml( rootNode.firstChildElement(), doc, QgsReadWriteContext() ) );
   QCOMPARE( copy->shapeType(), QgsLayoutItemShape::Triangle );
   QCOMPARE( copy->symbol()->symbolLayer( 0 )->color().name(), QStringLiteral( "#00ff00" ) );
@@ -258,7 +259,7 @@ void TestQgsLayoutShapes::bounds()
 {
   QgsProject p;
   QgsLayout l( &p );
-  QgsLayoutItemShape *shape = new QgsLayoutItemShape( &l );
+  std::unique_ptr< QgsLayoutItemShape > shape = qgis::make_unique< QgsLayoutItemShape >( &l );
   shape->attemptMove( QgsLayoutPoint( 20, 20 ) );
   shape->attemptResize( QgsLayoutSize( 150, 100 ) );
 
@@ -269,6 +270,7 @@ void TestQgsLayoutShapes::bounds()
   simpleFill->setStrokeColor( Qt::yellow );
   simpleFill->setStrokeWidth( 6 );
   shape->setSymbol( fillSymbol );
+  delete fillSymbol;
 
   // scene bounding rect should include symbol outline
   QRectF bounds = shape->sceneBoundingRect();