Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#22137 closed Cleanup/optimization (fixed)

Widgets class should drop the is_hidden bool property

Reported by: django@… Owned by: nobody
Component: Forms Version: dev
Severity: Normal Keywords:
Cc: alasdair@… 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

To me, it seems somewhat unnecessary, here are the reasons why:

  • In every place that is_hidden = True is set in forms/widgets.py, there is also a corresponding input_type = 'hidden' set. It seems like we should use one or the other for determining if a field is hidden (keep it DRY)
  • is_hiddendoesn't necessarily reflect whether the widget is actually an input of type type='hidden' or not
  • is_hidden cannot be set in the __init__() method, whereas input_type can (by passing an attrs={'type' : 'hidden' } dict)

The scenario where I came across this was that I wanted to make a DateInput widget hidden in one of my ModelForms. A basic example:

# Model
class MyMod(models.Model):
    date = models.DateField()

# Form
class MyModForm(forms.ModelForm):
    class Meta:
          widgets = {
                      'date' : forms.DateInput(format="%d/%m/%Y", attrs={'type' : 'hidden'}) # I need to use a DateInput here because a HiddenInput does not format the date properly (this is probably another bug in Django - the default date format is the US %m/%d/%y and it seems very difficult to change)
          }

# Template
{% if field.field.widget.is_hidden %}
{{ field }}
{% else %}
{{ field.label }} // custom html stuff {{ field }}

The call in the template does not work, since the DateInput has is_hidden = True. What I have to do in my template is:

{% if field.field.widget.is_hidden or field.field.widget.input_type == "hidden" %}

Not quite as elegant. If is_hidden were to be a method, which would return something like:

def is_hidden(self):
     return self.input_type == "hidden"

I think it would be a more reliable way of determining if a widget is hidden or not.

Attachments (1)

22137-1.diff (2.7 KB ) - added by Claude Paroz 11 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 by anonymous, 11 years ago

Sorry a typo. I should have said (4 lines from the end)

The call in the template does not work, since the DateInput has is_hidden = False, whereas in fact the DateInput will be hidden in HTML. What I have to do in my template is:

comment:2 by Claude Paroz, 11 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization
Version: 1.6master

by Claude Paroz, 11 years ago

Attachment: 22137-1.diff added

comment:3 by Claude Paroz, 11 years ago

Has patch: set

Attached patch passes tests. Opinion?

comment:4 by alasdair, 11 years ago

Removing is_hidden would be backwards incompatible, so would have to go through the deprecation cycle. I like the approach of turning it into a property.

comment:5 by Claude Paroz, 11 years ago

Backwards incompatibility does not automatically means a deprecation period, we might also simply document it in the release notes, depending on the severity.

comment:6 by alasdair, 11 years ago

is_hidden is documented. My understanding of the api stability promise is that we wouldn't remove it without a deprecation cycle unless there was a good reason.

My projects use {{ field.is_hidden }}, so I would have to update my templates if we removed it. I'm -1 on removing without deprecation warnings.

comment:7 by alasdair, 11 years ago

Cc: alasdair@… added

comment:8 by django@…, 11 years ago

My thought was to create a property as I said in the original ticket.

@cached_property
def is_hidden(self):
    return self.input_type == "hidden"

Since it's most likely to be used in templates, it won't make a difference even if it's not a property. Plus it would fail quietly if you were to remove it completely without deprecation warnings.
The patch looks good to me: backwards compatible, and does what I want ;-)

comment:9 by alasdair, 11 years ago

claudep's patch looks good to me, sorry if I wasn't clear before.

I saw "drop the is_hidden bool property" in the title, and thought you were suggesting "remove the attribute or turn it into a property". Re-reading the ticket, I can see that your suggested solution was always to create a method/property.

comment:10 by Claude Paroz, 11 years ago

Yes, but there's still a minor backwards incompatibility in that my patch doesn't allow the property to be set any longer. Thinking about it, we could even add a property setter so a deprecation warning is raised when some code tries to set the property (which was previously possible). Note also that we don't touch at all BoundField.is_hidden, but only Widget.is_hidden.

comment:12 by django@…, 11 years ago

Thanks claudep :)

Really looking forward to Django 1.7. Especially built in migrations!

Thanks everyone for your awesome work.

comment:13 by Tim Graham, 11 years ago

Triage Stage: AcceptedReady for checkin

comment:14 by Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: newclosed

In a19f0d0c1e128634b9e393c52148167bf8718b4c:

Fixed #22137 -- Made Widget.is_hidden a read-only property

Thanks django at patjack.co.uk for the report and the review.

comment:15 by Tim Graham <timograham@…>, 11 years ago

In 306283bf358470b7d439822af90051ac62e95bae:

Removed warning for Widget.is_hidden property.

refs #22137.

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