Allow plugins to register search operators

[joshdoe]

At least #5905 and #6232 require the functionality for plugins to register search operators that can be used in the search dialog. Relevant code changes would be in org.openstreetmap.josm.actions.search.SearchCompiler. Currently an if-else structure handles operators in parseFactor(). This could perhaps be changed so that all operators (really subclasses of Match) provide another method to return the name for the operator, or when registering operators the name would be provided. This of course means that SearchCompiler should have a method like addOperator() which will append the new operator to a list, which both core and plugins can use. This function should check for name conflicts and modify the new operator name by appending an integer. The Match class should also require the operator to return a short (and possibly a long) description which can be used for generating help.

Does this sound like a reasonable implementation? I wouldn't want to adversely impact performance of course.

[joshdoe]

Note that I asked for some feedback about this ticket on this ML thread:
​http://lists.openstreetmap.org/pipermail/josm-dev/2011-December/005928.html

[joshdoe]

add ability for plugins to register search match operators

[joshdoe]

adds insideselection search operator to utilsplugin2

[joshdoe]

I've attached a patch which implements this functionality, but I'm not terribly happy with the design. I would appreciate a thorough review. I also modified utilsplugin2 to provide an "insideselection" keyword to see how it works.

I'm not sure how best to handle the descriptions. I think the search dialog probably needs some reworking to handle the ever increasing number of special targets. Also, I feel like the search grammar and implementation could benefit from some changes, like allowing for functions of the form function(arg1, arg2, etc.) Also it would be good for search to be more flexible than just having match operators. However these issues are outside the scope of this ticket.

[simon04]

Replying to joshdoe:

I've attached a patch which implements this functionality, but I'm not terribly happy with the design. I would appreciate a thorough review.

• One could get around the reflection stuff by using a Map<String, PluginMatch> directly …
• This allows to extend the search by further keywords, but (if I see it correctly) no additional arguments are possible, e.g. foobar:123.

I'm not sure how best to handle the descriptions. I think the search dialog probably needs some reworking to handle the ever increasing number of special targets.

I agree. Most keywords are self-explanatory and therefore probably don't need a long description text. It would be handy to copy-and-paste some keywords to the search text.

Also, I feel like the search grammar and implementation could benefit from some changes

I agree, too.

like allowing for functions of the form function(arg1, arg2, etc.)

Sounds interesting – could you mention a useful example function?

[joshdoe]

Replying to simon04:

Replying to joshdoe:

I've attached a patch which implements this functionality, but I'm not terribly happy with the design. I would appreciate a thorough review.

• One could get around the reflection stuff by using a Map<String, PluginMatch> directly …

I would do that, but since currently a new instance is created for each occurrence of a keyword, I thought it would be safer to follow that pattern. While most Match subclasses seem to essentially be static, I didn't want to assume that that will always be the case, a silly example being one which only allows the first 100 objects.

• This allows to extend the search by further keywords, but (if I see it correctly) no additional arguments are possible, e.g. foobar:123.

Yes, I planned to do that as well, but I wanted comments on the implementation so far, especially if there was interest in having all Match subclasses provide getKeyword() and getDescription(). See below for further comments on this.

I'm not sure how best to handle the descriptions. I think the search dialog probably needs some reworking to handle the ever increasing number of special targets.

I agree. Most keywords are self-explanatory and therefore probably don't need a long description text. It would be handy to copy-and-paste some keywords to the search text.

Also, I feel like the search grammar and implementation could benefit from some changes

I agree, too.

like allowing for functions of the form function(arg1, arg2, etc.)

Sounds interesting – could you mention a useful example function?

Well for one allowing operators like inside to accept a search query without requiring some funky syntax with the current operator:argument form. Imagine building inside(amenity=school operator="My Government"), this would get tricky with the current method. Besides, at least to me, using parentheses are more natural. Other operators could more easily accept multiple arguments (e.g. different algorithms in the same class, fine tuning parameters, etc.).

[bastiK]

Replying to joshdoe:

Also, I feel like the search grammar and implementation could benefit from some changes

Please give more details, what you have in mind.

like allowing for functions of the form function(arg1, arg2, etc.)

This doesn't look like it fits in the current concept. It would mean that all colon syntax is changed to parentheses, e.g. "id(0)" instead of "id:0". Could be done, but I don't see the need for this right now.

Well for one allowing operators like inside to accept a search query without requiring some funky syntax with the current operator:argument form. Imagine building inside(amenity=school operator="My Government"), this would get tricky with the current method.

No, it wouldn't. An equivalent example is building parent (shop id:0) which finds newly created shops that are part of a building outline. This works just fine and so will your "inside" operator.

There are two kinds of parameter:

data parameter

custom string after colon, e.g. "id:0" or "tags:2-3"

osm-object parameter

e.g. "- selected" (not) or "child selected"
This uses Polish notation, so if a second object parameter should ever be needed, you can simply append it at the end, e.g.

intersection (type:node child boundary=administrative) -(type:node child boundary=administrative)

would search for any nodes of boundaries, that are connected to something that is not a boundary

Edit: The last example is just a complicated way to say "and".

[joshdoe]

SearchCompiler using factories/reflection

[joshdoe]

Replying to bastiK:

I was confused by how search works, but now I understand, thanks for the explanation.

I've changed SearchCompiler quite a bit now, adding UnaryMatch (for parent, child, and others like inside) and BinaryMatch (and and or) abstract classes, and half-way implementing both a factory- and reflection-based system for loading them. I've commented out the reflection bits and currently use factories in the attached file (attachment:SearchCompiler.java​). I want to get feedback as to which method is preferable before fully implementing one of them.

For the factory implementation, the core and each plugin will provide an implementation of MatchFactory to the MasterMatchFactory, which is responsible for returning a new instance of a Match class based upon the provided keyword and possible arguments (just PushbackTokenizer for a simple one like selected, or up to two Match'es for a BinaryMatch). This method provides plugins (and core) with more versatility, e.g. special processing of the following tokens as with timestamp, or returning the same class (with different parameters) as in the case of (all)inview and (all)indownloadedarea.

In the reflection implementation, the core and each plugin provides the keyword and the Class of the corresponding Match. The core itself is responsible for instantiating new classes using reflection. This is perhaps more elegant for plugin writers, who only have to create a simple Match class and call addMatchOperator(), but makes the core code a little bit obtuse and has more potential for run time errors.

Of course, it's quite likely that you'd rather not have me touch any of the core operators, but that doesn't change the above discussion.

[bastiK]

Please attach a patch file next time and no tabs.

Replying to joshdoe:

I've changed SearchCompiler quite a bit now, adding UnaryMatch (for parent, child, and others like inside) and BinaryMatch (and and or) abstract classes, and half-way implementing both a factory- and reflection-based system for loading them. I've commented out the reflection bits and currently use factories in the attached file (attachment:SearchCompiler.java​). I want to get feedback as to which method is preferable before fully implementing one of them.

For the factory implementation, the core and each plugin will provide an implementation of MatchFactory to the MasterMatchFactory, which is responsible for returning a new instance of a Match class based upon the provided keyword and possible arguments (just PushbackTokenizer for a simple one like selected, or up to two Match'es for a BinaryMatch). This method provides plugins (and core) with more versatility, e.g. special processing of the following tokens as with timestamp, or returning the same class (with different parameters) as in the case of (all)inview and (all)indownloadedarea.

In the reflection implementation, the core and each plugin provides the keyword and the Class of the corresponding Match. The core itself is responsible for instantiating new classes using reflection. This is perhaps more elegant for plugin writers, who only have to create a simple Match class and call addMatchOperator(), but makes the core code a little bit obtuse and has more potential for run time errors.

I think you are on the right track. As for factory vs. reflection - both is acceptable, choose whatever works better.

Of course, it's quite likely that you'd rather not have me touch any of the core operators, but that doesn't change the above discussion.

You can touch anything, provided

• it works
• code is still maintainable
• changes are necessary (no cosmetics).

[joshdoe]

Replying to bastiK:

Please attach a patch file next time and no tabs.

Will do, sorry.

Replying to joshdoe:
I think you are on the right track. As for factory vs. reflection - both is acceptable, choose whatever works better.

I'm going with the factory method, as it has more compile-time checks, offers more flexibility, and is easier to understand.

Of course, it's quite likely that you'd rather not have me touch any of the core operators, but that doesn't change the above discussion.

You can touch anything, provided

• it works
• code is still maintainable
• changes are necessary (no cosmetics).

Ok, I'll have all the core operators use this architecture.

One thing I'm not sure about is how to handle the keyword, description, and tooltip for each Match. I think perhaps each implementation of MatchFactory should provide a getKeywords() method to return a Collection of all the available keywords, and then getTooltip(String keyword) and getDescription(String keyword). Then it's up to the specific implementation whether they store the three strings as static member variables of each Match class, or just hardcode it.

[joshdoe]

add ability for plugins to register search match operators using a factory pattern

[joshdoe]

Add inside operator to utilsplugin2 using factory method

[joshdoe]

I've implemented something I'm a little happier with in the previous two patches, so I now have an inside operator, allowing searches like leisure=pitch inside amenity=school. There's still some kludgy bits, but I'd appreciate a review. I suppose the patch is somewhat bloated by my addition of comments to existing code and the addition of a XOR operator (in Git I'd be able to handle this much better, and I haven't had luck with git-svn yet). If this is very bothersome I can split this into separate patches.

I still don't properly handle returning descriptions/examples/tooltips, or BinaryMatch's. Also I don't like how I have isUnaryMatch(), but I somehow have to avoid calling parseFactor() except when needed. Suggestions are welcome!

[simon04]

Replying to joshdoe:

I still don't properly handle returning descriptions/examples/tooltips, or BinaryMatch's. Also I don't like how I have isUnaryMatch(), but I somehow have to avoid calling parseFactor() except when needed. Suggestions are welcome!

Typically, a match class either implements a unary or a binary match, right? You could extend MatchFactory by a generic type <T extends Match>, and have the methods T getMatch(...) and isMatch() (or matches()).
So, UtilsMatchFactory implements MatchFactory<UnaryMatch> …

[bastiK]

Replying to joshdoe:

I've implemented something I'm a little happier with in the previous two patches, so I now have an inside operator, allowing searches like leisure=pitch inside amenity=school. There's still some kludgy bits, but I'd appreciate a review. I suppose the patch is somewhat bloated by my addition of comments to existing code and the addition of a XOR operator (in Git I'd be able to handle this much better, and I haven't had luck with git-svn yet). If this is very bothersome I can split this into separate patches.

There is a git mirror on github.

(We don't use Vector class but List)

Also I don't like how I have isUnaryMatch(), but I somehow have to avoid calling parseFactor() except when needed. Suggestions are welcome!

There are 2 steps: 1. Check if the keyword equals and 2. create the corresponding Match object.

Maybe simply map the keywords to match factories?

private static Map<String, UnaryMatchFactory> unaryMatches;

public static interface UnaryMatchFactory {
  UnaryMatch get(Match matchOperand, PushbackTokenizer tokenizer);
}

[simon04]

Replying to bastiK:

(in Git I'd be able to handle this much better, and I haven't had luck with git-svn yet). If this is very bothersome I can split this into separate patches.
There is a git mirror on github.

I also develop using the Github mirror, export patches (git diff), and patch the SVN tree to commit them.

[joshdoe]

method closer to what bastik suggested

[joshdoe]

inside, intersecting, and allintersecting operators using newer MatchFactory

[joshdoe]

Replying to bastiK:

Replying to joshdoe:

Also I don't like how I have isUnaryMatch(), but I somehow have to avoid calling parseFactor() except when needed. Suggestions are welcome!

There are 2 steps: 1. Check if the keyword equals and 2. create the corresponding Match object.

Maybe simply map the keywords to match factories?

private static Map<String, UnaryMatchFactory> unaryMatches;

public static interface UnaryMatchFactory {
  UnaryMatch get(Match matchOperand, PushbackTokenizer tokenizer);
}

The two latest patches do something like this, however I didn't want to create one factory per Match, so instead I have one factory per type (simple, unary, binary) per plugin; I guess I dreaded creating 20 or so factory methods for core. The utilsplugin2 patch also now has an intersecting and allintersecting operator.

[bastiK]

Is this the final version that should be applied?

[joshdoe]

Replying to bastiK:

Is this the final version that should be applied?

It seems to work fine for me, but certainly needs further testing, though I suppose that's what josm-latest is for. :)

There will be some further minor modifications if/when the search dialog is redesigned to allow showing keywords from plugins.

[bastiK]

In [4817/josm]:

applied #7178 - Allow plugins to register search operators (patch by joshdoe)

[joshdoe]

Note that the utilsplugin2 patch has been applied, see #5905. Further discussion on operators will occur there.