yourcelf / olwidget

Javascript library to replace textareas that contain WKT data with editable OpenLayers maps, and a django app using it for django forms and admin.
Other
91 stars 43 forks source link

v0.4 Map Widget _has_changed() method does not work [incl. Patch] #42

Closed tspng closed 13 years ago

tspng commented 13 years ago

Hi,

I' just noticed the same bug in v0.4 as I reported in issue #41 for v0.3. The Map Widget's _has_changed() method does not work correctly, which results in admin log changed entires for all geometry types, whether they have been changed or not.

The problem is that the initial data is a geometry object and the form data is a string in EWKT format which cannot be compared for equality by the widgets used in the Map's vector layers.

Here's a proposed patch for it. I'm not quite sure if it's the correct way to do it but it works in all cases of the supplied test_project:

diff -u -r old/src/django-olwidget/django-olwidget/olwidget/widgets.py new/src/django-olwidget/django-olwidget/olwidget/widgets.py
--- old/src/django-olwidget/django-olwidget/olwidget/widgets.py 2010-12-15 16:21:40.000000000 +0100
+++ new/src/django-olwidget/django-olwidget/olwidget/widgets.py 2010-12-15 16:21:23.000000000 +0100
@@ -125,7 +125,7 @@
         if (initial is None) or (not isinstance(initial, (tuple, list))):
             initial = [u''] * len(data)
         for widget, initial, data in zip(self.vector_layers, initial, data):
-            if widget._has_changed(initial, data):
+            if widget._has_changed(utils.get_ewkt(initial), data):
                 return True
         return False
yourcelf commented 13 years ago

Hi,

The included patch didn't work for me -- possibly due to differences in version/OS for the underlying OGR library that utils.get_ewkt uses (on my machine, I got a comparison between "POINT(0 0)" and "POINT (0 0)". I pushed a fix in https://github.com/yourcelf/olwidget/commit/5238f8d826a1d484e621cf5450beb1e417518736 which changes the has_changed comparison to use OGR instances instead of strings, which ought to be robust against those implementation details.

I also added a test case for this in olwidget/tests.py. If you continue to have trouble, please reopen with a failing test case.