Skip to content

Commit

Permalink
qgsvectorlayer: Properly invalidate extent cache on feature deletion
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ptitjano authored and github-actions[bot] committed Feb 7, 2025
1 parent 544b506 commit bdaff94
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 8 deletions.
11 changes: 3 additions & 8 deletions src/core/vector/qgsvectorlayer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
Expand Down Expand Up @@ -5973,6 +5966,8 @@ void QgsVectorLayer::onFeatureDeleted( QgsFeatureId fid )
{
QGIS_PROTECT_QOBJECT_THREAD_ACCESS

updateExtents();

if ( mEditCommandActive || mCommitChangesActive )
{
mDeletedFids << fid;
Expand Down
16 changes: 16 additions & 0 deletions tests/src/core/testqgsvectorlayer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)" ) ) );
Expand All @@ -514,12 +515,27 @@ void TestQgsVectorLayer::testAddFeatureExtentUpdated()
QCOMPARE( layerLine->extent3D(), QgsBox3D( 1, 1, std::numeric_limits<double>::quiet_NaN(), 7, 12, std::numeric_limits<double>::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<long>( 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<long>( 0 ) );
QCOMPARE( spyDeleted.at( 0 ).at( 0 ), QVariant( lineF1.id() ) );

delete layerLine;
}

Expand Down
16 changes: 16 additions & 0 deletions tests/src/python/test_qgsvectorlayer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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):
Expand All @@ -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()
Expand All @@ -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)

Expand All @@ -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)
Expand All @@ -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())

Expand Down

0 comments on commit bdaff94

Please sign in to comment.