Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete selected tables as DataItem action #8507

Merged
merged 14 commits into from Jan 8, 2019
Merged
6 changes: 6 additions & 0 deletions python/core/auto_generated/qgsdataitem.sip.in
Expand Up @@ -201,6 +201,7 @@ Items that return valid URI will be returned in mime data when dragging a select
Fast,
Collapse,
Rename,
Delete,
};
typedef QFlags<QgsDataItem::Capability> Capabilities;

Expand Down Expand Up @@ -485,6 +486,11 @@ Returns the string representation of the given ``layerType``
Returns the icon name of the given ``layerType``

.. versionadded:: 3
%End

virtual bool deleteLayer();
%Docstring
Delete this layer item
%End

protected:
Expand Down
28 changes: 28 additions & 0 deletions src/app/browser/qgsinbuiltdataitemproviders.cpp
Expand Up @@ -366,6 +366,25 @@ void QgsLayerItemGuiProvider::populateContextMenu( QgsDataItem *item, QMenu *men
} );
menu->addAction( addAction );

if ( item->capabilities2() & QgsDataItem::Delete )
{
QList<QgsLayerItem *> selectedDeletableItems;
for ( QgsDataItem *selectedItem : selectedItems )
{
if ( qobject_cast<QgsLayerItem *>( selectedItem ) && ( selectedItem->capabilities2() & QgsDataItem::Delete ) )
selectedDeletableItems.append( qobject_cast<QgsLayerItem *>( selectedItem ) );
}

const QString deleteText = selectedDeletableItems.count() == 1 ? tr( "Delete Layer" )
: tr( "Delete Selected Layers" );
QAction *deleteAction = new QAction( deleteText, menu );
connect( deleteAction, &QAction::triggered, this, [ = ]
{
deleteLayers( selectedDeletableItems );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely to result in crashes, because after the first table is deleted providers may (and usually will) trigger a refresh of their parent. This happens in a background thread, and may result in items within the selectedDeletableItems list being deleted before deleteLayers has had a chance to handle them. I think what should be done here is that selectedDeletableItems will need to be a string list, storing the paths of the selected items instead of the items themselves. (I've experienced this crash myself when deleting OGR items, which was why the OGR provider was adapted to use a static deleteLayer method with strings instead of the layer item).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And my apologies, I should have picked this up in the earlier review. That's my fault.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... Good point.
Can't we just check, if the item is still valid before we call to delete it?
If I understand you correctly, it means, that it shouldn't be implemented in the specific layer items and have a general deleteItemAccordingUri( item-uri, item-type ) function in qgsinbuiltdataitemproviders?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely to result in crashes, because after the first table is deleted providers may (and usually will) trigger a refresh of their parent. This happens in a background thread, and may result in items within the selectedDeletableItems list being deleted before deleteLayers has had a chance to handle them.

I'm not sure I can follow this logic @nyalldawson

Does this mean, a background thread deletes items from the UI? If yes, this sounds like a guarantee for crashes and should be fixed directly instead of working around potential issues here.
Instead they should be deleted (via queued connection) on the main thread, in which case they will be deleted after this code has been executed (because this code is triggered by the UI on the main thread and executed via direct connection unless I'm mistaken).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead they should be deleted (via queued connection) on the main thread

I believe this is already the case

in which case they will be deleted after this code has been executed (because this code is triggered by the UI on the main thread and executed via direct connection unless I'm mistaken).

Ideally yes, but in practice there must be some processEvents calls or something messing things up, because I definitely got easily reproducible crashes when this approach was used in the OGR provider.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a stacktrace around for this or instructions how to reproduce this?

To be honest, I'm not a big fan of implementing workarounds for badly designed code without seeing it first and having a good ground to decide that fixing the broken code is (much) harder than implementing the workaround. :/

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's 7ad1d29 -- and that commit gives more clues, because it's while the confirmation dialog is shown (main thread) that the main thread event loop is running and deleting the items due to the previously queued refresh.

So that's all good -- no nasty bugs in master which we're hiding. But this PR also has messageboxes showing while deleting layers (confirmation for each layer, failure warning) and so needs to handle that same case. Approaches would be encapsulating the layer parameters from all items in advance and only using these during the delete, OR

  1. show a single confirmation once in advance showing a summary of all layers to be deleted, and not show a confirmation for each individual layer in turn, and
  2. only show the messagebox failure message after all items have been processed

} );
menu->addAction( deleteAction );
}

QAction *propertiesAction = new QAction( tr( "Layer Properties…" ), menu );
connect( propertiesAction, &QAction::triggered, this, [ = ]
{
Expand Down Expand Up @@ -433,6 +452,15 @@ void QgsLayerItemGuiProvider::addLayersFromItems( const QList<QgsDataItem *> &it
QgisApp::instance()->handleDropUriList( layerUriList );
}

void QgsLayerItemGuiProvider::deleteLayers( const QList<QgsLayerItem *> &items )
{
for ( QgsLayerItem *item : items )
{
if ( !item->deleteLayer() )
QMessageBox::information( QgisApp::instance(), tr( "Delete Layer" ), tr( "Item Layer %1 cannot be deleted." ).arg( item->name() ) );
}
}

void QgsLayerItemGuiProvider::showPropertiesForItem( QgsLayerItem *item )
{
if ( ! item )
Expand Down
1 change: 1 addition & 0 deletions src/app/browser/qgsinbuiltdataitemproviders.h
Expand Up @@ -100,6 +100,7 @@ class QgsLayerItemGuiProvider : public QObject, public QgsDataItemGuiProvider

void addLayersFromItems( const QList<QgsDataItem *> &items );
void showPropertiesForItem( QgsLayerItem *item );
void deleteLayers( const QList<QgsLayerItem *> &items );

};

Expand Down
5 changes: 5 additions & 0 deletions src/core/qgsdataitem.cpp
Expand Up @@ -701,6 +701,11 @@ QString QgsLayerItem::iconName( QgsLayerItem::LayerType layerType )
}
}

bool QgsLayerItem::deleteLayer()
{
return false;
}

bool QgsLayerItem::equal( const QgsDataItem *other )
{
//QgsDebugMsg ( mPath + " x " + other->mPath );
Expand Down
6 changes: 5 additions & 1 deletion src/core/qgsdataitem.h
Expand Up @@ -210,8 +210,9 @@ class CORE_EXPORT QgsDataItem : public QObject
SetCrs = 1 << 0, //!< Can set CRS on layer or group of layers
Fertile = 1 << 1, //!< Can create children. Even items without this capability may have children, but cannot create them, it means that children are created by item ancestors.
Fast = 1 << 2, //!< CreateChildren() is fast enough to be run in main thread when refreshing items, most root items (wms,wfs,wcs,postgres...) are considered fast because they are reading data only from QgsSettings
Collapse = 1 << 3, //!< The collapse/expand status for this items children should be ignored in order to avoid undesired network connections (wms etc.)
Collapse = 1 << 3, //!< The collapse/expand status for this items children should be ignored in order to avoid undesired network connections (wms etc.)
Rename = 1 << 4, //!< Item can be renamed
Delete = 1 << 5, //!< Item can be deleted
};
Q_DECLARE_FLAGS( Capabilities, Capability )

Expand Down Expand Up @@ -505,6 +506,9 @@ class CORE_EXPORT QgsLayerItem : public QgsDataItem
*/
static QString iconName( LayerType layerType );

//! Delete this layer item
virtual bool deleteLayer();

protected:

//! The provider key
Expand Down
86 changes: 45 additions & 41 deletions src/providers/mssql/qgsmssqldataitems.cpp
Expand Up @@ -561,64 +561,68 @@ QgsMssqlLayerItem::QgsMssqlLayerItem( QgsDataItem *parent, const QString &name,
: QgsLayerItem( parent, name, path, QString(), layerType, QStringLiteral( "mssql" ) )
, mLayerProperty( layerProperty )
{
mCapabilities |= Delete;
mUri = createUri();
setState( Populated );
}

#ifdef HAVE_GUI
QList<QAction *> QgsMssqlLayerItem::actions( QWidget *actionParent )
{
QgsMssqlConnectionItem *connItem = qobject_cast<QgsMssqlConnectionItem *>( parent() ? parent()->parent() : nullptr );

QList<QAction *> lst;

// delete
QAction *actionDeleteLayer = new QAction( tr( "Delete Table" ), actionParent );
connect( actionDeleteLayer, &QAction::triggered, this, [ = ]
{
if ( QMessageBox::question( nullptr, QObject::tr( "Delete Table" ),
QObject::tr( "Are you sure you want to delete [%1].[%2]?" ).arg( mLayerProperty.schemaName, mLayerProperty.tableName ),
QMessageBox::Yes | QMessageBox::No, QMessageBox::No ) != QMessageBox::Yes )
return;

QString errCause;
bool res = QgsMssqlConnection::dropTable( mUri, &errCause );
if ( !res )
{
QMessageBox::warning( nullptr, tr( "Delete Table" ), errCause );
}
else
{
QMessageBox::information( nullptr, tr( "Delete Table" ), tr( "Table deleted successfully." ) );
if ( connItem )
connItem->refresh();
}
} );
lst.append( actionDeleteLayer );

// truncate
QAction *actionTruncateLayer = new QAction( tr( "Truncate Table" ), actionParent );
connect( actionTruncateLayer, &QAction::triggered, this, [ = ]
{
if ( QMessageBox::question( nullptr, QObject::tr( "Truncate Table" ),
QObject::tr( "Are you sure you want to truncate [%1].[%2]?\n\nThis will delete all data within the table." ).arg( mLayerProperty.schemaName, mLayerProperty.tableName ),
QMessageBox::Yes | QMessageBox::No, QMessageBox::No ) != QMessageBox::Yes )
return;

QString errCause;
bool res = QgsMssqlConnection::truncateTable( mUri, &errCause );
if ( !res )
{
QMessageBox::warning( nullptr, tr( "Truncate Table" ), errCause );
}
else
{
QMessageBox::information( nullptr, tr( "Truncate Table" ), tr( "Table truncated successfully." ) );
}
truncateTable();
} );
lst.append( actionTruncateLayer );
return lst;
}

bool QgsMssqlLayerItem::deleteLayer()
{
QgsMssqlConnectionItem *connItem = qobject_cast<QgsMssqlConnectionItem *>( parent() ? parent()->parent() : nullptr );

if ( QMessageBox::question( nullptr, QObject::tr( "Delete Table" ),
QObject::tr( "Are you sure you want to delete [%1].[%2]?" ).arg( mLayerProperty.schemaName, mLayerProperty.tableName ),
QMessageBox::Yes | QMessageBox::No, QMessageBox::No ) != QMessageBox::Yes )
return true;

QString errCause;
bool res = QgsMssqlConnection::dropTable( mUri, &errCause );
if ( !res )
{
QMessageBox::warning( nullptr, tr( "Delete Table" ), errCause );
}
else
{
QMessageBox::information( nullptr, tr( "Delete Table" ), tr( "Table deleted successfully." ) );
if ( connItem )
connItem->refresh();
}
return true;
}

void QgsMssqlLayerItem::truncateTable()
{
if ( QMessageBox::question( nullptr, QObject::tr( "Truncate Table" ),
QObject::tr( "Are you sure you want to truncate [%1].[%2]?\n\nThis will delete all data within the table." ).arg( mLayerProperty.schemaName, mLayerProperty.tableName ),
QMessageBox::Yes | QMessageBox::No, QMessageBox::No ) != QMessageBox::Yes )
return;

QString errCause;
bool res = QgsMssqlConnection::truncateTable( mUri, &errCause );
if ( !res )
{
QMessageBox::warning( nullptr, tr( "Truncate Table" ), errCause );
}
else
{
QMessageBox::information( nullptr, tr( "Truncate Table" ), tr( "Table truncated successfully." ) );
}
}
#endif

QgsMssqlLayerItem *QgsMssqlLayerItem::createClone()
Expand Down
6 changes: 6 additions & 0 deletions src/providers/mssql/qgsmssqldataitems.h
Expand Up @@ -140,6 +140,12 @@ class QgsMssqlLayerItem : public QgsLayerItem

bool disableInvalidGeometryHandling() const;

public slots:
#ifdef HAVE_GUI
bool deleteLayer() override;
void truncateTable();
#endif

private:
QgsMssqlLayerProperty mLayerProperty;
bool mDisableInvalidGeometryHandling = false;
Expand Down
18 changes: 5 additions & 13 deletions src/providers/ogr/qgsgeopackagedataitems.cpp
Expand Up @@ -75,15 +75,6 @@ QVector<QgsDataItem *> QgsGeoPackageRootItem::createChildren()
}

#ifdef HAVE_GUI
QList<QAction *> QgsGeoPackageAbstractLayerItem::actions( QWidget * )
{
QList<QAction *> lst;
QAction *actionDeleteLayer = new QAction( tr( "Delete Layer '%1'…" ).arg( mName ), this );
connect( actionDeleteLayer, &QAction::triggered, this, &QgsGeoPackageAbstractLayerItem::deleteLayer );
lst.append( actionDeleteLayer );
return lst;
}

void QgsGeoPackageRootItem::onConnectionsChanged()
{
refresh();
Expand Down Expand Up @@ -487,7 +478,7 @@ void QgsGeoPackageCollectionItem::vacuumGeoPackageDbAction()
}
}

void QgsGeoPackageAbstractLayerItem::deleteLayer()
bool QgsGeoPackageAbstractLayerItem::deleteLayer()
{
// Check if the layer(s) are in the registry
QList<QgsMapLayer *> layersList;
Expand All @@ -505,14 +496,14 @@ void QgsGeoPackageAbstractLayerItem::deleteLayer()
if ( QMessageBox::question( nullptr, QObject::tr( "Delete Layer" ), QObject::tr( "The layer <b>%1</b> exists in the current project <b>%2</b>,"
" do you want to remove it from the project and delete it?" ).arg( mName, layersList.at( 0 )->name() ), QMessageBox::Yes | QMessageBox::No, QMessageBox::No ) != QMessageBox::Yes )
{
return;
return true;
}
}
else if ( QMessageBox::question( nullptr, QObject::tr( "Delete Layer" ),
QObject::tr( "Are you sure you want to delete layer <b>%1</b> from GeoPackage?" ).arg( mName ),
QMessageBox::Yes | QMessageBox::No, QMessageBox::No ) != QMessageBox::Yes )
{
return;
return true;
}

if ( layersList.isEmpty() )
Expand All @@ -532,7 +523,7 @@ void QgsGeoPackageAbstractLayerItem::deleteLayer()
if ( mParent )
mParent->refreshConnections();
}

return true;
}
#endif

Expand Down Expand Up @@ -708,6 +699,7 @@ bool QgsGeoPackageConnectionItem::equal( const QgsDataItem *other )
QgsGeoPackageAbstractLayerItem::QgsGeoPackageAbstractLayerItem( QgsDataItem *parent, const QString &name, const QString &path, const QString &uri, QgsLayerItem::LayerType layerType, const QString &providerKey )
: QgsLayerItem( parent, name, path, uri, layerType, providerKey )
{
mCapabilities |= Delete;
mToolTip = uri;
setState( Populated ); // no children are expected
}
Expand Down
5 changes: 2 additions & 3 deletions src/providers/ogr/qgsgeopackagedataitems.h
Expand Up @@ -36,10 +36,9 @@ class QgsGeoPackageAbstractLayerItem : public QgsLayerItem
* the real deletion implementation
*/
virtual bool executeDeleteLayer( QString &errCause );

#ifdef HAVE_GUI
QList<QAction *> actions( QWidget *menu ) override;
public slots:
virtual void deleteLayer();
bool deleteLayer() override;
#endif
};

Expand Down
10 changes: 4 additions & 6 deletions src/providers/postgres/qgspostgresdataitems.cpp
Expand Up @@ -296,6 +296,7 @@ QgsPGLayerItem::QgsPGLayerItem( QgsDataItem *parent, const QString &name, const
: QgsLayerItem( parent, name, path, QString(), layerType, QStringLiteral( "postgres" ) )
, mLayerProperty( layerProperty )
{
mCapabilities |= Delete;
mUri = createUri();
setState( Populated );
Q_ASSERT( mLayerProperty.size() == 1 );
Expand All @@ -317,10 +318,6 @@ QList<QAction *> QgsPGLayerItem::actions( QWidget *parent )
connect( actionRenameLayer, &QAction::triggered, this, &QgsPGLayerItem::renameLayer );
lst.append( actionRenameLayer );

QAction *actionDeleteLayer = new QAction( tr( "Delete %1" ).arg( typeName ), parent );
connect( actionDeleteLayer, &QAction::triggered, this, &QgsPGLayerItem::deleteLayer );
lst.append( actionDeleteLayer );

if ( !mLayerProperty.isView )
{
QAction *actionTruncateLayer = new QAction( tr( "Truncate %1" ).arg( typeName ), parent );
Expand All @@ -338,12 +335,12 @@ QList<QAction *> QgsPGLayerItem::actions( QWidget *parent )
return lst;
}

void QgsPGLayerItem::deleteLayer()
bool QgsPGLayerItem::deleteLayer()
{
if ( QMessageBox::question( nullptr, QObject::tr( "Delete Table" ),
QObject::tr( "Are you sure you want to delete %1.%2?" ).arg( mLayerProperty.schemaName, mLayerProperty.tableName ),
QMessageBox::Yes | QMessageBox::No, QMessageBox::No ) != QMessageBox::Yes )
return;
return true;

QString errCause;
bool res = ::deleteLayer( mUri, errCause );
Expand All @@ -357,6 +354,7 @@ void QgsPGLayerItem::deleteLayer()
if ( mParent )
mParent->refresh();
}
return true;
}

void QgsPGLayerItem::renameLayer()
Expand Down
2 changes: 1 addition & 1 deletion src/providers/postgres/qgspostgresdataitems.h
Expand Up @@ -129,7 +129,7 @@ class QgsPGLayerItem : public QgsLayerItem

public slots:
#ifdef HAVE_GUI
void deleteLayer();
bool deleteLayer() override;
void renameLayer();
void truncateTable();
void refreshMaterializedView();
Expand Down
17 changes: 4 additions & 13 deletions src/providers/spatialite/qgsspatialitedataitems.cpp
Expand Up @@ -37,27 +37,17 @@ QGISEXTERN bool deleteLayer( const QString &dbPath, const QString &tableName, QS
QgsSLLayerItem::QgsSLLayerItem( QgsDataItem *parent, const QString &name, const QString &path, const QString &uri, LayerType layerType )
: QgsLayerItem( parent, name, path, uri, layerType, QStringLiteral( "spatialite" ) )
{
mCapabilities |= Delete;
setState( Populated ); // no children are expected
}

#ifdef HAVE_GUI
QList<QAction *> QgsSLLayerItem::actions( QWidget *parent )
{
QList<QAction *> lst;

QAction *actionDeleteLayer = new QAction( tr( "Delete Layer" ), parent );
connect( actionDeleteLayer, &QAction::triggered, this, &QgsSLLayerItem::deleteLayer );
lst.append( actionDeleteLayer );

return lst;
}

void QgsSLLayerItem::deleteLayer()
bool QgsSLLayerItem::deleteLayer()
{
if ( QMessageBox::question( nullptr, QObject::tr( "Delete Object" ),
QObject::tr( "Are you sure you want to delete %1?" ).arg( mName ),
QMessageBox::Yes | QMessageBox::No, QMessageBox::No ) != QMessageBox::Yes )
return;
return true;

QgsDataSourceUri uri( mUri );
QString errCause;
Expand All @@ -71,6 +61,7 @@ void QgsSLLayerItem::deleteLayer()
QMessageBox::information( nullptr, tr( "Delete Layer" ), tr( "Layer deleted successfully." ) );
mParent->refresh();
}
return true;
}
#endif

Expand Down