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 Claude Paroz, 11 years ago

Triage Stage: UnreviewedAccepted

comment:2 by EricBoersma, 11 years ago

Owner: changed from nobody to EricBoersma
Status: newassigned

comment:3 by Tim Graham, 11 years ago

Has patch: set
Triage Stage: AcceptedReady 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.

comment:4 by Claude Paroz, 11 years ago

Triage Stage: Ready for checkinAccepted

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 Tim Graham, 11 years ago

Patch needs improvement: set

comment:6 by Olivier Tabone, 8 years ago

Cc: olivier.tabone@… added
Owner: changed from EricBoersma to Olivier Tabone

claim the ticket, working on it during duth sprint.

comment:7 by Olivier Tabone, 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 Olivier Tabone, 7 years ago

Owner: Olivier Tabone removed
Status: assignednew

comment:9 by Giannis Adamopoulos, 4 years ago

Owner: set to Giannis Adamopoulos
Status: newassigned

comment:10 by Mariusz Felisiak, 4 years ago

Has patch: set
Needs tests: set

comment:11 by Mariusz Felisiak, 4 years ago

Needs tests: unset
Triage Stage: AcceptedReady for checkin

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 7603036b:

Fixed #21021 -- Changed BaseGeometryWidget's default geometry type to 'Geometry'.

Note: See TracTickets for help on using tickets.
Back to Top