LayerGroup content synchronization #88

petrsloup · 2014-09-29T12:35:05Z

This PR ensures that changes inside LayerGroups (add/remove layer) are synchronized.

Closes #27.
Closes #38.

ahocevar · 2014-09-30T09:02:48Z

Since this is handled in the raster synchronizer, will that also work for vector layers? To me it seems it would be more logical if there was a group synchronizer, which would handle all layer types. Because groups can have vector and raster layers.

gberaudo · 2014-09-30T09:27:10Z

@petrsloup, with a hierarchy of layer groups aren't we receive a change event both on the child and on the father?
layergroup1 <- onchange: coming from layergroup2
layergroup2 <- onchange: initial event

@ahocevar, we would loose the flexibility to synchronize only one type of layers.

petrsloup · 2014-09-30T11:57:32Z

@gberaudo This is true, but only when the layergroup2 is visible: https://github.com/openlayers/ol3/blob/1c7f2ad5ccdc5961f212408c97c7fba8118cd86c/src/ol/layer/layergroup.js#L80. Since we don't synchronize the display properties (including visible), we need to always synchronize the group content and not rely on the event "bubbling" that might not happen.

@ahocevar I don't think the "group synchronizer" is necessary at the moment (and it would require large refactoring) -- I can add the same 3 lines of code to the vectorsynchronizer so the vector layers will stay synchronized as well. It could make sense to implemented some "group synchronizer" as part of the advanced layergroup synchronization (together with the synchronization of the display properties).

gberaudo · 2014-09-30T12:17:44Z

@petrsloup, interesting finding.

When layergroup2 is visible and gets changed, the change event will be
received for this layer so a synchronization of layergroup2 will
happen.

But, since the event will be dispatched to the parent layer, it
seems the layergroup1 will also be synchronized.

ahocevar · 2014-09-30T12:22:26Z

@petrsloup If vector layers can be kept in sync by adding the same 3 lines of code for vector layers, then I'm fine with it for now. Would you be working on this, or is it something for @gberaudo?

petrsloup · 2014-09-30T12:44:16Z

@gberaudo Yes, the worst-case scenario is that the synchronization is triggered multiple times (at most once per hierarchy level) if there are nested layergroups and a layer is added to/removed from the "deepest" layergroup.

@ahocevar I added it to this PR.

ahocevar · 2014-09-30T12:50:59Z

Looks good to me. Further improvements can be made later - I think this is good to merge.

gberaudo · 2014-09-30T13:06:32Z

@ahocevar, we cannot merge this PR knowing the above issue. We must find a real solution.

ahocevar · 2014-09-30T13:28:30Z

What about something like this:

var changing = false;
olLayer.on('change', function(e) {
  if (!changing) {
    changing = true;
    this.synchronize();
  }
  if (e.target === /* need reference to map */ map.getLayerGroup()) {
    changing = false;
  }
}, this);

gberaudo · 2014-09-30T13:42:20Z

Why not simply listen on add and remove events instead?

petrsloup · 2014-09-30T13:47:06Z

@ahocevar This would not work, because the event "propagation" can stop at any time and not reach the root.
It's also important to remember, that we currently skip the root layergroup a perform the synchronization on map.getLayers() collection.
One solution would be to add depth parameter to the synchronizeLayer function (which would be increased every time we recursively call synchronizeLayer), then we could do:

olLayer.on('change', function(e) {
  // if this is the last place where the change can be handled
  if (depth == 0 || !olLayer.getVisible()) {
    this.synchronize();
  }
}, this);

@gberaudo 'add' and 'remove' are events of the underlying ol.Collection. It would be possible to listen those, but we also need to handle the case when the whole collection is replaced via .setLayers so we would need to listen to change:layers as well (and create new 'add' and 'remove' listeners when it happens).

petrsloup · 2014-09-30T14:40:17Z

I just realized the solution drafted above won't work, because we actually need to determine the visibility of the parent group (if the group is hidden, it does not fire the event any more) which is not possible.
Instead of depth we could pass the parent group as a parameter to the synchronizeLayer. Then we could test parentLayer == map.getLayerGroup() || !parentLayer.getVisible().

Alternatively, we could listen add and remove on the collection and change:layers on the group (and re-listen add and remove when this happens).

@gberaudo @ahocevar What do you think -- is this solution acceptable? Any other ideas?

gberaudo · 2014-09-30T15:12:08Z

@petrsloup, I strongly prefer your second solution.
It is unfortunate that the ol.layer.Group.prototype.setLayers exist.
Changing the underlying collection of layers is a really strange thing to do.

… are added/removed

…rSynchronizer and VectorSynchronizer

klokan · 2014-09-30T17:29:56Z

@ahocevar how deep hierarchy of LayerGroups is the client using?

…event propagation issues)

petrsloup · 2014-09-30T17:38:45Z

Since the code duplicity introduced by this PR was getting larger and larger I moved the code shared between raster and vector synchronizer to the new olcs.AbstractSynchronizer -- there are still two independent instances of synchronizers, but the large code duplicity is removed.

This PR now implements the synchronization based on 'add', 'remove' and 'change:layers' events.

petrsloup · 2014-09-30T17:41:37Z

As a side-effect, I believe this PR now also fixes the issue described in #38 (comment).

gberaudo · 2014-10-01T08:13:46Z

src/abstractsynchronizer.js

+  this.map = map;
+
+  /**
+   * @type {?ol.View}


The ? is not necessary.
The convention in ol3 is to declare nullable object types without ?. It is closure default for objects.

gberaudo · 2014-10-01T08:41:35Z

src/abstractsynchronizer.js

+  }
+
+  this.destroyAll();
+  this.synchronize();


When the synchronize method has not been called yet by the app developer, we should prevent calling synchronize(). See https://github.com/boundlessgeo/ol3-cesium/pull/88/files#r18266082.

gberaudo · 2014-10-01T09:01:57Z

src/abstractsynchronizer.js

+    });
+
+    return;
+  } else if (!(olLayer instanceof ol.layer.Layer)) {


What about declaring the type of sources handled by the olcs.AbstractSynchronizer and use it here:
@template F,T and !(olLayer instanceof F)
Since the class is about the conversion of an Ol3 type into a Cesium type, it feels more natural to have both from and to types declared. Using F = ol.layer.Layer as the same effect as current code.

gberaudo · 2014-10-01T09:08:15Z

The introduction of an abstract base class would be a great addition, leading to more robust and readable code. Please see my comments.

petrsloup · 2014-10-01T09:13:56Z

Thank you for the comments, but most of them are proposing various improvements of the current synchronization mechanism -- this is not the point of this PR.
This PR implements layergroup content synchronization, which turned out to require code restructuring in order to avoid large code duplicity. I agree this will allow us to implement various enhancements more easily, but it should not be part of this PR, which is already getting quite large.

From my point of view, it makes sense to fix the code-style related comment (already done) and unlistening of the 'add' and 'remove' events when layergroup collection changes.

Other comments are out of scope of this PR.

gberaudo · 2014-10-01T09:29:08Z

src/vectorsynchronizer.js

-    goog.asserts.assert(!goog.isNull(view));
-    synchronizeLayer(el, view);
-  });
+  source.on('updatefeature', function(e) {


updatefeature has been renamed to changefeature.

Fixed, thanks

klokan · 2014-10-01T09:30:27Z

@gberaudo If fully agree with Petr - we go now for basic possible implementation - there is a ticket for Advanced LayerGroups sync #93 where your comments and more of the listeners can be implemented in future.

…r used or when collection is replaced

petrsloup · 2014-10-01T10:21:25Z

I added the required changes related to the LayerGroup synchronization.

petrsloup · 2014-10-01T12:48:39Z

@gberaudo Do you have any other comments regarding the LayerGroup synchronization or can this be merged? Synchronization improvements can be done later in a separate PR.

klokan · 2014-10-01T13:38:51Z

One more bug detected by C2C team (mentioned during the Hangout as "never removed listeners") - to be reported here and after it is fixed this can be merged.

elemoine · 2014-10-01T16:22:16Z

src/abstractsynchronizer.js

+      }, this);
+      listenAddRemove();
+
+      listenKeyArray.push(olLayer.on('change:layers', function(e) {


If a layer group is removed from the layer tree (using map.removeLayer(aLayerGroup)) what code is going to deregister the change:layers listener from that layer group and the add and remove listeners from that layer group's collection?

If a layer group is removed, it should fire remove event on some collection. This will result in this.synchronize(); call. The synchronizer keeps track of added event listeners and every time the synchronize is called, it detects which layer groups are no longer used and removes the listeners. (L153)

This has been tested on our side - the event listeners are removed. Considered to be fixed.

If a layer group is removed, it should fire remove event on some collection. This will result in this.synchronize(); call. The synchronizer keeps track of added event listeners and every time the synchronize is called, it detects which layer groups are no longer used and removes the listeners. (L153)

What does look strange to me is that this.map.unByKey is used to deregister listeners that were registered on the collection or on the layer group. I don't get it.

@gberaudo reminded me that this.map.unByKey is actually just a wrapper to a static function. But it would have been clearer to use ol.Observable.prototype.unByKey rather than this.map.unByKey in the ol3cesium source code. And I hope we'll have ol.Observable.unByKey in the future: openlayers/openlayers#2794.

LayerGroup content synchronization

petrsloup added 4 commits September 30, 2014 19:05

Listen for "change" events on LayerGroups and synchronize when layers…

f246abb

… are added/removed

Handle the LayerGroup content change in the VectorSynchronizer as well

73bf781

Created AbstractSynchronizer to minimize code duplicity between Raste…

558b565

…rSynchronizer and VectorSynchronizer

Modified VectorSynchronizer to use AbstractSynchronizer

7324892

klokan mentioned this pull request Sep 30, 2014

LayerGroups - advanced sync with visibility & opacity #93

Open

Listen for 'add', 'remove' and 'change:layers' rather than 'change' (…

8629544

…event propagation issues)

gberaudo mentioned this pull request Oct 1, 2014

Handle view and layergroup change in vector synchronizer #94

Closed

gberaudo reviewed Oct 1, 2014
View reviewed changes

Minor code style modifications

26abc26

gberaudo reviewed Oct 1, 2014
View reviewed changes

Minor comment added

5e4f44d

gberaudo reviewed Oct 1, 2014
View reviewed changes

klokan mentioned this pull request Oct 1, 2014

LayerGroups basic raster sync #27

Closed

petrsloup added 3 commits October 1, 2014 11:52

Remember the group listening keys and unlisten when group is no longe…

cc76103

…r used or when collection is replaced

Minor merging error fix

38dfe0e

Do not use instances as object keys

a43bc55

klokan mentioned this pull request Oct 1, 2014

Illegal reference to layerGroup and view #38

Closed

ahocevar added this to the QA milestone Oct 1, 2014

elemoine reviewed Oct 1, 2014
View reviewed changes

Properly handle null layers in collections

3c1ef8d

klokan added a commit that referenced this pull request Oct 2, 2014

Merge pull request #88 from klokantech/layergroup-content-change

e3e0249

LayerGroup content synchronization

klokan merged commit e3e0249 into openlayers:master Oct 2, 2014

klokan mentioned this pull request Oct 2, 2014

Type checking errors when compiling app+ol3+ol3cesium #77

Closed

LayerGroup content synchronization #88

LayerGroup content synchronization #88

Uh oh!

Conversation

petrsloup commented Sep 29, 2014

Uh oh!

ahocevar commented Sep 30, 2014

Uh oh!

gberaudo commented Sep 30, 2014

Uh oh!

petrsloup commented Sep 30, 2014

Uh oh!

gberaudo commented Sep 30, 2014

Uh oh!

ahocevar commented Sep 30, 2014

Uh oh!

petrsloup commented Sep 30, 2014

Uh oh!

ahocevar commented Sep 30, 2014

Uh oh!

gberaudo commented Sep 30, 2014

Uh oh!

ahocevar commented Sep 30, 2014

Uh oh!

gberaudo commented Sep 30, 2014

Uh oh!

petrsloup commented Sep 30, 2014

Uh oh!

petrsloup commented Sep 30, 2014

Uh oh!

gberaudo commented Sep 30, 2014

Uh oh!

klokan commented Sep 30, 2014

Uh oh!

petrsloup commented Sep 30, 2014

Uh oh!

petrsloup commented Sep 30, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gberaudo commented Oct 1, 2014

Uh oh!

petrsloup commented Oct 1, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klokan commented Oct 1, 2014

Uh oh!

petrsloup commented Oct 1, 2014

Uh oh!

petrsloup commented Oct 1, 2014

Uh oh!

klokan commented Oct 1, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!