Merge pull request #4283 from nyalldawson/filterrect

Remove QgsFeatureRequest::FilterRect
qgis · Mar 21, 2017 · b7d2b9f · b7d2b9f
2 parents 2e9f996 + f3e1772
commit b7d2b9f
Show file tree

Hide file tree

Showing 10 changed files with 57 additions and 52 deletions.
diff --git a/doc/api_break.dox b/doc/api_break.dox
@@ -1062,6 +1062,9 @@ QgsFeatureRequest        {#qgis_api_break_3_0_QgsFeatureRequest}
 -----------------
 
 - AllAttributes was renamed to ALL_ATTRIBUTES
+- FilterRect was removed. This enum value was unused, and QgsFeatureRequest.filterRect() should be used
+instead. Check for a null rectangle returned by filterRect() to determine whether a filter rect
+is in place.
 
 QgsFieldCombobox        {#qgis_api_break_3_0_QgsFieldCombobox}
 ----------------

diff --git a/python/core/qgsfeaturerequest.sip b/python/core/qgsfeaturerequest.sip
@@ -21,7 +21,6 @@ class QgsFeatureRequest
     enum FilterType
     {
       FilterNone,       //!< No filter is applied
-      FilterRect,       //!< Obsolete, will be ignored. If a filterRect is set it will be used anyway. Filter using a rectangle, no need to set NoGeometry. Instead check for request.filterRect().isNull()
       FilterFid,        //!< Filter using feature ID
       FilterExpression, //!< Filter using expression
       FilterFids        //!< Filter using feature IDs

diff --git a/src/core/qgscacheindexfeatureid.cpp b/src/core/qgscacheindexfeatureid.cpp
@@ -63,7 +63,6 @@ bool QgsCacheIndexFeatureId::getCacheIterator( QgsFeatureIterator &featureIterat
       break;
     }
     case QgsFeatureRequest::FilterNone:
-    case QgsFeatureRequest::FilterRect:
     case QgsFeatureRequest::FilterExpression:
     {
       if ( C->hasFullCache() )

diff --git a/src/core/qgsfeaturerequest.cpp b/src/core/qgsfeaturerequest.cpp
@@ -23,51 +23,36 @@
 const QString QgsFeatureRequest::ALL_ATTRIBUTES = QStringLiteral( "#!allattributes!#" );
 
 QgsFeatureRequest::QgsFeatureRequest()
-  : mFilter( FilterNone )
-  , mFilterFid( -1 )
-  , mFilterExpression( nullptr )
-  , mFlags( nullptr )
-  , mLimit( -1 )
+  : mFlags( nullptr )
 {
 }
 
 QgsFeatureRequest::QgsFeatureRequest( QgsFeatureId fid )
   : mFilter( FilterFid )
   , mFilterFid( fid )
-  , mFilterExpression( nullptr )
   , mFlags( nullptr )
-  , mLimit( -1 )
 {
 }
 
 QgsFeatureRequest::QgsFeatureRequest( const QgsFeatureIds &fids )
   : mFilter( FilterFids )
-  , mFilterFid( -1 )
   , mFilterFids( fids )
-  , mFilterExpression( nullptr )
   , mFlags( nullptr )
-  , mLimit( -1 )
 {
 
 }
 
 QgsFeatureRequest::QgsFeatureRequest( const QgsRectangle &rect )
-  : mFilter( FilterRect )
-  , mFilterRect( rect )
-  , mFilterFid( -1 )
-  , mFilterExpression( nullptr )
+  : mFilterRect( rect )
   , mFlags( nullptr )
-  , mLimit( -1 )
 {
 }
 
 QgsFeatureRequest::QgsFeatureRequest( const QgsExpression &expr, const QgsExpressionContext &context )
   : mFilter( FilterExpression )
-  , mFilterFid( -1 )
   , mFilterExpression( new QgsExpression( expr ) )
   , mExpressionContext( context )
   , mFlags( nullptr )
-  , mLimit( -1 )
 {
 }
 
@@ -85,11 +70,11 @@ QgsFeatureRequest &QgsFeatureRequest::operator=( const QgsFeatureRequest &rh )
   mFilterFids = rh.mFilterFids;
   if ( rh.mFilterExpression )
   {
-    mFilterExpression = new QgsExpression( *rh.mFilterExpression );
+    mFilterExpression.reset( new QgsExpression( *rh.mFilterExpression ) );
   }
   else
   {
-    mFilterExpression = nullptr;
+    mFilterExpression.reset( nullptr );
   }
   mExpressionContext = rh.mExpressionContext;
   mAttrs = rh.mAttrs;
@@ -99,15 +84,8 @@ QgsFeatureRequest &QgsFeatureRequest::operator=( const QgsFeatureRequest &rh )
   return *this;
 }
 
-QgsFeatureRequest::~QgsFeatureRequest()
-{
-  delete mFilterExpression;
-}
-
 QgsFeatureRequest &QgsFeatureRequest::setFilterRect( const QgsRectangle &rect )
 {
-  if ( mFilter == FilterNone )
-    mFilter = FilterRect;
   mFilterRect = rect;
   return *this;
 }
@@ -129,8 +107,7 @@ QgsFeatureRequest &QgsFeatureRequest::setFilterFids( const QgsFeatureIds &fids )
 QgsFeatureRequest &QgsFeatureRequest::setFilterExpression( const QString &expression )
 {
   mFilter = FilterExpression;
-  delete mFilterExpression;
-  mFilterExpression = new QgsExpression( expression );
+  mFilterExpression.reset( new QgsExpression( expression ) );
   return *this;
 }
 
@@ -245,14 +222,17 @@ QgsFeatureRequest &QgsFeatureRequest::setSimplifyMethod( const QgsSimplifyMethod
 
 bool QgsFeatureRequest::acceptFeature( const QgsFeature &feature )
 {
+  if ( !mFilterRect.isNull() )
+  {
+    if ( !feature.hasGeometry() || !feature.geometry().intersects( mFilterRect ) )
+      return false;
+  }
+
   switch ( mFilter )
   {
     case QgsFeatureRequest::FilterNone:
       return true;
 
-    case QgsFeatureRequest::FilterRect:
-      return ( feature.hasGeometry() && feature.geometry().intersects( mFilterRect ) );
-
     case QgsFeatureRequest::FilterFid:
       return ( feature.id() == mFilterFid );
 

diff --git a/src/core/qgsfeaturerequest.h b/src/core/qgsfeaturerequest.h
@@ -18,6 +18,7 @@
 #include "qgis_core.h"
 #include <QFlags>
 #include <QList>
+#include <memory>
 
 #include "qgsfeature.h"
 #include "qgsrectangle.h"
@@ -79,7 +80,6 @@ class CORE_EXPORT QgsFeatureRequest
     enum FilterType
     {
       FilterNone,       //!< No filter is applied
-      FilterRect,       //!< Obsolete, will be ignored. If a filterRect is set it will be used anyway. Filter using a rectangle, no need to set NoGeometry. Instead check for request.filterRect().isNull()
       FilterFid,        //!< Filter using feature ID
       FilterExpression, //!< Filter using expression
       FilterFids        //!< Filter using feature IDs
@@ -242,22 +242,21 @@ class CORE_EXPORT QgsFeatureRequest
     //! Assignment operator
     QgsFeatureRequest &operator=( const QgsFeatureRequest &rh );
 
-    ~QgsFeatureRequest();
-
     /**
      * Return the filter type which is currently set on this request
      *
      * @return Filter type
      */
-    FilterType filterType() const { if ( mFilter == FilterNone && !mFilterRect.isNull() ) return FilterRect; else return mFilter; }
+    FilterType filterType() const { return mFilter; }
 
     /**
      * Set rectangle from which features will be taken. Empty rectangle removes the filter.
      */
     QgsFeatureRequest &setFilterRect( const QgsRectangle &rect );
 
     /**
-     * Get the rectangle from which features will be taken.
+     * Get the rectangle from which features will be taken. If the returned
+     * rectangle is null, then no filter rectangle is set.
      */
     const QgsRectangle &filterRect() const { return mFilterRect; }
 
@@ -282,7 +281,7 @@ class CORE_EXPORT QgsFeatureRequest
      * @see setFilterExpression
      * @see expressionContext
      */
-    QgsExpression *filterExpression() const { return mFilterExpression; }
+    QgsExpression *filterExpression() const { return mFilterExpression.get(); }
 
     /** Modifies the existing filter expression to add an additional expression filter. The
      * filter expressions are combined using AND, so only features matching both
@@ -404,20 +403,17 @@ class CORE_EXPORT QgsFeatureRequest
      */
     bool acceptFeature( const QgsFeature &feature );
 
-    // TODO: in future
-    // void setFilterNativeExpression(con QString& expr);   // using provider's SQL (if supported)
-
   protected:
-    FilterType mFilter;
+    FilterType mFilter = FilterNone;
     QgsRectangle mFilterRect;
-    QgsFeatureId mFilterFid;
+    QgsFeatureId mFilterFid = -1;
     QgsFeatureIds mFilterFids;
-    QgsExpression *mFilterExpression = nullptr;
+    std::unique_ptr< QgsExpression > mFilterExpression;
     QgsExpressionContext mExpressionContext;
     Flags mFlags;
     QgsAttributeList mAttrs;
     QgsSimplifyMethod mSimplifyMethod;
-    long mLimit;
+    long mLimit = -1;
     OrderBy mOrderBy;
 };
 

diff --git a/src/core/qgsvectorlayercache.cpp b/src/core/qgsvectorlayercache.cpp
@@ -310,7 +310,6 @@ bool QgsVectorLayerCache::canUseCacheForRequest( const QgsFeatureRequest &featur
       break;
     }
     case QgsFeatureRequest::FilterNone:
-    case QgsFeatureRequest::FilterRect:
     case QgsFeatureRequest::FilterExpression:
     {
       if ( mFullCache )

diff --git a/src/core/qgsvectorlayerfeatureiterator.cpp b/src/core/qgsvectorlayerfeatureiterator.cpp
@@ -191,7 +191,7 @@ QgsVectorLayerFeatureIterator::QgsVectorLayerFeatureIterator( QgsVectorLayerFeat
         providerLimit += mSource->mChangedAttributeValues.size();
       }
 
-      if ( mProviderRequest.filterType() == QgsFeatureRequest::FilterExpression || mProviderRequest.filterType() == QgsFeatureRequest::FilterRect )
+      if ( mProviderRequest.filterType() == QgsFeatureRequest::FilterExpression || !mProviderRequest.filterRect().isNull() )
       {
         // geometry changes may mean some features no longer match expression or rect, so increase limit sent to provider
         providerLimit += mSource->mChangedGeometries.size();

diff --git a/src/providers/arcgisrest/qgsafsfeatureiterator.cpp b/src/providers/arcgisrest/qgsafsfeatureiterator.cpp
@@ -73,7 +73,7 @@ bool QgsAfsFeatureIterator::fetchFeature( QgsFeature &f )
   else
   {
     QgsRectangle filterRect = mSource->provider()->extent();
-    if ( mRequest.filterType() == QgsFeatureRequest::FilterRect )
+    if ( !mRequest.filterRect().isNull() )
       filterRect = filterRect.intersect( &mRequest.filterRect() );
     while ( mFeatureIterator < mSource->provider()->featureCount() )
     {

diff --git a/src/providers/oracle/qgsoraclefeatureiterator.cpp b/src/providers/oracle/qgsoraclefeatureiterator.cpp
@@ -142,9 +142,6 @@ QgsOracleFeatureIterator::QgsOracleFeatureIterator( QgsOracleFeatureSource *sour
       //handled below
       break;
 
-    case QgsFeatureRequest::FilterRect:
-      // Handled in the if-statement above
-      break;
   }
 
   if ( mSource->mRequestedGeomType != QgsWkbTypes::Unknown && mSource->mRequestedGeomType != mSource->mDetectedGeomType )

diff --git a/tests/src/python/providertestbase.py b/tests/src/python/providertestbase.py
@@ -141,6 +141,11 @@ def assert_query(self, provider, expression, expected):
         result = set([f['pk'] for f in provider.getFeatures(QgsFeatureRequest().setFilterExpression(expression).setSubsetOfAttributes([0]))])
         assert set(expected) == result, 'Expected {} and got {} when testing expression "{}" using empty attribute subset'.format(set(expected), result, expression)
 
+        # test that results match QgsFeatureRequest.acceptFeature
+        request = QgsFeatureRequest().setFilterExpression(expression)
+        for f in provider.getFeatures():
+            self.assertEqual(request.acceptFeature(f), f['pk'] in expected)
+
     def runGetFeatureTests(self, provider):
         assert len([f for f in provider.getFeatures()]) == 5
         self.assert_query(provider, 'name ILIKE \'QGIS\'', [])
@@ -485,6 +490,11 @@ def testGetFeaturesFidTests(self):
             expected = [id]
             assert result == expected, 'Expected {} and got {} when testing for feature ID filter'.format(expected, result)
 
+            # test that results match QgsFeatureRequest.acceptFeature
+            request = QgsFeatureRequest().setFilterFid(id)
+            for f in self.provider.getFeatures():
+                self.assertEqual(request.acceptFeature(f), f.id() == id)
+
         # bad features
         it = self.provider.getFeatures(QgsFeatureRequest().setFilterFid(-99999999))
         feature = QgsFeature(5)
@@ -503,6 +513,10 @@ def testGetFeaturesFidsTests(self):
         assert result == expected, 'Expected {} and got {} when testing for feature IDs filter'.format(expected, result)
         self.assertTrue(all_valid)
 
+        # test that results match QgsFeatureRequest.acceptFeature
+        for f in self.provider.getFeatures():
+            self.assertEqual(request.acceptFeature(f), f.id() in expected)
+
         result = set([f.id() for f in self.provider.getFeatures(QgsFeatureRequest().setFilterFids([fids[1], fids[3], fids[4]]))])
         expected = set([fids[1], fids[3], fids[4]])
         assert result == expected, 'Expected {} and got {} when testing for feature IDs filter'.format(expected, result)
@@ -545,6 +559,10 @@ def testGetFeaturesFilterRectTests(self):
         assert set(features) == set([2, 4]), 'Got {} instead'.format(features)
         self.assertTrue(all_valid)
 
+        # test that results match QgsFeatureRequest.acceptFeature
+        for f in self.provider.getFeatures():
+            self.assertEqual(request.acceptFeature(f), f['pk'] in set([2, 4]))
+
         # test with an empty rectangle
         extent = QgsRectangle()
         request = QgsFeatureRequest().setFilterRect(extent)
@@ -590,6 +608,20 @@ def testRectAndExpression(self):
         assert set(expected) == result, 'Expected {} and got {} when testing for combination of filterRect and expression'.format(set(expected), result)
         self.assertTrue(all_valid)
 
+        # shouldn't matter what order this is done in
+        request = QgsFeatureRequest().setFilterRect(extent).setFilterExpression('"cnt">200')
+        result = set([f['pk'] for f in self.provider.getFeatures(request)])
+        all_valid = (all(f.isValid() for f in self.provider.getFeatures(request)))
+        expected = [4]
+        assert set(
+            expected) == result, 'Expected {} and got {} when testing for combination of filterRect and expression'.format(
+            set(expected), result)
+        self.assertTrue(all_valid)
+
+        # test that results match QgsFeatureRequest.acceptFeature
+        for f in self.provider.getFeatures():
+            self.assertEqual(request.acceptFeature(f), f['pk'] in expected)
+
     def testGetFeaturesLimit(self):
         it = self.provider.getFeatures(QgsFeatureRequest().setLimit(2))
         features = [f['pk'] for f in it]