Skip to content

Commit ef9e013

Browse files
committed
Fix crashes when rapidly deleting/undeleting objects
Also fix some leaks
1 parent acb956a commit ef9e013

27 files changed

+86
-46
lines changed

python/core/layout/qgslayoutitem.sip

+6
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,12 @@ class QgsLayoutItem : QgsLayoutObject, QGraphicsRectItem, QgsLayoutUndoObjectInt
185185

186186
~QgsLayoutItem();
187187

188+
virtual void cleanup();
189+
%Docstring
190+
Called just before a batch of items are deleted, allowing them to run cleanup
191+
tasks.
192+
%End
193+
188194
virtual int type() const;
189195

190196
%Docstring

python/core/layout/qgslayoutitemgroup.sip

+3-1
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,9 @@ class QgsLayoutItemGroup: QgsLayoutItem
2424
%Docstring
2525
Constructor for QgsLayoutItemGroup, belonging to the specified ``layout``.
2626
%End
27-
~QgsLayoutItemGroup();
27+
28+
virtual void cleanup();
29+
2830

2931
virtual int type() const;
3032

src/app/layout/qgslayoutattributetablewidget.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@ class QgsLayoutAttributeTableWidget: public QgsLayoutItemBaseWidget, private Ui:
3535
bool setNewItem( QgsLayoutItem *item ) override;
3636

3737
private:
38-
QgsLayoutItemAttributeTable *mTable = nullptr;
39-
QgsLayoutFrame *mFrame = nullptr;
38+
QPointer< QgsLayoutItemAttributeTable > mTable;
39+
QPointer< QgsLayoutFrame > mFrame;
4040
QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
4141

4242
//! Blocks / unblocks the signals of all GUI elements

src/app/layout/qgslayouthtmlwidget.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,8 @@ class QgsLayoutHtmlWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLayout
6868

6969
void blockSignals( bool block );
7070

71-
QgsLayoutItemHtml *mHtml = nullptr;
72-
QgsLayoutFrame *mFrame = nullptr;
71+
QPointer< QgsLayoutItemHtml > mHtml;
72+
QPointer< QgsLayoutFrame > mFrame;
7373
QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
7474

7575
QgsCodeEditorHTML *mHtmlEditor = nullptr;

src/app/layout/qgslayoutlabelwidget.h

+2-3
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,7 @@
2020

2121
#include "ui_qgslayoutlabelwidgetbase.h"
2222
#include "qgslayoutitemwidget.h"
23-
24-
class QgsLayoutItemLabel;
23+
#include "qgslayoutitemlabel.h"
2524

2625
/**
2726
* \ingroup app
@@ -55,7 +54,7 @@ class QgsLayoutLabelWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLayou
5554
void justifyClicked();
5655

5756
private:
58-
QgsLayoutItemLabel *mLabel = nullptr;
57+
QPointer< QgsLayoutItemLabel > mLabel = nullptr;
5958

6059
QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
6160

src/app/layout/qgslayoutlegendwidget.h

+2-4
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,10 @@
2020

2121
#include "ui_qgslayoutlegendwidgetbase.h"
2222
#include "qgslayoutitemwidget.h"
23+
#include "qgslayoutitemlegend.h"
2324
#include <QWidget>
2425
#include <QItemDelegate>
2526

26-
class QgsLayoutItemLegend;
27-
28-
2927
/**
3028
* \ingroup app
3129
* A widget for setting properties relating to a layout legend.
@@ -111,7 +109,7 @@ class QgsLayoutLegendWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLayo
111109
QgsLayoutLegendWidget() = delete;
112110
void blockAllSignals( bool b );
113111

114-
QgsLayoutItemLegend *mLegend = nullptr;
112+
QPointer< QgsLayoutItemLegend > mLegend;
115113

116114
QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
117115
};

src/app/layout/qgslayoutmapgridwidget.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -106,8 +106,8 @@ class QgsLayoutMapGridWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLay
106106
void markerSymbolChanged();
107107

108108
private:
109-
QgsLayoutItemMap *mMap = nullptr;
110-
QgsLayoutItemMapGrid *mMapGrid = nullptr;
109+
QPointer< QgsLayoutItemMap > mMap;
110+
QPointer< QgsLayoutItemMapGrid > mMapGrid;
111111

112112
//! Blocks / unblocks the signals of all GUI elements
113113
void blockAllSignals( bool b );

src/app/layout/qgslayoutmapwidget.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ class QgsLayoutMapWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLayoutM
115115
void mapCrsChanged( const QgsCoordinateReferenceSystem &crs );
116116
void overviewSymbolChanged();
117117
private:
118-
QgsLayoutItemMap *mMapItem = nullptr;
118+
QPointer< QgsLayoutItemMap > mMapItem;
119119
QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
120120

121121
//! Sets extent of composer map from line edits

src/app/layout/qgslayoutpicturewidget.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ class QgsLayoutPictureWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLay
7878
void mNorthTypeComboBox_currentIndexChanged( int index );
7979

8080
private:
81-
QgsLayoutItemPicture *mPicture = nullptr;
81+
QPointer< QgsLayoutItemPicture > mPicture;
8282
QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
8383

8484

src/app/layout/qgslayoutpolygonwidget.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ class QgsLayoutPolygonWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLay
3535
bool setNewItem( QgsLayoutItem *item ) override;
3636

3737
private:
38-
QgsLayoutItemPolygon *mPolygon = nullptr;
38+
QPointer< QgsLayoutItemPolygon > mPolygon;
3939
QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
4040

4141
//! Sets the GUI elements to the currentValues of mComposerShape

src/app/layout/qgslayoutpolylinewidget.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ class QgsLayoutPolylineWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLa
3535
bool setNewItem( QgsLayoutItem *item ) override;
3636

3737
private:
38-
QgsLayoutItemPolyline *mPolyline = nullptr;
38+
QPointer< QgsLayoutItemPolyline > mPolyline;
3939
QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
4040

4141
void enableStartSvgInputElements( bool enable );

src/app/layout/qgslayoutscalebarwidget.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ class QgsLayoutScaleBarWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLa
6767
void fontChanged();
6868

6969
private:
70-
QgsLayoutItemScaleBar *mScalebar = nullptr;
70+
QPointer< QgsLayoutItemScaleBar > mScalebar;
7171
QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
7272

7373
QButtonGroup mSegmentSizeRadioGroup;

src/app/layout/qgslayoutshapewidget.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ class QgsLayoutShapeWidget: public QgsLayoutItemBaseWidget, private Ui::QgsLayou
3838

3939

4040
private:
41-
QgsLayoutItemShape *mShape = nullptr;
41+
QPointer< QgsLayoutItemShape > mShape;
4242
QgsLayoutItemPropertiesWidget *mItemPropertiesWidget = nullptr;
4343

4444
//! Blocks / unblocks the signal of all GUI elements

src/core/layout/qgslayout.cpp

+7-4
Original file line numberDiff line numberDiff line change
@@ -634,7 +634,8 @@ void QgsLayout::removeLayoutItemPrivate( QgsLayoutItem *item )
634634
#if 0 //TODO
635635
emit itemRemoved( item );
636636
#endif
637-
delete item;
637+
item->cleanup();
638+
item->deleteLater();
638639
}
639640

640641
void QgsLayout::deleteAndRemoveMultiFrames()
@@ -759,17 +760,17 @@ QList< QgsLayoutItem * > QgsLayout::addItemsFromXml( const QDomElement &parentEl
759760
item->readXml( currentItemElem, document, context );
760761
if ( position )
761762
{
762-
#if 0 //TODO
763763
if ( pasteInPlacePt )
764764
{
765+
#if 0 //TODO
765766
item->setItemPosition( newLabel->pos().x(), std::fmod( newLabel->pos().y(), ( paperHeight() + spaceBetweenPages() ) ) );
766767
item->move( pasteInPlacePt->x(), pasteInPlacePt->y() );
768+
#endif
767769
}
768770
else
769771
{
770-
item->move( pasteShiftPos.x(), pasteShiftPos.y() );
772+
item->attemptMoveBy( pasteShiftPos.x(), pasteShiftPos.y() );
771773
}
772-
#endif
773774
}
774775

775776
QgsLayoutItem *layoutItem = item.get();
@@ -807,6 +808,8 @@ QList< QgsLayoutItem * > QgsLayout::addItemsFromXml( const QDomElement &parentEl
807808
{
808809
QgsLayoutItemFrame * frame = mf->frame( frameIdx );
809810
frame->setZValue( frame->zValue() + zOrderOffset );
811+
812+
// also need to shift frames according to position/pasteInPlacePt
810813
}*/
811814
newMultiFrames << m;
812815
}

src/core/layout/qgslayoutitem.cpp

+5
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,11 @@ QgsLayoutItem::QgsLayoutItem( QgsLayout *layout, bool manageZValue )
7878
}
7979

8080
QgsLayoutItem::~QgsLayoutItem()
81+
{
82+
cleanup();
83+
}
84+
85+
void QgsLayoutItem::cleanup()
8186
{
8287
if ( mLayout && mLayoutManagesZValue )
8388
{

src/core/layout/qgslayoutitem.h

+6
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,12 @@ class CORE_EXPORT QgsLayoutItem : public QgsLayoutObject, public QGraphicsRectIt
218218

219219
~QgsLayoutItem();
220220

221+
/**
222+
* Called just before a batch of items are deleted, allowing them to run cleanup
223+
* tasks.
224+
*/
225+
virtual void cleanup();
226+
221227
/**
222228
* Return unique graphics item type identifier.
223229
*

src/core/layout/qgslayoutitemgroup.cpp

+7-2
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ QgsLayoutItemGroup::QgsLayoutItemGroup( QgsLayout *layout )
2323
: QgsLayoutItem( layout )
2424
{}
2525

26-
QgsLayoutItemGroup::~QgsLayoutItemGroup()
26+
void QgsLayoutItemGroup::cleanup()
2727
{
2828
//loop through group members and remove them from the scene
2929
for ( QgsLayoutItem *item : qgis::as_const( mItems ) )
@@ -35,8 +35,13 @@ QgsLayoutItemGroup::~QgsLayoutItemGroup()
3535
if ( mLayout )
3636
mLayout->removeLayoutItem( item );
3737
else
38-
delete item;
38+
{
39+
item->cleanup();
40+
item->deleteLater();
41+
}
3942
}
43+
mItems.clear();
44+
QgsLayoutItem::cleanup();
4045
}
4146

4247
int QgsLayoutItemGroup::type() const

src/core/layout/qgslayoutitemgroup.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,8 @@ class CORE_EXPORT QgsLayoutItemGroup: public QgsLayoutItem
3535
* Constructor for QgsLayoutItemGroup, belonging to the specified \a layout.
3636
*/
3737
explicit QgsLayoutItemGroup( QgsLayout *layout );
38-
~QgsLayoutItemGroup();
38+
39+
void cleanup() override;
3940

4041
int type() const override;
4142
QString displayName() const override;

src/core/layout/qgslayoutitemregistry.cpp

+1
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ QgsLayoutItemRegistry::QgsLayoutItemRegistry( QObject *parent )
4141
QgsLayoutItemRegistry::~QgsLayoutItemRegistry()
4242
{
4343
qDeleteAll( mMetadata );
44+
qDeleteAll( mMultiFrameMetadata );
4445
}
4546

4647
bool QgsLayoutItemRegistry::populate()

src/core/layout/qgslayoutitemundocommand.cpp

+2
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,10 @@ void QgsLayoutItemDeleteUndoCommand::redo()
120120
QgsLayoutItem *item = layout()->itemByUuid( itemUuid() );
121121
//Q_ASSERT_X( item, "QgsLayoutItemDeleteUndoCommand::redo", "could not find item to re-delete!" );
122122

123+
layout()->undoStack()->blockCommands( true );
123124
if ( item )
124125
layout()->removeLayoutItemPrivate( item );
126+
layout()->undoStack()->blockCommands( false );
125127
}
126128

127129
QgsLayoutItemAddItemCommand::QgsLayoutItemAddItemCommand( QgsLayoutItem *item, const QString &text, int id, QUndoCommand *parent )

src/gui/layout/qgslayoutview.cpp

+12-6
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,6 @@ QgsLayoutView::QgsLayoutView( QWidget *parent )
5555
mSpacePanTool = new QgsLayoutViewToolTemporaryKeyPan( this );
5656
mMidMouseButtonPanTool = new QgsLayoutViewToolTemporaryMousePan( this );
5757
mSpaceZoomTool = new QgsLayoutViewToolTemporaryKeyZoom( this );
58-
mSnapMarker = new QgsLayoutViewSnapMarker();
5958

6059
mPreviewEffect = new QgsPreviewEffect( this );
6160
viewport()->setGraphicsEffect( mPreviewEffect );
@@ -131,7 +130,8 @@ void QgsLayoutView::setTool( QgsLayoutViewTool *tool )
131130
disconnect( mTool, &QgsLayoutViewTool::itemFocused, this, &QgsLayoutView::itemFocused );
132131
}
133132

134-
mSnapMarker->hide();
133+
if ( mSnapMarker )
134+
mSnapMarker->hide();
135135
if ( mHorizontalSnapLine )
136136
mHorizontalSnapLine->hide();
137137
if ( mVerticalSnapLine )
@@ -785,7 +785,8 @@ void QgsLayoutView::ungroupSelectedItems()
785785

786786
void QgsLayoutView::mousePressEvent( QMouseEvent *event )
787787
{
788-
mSnapMarker->setVisible( false );
788+
if ( mSnapMarker )
789+
mSnapMarker->setVisible( false );
789790

790791
if ( mTool )
791792
{
@@ -849,11 +850,16 @@ void QgsLayoutView::mouseMoveEvent( QMouseEvent *event )
849850
if ( me->isSnapped() )
850851
{
851852
cursorPos = me->snappedPoint();
852-
mSnapMarker->setPos( me->snappedPoint() );
853-
mSnapMarker->setVisible( true );
853+
if ( mSnapMarker )
854+
{
855+
mSnapMarker->setPos( me->snappedPoint() );
856+
mSnapMarker->setVisible( true );
857+
}
854858
}
855-
else
859+
else if ( mSnapMarker )
860+
{
856861
mSnapMarker->setVisible( false );
862+
}
857863
}
858864
mTool->layoutMoveEvent( me.get() );
859865
event->setAccepted( me->isAccepted() );

tests/src/core/testqgslayoutitemgroup.cpp

+4
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ void TestQgsLayoutItemGroup::initTestCase()
8080

8181
void TestQgsLayoutItemGroup::cleanupTestCase()
8282
{
83+
QgsApplication::exitQgis();
8384
}
8485

8586
void TestQgsLayoutItemGroup::init()
@@ -185,6 +186,9 @@ void TestQgsLayoutItemGroup::createGroup()
185186
QVERIFY( item->isGroupMember() );
186187
QCOMPARE( item->parentGroup(), group );
187188
QCOMPARE( item2->parentGroup(), group );
189+
190+
delete item;
191+
delete item2;
188192
}
189193

190194
void TestQgsLayoutItemGroup::ungroup()

tests/src/core/testqgslayoutmap.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -351,10 +351,10 @@ void TestQgsLayoutMap::dataDefinedLayers()
351351
QCOMPARE( result.count(), 1 );
352352
QCOMPARE( result.at( 0 ), mPointsLayer );
353353
mComposition->atlasComposition().setEnabled( false );
354-
delete atlasLayer;
355-
356354
#endif
357355

356+
delete atlasLayer;
357+
358358
//render test
359359
map->dataDefinedProperties().setProperty( QgsLayoutObject::MapLayers, QgsProperty::fromExpression(
360360
QStringLiteral( "'%1|%2'" ).arg( mPolysLayer->name(), mPointsLayer->name() ) ) );

tests/src/core/testqgslayoutshapes.cpp

+5-3
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ void TestQgsLayoutShapes::readWriteXml()
226226
{
227227
QgsProject p;
228228
QgsLayout l( &p );
229-
QgsLayoutItemShape *shape = new QgsLayoutItemShape( &l );
229+
std::unique_ptr< QgsLayoutItemShape > shape = qgis::make_unique< QgsLayoutItemShape >( &l );
230230
shape->setShapeType( QgsLayoutItemShape::Triangle );
231231
QgsSimpleFillSymbolLayer *simpleFill = new QgsSimpleFillSymbolLayer();
232232
QgsFillSymbol *fillSymbol = new QgsFillSymbol();
@@ -235,6 +235,7 @@ void TestQgsLayoutShapes::readWriteXml()
235235
simpleFill->setStrokeColor( Qt::yellow );
236236
simpleFill->setStrokeWidth( 6 );
237237
shape->setSymbol( fillSymbol );
238+
delete fillSymbol;
238239

239240
//save original item to xml
240241
QDomImplementation DomImplementation;
@@ -247,7 +248,7 @@ void TestQgsLayoutShapes::readWriteXml()
247248
shape->writeXml( rootNode, doc, QgsReadWriteContext() );
248249

249250
//create new item and restore settings from xml
250-
QgsLayoutItemShape *copy = new QgsLayoutItemShape( &l );
251+
std::unique_ptr< QgsLayoutItemShape > copy = qgis::make_unique< QgsLayoutItemShape >( &l );
251252
QVERIFY( copy->readXml( rootNode.firstChildElement(), doc, QgsReadWriteContext() ) );
252253
QCOMPARE( copy->shapeType(), QgsLayoutItemShape::Triangle );
253254
QCOMPARE( copy->symbol()->symbolLayer( 0 )->color().name(), QStringLiteral( "#00ff00" ) );
@@ -258,7 +259,7 @@ void TestQgsLayoutShapes::bounds()
258259
{
259260
QgsProject p;
260261
QgsLayout l( &p );
261-
QgsLayoutItemShape *shape = new QgsLayoutItemShape( &l );
262+
std::unique_ptr< QgsLayoutItemShape > shape = qgis::make_unique< QgsLayoutItemShape >( &l );
262263
shape->attemptMove( QgsLayoutPoint( 20, 20 ) );
263264
shape->attemptResize( QgsLayoutSize( 150, 100 ) );
264265

@@ -269,6 +270,7 @@ void TestQgsLayoutShapes::bounds()
269270
simpleFill->setStrokeColor( Qt::yellow );
270271
simpleFill->setStrokeWidth( 6 );
271272
shape->setSymbol( fillSymbol );
273+
delete fillSymbol;
272274

273275
// scene bounding rect should include symbol outline
274276
QRectF bounds = shape->sceneBoundingRect();

0 commit comments

Comments
 (0)