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

Fix invalid attributes dialog on copy to another layer #33688

Merged
merged 10 commits into from
Jan 15, 2020
46 changes: 44 additions & 2 deletions src/app/qgisapp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
#include "qgssourceselectproviderregistry.h"
#include "qgssourceselectprovider.h"
#include "qgsprovidermetadata.h"
#include "qgsfixattributedialog.h"

#include "qgsanalysis.h"
#include "qgsgeometrycheckregistry.h"
Expand Down Expand Up @@ -9588,6 +9589,47 @@ void QgisApp::pasteFromClipboard( QgsMapLayer *destinationLayer )
// now create new feature using pasted feature as a template. This automatically handles default
// values and field constraints
QgsFeatureList newFeatures {QgsVectorLayerUtils::createFeatures( pasteVectorLayer, newFeaturesDataList, &context )};

// check constraints
QgsFeatureList validFeatures = newFeatures;
QgsFeatureList invalidFeatures;
for ( const QgsFeature &f : qgis::as_const( validFeatures ) )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we skip this whole loop if no constraints are present? I'm concerned about a regression in paste time for huge numbers of features.

{
for ( int idx = 0; idx < pasteVectorLayer->fields().count(); ++idx )
{
QStringList errors;
if ( !QgsVectorLayerUtils::validateAttribute( pasteVectorLayer, f, idx, errors, QgsFieldConstraints::ConstraintStrengthHard, QgsFieldConstraints::ConstraintOriginNotSet ) )
{
invalidFeatures << f;
validFeatures.removeOne( f );
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can't modify a qlist while iterating over it like this. You'd need to use a modifiable iterator instead and .erase the current entry.

break;
}
}
}

if ( !invalidFeatures.isEmpty() )
{
newFeatures.clear();

QgsFixAttributeDialog *dialog = new QgsFixAttributeDialog( pasteVectorLayer, invalidFeatures, this );
int feedback = dialog->exec();

if ( feedback == QgsFixAttributeDialog::VanishAll )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Switch instead of if

{
//vanish all
}
else if ( feedback == QgsFixAttributeDialog::CopyValid )
{
//copy valid and fixed, vanish unfixed
newFeatures << validFeatures << dialog->fixedFeatures();
}
else if ( feedback == QgsFixAttributeDialog::CopyAll )
{
//copy all, even unfixed
newFeatures << validFeatures << dialog->fixedFeatures() << dialog->unfixedFeatures();
}
}

pasteVectorLayer->addFeatures( newFeatures );
QgsFeatureIds newIds;
newIds.reserve( newFeatures.size() );
Expand All @@ -9600,8 +9642,8 @@ void QgisApp::pasteFromClipboard( QgsMapLayer *destinationLayer )
pasteVectorLayer->endEditCommand();
pasteVectorLayer->updateExtents();

int nCopiedFeatures = features.count();
Qgis::MessageLevel level = ( nCopiedFeatures == 0 || nCopiedFeatures < nTotalFeatures || invalidGeometriesCount > 0 ) ? Qgis::Warning : Qgis::Info;
int nCopiedFeatures = newFeatures.count();
Qgis::MessageLevel level = ( nCopiedFeatures == 0 || invalidGeometriesCount > 0 ) ? Qgis::Warning : Qgis::Info;
QString message;
if ( nCopiedFeatures == 0 )
{
Expand Down
2 changes: 2 additions & 0 deletions src/gui/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,7 @@ SET(QGIS_GUI_SRCS
qgsfeatureselectiondlg.cpp
qgsfieldcombobox.cpp
qgsfieldexpressionwidget.cpp
qgsfixattributedialog.cpp
qgsfeaturelistcombobox.cpp
qgsfieldvalidator.cpp
qgsfieldvalueslineedit.cpp
Expand Down Expand Up @@ -511,6 +512,7 @@ SET(QGIS_GUI_HDRS
qgsfilewidget.h
qgsfilterlineedit.h
qgsfindfilesbypatternwidget.h
qgsfixattributedialog.h
qgsfloatingwidget.h
qgsfocuswatcher.h
qgsfontbutton.h
Expand Down
134 changes: 134 additions & 0 deletions src/gui/qgsfixattributedialog.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
/***************************************************************************
qgsfixattributedialog.cpp
---------------------
begin : January 2020
copyright : (C) 2020 by David Signer
email : david at opengis dot ch
***************************************************************************
* *
* This program is free software; you can redistribute it and/or modify *
* it under the terms of the GNU General Public License as published by *
* the Free Software Foundation; either version 2 of the License, or *
* (at your option) any later version. *
* *
***************************************************************************/
#include "qgsfixattributedialog.h"

#include "qgsattributeform.h"
#include "qgsapplication.h"

#include <QtWidgets/QPushButton>

QgsFixAttributeDialog::QgsFixAttributeDialog( QgsVectorLayer *vl, QgsFeatureList &features, QWidget *parent )
: QDialog( parent )
, mFeatures( features )
{
init( vl );
}

void QgsFixAttributeDialog::init( QgsVectorLayer *layer )
{
QgsAttributeEditorContext context;
setWindowTitle( tr( "%1 - Fix Pastet Features" ).arg( layer->name() ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

"pasted"

setLayout( new QGridLayout() );
layout()->setMargin( 0 );
context.setFormMode( QgsAttributeEditorContext::StandaloneDialog );

mUnfixedFeatures = mFeatures;
mCurrentFeature = mFeatures.begin();

QGridLayout *infoLayout = new QGridLayout();
QWidget *infoBox = new QWidget();
infoBox->setLayout( infoLayout );
layout()->addWidget( infoBox );

mDescription = new QLabel( descriptionText() );
mDescription->setVisible( mFeatures.count() > 1 );
infoLayout->addWidget( mDescription );
mProgressBar = new QProgressBar();
mProgressBar->setOrientation( Qt::Horizontal );
mProgressBar->setRange( 0, mFeatures.count() );
mProgressBar->setVisible( mFeatures.count() > 1 );
infoLayout->addWidget( mProgressBar );
QgsFeature feature;
mAttributeForm = new QgsAttributeForm( layer, *mCurrentFeature, context, this );
mAttributeForm->setMode( QgsAttributeEditorContext::SingleEditMode );
mAttributeForm->disconnectButtonBox();
layout()->addWidget( mAttributeForm );

QDialogButtonBox *buttonBox = mAttributeForm->findChild<QDialogButtonBox *>();
QPushButton *cancelAllBtn = new QPushButton( tr( "Cancel all" ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fix capitalisation for buttons (should be title case)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually these strings are a bit confusing. I'd go with "discard all", "discard all invalid features" and "paste all (including invalid)".

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nirvn these buttons are very long - got any ideas on alternative ui?

QPushButton *cancelAllInvalidBtn = new QPushButton( tr( "Cancel all invalid" ) );
QPushButton *storeAllInvalidBtn = new QPushButton( tr( "Store all (even invalid)" ) );
if ( mFeatures.count() > 1 )
{
buttonBox->addButton( cancelAllBtn, QDialogButtonBox::ActionRole );
buttonBox->addButton( cancelAllInvalidBtn, QDialogButtonBox::ActionRole );
connect( cancelAllBtn, &QAbstractButton::clicked, this, [ = ]()
{
done( VanishAll );
} );
connect( cancelAllInvalidBtn, &QAbstractButton::clicked, this, [ = ]()
{
done( CopyValid );
} );
buttonBox->button( QDialogButtonBox::Cancel )->setText( tr( "Skip" ) );
}
else
{
storeAllInvalidBtn->setText( tr( "Store anyway" ) );
}
buttonBox->addButton( storeAllInvalidBtn, QDialogButtonBox::ActionRole );
connect( storeAllInvalidBtn, &QAbstractButton::clicked, this, [ = ]()
{
done( CopyAll );
} );
connect( buttonBox, &QDialogButtonBox::rejected, this, &QgsFixAttributeDialog::reject );
connect( buttonBox, &QDialogButtonBox::accepted, this, &QgsFixAttributeDialog::accept );
connect( layer, &QObject::destroyed, this, &QWidget::close );

focusNextChild();
}

QString QgsFixAttributeDialog::descriptionText()
{
return tr( "%1 of %2 features processed (%3 fixed, %4 skipped)" ).arg( mCurrentFeature - mFeatures.begin() ).arg( mFeatures.count() ).arg( mFixedFeatures.count() ).arg( mCurrentFeature - mFeatures.begin() - mFixedFeatures.count() );
}

void QgsFixAttributeDialog::accept()
{
mAttributeForm->save();
mFixedFeatures << mAttributeForm->feature();
mUnfixedFeatures.removeOne( *mCurrentFeature );

//next feature
++mCurrentFeature;
if ( mCurrentFeature != mFeatures.end() )
{
mAttributeForm->setFeature( *mCurrentFeature );
}
else
{
done( CopyValid );
}

mProgressBar->setValue( mCurrentFeature - mFeatures.begin() );
mDescription->setText( descriptionText() );
}

void QgsFixAttributeDialog::reject()
{
//next feature
++mCurrentFeature;
if ( mCurrentFeature != mFeatures.end() )
{
mAttributeForm->setFeature( *mCurrentFeature );
}
else
{
done( CopyValid );
}

mProgressBar->setValue( mCurrentFeature - mFeatures.begin() );
mDescription->setText( descriptionText() );
}
87 changes: 87 additions & 0 deletions src/gui/qgsfixattributedialog.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/***************************************************************************
qgsfixattributedialog.h
---------------------
begin : January 2020
copyright : (C) 2020 by David Signer
email : david at opengis dot ch
***************************************************************************
* *
* This program is free software; you can redistribute it and/or modify *
* it under the terms of the GNU General Public License as published by *
* the Free Software Foundation; either version 2 of the License, or *
* (at your option) any later version. *
* *
***************************************************************************/

#ifndef QGSFIXATTRIBUTEDIALOG_H
#define QGSFIXATTRIBUTEDIALOG_H

#include "qgsattributeeditorcontext.h"
#include "qgis_sip.h"
#include "qgsattributeform.h"
#include "qgstrackedvectorlayertools.h"

#include <QDialog>
#include <QGridLayout>
#include <QProgressBar>
#include "qgis_gui.h"

/**
* \ingroup gui
* \class QgsFixAttributeDialog
* \brief Dialog to fix a list of invalid feature regarding constraints
* \since 3.12
*/

class GUI_EXPORT QgsFixAttributeDialog : public QDialog
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe leave in app for now?

{
Q_OBJECT

public:

/**
* Feedback code on closing the dialog
*/
enum Feedback
{
VanishAll, //!< Feedback to cancel copy of all features (even valid ones)
Copy link
Collaborator

Choose a reason for hiding this comment

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

DIscardAll

CopyValid, //!< Feedback to copy the valid features and vanishe the invalid ones
CopyAll //!< Feedback to copy all features, no matter if valid or invalid
};

/**
* Constructor for QgsFixAttributeDialog
*/
QgsFixAttributeDialog( QgsVectorLayer *vl, QgsFeatureList &features, QWidget *parent SIP_TRANSFERTHIS = nullptr );

/**
* Returns fixed features
*/
QgsFeatureList fixedFeatures() { return mFixedFeatures; }

/**
* Returns unfixed features (canceled or not handeled)
*/
QgsFeatureList unfixedFeatures() { return mUnfixedFeatures; }

public slots:
void accept() override;
void reject() override;

private:
void init( QgsVectorLayer *layer );
QString descriptionText();

QgsFeatureList mFeatures;
QgsFeatureList::iterator mCurrentFeature;

QgsFeatureList mFixedFeatures;
QgsFeatureList mUnfixedFeatures;

QgsAttributeForm *mAttributeForm = nullptr;
QProgressBar *mProgressBar = nullptr;
QLabel *mDescription = nullptr;
};

#endif // QGSFIXATTRIBUTEDIALOG_H