Node tool: improvements for deletion of vertices

- fix issue when deleting the repeated first+last vertex of polygon's ring
- make deletion aware of topological editing (also delete other vertices at the same position)
- fix issue with dragging not being stopped when deleting multiple vertices
- make deletion in node editor follow the same code path
- unit tests for topological editing (moving / deleting vertices)

src/app/nodetool/qgsnodetool.cpp

@@ -945,13 +945,27 @@ void QgsNodeTool::deleteNodeEditorSelection()
   if ( firstSelectedIndex == -1 )
     return;
 
-  mSelectedFeature->deleteSelectedVertexes();
-
-  if ( mSelectedFeature->geometry()->isNull() )
+  // make a list of selected vertices
+  QList<Vertex> nodes;
+  QList<QgsVertexEntry *> &selFeatureVertices = mSelectedFeature->vertexMap();
+  QgsVectorLayer *layer = mSelectedFeature->vlayer();
+  QgsFeatureId fid = mSelectedFeature->featureId();
+  QgsGeometry geometry = cachedGeometry( layer, fid );
+  Q_FOREACH ( QgsVertexEntry *vertex, selFeatureVertices )
   {
-    emit messageEmitted( tr( "Geometry has been cleared. Use the add part tool to set geometry for this feature." ) );
+    if ( vertex->isSelected() )
+    {
+      int vertexIndex = geometry.vertexNrFromVertexId( vertex->vertexId() );
+      if ( vertexIndex != -1 )
+        nodes.append( Vertex( layer, fid, vertexIndex ) );
+    }
   }
-  else
+
+  // now select the vertices and delete them...
+  setHighlightedNodes( nodes );
+  deleteVertex();
+
+  if ( !mSelectedFeature->geometry()->isNull() )
   {
     int nextVertexToSelect = firstSelectedIndex;
     if ( mSelectedFeature->geometry()->type() == QgsWkbTypes::LineGeometry )
@@ -1475,24 +1489,48 @@ void QgsNodeTool::applyEditsToLayers( QgsNodeTool::NodeEdits &edits )
 
 void QgsNodeTool::deleteVertex()
 {
-  QList<Vertex> toDelete;
+  QSet<Vertex> toDelete;
   if ( !mSelectedNodes.isEmpty() )
   {
-    toDelete = mSelectedNodes;
+    toDelete = QSet<Vertex>::fromList( mSelectedNodes );
   }
   else
   {
     bool addingVertex = mDraggingVertexType == AddingVertex || mDraggingVertexType == AddingEndpoint;
     toDelete << *mDraggingVertex;
-    toDelete += mDraggingExtraVertices;
-    stopDragging();
+    toDelete += QSet<Vertex>::fromList( mDraggingExtraVertices );
 
     if ( addingVertex )
+    {
+      stopDragging();
       return;   // just cancel the vertex
+    }
   }
 
+  stopDragging();
   setHighlightedNodes( QList<Vertex>() ); // reset selection
 
+  if ( QgsProject::instance()->topologicalEditing() )
+  {
+    // if topo editing is enabled, delete all the vertices that are on the same location
+    QSet<Vertex> topoVerticesToDelete;
+    Q_FOREACH ( const Vertex &vertexToDelete, toDelete )
+    {
+      QgsPoint layerPt = cachedGeometryForVertex( vertexToDelete ).vertexAt( vertexToDelete.vertexId );
+      QgsPoint mapPt = toMapCoordinates( vertexToDelete.layer, layerPt );
+      Q_FOREACH ( const QgsPointLocator::Match &otherMatch, layerVerticesSnappedToPoint( vertexToDelete.layer, mapPt ) )
+      {
+        Vertex otherVertex( otherMatch.layer(), otherMatch.featureId(), otherMatch.vertexIndex() );
+        if ( toDelete.contains( otherVertex ) || topoVerticesToDelete.contains( otherVertex ) )
+          continue;
+
+        topoVerticesToDelete.insert( otherVertex );
+      }
+    }
+
+    toDelete.unite( topoVerticesToDelete );
+  }
+
   // switch from a plain list to dictionary { layer: { fid: [vertexNr1, vertexNr2, ...] } }
   QHash<QgsVectorLayer *, QHash<QgsFeatureId, QList<int> > > toDeleteGrouped;
   Q_FOREACH ( const Vertex &vertex, toDelete )
@@ -1500,6 +1538,41 @@ void QgsNodeTool::deleteVertex()
     toDeleteGrouped[vertex.layer][vertex.fid].append( vertex.vertexId );
   }
 
+  // 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 )
+  {
+    QgsVectorLayer *layer = itX.key();
+    QHash<QgsFeatureId, QList<int> > &featuresDict = itX.value();
+
+    QHash<QgsFeatureId, QList<int> >::iterator it2 = featuresDict.begin();
+    for ( ; it2 != featuresDict.end(); ++it2 )
+    {
+      QgsFeatureId fid = it2.key();
+      QList<int> &vertexIds = it2.value();
+      if ( vertexIds.count() >= 2 && layer->geometryType() == QgsWkbTypes::PolygonGeometry )
+      {
+        QSet<int> duplicateVertexIndices;
+        QgsGeometry geom = cachedGeometry( layer, fid );
+        for ( int i = 0; i < vertexIds.count(); ++i )
+        {
+          QgsVertexId vid;
+          geom.vertexIdFromVertexNr( vertexIds[i], vid );
+          int ringVertexCount = geom.geometry()->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 ) );
+          }
+        }
+        // now delete the duplicities
+        Q_FOREACH ( int duplicateVertexIndex, duplicateVertexIndices )
+          vertexIds.removeOne( duplicateVertexIndex );
+      }
+    }
+  }
+
   // main for cycle to delete all selected vertices
   QHash<QgsVectorLayer *, QHash<QgsFeatureId, QList<int> > >::iterator it = toDeleteGrouped.begin();
   for ( ; it != toDeleteGrouped.end(); ++it )
@@ -1528,6 +1601,11 @@ void QgsNodeTool::deleteVertex()
           success = false;
         }
       }
+
+      if ( res == QgsVectorLayer::EmptyGeometry )
+      {
+        emit messageEmitted( tr( "Geometry has been cleared. Use the add part tool to set geometry for this feature." ) );
+      }
     }
 
     if ( success )
@@ -1545,7 +1623,7 @@ void QgsNodeTool::deleteVertex()
   // pre-select next node for deletion if we are deleting just one node
   if ( toDelete.count() == 1 )
   {
-    const Vertex &vertex = toDelete[0];
+    const Vertex &vertex = *toDelete.constBegin();
     QgsGeometry geom( cachedGeometryForVertex( vertex ) );
     int vertexId = vertex.vertexId;
 

src/app/nodetool/qgsselectedfeature.cpp

@@ -223,91 +223,6 @@ void QgsSelectedFeature::validationFinished()
   sb->showMessage( tr( "Validation finished (%n error(s) found).", "number of geometry errors", mGeomErrorMarkers.size() ) );
 }
 
-void QgsSelectedFeature::deleteSelectedVertexes()
-{
-  int nSelected = 0;
-  Q_FOREACH ( QgsVertexEntry *entry, mVertexMap )
-  {
-    if ( entry->isSelected() )
-      nSelected++;
-  }
-
-  if ( nSelected == 0 )
-    return;
-
-  bool topologicalEditing = QgsProject::instance()->topologicalEditing();
-  QMultiMap<double, QgsSnappingResult> currentResultList;
-
-  mVlayer->beginEditCommand( QObject::tr( "Deleted vertices" ) );
-
-  beginGeometryChange();
-
-  bool success = false;
-  QgsVectorLayer::EditResult res = QgsVectorLayer::Success;
-  for ( int i = mVertexMap.size() - 1; i > -1 && nSelected > 0; i-- )
-  {
-    if ( mVertexMap.at( i )->isSelected() )
-    {
-      if ( topologicalEditing )
-      {
-        // snap from current vertex
-        currentResultList.clear();
-        mVlayer->snapWithContext( mVertexMap.at( i )->pointV1(), ZERO_TOLERANCE, currentResultList, QgsSnappingResult::SnapToVertex );
-      }
-
-      // only last update should trigger the geometry update
-      // as vertex selection gets lost on the update
-      if ( --nSelected == 0 )
-        endGeometryChange();
-
-      if ( res != QgsVectorLayer::EmptyGeometry )
-        res = mVlayer->deleteVertex( mFeatureId, i );
-
-      if ( res != QgsVectorLayer::Success && res != QgsVectorLayer::EmptyGeometry )
-      {
-        success = false;
-        QgsDebugMsg( QString( "Deleting vertex %1 failed - resetting" ).arg( i ) );
-        break;
-      }
-
-      success = true;
-
-      if ( topologicalEditing )
-      {
-        QMultiMap<double, QgsSnappingResult>::iterator resultIt =  currentResultList.begin();
-
-        for ( ; resultIt != currentResultList.end(); ++resultIt )
-        {
-          // move all other
-          if ( mFeatureId !=  resultIt.value().snappedAtGeometry )
-            mVlayer->deleteVertex( resultIt.value().snappedAtGeometry, resultIt.value().snappedVertexNr );
-        }
-      }
-
-      if ( res == QgsVectorLayer::EmptyGeometry )
-      {
-        //geometry has been cleared as a result of deleting vertices (e.g., not enough vertices left to leave a valid geometry),
-        //so nothing more to do
-        QgsGeometry empty;
-        geometryChanged( mFeatureId, empty );
-        break;
-      }
-    }
-  }
-
-  if ( nSelected > 0 )
-    endGeometryChange();
-
-  if ( success )
-  {
-    mVlayer->endEditCommand();
-  }
-  else
-  {
-    mVlayer->destroyEditCommand();
-  }
-}
-
 void QgsSelectedFeature::replaceVertexMap()
 {
   // delete old map

src/app/nodetool/qgsselectedfeature.h

@@ -71,11 +71,6 @@ class QgsSelectedFeature: public QObject
      */
     void deselectAllVertexes();
 
-    /**
-     * Deletes all selected vertexes
-     */
-    void deleteSelectedVertexes();
-
     /**
      * Inverts selection of vertex with number
      * \param vertexNr number of vertex which is to be inverted

tests/src/app/testqgsnodetool.cpp

@@ -61,6 +61,8 @@ class TestQgsNodeTool : public QObject
     void testAddVertexAtEndpoint();
     void testDeleteVertex();
     void testMoveMultipleVertices();
+    void testMoveVertexTopo();
+    void testDeleteVertexTopo();
 
   private:
     QPoint mapToScreen( double mapX, double mapY )
@@ -472,5 +474,63 @@ void TestQgsNodeTool::testMoveMultipleVertices()
   QCOMPARE( mLayerLine->getFeature( mFidLineF1 ).geometry(), QgsGeometry::fromWkt( "LINESTRING(2 1, 1 1, 1 3)" ) );
 }
 
+void TestQgsNodeTool::testMoveVertexTopo()
+{
+  // test moving of vertices of two features at once
+
+  QgsProject::instance()->setTopologicalEditing( true );
+
+  // connect linestring with polygon at point (2, 1)
+  mouseClick( 4, 1, Qt::LeftButton );
+  mouseClick( 2, 1, Qt::LeftButton );
+
+  // move shared node of linestring and polygon
+  mouseClick( 2, 1, Qt::LeftButton );
+  mouseClick( 3, 3, Qt::LeftButton );
+
+  QCOMPARE( mLayerLine->getFeature( mFidLineF1 ).geometry(), QgsGeometry::fromWkt( "LINESTRING(3 3, 1 1, 1 3)" ) );
+  QCOMPARE( mLayerPolygon->getFeature( mFidPolygonF1 ).geometry(), QgsGeometry::fromWkt( "POLYGON((3 3, 7 1, 7 4, 4 4, 3 3))" ) );
+
+  QCOMPARE( mLayerLine->undoStack()->index(), 2 );
+  QCOMPARE( mLayerPolygon->undoStack()->index(), 3 );  // one more move of node from earlier
+  mLayerLine->undoStack()->undo();
+  mLayerPolygon->undoStack()->undo();
+  mLayerPolygon->undoStack()->undo();
+
+  QCOMPARE( mLayerLine->getFeature( mFidLineF1 ).geometry(), QgsGeometry::fromWkt( "LINESTRING(2 1, 1 1, 1 3)" ) );
+  QCOMPARE( mLayerPolygon->getFeature( mFidPolygonF1 ).geometry(), QgsGeometry::fromWkt( "POLYGON((4 1, 7 1, 7 4, 4 4, 4 1))" ) );
+
+  QgsProject::instance()->setTopologicalEditing( false );
+}
+
+void TestQgsNodeTool::testDeleteVertexTopo()
+{
+  // test deletion of vertices with topological editing enabled
+
+  QgsProject::instance()->setTopologicalEditing( true );
+
+  // connect linestring with polygon at point (2, 1)
+  mouseClick( 4, 1, Qt::LeftButton );
+  mouseClick( 2, 1, Qt::LeftButton );
+
+  // move shared node of linestring and polygon
+  mouseClick( 2, 1, Qt::LeftButton );
+  keyClick( Qt::Key_Delete );
+
+  QCOMPARE( mLayerLine->getFeature( mFidLineF1 ).geometry(), QgsGeometry::fromWkt( "LINESTRING(1 1, 1 3)" ) );
+  QCOMPARE( mLayerPolygon->getFeature( mFidPolygonF1 ).geometry(), QgsGeometry::fromWkt( "POLYGON((7 1, 7 4, 4 4, 7 1))" ) );
+
+  QCOMPARE( mLayerLine->undoStack()->index(), 2 );
+  QCOMPARE( mLayerPolygon->undoStack()->index(), 3 );  // one more move of node from earlier
+  mLayerLine->undoStack()->undo();
+  mLayerPolygon->undoStack()->undo();
+  mLayerPolygon->undoStack()->undo();
+
+  QCOMPARE( mLayerLine->getFeature( mFidLineF1 ).geometry(), QgsGeometry::fromWkt( "LINESTRING(2 1, 1 1, 1 3)" ) );
+  QCOMPARE( mLayerPolygon->getFeature( mFidPolygonF1 ).geometry(), QgsGeometry::fromWkt( "POLYGON((4 1, 7 1, 7 4, 4 4, 4 1))" ) );
+
+  QgsProject::instance()->setTopologicalEditing( false );
+}
+
 QGSTEST_MAIN( TestQgsNodeTool )
 #include "testqgsnodetool.moc"