Merge pull request #3764 from nyalldawson/cache_fix2

Cache fixes

python/core/qgscacheindexfeatureid.sip

@@ -6,42 +6,8 @@ class QgsCacheIndexFeatureId : QgsAbstractCacheIndex
   public:
     QgsCacheIndexFeatureId( QgsVectorLayerCache* );
 
-    /**
-     * Is called, whenever a feature is removed from the cache. You should update your indexes, so
-     * they become invalid in case this feature was required to successfuly answer a request.
-     */
     virtual void flushFeature( const QgsFeatureId fid );
-
-    /**
-     * Sometimes, the whole cache changes its state and its easier to just withdraw everything.
-     * In this case, this method is issued. Be sure to clear all cache information in here.
-     */
     virtual void flush();
-
-    /**
-     * @brief
-     * Implement this method to update the the indices, in case you need information contained by the request
-     * to properly index. (E.g. spatial index)
-     * Does nothing by default
-     *
-     * @param featureRequest  The feature request that was answered
-     * @param fids            The feature ids that have been returned
-     */
     virtual void requestCompleted( const QgsFeatureRequest& featureRequest, const QgsFeatureIds& fids );
-
-    /**
-     * Is called, when a feature request is issued on a cached layer.
-     * If this cache index is able to completely answer the feature request, it will return true
-     * and write the list of feature ids of cached features to cachedFeatures. If it is not able
-     * it will return false and the cachedFeatures state is undefined.
-     *
-     * @param featureIterator  A reference to a {@link QgsFeatureIterator}. A valid featureIterator will
-     *                         be assigned in case this index is able to answer the request and the return
-     *                         value is true.
-     * @param featureRequest   The feature request, for which this index is queried.
-     *
-     * @return   True, if this index holds the information to answer the request.
-     *
-     */
     virtual bool getCacheIterator( QgsFeatureIterator& featureIterator, const QgsFeatureRequest& featureRequest );
 };

python/core/qgsvectorlayercache.sip

@@ -65,9 +65,18 @@ class QgsVectorLayerCache : QObject
      * be used for slow data sources, be aware, that the call to this method might take a long time.
      *
      * @param fullCache   True: enable full caching, False: disable full caching
+     * @see hasFullCache()
      */
     void setFullCache( bool fullCache );
 
+    /** Returns true if the cache is complete, ie it contains all features. This may happen as
+     * a result of a call to setFullCache() or by through a feature request which resulted in
+     * all available features being cached.
+     * @see setFullCache()
+     * @note added in QGIS 3.0
+     */
+    bool hasFullCache() const;
+
     /**
      * @brief
      * Adds a {@link QgsAbstractCacheIndex} to this cache. Cache indices know about features present
@@ -114,8 +123,15 @@ class QgsVectorLayerCache : QObject
      * Check if a certain feature id is cached.
      * @param  fid The feature id to look for
      * @return True if this id is in the cache
+     * @see cachedFeatureIds()
      */
-    bool isFidCached( const QgsFeatureId fid );
+    bool isFidCached( const QgsFeatureId fid ) const;
+
+    /** Returns the set of feature IDs for features which are cached.
+     * @note added in QGIS 3.0
+     * @see isFidCached()
+     */
+    QgsFeatureIds cachedFeatureIds() const;
 
     /**
      * Gets the feature at the given feature id. Considers the changed, added, deleted and permanent features

src/core/qgscacheindex.h

@@ -58,8 +58,8 @@ class CORE_EXPORT QgsAbstractCacheIndex
     /**
      * Is called, when a feature request is issued on a cached layer.
      * If this cache index is able to completely answer the feature request, it will return true
-     * and write the list of feature ids of cached features to cachedFeatures. If it is not able
+     * and set the iterator to a valid iterator over the cached features. If it is not able
-     * it will return false and the cachedFeatures state is undefined.
+     * it will return false.
      *
      * @param featureIterator  A reference to a {@link QgsFeatureIterator}. A valid featureIterator will
      *                         be assigned in case this index is able to answer the request and the return

src/core/qgscacheindexfeatureid.cpp

@@ -42,12 +42,36 @@ void QgsCacheIndexFeatureId::requestCompleted( const QgsFeatureRequest& featureR
 
 bool QgsCacheIndexFeatureId::getCacheIterator( QgsFeatureIterator &featureIterator, const QgsFeatureRequest &featureRequest )
 {
-  if ( featureRequest.filterType() == QgsFeatureRequest::FilterFid )
+  switch ( featureRequest.filterType() )
   {
-    if ( C->isFidCached( featureRequest.filterFid() ) )
+    case QgsFeatureRequest::FilterFid:
     {
-      featureIterator = QgsFeatureIterator( new QgsCachedFeatureIterator( C, featureRequest ) );
+      if ( C->isFidCached( featureRequest.filterFid() ) )
-      return true;
+      {
+        featureIterator = QgsFeatureIterator( new QgsCachedFeatureIterator( C, featureRequest ) );
+        return true;
+      }
+      break;
+    }
+    case QgsFeatureRequest::FilterFids:
+    {
+      if ( C->cachedFeatureIds().contains( featureRequest.filterFids() ) )
+      {
+        featureIterator = QgsFeatureIterator( new QgsCachedFeatureIterator( C, featureRequest ) );
+        return true;
+      }
+      break;
+    }
+    case QgsFeatureRequest::FilterNone:
+    case QgsFeatureRequest::FilterRect:
+    case QgsFeatureRequest::FilterExpression:
+    {
+      if ( C->hasFullCache() )
+      {
+        featureIterator = QgsFeatureIterator( new QgsCachedFeatureIterator( C, featureRequest ) );
+        return true;
+      }
+      break;
     }
   }
 

src/core/qgscacheindexfeatureid.h

@@ -28,43 +28,9 @@ class CORE_EXPORT QgsCacheIndexFeatureId : public QgsAbstractCacheIndex
   public:
     QgsCacheIndexFeatureId( QgsVectorLayerCache* );
 
-    /**
-     * Is called, whenever a feature is removed from the cache. You should update your indexes, so
-     * they become invalid in case this feature was required to successfuly answer a request.
-     */
     virtual void flushFeature( const QgsFeatureId fid ) override;
-
-    /**
-     * Sometimes, the whole cache changes its state and its easier to just withdraw everything.
-     * In this case, this method is issued. Be sure to clear all cache information in here.
-     */
     virtual void flush() override;
-
-    /**
-     * @brief
-     * Implement this method to update the the indices, in case you need information contained by the request
-     * to properly index. (E.g. spatial index)
-     * Does nothing by default
-     *
-     * @param featureRequest  The feature request that was answered
-     * @param fids            The feature ids that have been returned
-     */
     virtual void requestCompleted( const QgsFeatureRequest& featureRequest, const QgsFeatureIds& fids ) override;
-
-    /**
-     * Is called, when a feature request is issued on a cached layer.
-     * If this cache index is able to completely answer the feature request, it will return true
-     * and write the list of feature ids of cached features to cachedFeatures. If it is not able
-     * it will return false and the cachedFeatures state is undefined.
-     *
-     * @param featureIterator  A reference to a {@link QgsFeatureIterator}. A valid featureIterator will
-     *                         be assigned in case this index is able to answer the request and the return
-     *                         value is true.
-     * @param featureRequest   The feature request, for which this index is queried.
-     *
-     * @return   True, if this index holds the information to answer the request.
-     *
-     */
     virtual bool getCacheIterator( QgsFeatureIterator& featureIterator, const QgsFeatureRequest& featureRequest ) override;
 
   private:

src/core/qgsvectorlayercache.cpp

@@ -174,12 +174,16 @@ QgsVectorLayer* QgsVectorLayerCache::layer()
 void QgsVectorLayerCache::requestCompleted( const QgsFeatureRequest& featureRequest, const QgsFeatureIds& fids )
 {
   // If a request is too large for the cache don't notify to prevent from indexing incomplete requests
-  if ( fids.count() < mCache.size() )
+  if ( fids.count() <= mCache.size() )
   {
     Q_FOREACH ( QgsAbstractCacheIndex* idx, mCacheIndices )
     {
       idx->requestCompleted( featureRequest, fids );
     }
+    if ( featureRequest.filterType() == QgsFeatureRequest::FilterNone )
+    {
+      mFullCache = true;
+    }
   }
 }
 
@@ -266,6 +270,54 @@ void QgsVectorLayerCache::invalidate()
   emit invalidated();
 }
 
+bool QgsVectorLayerCache::canUseCacheForRequest( const QgsFeatureRequest &featureRequest, QgsFeatureIterator& it )
+{
+  // check first for available indices
+  Q_FOREACH ( QgsAbstractCacheIndex *idx, mCacheIndices )
+  {
+    if ( idx->getCacheIterator( it, featureRequest ) )
+    {
+      return true;
+    }
+  }
+
+  // no indexes available, but maybe we have already cached all required features anyway?
+  switch ( featureRequest.filterType() )
+  {
+    case QgsFeatureRequest::FilterFid:
+    {
+      if ( mCache.contains( featureRequest.filterFid() ) )
+      {
+        it = QgsFeatureIterator( new QgsCachedFeatureIterator( this, featureRequest ) );
+        return true;
+      }
+      break;
+    }
+    case QgsFeatureRequest::FilterFids:
+    {
+      if ( mCache.keys().toSet().contains( featureRequest.filterFids() ) )
+      {
+        it = QgsFeatureIterator( new QgsCachedFeatureIterator( this, featureRequest ) );
+        return true;
+      }
+      break;
+    }
+    case QgsFeatureRequest::FilterNone:
+    case QgsFeatureRequest::FilterRect:
+    case QgsFeatureRequest::FilterExpression:
+    {
+      if ( mFullCache )
+      {
+        it = QgsFeatureIterator( new QgsCachedFeatureIterator( this, featureRequest ) );
+        return true;
+      }
+      break;
+    }
+
+  }
+  return false;
+}
+
 QgsFeatureIterator QgsVectorLayerCache::getFeatures( const QgsFeatureRequest &featureRequest )
 {
   QgsFeatureIterator it;
@@ -281,15 +333,8 @@ QgsFeatureIterator QgsVectorLayerCache::getFeatures( const QgsFeatureRequest &fe
     }
     else
     {
-      // Check if an index is able to deliver the requested features
+      // may still be able to satisfy request using cache
-      Q_FOREACH ( QgsAbstractCacheIndex *idx, mCacheIndices )
+      requiresWriterIt = !canUseCacheForRequest( featureRequest, it );
-      {
-        if ( idx->getCacheIterator( it, featureRequest ) )
-        {
-          requiresWriterIt = false;
-          break;
-        }
-      }
     }
   }
   else
@@ -319,7 +364,7 @@ QgsFeatureIterator QgsVectorLayerCache::getFeatures( const QgsFeatureRequest &fe
   return it;
 }
 
-bool QgsVectorLayerCache::isFidCached( const QgsFeatureId fid )
+bool QgsVectorLayerCache::isFidCached( const QgsFeatureId fid ) const
 {
   return mCache.contains( fid );
 }

src/core/qgsvectorlayercache.h

@@ -133,9 +133,18 @@ class CORE_EXPORT QgsVectorLayerCache : public QObject
      * be used for slow data sources, be aware, that the call to this method might take a long time.
      *
      * @param fullCache   True: enable full caching, False: disable full caching
+     * @see hasFullCache()
      */
     void setFullCache( bool fullCache );
 
+    /** Returns true if the cache is complete, ie it contains all features. This may happen as
+     * a result of a call to setFullCache() or by through a feature request which resulted in
+     * all available features being cached.
+     * @see setFullCache()
+     * @note added in QGIS 3.0
+     */
+    bool hasFullCache() const { return mFullCache; }
+
     /**
      * @brief
      * Adds a {@link QgsAbstractCacheIndex} to this cache. Cache indices know about features present
@@ -196,8 +205,15 @@ class CORE_EXPORT QgsVectorLayerCache : public QObject
      * Check if a certain feature id is cached.
      * @param  fid The feature id to look for
      * @return True if this id is in the cache
+     * @see cachedFeatureIds()
      */
-    bool isFidCached( const QgsFeatureId fid );
+    bool isFidCached( const QgsFeatureId fid ) const;
+
+    /** Returns the set of feature IDs for features which are cached.
+     * @note added in QGIS 3.0
+     * @see isFidCached()
+     */
+    QgsFeatureIds cachedFeatureIds() const { return mCache.keys().toSet(); }
 
     /**
      * Gets the feature at the given feature id. Considers the changed, added, deleted and permanent features
@@ -329,5 +345,16 @@ class CORE_EXPORT QgsVectorLayerCache : public QObject
     friend class QgsCachedFeatureIterator;
     friend class QgsCachedFeatureWriterIterator;
     friend class QgsCachedFeature;
+
+    /** Returns true if the cache contains all the features required for a specified request.
+     * @param featureRequest feature request
+     * @param it will be set to iterator for matching features
+     * @returns true if cache can satisfy request
+     * @note this method only checks for available features, not whether the cache
+     * contains required attributes or geometry. For that, use checkInformationCovered()
+     */
+    bool canUseCacheForRequest( const QgsFeatureRequest& featureRequest, QgsFeatureIterator& it );
+
+    friend class TestVectorLayerCache;
 };
 #endif // QgsVectorLayerCache_H

src/gui/attributetable/qgsattributetablefiltermodel.cpp

@@ -294,11 +294,6 @@ void QgsAttributeTableFilterModel::setFilterMode( FilterMode filterMode )
       disconnect( mCanvas, SIGNAL( extentsChanged() ), this, SLOT( extentsChanged() ) );
     }
 
-    if ( filterMode == ShowSelected )
-    {
-      generateListOfVisibleFeatures();
-    }
-
     mFilterMode = filterMode;
     invalidateFilter();
   }
@@ -359,7 +354,6 @@ void QgsAttributeTableFilterModel::selectionChanged()
 {
   if ( ShowSelected == mFilterMode )
   {
-    generateListOfVisibleFeatures();
     invalidateFilter();
   }
   else if ( mSelectedOnTop )

src/gui/attributetable/qgsdualview.cpp

@@ -706,9 +706,7 @@ void QgsDualView::progress( int i, bool& cancel )
     mProgressDlg->show();
   }
 
-  mProgressDlg->setValue( i );
   mProgressDlg->setLabelText( tr( "%1 features loaded." ).arg( i ) );
-
   QCoreApplication::processEvents();
 
   cancel = mProgressDlg && mProgressDlg->wasCanceled();

src/ui/qgsoptionsbase.ui

@@ -1530,7 +1530,7 @@
                        <number>0</number>
                       </property>
                       <property name="maximum">
-                       <number>100000</number>
+                       <number>10000000</number>
                       </property>
                       <property name="singleStep">
                        <number>1000</number>

tests/src/core/testqgsvectorlayercache.cpp

@@ -52,6 +52,9 @@ class TestVectorLayerCache : public QObject
     void testCacheAttrActions(); // Test attribute add/ attribute delete
     void testFeatureActions();   // Test adding/removing features works
     void testSubsetRequest();
+    void testFullCache();
+    void testFullCacheThroughRequest();
+    void testCanUseCacheForRequest();
 
     void onCommittedFeaturesAdded( const QString&, const QgsFeatureList& );
 
@@ -219,6 +222,115 @@ void TestVectorLayerCache::testSubsetRequest()
   QVERIFY( a == f.attribute( 3 ) );
 }
 
+void TestVectorLayerCache::testFullCache()
+{
+  // cache is too small to fit all features
+  QgsVectorLayerCache cache( mPointsLayer, 2 );
+  QVERIFY( !cache.hasFullCache() );
+  QVERIFY( cache.cacheSize() < mPointsLayer->featureCount() );
+  // but we set it to full cache
+  cache.setFullCache( true );
+  // so now it should have sufficient size for all features
+  QVERIFY( cache.cacheSize() >= mPointsLayer->featureCount() );
+  QVERIFY( cache.hasFullCache() );
+
+  // double check that everything is indeed in the cache
+  QgsFeatureIterator it = mPointsLayer->getFeatures();
+  QgsFeature f;
+  while ( it.nextFeature( f ) )
+  {
+    QVERIFY( cache.isFidCached( f.id() ) );
+  }
+}
+
+void TestVectorLayerCache::testFullCacheThroughRequest()
+{
+  // make sure cache is sufficient size for all features
+  QgsVectorLayerCache cache( mPointsLayer, mPointsLayer->featureCount() * 2 );
+  QVERIFY( !cache.hasFullCache() );
+
+  // now request all features from cache
+  QgsFeatureIterator it = cache.getFeatures( QgsFeatureRequest() );
+  QgsFeature f;
+  while ( it.nextFeature( f ) )
+  {
+    // suck in all features
+  }
+
+  // cache should now contain all features
+  it = mPointsLayer->getFeatures();
+  while ( it.nextFeature( f ) )
+  {
+    QVERIFY( cache.isFidCached( f.id() ) );
+  }
+
+  // so it should be a full cache!
+  QVERIFY( cache.hasFullCache() );
+}
+
+void TestVectorLayerCache::testCanUseCacheForRequest()
+{
+  //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();
+
+  QgsVectorLayerCache cache( mPointsLayer, 10 );
+  // initially nothing in cache, so can't use it to fulfill the request
+  QVERIFY( !cache.canUseCacheForRequest( QgsFeatureRequest().setFilterFid( id1 ), it ) );
+  QVERIFY( !cache.canUseCacheForRequest( QgsFeatureRequest().setFilterFid( id2 ), it ) );
+  QVERIFY( !cache.canUseCacheForRequest( QgsFeatureRequest().setFilterFids( QgsFeatureIds() << id1 << id2 ), it ) );
+  QVERIFY( !cache.canUseCacheForRequest( QgsFeatureRequest().setFilterRect( QgsRectangle( 1, 2, 3, 4 ) ), it ) );
+  QVERIFY( !cache.canUseCacheForRequest( QgsFeatureRequest().setFilterExpression( "$x<5" ), it ) );
+
+  // get just the first feature into the cache
+  it = cache.getFeatures( QgsFeatureRequest().setFilterFid( id1 ) );
+  while ( it.nextFeature( f ) ) { }
+  QCOMPARE( cache.cachedFeatureIds(), QgsFeatureIds() << id1 );
+  QVERIFY( cache.canUseCacheForRequest( QgsFeatureRequest().setFilterFid( id1 ), it ) );
+  //verify that the returned iterator was correct
+  QVERIFY( it.nextFeature( f ) );
+  QCOMPARE( f.id(), id1 );
+  QVERIFY( !it.nextFeature( f ) );
+  QVERIFY( !cache.canUseCacheForRequest( QgsFeatureRequest().setFilterFid( id2 ), it ) );
+  QVERIFY( !cache.canUseCacheForRequest( QgsFeatureRequest().setFilterFids( QgsFeatureIds() << id1 << id2 ), it ) );
+  QVERIFY( !cache.canUseCacheForRequest( QgsFeatureRequest().setFilterRect( QgsRectangle( 1, 2, 3, 4 ) ), it ) );
+  QVERIFY( !cache.canUseCacheForRequest( QgsFeatureRequest().setFilterExpression( "$x<5" ), it ) );
+
+  // get feature 2 into cache
+  it = cache.getFeatures( QgsFeatureRequest().setFilterFid( id2 ) );
+  while ( it.nextFeature( f ) ) { }
+  QCOMPARE( cache.cachedFeatureIds(), QgsFeatureIds() << id1 << id2 );
+  QVERIFY( cache.canUseCacheForRequest( QgsFeatureRequest().setFilterFid( id1 ), it ) );
+  QVERIFY( it.nextFeature( f ) );
+  QCOMPARE( f.id(), id1 );
+  QVERIFY( !it.nextFeature( f ) );
+  QVERIFY( cache.canUseCacheForRequest( QgsFeatureRequest().setFilterFid( id2 ), it ) );
+  QVERIFY( it.nextFeature( f ) );
+  QCOMPARE( f.id(), id2 );
+  QVERIFY( !it.nextFeature( f ) );
+  QVERIFY( cache.canUseCacheForRequest( QgsFeatureRequest().setFilterFids( QgsFeatureIds() << id1 << id2 ), it ) );
+  QVERIFY( it.nextFeature( f ) );
+  QgsFeatureIds result;
+  result << f.id();
+  QVERIFY( it.nextFeature( f ) );
+  result << f.id();
+  QCOMPARE( result, QgsFeatureIds() << id1 << id2 );
+  QVERIFY( !cache.canUseCacheForRequest( QgsFeatureRequest().setFilterRect( QgsRectangle( 1, 2, 3, 4 ) ), it ) );
+  QVERIFY( !cache.canUseCacheForRequest( QgsFeatureRequest().setFilterExpression( "$x<5" ), it ) );
+
+  // can only use rect/expression requests if cache has everything
+  cache.setFullCache( true );
+  QVERIFY( cache.canUseCacheForRequest( QgsFeatureRequest().setFilterFid( id1 ), it ) );
+  QVERIFY( cache.canUseCacheForRequest( QgsFeatureRequest().setFilterFid( id2 ), it ) );
+  QVERIFY( cache.canUseCacheForRequest( QgsFeatureRequest().setFilterFids( QgsFeatureIds() << id1 << id2 ), it ) );
+  QVERIFY( cache.canUseCacheForRequest( QgsFeatureRequest().setFilterRect( QgsRectangle( 1, 2, 3, 4 ) ), it ) );
+  QVERIFY( cache.canUseCacheForRequest( QgsFeatureRequest().setFilterExpression( "$x<5" ), it ) );
+}
+
 void TestVectorLayerCache::onCommittedFeaturesAdded( const QString& layerId, const QgsFeatureList& features )
 {
   Q_UNUSED( layerId )

tests/src/gui/testqgsdualview.cpp

@@ -136,12 +136,12 @@ void TestQgsDualView::testSelectAll()
   // Only show parts of the canvas, so only one selected feature is visible
   mCanvas->setExtent( QgsRectangle( -139, 23, -100, 48 ) );
   mDualView->mTableView->selectAll();
-  QVERIFY( mPointsLayer->selectedFeatureCount() == 10 );
+  QCOMPARE( mPointsLayer->selectedFeatureCount(), 10 );
 
   mPointsLayer->selectByIds( QgsFeatureIds() );
   mCanvas->setExtent( QgsRectangle( -110, 40, -100, 48 ) );
   mDualView->mTableView->selectAll();
-  QVERIFY( mPointsLayer->selectedFeatureCount() == 1 );
+  QCOMPARE( mPointsLayer->selectedFeatureCount(), 1 );
 }
 
 void TestQgsDualView::testSort()