Opened 12 years ago
Closed 10 years ago
#19139 closed Bug (fixed)
OpenLayersWidget's 'Delete all Features' control doesn't respect GeoModelAdmin's modifiable attribute
Reported by: | Flavio Curella | Owned by: | Claude Paroz |
---|---|---|---|
Component: | GIS | Version: | dev |
Severity: | Normal | Keywords: | gis admin template |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | yes |
Description
When GeoModelAdmin
's
modiable
attribute is set to False, the controls for editing the geometry are not present on the change object page, but the 'Delete all Features' control is still there.
Attachments (3)
Change History (20)
comment:1 by , 12 years ago
Summary: | ``OpenLayersWidget``'s 'Delete all Features' control doesn't respect ``GeoModelAdmin``'s ``modifiable`` attribute → OpenLayersWidget's 'Delete all Features' control doesn't respect GeoModelAdmin's modifiable attribute |
---|
by , 12 years ago
Attachment: | patch1.diff added |
---|
comment:2 by , 12 years ago
Has patch: | set |
---|
comment:3 by , 12 years ago
UI/UX: | set |
---|
comment:4 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:5 by , 12 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
by , 12 years ago
Attachment: | patch2.diff added |
---|
comment:6 by , 12 years ago
Needs tests: | unset |
---|
@claudep: I was testing 'wkt' as a way to determine if the geometry was new or if it was an already existing one that needs to modified.
After running some tests I've realized that that's the wrong attribute to check. On my new patch I pass the field's value to the template and I'm checking directly against that.
comment:7 by , 12 years ago
Oh, now I understand that modifiable allows to create a feature, but not modify it afterwards. I didn't get it at first, and the docs are not clear either about this. This is something we should also fix.
Could you also enlighten me about the difference between testing wkt or value? (it's already late :-) )
comment:8 by , 12 years ago
wkt
is 'a string representation of the geometry in WKT format' (https://docs.djangoproject.com/en/1.4/ref/contrib/gis/gdal/#django.contrib.gis.gdal.OGRGeometry.wkt), while value
is the actual python object, whose __str__
(or __unicode__
) method returns something like POLYGON((0 0, 5 0, 5 5, 0 5))
.
I'd rather use value
because wrt
involves some geographical transformation, and could result in being ''
due to errors while converting. (See https://github.com/django/django/blob/master/django/contrib/gis/admin/widgets.py#L67)
comment:9 by , 12 years ago
Flavio, sorry for such a long time without news. I revisited this issue today, and I think we should set the modifiable
param to True when the widget has no existing value (instead of adding the value). If you can add a new feature, there are no reason you cannot edit it before saving. Thoughts?
comment:10 by , 12 years ago
@claudep
if i'm reading your comment correctly, are you proposing to have the editing tools (including the 'delete all features' button) only on the 'add_view', but not in the 'change_view'?
That would make sense, since 'modifiable' has a different meaning than 'editable'. I think we may need to make this semantic difference more explicit in the docs.
comment:11 by , 12 years ago
sorry, I see what you're saying now. you're proposing to set self.params['modifiable'] = True
instead of self.params['value'] = value
in widgets.py, and then just use {% if modifiable %}
in template, right?
If that's the case, I'm conflicted. On one hand, that's a simpler implementation. On the other hand, feels like hijacking a value explicitly set by the user. Maybe we could set self.params['editable'] = True
and check against that?
comment:12 by , 12 years ago
modifiable
has already the meaning: can add new value, cannot edit existing value. Quoting the docs:
Defaults to True
. When set to False
, disables editing of existing geometry fields in the admin.
So we are simply pushing that logic a bit further down to the JS code.
by , 12 years ago
Attachment: | patch3.diff added |
---|
comment:15 by , 10 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Claude, as someone with more experience with GIS, could you check this?
comment:16 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:17 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Why do you test
wkt == ''
in your patch?