Skip to content
Permalink
Browse files

[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.
  • Loading branch information
nyalldawson committed Oct 11, 2018
1 parent 64b5c2e commit e4b52a4d21209e445e1f923a1f4fca32d56c83f4
@@ -322,7 +322,7 @@ class CORE_EXPORT QgsDataItem : public QObject

Type mType;
Capabilities mCapabilities;
QPointer< QgsDataItem > mParent;
QgsDataItem *mParent = nullptr;
QVector<QgsDataItem *> mChildren; // easier to have it always
State mState;
QString mName;
@@ -126,53 +126,65 @@ QString QgsGdalLayerItem::layerName() const
#ifdef HAVE_GUI
QList<QAction *> QgsGdalLayerItem::actions( QWidget *parent )
{
QList<QAction *> lst;
QList<QAction *> 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
const QgsMapLayer *projectLayer = nullptr;
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,
QMessageBox::Yes | QMessageBox::No, QMessageBox::No ) != QMessageBox::Yes )
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
@@ -40,8 +40,9 @@ class QgsGdalLayerItem : public QgsLayerItem

#ifdef HAVE_GUI
QList<QAction *> actions( QWidget *parent ) override;
public slots:
void deleteLayer();

static void deleteLayer( const QString &uri, const QString &path, QPointer< QgsDataItem > parent );

#endif
};

@@ -309,61 +309,73 @@ QString QgsOgrLayerItem::layerName() const
#ifdef HAVE_GUI
QList<QAction *> QgsOgrLayerItem::actions( QWidget *parent )
{
QList<QAction *> lst;
QList<QAction *> 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;
}
}
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,
QMessageBox::Yes | QMessageBox::No, QMessageBox::No ) != QMessageBox::Yes )
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
@@ -69,8 +69,8 @@ class QgsOgrLayerItem : public QgsLayerItem

#ifdef HAVE_GUI
QList<QAction *> 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;

0 comments on commit e4b52a4

Please sign in to comment.
You can’t perform that action at this time.