Fix invalid attributes dialog on copy to another layer #33688
Conversation
with the option to cancel all or store invalid anyway
…ist of features that are not stored yet there for fixing the attributes
src/app/qgisapp.cpp
Outdated
| if ( !QgsVectorLayerUtils::validateAttribute( pasteVectorLayer, f, idx, errors, QgsFieldConstraints::ConstraintStrengthHard, QgsFieldConstraints::ConstraintOriginNotSet ) ) | ||
| { | ||
| invalidFeatures << f; | ||
| validFeatures.removeOne( f ); |
There was a problem hiding this comment.
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.
src/app/qgisapp.cpp
Outdated
| // check constraints | ||
| QgsFeatureList validFeatures = newFeatures; | ||
| QgsFeatureList invalidFeatures; | ||
| for ( const QgsFeature &f : qgis::as_const( validFeatures ) ) |
There was a problem hiding this comment.
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.
src/app/qgisapp.cpp
Outdated
| QgsFixAttributeDialog *dialog = new QgsFixAttributeDialog( pasteVectorLayer, invalidFeatures, this ); | ||
| int feedback = dialog->exec(); | ||
|
|
||
| if ( feedback == QgsFixAttributeDialog::VanishAll ) |
src/gui/qgsfixattributedialog.cpp
Outdated
| void QgsFixAttributeDialog::init( QgsVectorLayer *layer ) | ||
| { | ||
| QgsAttributeEditorContext context; | ||
| setWindowTitle( tr( "%1 - Fix Pastet Features" ).arg( layer->name() ) ); |
src/gui/qgsfixattributedialog.cpp
Outdated
| layout()->addWidget( mAttributeForm ); | ||
|
|
||
| QDialogButtonBox *buttonBox = mAttributeForm->findChild<QDialogButtonBox *>(); | ||
| QPushButton *cancelAllBtn = new QPushButton( tr( "Cancel all" ) ); |
There was a problem hiding this comment.
Fix capitalisation for buttons (should be title case)
There was a problem hiding this comment.
Actually these strings are a bit confusing. I'd go with "discard all", "discard all invalid features" and "paste all (including invalid)".
There was a problem hiding this comment.
@nirvn these buttons are very long - got any ideas on alternative ui?
src/gui/qgsfixattributedialog.h
Outdated
| * \since 3.12 | ||
| */ | ||
|
|
||
| class GUI_EXPORT QgsFixAttributeDialog : public QDialog |
There was a problem hiding this comment.
Maybe leave in app for now?
src/gui/qgsfixattributedialog.h
Outdated
| */ | ||
| enum Feedback | ||
| { | ||
| VanishAll, //!< Feedback to cancel copy of all features (even valid ones) |
better naming of the buttons and the enums
signedav
added
the
Needs Documentation
|
@signedav A documentation ticket will be opened at https://github.com/qgis/QGIS-Documentation when this PR is merged. Please update the description (not the comments) with helpful description and screenshot to help the work from documentors. Thank you! |
|
Looks good to me |
|
Thanks @nyalldawson |
|
@signedav |
timlinux
added
the
Changelog
|
@signedav A documentation ticket will be opened at https://github.com/qgis/QGIS-Documentation when this PR is merged. Please update the description (not the comments) with helpful description and screenshot to help the work from documentors. Thank you! |
|
@signedav |
timlinux
removed
the
Changelog
It's possible to copy features from one layer to another.
If there are the same fields in the destination layer, then the attributes for them are taken from the original feature. If not, the default value is taken. Otherwise the new attribute is null.
If the destination layer has constraints on the fields, these should be fulfilled now or disregarded on purpose. But not just copied invalid like it used to do.
That's why now the attributes are checked against the constraints. And for all the invalid features a dialog pops up.
And on pasting only one feature, the options are reduced:
Maybe there could be added in future a multi edit function.