Skip to content

Commit e4b52a4

Browse files
nyalldawsonnyalldawson
committedOct 11, 2018
[browser] Fix crash when deleting files quickly
It's possible for items to be deleted in the background while we are waiting on user input (e.g. from the confirmation prompt) by a parent directory refresh, so we need to capture everything relevant and not rely on the item itself existing anymore during the delete layer calls.
·
ltr-3_40final-3_4_0
1 parent 64b5c2e commit e4b52a4

File tree

5 files changed

+52
-27
lines changed

5 files changed

+52
-27
lines changed
 

‎src/core/qgsdataitem.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,7 @@ class CORE_EXPORT QgsDataItem : public QObject
322322

323323
Type mType;
324324
Capabilities mCapabilities;
325-
QPointer< QgsDataItem > mParent;
325+
QgsDataItem *mParent = nullptr;
326326
QVector<QgsDataItem *> mChildren; // easier to have it always
327327
State mState;
328328
QString mName;

‎src/providers/gdal/qgsgdaldataitems.cpp

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -126,53 +126,65 @@ QString QgsGdalLayerItem::layerName() const
126126
#ifdef HAVE_GUI
127127
QList<QAction *> QgsGdalLayerItem::actions( QWidget *parent )
128128
{
129-
QList<QAction *> lst;
129+
QList<QAction *> lst = QgsLayerItem::actions( parent );
130+
130131
// Messages are different for files and tables
131132
const QString message = QObject::tr( "Delete File “%1”…" ).arg( mName );
132133
QAction *actionDeleteLayer = new QAction( message, parent );
133-
connect( actionDeleteLayer, &QAction::triggered, this, &QgsGdalLayerItem::deleteLayer );
134+
135+
// IMPORTANT - we need to capture the stuff we need, and then hand the slot
136+
// off to a static method. This is because it's possible for this item
137+
// to be deleted in the background on us (e.g. by a parent directory refresh)
138+
const QString uri = mUri;
139+
const QString path = mPath;
140+
QPointer< QgsDataItem > parentItem( mParent );
141+
connect( actionDeleteLayer, &QAction::triggered, this, [ uri, path, parentItem ]
142+
{
143+
deleteLayer( uri, path, parentItem );
144+
} );
145+
134146
lst.append( actionDeleteLayer );
135147
return lst;
136148
}
137149

138-
void QgsGdalLayerItem::deleteLayer()
150+
void QgsGdalLayerItem::deleteLayer( const QString &uri, const QString &path, QPointer< QgsDataItem > parent )
139151
{
140152
const QString title = QObject::tr( "Delete File" );
141153
// Check if the layer is in the project
142154
const QgsMapLayer *projectLayer = nullptr;
143155
const auto mapLayers = QgsProject::instance()->mapLayers();
144156
for ( auto it = mapLayers.constBegin(); it != mapLayers.constEnd(); ++it )
145157
{
146-
if ( it.value()->publicSource() == mUri )
158+
if ( it.value()->publicSource() == uri )
147159
{
148160
projectLayer = it.value();
149161
}
150162
}
151163
if ( ! projectLayer )
152164
{
153-
const QString confirmMessage = QObject::tr( "Are you sure you want to delete file '%1'?" ).arg( mPath );
165+
const QString confirmMessage = QObject::tr( "Are you sure you want to delete file '%1'?" ).arg( path );
154166

155167
if ( QMessageBox::question( nullptr, title,
156168
confirmMessage,
157169
QMessageBox::Yes | QMessageBox::No, QMessageBox::No ) != QMessageBox::Yes )
158170
return;
159171

160172

161-
if ( !QFile::remove( mPath ) )
173+
if ( !QFile::remove( path ) )
162174
{
163175
QMessageBox::warning( nullptr, title, tr( "Could not delete file." ) );
164176
}
165177
else
166178
{
167179
QMessageBox::information( nullptr, title, tr( "File deleted successfully." ) );
168-
if ( mParent )
169-
mParent->refresh();
180+
if ( parent )
181+
parent->refresh();
170182
}
171183
}
172184
else
173185
{
174186
QMessageBox::warning( nullptr, title, QObject::tr( "The layer '%1' cannot be deleted because it is in the current project as '%2',"
175-
" remove it from the project and retry." ).arg( mName, projectLayer->name() ) );
187+
" remove it from the project and retry." ).arg( path, projectLayer->name() ) );
176188
}
177189
}
178190
#endif

‎src/providers/gdal/qgsgdaldataitems.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,9 @@ class QgsGdalLayerItem : public QgsLayerItem
4040

4141
#ifdef HAVE_GUI
4242
QList<QAction *> actions( QWidget *parent ) override;
43-
public slots:
44-
void deleteLayer();
43+
44+
static void deleteLayer( const QString &uri, const QString &path, QPointer< QgsDataItem > parent );
45+
4546
#endif
4647
};
4748

‎src/providers/ogr/qgsogrdataitems.cpp

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -309,61 +309,73 @@ QString QgsOgrLayerItem::layerName() const
309309
#ifdef HAVE_GUI
310310
QList<QAction *> QgsOgrLayerItem::actions( QWidget *parent )
311311
{
312-
QList<QAction *> lst;
312+
QList<QAction *> lst = QgsLayerItem::actions( parent );
313+
313314
// Messages are different for files and tables
314315
QString message = mIsSubLayer ? QObject::tr( "Delete Layer “%1”…" ).arg( mName ) : QObject::tr( "Delete File “%1”…" ).arg( mName );
315316
QAction *actionDeleteLayer = new QAction( message, parent );
316-
connect( actionDeleteLayer, &QAction::triggered, this, &QgsOgrLayerItem::deleteLayer );
317+
318+
// IMPORTANT - we need to capture the stuff we need, and then hand the slot
319+
// off to a static method. This is because it's possible for this item
320+
// to be deleted in the background on us (e.g. by a parent directory refresh)
321+
const bool isSubLayer = mIsSubLayer;
322+
const QString uri = mUri;
323+
const QString name = mName;
324+
QPointer< QgsDataItem > parentItem( mParent );
325+
connect( actionDeleteLayer, &QAction::triggered, this, [ isSubLayer, uri, name, parentItem ]
326+
{
327+
deleteLayer( isSubLayer, uri, name, parentItem );
328+
} );
317329
lst.append( actionDeleteLayer );
318330
return lst;
319331
}
320332

321-
void QgsOgrLayerItem::deleteLayer()
333+
void QgsOgrLayerItem::deleteLayer( const bool isSubLayer, const QString &uri, const QString &name, QPointer< QgsDataItem > parent )
322334
{
323335
// Messages are different for files and tables
324-
QString title = mIsSubLayer ? QObject::tr( "Delete Layer" ) : QObject::tr( "Delete File" );
336+
QString title = isSubLayer ? QObject::tr( "Delete Layer" ) : QObject::tr( "Delete File" );
325337
// Check if the layer is in the registry
326338
const QgsMapLayer *projectLayer = nullptr;
327339
Q_FOREACH ( const QgsMapLayer *layer, QgsProject::instance()->mapLayers() )
328340
{
329-
if ( layer->publicSource() == mUri )
341+
if ( layer->publicSource() == uri )
330342
{
331343
projectLayer = layer;
332344
}
333345
}
334346
if ( ! projectLayer )
335347
{
336348
QString confirmMessage;
337-
if ( mIsSubLayer )
349+
if ( isSubLayer )
338350
{
339-
confirmMessage = QObject::tr( "Are you sure you want to delete layer '%1' from datasource?" ).arg( mName );
351+
confirmMessage = QObject::tr( "Are you sure you want to delete layer '%1' from datasource?" ).arg( name );
340352
}
341353
else
342354
{
343-
confirmMessage = QObject::tr( "Are you sure you want to delete file '%1'?" ).arg( mUri );
355+
confirmMessage = QObject::tr( "Are you sure you want to delete file '%1'?" ).arg( uri );
344356
}
345357
if ( QMessageBox::question( nullptr, title,
346358
confirmMessage,
347359
QMessageBox::Yes | QMessageBox::No, QMessageBox::No ) != QMessageBox::Yes )
348360
return;
349361

350362
QString errCause;
351-
bool res = ::deleteLayer( mUri, errCause );
363+
bool res = ::deleteLayer( uri, errCause );
352364
if ( !res )
353365
{
354366
QMessageBox::warning( nullptr, title, errCause );
355367
}
356368
else
357369
{
358-
QMessageBox::information( nullptr, title, mIsSubLayer ? tr( "Layer deleted successfully." ) : tr( "File deleted successfully." ) );
359-
if ( mParent )
360-
mParent->refresh();
370+
QMessageBox::information( nullptr, title, isSubLayer ? tr( "Layer deleted successfully." ) : tr( "File deleted successfully." ) );
371+
if ( parent )
372+
parent->refresh();
361373
}
362374
}
363375
else
364376
{
365377
QMessageBox::warning( nullptr, title, QObject::tr( "The layer '%1' cannot be deleted because it is in the current project as '%2',"
366-
" remove it from the project and retry." ).arg( mName, projectLayer->name() ) );
378+
" remove it from the project and retry." ).arg( name, projectLayer->name() ) );
367379
}
368380
}
369381
#endif

‎src/providers/ogr/qgsogrdataitems.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,8 @@ class QgsOgrLayerItem : public QgsLayerItem
6969

7070
#ifdef HAVE_GUI
7171
QList<QAction *> actions( QWidget *parent ) override;
72-
public slots:
73-
void deleteLayer();
72+
73+
static void deleteLayer( bool isSubLayer, const QString &uri, const QString &name, QPointer< QgsDataItem > parent );
7474
#endif
7575
private:
7676
bool mIsSubLayer;

0 commit comments

Comments
 (0)
Please sign in to comment.