Insert MIXED node layers as layer.Group in Map #1018

olivierSemet · 2016-04-12T11:47:50Z

fgravin · 2016-04-12T12:56:21Z

contribs/gmf/src/directives/layertree.js

@@ -273,14 +276,14 @@ gmf.LayertreeController.prototype.getLayerCaseMixedGroup_ = function(node) {
  for (i = 0; i < subNodes.length; i++) {
    subNode = subNodes[i];
    // Create all sublayers include wms layers;
-    layer = this.getLayer(subNode, 1, true);
+    layer = this.getLayer(subNode, 1, true, node);


I wonder if all of this is still usefull.
@ger-benjamin is it still usefull to add the Image layers to the group in one shot here, while we could add them when they come from the getLayers function ?

ger-benjamin · 2016-04-12T13:23:29Z

I think that that's not the right way.
Please wait before to continue.

... I write something...

fgravin · 2016-04-12T13:23:30Z

contribs/gmf/src/services/permalink.js

-    this.registerLayer_(layer, true);
+    if (layer instanceof ol.layer.Group) {
+      layer.getLayers().forEach(function(l) {
+        this.registerLayer_(l, true);


Good, but they are never unregistered
See handleLayersRemove_

ger-benjamin · 2016-04-12T13:36:14Z

I think that the getLayer function should be called only on real "depth 1". And we must export in another function all line after https://github.com/camptocamp/ngeo/blob/master/contribs/gmf/src/directives/layertree.js#L274

Then the function "getLayerCaseMixedGroup_" should call another function that create all child layers, set their visibility, params, etc, then put theme in a layergroup add this layer group in the map and return this group as layer. Perhaps all these operation must be done in a new function addLayerOnMap.

The function getLayerCaseNotMixedGroup_ should do the same but for not mixed group perhaps the current result of this one can use the function "addLayerOnMap".

In summary, there is no use now to complexifiy call 'manually' (in the gmf layertree) the gmf.getLayer function

So,

one function to "getLayers" (handle only groups, depth 1)
one function to add layers on map (called by getLayerCaseNotMixedGroup_ and getLayerCaseMixedGroup_)

olivierSemet · 2016-04-13T14:08:32Z

@ger-benjamin @adube @fgravin
Following your comments, I pushed a new version.

fgravin · 2016-04-13T15:07:12Z

contribs/gmf/src/directives/layertree.js

 * Create and return a layer corresponding to the ngeo layertree's node.
 * This function will only create a layer for each "top-level" (depth 1) groups.
 *
 * On "not mixed" type nodes, the returned layer will be an ol.layer.Image (WMS)
 * with each name of node's children as LAYERS parameters.
 *
- * On "mixed" type node, the returned  layer will be an ol.layer.Group with
- * a collection of layers that corresponds to each children of the node.
+ * On "mixed" type node, the returned  layer will be an ol.layer.Group


extra space

fgravin · 2016-04-13T15:12:27Z

Some minor comments but it looks more to what i expected. Good work thanks.

However, you did not manage correctly the permaling changes, read @adube comments cause you are not doing things properly.

Also, we must add more tests.

fgravin · 2016-04-18T09:51:17Z

Ok looks good.
I would like @adube to review and confirm that the permalink thing works.
Thanks

adube · 2016-04-18T12:44:37Z

contribs/gmf/src/directives/layertree.js

+gmf.LayertreeController.prototype.prepareLayer_ = function(node, layer) {
+  var type = gmf.Themes.getNodeType(node);
+  var ids = this.getNodeIds_(node);
+  if (goog.isDefAndNotNull(layer)) {


if the layer can be null or undefined, it should be defined as such in the param section above, as such:

@param {?ol.layer.Base=} opt_layer The...

Also, a truthy comparison should be enough: if (layer) {

adube · 2016-04-18T12:48:36Z

I added some minor comments and a major one.

Would it also be possible to have a github.io live example to be able to test it ?

olivierSemet · 2016-04-18T13:55:43Z

@adube check the example here

fgravin · 2016-04-18T17:04:59Z

So ?

adube · 2016-04-18T17:14:43Z

The recursion of addition / removal of layers doesn't seem to have been fixed yet, thus why I hadn't tested yet.

I did for what's currently available. The setting of properties work, but I can't fully test the example. When trying to reload the page, we get 404 errors.

The LayerTree example seems to be missing some CSS too: https://oliviersemet.github.io/ngeo/1011-insert-mixed-node-layer-as-layer-group-in-map/examples/contribs/gmf/layertree.html

fgravin · 2016-04-19T07:35:46Z

The setting of properties work, but I can't fully test the example. When trying to reload the page, we get 404 errors.

Yeah, you cannot reload apps, because the theme go into path and break the path. So you need to remove everything in path after desktop

adube · 2016-04-19T12:00:23Z

Something's wrong. I toggled off the visibility of all layers, created the url without the theme then used it. I still have one layer visible where it shouldn't be.

Here's the url you can test with: https://oliviersemet.github.io/ngeo/1011-insert-mixed-node-layer-as-layer-group-in-map/examples/contribs/gmf/apps/desktop/theme/OSM?lang=fr&baselayer_ref=map&tree_enable_osm_time=false&tree_enable_osm_scale=false&tree_enable_osm_open=false&tree_group_layers_Layers&tree_group_layers_Group

fgravin · 2016-04-19T12:02:25Z

@olivierSemet is writing tests. We must have tests that cover all of this.

olivierSemet · 2016-04-19T12:56:30Z

@adube the bug you are talking about is known and described at #1029. So this bug already exists in the master branch. We should fix it but IMO this is out of the scope of this PR.

olivierSemet · 2016-04-19T13:05:06Z

@adube, @fgravin I pushed a new version which should fix the group recursion problem. I also provided a new unit test file for the permalink service. It tests the recursion.

adube · 2016-04-19T13:21:36Z

contribs/gmf/src/services/permalink.js

+      this.registerLayer_(layer, opt_init);
+    }, this)
+  }
+


When a layer is ol.layer.Group, the code carries on after looping inside its layers, which means that events are registered for the ol.layer.Group. Is this needed ?

Changing visibility at group level is possible so I would say yes.

Ok @adube, I talked with @fgravin about it. You're right, there is no need to put a listener on ol.layer.Group objects. I'm going to fix it on a new PR right now. I'll let you know when it's done.

Understood.

fgravin · 2016-04-19T13:30:46Z

I closely supervised the gmf layer tree refining. I think it is a very good improvment, thanks @olivierSemet
Hopefully it won't break anything.

Very important to add test, really appreciable.
Thanks for this good work.

fgravin reviewed Apr 12, 2016
View reviewed changes

olivierSemet force-pushed the 1011-insert-mixed-node-layer-as-layer-group-in-map branch from 2ed880d to aaed0f4 Compare April 13, 2016 13:11

fgravin reviewed Apr 13, 2016
View reviewed changes

olivierSemet force-pushed the 1011-insert-mixed-node-layer-as-layer-group-in-map branch 4 times, most recently from ddae139 to 3b216e0 Compare April 14, 2016 13:42

adube reviewed Apr 18, 2016
View reviewed changes

Insert MIXED node layers as layer.Group in Map

bb67b26

olivierSemet force-pushed the 1011-insert-mixed-node-layer-as-layer-group-in-map branch from 3b216e0 to bb67b26 Compare April 19, 2016 12:59

adube reviewed Apr 19, 2016
View reviewed changes

fgravin merged commit 9a3869f into camptocamp:master Apr 19, 2016

olivierSemet mentioned this pull request Apr 19, 2016

Permalink service should not add listeners on ol.layer.Group #1045

Merged

olivierSemet deleted the 1011-insert-mixed-node-layer-as-layer-group-in-map branch April 20, 2016 11:14

sbrunner added this to the Older milestone Aug 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Insert MIXED node layers as layer.Group in Map #1018

Insert MIXED node layers as layer.Group in Map #1018

olivierSemet commented Apr 12, 2016

fgravin Apr 12, 2016

ger-benjamin commented Apr 12, 2016

fgravin Apr 12, 2016

ger-benjamin commented Apr 12, 2016

olivierSemet commented Apr 13, 2016

fgravin Apr 13, 2016

fgravin commented Apr 13, 2016

fgravin commented Apr 18, 2016

adube Apr 18, 2016

adube Apr 18, 2016

adube commented Apr 18, 2016

olivierSemet commented Apr 18, 2016

fgravin commented Apr 18, 2016

adube commented Apr 18, 2016

fgravin commented Apr 19, 2016

adube commented Apr 19, 2016

fgravin commented Apr 19, 2016

olivierSemet commented Apr 19, 2016

olivierSemet commented Apr 19, 2016

adube Apr 19, 2016

olivierSemet Apr 19, 2016

olivierSemet Apr 19, 2016 •

edited

adube Apr 19, 2016

fgravin commented Apr 19, 2016

Insert MIXED node layers as layer.Group in Map #1018

Insert MIXED node layers as layer.Group in Map #1018

Conversation

olivierSemet commented Apr 12, 2016

Choose a reason for hiding this comment

ger-benjamin commented Apr 12, 2016

Choose a reason for hiding this comment

ger-benjamin commented Apr 12, 2016

olivierSemet commented Apr 13, 2016

Choose a reason for hiding this comment

fgravin commented Apr 13, 2016

fgravin commented Apr 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adube commented Apr 18, 2016

olivierSemet commented Apr 18, 2016

fgravin commented Apr 18, 2016

adube commented Apr 18, 2016

fgravin commented Apr 19, 2016

adube commented Apr 19, 2016

fgravin commented Apr 19, 2016

olivierSemet commented Apr 19, 2016

olivierSemet commented Apr 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivierSemet Apr 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgravin commented Apr 19, 2016

olivierSemet Apr 19, 2016 •

edited