ckanext-spatial icon indicating copy to clipboard operation
ckanext-spatial copied to clipboard

91 spatial form widget, upgrade leaflet and leaflet.draw

Open florianm opened this issue 10 years ago • 10 comments

Here's an attempt at providing a spatial widget, addressing #91 . There are implications and I'm not sure whether my percussive implementation approach (hitting it with a wrench until it works) was the most graceful way, so I'd be grateful for feedback and suggestions.

Main motivation: My end users love geo-referencing their datasets, and being able to search by map. But many (especially non-GIS-trained staff) struggle with GeoJSON, let alone a GeoJSON geometry. At the same time, my end users have expert knowledge about the true extent of their dataset. This means, letting them draw rather than paste or derive the extent would both make their lives easier (actually, enable them to provide geo-reference at all), and capture the most accurate study extent. Having accurate enough dataset extents would enable an agency to use the search tool to identify possible conflicts of planned field work with existing field sites. E.g., some of my end users manage controlled burns, while others monitor vegetation through 40+ years of non-burning.

So all we need is a map on which we can draw and edit polygons and rectangles, and the map needs to be data-bound to the "spatial" input. Bonus if one could pick spatial extents from a dropdown menu of favourite areas. Even better (haven't solved that one yet) if that list could be maintained through an API and would auto-complete like the tags do.

  • Updated Leaflet to 0.8-dev and Leaflet.draw to 0.2.3 (is there any better way than overwriting files in /vendor/?)
  • Used a patched geoalchemy as per #92
  • Updated spatial-query's interaction with Leaflet.draw (required due to breaking changes in Leaflet.draw)
  • Provided a new module spatial-form (added usage docs directly into the file, to be moved into a better place later) and, over at ckanext-scheming, there's a new spatial input widget which will be a separate pull request for ckanext-scheming.

Currently, the dropdown comes out of ckanext-scheming, while the map comes from ckanext-spatial. To keep things simple, I've only data-bound map->text, dropdown->text, but neither dropdown->map nor text->map.

Screenshots of working examples are at ckanext-scheming issue 11, but feel free to take my sandbox for a whirl (request a login from me) or, if you need your hands free (depending on how much you enjoy spatial stuff, I'm not judging), watch the thing in action at https://vimeo.com/116324887

Review on Reviewable

florianm avatar Dec 19 '14 07:12 florianm

+1 =)

adonm avatar Jan 11 '15 17:01 adonm

This looks fantastic, I'll try and merge it as soon as I sort out the 2.3 support

amercader avatar Mar 25 '15 16:03 amercader

@florianm I had a look at this and it looks really good, see some comments below:

  • No need to deal with GeoAlchemy versions on this PR, this has been dealt with on master (#97)
  • Can we stick with the latest stable version for Leaflet (0.7.x) instead of 0.8-dev? Or do you need this specific version? BTW updating the libraries on vendor is fine
  • The changes in Leaflet.draw have introduced a bug on the spatial query widget, where the old bbox (eg from a previous search) is kept when drawing a new one:

g3go8b3

  • The new icon to draw a bbox seems a bit confusing to me. I think that the pencil one is more intuitive for users. I realize this is the default from the new Leaflet.draw version, but maybe it can be overridden with a config option.

voka1ny oj1uv58

  • Regarding the discussion you and @wardi had on https://github.com/open-data/ckanext-scheming/issues/11, my view is that this feature would be more useful to users if it was shipped on ckanext-spatial. The pattern followed by other widgets is to provide a single snippet that wraps all markup, libraries, etc and users only have to add one line to their templates. I can't see why this should not be the case for the extent editor as well. We will also avoid duplicating libraries in different extensions. That doesn't mean of course it shouldn't place nicely with ckanext-scheming. I'm still not familiar enough with how ckanext-scheming works, but it looks like using presets and/or form snippets could be an easy way for users to integrate.
  • AFAICT the spatial_form.html snippet requires you to manually add a spatial field elsewhere in the form. Let's make things even easier for users and add that field in the snippet as well so you don't have to worry about creating it. Also handling how to receive a previously added extent when editing, etc.
  • Related to the previous point, I might be wrong, but it seems to me that the widget assumes that the field used to store the geometry is a root field called spatial (ie that you are using a custom schema). If that is the case we should also consider the case where users just use a normal extra with a key spatial (or always assume this case)

Also if you can give me permissions to create a dataset on your sandbox site that would be great (user amercader)

amercader avatar Apr 07 '15 17:04 amercader

@amercader - apologies for the delay and thanks for your feedback! You have now admin access to http://data-demo.dpaw.wa.gov.au/organization/ckan-developers

I agree with all your points and will try and implement them, with the only exception being that I need leaflet-draw for the drawing functionality, which in turn requires Leaflet-0.8.

I'll probably need some advice around refactoring the code into a snippet later!

florianm avatar Apr 20 '15 07:04 florianm

This is amazing. Works well on 2.3 as is (merge conflicts notwithstanding). I'm using it with ckanext-scheming, with the relevant display and form snippets

@florianm @amercader would love to see this merged (as well as see a PR to ckanext-scheming), let me know if there is any way I can help.

deniszgonjanin avatar Jun 12 '15 19:06 deniszgonjanin

One small issue:

  • The new snippets/spatial_query.html does not clear the bbox between queries, so if you do multiple spatial queries with the widget, the extent only gets bigger and bigger and there is no way to clear it.

EDIT: Except the umm.. clear button. But I don't know if this was a desired effect? It seems the extent should be cleared between searches.

deniszgonjanin avatar Jun 12 '15 19:06 deniszgonjanin

Denis, Adria,

thanks for your feedback, very appreciated!

There are some issues with my PR:

  • I've changed more files than necessary as I got a little overexcited when trying to update third party libraries (Leaflet and Leaflet-draw)
  • The GeoAlchemy problem has been fixed
  • I provide three separate input methods, but did not data-bind all of them both ways to keep things simple
  • both you and Adria spotted the immortal "old search box" bug
  • I've written this PR late last year, so it's quite a few commits behind master now
  • Joel has a fourth, awesome "calculate extent from existing attached spatial resources" georeferencing method, although I couldn't find the code yet (could be proprietary)

I suggest to not merge this PR, but for me to try and re-implement it starting from the current master, trying to follow all feedback I've gotten so far. I will definitely run into problems - especially writing tests (can Selenium draw on maps?) where I'll have to ask you for advice :-)

I would value your input on functional specs - which additional use cases would you like to see implemented?

Cheers, Florian

On Sat, Jun 13, 2015 at 3:45 AM, Denis Zgonjanin [email protected] wrote:

One small issue:

  • The new snippets/spatial_query.html does not clear the bbox between queries, so if you do multiple spatial queries with the widget, the extent only gets bigger and bigger and there is no way to clear it.

— Reply to this email directly or view it on GitHub https://github.com/ckan/ckanext-spatial/pull/93#issuecomment-111597584.

florianm avatar Jun 13 '15 03:06 florianm

Is there a status update for this. It looks very promising and exactly what we're chasing. Thanks.

Stephen-Gates avatar Jun 15 '16 19:06 Stephen-Gates

@Stephen-Gates we've been using this version in production internally in my Department without problems (setup docs here), but unfortunately I haven't had resources to follow up on this PR.

One improvement apart from the suggestions in this thread is to look up geometries by name from an API endpoint, like data.gov.au does (but with polygons) - similar to CKAN's tag autocomplete.

florianm avatar Jun 16 '16 01:06 florianm

If someone is willing to take this one on and address the issues mentioned on the thread I'd be more than happy to review again and merge if ok.

amercader avatar Jun 17 '16 10:06 amercader