PrePostFilterVisitors

Jody Garnett added a comment - 13/Apr/11 9:38 AM Okay had a slightly better look at how PrePostFilterVisitor works; it is interesting it is used to keep a little bit of context as what is trying to be accomplished here is doing queries against the tree structure. We have something similar going on with a stack when copying features; but it is not quite as cool. Will need to think about this more; I was able to accomplish the functionality for Filters using brute force and ignorance (with what should be a little better performance).

Eric Sword added a comment - 13/Apr/11 10:31 AM Oops - I should have posted this comment here rather than under the original issue. Sorry for the duplicate: AbstractSearchFilterVisitor makes sense because the two Filters where the capability is most useful are the two I originally wrote - FindType and FindProperty. Those do want to check every type of Filter that comes through. The only reason for the instanceof check in FindProperty is because the parent classes implement ExpressionVisitor as well. As for the other two (GetParent and Replace)... mea culpa. I have to think back to when I wrote them a bit over a year ago and guess that I just got in the habit of extending PrePostFilterVisitor rather than thinking about what fit best. Having them extend DefaultFilterVisitor and then override And/Or/Not is cleaner. Or, generalizing the use case (which I tend to do too much), we could have "grouping visits" that are called for certain types of related items. e.g. preVisit/postVisit[All, Filter, Expression, Logic, BinaryComparison, BinarySpatial, etc]. With these, FindType would override postVisitAll, FindPropertyVistor would override postVisitFilter and GetParent and Replace would override postVisitLogic. (Pardon the names.) I am just tossing around ideas to avoid having many one line functions that call into some other function to do the guts of their work as will happen with GetParent and Replace when overriding the visit methods for And/Or and maybe Not. This isn't an urgent problem in any sense since only these classes do it at the moment. It is probably easier to just wait and see if additional classes could benefit from having a set of "functional grouping" visit calls.

Eric Sword added a comment - 15/Apr/11 10:45 AM Jody added a comment to GEOT-3508 which I am copying here: You are onto something with how you use your PrepPostFilterVisitors; I am just not sure what :-) One common thing is that you are trying to look around (parent and child relationships) as you process a filter ... 1- we already have one good way to "look down" at the children (pass control to another visitor which will go down and have a look) 2- but there is no good way to look up at parent So #2 is our real trouble here; I have an idea ... when we are writing the duplicate visitor we have to keep a stack around so we don't go insane with recursion. If we make a visitor that keeps a stack around then in your normal visitor methods you could ask the stack questions about your parents. Would that work for you?

Eric Sword added a comment - 15/Apr/11 10:50 AM That sounds fine, though are you considering putting that in some general filter capability or just in duplicate visitor?

Jody Garnett added a comment - 15/Apr/11 7:30 PM Well I figured I would attach a new class to this bug report and you could tell me if it is useful? I would not like to confuse duplicate visitor with an additional responsibility.