From e79a327ab7f0d7a3e91132cf53c467bd8389a480 Mon Sep 17 00:00:00 2001 From: Patrick Valsecchi Date: Fri, 13 May 2016 15:08:30 +0200 Subject: [PATCH 1/2] WMS GetCapabilities: override parent's style if they have the same name When there is a layer group with several sub-layers, the group has a "default" style and the sub-layers each have a "default" layer. QGIS was showing two "default" styles for the sub-layers, which would be confusing to the user and could pick the wrong legend for the sub-layer if the user picked the wrong entry (the first one). Had to create a static lib for wmsprovider in order to unittest it. --- src/providers/wms/CMakeLists.txt | 6 + src/providers/wms/qgswmscapabilities.cpp | 11 + tests/src/providers/CMakeLists.txt | 5 + .../src/providers/testqgswmscapabilities.cpp | 70 +++++++ tests/testdata/provider/GetCapabilities.xml | 188 ++++++++++++++++++ 5 files changed, 280 insertions(+) create mode 100644 tests/src/providers/testqgswmscapabilities.cpp create mode 100644 tests/testdata/provider/GetCapabilities.xml diff --git a/src/providers/wms/CMakeLists.txt b/src/providers/wms/CMakeLists.txt index 310d566cea4..bc19de46db6 100644 --- a/src/providers/wms/CMakeLists.txt +++ b/src/providers/wms/CMakeLists.txt @@ -41,6 +41,7 @@ INCLUDE_DIRECTORIES(SYSTEM ${QCA_INCLUDE_DIR} ) +ADD_LIBRARY(wmsprovider_a STATIC ${WMS_SRCS} ${WMS_MOC_SRCS}) ADD_LIBRARY(wmsprovider MODULE ${WMS_SRCS} ${WMS_MOC_SRCS}) TARGET_LINK_LIBRARIES(wmsprovider @@ -50,6 +51,11 @@ TARGET_LINK_LIBRARIES(wmsprovider ${GDAL_LIBRARY} # for OGR_G_CreateGeometryFromJson() ) +TARGET_LINK_LIBRARIES(wmsprovider_a + qgis_core + ${QT_QTSCRIPT_LIBRARY} +) + INSTALL (TARGETS wmsprovider RUNTIME DESTINATION ${QGIS_PLUGIN_DIR} LIBRARY DESTINATION ${QGIS_PLUGIN_DIR}) diff --git a/src/providers/wms/qgswmscapabilities.cpp b/src/providers/wms/qgswmscapabilities.cpp index 9a46ae8b07f..72ab68cad3b 100644 --- a/src/providers/wms/qgswmscapabilities.cpp +++ b/src/providers/wms/qgswmscapabilities.cpp @@ -905,6 +905,17 @@ void QgsWmsCapabilities::parseLayer( QDomElement const & e, QgsWmsLayerProperty& parseStyle( e1, styleProperty ); + for ( int i = 0; i < layerProperty.style.size(); ++i ) + { + if ( layerProperty.style[i].name == styleProperty.name ) + { + // override inherited parent's style if it has the same name + // according to the WMS spec, it should not happen, but Mapserver + // does it all the time. + layerProperty.style.remove( i ); + break; + } + } layerProperty.style.push_back( styleProperty ); } else if ( tagName == "MinScaleDenominator" ) diff --git a/tests/src/providers/CMakeLists.txt b/tests/src/providers/CMakeLists.txt index badad7327f5..c260df4429f 100644 --- a/tests/src/providers/CMakeLists.txt +++ b/tests/src/providers/CMakeLists.txt @@ -9,6 +9,7 @@ INCLUDE_DIRECTORIES(${CMAKE_CURRENT_SOURCE_DIR} ${CMAKE_SOURCE_DIR}/src/core/auth ${CMAKE_SOURCE_DIR}/src/core/geometry ${CMAKE_SOURCE_DIR}/src/core/raster + ${CMAKE_SOURCE_DIR}/src/providers/wms ) INCLUDE_DIRECTORIES(SYSTEM ${QT_INCLUDE_DIR} @@ -84,6 +85,10 @@ SET_TARGET_PROPERTIES(qgis_wcsprovidertest PROPERTIES ADD_QGIS_TEST(gdalprovidertest testqgsgdalprovider.cpp) +ADD_QGIS_TEST(wmscapabilititestest + testqgswmscapabilities.cpp) +TARGET_LINK_LIBRARIES(qgis_wmscapabilititestest wmsprovider_a) + ############################################################# # WCS public servers test: # No need to test on all platforms diff --git a/tests/src/providers/testqgswmscapabilities.cpp b/tests/src/providers/testqgswmscapabilities.cpp new file mode 100644 index 00000000000..686c30abd70 --- /dev/null +++ b/tests/src/providers/testqgswmscapabilities.cpp @@ -0,0 +1,70 @@ +#include +#include +#include +#include +#include + +/** \ingroup UnitTests + * This is a unit test for the WMS capabilities parser. + */ +class TestQgsWmsCapabilities: public QObject +{ + Q_OBJECT + private slots: + + void initTestCase() + { + // init QGIS's paths - true means that all path will be inited from prefix + QgsApplication::init(); + QgsApplication::initQgis(); + } + + //runs after all tests + void cleanupTestCase() + { + QgsApplication::exitQgis(); + } + + + void read() + { + QgsWmsCapabilities capabilities; + + QFile file( QString( TEST_DATA_DIR ) + "/provider/GetCapabilities.xml" ); + QVERIFY( file.open( QIODevice::ReadOnly | QIODevice::Text ) ); + const QByteArray content = file.readAll(); + QVERIFY( content.size() > 0 ); + const QgsWmsParserSettings config; + + QVERIFY( capabilities.parseResponse( content, config ) ); + QCOMPARE( capabilities.supportedLayers().size(), 5 ); + QCOMPARE( capabilities.supportedLayers()[0].name, QString( "agri_zones" ) ); + QCOMPARE( capabilities.supportedLayers()[1].name, QString( "buildings" ) ); + QCOMPARE( capabilities.supportedLayers()[2].name, QString( "land_surveing_parcels" ) ); + QCOMPARE( capabilities.supportedLayers()[3].name, QString( "cadastre" ) ); + QCOMPARE( capabilities.supportedLayers()[4].name, QString( "test" ) ); + + // make sure the default style is not seen twice in the child layers + QCOMPARE( capabilities.supportedLayers()[3].style.size(), 1 ); + QCOMPARE( capabilities.supportedLayers()[3].style[0].name, QString( "default" ) ); + QCOMPARE( capabilities.supportedLayers()[1].style.size(), 1 ); + QCOMPARE( capabilities.supportedLayers()[1].style[0].name, QString( "default" ) ); + QCOMPARE( capabilities.supportedLayers()[2].style.size(), 1 ); + QCOMPARE( capabilities.supportedLayers()[2].style[0].name, QString( "default" ) ); + + // check it can read 2 styles for a layer and that the legend URL is OK + QCOMPARE( capabilities.supportedLayers()[0].style.size(), 2 ); + QCOMPARE( capabilities.supportedLayers()[0].style[0].name, QString( "yt_style" ) ); + QCOMPARE( capabilities.supportedLayers()[0].style[0].legendUrl.size(), 1 ); + QCOMPARE( capabilities.supportedLayers()[0].style[0].legendUrl[0].onlineResource.xlinkHref, + QString( "http://www.example.com/yt.png" ) ); + QCOMPARE( capabilities.supportedLayers()[0].style[1].name, QString( "fb_style" ) ); + QCOMPARE( capabilities.supportedLayers()[0].style[1].legendUrl.size(), 1 ); + QCOMPARE( capabilities.supportedLayers()[0].style[1].legendUrl[0].onlineResource.xlinkHref, + QString( "http://www.example.com/fb.png" ) ); + } + +}; + +QTEST_MAIN( TestQgsWmsCapabilities ) +#include "testqgswmscapabilities.moc" diff --git a/tests/testdata/provider/GetCapabilities.xml b/tests/testdata/provider/GetCapabilities.xml new file mode 100644 index 00000000000..ed593d87a61 --- /dev/null +++ b/tests/testdata/provider/GetCapabilities.xml @@ -0,0 +1,188 @@ + + + + + WMS + Test + Test + + + + 5000 + 5000 + + + + + + text/xml + + + + + + + + + image/jpeg + image/png + + + + + + + + + text/plain + application/vnd.ogc.gml + + + + + + + + + text/xml + + + + + + + + + image/jpeg + image/png + image/png; mode=8bit + + + + + + + + + text/xml + + + + + + + + + + XML + BLANK + + + + test + Test + Test + EPSG:2056 + + 5.01393 + 11.4774 + 45.356 + 48.3001 + + + + agri_zones + agri_zones + EPSG:2056 + + 5.01393 + 11.4774 + 45.356 + 48.3001 + + + + text/html + + + + + + + cadastre + cadastre + cadastre + + + buildings + buildings + EPSG:2056 + + 5.01393 + 11.4774 + 45.356 + 48.3001 + + + + text/html + + + + + + land_surveing_parcels + land_surveing_parcels + EPSG:2056 + + 5.01393 + 11.4774 + 45.356 + 48.3001 + + + + text/html + + + + + + + + \ No newline at end of file From 69bed218373b3f93671f65bc3d02c45cbf683a48 Mon Sep 17 00:00:00 2001 From: Patrick Valsecchi Date: Fri, 13 May 2016 15:13:58 +0200 Subject: [PATCH 2/2] WMS: Better logic to pick the legend URL QGIS had two problems: 1) It was using the specified legend URL only if its mime type was matching the layer's mime type. There is no reason for that. 2) When QGIS was using the default layer (empty string), it was not even trying to find out in what style to pick the legend URL. --- src/providers/wms/qgswmscapabilities.cpp | 2 +- src/providers/wms/qgswmsprovider.cpp | 68 ++++++++++++++++----- tests/src/providers/CMakeLists.txt | 4 ++ tests/src/providers/testqgswmsprovider.cpp | 69 ++++++++++++++++++++++ 4 files changed, 126 insertions(+), 17 deletions(-) create mode 100644 tests/src/providers/testqgswmsprovider.cpp diff --git a/src/providers/wms/qgswmscapabilities.cpp b/src/providers/wms/qgswmscapabilities.cpp index 72ab68cad3b..14d8bbb11db 100644 --- a/src/providers/wms/qgswmscapabilities.cpp +++ b/src/providers/wms/qgswmscapabilities.cpp @@ -907,7 +907,7 @@ void QgsWmsCapabilities::parseLayer( QDomElement const & e, QgsWmsLayerProperty& for ( int i = 0; i < layerProperty.style.size(); ++i ) { - if ( layerProperty.style[i].name == styleProperty.name ) + if ( layerProperty.style.at( i ).name == styleProperty.name ) { // override inherited parent's style if it has the same name // according to the WMS spec, it should not happen, but Mapserver diff --git a/src/providers/wms/qgswmsprovider.cpp b/src/providers/wms/qgswmsprovider.cpp index be006a7260b..f59b1fb62ee 100644 --- a/src/providers/wms/qgswmsprovider.cpp +++ b/src/providers/wms/qgswmsprovider.cpp @@ -215,6 +215,36 @@ QString QgsWmsProvider::getTileUrl() const } } +static bool isValidLegend( const QgsWmsLegendUrlProperty &l ) +{ + return l.format.startsWith( "image/" ); +} + +/** + * Picks a usable legend URL for a given style. + */ +static QString pickLegend( const QgsWmsStyleProperty &s ) +{ + QString url; + for ( int k = 0; k < s.legendUrl.size() && url.isEmpty(); k++ ) + { + const QgsWmsLegendUrlProperty &l = s.legendUrl[k]; + if ( isValidLegend( l ) ) + { + url = l.onlineResource.xlinkHref; + } + } + return url; +} + +static const QgsWmsStyleProperty *searchStyle( const QVector& styles, const QString& name ) +{ + Q_FOREACH ( const QgsWmsStyleProperty &s, styles ) + if ( s.name == name ) + return &s; + return nullptr; +} + QString QgsWmsProvider::getLegendGraphicUrl() const { QString url; @@ -223,25 +253,31 @@ QString QgsWmsProvider::getLegendGraphicUrl() const { const QgsWmsLayerProperty &l = mCaps.mLayersSupported[i]; - if ( l.name != mSettings.mActiveSubLayers[0] ) - continue; - - for ( int j = 0; j < l.style.size() && url.isEmpty(); j++ ) + if ( l.name == mSettings.mActiveSubLayers[0] ) { - const QgsWmsStyleProperty &s = l.style[j]; - - if ( s.name != mSettings.mActiveSubStyles[0] ) - continue; - - for ( int k = 0; k < s.legendUrl.size() && url.isEmpty(); k++ ) + if ( !mSettings.mActiveSubStyles[0].isEmpty() && mSettings.mActiveSubStyles[0] != "default" ) { - const QgsWmsLegendUrlProperty &l = s.legendUrl[k]; - - if ( l.format != mSettings.mImageMimeType ) - continue; - - url = l.onlineResource.xlinkHref; + const QgsWmsStyleProperty *s = searchStyle( l.style, mSettings.mActiveSubStyles[0] ); + if ( s ) + url = pickLegend( *s ); } + else + { + // QGIS wants the default style, but GetCapabilities doesn't give us a + // way to know what is the default style. So we look for the onlineResource + // only if there is a single style available or if there is a style called "default". + if ( l.style.size() == 1 ) + { + url = pickLegend( l.style[0] ); + } + else + { + const QgsWmsStyleProperty *s = searchStyle( l.style, "default" ); + if ( s ) + url = pickLegend( *s ); + } + } + break; } } diff --git a/tests/src/providers/CMakeLists.txt b/tests/src/providers/CMakeLists.txt index c260df4429f..7e8b2e9ba0e 100644 --- a/tests/src/providers/CMakeLists.txt +++ b/tests/src/providers/CMakeLists.txt @@ -89,6 +89,10 @@ ADD_QGIS_TEST(wmscapabilititestest testqgswmscapabilities.cpp) TARGET_LINK_LIBRARIES(qgis_wmscapabilititestest wmsprovider_a) +ADD_QGIS_TEST(wmsprovidertest + testqgswmsprovider.cpp) +TARGET_LINK_LIBRARIES(qgis_wmsprovidertest wmsprovider_a) + ############################################################# # WCS public servers test: # No need to test on all platforms diff --git a/tests/src/providers/testqgswmsprovider.cpp b/tests/src/providers/testqgswmsprovider.cpp new file mode 100644 index 00000000000..1f55efd9ac3 --- /dev/null +++ b/tests/src/providers/testqgswmsprovider.cpp @@ -0,0 +1,69 @@ +#include +#include +#include +#include +#include + +/** \ingroup UnitTests + * This is a unit test for the WMS provider. + */ +class TestQgsWmsProvider: public QObject +{ + Q_OBJECT + private slots: + + void initTestCase() + { + // init QGIS's paths - true means that all path will be inited from prefix + QgsApplication::init(); + QgsApplication::initQgis(); + + QFile file( QString( TEST_DATA_DIR ) + "/provider/GetCapabilities.xml" ); + QVERIFY( file.open( QIODevice::ReadOnly | QIODevice::Text ) ); + const QByteArray content = file.readAll(); + QVERIFY( content.size() > 0 ); + const QgsWmsParserSettings config; + + mCapabilities = new QgsWmsCapabilities(); + QVERIFY( mCapabilities->parseResponse( content, config ) ); + } + + //runs after all tests + void cleanupTestCase() + { + delete mCapabilities; + QgsApplication::exitQgis(); + } + + void legendGraphicsWithStyle() + { + QgsWmsProvider provider( "http://localhost:8380/mapserv?xxx&layers=agri_zones&styles=fb_style&format=image/jpg", mCapabilities ); + QCOMPARE( provider.getLegendGraphicUrl(), QString( "http://www.example.com/fb.png?" ) ); + } + + void legendGraphicsWithSecondStyle() + { + QgsWmsProvider provider( "http://localhost:8380/mapserv?xxx&layers=agri_zones&styles=yt_style&format=image/jpg", mCapabilities ); + QCOMPARE( provider.getLegendGraphicUrl(), QString( "http://www.example.com/yt.png?" ) ); + } + + void legendGraphicsWithoutStyleWithDefault() + { + QgsWmsProvider provider( "http://localhost:8380/mapserv?xxx&layers=buildings&styles=&format=image/jpg", mCapabilities ); + //only one style, can guess default => use it + QCOMPARE( provider.getLegendGraphicUrl(), QString( "http://www.example.com/buildings.png?" ) ); + } + + void legendGraphicsWithoutStyleWithoutDefault() + { + QgsWmsProvider provider( "http://localhost:8380/mapserv?xxx&layers=agri_zones&styles=&format=image/jpg", mCapabilities ); + //two style, cannot guess default => use the WMS GetLegendGraphics + QCOMPARE( provider.getLegendGraphicUrl(), QString( "http://localhost:8380/mapserv?" ) ); + } + + private: + QgsWmsCapabilities* mCapabilities; +}; + +QTEST_MAIN( TestQgsWmsProvider ) +#include "testqgswmsprovider.moc"