Clear existing cache when QgsVectorLayerCache is set to cache geom

nyalldawson · nyalldawson · commit 11c444466c40 · 2017-03-05T11:53:24.000+10:00
Any features inside the cache must be cleared, because they
won't necessarily have the feature's geometry cached

On behalf of Faunalia, sponsored by ENEL
diff --git a/src/core/qgsvectorlayercache.cpp b/src/core/qgsvectorlayercache.cpp
@@ -58,7 +58,9 @@ int QgsVectorLayerCache::cacheSize()
 
 void QgsVectorLayerCache::setCacheGeometry( bool cacheGeometry )
 {
-  mCacheGeometry = cacheGeometry && mLayer->hasGeometryType();
+  bool shouldCache = cacheGeometry && mLayer->hasGeometryType();
+  bool mustInvalidate = shouldCache && !mCacheGeometry; // going from no geometry -> geometry, so have to clear existing cache entries
+  mCacheGeometry = shouldCache;
   if ( cacheGeometry )
   {
     connect( mLayer, &QgsVectorLayer::geometryChanged, this, &QgsVectorLayerCache::geometryChanged );
@@ -67,6 +69,10 @@ void QgsVectorLayerCache::setCacheGeometry( bool cacheGeometry )
   {
     disconnect( mLayer, &QgsVectorLayer::geometryChanged, this, &QgsVectorLayerCache::geometryChanged );
   }
+  if ( mustInvalidate )
+  {
+    invalidate();
+  }
 }
 
 void QgsVectorLayerCache::setCacheSubsetOfAttributes( const QgsAttributeList &attributes )
diff --git a/tests/src/core/testqgsvectorlayercache.cpp b/tests/src/core/testqgsvectorlayercache.cpp
@@ -55,6 +55,7 @@ class TestVectorLayerCache : public QObject
     void testFullCache();
     void testFullCacheThroughRequest();
     void testCanUseCacheForRequest();
+    void testCacheGeom();
 
     void onCommittedFeaturesAdded( const QString &, const QgsFeatureList & );
 
@@ -331,6 +332,51 @@ void TestVectorLayerCache::testCanUseCacheForRequest()
   QVERIFY( cache.canUseCacheForRequest( QgsFeatureRequest().setFilterExpression( "$x<5" ), it ) );
 }
 
+void TestVectorLayerCache::testCacheGeom()
+{
+  QgsVectorLayerCache cache( mPointsLayer, 2 );
+  // cache geometry
+  cache.setCacheGeometry( true );
+
+  //first get some feature ids from layer
+  QgsFeature f;
+  QgsFeatureIterator it = mPointsLayer->getFeatures();
+  it.nextFeature( f );
+  QgsFeatureId id1 = f.id();
+  it.nextFeature( f );
+  QgsFeatureId id2 = f.id();
+
+  QgsFeatureRequest req;
+  req.setFlags( QgsFeatureRequest::NoGeometry ); // should be ignored by cache
+  req.setFilterFids( QgsFeatureIds() << id1 << id2 );
+
+  it = cache.getFeatures( req );
+  while ( it.nextFeature( f ) )
+  {
+    QVERIFY( f.hasGeometry() );
+  }
+
+  // disabled geometry caching
+  cache.setCacheGeometry( false );
+  // we should still have cached features... no need to lose these!
+  QCOMPARE( cache.cachedFeatureIds(), QgsFeatureIds() << id1 << id2 );
+  it = cache.getFeatures( req );
+  while ( it.nextFeature( f ) )
+  {
+    QVERIFY( f.hasGeometry() );
+  }
+
+  // now upgrade cache from no geometry -> geometry, should be cleared since we
+  // cannot be confident that features existing in the cache have geometry
+  cache.setCacheGeometry( true );
+  QVERIFY( cache.cachedFeatureIds().isEmpty() );
+  it = cache.getFeatures( req );
+  while ( it.nextFeature( f ) )
+  {
+    QVERIFY( f.hasGeometry() );
+  }
+}
+
 void TestVectorLayerCache::onCommittedFeaturesAdded( const QString &layerId, const QgsFeatureList &features )
 {
   Q_UNUSED( layerId )

Original file line number	Diff line number	Diff line change
`@@ -58,7 +58,9 @@ int QgsVectorLayerCache::cacheSize()`
`58`	`58`
`59`	`59`	`void QgsVectorLayerCache::setCacheGeometry( bool cacheGeometry )`
`60`	`60`	`{`
`61`		`- mCacheGeometry = cacheGeometry && mLayer->hasGeometryType();`
	`61`	`+ bool shouldCache = cacheGeometry && mLayer->hasGeometryType();`
	`62`	`+ bool mustInvalidate = shouldCache && !mCacheGeometry; // going from no geometry -> geometry, so have to clear existing cache entries`
	`63`	`+ mCacheGeometry = shouldCache;`
`62`	`64`	`if ( cacheGeometry )`
`63`	`65`	`{`
`64`	`66`	`connect( mLayer, &QgsVectorLayer::geometryChanged, this, &QgsVectorLayerCache::geometryChanged );`
`@@ -67,6 +69,10 @@ void QgsVectorLayerCache::setCacheGeometry( bool cacheGeometry )`
`67`	`69`	`{`
`68`	`70`	`disconnect( mLayer, &QgsVectorLayer::geometryChanged, this, &QgsVectorLayerCache::geometryChanged );`
`69`	`71`	`}`
	`72`	`+ if ( mustInvalidate )`
	`73`	`+ {`
	`74`	`+ invalidate();`
	`75`	`+ }`
`70`	`76`	`}`
`71`	`77`
`72`	`78`	`void QgsVectorLayerCache::setCacheSubsetOfAttributes( const QgsAttributeList &attributes )`