Merge pull request #50924 from qgis/backport-50899-to-release-3_28

[Backport release-3_28] Don't emit layerModified for every atomic change made during a bulk operation to the vector layer edit buffer
qgis · Nov 18, 2022 · 48b9b5c · 48b9b5c
2 parents 332a3fd + 4a7df60
commit 48b9b5c
Show file tree

Hide file tree

Showing 5 changed files with 232 additions and 8 deletions.
diff --git a/python/core/auto_generated/vector/qgsvectorlayereditbuffer.sip.in b/python/core/auto_generated/vector/qgsvectorlayereditbuffer.sip.in
@@ -331,6 +331,7 @@ Updates an index in an attribute map to a new value (for updates of changed attr
 
 
 
+
 };
 
 /************************************************************************

diff --git a/src/core/vector/qgsvectorlayer.cpp b/src/core/vector/qgsvectorlayer.cpp
@@ -3153,9 +3153,12 @@ bool QgsVectorLayer::changeAttributeValues( QgsFeatureId fid, const QgsAttribute
     result = mJoinBuffer->changeAttributeValues( fid, newValuesJoin, oldValuesJoin );
   }
 
-  if ( ! newValuesNotJoin.isEmpty() && mEditBuffer && mDataProvider )
+  if ( ! newValuesNotJoin.isEmpty() )
   {
-    result &= mEditBuffer->changeAttributeValues( fid, newValuesNotJoin, oldValues );
+    if ( mEditBuffer && mDataProvider )
+      result &= mEditBuffer->changeAttributeValues( fid, newValuesNotJoin, oldValues );
+    else
+      result = false;
   }
 
   if ( result && !skipDefaultValues && !mDefaultValueOnUpdateFields.isEmpty() )
@@ -3435,9 +3438,17 @@ bool QgsVectorLayer::deleteFeature( QgsFeatureId fid, QgsVectorLayer::DeleteCont
 bool QgsVectorLayer::deleteFeatures( const QgsFeatureIds &fids, QgsVectorLayer::DeleteContext *context )
 {
   bool res = true;
-  const auto constFids = fids;
-  for ( QgsFeatureId fid : constFids )
-    res = deleteFeatureCascade( fid, context ) && res;
+
+  if ( ( context && context->cascade ) || mJoinBuffer->containsJoins() )
+  {
+    // should ideally be "deleteFeaturesCascade" for performance!
+    for ( QgsFeatureId fid : fids )
+      res = deleteFeatureCascade( fid, context ) && res;
+  }
+  else
+  {
+    res = mEditBuffer && mEditBuffer->deleteFeatures( fids );
+  }
 
   if ( res )
   {

diff --git a/src/core/vector/qgsvectorlayereditbuffer.cpp b/src/core/vector/qgsvectorlayereditbuffer.cpp
@@ -53,6 +53,9 @@ bool QgsVectorLayerEditBuffer::isModified() const
 
 void QgsVectorLayerEditBuffer::undoIndexChanged( int index )
 {
+  if ( mBlockModifiedSignals )
+    return;
+
   QgsDebugMsgLevel( QStringLiteral( "undo index changed %1" ).arg( index ), 4 );
   Q_UNUSED( index )
   emit layerModified();
@@ -149,12 +152,23 @@ bool QgsVectorLayerEditBuffer::addFeatures( QgsFeatureList &features )
   if ( !( L->dataProvider()->capabilities() & QgsVectorDataProvider::AddFeatures ) )
     return false;
 
+  // we don't want to emit layerModified for every added feature, rather just once for the batch lot
+  mBlockModifiedSignals++;
+
   bool result = true;
+  bool anyAdded = false;
   for ( QgsFeatureList::iterator iter = features.begin(); iter != features.end(); ++iter )
   {
-    result = result && addFeature( *iter );
+    const bool thisFeatureResult = addFeature( *iter );
+    result = result && thisFeatureResult;
+    anyAdded |= thisFeatureResult;
   }
 
+  mBlockModifiedSignals--;
+
+  if ( anyAdded )
+    emit layerModified();
+
   L->updateExtents();
   return result;
 }
@@ -198,11 +212,16 @@ bool QgsVectorLayerEditBuffer::deleteFeatures( const QgsFeatureIds &fids )
     return false;
   }
 
+  // we don't want to emit layerModified for every deleted feature, rather just once for the batch lot
+  mBlockModifiedSignals++;
+
   bool ok = true;
-  const auto constFids = fids;
-  for ( QgsFeatureId fid : constFids )
+  for ( QgsFeatureId fid : fids )
     ok = deleteFeature( fid ) && ok;
 
+  mBlockModifiedSignals--;
+  emit layerModified();
+
   return ok;
 }
 
@@ -231,6 +250,10 @@ bool QgsVectorLayerEditBuffer::changeGeometry( QgsFeatureId fid, const QgsGeomet
 bool QgsVectorLayerEditBuffer::changeAttributeValues( QgsFeatureId fid, const QgsAttributeMap &newValues, const QgsAttributeMap &oldValues )
 {
   bool success = true;
+
+  // we don't want to emit layerModified for every changed attribute, rather just once for the batch lot
+  mBlockModifiedSignals++;
+
   for ( auto it = newValues.constBegin() ; it != newValues.constEnd(); ++it )
   {
     const int field = it.key();
@@ -243,6 +266,9 @@ bool QgsVectorLayerEditBuffer::changeAttributeValues( QgsFeatureId fid, const Qg
     success &= changeAttributeValue( fid, field, newValue, oldValue );
   }
 
+  mBlockModifiedSignals--;
+  emit layerModified();
+
   return success;
 }
 

diff --git a/src/core/vector/qgsvectorlayereditbuffer.h b/src/core/vector/qgsvectorlayereditbuffer.h
@@ -330,6 +330,8 @@ class CORE_EXPORT QgsVectorLayerEditBuffer : public QObject
 
     QgsVectorLayerEditBufferGroup *mEditBufferGroup = nullptr;
 
+    int mBlockModifiedSignals = 0;
+
     friend class QgsGrassProvider; //GRASS provider totally abuses the edit buffer
 
   private:

diff --git a/tests/src/python/test_qgsvectorlayer.py b/tests/src/python/test_qgsvectorlayer.py
@@ -614,6 +614,8 @@ def checkBefore():
 
         checkBefore()
 
+        spy = QSignalSpy(layer.layerModified)
+
         # try to add feature without editing mode
         self.assertFalse(layer.addFeature(feat))
 
@@ -624,18 +626,26 @@ def checkBefore():
         bad_feature = QgsFeature()
         self.assertFalse(layer.addFeature(bad_feature))
 
+        self.assertEqual(len(spy), 0)
+
         # add good feature
         self.assertTrue(layer.addFeature(feat))
 
+        self.assertEqual(len(spy), 1)
+
         checkAfter()
         self.assertEqual(layer.dataProvider().featureCount(), 0)
 
         # now try undo/redo
         layer.undoStack().undo()
         checkBefore()
+        self.assertEqual(len(spy), 2)
+
         layer.undoStack().redo()
         checkAfter()
 
+        self.assertEqual(len(spy), 3)
+
         self.assertTrue(layer.commitChanges())
 
         checkAfter()
@@ -681,22 +691,33 @@ def checkBefore():
         # add feature
         layer.startEditing()
 
+        spy = QSignalSpy(layer.layerModified)
+
         # try adding feature with incorrect number of fields
         bad_feature = QgsFeature()
         self.assertFalse(layer.addFeatures([bad_feature]))
 
+        self.assertEqual(len(spy), 0)
+
         # add good features
         self.assertTrue(layer.addFeatures([feat1, feat2]))
 
+        self.assertEqual(len(spy), 1)
+
         checkAfter()
         self.assertEqual(layer.dataProvider().featureCount(), 0)
 
         # now try undo/redo
         layer.undoStack().undo()
+
+        self.assertEqual(len(spy), 2)
         layer.undoStack().undo()
+        self.assertEqual(len(spy), 3)
         checkBefore()
         layer.undoStack().redo()
+        self.assertEqual(len(spy), 4)
         layer.undoStack().redo()
+        self.assertEqual(len(spy), 5)
         checkAfter()
 
         self.assertTrue(layer.commitChanges())
@@ -737,22 +758,30 @@ def checkBefore():
 
         checkBefore()
 
+        spy = QSignalSpy(layer.layerModified)
+
         # try to delete feature without editing mode
         self.assertFalse(layer.deleteFeature(fid))
 
+        self.assertEqual(len(spy), 0)
+
         # delete feature
         layer.startEditing()
         self.assertTrue(layer.deleteFeature(fid))
 
+        self.assertEqual(len(spy), 1)
+
         checkAfter()
 
         # make sure calling it twice does not work
         self.assertFalse(layer.deleteFeature(fid))
 
         # now try undo/redo
         layer.undoStack().undo()
+        self.assertEqual(len(spy), 2)
         checkBefore()
         layer.undoStack().redo()
+        self.assertEqual(len(spy), 3)
         checkAfter()
 
         self.assertEqual(layer.dataProvider().featureCount(), 1)
@@ -762,6 +791,107 @@ def checkBefore():
         checkAfter()
         self.assertEqual(layer.dataProvider().featureCount(), 0)
 
+    def test_DeleteFeatures(self):
+        layer = createLayerWithFivePoints()
+
+        def checkAfter():
+            self.assertEqual(layer.featureCount(), 3)
+
+            # check select+nextFeature
+            fi = layer.getFeatures()
+            f = next(fi)
+            fid2 = f.id()
+            self.assertEqual(f.geometry().asPoint(), QgsPointXY(200, 200))
+            f = next(fi)
+            fid4 = f.id()
+            self.assertEqual(f.geometry().asPoint(), QgsPointXY(400, 300))
+            f = next(fi)
+            fid5 = f.id()
+            self.assertEqual(f.geometry().asPoint(), QgsPointXY(0, 0))
+            with self.assertRaises(StopIteration):
+                next(fi)
+
+            # check feature at id
+            f2 = next(layer.getFeatures(QgsFeatureRequest(fid2)))
+            self.assertEqual(f2.geometry().asPoint(), QgsPointXY(200, 200))
+            f2 = next(layer.getFeatures(QgsFeatureRequest(fid4)))
+            self.assertEqual(f2.geometry().asPoint(), QgsPointXY(400, 300))
+            f2 = next(layer.getFeatures(QgsFeatureRequest(fid5)))
+            self.assertEqual(f2.geometry().asPoint(), QgsPointXY(0, 0))
+
+        def checkBefore():
+            self.assertEqual(layer.featureCount(), 5)
+
+            # check select+nextFeature
+            fi = layer.getFeatures()
+            f = next(fi)
+            fid1 = f.id()
+            self.assertEqual(f.geometry().asPoint(), QgsPointXY(100, 200))
+            f = next(fi)
+            fid2 = f.id()
+            self.assertEqual(f.geometry().asPoint(), QgsPointXY(200, 200))
+            f = next(fi)
+            fid3 = f.id()
+            self.assertEqual(f.geometry().asPoint(), QgsPointXY(300, 200))
+            f = next(fi)
+            fid4 = f.id()
+            self.assertEqual(f.geometry().asPoint(), QgsPointXY(400, 300))
+            f = next(fi)
+            fid5 = f.id()
+            self.assertEqual(f.geometry().asPoint(), QgsPointXY(0, 0))
+            with self.assertRaises(StopIteration):
+                next(fi)
+
+            # check feature at id
+            f2 = next(layer.getFeatures(QgsFeatureRequest(fid1)))
+            self.assertEqual(f2.geometry().asPoint(), QgsPointXY(100, 200))
+            f2 = next(layer.getFeatures(QgsFeatureRequest(fid2)))
+            self.assertEqual(f2.geometry().asPoint(), QgsPointXY(200, 200))
+            f2 = next(layer.getFeatures(QgsFeatureRequest(fid3)))
+            self.assertEqual(f2.geometry().asPoint(), QgsPointXY(300, 200))
+            f2 = next(layer.getFeatures(QgsFeatureRequest(fid4)))
+            self.assertEqual(f2.geometry().asPoint(), QgsPointXY(400, 300))
+            f2 = next(layer.getFeatures(QgsFeatureRequest(fid5)))
+            self.assertEqual(f2.geometry().asPoint(), QgsPointXY(0, 0))
+
+            return fid1, fid2, fid3, fid4, fid5
+
+        fid1, fid2, fid3, fid4, fid5 = checkBefore()
+
+        spy = QSignalSpy(layer.layerModified)
+
+        # try to delete features without editing mode
+        self.assertFalse(layer.deleteFeatures([fid1, fid2]))
+
+        self.assertEqual(len(spy), 0)
+
+        # delete features
+        layer.startEditing()
+        self.assertTrue(layer.deleteFeatures([fid1, fid3]))
+
+        self.assertEqual(len(spy), 1)
+
+        checkAfter()
+
+        # now try undo/redo
+        layer.undoStack().undo()
+        self.assertEqual(len(spy), 2)
+        layer.undoStack().undo()
+        self.assertEqual(len(spy), 3)
+        checkBefore()
+        layer.undoStack().redo()
+        self.assertEqual(len(spy), 4)
+        layer.undoStack().redo()
+        self.assertEqual(len(spy), 5)
+        checkAfter()
+
+        self.assertEqual(layer.dataProvider().featureCount(), 5)
+
+        self.assertTrue(layer.commitChanges())
+
+        checkAfter()
+        self.assertEqual(layer.dataProvider().featureCount(), 3)
+
     def test_DeleteFeatureAfterAddFeature(self):
 
         layer = createEmptyLayer()
@@ -894,6 +1024,60 @@ def checkBefore():
         self.assertTrue(layer.commitChanges())
         checkAfter()
 
+    def test_ChangeAttributeValues(self):
+        layer = createLayerWithOnePoint()
+        fid = 1
+
+        def checkAfter():
+            # check select+nextFeature
+            fi = layer.getFeatures()
+            f = next(fi)
+            self.assertEqual(f[0], "good")
+            self.assertEqual(f[1], 100)
+
+            # check feature at id
+            f2 = next(layer.getFeatures(QgsFeatureRequest(f.id())))
+            self.assertEqual(f2[0], "good")
+            self.assertEqual(f2[1], 100)
+
+        def checkBefore():
+            # check select+nextFeature
+            f = next(layer.getFeatures())
+            self.assertEqual(f[0], "test")
+            self.assertEqual(f[1], 123)
+
+        checkBefore()
+
+        spy = QSignalSpy(layer.layerModified)
+
+        # try to change attribute without editing mode
+        self.assertFalse(layer.changeAttributeValues(fid, {0: "good", 1: 100}))
+
+        self.assertEqual(len(spy), 0)
+
+        # change attribute
+        layer.startEditing()
+        self.assertTrue(layer.changeAttributeValues(fid, {0: "good", 1: 100}))
+
+        self.assertEqual(len(spy), 1)
+
+        checkAfter()
+
+        # now try undo/redo
+        layer.undoStack().undo()
+        self.assertEqual(len(spy), 2)
+        layer.undoStack().undo()
+        self.assertEqual(len(spy), 3)
+        checkBefore()
+        layer.undoStack().redo()
+        self.assertEqual(len(spy), 4)
+        layer.undoStack().redo()
+        self.assertEqual(len(spy), 5)
+        checkAfter()
+
+        self.assertTrue(layer.commitChanges())
+        checkAfter()
+
     def test_ChangeAttributeAfterAddFeature(self):
         layer = createLayerWithOnePoint()
         layer.dataProvider().deleteFeatures([1])  # no need for this feature