Fix incorrect results when using QgsFeatureRequest.acceptFeature with filter rect

src/core/qgsfeaturerequest.cpp

@@ -245,14 +245,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 );
 

src/core/qgsfeaturerequest.h

@@ -404,9 +404,6 @@ 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;
     QgsRectangle mFilterRect;

tests/src/python/providertestbase.py

@@ -141,6 +141,11 @@ class ProviderTestCase(object):
         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 @@ class ProviderTestCase(object):
             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 @@ class ProviderTestCase(object):
         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 @@ class ProviderTestCase(object):
         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 @@ class ProviderTestCase(object):
         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]