Vertex tool delete ring / part fix

Don't also delete random vertices when a part or ring gets deleted Fixes #35428 Fixes #25650
qgis · Jun 1, 2020 · 9116b4c · 9116b4c
1 parent 43e3e63
commit 9116b4c
Show file tree

Hide file tree

Showing 2 changed files with 142 additions and 23 deletions.
diff --git a/src/app/vertextool/qgsvertextool.cpp b/src/app/vertextool/qgsvertextool.cpp
@@ -2240,37 +2240,50 @@ void QgsVertexTool::deleteVertex()
 
   // de-duplicate vertices in linear rings - if there is the first vertex selected,
   // then also the last vertex will be selected - but we want just one out of the pair
-  QHash<QgsVectorLayer *, QHash<QgsFeatureId, QList<int> > >::iterator itX = toDeleteGrouped.begin();
-  for ( ; itX != toDeleteGrouped.end(); ++itX )
+  // also deselect vertices of parts or rings that will be automatically removed
+  QHash<QgsVectorLayer *, QHash<QgsFeatureId, QList<int> > >::iterator lIt = toDeleteGrouped.begin();
+  for ( ; lIt != toDeleteGrouped.end(); ++lIt )
   {
-    QgsVectorLayer *layer = itX.key();
-    QHash<QgsFeatureId, QList<int> > &featuresDict = itX.value();
+    QgsVectorLayer *layer = lIt.key();
+    QHash<QgsFeatureId, QList<int> > &featuresDict = lIt.value();
 
-    QHash<QgsFeatureId, QList<int> >::iterator it2 = featuresDict.begin();
-    for ( ; it2 != featuresDict.end(); ++it2 )
+    QHash<QgsFeatureId, QList<int> >::iterator fIt = featuresDict.begin();
+    for ( ; fIt != featuresDict.end(); ++fIt )
     {
-      QgsFeatureId fid = it2.key();
-      QList<int> &vertexIds = it2.value();
-      if ( vertexIds.count() >= 2 && layer->geometryType() == QgsWkbTypes::PolygonGeometry )
+      QgsFeatureId fid = fIt.key();
+      QList<int> &vertexIds = fIt.value();
+      if ( vertexIds.count() >= 2 && ( layer->geometryType() == QgsWkbTypes::PolygonGeometry ||
+                                       layer->geometryType() == QgsWkbTypes::LineGeometry ) )
       {
-        QSet<int> duplicateVertexIndices;
-        QgsGeometry geom = cachedGeometry( layer, fid );
-        for ( int i = 0; i < vertexIds.count(); ++i )
+        std::sort( vertexIds.begin(), vertexIds.end(), std::greater<int>() );
+        const QgsGeometry geom = cachedGeometry( layer, fid );
+        const QgsAbstractGeometry *ag = geom.constGet();
+        QVector<QVector<int>> numberOfVertices;
+        for ( int p = 0 ; p < ag->partCount() ; ++p )
+        {
+          numberOfVertices.append( QVector<int>() );
+          for ( int r = 0 ; r < ag->ringCount( p ) ; ++r )
+          {
+            numberOfVertices[p].append( ag->vertexCount( p, r ) );
+          }
+        }
+        // polygonal rings with less than 4 vertices get deleted automatically
+        // linear parts with less than 2 vertices get deleted automatically
+        // let's keep that number and don't remove vertices beyond that point
+        const int minAllowedVertices = geom.type() == QgsWkbTypes::PolygonGeometry ? 4 : 2;
+        for ( int i = vertexIds.count() - 1; i >= 0 ; --i )
         {
           QgsVertexId vid;
           if ( geom.vertexIdFromVertexNr( vertexIds[i], vid ) )
           {
-            int ringVertexCount = geom.constGet()->vertexCount( vid.part, vid.ring );
-            if ( vid.vertex == ringVertexCount - 1 )
-            {
-              // this is the last vertex of the ring - remove the first vertex from the list
-              duplicateVertexIndices << geom.vertexNrFromVertexId( QgsVertexId( vid.part, vid.ring, 0 ) );
-            }
+            // also don't try to delete the first vertex of a ring since we have already deleted the last
+            if ( numberOfVertices.at( vid.part ).at( vid.ring ) < minAllowedVertices ||
+                 ( 0 == vid.vertex && geom.type() == QgsWkbTypes::PolygonGeometry ) )
+              vertexIds.removeOne( vertexIds.at( i ) );
+            else
+              --numberOfVertices[vid.part][vid.ring];
           }
         }
-        // now delete the duplicities
-        for ( int duplicateVertexIndex : qgis::as_const( duplicateVertexIndices ) )
-          vertexIds.removeOne( duplicateVertexIndex );
       }
     }
   }

diff --git a/tests/src/app/testqgsvertextool.cpp b/tests/src/app/testqgsvertextool.cpp
@@ -145,16 +145,20 @@ class TestQgsVertexTool : public QObject
     QgsAdvancedDigitizingDockWidget *mAdvancedDigitizingDockWidget = nullptr;
     QgsVertexTool *mVertexTool = nullptr;
     QgsVectorLayer *mLayerLine = nullptr;
+    QgsVectorLayer *mLayerMultiLine = nullptr;
     QgsVectorLayer *mLayerPolygon = nullptr;
+    QgsVectorLayer *mLayerMultiPolygon = nullptr;
     QgsVectorLayer *mLayerPoint = nullptr;
     QgsVectorLayer *mLayerLineZ = nullptr;
     QgsVectorLayer *mLayerCompoundCurve = nullptr;
     QgsVectorLayer *mLayerLineReprojected = nullptr;
     QgsFeatureId mFidLineZF1 = 0;
     QgsFeatureId mFidLineZF2 = 0;
     QgsFeatureId mFidLineF1 = 0;
+    QgsFeatureId mFidMultiLineF1 = 0;
     QgsFeatureId mFidLineF13857 = 0;
     QgsFeatureId mFidPolygonF1 = 0;
+    QgsFeatureId mFidMultiPolygonF1 = 0;
     QgsFeatureId mFidPointF1 = 0;
     QgsFeatureId mFidCompoundCurveF1 = 0;
     QgsFeatureId mFidCompoundCurveF2 = 0;
@@ -186,23 +190,30 @@ void TestQgsVertexTool::initTestCase()
   // make testing layers
   mLayerLine = new QgsVectorLayer( QStringLiteral( "LineString?crs=EPSG:27700" ), QStringLiteral( "layer line" ), QStringLiteral( "memory" ) );
   QVERIFY( mLayerLine->isValid() );
+  mLayerMultiLine = new QgsVectorLayer( QStringLiteral( "MultiLineString?crs=EPSG:27700" ), QStringLiteral( "layer multiline" ), QStringLiteral( "memory" ) );
+  QVERIFY( mLayerMultiLine->isValid() );
   mLayerLineReprojected = new QgsVectorLayer( QStringLiteral( "LineString?crs=EPSG:3857" ), QStringLiteral( "layer line" ), QStringLiteral( "memory" ) );
   QVERIFY( mLayerLineReprojected->isValid() );
   mLayerPolygon = new QgsVectorLayer( QStringLiteral( "Polygon?crs=EPSG:27700" ), QStringLiteral( "layer polygon" ), QStringLiteral( "memory" ) );
   QVERIFY( mLayerPolygon->isValid() );
+  mLayerMultiPolygon = new QgsVectorLayer( QStringLiteral( "MultiPolygon?crs=EPSG:27700" ), QStringLiteral( "layer multipolygon" ), QStringLiteral( "memory" ) );
+  QVERIFY( mLayerMultiPolygon->isValid() );
   mLayerPoint = new QgsVectorLayer( QStringLiteral( "Point?crs=EPSG:27700" ), QStringLiteral( "layer point" ), QStringLiteral( "memory" ) );
   QVERIFY( mLayerPoint->isValid() );
   mLayerLineZ = new QgsVectorLayer( QStringLiteral( "LineStringZ?crs=EPSG:27700" ), QStringLiteral( "layer line" ), QStringLiteral( "memory" ) );
   QVERIFY( mLayerLineZ->isValid() );
   mLayerCompoundCurve = new QgsVectorLayer( QStringLiteral( "CompoundCurve?crs=27700" ), QStringLiteral( "layer compound curve" ), QStringLiteral( "memory" ) );
   QVERIFY( mLayerCompoundCurve->isValid() );
-  QgsProject::instance()->addMapLayers( QList<QgsMapLayer *>() << mLayerLine << mLayerPolygon << mLayerPoint << mLayerLineZ << mLayerCompoundCurve );
+  QgsProject::instance()->addMapLayers( QList<QgsMapLayer *>() << mLayerLine << mLayerMultiLine << mLayerPolygon << mLayerMultiPolygon << mLayerPoint << mLayerLineZ << mLayerCompoundCurve );
 
   QgsPolylineXY line1;
   line1 << QgsPointXY( 2, 1 ) << QgsPointXY( 1, 1 ) << QgsPointXY( 1, 3 );
   QgsFeature lineF1;
   lineF1.setGeometry( QgsGeometry::fromPolylineXY( line1 ) );
 
+  QgsFeature multiLineF1;
+  multiLineF1.setGeometry( QgsGeometry::fromWkt( "MultiLineString ((3 1, 3 2),(3 3, 3 4))" ) );
+
   QgsCoordinateTransform ct( mLayerLine->crs(), mLayerLineReprojected->crs(), QgsCoordinateTransformContext() );
   QgsGeometry line3857 = lineF1.geometry();
   line3857.transform( ct );
@@ -216,6 +227,9 @@ void TestQgsVertexTool::initTestCase()
   QgsFeature polygonF1;
   polygonF1.setGeometry( QgsGeometry::fromPolygonXY( polygon1 ) );
 
+  QgsFeature multiPolygonF1;
+  multiPolygonF1.setGeometry( QgsGeometry::fromWkt( "MultiPolygon (((1 5, 2 5, 2 6.5, 2 8, 1 8, 1 6.5, 1 5),(1.25 5.5, 1.25 6, 1.75 6, 1.75 5.5, 1.25 5.5),(1.25 7, 1.75 7, 1.75 7.5, 1.25 7.5, 1.25 7)),((3 5, 3 6.5, 3 8, 4 8, 4 6.5, 4 5, 3 5),(3.25 5.5, 3.75 5.5, 3.75 6, 3.25 6, 3.25 5.5),(3.25 7, 3.75 7, 3.75 7.5, 3.25 7.5, 3.25 7)))" ) );
+
   QgsFeature pointF1;
   pointF1.setGeometry( QgsGeometry::fromPointXY( QgsPointXY( 2, 3 ) ) );
 
@@ -246,6 +260,11 @@ void TestQgsVertexTool::initTestCase()
   mFidLineF1 = lineF1.id();
   QCOMPARE( mLayerLine->featureCount(), ( long )1 );
 
+  mLayerMultiLine->startEditing();
+  mLayerMultiLine->addFeature( multiLineF1 );
+  mFidMultiLineF1 = multiLineF1.id();
+  QCOMPARE( mLayerMultiLine->featureCount(), ( long )1 );
+
   mLayerLineReprojected->startEditing();
   mLayerLineReprojected->addFeature( lineF13857 );
   mFidLineF13857 = lineF13857.id();
@@ -256,6 +275,11 @@ void TestQgsVertexTool::initTestCase()
   mFidPolygonF1 = polygonF1.id();
   QCOMPARE( mLayerPolygon->featureCount(), ( long )1 );
 
+  mLayerMultiPolygon->startEditing();
+  mLayerMultiPolygon->addFeature( multiPolygonF1 );
+  mFidMultiPolygonF1 = multiPolygonF1.id();
+  QCOMPARE( mLayerMultiPolygon->featureCount(), ( long )1 );
+
   mLayerPoint->startEditing();
   mLayerPoint->addFeature( pointF1 );
   mFidPointF1 = pointF1.id();
@@ -277,7 +301,9 @@ void TestQgsVertexTool::initTestCase()
 
   // just one added feature in each undo stack
   QCOMPARE( mLayerLine->undoStack()->index(), 1 );
+  QCOMPARE( mLayerMultiLine->undoStack()->index(), 1 );
   QCOMPARE( mLayerPolygon->undoStack()->index(), 1 );
+  QCOMPARE( mLayerMultiPolygon->undoStack()->index(), 1 );
   QCOMPARE( mLayerPoint->undoStack()->index(), 1 );
   // except for layerLineZ
   QCOMPARE( mLayerLineZ->undoStack()->index(), 2 );
@@ -291,14 +317,16 @@ void TestQgsVertexTool::initTestCase()
   QCOMPARE( mCanvas->mapSettings().outputSize(), QSize( 512, 512 ) );
   QCOMPARE( mCanvas->mapSettings().visibleExtent(), QgsRectangle( 0, 0, 8, 8 ) );
 
-  mCanvas->setLayers( QList<QgsMapLayer *>() << mLayerLine << mLayerLineReprojected << mLayerPolygon << mLayerPoint << mLayerLineZ << mLayerCompoundCurve );
+  mCanvas->setLayers( QList<QgsMapLayer *>() << mLayerLine << mLayerMultiLine << mLayerLineReprojected << mLayerPolygon << mLayerMultiPolygon << mLayerPoint << mLayerLineZ << mLayerCompoundCurve );
 
   QgsMapCanvasSnappingUtils *snappingUtils = new QgsMapCanvasSnappingUtils( mCanvas, this );
   mCanvas->setSnappingUtils( snappingUtils );
 
   snappingUtils->locatorForLayer( mLayerLine )->init();
+  snappingUtils->locatorForLayer( mLayerMultiLine )->init();
   snappingUtils->locatorForLayer( mLayerLineReprojected )->init();
   snappingUtils->locatorForLayer( mLayerPolygon )->init();
+  snappingUtils->locatorForLayer( mLayerMultiPolygon )->init();
   snappingUtils->locatorForLayer( mLayerPoint )->init();
   snappingUtils->locatorForLayer( mLayerLineZ )->init();
   snappingUtils->locatorForLayer( mLayerCompoundCurve )->init();
@@ -683,6 +711,84 @@ void TestQgsVertexTool::testDeleteVertex()
 
   QCOMPARE( mLayerLine->getFeature( mFidLineF1 ).geometry(), QgsGeometry::fromWkt( "LINESTRING(2 1, 1 1, 1 3)" ) );
 
+  // delete multiline part by dragging
+
+  mousePress( 2.5, 0.5, Qt::LeftButton );
+  mouseMove( 3.5, 2.5 );
+  mouseRelease( 3.5, 2.5, Qt::LeftButton );
+  keyClick( Qt::Key_Delete );
+
+  QCOMPARE( mLayerMultiLine->undoStack()->index(), 2 );
+  QCOMPARE( mLayerMultiLine->getFeature( mFidMultiLineF1 ).geometry(), QgsGeometry::fromWkt( "MultiLineString ((3 3, 3 4))" ) );
+
+  mLayerMultiLine->undoStack()->undo();
+  QCOMPARE( mLayerMultiLine->undoStack()->index(), 1 );
+
+  // delete multiline part by dragging and leaving only one vertex to the part
+
+  mousePress( 2.5, 0.5, Qt::LeftButton );
+  mouseMove( 3.5, 1.5 );
+  mouseRelease( 3.5, 1.5, Qt::LeftButton );
+  keyClick( Qt::Key_Delete );
+
+  QCOMPARE( mLayerMultiLine->undoStack()->index(), 2 );
+  QCOMPARE( mLayerMultiLine->getFeature( mFidMultiLineF1 ).geometry(), QgsGeometry::fromWkt( "MultiLineString ((3 3, 3 4))" ) );
+
+  mLayerMultiLine->undoStack()->undo();
+  QCOMPARE( mLayerMultiLine->undoStack()->index(), 1 );
+
+  // delete inner ring by dragging
+
+  mousePress( 1.1, 5.1, Qt::LeftButton );
+  mouseMove( 1.8, 6.2 );
+  mouseRelease( 1.8, 6.2, Qt::LeftButton );
+  keyClick( Qt::Key_Delete );
+
+  QCOMPARE( mLayerMultiPolygon->undoStack()->index(), 2 );
+  QCOMPARE( mLayerMultiPolygon->getFeature( mFidMultiPolygonF1 ).geometry(), QgsGeometry::fromWkt( "MultiPolygon (((1 5, 2 5, 2 6.5, 2 8, 1 8, 1 6.5, 1 5),(1.25 7, 1.75 7, 1.75 7.5, 1.25 7.5, 1.25 7)),((3 5, 3 6.5, 3 8, 4 8, 4 6.5, 4 5, 3 5),(3.25 5.5, 3.75 5.5, 3.75 6, 3.25 6, 3.25 5.5),(3.25 7, 3.75 7, 3.75 7.5, 3.25 7.5, 3.25 7)))" ) );
+
+  mLayerMultiPolygon->undoStack()->undo();
+  QCOMPARE( mLayerMultiPolygon->undoStack()->index(), 1 );
+
+  // delete inner ring by dragging and leaving less than 4 vertices to the ring
+
+  mousePress( 1.1, 5.1, Qt::LeftButton );
+  mouseMove( 1.8, 5.7 );
+  mouseRelease( 1.8, 5.7, Qt::LeftButton );
+  keyClick( Qt::Key_Delete );
+
+  QCOMPARE( mLayerMultiPolygon->undoStack()->index(), 2 );
+  QCOMPARE( mLayerMultiPolygon->getFeature( mFidMultiPolygonF1 ).geometry(), QgsGeometry::fromWkt( "MultiPolygon (((1 5, 2 5, 2 6.5, 2 8, 1 8, 1 6.5, 1 5),(1.25 7, 1.75 7, 1.75 7.5, 1.25 7.5, 1.25 7)),((3 5, 3 6.5, 3 8, 4 8, 4 6.5, 4 5, 3 5),(3.25 5.5, 3.75 5.5, 3.75 6, 3.25 6, 3.25 5.5),(3.25 7, 3.75 7, 3.75 7.5, 3.25 7.5, 3.25 7)))" ) );
+
+  mLayerMultiPolygon->undoStack()->undo();
+  QCOMPARE( mLayerMultiPolygon->undoStack()->index(), 1 );
+
+  // delete part and rings by dragging
+
+  mousePress( 0.5, 4.5, Qt::LeftButton );
+  mouseMove( 2.5, 8.5 );
+  mouseRelease( 2.5, 8.5, Qt::LeftButton );
+  keyClick( Qt::Key_Delete );
+
+  QCOMPARE( mLayerMultiPolygon->undoStack()->index(), 2 );
+  QCOMPARE( mLayerMultiPolygon->getFeature( mFidMultiPolygonF1 ).geometry(), QgsGeometry::fromWkt( "MultiPolygon (((3 5, 3 6.5, 3 8, 4 8, 4 6.5, 4 5, 3 5),(3.25 5.5, 3.75 5.5, 3.75 6, 3.25 6, 3.25 5.5),(3.25 7, 3.75 7, 3.75 7.5, 3.25 7.5, 3.25 7)))" ) );
+
+  mLayerMultiPolygon->undoStack()->undo();
+  QCOMPARE( mLayerMultiPolygon->undoStack()->index(), 1 );
+
+  // combined delete rings from different parts by dragging
+
+  mousePress( 0.5, 4.5, Qt::LeftButton );
+  mouseMove( 4.5, 6.2 );
+  mouseRelease( 4.5, 6.2, Qt::LeftButton );
+  keyClick( Qt::Key_Delete );
+
+  QCOMPARE( mLayerMultiPolygon->undoStack()->index(), 2 );
+  QCOMPARE( mLayerMultiPolygon->getFeature( mFidMultiPolygonF1 ).geometry(), QgsGeometry::fromWkt( "MultiPolygon (((1 6.5, 2 6.5, 2 8, 1 8, 1 6.5),(1.25 7, 1.75 7, 1.75 7.5, 1.25 7.5, 1.25 7)),((4 6.5, 3 6.5, 3 8, 4 8, 4 6.5),(3.25 7, 3.75 7, 3.75 7.5, 3.25 7.5, 3.25 7)))" ) );
+
+  mLayerMultiPolygon->undoStack()->undo();
+  QCOMPARE( mLayerMultiPolygon->undoStack()->index(), 1 );
+
   // no other unexpected changes happened
   QCOMPARE( mLayerLine->undoStack()->index(), 1 );
   QCOMPARE( mLayerPolygon->undoStack()->index(), 1 );