Don't use QgsVectorLayer::selectedFeatures() to check for a selection

in a layer This is incredibly inefficient, because selectedFeatures() actually fetches a full copy of all selected features (including all attributes and geometry). Instead use selectedFeatureIds(), which is just a list of numbers. Add warning note to docs cautioning against this practice. Fixes massive ui lockup when right clicking on a layer with selected features in the layer tree
qgis · May 28, 2018 · b6f2f7b · b6f2f7b
1 parent e552ce6
commit b6f2f7b
Show file tree

Hide file tree

Showing 6 changed files with 46 additions and 21 deletions.
diff --git a/python/core/auto_generated/qgsvectorlayer.sip.in b/python/core/auto_generated/qgsvectorlayer.sip.in
@@ -524,9 +524,9 @@ which is used by the layer, so any changes are immediately applied.
 
     int selectedFeatureCount() const;
 %Docstring
-The number of features that are selected in this layer
+Returns the number of features that are selected in this layer.
 
-:return: See description
+.. seealso:: :py:func:`selectedFeatureIds`
 %End
 
     void selectByRect( QgsRectangle &rect, SelectBehavior behavior = SetSelection );
@@ -614,8 +614,15 @@ Invert selection of features found within the search rectangle (in layer's coord
 %Docstring
 Returns a copy of the user-selected features.
 
+.. warning::
+
+   Calling this method triggers a request for all attributes and geometry for the selected features.
+   Consider using the much more efficient selectedFeatureIds() or selectedFeatureCount() if you do not
+   require access to the feature attributes or geometry.
+
 :return: A list of :py:class:`QgsFeature`
 
+
 .. seealso:: :py:func:`selectedFeatureIds`
 
 .. seealso:: :py:func:`getSelectedFeatures`
@@ -631,18 +638,25 @@ Returns an iterator of the selected features.
 :return: Iterator over the selected features
 
 
+.. warning::
+
+   Calling this method returns an iterator for all attributes and geometry for the selected features.
+   Consider using the much more efficient selectedFeatureIds() or selectedFeatureCount() if you do not
+   require access to the feature attributes or geometry.
+
+
 .. seealso:: :py:func:`selectedFeatureIds`
 
 .. seealso:: :py:func:`selectedFeatures`
 %End
 
     const QgsFeatureIds &selectedFeatureIds() const;
 %Docstring
-Returns reference to identifiers of selected features
-
-:return: A list of :py:class:`QgsFeatureId`
+Returns a list of the selected features IDs in this layer.
 
 .. seealso:: :py:func:`selectedFeatures`
+
+.. seealso:: :py:func:`selectedFeatureCount`
 %End
 
     QgsRectangle boundingBoxOfSelected() const;

diff --git a/src/analysis/processing/qgsalgorithmsaveselectedfeatures.cpp b/src/analysis/processing/qgsalgorithmsaveselectedfeatures.cpp
@@ -82,7 +82,7 @@ QVariantMap QgsSaveSelectedFeatures::processAlgorithm( const QVariantMap &parame
   int current = 0;
   double step = count > 0 ? 100.0 / count : 1;
 
-  QgsFeatureIterator it = selectLayer->getSelectedFeatures();;
+  QgsFeatureIterator it = selectLayer->getSelectedFeatures();
   QgsFeature feat;
   while ( it.nextFeature( feat ) )
   {

diff --git a/src/app/qgisapp.cpp b/src/app/qgisapp.cpp
@@ -7903,7 +7903,7 @@ void QgisApp::mergeAttributesOfSelectedFeatures()
   }
 
   //get initial selection (may be altered by attribute merge dialog later)
-  QgsFeatureList featureList = vl->selectedFeatures();  //get QList<QgsFeature>
+  QgsFeatureList featureList = vl->selectedFeatures();
 
   //merge the attributes together
   QgsMergeAttributesDialog d( featureList, vl, mapCanvas() );
@@ -8043,7 +8043,7 @@ void QgisApp::mergeSelectedFeatures()
 
   //get initial selection (may be altered by attribute merge dialog later)
   QgsFeatureIds featureIds = vl->selectedFeatureIds();
-  QgsFeatureList featureList = vl->selectedFeatures();  //get QList<QgsFeature>
+  QgsFeatureList featureList = vl->selectedFeatures();
   bool canceled;
   QgsGeometry unionGeom = unionGeometries( vl, featureList, canceled );
   if ( unionGeom.isNull() )
@@ -13600,11 +13600,7 @@ QgsFeature QgisApp::duplicateFeatures( QgsMapLayer *mlayer, const QgsFeature &fe
   }
   else
   {
-    const auto selectedFeatures = layer->selectedFeatures();
-    for ( const QgsFeature &f : selectedFeatures )
-    {
-      featureList.append( f );
-    }
+    featureList.append( layer->selectedFeatures() );
   }
 
   int featureCount = 0;

diff --git a/src/app/qgsapplayertreeviewmenuprovider.cpp b/src/app/qgsapplayertreeviewmenuprovider.cpp
@@ -139,7 +139,7 @@ QMenu *QgsAppLayerTreeViewMenuProvider::createContextMenu()
         if ( vlayer )
         {
           QAction *actionZoomSelected = actions->actionZoomToSelection( mCanvas, menu );
-          actionZoomSelected->setEnabled( !vlayer->selectedFeatures().isEmpty() );
+          actionZoomSelected->setEnabled( !vlayer->selectedFeatureIds().isEmpty() );
           menu->addAction( actionZoomSelected );
         }
         menu->addAction( actions->actionShowInOverview( menu ) );

diff --git a/src/app/qgsmaptooladdpart.cpp b/src/app/qgsmaptooladdpart.cpp
@@ -64,9 +64,16 @@ void QgsMapToolAddPart::cadCanvasReleaseEvent( QgsMapMouseEvent *e )
   }
 
   bool isGeometryEmpty = false;
-  QgsFeatureList selectedFeatures = vlayer->selectedFeatures();
-  if ( !selectedFeatures.isEmpty() && selectedFeatures.at( 0 ).geometry().isNull() )
-    isGeometryEmpty = true;
+  if ( vlayer->selectedFeatureCount() > 0 )
+  {
+    // be efficient here - only grab the first selected feature if there's a selection, don't
+    // fetch all the other features which we don't require.
+    QgsFeatureIterator selectedFeatures = vlayer->getSelectedFeatures();
+    QgsFeature firstSelectedFeature;
+    if ( selectedFeatures.nextFeature( firstSelectedFeature ) )
+      if ( !firstSelectedFeature.geometry().isNull() )
+        isGeometryEmpty = true;
+  }
 
   if ( !checkSelection() )
   {

diff --git a/src/core/qgsvectorlayer.h b/src/core/qgsvectorlayer.h
@@ -588,9 +588,9 @@ class CORE_EXPORT QgsVectorLayer : public QgsMapLayer, public QgsExpressionConte
     const QgsActionManager *actions() const SIP_SKIP { return mActions; }
 
     /**
-     * The number of features that are selected in this layer
+     * Returns the number of features that are selected in this layer.
      *
-     * \returns See description
+     * \see selectedFeatureIds()
      */
     int selectedFeatureCount() const;
 
@@ -659,6 +659,10 @@ class CORE_EXPORT QgsVectorLayer : public QgsMapLayer, public QgsExpressionConte
     /**
      * Returns a copy of the user-selected features.
      *
+     * \warning Calling this method triggers a request for all attributes and geometry for the selected features.
+     * Consider using the much more efficient selectedFeatureIds() or selectedFeatureCount() if you do not
+     * require access to the feature attributes or geometry.
+     *
      * \returns A list of QgsFeature
      *
      * \see    selectedFeatureIds()
@@ -674,16 +678,20 @@ class CORE_EXPORT QgsVectorLayer : public QgsMapLayer, public QgsExpressionConte
      *
      * \returns Iterator over the selected features
      *
+     * \warning Calling this method returns an iterator for all attributes and geometry for the selected features.
+     * Consider using the much more efficient selectedFeatureIds() or selectedFeatureCount() if you do not
+     * require access to the feature attributes or geometry.
+     *
      * \see    selectedFeatureIds()
      * \see    selectedFeatures()
      */
     QgsFeatureIterator getSelectedFeatures( QgsFeatureRequest request = QgsFeatureRequest() ) const;
 
     /**
-     * Returns reference to identifiers of selected features
+     * Returns a list of the selected features IDs in this layer.
      *
-     * \returns A list of QgsFeatureId
      * \see selectedFeatures()
+     * \see selectedFeatureCount()
      */
     const QgsFeatureIds &selectedFeatureIds() const;