Add option to allow Select interaction logic to select overlapping features #3019
Conversation
|
I like the idea of adding flexibility here, but I don't think we should change the default behavior. It makes sense to me that the default behavior is to work with one feature at a time. It would be nice to have an option to be able to add/remove multiple features per gesture. |
|
Reworked as an optional feature. I'm still at odds with the "no change" condition. It should work as before in the default case but when enabling multi I'm guessing it can't be the right logic. I'll try to get my head around it eventually. |
bjornharrtell
changed the title
|
Anyone wants to review this for 3.2? I'll move it to v3.3.0 if not reviewed by tomorrow Wednesday. |
| * @private | ||
| * @type {boolean} | ||
| */ | ||
| this.multi_ = goog.isDef(options.multi) ? |
|
Thanks for the review @fredj, I've fixed those and squashed to a single commit. Travis CI build fails and I can get the same error locally but I don't understand what it means or how it can be related to my branch. |
| @@ -2337,7 +2337,8 @@ olx.interaction.PinchZoomOptions.prototype.duration; | |||
| * layers: (Array.<ol.layer.Layer>|function(ol.layer.Layer): boolean|undefined), | |||
| * style: (ol.style.Style|Array.<ol.style.Style>|ol.style.StyleFunction|undefined), | |||
| * removeCondition: (ol.events.ConditionType|undefined), | |||
| * toggleCondition: (ol.events.ConditionType|undefined)}} | |||
| * toggleCondition: (ol.events.ConditionType|undefined), | |||
| * multi: {boolean|undefined}} | |||
There was a problem hiding this comment.
Use parenthesis, not curly braces
|
Thanks @gberaudo, don't know why I missed that. Fixed with updated squashed commit. |
| @@ -2519,7 +2519,8 @@ olx.interaction.PointerOptions.prototype.handleUpEvent; | |||
| * layers: (Array.<ol.layer.Layer>|function(ol.layer.Layer): boolean|undefined), | |||
| * style: (ol.style.Style|Array.<ol.style.Style>|ol.style.StyleFunction|undefined), | |||
| * removeCondition: (ol.events.ConditionType|undefined), | |||
| * toggleCondition: (ol.events.ConditionType|undefined)}} | |||
| * toggleCondition: (ol.events.ConditionType|undefined), | |||
| * multi: (boolean|undefined)} | |||
|
Thanks @fredj, fixed missing curly brace, and that seem to have made Travis happy. Will try to avoid similar typos in the future. |
|
LVGTM, thanks @bjornharrtell |
Add option to allow Select interaction logic to select overlapping features
|
It looks like this introduced a regression in behavior. See #3218 (and a mailing list issue which may or may not be related). |
|
The intention was to keep the existing behaviour and I when looking at the changes I don't understand what is causing the regression. The feature that gets selected should be the last one returned by forEachFeatureAtPixel both before and after this change. |
|
In the previous implementation, the first feature was returned by See #3221 for a fix |
The original default behaviour of the Select interaction is to "Replace the currently selected feature(s) with the feature at the pixel". I made this code change to change the semantics to "Replace the currently selected feature(s) with the feature(s) at the pixel". I could not find a way to configure to get the desired behaviour with add/remove/toggle conditions).
This PR is a proposal with some unanswered questions. The "no change" condition was and is at fault since it only compares the first of the current selected features. I'm prepared to rework the PR if the questions can be resolved.
What is a sensible default behaviour?
Should the default behaviour be configurable?