Skip to content

Commit

Permalink
Only convert geometries to provider type when provider does strict ty…
Browse files Browse the repository at this point in the history 
…pe checking (ie. not for shapes; fixes #16593, #16784, #16792, #16770, followup 53d90b5)
  • Loading branch information
@jef-n
jef-n committed Jul 5, 2017
1 parent 8731840 commit d19ed1c
Showing 1 changed file with 14 additions and 11 deletions.
25 changes: 14 additions & 11 deletions src/core/qgsvectorlayereditbuffer.cpp
View file
Expand Up @@ -310,19 +310,22 @@ bool QgsVectorLayerEditBuffer::commitChanges( QStringList& commitErrors )
{
if ( cap & QgsVectorDataProvider::AddFeatures )
{
QgsFeatureMap::iterator featureIt = mAddedFeatures.begin();
for ( ; featureIt != mAddedFeatures.end(); ++featureIt )
if ( provider->doesStrictFeatureTypeCheck() )
{
if ( !featureIt->geometry() ||
featureIt->geometry()->isEmpty() ||
featureIt->geometry()->wkbType() == provider->geometryType() )
continue;

if ( !provider->convertToProviderType( featureIt->geometry() ) )
QgsFeatureMap::iterator featureIt = mAddedFeatures.begin();
for ( ; featureIt != mAddedFeatures.end(); ++featureIt )
{
commitErrors << tr( "ERROR: %n feature(s) not added - geometry type is not compatible with the current layer.", "not added features count", mAddedFeatures.size() );
success = false;
break;
if ( !featureIt->geometry() ||
featureIt->geometry()->isEmpty() ||
featureIt->geometry()->wkbType() == provider->geometryType() )
continue;

if ( !provider->convertToProviderType( featureIt->geometry() ) )
{
commitErrors << tr( "ERROR: %n feature(s) not added - geometry type is not compatible with the current layer.", "not added features count", mAddedFeatures.size() );
success = false;
break;
}
}
}
}
Expand Down

9 comments on commit d19ed1c

@gioman
Copy link
Contributor

@gioman gioman commented on d19ed1c Jul 13, 2017 •
edited

Choose a reason for hiding this comment

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

@jef-n Hi Jurgen, I tested the fix on 2.18.10 nightly and it seems to work fine in general. I was anyway (by pure chance) "able" to still get a case where the copy/paste operation fails with the

Errors: ERROR: 1087 feature(s) not added - geometry type is not compatible with the current layer.

message.

This happens when you copy/paste feature between layers in different projects, and does not happen if the operation is done within the same project.

I found that while triaging

https://issues.qgis.org/issues/16822

that also shows another problem: copy/pasting between table in different projects does not retains the attributes, while is ok if the operation is done within the same project.

@jef-n
Copy link
Member Author

@jef-n jef-n commented on d19ed1c Jul 13, 2017

Choose a reason for hiding this comment

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

Pasting to shape files?

@gioman
Copy link
Contributor

@gioman gioman commented on d19ed1c Jul 13, 2017

Choose a reason for hiding this comment

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

Pasting to shape files?

yes. The tables are identical copies, and there is no issue if the operation is done within the same project (both regarding the error message on save and the lost attributes).

@jef-n
Copy link
Member Author

@jef-n jef-n commented on d19ed1c Jul 13, 2017

Choose a reason for hiding this comment

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

yes. The tables are identical copies, and there is no issue if the operation is done within the same project (both regarding the error message on save and the lost attributes).

Point features?

@gioman
Copy link
Contributor

@gioman gioman commented on d19ed1c Jul 13, 2017

Choose a reason for hiding this comment

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

Point features?

yes. Didn't tried with other type of geometries. I did tried anyway with 2.14.16 and the operation works fine regarding the geometries (but still does not retain the attributes).

@gioman
Copy link
Contributor

@gioman gioman commented on d19ed1c Jul 14, 2017

Choose a reason for hiding this comment

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

Point features?
@jef-n

Hi Jurgen, better open/re-open a ticket as a reminder or there is something "in the work"?

@jef-n
Copy link
Member Author

@jef-n jef-n commented on d19ed1c Jul 14, 2017

Choose a reason for hiding this comment

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

This commit doesn't touch point features - it just cures the regression introduced with the referenced commit. doesStrictFeatureTypeCheck returns false only for non-point shapefiles; because OGR is - or was at the point of it's introduction - as picky about single vs. multi geomety features in the point case as other data sinks; just with line strings and polygon it copes(/d?) with both.
So convertToProviderType still kicks in with points and convertToProviderType doesn't convert multi to single geometries. I guess you are trying to paste multi points to a shapefile.

@gioman
Copy link
Contributor

@gioman gioman commented on d19ed1c Jul 14, 2017 •
edited

Choose a reason for hiding this comment

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

I guess you are trying to paste multi points to a shapefile.

@jef-n

sorry, upon further tests the issue appears description appears to be different from what I wrote yesterday.

With 2.18.10 nightly and a specific point dataset (attached, just make a clone of it to test) the copy/paste/save operation fails even within the same qgis project/instance (don't know why yesterday it seemed to fail only when done between separate instances of QGIS).

With the same dataset the operation works in 2.14, works in 2.18.10 and then it broke in 2.18.10-nigthly and master.

I cannot replicate with other datasets, so it can be an edge case, but is indeed a regression. Will file a ticket to not forget about it.

points.zip

@gioman
Copy link
Contributor

@gioman gioman commented on d19ed1c Jul 14, 2017

Choose a reason for hiding this comment

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

here it goes https://issues.qgis.org/issues/16870

Please sign in to comment.