Fix using add part tool to add part to geometryless rows

(fix #12885, #11319) Also fix some potential crashes with edit tools and null geometry
qgis · Oct 13, 2015 · ad10b52 · ad10b52
1 parent 6f860d0
commit ad10b52
Show file tree

Hide file tree

Showing 6 changed files with 236 additions and 46 deletions.
diff --git a/python/core/geometry/qgsgeometry.sip b/python/core/geometry/qgsgeometry.sip
@@ -277,15 +277,21 @@ class QgsGeometry
      3 ring is not valid geometry, 4 ring not disjoint with existing rings, 5 no polygon found which contained the ring*/
     int addRing( QgsCurveV2* ring );
 
-    /** Adds a new island polygon to a multipolygon feature
-     @return 0 in case of success, 1 if not a multipolygon, 2 if ring is not a valid geometry, 3 if new polygon ring
-     not disjoint with existing polygons of the feature*/
+    /** Adds a new part to a the geometry.
+     * @param points points describing part to add
+     * @param geomType default geometry type to create if no existing geometry
+     * @returns 0 in case of success, 1 if not a multipolygon, 2 if ring is not a valid geometry, 3 if new polygon ring
+     * not disjoint with existing polygons of the feature
+    */
     int addPart( const QList<QgsPoint> &points, QGis::GeometryType geomType = QGis::UnknownGeometry );
 
-    /** Adds a new part to this geometry (takes ownership)
-     @return 0 in case of success, 1 if not a multipolygon, 2 if ring is not a valid geometry, 3 if new polygon ring
-     not disjoint with existing polygons of the feature*/
-    int addPart( QgsAbstractGeometryV2* part /Transfer/ );
+    /** Adds a new part to this geometry.
+     * @param part part to add (ownership is transferred)
+     * @param geomType default geometry type to create if no existing geometry
+     * @returns 0 in case of success, 1 if not a multipolygon, 2 if ring is not a valid geometry, 3 if new polygon ring
+     * not disjoint with existing polygons of the feature
+    */
+    int addPart( QgsAbstractGeometryV2* part /Transfer/, QGis::GeometryType geomType = QGis::UnknownGeometry );
 
     /** Adds a new island polygon to a multipolygon feature
      @return 0 in case of success, 1 if not a multipolygon, 2 if ring is not a valid geometry, 3 if new polygon ring
@@ -518,6 +524,17 @@ class QgsGeometry
     @return true in case of success and false else*/
     bool convertToMultiType();
 
+    /**
+     * Converts multi type geometry into single type geometry
+     * e.g. a multipolygon into a polygon geometry. Only the first part of the
+     * multi geometry will be retained.
+     * If it is already a single part geometry, it will return true and
+     * not change the geometry.
+     *
+     * @return true in case of success and false else
+     */
+    bool convertToSingleType();
+
     /** Modifies geometry to avoid intersections with the layers specified in project properties
      *  @return 0 in case of success,
      *          1 if geometry is not of polygon type,

diff --git a/src/core/geometry/qgsgeometry.cpp b/src/core/geometry/qgsgeometry.cpp
@@ -590,6 +590,24 @@ int QgsGeometry::addRing( QgsCurveV2* ring )
 }
 
 int QgsGeometry::addPart( const QList<QgsPoint> &points, QGis::GeometryType geomType )
+{
+  QgsAbstractGeometryV2* partGeom = 0;
+  if ( points.size() == 1 )
+  {
+    partGeom = new QgsPointV2( points[0].x(), points[0].y() );
+  }
+  else if ( points.size() > 1 )
+  {
+    QgsLineStringV2* ringLine = new QgsLineStringV2();
+    QList< QgsPointV2 > partPoints;
+    convertPointList( points, partPoints );
+    ringLine->setPoints( partPoints );
+    partGeom = ringLine;
+  }
+  return addPart( partGeom, geomType );
+}
+
+int QgsGeometry::addPart( QgsAbstractGeometryV2* part, QGis::GeometryType geomType )
 {
   if ( !d )
   {
@@ -614,29 +632,13 @@ int QgsGeometry::addPart( const QList<QgsPoint> &points, QGis::GeometryType geom
         return 1;
     }
   }
-
-  convertToMultiType();
-
-  QgsAbstractGeometryV2* partGeom = 0;
-  if ( points.size() == 1 )
-  {
-    partGeom = new QgsPointV2( points[0].x(), points[0].y() );
-  }
-  else if ( points.size() > 1 )
+  else
   {
-    QgsLineStringV2* ringLine = new QgsLineStringV2();
-    QList< QgsPointV2 > partPoints;
-    convertPointList( points, partPoints );
-    ringLine->setPoints( partPoints );
-    partGeom = ringLine;
+    detach( true );
+    removeWkbGeos();
   }
-  return addPart( partGeom );
-}
 
-int QgsGeometry::addPart( QgsAbstractGeometryV2* part )
-{
-  detach( true );
-  removeWkbGeos();
+  convertToMultiType();
   return QgsGeometryEditUtils::addPart( d->geometry, part );
 }
 
@@ -958,6 +960,30 @@ bool QgsGeometry::convertToMultiType()
   return true;
 }
 
+bool QgsGeometry::convertToSingleType()
+{
+  if ( !d || !d->geometry )
+  {
+    return false;
+  }
+
+  if ( !isMultipart() ) //already single part, no need to convert
+  {
+    return true;
+  }
+
+  QgsGeometryCollectionV2* multiGeom = dynamic_cast<QgsGeometryCollectionV2*>( d->geometry );
+  if ( multiGeom->partCount() < 1 )
+    return false;
+
+  QgsAbstractGeometryV2* firstPart = multiGeom->geometryN( 0 )->clone();
+  detach( false );
+
+  d->geometry = firstPart;
+  removeWkbGeos();
+  return true;
+}
+
 QgsPoint QgsGeometry::asPoint() const
 {
   if ( !d || !d->geometry || d->geometry->geometryType() != "Point" )

diff --git a/src/core/geometry/qgsgeometry.h b/src/core/geometry/qgsgeometry.h
@@ -312,15 +312,21 @@ class CORE_EXPORT QgsGeometry
      3 ring is not valid geometry, 4 ring not disjoint with existing rings, 5 no polygon found which contained the ring*/
     int addRing( QgsCurveV2* ring );
 
-    /** Adds a new island polygon to a multipolygon feature
-     @return 0 in case of success, 1 if not a multipolygon, 2 if ring is not a valid geometry, 3 if new polygon ring
-     not disjoint with existing polygons of the feature*/
+    /** Adds a new part to a the geometry.
+     * @param points points describing part to add
+     * @param geomType default geometry type to create if no existing geometry
+     * @returns 0 in case of success, 1 if not a multipolygon, 2 if ring is not a valid geometry, 3 if new polygon ring
+     * not disjoint with existing polygons of the feature
+    */
     int addPart( const QList<QgsPoint> &points, QGis::GeometryType geomType = QGis::UnknownGeometry );
 
-    /** Adds a new part to this geometry (takes ownership)
-     @return 0 in case of success, 1 if not a multipolygon, 2 if ring is not a valid geometry, 3 if new polygon ring
-     not disjoint with existing polygons of the feature*/
-    int addPart( QgsAbstractGeometryV2* part );
+    /** Adds a new part to this geometry.
+     * @param part part to add (ownership is transferred)
+     * @param geomType default geometry type to create if no existing geometry
+     * @returns 0 in case of success, 1 if not a multipolygon, 2 if ring is not a valid geometry, 3 if new polygon ring
+     * not disjoint with existing polygons of the feature
+    */
+    int addPart( QgsAbstractGeometryV2* part, QGis::GeometryType geomType = QGis::UnknownGeometry );
 
     /** Adds a new island polygon to a multipolygon feature
      @return 0 in case of success, 1 if not a multipolygon, 2 if ring is not a valid geometry, 3 if new polygon ring
@@ -558,6 +564,17 @@ class CORE_EXPORT QgsGeometry
      */
     bool convertToMultiType();
 
+    /**
+     * Converts multi type geometry into single type geometry
+     * e.g. a multipolygon into a polygon geometry. Only the first part of the
+     * multi geometry will be retained.
+     * If it is already a single part geometry, it will return true and
+     * not change the geometry.
+     *
+     * @return true in case of success and false else
+     */
+    bool convertToSingleType();
+
     /** Modifies geometry to avoid intersections with the layers specified in project properties
      *  @return 0 in case of success,
      *          1 if geometry is not of polygon type,

diff --git a/src/core/qgsvectorlayereditutils.cpp b/src/core/qgsvectorlayereditutils.cpp
@@ -20,6 +20,9 @@
 #include "qgslinestringv2.h"
 #include "qgslogger.h"
 #include "qgspointv2.h"
+#include "qgsgeometryfactory.h"
+#include "qgis.h"
+#include "qgswkbtypes.h"
 
 #include <limits>
 
@@ -144,6 +147,9 @@ int QgsVectorLayerEditUtils::addRing( QgsCurveV2* ring, const QgsFeatureIds& tar
   //find first valid feature we can add the ring to
   while ( fit.nextFeature( f ) )
   {
+    if ( !f.constGeometry() )
+      continue;
+
     //add ring takes ownership of ring, and deletes it if there's an error
     addRingReturnCode = f.geometry()->addRing( static_cast< QgsCurveV2* >( ring->clone() ) );
     if ( addRingReturnCode == 0 )
@@ -167,21 +173,34 @@ int QgsVectorLayerEditUtils::addPart( const QList<QgsPoint> &points, QgsFeatureI
     return 6;
 
   QgsGeometry geometry;
+  bool firstPart = false;
   if ( !cache()->geometry( featureId, geometry ) ) // maybe it's in cache
   {
     // it's not in cache: let's fetch it from layer
     QgsFeature f;
-    if ( !L->getFeatures( QgsFeatureRequest().setFilterFid( featureId ).setSubsetOfAttributes( QgsAttributeList() ) ).nextFeature( f ) || !f.geometry() )
-      return 6; //geometry not found
+    if ( !L->getFeatures( QgsFeatureRequest().setFilterFid( featureId ).setSubsetOfAttributes( QgsAttributeList() ) ).nextFeature( f ) )
+      return 6; //not found
 
-    geometry = *f.geometry();
+    if ( !f.constGeometry() )
+    {
+      //no existing geometry, so adding first part to null geometry
+      firstPart = true;
+    }
+    else
+    {
+      geometry = *f.geometry();
+    }
   }
 
-  geometry.convertToMultiType();
-
   int errorCode = geometry.addPart( points, L->geometryType() );
   if ( errorCode == 0 )
   {
+    if ( firstPart && QgsWKBTypes::isSingleType( QGis::fromOldWkbType( L->wkbType() ) )
+         && L->dataProvider()->doesStrictFeatureTypeCheck() )
+    {
+      //convert back to single part if required by layer
+      geometry.convertToSingleType();
+    }
     L->editBuffer()->changeGeometry( featureId, &geometry );
   }
   return errorCode;
@@ -193,21 +212,34 @@ int QgsVectorLayerEditUtils::addPart( QgsCurveV2* ring, QgsFeatureId featureId )
     return 6;
 
   QgsGeometry geometry;
+  bool firstPart = false;
   if ( !cache()->geometry( featureId, geometry ) ) // maybe it's in cache
   {
     // it's not in cache: let's fetch it from layer
     QgsFeature f;
-    if ( !L->getFeatures( QgsFeatureRequest().setFilterFid( featureId ).setSubsetOfAttributes( QgsAttributeList() ) ).nextFeature( f ) || !f.geometry() )
-      return 6; //geometry not found
+    if ( !L->getFeatures( QgsFeatureRequest().setFilterFid( featureId ).setSubsetOfAttributes( QgsAttributeList() ) ).nextFeature( f ) )
+      return 6; //not found
 
-    geometry = *f.geometry();
+    if ( !f.constGeometry() )
+    {
+      //no existing geometry, so adding first part to null geometry
+      firstPart = true;
+    }
+    else
+    {
+      geometry = *f.geometry();
+    }
   }
 
-  geometry.convertToMultiType();
-
-  int errorCode = geometry.addPart( ring );
+  int errorCode = geometry.addPart( ring, L->geometryType() );
   if ( errorCode == 0 )
   {
+    if ( firstPart && QgsWKBTypes::isSingleType( QGis::fromOldWkbType( L->wkbType() ) )
+         && L->dataProvider()->doesStrictFeatureTypeCheck() )
+    {
+      //convert back to single part if required by layer
+      geometry.convertToSingleType();
+    }
     L->editBuffer()->changeGeometry( featureId, &geometry );
   }
   return errorCode;

diff --git a/src/core/qgsvectorlayerundocommand.cpp b/src/core/qgsvectorlayerundocommand.cpp
@@ -119,7 +119,7 @@ QgsVectorLayerUndoCommandChangeGeometry::QgsVectorLayerUndoCommandChangeGeometry
   {
     QgsFeatureMap::const_iterator it = mBuffer->mAddedFeatures.find( mFid );
     Q_ASSERT( it != mBuffer->mAddedFeatures.end() );
-    mOldGeom = new QgsGeometry( *it.value().constGeometry() );
+    mOldGeom = ( it.value().constGeometry() ? new QgsGeometry( *it.value().constGeometry() ) : 0 );
   }
   else
   {