Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possibility to restrict the type of features you can select #3277

Closed
wants to merge 15 commits into from

Conversation

acanimal
Copy link

See #3257 issue.

This PR includes samples and also an initial implementation based on a new filter property which accepts a ol.interaction.SelectFilterFunction invoked for each selected feature and that determines if the feature must be selected or not.

* @private
* @type {ol.interaction.SelectFilterFunction}
*/
this.filter_ = goog.isDef(options.filter) ? options.filter : null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new filter option needs to be added to olx.interaction.SelectOptions. See the externs/olx.js file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could change this to:

  this.filter_ = goog.isDef(options.filter) ? options.filter : goog.functions.TRUE;

and thereby avoid testing that this.filter_ is defined in the handleEvent function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tip. I just start reading "Closure the definite guide".

@@ -150,7 +167,9 @@ ol.interaction.Select.handleEvent = function(mapBrowserEvent) {
* @param {ol.layer.Layer} layer Layer.
*/
function(feature, layer) {
selected.push(feature);
if (goog.isFunction(filter) && filter(feature, layer)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need the isFunction test.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@acanimal
Copy link
Author

Updated PR with the @elemoine comments. Added documentation to the olx.js file too.

Although the ./build.py ci task runs fine I don't know why the apidoc doesn't show the new filter property.

In addition, I have added a second parameter to the ol.interaction.SelectFilterFunction that references the layers which feature belongs. By some comment on the list I think having a reference to the layer can help in some situations.

if (types === null) {
return true;
} else {
return goog.array.contains(types, feature.getGeometry().getType());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't use goog functions in ol3 examples.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to use indexOf.

@elemoine
Copy link
Member

@acanimal, I don't think we need a new example for that new option.

/**
* A function that takes an {@link ol.Feature} and an {ol.layer.Layer} and
* returns true if the feature may be selected or false otherwise.
* @typedef {function(ol.Feature, ol.layer.Layer): boolean}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@typedef - > @type. And why don't you use ol.interaction.SelectFilterFunction here too?

We don't have a typedef for the layer filter so I think I'd drop the typedef entirely.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, mistake copy&paste. Now apidoc is generated fine.

@acanimal
Copy link
Author

I think sample can help to show how to use the property. Anyway the property is so trivial that can be removed. Simply confirm me and I will remove. I vote for not removing :)

@elemoine
Copy link
Member

@acanimal, yes, using the new filter option is trivial, and we can't add an example for every option we add. So I would remove the example.

@acanimal
Copy link
Author

Sample removed.

@elemoine
Copy link
Member

Did you see my other comments?

@acanimal
Copy link
Author

Strange, I didn't receive any notification to my email.
I just make the changes.

@acanimal
Copy link
Author

acanimal commented Mar 4, 2015

@ahocevar @elemoine Hi, what happened with this features? I though it would be merged in the 3.3 release :(
I don't know the the process must follow a PR to be accepted but this feature is important to me at work. Could you let me know some feedback?

Thanks.

@ahocevar
Copy link
Member

ahocevar commented Mar 4, 2015

@acanimal Sorry for that. Your pull request cannot be merged without conflicts, this is why it did not get merged yet. Also, it did not have a milestone assigned, so I did not wait for it when I created the release yesterday. But since we have a monthly release schedule, I'm assigning it the 3.4.0 milestone now. So if you are able to resolve the conflicts, we can merge your pull request and it will be in the next release.

@ahocevar ahocevar added this to the v3.4.0 milestone Mar 4, 2015
@acanimal
Copy link
Author

acanimal commented Mar 4, 2015

I though the conflict was due the addition of #3018, ./build.py ci tasks runs fine and also travis report "green" result.
I will resolve and update the PR.

Thanks

@acanimal
Copy link
Author

@ahocevar Merge done

@ahocevar
Copy link
Member

Have you seen @elemoine's last comment?

@acanimal
Copy link
Author

Sorry for the late reply. Yes, I just uploaded the modified code using this.filter_ property instead a local variable.

@tremby
Copy link
Contributor

tremby commented Mar 25, 2015

This feature is important for a project I'm currently working on. Very much looking forward to it being available in 3.4.0. Thanks to those involved.

@ahocevar
Copy link
Member

@elemoine, are your concerns addressed now?

@elemoine
Copy link
Member

LGTM. I just think we should squash the commits into one before merging. I will do that myself this morning, and create a new PR.

@@ -27,6 +27,15 @@ ol.SelectEventType = {
};


/**
* A function that takes an {@link ol.Feature} and an {ol.layer.Layer} and

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link for ol.layer.Layer should be
{@link ol.layer.Layer} - missing the @link

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Thanks for catching this.

@bartvde bartvde modified the milestones: v3.5.0, v3.4.0 Mar 29, 2015
@tschaub tschaub removed this from the v3.5.0 milestone May 5, 2015
@acanimal
Copy link
Author

Hi,
I'm "disconnected" from the project due time limitation but, why has been removed this PR from 3.5.0? I code it since 3.3 and never released.
Is there any other new functionallity in 3.5 to achieve the same result?

Cheers.

@bartvde
Copy link
Member

bartvde commented May 18, 2015

@acanimal it needs a rebase at least, not sure if there are any other reasons.

@fredj
Copy link
Member

fredj commented Aug 31, 2015

fixed with #3402 ?

@elemoine
Copy link
Member

fixed with #3402 ?

Yes.

@elemoine elemoine closed this Sep 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants