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

Warn that layers will be lost when overwriting container vector file #34817

Merged
merged 7 commits into from
Apr 12, 2020

Conversation

uclaros
Copy link
Contributor

@uclaros uclaros commented Mar 2, 2020

Description

Removed the dangerous option to delete the whole container file (geopackage, spatialite etc) when it contains a layer with the same name as the one the user is trying to save.

Fixes #32823

@roya0045
Copy link
Contributor

roya0045 commented Mar 2, 2020

I wouldn't vouch for removing this option, the only thing that I would do is add a dialog asking for confirmation.

@3nids
Copy link
Member

3nids commented Mar 2, 2020

To me, it's the very first message which is problematic.
When you select an existing file, it says it is going to be overwritten even when your intention is to add a layer to it.

I think we should have a radio button on top of the dialog, letting the choice between: "create/overwrite GPGK" and "add layer to existing GPKG".

That would remove any confusion, and bonus, you won't get a warning when you select an existing file in the dialog.

@uclaros
Copy link
Contributor Author

uclaros commented Mar 2, 2020

When you select an existing file, it says it is going to be overwritten even when your intention is to add a layer to it.

No, it does not. It just saves the new layer in the existing geopackage.

Imagine the exact equivalent to the Overwrite File option when saving for example a spreadsheet:
You get a dialog saying File spreadsheet.xls already exists. Do you want to overwrite it, or do you want to overwrite it and then also delete all other files (regardless of their format) that exist in the same folder?
This does not exist in any software because it just makes no sense. If the user wants to delete the folder (or geopackage) he can do it explicitly in the file selection dialog while selecting the filename and seeing that it already exists.

An ideal solution for me would be to be able to browse the geopackage contents like it was a folder, in the file selection dialog. Then the user would already be aware of the geopackage contents and in a better position to decide the name of the layer to choose or if he wants to delete the geopackage and all it's contents. But that is probably too much work.

@nirvn
Copy link
Contributor

nirvn commented Mar 2, 2020

-1 to remove a feature here. If wording is not clear enough, we can update that. I personally use file overwrite quite often.

Not offering this option also means ppl might end up adding a layer to a preexisting gpkg without realizing it, and potentially share sensitive data by mistake.

@uclaros
Copy link
Contributor Author

uclaros commented Mar 2, 2020

Not offering this option also means ppl might end up adding a layer to a preexisting gpkg without realizing it, and potentially share sensitive data by mistake.

This may already be happening, since the dialog appears only on layer collision, not on filename collision.

@3nids
Copy link
Member

3nids commented Mar 2, 2020

No, it does not. It just saves the new layer in the existing geopackage.

Then this is really misleading:

Screen Recording 2020-03-02 at 15 07 57

@uclaros
Copy link
Contributor Author

uclaros commented Mar 2, 2020

Is this an OSX thing? In my build this does not pop up, neither with the New Geopackage Layer QgsNewGeoPackageLayerDialog in your screencast (untouched by this PR), nor with the Save Vector Layer As... QgsVectorLayerSaveAsDialog (modified in this PR) dialogs.

QGIS version 3.13.0-Master QGIS code revision 09fb04d
Compiled against Qt 5.12.5 Running against Qt 5.12.5
Compiled against GDAL/OGR 3.0.4 Running against GDAL/OGR 3.0.4
Compiled against GEOS 3.8.0-CAPI-1.13.1 Running against GEOS 3.8.0-CAPI-1.13.1
Compiled against SQLite 3.31.1 Running against SQLite 3.31.1
PostgreSQL Client Version 12.2 (Debian 12.2-1) SpatiaLite Version 4.3.0a
QWT Version 6.1.4 QScintilla2 Version 2.11.2
Compiled against PROJ 6.3.1 Running against PROJ Rel. 6.3.1, February 10th, 2020
OS Version Debian GNU/Linux bullseye/sid This copy of QGIS writes debugging output.
Active python plugins firstaid; plugin_reloader; openlayers_plugin; QuickWKT; StreetView; FreehandRasterGeoreferencer; processing; db_manager

@nyalldawson
Copy link
Collaborator

I'm with @nirvn here, I'm -1 to removing the option and think we just need extra confirmation/warning prompts instead.

@github-actions github-actions bot added this to the 3.14.0 milestone Mar 4, 2020
@uclaros uclaros changed the title Don't offer to overwrite whole file on container formats Warn that layers will be lost when overwriting container vector file Mar 8, 2020
@uclaros
Copy link
Contributor Author

uclaros commented Mar 8, 2020

Added a warning with the layers that the user is about to say goodbye to. It should not spawn when the file has only one layer (ie Overwrite file and overwrite layer will have the same effect) so the user does not get used to it and clicks OK without thinking. It only spawns when other layers are going to be deleted.
I also have the following thoughts that bother me:

  1. Is my use of QgsOgrLayerItem::subLayers() proper for the occasion or should I have looked elsewhere?
  2. Am I wrong or the file has been opened three times, once for QgsVectorFileWriter::editionCapabilities(), once for QgsVectorFileWriter::targetLayerExists() and once for QgsOgrLayerItem::subLayers()? Should this be optimized?
  3. I don't like that I had to copy paste the same code in 3 places in the same file. Wouldn't it be better to create a single message box for all capabilities cases and only add the proper buttons depending on the case?
  4. This has to be applied to at least qgsnewgeopackagelayerdialog.cpp and qgsnewspatialitelayerdialog.cpp, maybe more places. There's a lot of duplicate code there that can lead to inconsistencies (like the new shapefile dialog has radio buttons for Z/M values, while the geopackage and spatialite have chackboxes). Wouldn't it be better to have a QgsNew*LayerDialog that handles such stuff centrally and subclass it for each format? Even more, have a base class that both QgsVectorLayerSaveAsDialog and QgsNew*LayerDialog could use?

@stale
Copy link

stale bot commented Mar 22, 2020

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.

@stale stale bot added stale Uh oh! Seems this work is abandoned, and the PR is about to close. and removed stale Uh oh! Seems this work is abandoned, and the PR is about to close. labels Mar 22, 2020
@uclaros
Copy link
Contributor Author

uclaros commented Mar 22, 2020

This is what the new warning dialog looks like:
Screenshot_20200322_184906

@roya0045
Copy link
Contributor

This is what the new warning dialog looks like:
Screenshot_20200322_184906

I can get behind this.

@roya0045
Copy link
Contributor

roya0045 commented Apr 1, 2020

@uclaros what happens when the layer list is too big, does the ok gets out of the screen or can there be a text box with a scroll bar in that case?

@uclaros
Copy link
Contributor Author

uclaros commented Apr 1, 2020

This is a generic QMessageBox::warning so it will probably grow with the contents and push the buttons out of the screen, indeed. Maybe I could put the layers list into detailedText which requires pressing a show details button and display a scrollable text box.

does the ok gets out of the screen

with a list that long, you're probably looking for the cancel button though!

@uclaros
Copy link
Contributor Author

uclaros commented Apr 4, 2020

I restructured the code to avoid all the duplicate code in each capability case.

As for the overwrite whole file confirmation, it now looks like this:
Peek 2020-04-04 17-32

It still only spawns when there is more than one layer on the file to be overwritten.

@nyalldawson
Copy link
Collaborator

@uclaros I like that!

(off topic: why are your layers named .shp? 😄 )

@uclaros
Copy link
Contributor Author

uclaros commented Apr 5, 2020

(off topic: why are your layers named .shp? smile )

@nyalldawson They were merged in a geopackage with some ad-hoc ogr2ogr bash script but didn't bother to trim the extension! Apparently Greece still lives in the age of .shp and .mdb!

Copy link
Collaborator

@nyalldawson nyalldawson left a comment

Choose a reason for hiding this comment

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

Looking good! Just some minor cleanups left...

{
if ( QMessageBox::question( this,
tr( "Save Vector Layer As" ),
tr( "The existing layer has different fields. Do you want to add the missing fields to the layer?" ) ) == QMessageBox::Yes )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
tr( "The existing layer has different fields. Do you want to add the missing fields to the layer?" ) ) == QMessageBox::Yes )
tr( "The existing layer has additional fields. Do you want to add the missing fields to the layer?" ) ) == QMessageBox::Yes )

}
else if ( mActionOnExistingFile == QgsVectorFileWriter::CreateOrOverwriteFile )
{
const QList<QgsOgrDbLayerInfo *> subLayers = QgsOgrLayerItem::subLayers( filename(), format() );
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 leaked - the entries need to be deleted

else if ( mActionOnExistingFile == QgsVectorFileWriter::CreateOrOverwriteFile )
{
const QList<QgsOgrDbLayerInfo *> subLayers = QgsOgrLayerItem::subLayers( filename(), format() );
QStringList layerList = QStringList();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
QStringList layerList = QStringList();
QStringList layerList;

{
const QList<QgsOgrDbLayerInfo *> subLayers = QgsOgrLayerItem::subLayers( filename(), format() );
QStringList layerList = QStringList();
for ( const auto layer : subLayers )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for ( const auto layer : subLayers )
for ( const QgsOgrDbLayerInfo* layer : subLayers )

layerList.sort( Qt::CaseInsensitive );
QMessageBox msgBox;
msgBox.setIcon( QMessageBox::Warning );
msgBox.setWindowTitle( tr( "Overwrite file" ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
msgBox.setWindowTitle( tr( "Overwrite file" ) );
msgBox.setWindowTitle( tr( "Overwrite File" ) );

@nyalldawson nyalldawson added GUI/UX Related to QGIS application GUI or User Experience backport release-3_10 labels Apr 7, 2020
@nyalldawson
Copy link
Collaborator

(I think we should backport this to LTR)

@roya0045
Copy link
Contributor

roya0045 commented Apr 7, 2020

@uclaros nice UI improvement on the dialog!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI/UX Related to QGIS application GUI or User Experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid accidentally overwriting Geopackage database when creating new layer
5 participants