Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Logical filter touch-ups #501

Merged
merged 3 commits into from

4 participants

@tschaub
Owner

This corrects the behavior of the logical OR filter, adds logical NOT, and provides factories for all logical filters.

@ahocevar
Owner

Nice! Please merge.

@elemoine
Owner

Looks good to me too. I'd have one API-related comment though: elsewhere in the lib we specialize using inheritance, and with this patch we start specializing using factory functions. Relying on factory functions makes more sense to me, but I think we should be consistent.

@elemoine elemoine referenced this pull request
Closed

Add ol.Map2D #463

@tschaub
Owner

Thanks for the reviews.

with this patch we start specializing using factory functions

I don't see that inheritance is an option here. The ol.filter.not function, for example, is just a convenience for this:

new ol.filter.Logical([filter], ol.filter.LogicalOperator.NOT);

(And a logical filter still inherits from the base filter.)

So the not factory is just a convenience function.

In a related note, I'd like to change the filter constructors to take options objects. We have 7 years of painful backwards compatibility that demonstrates (to me) that public facing constructors with ordered arguments are a bad idea. I'm not pretending that we didn't learn lessons with OpenLayers 2.

Update: I see now that you probably meant there could be ol.filter.LogicalNot, ol.filter.LogicalAnd, and ol.filter.LogicalOr types (we could even go deeper if we really wanted to - and you have to overlook that not is a binary operator there). I'm used to being able to do a runtime check for a logical filter. Would you suggest we have an abstract logical filter?

@elemoine
Owner

I don't like deep class hierarchies, and long inheritance chains may affect perfomance. I'm just looking at building a consistent API. Looking at the current API using factory functions for the OR, AND, and NOT logical filters just looked inconsistent to me. Maybe we should use factory functions everywhere it makes sense, for the XYZ sources for example. Please merge with what you think is best. I don't have a strong opinion on this.

@elemoine
Owner

An alternative would be to have factory functions for every public-facing type. In this way we could even have ol.layer.openstreetmap, which would return an ol.layer.TileLayer configured with an ol.source.OpenStreetMap. It sounds to me that using factories for everything would allow us to build a consistent API, giving us more freedom and avoiding deep class hierarchies. I'm almost tempted to give it a try.

@elemoine
Owner

See https://github.com/elemoine/ol3/compare/factories for a proof-of-concept. It doesn't compile, but the simple example works in debug mode. If people agree with the approach I can try to go further.

@twpayne

See https://github.com/elemoine/ol3/compare/factories for a proof-of-concept. It doesn't compile, but the simple example works in debug mode. If people agree with the approach I can try to go further.

See also 6cb3622 from last summer.

I think you'll find the conflict between namespaces and factories troublesome in compiled mode, and will eventually have to split create a separate namespaces (e.g. ol for the factory functions/API and OL for internal stuff). But if you really want, go for it.

@elemoine
Owner

As discussed offline with @twpayne we can certainly avoid naming collisions if the factories aren't shortcuts but just replacements for the constructors. My "factory" branch demonstrates that already, though it would require more changes (to controls, interactions etc.) to fully demonstrate that the approach works.

@tschaub
Owner

I like the factory discussion and think it should continue on a separate pull request. Out of curiosity, what is an example of the naming collision?

@tschaub tschaub merged commit 2ecaf2b into from
@tschaub tschaub deleted the branch
@twpayne

I like the factory discussion and think it should continue on a separate pull request. Out of curiosity, what is an example of the naming collision?

An example of a naming collision would be if you wanted your ol.Projection factory to be called ol.projection. ol.projection is already a namespace. This is why ol.projection.get is not called ol.projection.

I think this can be made to work in simple cases, but IIRC things do start to get messy with the compiler after a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 7, 2013
  1. @tschaub

    Correcting or and adding not

    tschaub authored
    Previously, OR filters always returned true (fixes #500).
  2. @tschaub
  3. @tschaub

    Doc fixes

    tschaub authored
This page is out of date. Refresh to see the latest.
View
79 src/ol/filter/logicalfilter.js
@@ -1,6 +1,10 @@
goog.provide('ol.filter.Logical');
goog.provide('ol.filter.LogicalOperator');
+goog.provide('ol.filter.and');
+goog.provide('ol.filter.not');
+goog.provide('ol.filter.or');
+goog.require('goog.asserts');
goog.require('ol.filter.Filter');
@@ -9,7 +13,7 @@ goog.require('ol.filter.Filter');
* @constructor
* @extends {ol.filter.Filter}
* @param {Array.<ol.filter.Filter>} filters Filters to and-combine.
- * @param {!ol.filter.LogicalOperator} operator Operator.
+ * @param {ol.filter.LogicalOperator} operator Operator.
*/
ol.filter.Logical = function(filters, operator) {
goog.base(this);
@@ -19,9 +23,10 @@ ol.filter.Logical = function(filters, operator) {
* @private
*/
this.filters_ = filters;
+ goog.asserts.assert(filters.length > 0, 'Must supply at least one filter');
/**
- * @type {!ol.filter.LogicalOperator}
+ * @type {ol.filter.LogicalOperator}
*/
this.operator = operator;
@@ -35,14 +40,29 @@ goog.inherits(ol.filter.Logical, ol.filter.Filter);
ol.filter.Logical.prototype.applies = function(feature) {
var filters = this.filters_,
i = 0, ii = filters.length,
- operator = this.operator,
- start = operator(true, false),
- result = start;
- while (result === start && i < ii) {
- result = operator(result, filters[i].applies(feature));
- ++i;
+ result;
+ switch (this.operator) {
+ case ol.filter.LogicalOperator.AND:
+ result = true;
+ while (result && i < ii) {
+ result = result && filters[i].applies(feature);
+ ++i;
+ }
+ break;
+ case ol.filter.LogicalOperator.OR:
+ result = false;
+ while (!result && i < ii) {
+ result = result || filters[i].applies(feature);
+ ++i;
+ }
+ break;
+ case ol.filter.LogicalOperator.NOT:
+ result = !filters[i].applies(feature);
+ break;
+ default:
+ goog.asserts.assert(false, 'Unsupported operation: ' + this.operator);
}
- return result;
+ return !!result;
};
@@ -55,9 +75,44 @@ ol.filter.Logical.prototype.getFilters = function() {
/**
- * @enum {!Function}
+ * @enum {string}
*/
ol.filter.LogicalOperator = {
- AND: /** @return {boolean} result. */ function(a, b) { return a && b; },
- OR: /** @return {boolean} result. */ function(a, b) { return a || b; }
+ AND: '&&',
+ OR: '||',
+ NOT: '!'
+};
+
+
+/**
+ * Create a filter that evaluates to true if all of the provided filters
+ * evaluate to true.
+ * @param {...ol.filter.Filter} var_filters Filters.
+ * @return {ol.filter.Logical} A logical AND filter.
+ */
+ol.filter.and = function(var_filters) {
+ var filters = Array.prototype.slice.call(arguments);
+ return new ol.filter.Logical(filters, ol.filter.LogicalOperator.AND);
+};
+
+
+/**
+ * Create a new filter that is the logical compliment of another.
+ * @param {ol.filter.Filter} filter The filter to negate.
+ * @return {ol.filter.Logical} A logical NOT filter.
+ */
+ol.filter.not = function(filter) {
+ return new ol.filter.Logical([filter], ol.filter.LogicalOperator.NOT);
+};
+
+
+/**
+ * Create a filter that evaluates to true if any of the provided filters
+ * evaluate to true.
+ * @param {...ol.filter.Filter} var_filters Filters.
+ * @return {ol.filter.Logical} A logical OR filter.
+ */
+ol.filter.or = function(var_filters) {
+ var filters = Array.prototype.slice.call(arguments);
+ return new ol.filter.Logical(filters, ol.filter.LogicalOperator.OR);
};
View
144 test/spec/ol/filter/logicalfilter.test.js
@@ -0,0 +1,144 @@
+goog.provide('ol.test.filter.Logical');
+
+
+describe('ol.filter.Logical', function() {
+
+ var OR = ol.filter.LogicalOperator.OR;
+ var AND = ol.filter.LogicalOperator.AND;
+ var NOT = ol.filter.LogicalOperator.NOT;
+ var include = new ol.filter.Filter(function() {return true});
+ var exclude = new ol.filter.Filter(function() {return false});
+
+ var apple = new ol.Feature({});
+ var orange = new ol.Feature({});
+ var duck = new ol.Feature({});
+
+ var isApple = new ol.filter.Filter(function(feature) {
+ return feature === apple;
+ });
+ var isOrange = new ol.filter.Filter(function(feature) {
+ return feature === orange;
+ });
+ var isDuck = new ol.filter.Filter(function(feature) {
+ return feature === duck;
+ });
+
+ describe('constructor', function() {
+ it('creates a new filter', function() {
+ var filter = new ol.filter.Logical([include, exclude], OR);
+ expect(filter).to.be.a(ol.filter.Logical);
+ });
+ });
+
+ describe('#operator', function() {
+ it('can be OR', function() {
+ var filter = new ol.filter.Logical([include, exclude], OR);
+ expect(filter.operator).to.be(OR);
+ });
+
+ it('can be AND', function() {
+ var filter = new ol.filter.Logical([include, exclude], AND);
+ expect(filter.operator).to.be(AND);
+ });
+
+ it('can be NOT', function() {
+ var filter = new ol.filter.Logical([include], NOT);
+ expect(filter.operator).to.be(NOT);
+ });
+ });
+
+ describe('#applies', function() {
+
+ it('works for OR', function() {
+ var isFruit = new ol.filter.Logical([isApple, isOrange], OR);
+
+ expect(isApple.applies(apple)).to.be(true);
+ expect(isOrange.applies(apple)).to.be(false);
+ expect(isFruit.applies(apple)).to.be(true);
+
+ expect(isApple.applies(duck)).to.be(false);
+ expect(isOrange.applies(duck)).to.be(false);
+ expect(isFruit.applies(duck)).to.be(false);
+ });
+
+ it('works for AND', function() {
+ expect(include.applies(apple)).to.be(true);
+ expect(isApple.applies(apple)).to.be(true);
+ expect(isDuck.applies(apple)).to.be(false);
+
+ var pass = new ol.filter.Logical([include, isApple], AND);
+ expect(pass.applies(apple)).to.be(true);
+
+ var fail = new ol.filter.Logical([isApple, isDuck], AND);
+ expect(fail.applies(apple)).to.be(false);
+ });
+
+ it('works for NOT', function() {
+ expect(isApple.applies(apple)).to.be(true);
+ expect(isDuck.applies(apple)).to.be(false);
+ expect(isDuck.applies(duck)).to.be(true);
+ expect(isDuck.applies(apple)).to.be(false);
+
+ var notApple = new ol.filter.Logical([isApple], NOT);
+ expect(notApple.applies(apple)).to.be(false);
+ expect(notApple.applies(duck)).to.be(true);
+
+ var notDuck = new ol.filter.Logical([isDuck], NOT);
+ expect(notDuck.applies(apple)).to.be(true);
+ expect(notDuck.applies(duck)).to.be(false);
+ });
+ });
+
+});
+
+describe('ol.filter.and', function() {
+ it('creates the a logical AND filter', function() {
+ var a = new ol.filter.Filter();
+ var b = new ol.filter.Filter();
+ var c = new ol.filter.Filter();
+ var and = ol.filter.and(a, b, c);
+ expect(and).to.be.a(ol.filter.Logical);
+ expect(and.operator).to.be(ol.filter.LogicalOperator.AND);
+ var filters = and.getFilters();
+ expect(filters[0]).to.be(a);
+ expect(filters[1]).to.be(b);
+ expect(filters[2]).to.be(c);
+ });
+});
+
+describe('ol.filter.not', function() {
+ it('creates the logical compliment of another filter', function() {
+ var include = new ol.filter.Filter(function() {return true;});
+ var notInclude = ol.filter.not(include);
+ expect(notInclude).to.be.a(ol.filter.Logical);
+ expect(notInclude.applies()).to.be(false);
+
+ var exclude = new ol.filter.Filter(function() {return false;});
+ var notExclude = ol.filter.not(exclude);
+ expect(notExclude).to.be.a(ol.filter.Logical);
+ expect(notExclude.applies()).to.be(true);
+ });
+});
+
+describe('ol.filter.or', function() {
+ it('creates the a logical OR filter', function() {
+ var a = new ol.filter.Filter();
+ var b = new ol.filter.Filter();
+ var c = new ol.filter.Filter();
+ var and = ol.filter.or(a, b, c);
+ expect(and).to.be.a(ol.filter.Logical);
+ expect(and.operator).to.be(ol.filter.LogicalOperator.OR);
+ var filters = and.getFilters();
+ expect(filters[0]).to.be(a);
+ expect(filters[1]).to.be(b);
+ expect(filters[2]).to.be(c);
+ });
+});
+
+goog.require('ol.Feature');
+goog.require('ol.filter.Filter');
+goog.require('ol.filter.Logical');
+goog.require('ol.filter.LogicalOperator');
+goog.require('ol.filter.and');
+goog.require('ol.filter.not');
+goog.require('ol.filter.or');
Something went wrong with that request. Please try again.