Merge pull request #38908 from 3nids/locator-fixes

Some locator fixes
qgis · Sep 22, 2020 · b4797b1 · b4797b1
2 parents 063b8da + d6f9b07
commit b4797b1
Show file tree

Hide file tree

Showing 5 changed files with 166 additions and 40 deletions.
diff --git a/src/app/locator/qgsinbuiltlocatorfilters.cpp b/src/app/locator/qgsinbuiltlocatorfilters.cpp
@@ -244,12 +244,13 @@ QgsActiveLayerFeaturesLocatorFilter *QgsActiveLayerFeaturesLocatorFilter::clone(
   return new QgsActiveLayerFeaturesLocatorFilter();
 }
 
-QString QgsActiveLayerFeaturesLocatorFilter::fieldRestriction( QString &searchString ) const
+QString QgsActiveLayerFeaturesLocatorFilter::fieldRestriction( QString &searchString )
 {
   QString _fieldRestriction;
+  searchString = searchString.trimmed();
   if ( searchString.startsWith( '@' ) )
   {
-    _fieldRestriction = searchString.left( std::max( searchString.indexOf( ' ' ), 0 ) ).remove( 0, 1 );
+    _fieldRestriction = searchString.left( std::min( searchString.indexOf( ' ' ), searchString.length() ) ).remove( 0, 1 );
     searchString = searchString.mid( _fieldRestriction.length() + 2 );
   }
   return _fieldRestriction;
@@ -278,11 +279,30 @@ QStringList QgsActiveLayerFeaturesLocatorFilter::prepare( const QString &string,
   bool allowNumeric = false;
   double numericalValue = searchString.toDouble( &allowNumeric );
 
+  // search in display expression if no field restriction
+  if ( _fieldRestriction.isEmpty() )
+  {
+    QgsFeatureRequest req;
+    req.setSubsetOfAttributes( qgis::setToList( mDispExpression.referencedAttributeIndexes( layer->fields() ) ) );
+    if ( !mDispExpression.needsGeometry() )
+      req.setFlags( QgsFeatureRequest::NoGeometry );
+    QString enhancedSearch = searchString;
+    enhancedSearch.replace( ' ', '%' );
+    req.setFilterExpression( QStringLiteral( "%1 ILIKE '%%2%'" )
+                             .arg( layer->displayExpression(), enhancedSearch ) );
+    req.setLimit( mMaxTotalResults );
+    mDisplayTitleIterator = layer->getFeatures( req );
+  }
+  else
+  {
+    mDisplayTitleIterator = QgsFeatureIterator();
+  }
+
   // build up request expression
   QStringList expressionParts;
   QStringList completionList;
   const QgsFields fields = layer->fields();
-  QgsAttributeList subsetOfAttributes;
+  QgsAttributeList subsetOfAttributes = qgis::setToList( mDispExpression.referencedAttributeIndexes( layer->fields() ) );
   for ( const QgsField &field : fields )
   {
     if ( field.configurationFlags().testFlag( QgsField::ConfigurationFlag::NotSearchable ) )
@@ -292,9 +312,14 @@ QStringList QgsActiveLayerFeaturesLocatorFilter::prepare( const QString &string,
       continue;
 
     if ( !_fieldRestriction.isEmpty() )
-      subsetOfAttributes << layer->fields().indexFromName( field.name() );
+    {
+      int index = layer->fields().indexFromName( field.name() );
+      if ( !subsetOfAttributes.contains( index ) )
+        subsetOfAttributes << index;
+    }
 
     completionList.append( QStringLiteral( "@%1 " ).arg( field.name() ) );
+
     if ( field.type() == QVariant::String )
     {
       expressionParts << QStringLiteral( "%1 ILIKE '%%2%'" ).arg( QgsExpression::quotedColumnRef( field.name() ),
@@ -309,13 +334,14 @@ QStringList QgsActiveLayerFeaturesLocatorFilter::prepare( const QString &string,
   QString expression = QStringLiteral( "(%1)" ).arg( expressionParts.join( QStringLiteral( " ) OR ( " ) ) );
 
   QgsFeatureRequest req;
-  req.setFlags( QgsFeatureRequest::NoGeometry );
+  if ( !mDispExpression.needsGeometry() )
+    req.setFlags( QgsFeatureRequest::NoGeometry );
   req.setFilterExpression( expression );
   if ( !_fieldRestriction.isEmpty() )
     req.setSubsetOfAttributes( subsetOfAttributes );
 
-  req.setLimit( 30 );
-  mIterator = layer->getFeatures( req );
+  req.setLimit( mMaxTotalResults );
+  mFieldIterator = layer->getFeatures( req );
 
   mLayerId = layer->id();
   mLayerIcon = QgsMapLayerModel::iconForLayer( layer );
@@ -330,16 +356,46 @@ QStringList QgsActiveLayerFeaturesLocatorFilter::prepare( const QString &string,
 
 void QgsActiveLayerFeaturesLocatorFilter::fetchResults( const QString &string, const QgsLocatorContext &, QgsFeedback *feedback )
 {
-  int found = 0;
+  QgsFeatureIds featuresFound;
   QgsFeature f;
   QString searchString = string;
   fieldRestriction( searchString );
 
-  while ( mIterator.nextFeature( f ) )
+  // search in display title
+  if ( mDisplayTitleIterator.isValid() )
+  {
+    while ( mDisplayTitleIterator.nextFeature( f ) )
+    {
+      if ( feedback->isCanceled() )
+        return;
+
+      mContext.setFeature( f );
+
+      QgsLocatorResult result;
+
+      result.displayString =  mDispExpression.evaluate( &mContext ).toString();
+      result.userData = QVariantList() << f.id() << mLayerId;
+      result.icon = mLayerIcon;
+      result.score = static_cast< double >( searchString.length() ) / result.displayString.size();
+      emit resultFetched( result );
+
+      featuresFound << f.id();
+
+      if ( featuresFound.count() >= mMaxTotalResults )
+        break;
+    }
+  }
+
+  // search in fields
+  while ( mFieldIterator.nextFeature( f ) )
   {
     if ( feedback->isCanceled() )
       return;
 
+    // do not display twice the same feature
+    if ( featuresFound.contains( f.id() ) )
+      continue;
+
     QgsLocatorResult result;
 
     mContext.setFeature( f );
@@ -364,14 +420,13 @@ void QgsActiveLayerFeaturesLocatorFilter::fetchResults( const QString &string, c
       continue; //not sure how this result slipped through...
 
     result.description = mDispExpression.evaluate( &mContext ).toString();
-
     result.userData = QVariantList() << f.id() << mLayerId;
     result.icon = mLayerIcon;
     result.score = static_cast< double >( searchString.length() ) / result.displayString.size();
     emit resultFetched( result );
 
-    found++;
-    if ( found >= mMaxTotalResults )
+    featuresFound << f.id();
+    if ( featuresFound.count() >= mMaxTotalResults )
       break;
   }
 }

diff --git a/src/app/locator/qgsinbuiltlocatorfilters.h b/src/app/locator/qgsinbuiltlocatorfilters.h
@@ -114,15 +114,18 @@ class APP_EXPORT QgsActiveLayerFeaturesLocatorFilter : public QgsLocatorFilter
      * Returns the field restriction if defined (starting with @)
      * The \a searchString is modified accordingly by removing the field restriction
      */
-    QString fieldRestriction( QString &searchString ) const;
+    static QString fieldRestriction( QString &searchString );
 
     QgsExpression mDispExpression;
     QgsExpressionContext mContext;
-    QgsFeatureIterator mIterator;
+    QgsFeatureIterator mDisplayTitleIterator;
+    QgsFeatureIterator mFieldIterator;
     QString mLayerId;
     QIcon mLayerIcon;
     QStringList mAttributeAliases;
     int mMaxTotalResults = 30;
+
+    friend class TestQgsAppLocatorFilters;
 };
 
 class APP_EXPORT QgsAllLayersFeaturesLocatorFilter : public QgsLocatorFilter

diff --git a/src/gui/qgsmapcanvas.cpp b/src/gui/qgsmapcanvas.cpp
@@ -1240,33 +1240,13 @@ void QgsMapCanvas::clearExtentHistory()
   emit zoomNextStatusChanged( mLastExtentIndex < mLastExtent.size() - 1 );
 }// clearExtentHistory
 
-void QgsMapCanvas::zoomToSelected( QgsVectorLayer *layer )
+QgsRectangle QgsMapCanvas::optimalExtentForPointLayer( QgsVectorLayer *layer, const QgsPointXY &center, int scaleFactor )
 {
-  if ( !layer )
-  {
-    // use current layer by default
-    layer = qobject_cast<QgsVectorLayer *>( mCurrentLayer );
-  }
+  QgsRectangle rect( center, center );
 
-  if ( !layer || !layer->isSpatial() || layer->selectedFeatureCount() == 0 )
-    return;
-
-  QgsRectangle rect = layer->boundingBoxOfSelected();
-  if ( rect.isNull() )
+  if ( layer->geometryType() == QgsWkbTypes::PointGeometry )
   {
-    emit messageEmitted( tr( "Cannot zoom to selected feature(s)" ), tr( "No extent could be determined." ), Qgis::Warning );
-    return;
-  }
-
-  rect = mapSettings().layerExtentToOutputExtent( layer, rect );
-
-  // zoom in if point cannot be distinguished from others
-  // also check that rect is empty, as it might not in case of multi points
-  if ( layer->geometryType() == QgsWkbTypes::PointGeometry && rect.isEmpty() )
-  {
-    int scaleFactor = 5;
-    QgsPointXY centerMapCoordinates = rect.center();
-    QgsPointXY centerLayerCoordinates = mSettings.mapToLayerCoordinates( layer, centerMapCoordinates );
+    QgsPointXY centerLayerCoordinates = mSettings.mapToLayerCoordinates( layer, center );
     QgsRectangle extentRect = mSettings.mapToLayerCoordinates( layer, extent() ).scaled( 1.0 / scaleFactor, &centerLayerCoordinates );
     QgsFeatureRequest req = QgsFeatureRequest().setFilterRect( extentRect ).setLimit( 1000 ).setNoAttributes();
     QgsFeatureIterator fit = layer->getFeatures( req );
@@ -1288,10 +1268,38 @@ void QgsMapCanvas::zoomToSelected( QgsVectorLayer *layer )
     {
       // combine selected point with closest point and scale this rect
       rect.combineExtentWith( mSettings.layerToMapCoordinates( layer, closestPoint ) );
-      rect.scale( scaleFactor, &centerMapCoordinates );
+      rect.scale( scaleFactor, &center );
     }
   }
+  return rect;
+}
+
+void QgsMapCanvas::zoomToSelected( QgsVectorLayer *layer )
+{
+  if ( !layer )
+  {
+    // use current layer by default
+    layer = qobject_cast<QgsVectorLayer *>( mCurrentLayer );
+  }
 
+  if ( !layer || !layer->isSpatial() || layer->selectedFeatureCount() == 0 )
+    return;
+
+  QgsRectangle rect = layer->boundingBoxOfSelected();
+  if ( rect.isNull() )
+  {
+    emit messageEmitted( tr( "Cannot zoom to selected feature(s)" ), tr( "No extent could be determined." ), Qgis::Warning );
+    return;
+  }
+
+  rect = mapSettings().layerExtentToOutputExtent( layer, rect );
+
+  // zoom in if point cannot be distinguished from others
+  // also check that rect is empty, as it might not in case of multi points
+  if ( layer->geometryType() == QgsWkbTypes::PointGeometry && rect.isEmpty() )
+  {
+    rect = optimalExtentForPointLayer( layer, rect.center() );
+  }
   zoomToFeatureExtent( rect );
 }
 
@@ -1330,6 +1338,10 @@ void QgsMapCanvas::zoomToFeatureIds( QgsVectorLayer *layer, const QgsFeatureIds
   QString errorMsg;
   if ( boundingBoxOfFeatureIds( ids, layer, bbox, errorMsg ) )
   {
+    if ( bbox.isEmpty() )
+    {
+      bbox = optimalExtentForPointLayer( layer, bbox.center() );
+    }
     zoomToFeatureExtent( bbox );
   }
   else

diff --git a/src/gui/qgsmapcanvas.h b/src/gui/qgsmapcanvas.h
@@ -1265,6 +1265,14 @@ class GUI_EXPORT QgsMapCanvas : public QGraphicsView
     */
     bool boundingBoxOfFeatureIds( const QgsFeatureIds &ids, QgsVectorLayer *layer, QgsRectangle &bbox, QString &errorMsg ) const;
 
+    /**
+     * Rerturns the optimal extent for a point \a layer and a given \a center point in canvas CRS.
+     * This will return an extent combined of the center and the closest point in the layer.
+     * The extent can be scaled with a \a scale factor.
+     * The returned extent might be an empty rect if it cannot be determnined.
+     */
+    QgsRectangle optimalExtentForPointLayer( QgsVectorLayer *layer, const QgsPointXY &center, int scaleFactor = 5 );
+
     void setLayersPrivate( const QList<QgsMapLayer *> &layers );
 
     void startPreviewJobs();

diff --git a/tests/src/app/testqgsapplocatorfilters.cpp b/tests/src/app/testqgsapplocatorfilters.cpp
@@ -37,6 +37,8 @@ class TestQgsAppLocatorFilters : public QObject
     void testLayers();
     void testLayouts();
     void testSearchActiveLayer();
+    void testActiveLayerFieldRestriction();
+    void testActiveLayerCompletion();
     void testSearchAllLayers();
     void testSearchAllLayersPrioritizeExactMatch();
     void testGoto();
@@ -160,8 +162,11 @@ void TestQgsAppLocatorFilters::testSearchActiveLayer()
   QgsFeature f;
   f.setAttributes( QVector<QVariant>() << 1001 << "A nice feature" << 1234567890 << 12345.6789 );
   f.setGeometry( QgsGeometry::fromWkt( "Point (-71.123 78.23)" ) );
-
   vl->dataProvider()->addFeature( f );
+  QgsFeature f2;
+  f2.setAttributes( QVector<QVariant>() << 100 << "@home" << 13579 << 13.57 );
+  f2.setGeometry( QgsGeometry::fromWkt( "Point (-71.223 78.33)" ) );
+  vl->dataProvider()->addFeature( f2 );
 
   mQgisApp->setActiveLayer( vl );
 
@@ -189,6 +194,49 @@ void TestQgsAppLocatorFilters::testSearchActiveLayer()
   results = gatherResults( &filter, QStringLiteral( "@unknown_field nice" ), context );
   QCOMPARE( results.count(), 0 );
 
+  // check with display expression, feature should not be shown twice
+  vl->setDisplayExpression( QStringLiteral( "concat(\"my_text\", ' ', \"my_double\")" ) );
+  results = gatherResults( &filter, QStringLiteral( "nice" ), context );
+  QCOMPARE( results.count(), 1 );
+  results = gatherResults( &filter, QStringLiteral( "a feature" ), context );
+  QCOMPARE( results.count(), 1 );
+  results = gatherResults( &filter, QStringLiteral( "nice .678" ), context );
+  QCOMPARE( results.count(), 1 );
+
+  results = gatherResults( &filter, QStringLiteral( "@my_text @home" ), context );
+  QCOMPARE( results.count(), 1 );
+
+  QgsProject::instance()->removeAllMapLayers();
+}
+
+void TestQgsAppLocatorFilters::testActiveLayerFieldRestriction()
+{
+  QString search = QStringLiteral( "@my_field search" );
+  QString restr = QgsActiveLayerFeaturesLocatorFilter::fieldRestriction( search );
+  QCOMPARE( restr, QStringLiteral( "my_field" ) );
+  QCOMPARE( search, QStringLiteral( "search" ) );
+
+  search = QStringLiteral( "@home" );
+  restr = QgsActiveLayerFeaturesLocatorFilter::fieldRestriction( search );
+  QCOMPARE( restr, QStringLiteral( "home" ) );
+  QCOMPARE( search, QStringLiteral( "" ) );
+}
+
+void TestQgsAppLocatorFilters::testActiveLayerCompletion()
+{
+  QString layerDef = QStringLiteral( "Point?crs=epsg:4326&field=pk:integer&field=my_text:string&field=my_integer:integer&field=my_double:double&key=pk" );
+  QgsVectorLayer *vl = new QgsVectorLayer( layerDef, QStringLiteral( "Layer" ), QStringLiteral( "memory" ) );
+  QgsProject::instance()->addMapLayer( vl );
+  mQgisApp->setActiveLayer( vl );
+
+  QgsFeedback f;
+  QgsActiveLayerFeaturesLocatorFilter filter;
+  QgsLocatorContext context;
+  context.usingPrefix = true;
+
+  QCOMPARE( filter.prepare( QStringLiteral( "" ), context ), QStringList( { "@pk ", "@my_text ", "@my_integer ", "@my_double " } ) );
+  QCOMPARE( filter.prepare( QStringLiteral( "@my_i" ), context ), QStringList( { "@my_integer " } ) );
+
   QgsProject::instance()->removeAllMapLayers();
 }