Skip to content

Commit

Permalink
[browser] Fix crash when deleting files quickly
Browse files Browse the repository at this point in the history 
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
nyalldawson committed Oct 11, 2018
1 parent 64b5c2e commit e4b52a4
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 27 deletions.
2 changes: 1 addition & 1 deletion src/core/qgsdataitem.h
View file
Expand Up @@ -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;
Expand Down
30 changes: 21 additions & 9 deletions src/providers/gdal/qgsgdaldataitems.cpp
View file
Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions src/providers/gdal/qgsgdaldataitems.h
View file
Expand Up @@ -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
};

Expand Down
38 changes: 25 additions & 13 deletions src/providers/ogr/qgsogrdataitems.cpp
View file
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/providers/ogr/qgsogrdataitems.h
View file
Expand Up @@ -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;
Expand Down

0 comments on commit e4b52a4

Please sign in to comment.