Opened 18 months ago

Closed 18 months ago

Last modified 17 months ago

#22137 closed Cleanup/optimization (fixed)

Widgets class should drop the is_hidden bool property

Reported by: django@… Owned by: nobody
Component: Forms Version: master
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 claudep 18 months ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 18 months ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 18 months ago by claudep

  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization
  • Version changed from 1.6 to master

Changed 18 months ago by claudep

comment:3 Changed 18 months ago by claudep

  • Has patch set

Attached patch passes tests. Opinion?

comment:4 Changed 18 months ago by alasdair

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 Changed 18 months ago by claudep

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 Changed 18 months ago by alasdair

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 Changed 18 months ago by alasdair

  • Cc alasdair@… added

comment:8 Changed 18 months ago by django@…

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 Changed 18 months ago by alasdair

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 Changed 18 months ago by claudep

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 Changed 18 months ago by django@…

Thanks claudep :)

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

Thanks everyone for your awesome work.

comment:13 Changed 18 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

comment:14 Changed 18 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

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 Changed 17 months ago by Tim Graham <timograham@…>

In 306283bf358470b7d439822af90051ac62e95bae:

Removed warning for Widget.is_hidden property.

refs #22137.

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