From 98bfa4a58993fc80f513d188dd7e61917f54334b Mon Sep 17 00:00:00 2001 From: Jean Felder Date: Wed, 5 Feb 2025 15:29:41 +0100 Subject: [PATCH] qgsvectorlayer: Properly invalidate extent cache on feature deletion This change is similar to the previous commit on feature addition. When a feature is removed from a `QgsVectorLayer` `QgsVectorLayer::deleteFeature` is called. This function makes mostly 2 things in case of a successful operation: 1. it calls `QgsVectorLayerEditBuffer::deleteFeature` which itself will emit the `QgsVectorLayerEditBuffer::featureDeleted` signal. Finally, `QgsVectorLayer` listend to this signal to directly emit the `QgsVectorLayer::featureDeleted` signal 2. Call `QgsVectorLayer::updateExtents()` to invalidate the cache of the extent In practice, this means that the `QgsVectorLayer::featureDeleted` signal may be emitted before the cache of the extent is invalidated. This can cause some issues. This issue is solved by calling `updateExtents()` before emitting the `featureDeleted` signal. --- src/core/vector/qgsvectorlayer.cpp | 11 +++-------- tests/src/core/testqgsvectorlayer.cpp | 16 ++++++++++++++++ tests/src/python/test_qgsvectorlayer.py | 16 ++++++++++++++++ 3 files changed, 35 insertions(+), 8 deletions(-) diff --git a/src/core/vector/qgsvectorlayer.cpp b/src/core/vector/qgsvectorlayer.cpp index 4723e640a2a6..38452e9d9ed4 100644 --- a/src/core/vector/qgsvectorlayer.cpp +++ b/src/core/vector/qgsvectorlayer.cpp @@ -3817,14 +3817,7 @@ bool QgsVectorLayer::deleteFeature( QgsFeatureId fid, QgsVectorLayer::DeleteCont if ( !mEditBuffer ) return false; - bool res = deleteFeatureCascade( fid, context ); - - if ( res ) - { - updateExtents(); - } - - return res; + return deleteFeatureCascade( fid, context ); } bool QgsVectorLayer::deleteFeatures( const QgsFeatureIds &fids, QgsVectorLayer::DeleteContext *context ) @@ -5976,6 +5969,8 @@ void QgsVectorLayer::onFeatureDeleted( QgsFeatureId fid ) { QGIS_PROTECT_QOBJECT_THREAD_ACCESS + updateExtents(); + if ( mEditCommandActive || mCommitChangesActive ) { mDeletedFids << fid; diff --git a/tests/src/core/testqgsvectorlayer.cpp b/tests/src/core/testqgsvectorlayer.cpp index e1f9a4d64515..cc3ce2bd2bab 100644 --- a/tests/src/core/testqgsvectorlayer.cpp +++ b/tests/src/core/testqgsvectorlayer.cpp @@ -504,6 +504,7 @@ void TestQgsVectorLayer::testAddFeatureExtentUpdated() QCOMPARE( layerLine->extent(), QgsRectangle() ); const QSignalSpy spyAdded( layerLine, &QgsVectorLayer::featureAdded ); + const QSignalSpy spyDeleted( layerLine, &QgsVectorLayer::featureDeleted ); QgsFeature lineF1; lineF1.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "LineString (2 1, 1 1, 1 5, 7 12)" ) ) ); @@ -514,12 +515,27 @@ void TestQgsVectorLayer::testAddFeatureExtentUpdated() QCOMPARE( layerLine->extent3D(), QgsBox3D( 1, 1, std::numeric_limits::quiet_NaN(), 7, 12, std::numeric_limits::quiet_NaN() ) ); } ); + connect( layerLine, &QgsVectorLayer::featureDeleted, this, [&layerLine, &lineF1]( const QgsFeatureId &fid ) { + QCOMPARE( fid, lineF1.id() ); + QCOMPARE( layerLine->extent(), QgsRectangle() ); + QCOMPARE( layerLine->extent3D(), QgsBox3D() ); + } + + ); + layerLine->startEditing(); layerLine->addFeature( lineF1 ); QCOMPARE( spyAdded.count(), 1 ); + QCOMPARE( spyDeleted.count(), 0 ); QCOMPARE( layerLine->featureCount(), static_cast( 1 ) ); QCOMPARE( spyAdded.at( 0 ).at( 0 ), QVariant( lineF1.id() ) ); + layerLine->deleteFeature( lineF1.id() ); + QCOMPARE( spyAdded.count(), 1 ); + QCOMPARE( spyDeleted.count(), 1 ); + QCOMPARE( layerLine->featureCount(), static_cast( 0 ) ); + QCOMPARE( spyDeleted.at( 0 ).at( 0 ), QVariant( lineF1.id() ) ); + delete layerLine; } diff --git a/tests/src/python/test_qgsvectorlayer.py b/tests/src/python/test_qgsvectorlayer.py index 9fddac4f96f2..1ec7dbff81a3 100644 --- a/tests/src/python/test_qgsvectorlayer.py +++ b/tests/src/python/test_qgsvectorlayer.py @@ -701,6 +701,7 @@ def test_AddFeature(self): def checkAfter(): self.assertEqual(layer.featureCount(), 1) + self.assertEqual(layer.extent(), QgsRectangle(1, 2, 1, 2)) # check select+nextFeature f = next(layer.getFeatures()) @@ -712,6 +713,7 @@ def checkAfter(): def checkBefore(): self.assertEqual(layer.featureCount(), 0) + self.assertEqual(layer.extent(), QgsRectangle()) # check select+nextFeature with self.assertRaises(StopIteration): @@ -721,10 +723,15 @@ def checkBefore(): spy = QSignalSpy(layer.layerModified) repaint_spy = QSignalSpy(layer.repaintRequested) + feature_added_spy = QSignalSpy(layer.featureAdded) + feature_deleted_spy = QSignalSpy(layer.featureDeleted) # try to add feature without editing mode self.assertFalse(layer.addFeature(feat)) self.assertEqual(len(repaint_spy), 0) + self.assertEqual(len(feature_added_spy), 0) + self.assertEqual(len(feature_deleted_spy), 0) + self.assertEqual(layer.extent(), QgsRectangle()) # add feature layer.startEditing() @@ -733,6 +740,9 @@ def checkBefore(): bad_feature = QgsFeature() self.assertFalse(layer.addFeature(bad_feature)) self.assertEqual(len(repaint_spy), 0) + self.assertEqual(len(feature_added_spy), 0) + self.assertEqual(len(feature_deleted_spy), 0) + self.assertEqual(layer.extent(), QgsRectangle()) self.assertEqual(len(spy), 0) @@ -741,12 +751,16 @@ def checkBefore(): self.assertEqual(len(spy), 1) self.assertEqual(len(repaint_spy), 1) + self.assertEqual(len(feature_added_spy), 1) + self.assertEqual(len(feature_deleted_spy), 0) checkAfter() self.assertEqual(layer.dataProvider().featureCount(), 0) # now try undo/redo layer.undoStack().undo() + self.assertEqual(len(feature_added_spy), 1) + self.assertEqual(len(feature_deleted_spy), 1) checkBefore() self.assertEqual(len(spy), 2) self.assertEqual(len(repaint_spy), 2) @@ -756,6 +770,8 @@ def checkBefore(): self.assertEqual(len(spy), 3) self.assertEqual(len(repaint_spy), 3) + self.assertEqual(len(feature_added_spy), 2) + self.assertEqual(len(feature_deleted_spy), 1) self.assertTrue(layer.commitChanges())