add basemap to qgs project #12

boney-bun · 2018-07-19T09:15:41Z

ismailsunni · 2018-07-19T09:41:30Z

filters/map_composition.py

+            # add basemap to the qgs project so it can be called later to create a thumbnail
+            qgis_layer = QgsRasterLayer('type=xyz&url=http://tile.osm.org/{z}/{x}/{y}.png?layers=osm', 'osm', 'wms')
+            if not qgis_layer.isValid():
+                print("osm not found")


Hindari print sih kalau bisa

Eh, avoid using print

Use Logger, because we can't see it if it is using print statement.

lucernae · 2018-07-19T09:49:15Z

filters/map_composition.py

-                # Add layer to the registry
-                QgsMapLayerRegistry.instance().addMapLayer(qgis_layer)
+            # add basemap to the qgs project so it can be called later to create a thumbnail
+            qgis_layer = QgsRasterLayer('type=xyz&url=http://tile.osm.org/{z}/{x}/{y}.png?layers=osm', 'osm', 'wms')


Make another parameter that accepts basemap urls/source.

boney-bun · 2018-07-19T16:34:42Z

@lucernae instead of using logger, i'll keep with the current implementation. i've updated the osm request so no more hardcoded.
@ismailsunni yes, it was intended for debuging, but forgot to remove them.

Gustry

This will add the OSM layer in the project. Are you sure it's what you want? (in WMS, QGS, ...)

Please also update the readme with this new parameter saying it's optional.

Gustry · 2018-07-19T16:39:33Z

filters/map_composition.py

@@ -46,7 +46,8 @@ def responseComplete(self):
        FILES=/path/1.shp;/path/2.shp;/path/3.asc&
        NAMES=Layer 1;Layer 2;Layer 3&
        REMOVEQML=true&
-        OVERWRITE=true
+        OVERWRITE=true&
+        BASEMAP=[<base map url>, <base map name>]


Like other parameters, you should put the example how to use it:

BASEMAP=http://foo.com/{z}/{x}/{y}.png;OSM

Gustry · 2018-07-19T16:39:36Z

filters/map_composition.py

+            if params.get('BASEMAP'):
+                # basemap is comprised of url and name with semicolon as separator
+                basemap = params.get('BASEMAP').split(';')
+                if len(basemap) > 1:


Gustry · 2018-07-19T16:39:56Z

filters/map_composition.py

+                if len(basemap) > 1:
+                    qgis_layer = QgsRasterLayer(basemap[0], basemap[1], 'wms')
+                    if not qgis_layer.isValid():
+                        request.appendBody('%s cannot found' % basemap[1])


cannot be found

gubuntu · 2018-07-19T17:19:27Z

@boney-bun why are you doing this? See my comment in the issue! Do not add anything to the QGIS project.

boney-bun · 2018-07-19T17:33:09Z

Hi @Gustry
thanks for the comments.
i've changed based on your input.

This will add the OSM layer in the project. Are you sure it's what you want? (in WMS, QGS, ...)

i've tried a few scenario in the geonode. it seems to be harmless.
imo, the osm layer will keep silent in qgs file. the layer will not be activated in the wms request as long as the request doesn't include osm in the layers parameter.
or, can you think of a harmful effect by adding the osm layer to the project?

@gubuntu this PR is to overcome the technical challenge in the geonode and qgis-server perspective for kartoza/geonode#401.
i don't mind to abandon this PR if we think it's not a good idea.

lucernae

Have you checked the layer order too?
Just make sure if the base layer were to be put last, then it is at the bottom of the layers when rendered.
If it is on top, it will cover all layers.

lucernae · 2018-07-31T07:56:26Z

README.md

@@ -14,6 +14,7 @@
  * NAMES, compulsory, it's a list of names, separated by a semicolon. It will be used for the legend. Items in this list should match layers in the FILES list.
  * OVERWRITE, optional, false by default. Boolean if we can overwrite the existing PROJECT above. Values can be '1', 'YES', 'TRUE', 'yes', 'true'.
  * REMOVEQML, optional, false by default. Boolean if we can remove the QML. The style is already in the QGS file. Values can be '1', 'YES', 'TRUE', 'yes', 'true'.
+  * BASEMAP, optional, None by default. it's a list of string comprised of a tile url and its service name, separated by a semicolon.


Give example of values here.

For me tile url like mentioned, it's what we get on geonode or a normal TILE url such as: http://tile.osm.org/{z}/{x}/{y}.png but here, you want a URI according to QGIS specifications (with type=)

lucernae · 2018-07-31T08:17:50Z

or, can you think of a harmful effect by adding the osm layer to the project?

Well, I think @Gustry 's concern is similar to what I told you previously. If you put a basemap, does the basemap gets accidentally rendered in WMS tile request from map client? You can check this by requesting the tile via WMS after creating a QGS project with basemap. If it is ok, you can proceed. All of our docker build for QGIS Server includes otf-project and it is possible that it was used on another project, so we have to make sure it doesn't have any unexpected side effects.

This will achieve:

Generating thumbnail with basemap can now be requested by WMS request with optional basemap name in LAYERS parameter
If somehow in the future we want to make "download qgs project" functionality in GeoNode, the basemap is already there. Although it doesn't seem to be possible at the moment, because all the other layers were using local file connections.

From @gubuntu 's comment in the issue:

map thumbnails alreday flatten multiple layers into the thumbnail png so it's just a matter of adding a basemap response to the flattening process (or are map layers returned in one already-flattened response by QGIS?)

We did this by putting all the layer's connections in the QGIS project file. If we want to include the basemap, we have to put it in QGIS project file, in order for one WMS request to return all the layers including basemap as a map thumbnail.

gubuntu · 2018-07-31T12:58:40Z

since this is done it's worth testing so merge and deploy to testing.

If upstream accepts it, great!

Gustry · 2018-07-31T15:45:00Z

filters/map_composition.py

@@ -46,7 +46,8 @@ def responseComplete(self):
        FILES=/path/1.shp;/path/2.shp;/path/3.asc&
        NAMES=Layer 1;Layer 2;Layer 3&
        REMOVEQML=true&
-        OVERWRITE=true
+        OVERWRITE=true&
+        BASEMAP='type=xyz&url=http://tile.osm.org/{z}/{x}/{y}.png?layers=osm;osm'


I'm wondering what is the ?layers=osm for?

Need to add the simple quote too in the documentation

the function of layers=osm is to naming the layer as osm. so that we can refer to the layer name later.

Gustry

I'm just wondering, is there any pros for having a new parameter?
We already have FILES for path to layers and NAMES for the name of each path. Now we are introducing a new one BASEMAP which include path and name.

Would it be difficult to use the same API?
FILES=type=xyz&url=http://foo/{z}{x}{y}.png;/path/1.shp;/path/2.shp;/path/3.asc&
NAMES=OSM;Layer 1;Layer 2;Layer 3&
NAMES=Layer 1;Layer 2;Layer 3&

Just a question, it looks a little tricky in my example.

boney-bun · 2018-07-31T16:42:28Z

I can see your point @Gustry
the main goal here is to add the basemap layer to the qgs.

i initially though that the BASEMAP parameter would be easier to understand when a new user read the doc.
the parameter (hopefully) can show that we would like to add a separate layer than the "normal" layers.

but, if we would like to avoid adding a new parameter, your idea work too.

boney-bun · 2018-07-31T17:00:21Z

@lucernae i'm not sure if i understand your comment correctly.

You can check this by requesting the tile via WMS after creating a QGS project with basemap.

does the steps on the GIF below is what you meant?
if so, I think the basemap doesn't have effect on the tile request.

lucernae · 2018-08-02T10:47:16Z

I'm just wondering, is there any pros for having a new parameter?
We already have FILES for path to layers and NAMES for the name of each path. Now we are introducing a new one BASEMAP which include path and name.

@Gustry, that makes sense. We can just use that, but maybe we need a different name since it's not just FILES anymore. Maybe SOURCES is a good idea, but we still handles FILES for legacy support. What do you think?

does the steps on the GIF below is what you meant?
if so, I think the basemap doesn't have effect on the tile request.

That's not what I meant, but I'll try that later tonight @boney. Sorry for the late reply.

- This will enable inserting Tile URL as basemap stored in QGIS Project - Update README

lucernae

@Gustry @boney-bun I made some changes. Need review and comment on this.

lucernae · 2018-08-03T01:45:16Z

filters/map_composition.py

+                # Overwrite means create from scratch again
+                remove(project_path)
+
+            sources_parameters = params.get('SOURCES')


I changed the params name because now it's not just files.
But we still support old name

lucernae · 2018-08-03T01:45:17Z

filters/map_composition.py

-                    return
-                else:
-                    remove(project_path)
+            if exists(project_path) and overwrite:


The changes now:

If overwrite is specified, then delete qgs project file and create new later

If project exists and not overwrite, then update layer registry

lucernae · 2018-08-03T01:46:41Z

filters/map_composition.py


            QgsMessageLog.logMessage('Setting up project to %s' % project_path)
            project = QgsProject.instance()
            project.setFileName(project_path)
+            if exists(project_path) and not overwrite:


If project exists and not overwrite, then we read from current file first.

lucernae · 2018-08-03T01:46:47Z

filters/map_composition.py

+            # Add layer to the registry
+            if overwrite:
+                # Insert all new layers
+                QgsMapLayerRegistry.instance().addMapLayers(qgis_layers)


Just insert all of new layers, because the old file was deleted.

lucernae · 2018-08-03T01:47:16Z

filters/map_composition.py

+                # 2. Compare source, if it is the same, don't update
+                # 3. If it is a new name, add it
+                # 4. If same name but different source, then update
+


Maybe I miss something here, but this is the update rules.

Gustry · 2018-08-03T02:53:40Z

@Gustry, that makes sense. We can just use that, but maybe we need a different name since it's not just FILES anymore. Maybe SOURCES is a good idea, but we still handles FILES for legacy support. What do you think?

Sure, better SOURCES. Because we can later add PostGIS or other sources of layers (not only file based).

Just use SOURCES first, and if FILES is still used, you can add a warning in the output saying that is deprecated.

Il will review tomorrow

Gustry

Quick review tonight ;-)

Nice work

Gustry · 2018-08-03T02:55:14Z

README.md

@@ -10,7 +10,9 @@
 * Parameters : 
  * SERVICE=MAPCOMPOSITION, compulsory
  * PROJECT, compulsory, path where the project will be written on the file system.
-  * FILES, compulsory, it's a list of files on the filesystem, separated by a semicolon.
+  * SOURCES, compulsory, it's a list of layer sources. It can be tile url or QGIS DataSource URI or files on the filesystem, separated by a semicolon.
+  	Especially for QGIS DataSource URI, it must be url quoted twice (first the url, second the whole datasource string).


I would just move FILES here, at the end of SOURCES.

Previously FILES

Gustry · 2018-08-03T02:56:02Z

README.md

+OVERWRITE=true
+```
+
+In the sample request above, note that the datasource: ```type%253Dxyz%2526url%253Dhttp%25253A%2F%2Fa.tile.osm.org%2F%25257Bz%25257D%2F%25257Bx%25257D%2F%25257By%25257D.png``` were urlquoted twice.


You could use single quote for inline code ;-)
Nothing to update

Hahaha... my habit, I guess.

boney-bun

LGTM.
i have only one comment regarding a mere coding style.

boney-bun · 2018-08-03T03:28:37Z

filters/map_composition.py

-            files_parameters = params.get('FILES')
-            if not files_parameters:
-                request.appendBody('FILES parameter is missing.\n')
+            if not sources_parameters:


this is perhaps a matter of coding style.
but sometimes it leads a new developer to believe that there's a repeated if condition in the second checks.
perhaps we need to add a comment here describing the business logic.
something like:
# if SOURCES and FILES don't exist, ask for providing SOURCES

Sure, I will add some comment.

lucernae · 2018-08-03T04:06:49Z

Thanks everyone

gubuntu · 2018-08-05T13:28:20Z

what ar you waiting for @boney-bun?

add basemap to qgs project

b90a7de

boney-bun mentioned this pull request Jul 19, 2018

add osm basemap to the thumbnail kartoza/geonode#446

Closed

ismailsunni reviewed Jul 19, 2018

View reviewed changes

lucernae reviewed Jul 19, 2018

View reviewed changes

revise PR after feedback

0879b66

Gustry reviewed Jul 19, 2018

View reviewed changes

revise based on Etienne's feedback

f9faedc

lucernae reviewed Jul 31, 2018

View reviewed changes

Gustry reviewed Jul 31, 2018

View reviewed changes

lucernae force-pushed the add-basemap-to-qgs-project branch from a8c1394 to e1a59ca Compare August 3, 2018 01:30

Support QGIS Datasource on Map Composition

ddf4b75

- This will enable inserting Tile URL as basemap stored in QGIS Project - Update README

lucernae force-pushed the add-basemap-to-qgs-project branch from e1a59ca to ddf4b75 Compare August 3, 2018 01:43

lucernae reviewed Aug 3, 2018

View reviewed changes

lucernae mentioned this pull request Aug 3, 2018

Add basemap to QGIS Project kartoza/geonode#465

Merged

Gustry reviewed Aug 3, 2018

View reviewed changes

boney-bun commented Aug 3, 2018

View reviewed changes

Improve documentation and comments

dea5b58

lucernae merged commit 7ab82a4 into kartoza:master Aug 6, 2018

add basemap to qgs project #12

add basemap to qgs project #12

Conversation

boney-bun commented Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boney-bun commented Jul 19, 2018

Gustry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gubuntu commented Jul 19, 2018

boney-bun commented Jul 19, 2018

lucernae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucernae commented Jul 31, 2018

gubuntu commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gustry left a comment

Choose a reason for hiding this comment

boney-bun commented Jul 31, 2018

boney-bun commented Jul 31, 2018

lucernae commented Aug 2, 2018

lucernae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gustry commented Aug 3, 2018

Gustry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gustry Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boney-bun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucernae commented Aug 3, 2018

gubuntu commented Aug 5, 2018

boney-bun commented Jul 19, 2018 •

edited

Loading

Gustry Aug 3, 2018 •

edited

Loading