Fix attributetable and vectorlayercache

* In the attributetable there was a mess with references and pointers, originating from 66fadee8ef.
* QgsVectorLayerCache did sometimes cache features which did not contain all information which needs to be cached and therefore corrupting the cache and leading to incomplete cache hits.
* Add a unit test for the cache problem
* Fix QgsCacheIndexFeatureId
* QgsAbstractCacheIndex::getCacheIterator now produces a QgsFeatureIterator (instead of a list of Feature Ids). This allows to combine a mixed response, partly satisfied by the cache and partly by an additional query to the backend.

src/core/qgscacheindex.h

@@ -19,6 +19,7 @@
 #include "qgsfeature.h" // QgsFeatureIds
 
 class QgsFeatureRequest;
+class QgsFeatureIterator;
 
 /**
  * @brief
@@ -60,14 +61,15 @@ class QgsAbstractCacheIndex
      * 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 cachedFeatures   A reference to {@link QgsFeatureIds}, where a list of ids is written to,
-     *                         in case this index is able to answer the request.
+     * @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 getCachedIds( QgsFeatureIds& cachedFeatures, const QgsFeatureRequest& featureRequest ) = 0;
+    virtual bool getCacheIterator( QgsFeatureIterator& featureIterator, const QgsFeatureRequest& featureRequest ) = 0;
 };
 
 #endif // QGSCACHEINDEX_H

src/core/qgscacheindexfeatureid.cpp

@@ -14,11 +14,45 @@
  ***************************************************************************/
 
 #include "qgscacheindexfeatureid.h"
+#include "qgsfeaturerequest.h"
+#include "qgscachedfeatureiterator.h"
+#include "qgsvectorlayercache.h"
 
-QgsCacheIndexFeatureId::QgsCacheIndexFeatureId( QgsCachedVectorLayer* cachedVectorLayer )
+QgsCacheIndexFeatureId::QgsCacheIndexFeatureId( QgsVectorLayerCache* cachedVectorLayer )
     : QgsAbstractCacheIndex()
     , C( cachedVectorLayer )
 {
 
 }
 
+void QgsCacheIndexFeatureId::flushFeature(const QgsFeatureId fid)
+{
+  Q_UNUSED( fid )
+}
+
+void QgsCacheIndexFeatureId::flush()
+{
+}
+
+void QgsCacheIndexFeatureId::requestCompleted( QgsFeatureRequest featureRequest, QgsFeatureIds fids )
+{
+  Q_UNUSED( featureRequest )
+  Q_UNUSED( fids )
+}
+
+bool QgsCacheIndexFeatureId::getCacheIterator( QgsFeatureIterator &featureIterator, const QgsFeatureRequest &featureRequest )
+{
+  if( featureRequest.filterType() == QgsFeatureRequest::FilterFid )
+  {
+    if ( C->isFidCached( featureRequest.filterFid() ) )
+    {
+      QgsFeatureIds fids;
+      fids << featureRequest.filterFid();
+      featureIterator = QgsFeatureIterator( new QgsCachedFeatureIterator( C, featureRequest, fids ) );
+      return true;
+    }
+  }
+
+  return false;
+}
+

src/core/qgscacheindexfeatureid.h

@@ -18,19 +18,57 @@
 
 #include "qgscacheindex.h"
 
-class QgsCachedVectorLayer;
+class QgsVectorLayerCache;
 
 class QgsCacheIndexFeatureId : public QgsAbstractCacheIndex
 {
   public:
-    QgsCacheIndexFeatureId( QgsCachedVectorLayer* cachedVectorLayer );
+    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( QgsFeatureRequest featureRequest, 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 cachedFeatures   A reference to {@link QgsFeatureIds}, where a list of ids is written to,
+     *                         in case this index is able to answer the request.
+     * @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 );
 
   signals:
 
   public slots:
 
   private:
-    QgsCachedVectorLayer* C;
+    QgsVectorLayerCache* C;
 };
 
 #endif // QGSCACHEINDEXFEATUREID_H

src/core/qgsfeaturerequest.cpp

@@ -38,6 +38,15 @@ QgsFeatureRequest::QgsFeatureRequest( const QgsRectangle& rect )
 {
 }
 
+QgsFeatureRequest::QgsFeatureRequest( const QgsFeatureRequest &rh )
+{
+  mFlags = rh.mFlags;
+  mFilter = rh.mFilter;
+  mFilterRect = rh.mFilterRect;
+  mFilterFid = rh.mFilterFid;
+  mAttrs = rh.mAttrs;
+}
+
 
 QgsFeatureRequest& QgsFeatureRequest::setFilterRect( const QgsRectangle& rect )
 {

src/core/qgsfeaturerequest.h

@@ -78,6 +78,8 @@ class CORE_EXPORT QgsFeatureRequest
     //! construct a request with rectangle filter
     explicit QgsFeatureRequest( const QgsRectangle& rect );
 
+    QgsFeatureRequest( const QgsFeatureRequest& rh );
+
     FilterType filterType() const { return mFilter; }
 
     //! Set rectangle from which features will be taken. Empty rectangle removes the filter.

src/core/qgsvectorlayercache.cpp

@@ -105,6 +105,11 @@ void QgsVectorLayerCache::setFullCache( bool fullCache )
   }
 }
 
+void QgsVectorLayerCache::addCacheIndex( QgsAbstractCacheIndex* cacheIndex )
+{
+  mCacheIndices.append( cacheIndex );
+}
+
 void QgsVectorLayerCache::setCacheAddedAttributes( bool cacheAddedAttributes )
 {
   if ( cacheAddedAttributes )
@@ -251,11 +256,8 @@ QgsFeatureIterator QgsVectorLayerCache::getFeatures( const QgsFeatureRequest &fe
     // Check if an index is able to deliver the requested features
     foreach ( QgsAbstractCacheIndex *idx, mCacheIndices )
     {
-      QgsFeatureIds featureIds;
-
-      if ( idx->getCachedIds( featureIds, featureRequest ) )
+      if ( idx->getCacheIterator( it, featureRequest ) )
       {
-        it = QgsFeatureIterator( new QgsCachedFeatureIterator( this, featureRequest, featureIds ) );
         requiresWriterIt = false;
         break;
       }
@@ -272,12 +274,27 @@ QgsFeatureIterator QgsVectorLayerCache::getFeatures( const QgsFeatureRequest &fe
   if ( requiresWriterIt && mLayer->dataProvider() )
   {
     // No index was able to satisfy the request
-    it = QgsFeatureIterator( new QgsCachedFeatureWriterIterator( this, featureRequest ) );
+    QgsFeatureRequest myRequest = QgsFeatureRequest( myRequest );
+
+    // Make sure if we cache the geometry, it gets fetched
+    if ( mCacheGeometry )
+      myRequest.setFlags( featureRequest.flags() & ~QgsFeatureRequest::NoGeometry );
+
+    // Make sure, all the cached attributes are requested as well
+    QSet<int> attrs = featureRequest.subsetOfAttributes().toSet() + mCachedAttributes.toSet();
+    myRequest.setSubsetOfAttributes( attrs.toList() );
+
+    it = QgsFeatureIterator( new QgsCachedFeatureWriterIterator( this, myRequest ) );
   }
 
   return it;
 }
 
+bool QgsVectorLayerCache::isFidCached( const QgsFeatureId fid )
+{
+  return mCache.contains( fid );
+}
+
 bool QgsVectorLayerCache::checkInformationCovered( const QgsFeatureRequest& featureRequest )
 {
   QgsAttributeList requestedAttributes;

src/core/qgsvectorlayercache.h

@@ -139,10 +139,26 @@ class CORE_EXPORT QgsVectorLayerCache : public QObject
      *
      * @param cacheIndex  The cache index to add.
      */
-    void addCacheIndex( const QgsAbstractCacheIndex& cacheIndex );
+    void addCacheIndex( QgsAbstractCacheIndex *cacheIndex );
 
+    /**
+     * Query this VectorLayerCache for features.
+     * If the VectorLayerCache (and moreover any of its indices) is able to satisfy
+     * the request, the returned {@link QgsFeatureIterator} will iterate over cached features.
+     * If it's not possible to fully satisfy the request from the cache, part or all of the features
+     * will be requested from the data provider.
+     * @param featureRequest  The request specifying filter and required data.
+     * @return An iterator over the requested data.
+     */
     QgsFeatureIterator getFeatures( const QgsFeatureRequest& featureRequest );
 
+    /**
+     * 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
+     */
+    bool isFidCached(const QgsFeatureId fid );
+
     /**
      * Gets the feature at the given feature id. Considers the changed, added, deleted and permanent features
      * @param featureId The id of the feature to query

src/gui/attributetable/qgsattributetablemodel.cpp

@@ -486,30 +486,26 @@ QVariant QgsAttributeTableModel::data( const QModelIndex &index, int role ) cons
       return QVariant( Qt::AlignLeft );
   }
 
-  const QVariant* pVal = NULL;
+  QVariant val;
 
   // if we don't have the row in current cache, load it from layer first
-  if ( mFeat.id() != rowId || !mFeat.isValid() )
-  {
   if ( mCachedField == fieldId )
   {
-      const QVariant& val = mFieldCache[rowId];
-      pVal = &val;
+    val = mFieldCache[ rowId ];
   }
-    else if ( !loadFeatureAtId( rowId ) )
-      return QVariant( "ERROR" );
-  }
-
-  if ( pVal == NULL && mFeat.id() != rowId )
-    return QVariant( "ERROR" );
-
-  if ( !pVal )
+  else
   {
-    const QVariant& val = mFeat.attribute( fieldId );
-    pVal =&val;
+    if ( mFeat.id() != rowId || !mFeat.isValid() )
+    {
+      if ( !loadFeatureAtId( rowId ) )
+        return QVariant( "ERROR" );
+
+      if ( mFeat.id() != rowId )
+        return QVariant( "ERROR" );
     }
 
-  const QVariant& val = *pVal;
+    val = mFeat.attribute( fieldId );
+  }
 
   // For sorting return unprocessed value
   if ( SortRole == role )

src/gui/attributetable/qgsattributetablemodel.h

@@ -261,7 +261,7 @@ class GUI_EXPORT QgsAttributeTableModel: public QAbstractTableModel
     /** The currently cached column */
     int mCachedField;
     /** Allows to cache one specific column (used for sorting) */
-    QMap<QgsFeatureId, QVariant> mFieldCache;
+    QHash<QgsFeatureId, QVariant> mFieldCache;
 };
 
 

tests/src/core/testqgsvectorlayercache.cpp

@@ -23,6 +23,8 @@
 #include <qgsvectordataprovider.h>
 #include <qgsapplication.h>
 #include <qgsvectorlayereditbuffer.h>
+#include <qgscacheindexfeatureid.h>
+#include <QDebug>
 
 /** @ingroup UnitTests
  * This is a unit test for the vector layer cache
@@ -36,17 +38,19 @@ class TestVectorLayerCache: public QObject
   private slots:
     void initTestCase();      // will be called before the first testfunction is executed.
     void cleanupTestCase();   // will be called after the last testfunction was executed.
-    void init() {};           // will be called before each testfunction is executed.
-    void cleanup() {};        // will be called after every testfunction.
+    void init();              // will be called before each testfunction is executed.
+    void cleanup();           // will be called after every testfunction.
 
     void testCacheOverflow();    // Test cache will work if too many features to cache them all are present
     void testCacheAttrActions(); // Test attribute add/ attribute delete
     void testFeatureActions();   // Test adding/removing features works
+    void testSubsetRequest();
 
     void onCommittedFeaturesAdded( QString, QgsFeatureList );
 
   private:
     QgsVectorLayerCache*           mVectorLayerCache;
+    QgsCacheIndexFeatureId*        mFeatureIdIndex;
     QgsVectorLayer*                mPointsLayer;
     QgsFeatureList                 mAddedFeatures;
     QMap<QString, QString> mTmpFiles;
@@ -88,8 +92,19 @@ void TestVectorLayerCache::initTestCase()
   QFileInfo myPointFileInfo( myPointsFileName );
   mPointsLayer = new QgsVectorLayer( myPointFileInfo.filePath(),
                                      myPointFileInfo.completeBaseName(), "ogr" );
+}
 
+void TestVectorLayerCache::init()
+{
   mVectorLayerCache = new QgsVectorLayerCache( mPointsLayer, 10 );
+  mFeatureIdIndex = new QgsCacheIndexFeatureId( mVectorLayerCache );
+  mVectorLayerCache->addCacheIndex( mFeatureIdIndex );
+}
+
+void TestVectorLayerCache::cleanup()
+{
+  delete mVectorLayerCache;
+  delete mFeatureIdIndex;
 }
 
 //runs after all tests
@@ -107,8 +122,6 @@ void TestVectorLayerCache::cleanupTestCase()
     mAddedFeatures.clear();
   }
 
-  delete mVectorLayerCache;
-  mVectorLayerCache = NULL;
 
   delete mPointsLayer;
   mPointsLayer = NULL;
@@ -190,9 +203,28 @@ void TestVectorLayerCache::testFeatureActions()
   // Delete feature...
   mPointsLayer->startEditing();
   QVERIFY( mPointsLayer->deleteFeature( fid ) );
-  mPointsLayer->commitChanges();
 
   QVERIFY( false == mVectorLayerCache->featureAtId( fid, f ) );
+  mPointsLayer->rollBack();
+}
+
+void TestVectorLayerCache::testSubsetRequest()
+{
+  QgsFeature f;
+
+  QgsFields fields = mPointsLayer->pendingFields();
+  QStringList requiredFields;
+  requiredFields << "Class" << "Cabin Crew";
+
+  mVectorLayerCache->featureAtId( 16, f );
+  QVariant a = f.attribute( 3 );
+
+  QgsFeatureIterator itSubset = mVectorLayerCache->getFeatures( QgsFeatureRequest().setSubsetOfAttributes( requiredFields, fields) );
+  while ( itSubset.nextFeature( f ) ) {}
+  itSubset.close();
+
+  mVectorLayerCache->featureAtId( 16, f );
+  QVERIFY( a == f.attribute( 3 ) );
 }
 
 void TestVectorLayerCache::onCommittedFeaturesAdded( QString layerId, QgsFeatureList features )