Opened 11 years ago
Closed 4 years ago
#21021 closed Cleanup/optimization (fixed)
Default geom_type attribute for GeometryWidget should be "Geometry", not "Unknown"
Reported by: | Mathieu Leplatre | Owned by: | Giannis Adamopoulos |
---|---|---|---|
Component: | GIS | Version: | 1.6-beta-1 |
Severity: | Normal | Keywords: | minor |
Cc: | Claude Paroz, olivier.tabone@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The default value for geom_type is "Unknown" whereas we expect it to be "Geometry".
Indeed, by default we rely on GDAL to set the geom_type attribute, and it returns an unexpected value :
>>> from django.contrib.gis import gdal >>> str(gdal.OGRGeomType('GEOMETRY')) 'Unknown'
In fields, we override explicitly his value, so the problem is hidden.
https://github.com/django/django/blob/1.6b2/django/contrib/gis/forms/fields.py#L42
But IMHO the widget should behave nicely with default values, for example instantiated like this :
class BigMapWidget(BaseGeometryWidget): map_height = 1200 class GeomForm(forms.Form): def __init__(self, *args, **kwargs): super(GeomForm, self).__init__(*args, **kwargs) self.fields['geom'].widget = BigMapWidget()
Of course, this problem only applies to generic Geometry field types, and those are commonly used.
Change History (12)
comment:1 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 11 years ago
Has patch: | set |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:4 by , 11 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
Eric, can you explain where the 102
number comes from in your patch? I'm not sure changing the OGR semantic is a good idea. I think we'd rather solve this in the forms/widgets layer. But I'm open to any other argument.
comment:5 by , 11 years ago
Patch needs improvement: | set |
---|
comment:6 by , 8 years ago
Cc: | added |
---|---|
Owner: | changed from | to
claim the ticket, working on it during duth sprint.
comment:7 by , 8 years ago
Has patch: | unset |
---|---|
Patch needs improvement: | unset |
Looking at GDAL documentation, there is no OGR type matching 'POLYGON'. enum OGRwkbGeometryType
Hence the '102' magic number in the patch does not match OGR specification.
I agree with Claude we should not change OGR semantics. returning wkbUnknown for an empty geometry seems correct according to GDAL.
removed 'has patch' flag because the pull request has been closed quite a long time ago by tim.
comment:8 by , 7 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:9 by , 4 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:11 by , 4 years ago
Needs tests: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Eric, don't forget to link to your PR from the ticket and check "Has patch" so it's picked up for review.
Here's Eric's PR. It looks good to me but I'll let Claude or something more familiar with GeoDjango do a final review.