Address comments from Nyall's review

qgis · Apr 15, 2020 · ba7b785 · ba7b785
1 parent 59c1ac3
commit ba7b785
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 6 deletions.
diff --git a/src/core/vectortile/qgsvectortileutils.cpp b/src/core/vectortile/qgsvectortileutils.cpp
@@ -53,7 +53,7 @@ QgsFields QgsVectorTileUtils::makeQgisFields( QSet<QString> flds )
   QgsFields fields;
   QStringList fieldsSorted = flds.toList();
   std::sort( fieldsSorted.begin(), fieldsSorted.end() );
-  for ( QString fieldName : qgis::as_const( fieldsSorted ) )
+  for ( const QString &fieldName : qgis::as_const( fieldsSorted ) )
   {
     fields.append( QgsField( fieldName, QVariant::String ) );
   }

diff --git a/src/gui/qgsmaptoolidentify.cpp b/src/gui/qgsmaptoolidentify.cpp
@@ -329,11 +329,11 @@ bool QgsMapToolIdentify::identifyVectorTileLayer( QList<QgsMapToolIdentify::Iden
 
   if ( !layer->isInScaleRange( mCanvas->mapSettings().scale() ) )
   {
-    QgsDebugMsg( QStringLiteral( "Out of scale limits" ) );
+    QgsDebugMsgLevel( QStringLiteral( "Out of scale limits" ), 2 );
     return false;
   }
 
-  QApplication::setOverrideCursor( Qt::WaitCursor );
+  QgsTemporaryCursorOverride waitCursor( Qt::WaitCursor );
 
   QMap< QString, QString > commonDerivedAttributes;
 
@@ -403,14 +403,16 @@ bool QgsMapToolIdentify::identifyVectorTileLayer( QList<QgsMapToolIdentify::Iden
           continue;  // failed to decode
 
         QMap<QString, QgsFields> perLayerFields;
-        for ( QString layerName : decoder.layers() )
+        const QStringList layerNames = decoder.layers();
+        for ( const QString &layerName : layerNames )
         {
           QSet<QString> fieldNames = QSet<QString>::fromList( decoder.layerFieldNames( layerName ) );
           perLayerFields[layerName] = QgsVectorTileUtils::makeQgisFields( fieldNames );
         }
 
         const QgsVectorTileFeatures features = decoder.layerFeatures( perLayerFields, QgsCoordinateTransform() );
-        for ( QString layerName : features.keys() )
+        const QStringList featuresLayerNames = features.keys();
+        for ( const QString &layerName : featuresLayerNames )
         {
           const QgsFields fFields = perLayerFields[layerName];
           const QVector<QgsFeature> &layerFeatures = features[layerName];
@@ -438,7 +440,6 @@ bool QgsMapToolIdentify::identifyVectorTileLayer( QList<QgsMapToolIdentify::Iden
     QgsDebugMsg( QStringLiteral( "Caught CRS exception %1" ).arg( cse.what() ) );
   }
 
-  QApplication::restoreOverrideCursor();
   return featureCount > 0;
 }
 

diff --git a/tests/src/app/CMakeLists.txt b/tests/src/app/CMakeLists.txt
@@ -19,6 +19,7 @@ INCLUDE_DIRECTORIES(
   ${CMAKE_SOURCE_DIR}/src/core/symbology
   ${CMAKE_SOURCE_DIR}/src/core/effects
   ${CMAKE_SOURCE_DIR}/src/core/validity
+  ${CMAKE_SOURCE_DIR}/src/core/vectortile
   ${CMAKE_SOURCE_DIR}/src/ui
   ${CMAKE_SOURCE_DIR}/src/gui
   ${CMAKE_SOURCE_DIR}/src/gui/editorwidgets

diff --git a/tests/src/app/testqgsmaptoolidentifyaction.cpp b/tests/src/app/testqgsmaptoolidentifyaction.cpp
@@ -21,6 +21,7 @@
 #include "qgsfeature.h"
 #include "qgsgeometry.h"
 #include "qgsvectordataprovider.h"
+#include "qgsvectortilelayer.h"
 #include "qgsproject.h"
 #include "qgsmapcanvas.h"
 #include "qgsmeshlayer.h"
@@ -56,6 +57,7 @@ class TestQgsMapToolIdentifyAction : public QObject
     void identifyRasterFloat32(); // test pixel identification and decimal precision
     void identifyRasterFloat64(); // test pixel identification and decimal precision
     void identifyMesh(); // test identification for mesh layer
+    void identifyVectorTile();  // test identification for vector tile layer
     void identifyInvalidPolygons(); // test selecting invalid polygons
     void clickxy(); // test if clicked_x and clicked_y variables are propagated
     void closestPoint();
@@ -70,6 +72,7 @@ class TestQgsMapToolIdentifyAction : public QObject
     QString testIdentifyRaster( QgsRasterLayer *layer, double xGeoref, double yGeoref );
     QList<QgsMapToolIdentify::IdentifyResult> testIdentifyVector( QgsVectorLayer *layer, double xGeoref, double yGeoref );
     QList<QgsMapToolIdentify::IdentifyResult> testIdentifyMesh( QgsMeshLayer *layer, double xGeoref, double yGeoref );
+    QList<QgsMapToolIdentify::IdentifyResult> testIdentifyVectorTile( QgsVectorTileLayer *layer, double xGeoref, double yGeoref );
 
     // Release return with delete []
     unsigned char *
@@ -540,6 +543,16 @@ TestQgsMapToolIdentifyAction::testIdentifyVector( QgsVectorLayer *layer, double
   return result;
 }
 
+// private
+QList<QgsMapToolIdentify::IdentifyResult>
+TestQgsMapToolIdentifyAction::testIdentifyVectorTile( QgsVectorTileLayer *layer, double xGeoref, double yGeoref )
+{
+  std::unique_ptr< QgsMapToolIdentifyAction > action( new QgsMapToolIdentifyAction( canvas ) );
+  QgsPointXY mapPoint = canvas->getCoordinateTransform()->transform( xGeoref, yGeoref );
+  QList<QgsMapToolIdentify::IdentifyResult> result = action->identify( mapPoint.x(), mapPoint.y(), QList<QgsMapLayer *>() << layer );
+  return result;
+}
+
 void TestQgsMapToolIdentifyAction::identifyRasterFloat32()
 {
   //create a temporary layer
@@ -663,6 +676,33 @@ void TestQgsMapToolIdentifyAction::identifyMesh()
   QCOMPARE( results[0].mAttributes[ QStringLiteral( "Vector y-component" )], QStringLiteral( "2.4" ) );
 }
 
+void TestQgsMapToolIdentifyAction::identifyVectorTile()
+{
+  //create a temporary layer
+  QgsDataSourceUri dsUri;
+  dsUri.setParam( QStringLiteral( "type" ), QStringLiteral( "xyz" ) );
+  dsUri.setParam( QStringLiteral( "url" ), QStringLiteral( "file://%1/vector_tile/{z}-{x}-{y}.pbf" ).arg( QStringLiteral( TEST_DATA_DIR ) ) );
+  QgsVectorTileLayer *tempLayer = new QgsVectorTileLayer( dsUri.encodedUri(), QStringLiteral( "testlayer" ) );
+  QVERIFY( tempLayer->isValid() );
+
+  QgsCoordinateReferenceSystem srs( QStringLiteral( "EPSG:3857" ) );
+  canvas->setDestinationCrs( srs );
+  canvas->setExtent( tempLayer->extent() );
+  canvas->setLayers( QList<QgsMapLayer *>() << tempLayer );
+  canvas->setCurrentLayer( tempLayer );
+
+  QList<QgsMapToolIdentify::IdentifyResult> results;
+  results = testIdentifyVectorTile( tempLayer, 15186127, -2974969 );
+  QCOMPARE( results.size(), 1 );
+  QCOMPARE( results[0].mLayer, tempLayer );
+  QCOMPARE( results[0].mLabel, QStringLiteral( "place" ) );
+  QCOMPARE( results[0].mFeature.geometry().wkbType(), QgsWkbTypes::Point );
+  QCOMPARE( results[0].mFeature.attribute( QStringLiteral( "class" ) ).toString(), QStringLiteral( "country" ) );
+  QCOMPARE( results[0].mFeature.attribute( QStringLiteral( "name" ) ).toString(), QStringLiteral( "Australia" ) );
+
+  delete tempLayer;
+}
+
 void TestQgsMapToolIdentifyAction::identifyInvalidPolygons()
 {
   //create a temporary layer