From e4b52a4d21209e445e1f923a1f4fca32d56c83f4 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Thu, 11 Oct 2018 12:47:01 +1000 Subject: [PATCH] [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. --- src/core/qgsdataitem.h | 2 +- src/providers/gdal/qgsgdaldataitems.cpp | 30 +++++++++++++------ src/providers/gdal/qgsgdaldataitems.h | 5 ++-- src/providers/ogr/qgsogrdataitems.cpp | 38 ++++++++++++++++--------- src/providers/ogr/qgsogrdataitems.h | 4 +-- 5 files changed, 52 insertions(+), 27 deletions(-) diff --git a/src/core/qgsdataitem.h b/src/core/qgsdataitem.h index 3a3d0b52577e..f0c58119bad4 100644 --- a/src/core/qgsdataitem.h +++ b/src/core/qgsdataitem.h @@ -322,7 +322,7 @@ class CORE_EXPORT QgsDataItem : public QObject Type mType; Capabilities mCapabilities; - QPointer< QgsDataItem > mParent; + QgsDataItem *mParent = nullptr; QVector mChildren; // easier to have it always State mState; QString mName; diff --git a/src/providers/gdal/qgsgdaldataitems.cpp b/src/providers/gdal/qgsgdaldataitems.cpp index 14a1bcdeea04..40b67f2d93b5 100644 --- a/src/providers/gdal/qgsgdaldataitems.cpp +++ b/src/providers/gdal/qgsgdaldataitems.cpp @@ -126,16 +126,28 @@ QString QgsGdalLayerItem::layerName() const #ifdef HAVE_GUI QList QgsGdalLayerItem::actions( QWidget *parent ) { - QList lst; + QList lst = QgsLayerItem::actions( parent ); + // Messages are different for files and tables const QString message = QObject::tr( "Delete File “%1”…" ).arg( mName ); QAction *actionDeleteLayer = new QAction( message, parent ); - connect( actionDeleteLayer, &QAction::triggered, this, &QgsGdalLayerItem::deleteLayer ); + + // IMPORTANT - we need to capture the stuff we need, and then hand the slot + // off to a static method. This is because it's possible for this item + // to be deleted in the background on us (e.g. by a parent directory refresh) + const QString uri = mUri; + const QString path = mPath; + QPointer< QgsDataItem > parentItem( mParent ); + connect( actionDeleteLayer, &QAction::triggered, this, [ uri, path, parentItem ] + { + deleteLayer( uri, path, parentItem ); + } ); + lst.append( actionDeleteLayer ); return lst; } -void QgsGdalLayerItem::deleteLayer() +void QgsGdalLayerItem::deleteLayer( const QString &uri, const QString &path, QPointer< QgsDataItem > parent ) { const QString title = QObject::tr( "Delete File" ); // Check if the layer is in the project @@ -143,14 +155,14 @@ void QgsGdalLayerItem::deleteLayer() const auto mapLayers = QgsProject::instance()->mapLayers(); for ( auto it = mapLayers.constBegin(); it != mapLayers.constEnd(); ++it ) { - if ( it.value()->publicSource() == mUri ) + if ( it.value()->publicSource() == uri ) { projectLayer = it.value(); } } if ( ! projectLayer ) { - const QString confirmMessage = QObject::tr( "Are you sure you want to delete file '%1'?" ).arg( mPath ); + const QString confirmMessage = QObject::tr( "Are you sure you want to delete file '%1'?" ).arg( path ); if ( QMessageBox::question( nullptr, title, confirmMessage, @@ -158,21 +170,21 @@ void QgsGdalLayerItem::deleteLayer() return; - if ( !QFile::remove( mPath ) ) + if ( !QFile::remove( path ) ) { QMessageBox::warning( nullptr, title, tr( "Could not delete file." ) ); } else { QMessageBox::information( nullptr, title, tr( "File deleted successfully." ) ); - if ( mParent ) - mParent->refresh(); + if ( parent ) + parent->refresh(); } } else { QMessageBox::warning( nullptr, title, QObject::tr( "The layer '%1' cannot be deleted because it is in the current project as '%2'," - " remove it from the project and retry." ).arg( mName, projectLayer->name() ) ); + " remove it from the project and retry." ).arg( path, projectLayer->name() ) ); } } #endif diff --git a/src/providers/gdal/qgsgdaldataitems.h b/src/providers/gdal/qgsgdaldataitems.h index c3b6071d26c5..ef73ea48605f 100644 --- a/src/providers/gdal/qgsgdaldataitems.h +++ b/src/providers/gdal/qgsgdaldataitems.h @@ -40,8 +40,9 @@ class QgsGdalLayerItem : public QgsLayerItem #ifdef HAVE_GUI QList actions( QWidget *parent ) override; - public slots: - void deleteLayer(); + + static void deleteLayer( const QString &uri, const QString &path, QPointer< QgsDataItem > parent ); + #endif }; diff --git a/src/providers/ogr/qgsogrdataitems.cpp b/src/providers/ogr/qgsogrdataitems.cpp index 2f608476d6d5..befdfdda2db0 100644 --- a/src/providers/ogr/qgsogrdataitems.cpp +++ b/src/providers/ogr/qgsogrdataitems.cpp @@ -309,24 +309,36 @@ QString QgsOgrLayerItem::layerName() const #ifdef HAVE_GUI QList QgsOgrLayerItem::actions( QWidget *parent ) { - QList lst; + QList lst = QgsLayerItem::actions( parent ); + // Messages are different for files and tables QString message = mIsSubLayer ? QObject::tr( "Delete Layer “%1”…" ).arg( mName ) : QObject::tr( "Delete File “%1”…" ).arg( mName ); QAction *actionDeleteLayer = new QAction( message, parent ); - connect( actionDeleteLayer, &QAction::triggered, this, &QgsOgrLayerItem::deleteLayer ); + + // IMPORTANT - we need to capture the stuff we need, and then hand the slot + // off to a static method. This is because it's possible for this item + // to be deleted in the background on us (e.g. by a parent directory refresh) + const bool isSubLayer = mIsSubLayer; + const QString uri = mUri; + const QString name = mName; + QPointer< QgsDataItem > parentItem( mParent ); + connect( actionDeleteLayer, &QAction::triggered, this, [ isSubLayer, uri, name, parentItem ] + { + deleteLayer( isSubLayer, uri, name, parentItem ); + } ); lst.append( actionDeleteLayer ); return lst; } -void QgsOgrLayerItem::deleteLayer() +void QgsOgrLayerItem::deleteLayer( const bool isSubLayer, const QString &uri, const QString &name, QPointer< QgsDataItem > parent ) { // Messages are different for files and tables - QString title = mIsSubLayer ? QObject::tr( "Delete Layer" ) : QObject::tr( "Delete File" ); + QString title = isSubLayer ? QObject::tr( "Delete Layer" ) : QObject::tr( "Delete File" ); // Check if the layer is in the registry const QgsMapLayer *projectLayer = nullptr; Q_FOREACH ( const QgsMapLayer *layer, QgsProject::instance()->mapLayers() ) { - if ( layer->publicSource() == mUri ) + if ( layer->publicSource() == uri ) { projectLayer = layer; } @@ -334,13 +346,13 @@ void QgsOgrLayerItem::deleteLayer() if ( ! projectLayer ) { QString confirmMessage; - if ( mIsSubLayer ) + if ( isSubLayer ) { - confirmMessage = QObject::tr( "Are you sure you want to delete layer '%1' from datasource?" ).arg( mName ); + confirmMessage = QObject::tr( "Are you sure you want to delete layer '%1' from datasource?" ).arg( name ); } else { - confirmMessage = QObject::tr( "Are you sure you want to delete file '%1'?" ).arg( mUri ); + confirmMessage = QObject::tr( "Are you sure you want to delete file '%1'?" ).arg( uri ); } if ( QMessageBox::question( nullptr, title, confirmMessage, @@ -348,22 +360,22 @@ void QgsOgrLayerItem::deleteLayer() return; QString errCause; - bool res = ::deleteLayer( mUri, errCause ); + bool res = ::deleteLayer( uri, errCause ); if ( !res ) { QMessageBox::warning( nullptr, title, errCause ); } else { - QMessageBox::information( nullptr, title, mIsSubLayer ? tr( "Layer deleted successfully." ) : tr( "File deleted successfully." ) ); - if ( mParent ) - mParent->refresh(); + QMessageBox::information( nullptr, title, isSubLayer ? tr( "Layer deleted successfully." ) : tr( "File deleted successfully." ) ); + if ( parent ) + parent->refresh(); } } else { QMessageBox::warning( nullptr, title, QObject::tr( "The layer '%1' cannot be deleted because it is in the current project as '%2'," - " remove it from the project and retry." ).arg( mName, projectLayer->name() ) ); + " remove it from the project and retry." ).arg( name, projectLayer->name() ) ); } } #endif diff --git a/src/providers/ogr/qgsogrdataitems.h b/src/providers/ogr/qgsogrdataitems.h index 9f673bcdf1f2..c24cf51ffa11 100644 --- a/src/providers/ogr/qgsogrdataitems.h +++ b/src/providers/ogr/qgsogrdataitems.h @@ -69,8 +69,8 @@ class QgsOgrLayerItem : public QgsLayerItem #ifdef HAVE_GUI QList actions( QWidget *parent ) override; - public slots: - void deleteLayer(); + + static void deleteLayer( bool isSubLayer, const QString &uri, const QString &name, QPointer< QgsDataItem > parent ); #endif private: bool mIsSubLayer;