diff --git a/doc/api_break.dox b/doc/api_break.dox index d3b2a60efd7a..82bb4aeea6b2 100644 --- a/doc/api_break.dox +++ b/doc/api_break.dox @@ -1049,6 +1049,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 index b82ec2a8ab36..743a6efe9f4a 100644 --- 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 index 42a9c865a430..0a89a6548ddc 100644 --- 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 index 3870f2738cfc..99a704fbf5d3 100644 --- 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 index c90eecdff8f6..1ac8672d32d9 100644 --- a/src/core/qgsfeaturerequest.h +++ b/src/core/qgsfeaturerequest.h @@ -18,6 +18,7 @@ #include "qgis_core.h" #include #include +#include #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,14 +242,12 @@ 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. @@ -257,7 +255,8 @@ class CORE_EXPORT QgsFeatureRequest 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 index f4276d55ba35..ac658e71cda1 100644 --- 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 index f3d446a8ead1..fd1e20a8b833 100644 --- 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 index 4e4d5314dfdd..c2f48f23a6e9 100644 --- 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 index 20c01fa06815..92bf8bb01d58 100644 --- 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 index a853881ce52a..2132b9aae4fe 100644 --- 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]