Opened 8 years ago

Closed 8 years ago

#1699 closed feature (fixed)

add Protocol.SQL.Gears

Reported by: elemoine Owned by: crschmidt
Priority: major Milestone: 2.7 Release
Component: Protocol Version: 2.7 RC1
Keywords: Cc:
State: Complete

Description

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.

Attachments (4)

patch-1699-r7875-A0.diff (48.2 KB) - added by elemoine 8 years ago.
patch-1699-r7882-A1.diff (38.7 KB) - added by elemoine 8 years ago.
patch-1699-r7917-B0.diff (41.9 KB) - added by elemoine 8 years ago.
patch-1699-r7944-C0.diff (52.6 KB) - added by elemoine 8 years ago.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 8 years ago by euzuro

  • Milestone changed from 2.7 Release to 2.8 Release

Would be great, but I think this is 2.8

comment:2 Changed 8 years ago by elemoine

  • Milestone changed from 2.8 Release to 2.7 Release

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

Changed 8 years ago by elemoine

comment:3 Changed 8 years ago by elemoine

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

comment:4 Changed 8 years ago by elemoine

Forgot to say that this ticket depends on #1677.

comment:5 Changed 8 years ago by elemoine

  • Owner elemoine deleted
  • State set to Review

comment:6 follow-up: Changed 8 years ago by fredj

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

comment:7 Changed 8 years ago by 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.

Changed 8 years ago by elemoine

comment:8 Changed 8 years ago by elemoine

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

comment:9 in reply to: ↑ 6 Changed 8 years ago by 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.

comment:10 Changed 8 years ago by euzuro

  • Owner set to euzuro
  • Status changed from new to assigned

comment:11 Changed 8 years ago by crschmidt

  • Owner changed from euzuro to crschmidt
  • Status changed from assigned to new

comment:12 Changed 8 years ago by crschmidt

  • Status changed from new to assigned

Changed 8 years ago by elemoine

comment:13 follow-up: Changed 8 years ago by 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?

comment:14 in reply to: ↑ 13 Changed 8 years ago by 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.

comment:15 Changed 8 years ago by crschmidt

Okay. Thanks.

comment:16 Changed 8 years ago by euzuro

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

comment:17 Changed 8 years ago by elemoine

Note that is part of the vector-behavior work.

Changed 8 years ago by elemoine

comment:18 Changed 8 years ago by 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

comment:19 Changed 8 years ago by euzuro

  • Version changed from 2.6 to 2.7 RC1

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

comment:20 follow-up: Changed 8 years ago by 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.)

comment:21 in reply to: ↑ 20 Changed 8 years ago by 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.

comment:22 Changed 8 years ago by crschmidt

  • State changed from Review to Commit

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.

comment:23 Changed 8 years ago by elemoine

  • Resolution set to fixed
  • State changed from Commit to Complete
  • Status changed from assigned to closed

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

comment:24 Changed 8 years ago by euzuro

  • State changed from Complete to Pullup

comment:25 Changed 8 years ago by euzuro

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:26 Changed 8 years ago by elemoine

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

comment:27 Changed 8 years ago by euzuro

  • Resolution set to fixed
  • State changed from Pullup to Complete
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.