Code

Opened 10 months ago

Last modified 4 weeks ago

#21021 assigned Cleanup/optimization

Default geom_type attribute for GeometryWidget should be "Geometry", not "Unknown"

Reported by: leplatrem Owned by: EricBoersma
Component: GIS Version: 1.6-beta-1
Severity: Normal Keywords: minor
Cc: claudep Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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.

Attachments (0)

Change History (5)

comment:1 Changed 10 months ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 10 months ago by EricBoersma

  • Owner changed from nobody to EricBoersma
  • Status changed from new to assigned

comment:3 Changed 9 months ago by timo

  • Has patch set
  • Triage Stage changed from Accepted to 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.

comment:4 Changed 9 months ago by claudep

  • Triage Stage changed from Ready for checkin to 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 Changed 4 weeks ago by timo

  • Patch needs improvement set

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from EricBoersma to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.