Delete selected tables as DataItem action #8507

signedav · 2018-11-19T09:07:52Z

With right click on the browser the action is "Delete Selected Tables" in case there are more than one selected. This is implemented for:

GeoPackage
SpatiaLite
PostGIS
MSSQL (including. "Truncate Selected Tables")

On right click on a (e.g.) GeoPackage and "Delete selected layers" only the selected GeoPackage-tables are deleted. If there are other selected layers the action for them is ignored.

Couldn't test with MSSQL - just implemented it the way like in the other items.

This implements https://issues.qgis.org/issues/19875

nyalldawson · 2018-11-19T10:20:15Z

Is this for master? There's a whole new api for the gui component of data items which was partly designed to cleanly address this -- see #8352

signedav · 2018-11-19T17:11:06Z

Thanks @nyalldawson I have overseen that. I will update the implementation...

implemented for derived functions in: - `QgsGeoPackageAbstractLayerItem` - `QgsSLLayerItem` - `QgsPGLayerItem` - `QgsMssqlLayerItem`

signedav · 2018-11-20T10:06:37Z

Implemented now over the api mentioned by @nyalldawson
Means it calls for each selected item the derived deleteLayer(). Means the delete-action is removed from:

QgsGeoPackageAbstractLayerItem
QgsSLLayerItem
QgsPGLayerItem
QgsMssqlLayerItem

I hope I use it like it's intendet. Not sure yet how I provide that the action is only shown for the relevant items, that have a derived deleteLayer() function. Should I object_cast the QgsDataItems or is there a type handling for that I've overlooked?

PS. I only cared about delete layer and not truncate, rename etc. for QgsPGLayerItems and QgsMssqlLayerItem. Does this need to be part for this PR as well?

nyalldawson · 2018-11-21T04:57:12Z

src/core/qgsdataitem.h

@@ -323,6 +323,9 @@ class CORE_EXPORT QgsDataItem : public QObject
    //! Move object and all its descendants to thread
    void moveToThread( QThread *targetThread );

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


I think this belongs in the QgsLayerItem subclass

nyalldawson · 2018-11-21T04:59:01Z

I hope I use it like it's intendet. Not sure yet how I provide that the action is only shown for the relevant items, that have a derived deleteLayer() function. Should I object_cast the QgsDataItems or is there a type handling for that I've overlooked?

You could also add a "DeleteLayer" capability to the QgsDataItem::Capability enum, and then only return this flag for layer items which support delete. I think that's the cleanest approach here.

check if the item with the menu has this capability and only handle the selected items with this capabiltiy

nyalldawson · 2018-11-26T02:04:29Z

src/app/browser/qgsinbuiltdataitemproviders.cpp

+    QAction *deleteAction = new QAction( deleteText, menu );
+    connect( deleteAction, &QAction::triggered, this, [ = ]
+    {
+      deleteLayers( selectedDeletableItems );


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).

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

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?

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).

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.

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. :/

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

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

only show the messagebox failure message after all items have been processed

stale · 2018-12-25T01:04:52Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

signedav · 2018-12-25T21:57:51Z

I'll do that on the begin of next year

signedav · 2019-01-03T12:53:08Z

I store now the uris in a list, pass the list to the delete-function and there I get the items from the browser model according to the uri. I hope this solves the issue @nyalldawson is reporting. I'm not completely sure if this matches to your suggestion.

src/app/browser/qgsinbuiltdataitemproviders.cpp

src/core/qgsbrowsermodel.h

nyalldawson · 2019-01-07T23:28:53Z

Looks good -- I can't see any remaining issues here, thanks!

signedav · 2019-01-08T09:55:02Z

Thanks @nyalldawson and @m-kuhn

signedav added 5 commits November 15, 2018 09:10

pass items to delete layer activity

fa06246

do not pass item as argument

be0ce0c

delete selected layers on spatialite and pg

7acd83f

remove unused comments

9dd96da

delete/truncate selected tables for mssql-tables

f06bb79

signedav added 2 commits November 20, 2018 10:49

action deleteLayer in the dataitemproviders

a1ea0ac

implemented for derived functions in: - `QgsGeoPackageAbstractLayerItem` - `QgsSLLayerItem` - `QgsPGLayerItem` - `QgsMssqlLayerItem`

remove unused variable

4296c77

nyalldawson reviewed Nov 21, 2018

View reviewed changes

nyalldawson added the Feature label Nov 21, 2018

nyalldawson added this to the 3.6 milestone Nov 21, 2018

delete layer action only on items that have capability "delete"

1136aab

check if the item with the menu has this capability and only handle the selected items with this capabiltiy

nyalldawson reviewed Nov 26, 2018

View reviewed changes

stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Dec 25, 2018

stale bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Dec 25, 2018

signedav added 5 commits January 3, 2019 11:50

find index of layeritem with the passed uri

d453006

use uri to get layeritem to delete

81faf95

fix - removes layers from project if they exist there

5a75195

autogenerated sip in file

0373fe7

Merge branch 'master' into basicfix

666f959

m-kuhn assigned nyalldawson Jan 5, 2019

nyalldawson reviewed Jan 7, 2019

View reviewed changes

src/app/browser/qgsinbuiltdataitemproviders.cpp Outdated Show resolved Hide resolved

src/app/browser/qgsinbuiltdataitemproviders.cpp Outdated Show resolved Hide resolved

src/core/qgsbrowsermodel.h Outdated Show resolved Hide resolved

put message in log instead of message box

37eec19

m-kuhn merged commit 5df5c37 into qgis:master Jan 8, 2019

This was referenced May 24, 2019

Cannot build python_module_qgis_gui (2.0.1) with SIP 4.15.2 #17519

Closed

Compiling fails on Arch Linux for python_module_qgis_gui #17690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete selected tables as DataItem action #8507

Delete selected tables as DataItem action #8507

signedav commented Nov 19, 2018

nyalldawson commented Nov 19, 2018

signedav commented Nov 19, 2018

signedav commented Nov 20, 2018

nyalldawson Nov 21, 2018

nyalldawson commented Nov 21, 2018

nyalldawson Nov 26, 2018

nyalldawson Nov 26, 2018

signedav Dec 4, 2018

m-kuhn Dec 7, 2018

nyalldawson Dec 10, 2018

m-kuhn Dec 10, 2018

nyalldawson Dec 10, 2018

stale bot commented Dec 25, 2018

signedav commented Dec 25, 2018

signedav commented Jan 3, 2019

nyalldawson commented Jan 7, 2019

signedav commented Jan 8, 2019

Delete selected tables as DataItem action #8507

Delete selected tables as DataItem action #8507

Conversation

signedav commented Nov 19, 2018

nyalldawson commented Nov 19, 2018

signedav commented Nov 19, 2018

signedav commented Nov 20, 2018

Choose a reason for hiding this comment

nyalldawson commented Nov 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Dec 25, 2018

signedav commented Dec 25, 2018

signedav commented Jan 3, 2019

nyalldawson commented Jan 7, 2019

signedav commented Jan 8, 2019