#9321 closed Bug (fixed)
When overriding the widget for a manytomany field default help_text stays
Reported by: | mrcai | Owned by: | nobody |
---|---|---|---|
Component: | Forms | Version: | 1.4 |
Severity: | Normal | Keywords: | manytomanyfield, widget, related.py, admin |
Cc: | carlos.palol@…, domen@…, dguardiola@…, James Pic, Ivan Virabyan | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | yes | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | yes |
Description
When you override the widget for a manytomany field, the default helper_text "Hold down "Control", or "Command" on a Mac, to select more than one" remains. Which can be a little misleading.
In models.py I define a model Profile with:
favorite_genres = models.ManyToManyField(Genre, verbose_name='Favorite Genres',)
In forms.py I define a form ProfileEditForm(ModelForm):
class Meta: model = Profile
In my views.py I override the widget using:
ProfileEditForm.base_fields['favorite_genres'].widget = CheckboxSelectMultiple( choices=ProfileEditForm.base_fields['favorite_genres'].choices)
And it gets rendered as seen in the screen shot.
I guess the issue is with line 761 in django.db.models.fields.related
msg = ugettext_lazy('Hold down "Control", or "Command" on a Mac, to select more than one.')
I'm not really sure about how to go about patching this, so I'll just flag it if that's ok?
(My first ticket, hope I've done it right. I've put this in Database layer based on it being related.py that's going to need a patch)
Attachments (6)
Change History (59)
by , 16 years ago
Attachment: | manytomany.tiff added |
---|
comment:1 by , 16 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:2 by , 16 years ago
Component: | Database layer (models, ORM) → Forms |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:3 by , 16 years ago
Resolution: | invalid |
---|---|
Status: | closed → reopened |
Malcolm, did you mean to reopen this too?
comment:4 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Apparently not. I missed Julien's comment. He's correct.
comment:5 by , 16 years ago
Actually, julien is incorrect
if you check the file that mrcai reported, the ManyToManyField makes the assumption than django forms will use
SelectMultiple widget and concatenate the help_text of the model-field with the msg variable.
msg = ugettext_lazy('Hold down "Control", or "Command" on a Mac, to select more than one.')
However, if you'd change the default widgets for MultipleChoiceFields (Model and plain) to CheckboxSelectMultiple, from for instance
your settings file.
forms.MultipleChoiceField.widget = forms.widgets.CheckboxSelectMultiple forms.ModelMultipleChoiceField.widget = forms.widgets.CheckboxSelectMultiple
A forms field will now have concatenated the models help_text with the msg and you will end up
with a checkboxes and a misleading help_text (Hold down control....).
And as mrcai pointed out, the bug is in django.db.models.fields.related.py.
In my opinion this is a bug as the text should be appended by the widget and not by the db model field as it currently is. Please
verify your assumptions that it has nothing to do with the model field before closing this again, because it does.
comment:6 by , 16 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:7 by , 14 years ago
Needs documentation: | set |
---|---|
Version: | 1.0 → 1.2 |
I ran into this very issue just now and solved it by creating an UnhelpFullManyToManyField that overrides the init method (actually it just sets the help_text to an empty string)
Personally, I think the last two lines of the init should look something like:
if not self.help_text or self.help_text=="":
self.help_text = _('Hold down "Control", or "Command" on a Mac, to select more than one.')
This would change the behaviour though, and needs to be documented. It makes more sense to me, because this will act as a 'default' value that the user can still override if he wants to. He'll then loose the original help_text, but it's easy enough to copy/paste the string.
comment:8 by , 14 years ago
I agree with Usip.
The help text should be attached to a widget ( view ) and not to the model. ( In my vision of the mvc/mvt )
It makes totally nonsense to me to attach it to the model. Moreover if we can change the widget...
regards.
Gorghoa
comment:9 by , 14 years ago
Summary: | When overriding the widget for a manytomany field default helper text stays → When overriding the widget for a manytomany field default help_text stays |
---|
comment:10 by , 14 years ago
Is there update for this issue? The workarounds are all pretty annoying
comment:11 by , 14 years ago
Component: | Forms → Database layer (models, ORM) |
---|---|
Has patch: | set |
Keywords: | admin added |
Attached an updated version of the patch from #12359 by trey@… which moves the assignment of the help_text from django.db.models.fields.related to django.contrib.admin.options.
I had begun making a patch to move the help_text assignment to django.forms.widgets.SelectMultiple, but in doing so I realized that this, too, is poor behavior. The help_text property is expected to be in direct control of the application; in no situation should it be modified by the framework.
comment:12 by , 14 years ago
milestone: | → 1.4 |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Accepted → Design decision needed |
I'm marking as DDN since we still need to figure out the best approach for amending the help text based on which widget is used. I strongly feel that each widget should be able to control what help text gets displayed. I haven't given this enough thoughts yet, but perhaps it could be along the lines of:
# In django/forms/widgets.py class Widget(object): ... def get_help_text(self, text): """ Hook to allow a widget to amend its help text """ return text class SelectMultiple(Select): def get_help_text(self, text): return string_concat(text, ' ', _('Hold down "Control", or "Command" on a Mac, to select more than one.'))
... and then call the get_help_text()
method at the right time either when building or rendering the form.
See also #14402 for a tightly linked issue. I'd recommend solving both tickets at once.
I'd really like to see this fixed in 1.4.
comment:13 by , 14 years ago
Component: | Database layer (models, ORM) → Forms |
---|
by , 14 years ago
Attachment: | 9321+14402.ManyToManyField-help-text.diff added |
---|
comment:14 by , 14 years ago
Easy pickings: | unset |
---|---|
Needs documentation: | unset |
Needs tests: | unset |
Patch needs improvement: | unset |
Severity: | → Normal |
Type: | → Bug |
The attached patch solves two tickets at once, i.e. this one and #14402. The reason is that the bug fixes for those two tickets both rely on the introduction of the same new method Widget.alter_help_text
. If this approach is validated then I'll break the patch in two, and #14402 can be checked in after this one.
The idea behing alter_help_text
is to allow the widget to customize the form field's help text -- for example SelectMultiple
would now be responsible for appending the 'Hold down "Control"
' message instead of letting ManyToManyField
do it. This new feature introduces a backwards incompatible change, although the upgrade path is really simple. See the doc in the patch for more info about that.
Let me know what you think!
comment:15 by , 14 years ago
Patch needs improvement: | set |
---|
Yeah, I really think this callback API is odd, I don't like it at all, why not just have a extra_help_text
class attribute in a widget that is appended if it exists?
comment:16 by , 14 years ago
Well, this callback would allow the widget to operate more changes that just append some text, for example prepend some text or replace it altogether. A bit like the way the ModelAdmin.save_model()
hook works. If you think that's a YAGNI and that we should only allow appending text, then a extra_help_text
class attribute would work. Note that it would also be backwards compatible (which is cool, I think, if we want to get it right).
follow-up: 24 comment:17 by , 14 years ago
Just following on your comment, I see your point about the oddness of that callback. It felt similarly odd to me when the ModelAdmin.save_model()
and ModelAdmin.delete_model()
hooks were introduced. I think the oddness is the price to pay for more flexibility and future-proofness. So I'm a bit ambivalent about this. I'll resubmit a patch using the extra_help_text
class attribute so we can compare approaches.
comment:18 by , 14 years ago
I also don't like the fact this is adding the default help text for all SelectMultiple
widgets, as noted by the requirement to change current tests.
comment:19 by , 14 years ago
@SmileyChris: Yes, this is a manifestation of the backwards incompatible change of this patch. The "Hold down Control" message only makes sense when used with a <select>
widget where multiple items can be selected, i.e. with SelectMultiple
. So, had the behaviour been proper, then that message should have always been there in the tests, not just when used in the context of a ManyToManyField
. I see this change more as way of re-establishing the correct behaviour. What do you think?
comment:20 by , 14 years ago
I'd prefer to minimize the backwards compatibility of the patch. I don't really want people to have to hunt through and modify every instance of SelectMultiple
they've used to continue to have their help_text unmolested.
comment:21 by , 14 years ago
Er, minimize the backwards incompatibility...
(edit: and now I remember we have an edit comment button now)
comment:22 by , 14 years ago
On further reflection, one radical approach would be to remove the 'Hold down Control' extra message altogether, and let the developers directly add some extra information, if they see fit, using the traditional way via the model field's or form field's help_text
attributes. The use of that extra message is actually quite debatable, and it has caused a lot of confusion for years from an implementation standpoint.
Anyways, just a thought.
comment:23 by , 14 years ago
OK, thank you all for your feedback. I'll try to summarize the problem and discussions so far. So, an ideal solution would:
- stop
ManyToManyField
from directly appending the 'Hold down Control' message, since this has nothing to do with ORM-level business. - allow non-standard widgets and widgets inheriting from
SelectMultiple
to not append the 'Hold down Control' message when used in conjunction with aManyToManyField
. - be backwards compatible as it would potentially affect a lot of people:
- When the default
SelectMultiple
widget, or any widget inheriting fromSelectMultiple
, is not used in conjunction with aManyToManyField
, then do not append the message. - When the default
SelectMultiple
is used in conjunction with aManyToManyField
, then do append the message.
- When the default
A compromise needs to be found between doing things right and not annoying too many people with a potentially backwards incompatible change. The challenge is that, as much as ManyToManyField
should not be allowed to guess which widget will be used, the SelectMultiple
widget should also not trying to guess that it's used with a ManyToManyField
. Maybe the compromise lies in the form field's realm, in particular ModelMultipleChoiceField
.
Finally, a bit of forensic research reveals this behaviour was introduced at least at the time of the magic-removal branch merge: http://code.djangoproject.com/browser/django/trunk/django/db/models/fields/related.py?rev=2809#L580
follow-up: 25 comment:24 by , 14 years ago
Replying to julien:
Just following on your comment, I see your point about the oddness of that callback. It felt similarly odd to me when the
ModelAdmin.save_model()
andModelAdmin.delete_model()
hooks were introduced. I think the oddness is the price to pay for more flexibility and future-proofness. So I'm a bit ambivalent about this. I'll resubmit a patch using theextra_help_text
class attribute so we can compare approaches.
Honestly this is not a good comparison at all, ModelAdmin
is at a completely different abstraction layer; users are encouraged to write them while Widgets (especially the SelectMultiple) not so much.
comment:25 by , 14 years ago
Replying to jezdez:
Honestly this is not a good comparison at all,
ModelAdmin
is at a completely different abstraction layer; users are encouraged to write them while Widgets (especially the SelectMultiple) not so much.
The comparison was just purely about the hook implementation paradigm, nothing more. I've actually had to write custom widgets on many occasions for the admin (in particular for the ManyToManyField
), which is why I've always been keen to fix this ticket :-)
comment:26 by , 13 years ago
UI/UX: | set |
---|
comment:28 by , 13 years ago
I just ran into this and am having trouble finding a workaround. Can someone list the preferred workaround until the bug is resolved?
comment:30 by , 13 years ago
Cc: | added |
---|
comment:31 by , 13 years ago
I solved this with:
class mymodelform(ModelForm):
class Meta:
pass
def init(self, *args, kwargs):
super(RegisterForm, self).init(*args, kwargs)
self.fieldsstaying.help_text =
self.fieldsfamily_members.help_text =
don't know if this is 'kosher' but it seems to work for me!
comment:32 by , 13 years ago
and w/ formatting and me logged in
I solved this with:
class mymodelform(ModelForm): class Meta: pass def __init__(self, *args, **kwargs): super(RegisterForm, self).__init__(*args, **kwargs) self.fields['staying'].help_text = '' self.fields['family_members'].help_text = ''
don't know if this is 'kosher' but it seems to work for me!
by , 13 years ago
Attachment: | hold-control-selectmultiple-fix.patch added |
---|
_patch_help_text() function and calls for ChoiceField and subsclasses
comment:33 by , 13 years ago
Needs tests: | set |
---|---|
Resolution: | → fixed |
Status: | reopened → closed |
Version: | 1.2 → 1.4 |
Please accept my apologies for this patch if the format is wrong or something similar.
I think I've fixed this bug in a fairly simple way.
The thing that is at issue here is the addition of widget-related help-text by the Database field object (ManyToManyField) instead of the Form field object (MultipleChoiceField, ModelMultipleChoiceField, TypedChoiceField etc.)
The patch does the following :
- remove the 2 lines that add the message in django.db.models.related.ManyToManyField.init
- add a private method to django.forms.fields.ChoiceField field called _patch_help_text().
- add calls to this method in the init of MultipleChoiceField and ModelMultipleChoiceField
The _patch_help_text() method, when called :
- checks the widget on the form and updates the help text if the widget is a SelectMultiple.
- uses type() to check self.widget so the message isn't printed for subclasses of SelectMultiple
What do u think? Sorry, I haven't read up on writing tests yet, so no tests. But, the goal was simple, show the message for SelectMultiple, suppress it all other times.
by , 13 years ago
Attachment: | hold-control-selectmultiple-fix.2.patch added |
---|
patch described in comment 33
comment:34 by , 13 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
A ticket is closed when the patch is applied, not when the patch is provided. Reopening.
comment:36 by , 13 years ago
Cc: | added |
---|
comment:37 by , 12 years ago
Cc: | added |
---|
comment:38 by , 12 years ago
Cc: | added |
---|
comment:40 by , 12 years ago
I keep running into this issue. It's a really obnoxious test that certain does not belong in ManyToManyField. It belongs in widgets if anywhere. But why add this to every Many To Many? We don't show "Press Enter to submit" help texts when displaying forms?
This functionality assumes a HTML select widget, and nowadays much more different widget styles (scripted and otherwise) are available.
Basically, the text is a piece of hardcoded UI that should be completely removed instead of toned down. The only reason not to remove this is backward compatibility. If removing it is too big a step, it should be moved to SelectMultiple widget.
comment:41 by , 12 years ago
Status: | reopened → new |
---|
comment:42 by , 12 years ago
1) As many people have stated (in various duplicate tickets) the help text should not be in the database layer
2) Also, overriding the help text from the model definition has no effect - so putting the help text in the database layer is breaking the model definition
This has been going on for 4 years now, without resolution, please allow me to propose a different approach:
Firstly it's a bug, there is no need to maintain any backwards compatibility. Why don't we delete it from the database layer and then opening a new ticket that states that the widget does not display the help text. That way at least just the widget is broken, currently the widget and the database layer is broken.
comment:44 by , 12 years ago
Triage Stage: | Design decision needed → Accepted |
---|
comment:45 by , 12 years ago
Adding my support to absolutely removing this from the database layer. I keep running into this. And sometimes it's just plain ridiculous (for example when using CheckboxSelectMultiple
for a ManyToMany.
comment:46 by , 12 years ago
This is the single most annoying bug on django, if dyve won't fix it I will. For now I'll wait.
comment:47 by , 12 years ago
I've posted a proposal to the django-dev mailing list: https://groups.google.com/forum/?hl=en&fromgroups#!topic/django-developers/PqfHA3hvAxo
Feedback welcome.
comment:48 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:52 by , 10 years ago
will this be implemented in 1.8? the ticket says 1.4 and the "press ctrl..."-help text is still appended to any custom text.
comment:53 by , 10 years ago
Please look at the commits above for details. The Version field on the ticket merely indicates what the current version was when the ticket was reported.
help_text
is a field attribute and is independent from the widget. So, what you'd need to do is something like: