Select control #897

ahocevar · 2013-08-11T08:31:08Z

This pull request implements the Select control as described in https://github.com/openlayers/ol3/wiki/Vector-Editing#olcontrolselect. Button css is added in the example only for now, in anticipation with the ongoing control button refactoring that should simplify button css handling.

tschaub · 2013-08-15T21:02:38Z

Cool to see this in action @ahocevar. We've discussed this, but just to be clear here, this doesn't do anything with features on the existing layer (it doesn't remove them or suggest to the renderer that they be hidden). It just adds the same features to a new layer.

I still think it makes sense to clone the features before adding them to a new layer. The idea behind the render intent would be that it would be a styling hook. So we'd tell the original layer that the rendering intent for the selected features is "hidden" (or similar). And then after cloning the layer, we'd set the render intent to "select" and add them to the temp layer. This would allow the user to decide how selected features should be styled with filter syntax like this:

new ol.style.Style({rules: [
  new ol.style.Rule({
    filter: 'this.renderIntent == "selected"'
    symbolizers: [ /** symbolizers for selected style here */ ]
  })
]});

I can also imagine we might want to have one temp layer per original layer. This would make it easier to handle applications that let people do things like remove a layer or choose new layers for editing during an editing session. It would also allow the control to clone the original layer's style for the temp layer. This would mean people could just configure their styles in one place and add rules for styling selected or temporary features.

The feature cloning and one temp per original layer can change later - we just want to make sure they are considered before adding a way for people to style their selected features.

I'll take a closer look tomorrow, but I'm looking forward to having this functionality in.

elemoine · 2013-08-16T08:54:18Z

And then after cloning the layer,

Tim, did you mean "after cloning the feature"?

elemoine · 2013-08-16T09:17:42Z

Don't we want that the feature selection/deselection logic be implemented in an interaction rather in the control? With this the select control would just add the button, and add/remove the interaction when the button is clicked. This can probably be changed later, but I just wanted to know if it makes sense to people.

elemoine · 2013-08-16T09:18:59Z

src/objectliterals.jsdoc

@@ -337,6 +346,7 @@
 * @property {number|undefined} opacity Opacity. 0-1. Default is 1.
 * @property {ol.source.Source} source Source for this layer.
 * @property {ol.style.Style|undefined} style Style.
+ * @property {boolean|undefined} temp True for temporary layers. For internal use only.


Do we want to export that property if it's for internal use only? How about adding a private setTemporary method to ol.layer.Vector instead?

ahocevar · 2013-08-17T07:37:52Z

@elemoine If we wanted to split this into control and interaction, the control would have to be responsible for adding the interaction to the map, and would have to activate and deactivate the interaction. Is this what you're envisioning? I also think this can be changed later if we want to have this split.

ahocevar · 2013-08-17T07:39:38Z

@tschaub Thanks for your great suggestions. I see a way now how the mapping between features on the source layer and the target sketch layer can work efficiently. I'll add a commit. I also like the idea of having a separate sketch layer for each source layer and cloning styles of the original layer. I'll add a commit for that as well.

elemoine · 2013-08-17T07:43:59Z

Yes, this is what I envisioned. Adding a select control/toggle button should not be necessary to activate interactive feature selection. Also programmatically selecting features should be possible, eventually.

tschaub · 2013-08-17T13:27:06Z

As it is, you can activate the control programmatically. The "interaction" is just a click. What else were you envisioning that the interaction would do?

elemoine · 2013-08-17T19:17:50Z

ol2's select control doesn't add any DOM element. It just enables feature selection on click or drag-box. So I tend to think that his functionality should be implemented in an interaction in ol3 (for example, ol.interaction.ClickSelect and ol.interaction.DragSelect). The ol.control.Select control would add/remove the interaction when the toggle button is pressed/depressed. I haven't thought too much about it, so please tell me if that doesn't make sense to you.

ahocevar · 2013-08-20T21:30:57Z

@tschaub I implemented your suggestions, and also the renderIntent handling. I think this is in good shape now. You may want to continue/finish your review. Thanks!

ahocevar · 2013-08-21T07:44:26Z

@elemoine I spent a bit more time thinking about the control - interaction split. As far as I can see, interactions are designed to change the state of the map's view. The select control does not do that. It manages its own state by keeping track of selected/unselected features, and manages temporary layers to display them. A BoxSelect interaction would have to share the state with a ClickSelect interaction, which suggests that a third component is needed for the state and the selection layer management. And a fourth for providing the button if people want to use it as control. All this makes me think that for now we should stick with the control, and if we come up with something smart we can change it later. Having the events dispatched on the control is not a problem for now, because even if we decide to change that later, we can still relay the events on the control.

elemoine · 2013-08-21T08:24:26Z

@elemoine I spent a bit more time thinking about the control - interaction split. As far as I can see, interactions are designed to change the state of the map's view.

To me interactions just receive map browser events. They may perform any action. The interactions we currently have in master update the view, but I think we could have interactions that call map.getFeatures.

Also, interactions are organized in a prioritized array. This means that a "select interaction" could steal the map browser event, and prevent lower-priority interactions from getting that event.

A BoxSelect interaction would have to share the state with a ClickSelect interaction, which suggests that a third component is needed for the state

In ol2 the arrays of selected features are on the layers.

This makes me wonder: with the current ol.control.Select code, what do we do when a vector layer is removed? Should the control listen to "remove layer" events and clean up its selected features array?

All this makes me think that for now we should stick with the control

Yes, the control is a high-level component. If we agree on, and restrict, the control API we can later decompose the code into separate modules.

ahocevar · 2013-08-21T08:38:14Z

Thanks for your valuable comment @elemoine. You are right - managing addition and removal of layers is still a TODO. If we decide to handle feature selection on the layer, it needs to be done on ol.layer.Layer, not on ol.layer.Vector - because you can select features on all layers whose sources can provide access to the underlying features (e.g. WMS through WFS or GML-GetFeatureInfo).

elemoine · 2013-08-21T09:18:30Z

Some comments on the control API:

Currently an array of layers can be passed to the ol.control.Select constructor. The control keeps a reference to that array. As layers are added to/removed from the map, and made visible/invisible, the control and/or the control user need to update that array. The bookkeeping of this array can be tedious and error-prone.

What about working with a layer filter function instead? That function would take a layer as its argument and return a boolean value. For example, if you don't want invisible layers to participate in the layer selection you would do:

var control = new ol.control.Select({
  filter: function(layer) {
    return layer.getVisible();
  }
});

And with this, layers that are not in the map would automatically be excluded from the layer selection.

Thoughts?

schedul-xor · 2013-08-21T18:11:10Z

Your work is very cool, @ahocevar !

I tried this at my environment, and would like to add some comments.

Since the "selection" action is detected only when there is no dragging event between mousedown and mouseup, I fail to select once in three times when I click on a polygon (maybe I'm dragging few pixels from mousedown to mouseup). I suggest to add an "dragging disabled mode on/off" control to map, and discard mousemove events while dragging is disabled.
If you're looking forward to implementing "Point selection" (actually what I want it), nearest neighbor point searching function in R-Tree structure will be required (It is nearly impossible to let users click one single pixel). I've implemented the nearest neighbor point searching function in my own branch https://github.com/schedul-xor/ol3/tree/rtree . Currently, R-Tree structure is used only for vector feature caching, and it's property is private, so it is necessary to make it a public property.

ahocevar · 2013-08-21T22:12:17Z

@schedul-xor Thanks for your feedback.

Point selection is already implemented, and it is even smarter than nearest neighbor search, because it takes the actual symbolizer into account. And it also uses the R-Tree. You can see this outside the context of the Select control in http://ol3js.org/en/r3.0.0-alpha.4/examples/gpx.html - just hover over the points.

Regarding the click selection, there is a trade-off between a pure selection mode where map dragging is disabled and the convenience of being able to drag the map. But you can always remove the dragpaninteraction from the map when you want pure selection mode.

elemoine · 2013-08-22T08:44:13Z

This is what I'm thinking about: https://github.com/elemoine/ol3/commit/cf96b102f2c95b76e43a9fbe5f23c9827b6c6bdb.

elemoine · 2013-08-22T12:02:16Z

Strange user facing API aside

Passing filter functions to functions/constructors is a typical JavaScript pattern.

you would still have to do the same bookkeeping, even with a filter function. Because you need to create/remove/show/hide the selection layers. Or am I missing something?

Couldn't we lazily create selection layers? A feature is clicked, if we don't have a selection layer with the proper style for that feature then we create a new selection layer, otherwise we reuse the selection layer. In this way, we don't have a 1:1 mapping between original layers and selection layers, which means no bookkeeping.

Now, what to do when we have selected features and the original layer gets removed from the map, or is made invisible, is application-specific. There are certainly use-cases where you want the selection to remain visible, and others where you don't.

Our controls are currently not designed to be removed from a map. Once we have an ol.control.Control#removeMap() method, we can add clean up code. But without such an API in place, I think that is beyond the scope of this ticket.

We have ol.Map#removeControl, which does control.setMap(null). I think your code re-registers a listener when removeControl is called.

ahocevar · 2013-08-22T12:56:06Z

Thanks @elemoine, I'll give it a try. There will still be some bookkeeping to see if we have a selection layer already.

tschaub · 2013-08-22T14:48:49Z

I like the idea of filter functions, but they are more of a hassle if you have multiple layers without clearly distinguishable properties. Say you have four vector layers and want one editable/selectable, it means you have to give the layer some id and then create a function that returns true if the id matches. It is more straightforward to pass a reference to the layer itself.

A flexible API would take a filter function or a list of layers.

ahocevar · 2013-08-26T18:22:50Z

@elemoine I have converted the control to an interaction. Would be great if you could take a look. Thanks.

elemoine · 2013-08-28T10:11:17Z

Thanks @ahocevar. Will have a look this afternoon.

elemoine · 2013-08-28T11:42:10Z

examples/select-features.html

+          <div id="docs">
+            <p>See the <a href="select-features.js" target="_blank">select-features.js source</a> to see how this is done.</p>
+          </div>
+          <div id="tags">SelectFeature, vector</div>


"select" sounds like a better tag than "SelectFeature" to me. This is minor.

elemoine · 2013-08-28T13:36:52Z

The select-features example does not work in advanced mode (select-features.html?mode=advanced). Is it because the feature.renderIntent property gets renamed, preventing the rule filter from doing its job?

elemoine · 2013-08-28T13:43:00Z

src/ol/feature.js

+ol.Feature.prototype.clone = function() {
+  var clone = new ol.Feature(this.getAttributes());
+  clone.setGeometry(this.getGeometry().clone());
+  clone.featureId_ = this.featureId_;


The ol.feature#getFeatureId API doc says: "This identifier is usually the unique id in the source store", so I'm wondering if the clone should have the same featureId as the original feature.

The source store is the server. Think of it as GML fid. If the use case is to clone a feature because it will be persisted on the server with changes later, then the current behavior makes sense. If the use case is to create a copy and duplicate the feature on the server too, then it would be better to not use the same fid for the clone. So how should we proceed?

To me this suggests that the decision should be made at the application level.

What we need here is a mix of deep and shallow cloning, and we do not want to do this in a generic ol.Feature#clone() method.

tschaub · 2013-08-29T20:34:49Z

examples/select-features.js

+  renderer: ol.RendererHint.CANVAS,
+  target: 'map',
+  view: new ol.View2D({
+    center: [-10997171.194994785, 5206335.565590534],


Minor (and I know this is a problem in the example from which it was copied as well), but it would be nice to center the view on the visible data. Something like [-11000000, 4600000] makes a nice starting point.

tschaub · 2013-08-29T22:32:21Z

Really nice work @ahocevar. I'm in favor of merging this in - the comments I added above can be addressed now or later.

I'll admit I'm not totally sold on "select" as an interaction (wondering if instead a more basic "feature" interaction makes sense). I do think we should get this in and move on to editing to see how the different parts work best together.

Select control

elemoine · 2013-08-30T13:26:22Z

I was in the process of reviewing :) No problem. I'll make comments anyway, if I have some.

ahocevar · 2013-08-30T13:27:46Z

Sorry for that @elemoine - additional comments welcome of course.

elemoine · 2013-08-30T13:38:04Z

src/ol/interaction/interaction.js

 };
+goog.inherits(ol.interaction.Interaction, goog.Disposable);


When the map calls ol.interaction.defaults() to create the default collection of interactions, it should also call registerDisposable on each interaction of the collection.

With the map not disposing of the interactions it creates, and with no exports of our disposable objects' dispose methods, it is useless to have ol.interaction.Interaction inherit from goog.Disposable.

@elemoine So effectively this means I can also remove ol.interaction.Select#disposeInternal()?

I think so.

And in any way isn't it when the interaction is removed from the map that the selection layers should be removed, etc.?

Maybe, but I don't see a way to do that because there is no hook that is called when an interaction is removed from the map. For now I'm adding some code to remove selection layers when they do not contain any selection.

Until the select interaction the interactions were all stateless. We need to think this through I guess.

As @tschaub said, it may not be the best thing to have such a high level interaction. But let's see how we get along with it when we work on editing, and make changes as we need them.

elemoine · 2013-08-30T13:59:20Z

I added a few comments. And I'm done.

* Interaction is no goog.Disposable any more. * Permanent cleanup during selection instead of disposeInternal. * Moved selectionLayers creation outside feature loop. * Maintain selectedFeatures and unselectedFeatures only for layers that have a setRenderIntent method.

ahocevar · 2013-08-30T14:32:09Z

@elemoine Please take a quick look at #945, which addresses most of your post-merge comments. The only one I did not address was the goog.isFunction() check for getStyle(), because we do similar duck typing for setRenderIntent(). We do this elsewhere too, and to make it less evil we could define an interface that the vector layer and other potential layers that can display and style features implement.

Follow-up improvements for #897

elemoine reviewed Aug 16, 2013
View reviewed changes

ahocevar mentioned this pull request Aug 20, 2013

SelectFeature Control #623

Closed

elemoine reviewed Aug 28, 2013
View reviewed changes

ahocevar added 5 commits August 29, 2013 16:50

Use select as keyword

2394b39

Do not implement a specific clone method

0c1f232

What we need here is a mix of deep and shallow cloning, and we do not want to do this in a generic ol.Feature#clone() method.

Less closures

95cf0e1

Do not dispatch selection events for now

43c67ba

Filter layer by id

8a180e6

tschaub reviewed Aug 29, 2013
View reviewed changes

ahocevar added 6 commits August 30, 2013 11:43

No longer inheriting from EventTarget

6bb23cd

THIS_IDENTIFIER no longer needed

2a4aef0

Let's at least be disposable, so we can clean up after ourselves

e2c4fec

More specific INTENTCHANGE instead of SYMBOLIZER event

8372097

Symbolizer defaults for the select renderIntent

9dae49d

Better initial extent

643eb5c

ahocevar added a commit that referenced this pull request Aug 30, 2013

Merge pull request #897 from ahocevar/select

29317c3

Select control

ahocevar merged commit 29317c3 into openlayers:master Aug 30, 2013

ahocevar deleted the select branch August 30, 2013 13:25

elemoine reviewed Aug 30, 2013
View reviewed changes

ahocevar added a commit that referenced this pull request Sep 2, 2013

Merge pull request #945 from ahocevar/select-followup

39b4da5

Follow-up improvements for #897

		};
		goog.inherits(ol.interaction.Interaction, goog.Disposable);

Select control #897

Select control #897

Conversation

ahocevar commented Aug 11, 2013

tschaub commented Aug 15, 2013

elemoine commented Aug 16, 2013

elemoine commented Aug 16, 2013

Choose a reason for hiding this comment

ahocevar commented Aug 17, 2013

ahocevar commented Aug 17, 2013

elemoine commented Aug 17, 2013

tschaub commented Aug 17, 2013

elemoine commented Aug 17, 2013

ahocevar commented Aug 20, 2013

ahocevar commented Aug 21, 2013

elemoine commented Aug 21, 2013

ahocevar commented Aug 21, 2013

elemoine commented Aug 21, 2013

schedul-xor commented Aug 21, 2013

ahocevar commented Aug 21, 2013

elemoine commented Aug 22, 2013

elemoine commented Aug 22, 2013

ahocevar commented Aug 22, 2013

tschaub commented Aug 22, 2013

ahocevar commented Aug 26, 2013

elemoine commented Aug 28, 2013

Choose a reason for hiding this comment

elemoine commented Aug 28, 2013

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

tschaub commented Aug 29, 2013

elemoine commented Aug 30, 2013

ahocevar commented Aug 30, 2013

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

Choose a reason for hiding this comment

elemoine commented Aug 30, 2013

ahocevar commented Aug 30, 2013