Move rule subclasses to filter subclasses

The change may look big, but it is mostly a lot of renaming (and removes 40 lines from the SLD parser).

 Filter.js            |   50 ++++++++
 Filter/Comparison.js |  237 +++++++++++++++++++++++++++++++++++++++
 Filter/FeatureId.js  |   66 ++++++++++
 Filter/Logical.js    |   99 ++++++++++++++++
 Format/SLD/v1.js     |  309 ++++++++++++++++++++++-----------------------------
 Rule.js              |   18 ++
 Rule/Comparison.js   |  241 ---------------------------------------
 Rule/FeatureId.js    |   69 -----------
 Rule/Logical.js      |  104 -----------------
 9 files changed, 603 insertions(+), 590 deletions(-)

As an added benefit, with this patch comes the guts of an OpenLayers.Format.Filter class (by moving the ogc readers/writers from the SLD format). Then the SLD format extends itself with OpenLayers.Format.Filter.prototype.readers.ogc and same for writers.

Anyway, on to tests and examples.

Tests pass examples still work. I think this belongs in 2.6.

Meant to add that tests pass in FF, Safari, and IE.

Changes:

Same as old Rule version, with global replace rule/feature:

• tests/Filter/FeatureId.js
• tests/Filter/Comparison.js
• lib/OpenLayers/Filter/Comparison.js

tests/Filter/Logical.js: tests mostly the same, but changed slightly:

-        t.eq(rule.evaluate(feature), false,
+        t.eq(filter.evaluate(feature.attributes), false,
                 "feature evaluates to false correctly.");

This makes sense given the following change.

lib/OpenLayers/Filter/Logical.js:

• Evaluate now takes a context instead of a feature

lib/OpenLayers/Filter/FeatureId.js:

• Doesn't call parent evaluate() anymore (because it is now empty)

lib/OpenLayers/Filter/Comparison.js:

• Mentions that evaluate should be implemented by subclasses: what subclasses of comparison?
• Doesn't use getContext anymore

lib/OpenLayers/Format/SLD:

• Several instances of removing things that were described to allow nested FeatureID rules:

     // since FeatureId rules may be nested here, make room for them 
  

Concern:

• elseFilter on an OpenLayers.Rule now feels wrong somehow? I see that the name maps into SLD, but it feels like it should be something like a "fallbackRule" boolean (having read the relevant portions of the SLD code, Style code, SLD spec, and SLD example in the tasmania/ folder). I don't feel strongly about this though.

Overall: I have just spent three hours attempting to review this patch, so I'll admit that I'm likely a little wired, but I don't see anything in this patch that would cause me to reject it. Calling a Rule an elseFilter (and having it be unrelated to filters) seems weird to me, but I'm willing to accept that that is just the 'way things are'.

The "FeatureId" change mentioned above for Format/SLD seems odd, but I can't see a reason why it would need to be there.

In an effort to get 2.6RC2 out, I'm willing to accept this into trunk if Andreas also gives it a pass. Andreas, this is the only significant blocker for 2.6: Please review as soon as you are able.

I have confirmed that all tests continue to pass in FF3, Safari3, and I see no code that indicates that there would be any change in testing status for IE browsers.

Replying to crschmidt:

lib/OpenLayers/Format/SLD: * Several instances of removing things that were described to allow nested FeatureID rules:

As far as I understand the code, nested FeatureId filters are still possible. Those are now treated like any other nested filters.

Concern: * elseFilter on an OpenLayers.Rule now feels wrong somehow? I see that the name maps into SLD, but it feels like it should be something like a "fallbackRule" boolean (having read the relevant portions of the SLD code, Style code, SLD spec, and SLD example in the tasmania/ folder). I don't feel strongly about this though.

It does not feel too wrong to me. Since we have a filter property, why not have an elseFilter property? It just shows that this rule does not use a regular filter, but the special elseFilter. It would also be possible to move the elseFilter property into Filter.js, but creating a class instance just to tell the rule that it has an ElseFilter is probably overkill.

There is only a ND comment referring to subclasses that are not existent any more here.

I have just spent three hours attempting to review this patch, so I'll admit that I'm likely a little wired, but I don't see anything in this patch that would cause me to reject it. Calling a Rule an elseFilter (and having it be unrelated to filters) seems weird to me, but I'm willing to accept that that is just the 'way things are'.

If you see it in a slightly different way (the rule has an elseFilter instead of a filter), it might seem a little less weird.

The "FeatureId" change mentioned above for Format/SLD seems odd, but I can't see a reason why it would need to be there.

To me it also seems that no functionality is lost.

In an effort to get 2.6RC2 out, I'm willing to accept this into trunk if Andreas also gives it a pass. Andreas, this is the only significant blocker for 2.6: Please review as soon as you are able.

IMO this is good to commit.

Tim, thanks for the great work. And thanks Chris for making my review easier with your detailled description of the changes.

Andreas, thanks for the review: I'm happy with your comments regarding this patch, and happy with it goin in with no further feedback from me.

Replying to crschmidt:

Changes: Same as old Rule version, with global replace rule/feature: * tests/Filter/FeatureId.js * tests/Filter/Comparison.js * lib/OpenLayers/Filter/Comparison.js tests/Filter/Logical.js: tests mostly the same, but changed slightly: {{{ - t.eq(rule.evaluate(feature), false, + t.eq(filter.evaluate(feature.attributes), false, "feature evaluates to false correctly."); }}} This makes sense given the following change. lib/OpenLayers/Filter/Logical.js: * Evaluate now takes a context instead of a feature lib/OpenLayers/Filter/FeatureId.js: * Doesn't call parent evaluate() anymore (because it is now empty) lib/OpenLayers/Filter/Comparison.js: * Mentions that evaluate should be implemented by subclasses: what subclasses of comparison? * Doesn't use getContext anymore lib/OpenLayers/Format/SLD: * Several instances of removing things that were described to allow nested FeatureID rules: {{{ // since FeatureId rules may be nested here, make room for them }}} Concern: * elseFilter on an OpenLayers.Rule now feels wrong somehow? I see that the name maps into SLD, but it feels like it should be something like a "fallbackRule" boolean (having read the relevant portions of the SLD code, Style code, SLD spec, and SLD example in the tasmania/ folder). I don't feel strongly about this though.

There's no such thing as an "else" filter in the filter encoding spec. The idea of "else" comes from sld:Rule. A rule with no fe:Filter can be treated as "else" (if no other rules apply) by giving it the elseFilter property (true).

Overall: I have just spent three hours attempting to review this patch, so I'll admit that I'm likely a little wired, but I don't see anything in this patch that would cause me to reject it. Calling a Rule an elseFilter (and having it be unrelated to filters) seems weird to me, but I'm willing to accept that that is just the 'way things are'.

Thanks for the effort. See above re: elseFilter. Can explain more if that is still too vague.

The "FeatureId" change mentioned above for Format/SLD seems odd, but I can't see a reason why it would need to be there.

Ok, you caught me sneaking in an unrelated change. News flash: filters with logical operators filters *cannot* take FeatureId operators as children. I corrected this elsewhere before, but this is a more complete correction. A logical operator is either binary or unary. Binary logic ops have at least two children - any of comparison, spatial, or logical operators. Unary logic ops get one child - a choice of comparison, spatial, or logical. No nesting of FeatureId.

In an effort to get 2.6RC2 out, I'm willing to accept this into trunk if Andreas also gives it a pass. Andreas, this is the only significant blocker for 2.6: Please review as soon as you are able. I have confirmed that all tests continue to pass in FF3, Safari3, and I see no code that indicates that there would be any change in testing status for IE browsers.

A bit more clarification for whom it may concern. Rules have three different types of behavior with regard to filters: 1) A rule with no filter (fe:Filter element or filter property) and no elseFilter (sld:ElseFilter element or elseFilter property true) always applies. 2) A rule with a filter applies if the filter evaluates to true. 3) A rule with elseFilter applies if no other rules with filters apply.

You can see we allow a conflict where a user can create a rule with a filter and with elseFilter true. A more concise way to represent an elseFilter would be to set the filter property to false (default is null). This is a bit subtle and I imagine it would trip more people up.

All that said, the grand vision is that people don't have to create any of this by hand (coding). A graphical style editor is an appropriate place to create styles.

"A more concise way to represent an elseFilter would be to set the filter property to false (default is null)"

This is what I came up with as I was falling asleep last night: I was actually thinking of it as setting the filter to 'else', or something along those lines, but same idea.

I don't have any problem with the current code. I was just mentioning things that bothered me: it was 1:30AM after all.

"A graphical style editor is an appropriate place to create styles."

To the same extent that a graphical editor is an appropriate place to create code?

Replying to crschmidt:

"A more concise way to represent an elseFilter would be to set the filter property to false (default is null)" This is what I came up with as I was falling asleep last night: I was actually thinking of it as setting the filter to 'else', or something along those lines, but same idea. I don't have any problem with the current code. I was just mentioning things that bothered me: it was 1:30AM after all. "A graphical style editor is an appropriate place to create styles." To the same extent that a graphical editor is an appropriate place to create code?

No, in the same way that when you want to pick socks that match your pants, you look at the colors available in your drawer rather than looking at a list of hex rgb values.

Thanks for the review(s). I'm going to add in some SLD write tests, then I'll commit.

Replying to tschaub:

Replying to crschmidt:

"A graphical style editor is an appropriate place to create styles." To the same extent that a graphical editor is an appropriate place to create code?

If my above answer was too smartass, here's another. In the same way that we provide graphical tools for the editing of feature geometries (instead of suggesting that people "code" geometries by hand), it makes sense to provide tools for editing layer styles.

Replying to tschaub:

No, in the same way that when you want to pick socks that match your pants, you look at the colors available in your drawer rather than looking at a list of hex rgb values.

Shit. I *knew* I was doing something wrong.

(Both this, and my previous comment, are tongue in cheek.)

move rule subclasses to filter subclasses and test sld write

Ok, I know this is an insane changeset between release candidates, but it is an improvement.

The new SLD tests turned up a handful of problems. Four mispellings (filters and fitler instead of filter) and one problem with writing ogc:Literal elements for ogc:LowerBounds and ogc:UpperBounds.

With the above "commit" approval, I'll put this in. I do think a bit more fiddling would be good before the next release candidate - but understand if there is too much momentum to ask for that.

(In [6818]) With dramatic flourish, I'm modifying 21 files with 1033 insertions and 815 deletions between release candidates. This moves all rule subclasses to a more natural home as filter subclasses. Also adds tests for SLD write and corrects a handful of issues there. Apologies to all who lose sleep over this sort of thing. r=ahocevar,crschmidt (closes #1492)

(In [6819]) Pullup commits from trunk to 2.6 branch:

• virtualStyle typo (Closes #1495)
• JSON fix for Safari 3.1 (Closes #1493)
• panzoombar off-by-one (Closes #1486)
• Handler Hover exception (Closes #1480)
• Popup.framedcloud exception (Closes #1479)
• VML Renderer when including namespace in page (Closes #1477)
• SLD/Rule/Filter changes -- most of this commit (Closes #1492)

(In [6821]) Removing the Rule directory. (see #1492)

(In [6822]) Correcting dependencies for the SLD format. (see #1492)

r6821 removes unused dir

r6822 fixes single-file build

marking as pullup again

Pierre points out that addUniqueValueRules still uses OpenLayers.Rule.Comparison in StyleMap.js.

fix addUniqueValueRules

The styles-unique.html and wfs-reprojection.html examples work again. Thanks for the catch Pierre.

Good catch.

(In [6834]) Modifying the addUniqueValueRules method to use filter subclass. r=crschmidt (closes #1492)