Refactor QgsFeaturePool

QgsFeaturePool is now an abstract baseclass, with a new inherited class QgsVectorDataProviderFeaturePool. This allows creating other subclasses, most notably a QgsVectorLayerFeaturePool subclass, that is able to also work on uncommitted features.

Critical calls to methods which are not threadsafe have been protected by executing them on the main thread.

Author: m-kuhn

src/analysis/CMakeLists.txt

@@ -132,6 +132,7 @@ SET(QGIS_ANALYSIS_SRCS
   network/qgsgraphanalyzer.cpp
 
   vector/geometry_checker/qgsfeaturepool.cpp
+  vector/geometry_checker/qgsvectordataproviderfeaturepool.cpp
   vector/geometry_checker/qgsgeometrychecker.cpp
   vector/geometry_checker/qgsgeometryanglecheck.cpp
   vector/geometry_checker/qgsgeometryareacheck.cpp
@@ -232,6 +233,7 @@ SET(QGIS_ANALYSIS_HDRS
   vector/qgszonalstatistics.h
   vector/geometry_checker/qgsgeometrycheckerutils.h
   vector/geometry_checker/qgsfeaturepool.h
+  vector/geometry_checker/qgsvectordataproviderfeaturepool.h
 
   interpolation/qgsinterpolator.h
   interpolation/qgsgridfilewriter.h

src/analysis/vector/geometry_checker/qgsfeaturepool.cpp

@@ -20,44 +20,24 @@
 #include "qgsgeometry.h"
 #include "qgsvectorlayer.h"
 #include "qgsvectordataprovider.h"
+#include "qgsvectorlayerutils.h"
 
 #include <QMutexLocker>
 
-QgsFeaturePool::QgsFeaturePool( QgsVectorLayer *layer, double layerToMapUnits, const QgsCoordinateTransform &layerToMapTransform, bool selectedOnly )
+QgsFeaturePool::QgsFeaturePool( QgsVectorLayer *layer, double layerToMapUnits, const QgsCoordinateTransform &layerToMapTransform )
   : mFeatureCache( CACHE_SIZE )
   , mLayer( layer )
   , mLayerToMapUnits( layerToMapUnits )
   , mLayerToMapTransform( layerToMapTransform )
-  , mSelectedOnly( selectedOnly )
+  , mLayerId( layer->id() )
+  , mGeometryType( layer->geometryType() )
 {
-  // Build spatial index
-  QgsFeature feature;
-  QgsFeatureRequest req;
-  req.setSubsetOfAttributes( QgsAttributeList() );
-  if ( selectedOnly )
-  {
-    mFeatureIds = layer->selectedFeatureIds();
-    req.setFilterFids( mFeatureIds );
-  }
 
-  QgsFeatureIterator it = layer->getFeatures( req );
-  while ( it.nextFeature( feature ) )
-  {
-    if ( feature.geometry() )
-    {
-      mIndex.insertFeature( feature );
-      mFeatureIds.insert( feature.id() );
-    }
-    else
-    {
-      mFeatureIds.remove( feature.id() );
-    }
-  }
 }
 
 bool QgsFeaturePool::get( QgsFeatureId id, QgsFeature &feature )
 {
-  QMutexLocker lock( &mLayerMutex );
+  mCacheLock.lockForRead();
   QgsFeature *cachedFeature = mFeatureCache.object( id );
   if ( cachedFeature )
   {
@@ -66,78 +46,83 @@ bool QgsFeaturePool::get( QgsFeatureId id, QgsFeature &feature )
   }
   else
   {
+    std::unique_ptr<QgsVectorLayerFeatureSource> source = QgsVectorLayerUtils::getFeatureSource( mLayer );
+
     // Feature not in cache, retrieve from layer
     // TODO: avoid always querying all attributes (attribute values are needed when merging by attribute)
-    if ( !mLayer->getFeatures( QgsFeatureRequest( id ) ).nextFeature( feature ) )
+    if ( !source->getFeatures( QgsFeatureRequest( id ) ).nextFeature( feature ) )
     {
       return false;
     }
+    mCacheLock.unlock();
+    mCacheLock.lockForWrite();
     mFeatureCache.insert( id, new QgsFeature( feature ) );
   }
+  mCacheLock.unlock();
   return true;
 }
 
-void QgsFeaturePool::addFeature( QgsFeature &feature )
+QgsFeatureIds QgsFeaturePool::getFeatureIds() const
 {
-  QgsFeatureList features;
-  features.append( feature );
-  mLayerMutex.lock();
-  mLayer->dataProvider()->addFeatures( features );
-  feature.setId( features.front().id() );
-  if ( mSelectedOnly )
-  {
-    QgsFeatureIds selectedFeatureIds = mLayer->selectedFeatureIds();
-    selectedFeatureIds.insert( feature.id() );
-    mLayer->selectByIds( selectedFeatureIds );
-  }
-  mLayerMutex.unlock();
-  mIndexMutex.lock();
-  mIndex.insertFeature( feature );
-  mIndexMutex.unlock();
+  return mFeatureIds;
 }
 
-void QgsFeaturePool::updateFeature( QgsFeature &feature )
+QgsFeatureIds QgsFeaturePool::getIntersects( const QgsRectangle &rect ) const
 {
-  QgsFeature origFeature;
-  get( feature.id(), origFeature );
+  mIndexLock.lockForRead();
+  QgsFeatureIds ids = QgsFeatureIds::fromList( mIndex.intersects( rect ) );
+  mIndexLock.unlock();
+  return ids;
+}
 
-  QgsGeometryMap geometryMap;
-  geometryMap.insert( feature.id(), feature.geometry() );
-  QgsChangedAttributesMap changedAttributesMap;
-  QgsAttributeMap attribMap;
-  for ( int i = 0, n = feature.attributes().size(); i < n; ++i )
-  {
-    attribMap.insert( i, feature.attributes().at( i ) );
-  }
-  changedAttributesMap.insert( feature.id(), attribMap );
-  mLayerMutex.lock();
-  mFeatureCache.remove( feature.id() ); // Remove to force reload on next get()
-  mLayer->dataProvider()->changeGeometryValues( geometryMap );
-  mLayer->dataProvider()->changeAttributeValues( changedAttributesMap );
-  mLayerMutex.unlock();
-  mIndexMutex.lock();
-  mIndex.deleteFeature( origFeature );
+QgsVectorLayer *QgsFeaturePool::layer() const
+{
+  Q_ASSERT( QThread::currentThread() == qApp->thread() );
+
+  return mLayer.data();
+}
+
+void QgsFeaturePool::insertFeature( const QgsFeature &feature )
+{
+  mCacheLock.lockForWrite();
+  mFeatureCache.insert( feature.id(), new QgsFeature( feature ) );
+  mIndex.insertFeature( feature );
+  mCacheLock.unlock();
+}
+
+void QgsFeaturePool::changeFeature( const QgsFeature &feature )
+{
+  mCacheLock.lockForWrite();
+  mFeatureCache.remove( feature.id() );
+  mFeatureCache.insert( feature.id(), new QgsFeature( feature ) );
+  mIndex.deleteFeature( feature );
   mIndex.insertFeature( feature );
-  mIndexMutex.unlock();
+  mCacheLock.unlock();
 }
 
-void QgsFeaturePool::deleteFeature( QgsFeatureId fid )
+void QgsFeaturePool::removeFeature( const QgsFeatureId featureId )
 {
   QgsFeature origFeature;
-  if ( get( fid, origFeature ) )
+  mCacheLock.lockForWrite();
+  if ( get( featureId, origFeature ) )
   {
-    mIndexMutex.lock();
     mIndex.deleteFeature( origFeature );
-    mIndexMutex.unlock();
   }
-  mLayerMutex.lock();
   mFeatureCache.remove( origFeature.id() );
-  mLayer->dataProvider()->deleteFeatures( QgsFeatureIds() << fid );
-  mLayerMutex.unlock();
+  mCacheLock.unlock();
 }
 
-QgsFeatureIds QgsFeaturePool::getIntersects( const QgsRectangle &rect ) const
+void QgsFeaturePool::setFeatureIds( const QgsFeatureIds &ids )
+{
+  mFeatureIds = ids;
+}
+
+QgsWkbTypes::GeometryType QgsFeaturePool::geometryType() const
+{
+  return mGeometryType;
+}
+
+QString QgsFeaturePool::layerId() const
 {
-  QMutexLocker lock( &mIndexMutex );
-  return QgsFeatureIds::fromList( mIndex.intersects( rect ) );
+  return mLayerId;
 }

src/analysis/vector/geometry_checker/qgsfeaturepool.h

@@ -24,39 +24,126 @@
 #include "qgis_analysis.h"
 #include "qgsfeature.h"
 #include "qgsspatialindex.h"
+#include "qgsvectorlayer.h"
 
 class QgsVectorLayer;
 
+/**
+ * \ingroup analysis
+ * A feature pool is based on a vector layer and caches features.
+ */
 class ANALYSIS_EXPORT QgsFeaturePool
 {
 
   public:
-    QgsFeaturePool( QgsVectorLayer *layer, double layerToMapUnits, const QgsCoordinateTransform &layerToMapTransform, bool selectedOnly = false );
+    QgsFeaturePool( QgsVectorLayer *layer, double layerToMapUnits, const QgsCoordinateTransform &layerToMapTransform );
+    virtual ~QgsFeaturePool() = default;
+
+    /**
+     * Retrieve the feature with the specified \a id into \a feature.
+     * It will be retrieved from the cache or from the underlying layer if unavailable.
+     * If the feature is neither available from the cache nor from the layer it will return false.
+     */
     bool get( QgsFeatureId id, QgsFeature &feature );
-    void addFeature( QgsFeature &feature );
-    void updateFeature( QgsFeature &feature );
-    void deleteFeature( QgsFeatureId fid );
+
+    /**
+     * Adds a feature to this pool.
+     * Implementations will add the feature to the layer or to the data provider.
+     */
+    virtual void addFeature( QgsFeature &feature ) = 0;
+
+    /**
+     * Updates a feature in this pool.
+     * Implementations will update the feature on the layer or on the data provider.
+     */
+    virtual void updateFeature( QgsFeature &feature ) = 0;
+
+    /**
+     * Removes a feature from this pool.
+     * Implementations will remove the feature from the layer or from the data provider.
+     */
+    virtual void deleteFeature( QgsFeatureId fid ) = 0;
+
+    /**
+     * Returns the complete set of feature ids in this pool.
+     * Note that this concerns the features governed by this pool, which are not necessarily all cached.
+     */
+    QgsFeatureIds getFeatureIds() const;
+
+    /**
+     * Get all feature ids in the bounding box \a rect. It will use a spatial index to
+     * determine the ids.
+     */
     QgsFeatureIds getIntersects( const QgsRectangle &rect ) const;
-    QgsVectorLayer *getLayer() const { return mLayer; }
-    const QgsFeatureIds &getFeatureIds() const { return mFeatureIds; }
+
+    /**
+     * The factor of layer units to map units.
+     * TODO: should this be removed and determined on runtime by checks that need it?
+     */
     double getLayerToMapUnits() const { return mLayerToMapUnits; }
+
+    /**
+     * A coordinate transform from layer to map CRS.
+     * TODO: should this be removed and determined on runtime by checks that need it?
+     */
     const QgsCoordinateTransform &getLayerToMapTransform() const { return mLayerToMapTransform; }
 
-    void clearLayer() { mLayer = nullptr; }
+    /**
+     * Get a pointer to the underlying layer.
+     * May return a ``nullptr`` if the layer has been deleted.
+     * This must only be called from the main thread.
+     */
+    QgsVectorLayer *layer() const;
 
-  private:
+    /**
+     * The layer id of the layer.
+     */
+    QString layerId() const;
 
-    static const int CACHE_SIZE = 1000;
+    /**
+     * The geometry type of this layer.
+     */
+    QgsWkbTypes::GeometryType geometryType() const;
+
+  protected:
+
+    /**
+     * Inserts a feature into the cache and the spatial index.
+     * To be used by implementations of ``addFeature``.
+     */
+    void insertFeature( const QgsFeature &feature );
+
+    /**
+     * Changes a feature in the cache and the spatial index.
+     * To be used by implementations of ``updateFeature``.
+     */
+    void changeFeature( const QgsFeature &feature );
 
+    /**
+     * Removes a feature from the cache and the spatial index.
+     * To be used by implementations of ``deleteFeature``.
+     */
+    void removeFeature( const QgsFeatureId featureId );
+
+    /**
+     * Set all the feature ids governed by this feature pool.
+     * Should be called by subclasses constructor and whenever
+     * they insert a new feature.
+     */
+    void setFeatureIds( const QgsFeatureIds &ids );
+
+  private:
+    static const int CACHE_SIZE = 1000;
     QCache<QgsFeatureId, QgsFeature> mFeatureCache;
-    QgsVectorLayer *mLayer = nullptr;
+    QPointer<QgsVectorLayer> mLayer;
+    QReadWriteLock mCacheLock;
     QgsFeatureIds mFeatureIds;
-    QMutex mLayerMutex;
-    mutable QMutex mIndexMutex;
+    mutable QReadWriteLock mIndexLock;
     QgsSpatialIndex mIndex;
     double mLayerToMapUnits = 1.0;
     QgsCoordinateTransform mLayerToMapTransform;
-    bool mSelectedOnly = false;
+    QString mLayerId;
+    QgsWkbTypes::GeometryType mGeometryType;
 };
 
 #endif // QGS_FEATUREPOOL_H

src/analysis/vector/geometry_checker/qgsgeometrycheck.cpp

@@ -149,7 +149,7 @@ QMap<QString, QgsFeatureIds> QgsGeometryCheck::allLayerFeatureIds() const
   QMap<QString, QgsFeatureIds> featureIds;
   for ( QgsFeaturePool *pool : mContext->featurePools )
   {
-    featureIds.insert( pool->getLayer()->id(), pool->getFeatureIds() );
+    featureIds.insert( pool->layerId(), pool->getFeatureIds() );
   }
   return featureIds;
 }