add Protocol.SQL.Gears

[elemoine]

Add a Gears-specific protocol.

This protocol is implemented in the class Protocol.SQL.Gears, which relies on a more general class, namely Protocol.SQL.

[euzuro]

Would be great, but I think this is 2.8

[elemoine]

As discussed with Erik, and given that I'm currently actively working on this ticket, it is 2.7 material. Patch to come.

[elemoine]

All unit tests pass on FF2, FF3 and IE7 (with Gears installed). The example works as expected. Please review.

[elemoine]

Forgot to say that this ticket depends on #1677.

[fredj]

Forget to say that most of the code was stolen from crschmidt:  http://crschmidt.net/mapping/localdb/

[fredj]

Please remove the OpenLayers.Protocol.SQL.factory function in OpenLayers/Protocol/SQL.js: since there is only one supported backend no need to find a working one.

[elemoine]

patch-1699-r7882-A1.diff takes fredj's comments into account.

[elemoine]

Replying to fredj:

Forget to say that most of the code was stolen from crschmidt:  http://crschmidt.net/mapping/localdb/

Good point. Credits go to crschmidt.

[crschmidt]

I also wrote another SQL backend, based on HTML5 storage: did that make it through to the vector-behavior sandbox, or do I need to pull that out of localdb and make a patch?

[elemoine]

Replying to crschmidt:

I also wrote another SQL backend, based on HTML5 storage: did that make it through to the vector-behavior sandbox, or do I need to pull that out of localdb and make a patch?

No it did not. We've been working with Gears for a customer project, and haven't had time to move forward with the HTML5 protocol.

[crschmidt]

Okay. Thanks.

[euzuro]

warning: this is getting moved to 2.8 unless it can gets reviewed and committed in the next 30 hours or so....

[elemoine]

Note that is part of the vector-behavior work.

[elemoine]

New in patch-1699-r7944-C0.diff:

• OpenLayers.Protocol.SQL includes a postReadFiltering property, which indicates whether the user wants some filtering to be applied on the features read from the database
• OpenLayers.Protocol.SQL includes an evaluateFilter method, this method is to called from subclasses upon reading features from the database
• OpenLayers.Protocol.SQL.Gears calls the evaluateFilter method for each feature read from the database, if it returns false then the feature isn't added to resulting array

[euzuro]

Floating to RC1... vector-behavior patches will go in on RC2.

[crschmidt]

elemoine:

Thanks. I've reviewed this one, tested it, and am happy with it. (Somewhat disappointed that I didn't get time to make the HTML5 sql stuff go in, but that's my problem, not yours.) I can imagine enhancements to this, but I think this is good for 2.7.

(Since Tim said he as going to look at it, I'll not mark commit quite yet; I know that he is closer to this code than I am.)

[tschaub]

Replying to crschmidt:

elemoine: Thanks. I've reviewed this one, tested it, and am happy with it. (Somewhat disappointed that I didn't get time to make the HTML5 sql stuff go in, but that's my problem, not yours.) I can imagine enhancements to this, but I think this is good for 2.7. (Since Tim said he as going to look at it, I'll not mark commit quite yet; I know that he is closer to this code than I am.)

You know, I said I'd do it because I wanted to understand it - but I'm not a gears user and I don't really have time to get set up to review this now.

My one comment looking at the patch is why wkt + json? GeoJSON seems an obvious candidate, and would mean one format for the protocol. I'm sure I'm missing something, because I actually can't see immediately why the protocol cannot be made to work with whatever format the user decides to use (assuming one might want to persist data that only the Atom format persists or something - or they might create their own custom format that persists their own custom features). Anyway, that seems like it would be a pretty nice feature to me. I also understand that there is probably a good explanation of why it isn't this simple (does gears index data for queries or something?).

So, I don't want to hold this up. If you can educate me on why a custom format cannot be used with gears storage, I'd appreciate it. Otherwise, please mark as commit or take the commit as appropriate - assuming you guys have given this a good review.

[crschmidt]

Using a custom format with the gears storage seems ill-advised to me. The reason that I went with WKT in what I originally wrote was for similarity of treating this like other existing spatial databases; in the end, I sort of expect to have a database where attributes are treated as seperate columns in the database as well (as an option) to make it even more similar. If we had a WKB writer, I'd probably have preferred that even (though no particular reason why, just for similarity's sake).

So, Storing WKT in the geometry column seems to allow us to be in a state where the 'standard' representation of data is matching other geodatabases, especially in the cases where sqlite is used. (This is the same technique used by FeatureSever, for example, though our storage of attributes is different.) I liked this symmetry.

That said, I don't see a strong reason that we can't improve these options after checking this code in, given a migration path from 2.7 DBs to future DBs.

I see the database itself as being 'owned' by OpenLayers in the same way we own the map div: If we want to decide that we provide more control to the user for some reason, we can do that in the future.

GeoJSON may make more sense, but I was concerned about issues with escaping SQL at the time: in the end, that may have been the better route to go, but I think it's not a better route to go at this point. I'm prepared for this to go in as is, since I can confirm that it works, and I'll take the responsibility of maintaining backwards compatibility upgrades for existing databases as we decide to do things better.

Given that, I'm going to mark commit, with a mind towards refactoring in a backwards compatible way post-2.7.

[elemoine]

(In [8005]) add Protocol.SQL.Gears, and go do editing while being offline! r=crschmidt (closes #1699)

[elemoine]

(In [8008]) these two files were erroneously not committed as part of [8005], thanks Erik for catching this (See #1699)