Fixed #29232 -- Added title option for admin filter when providing field name #12837
Conversation
| if isinstance(list_filter, (tuple, list)): | ||
| # This is a custom FieldListFilter class for a given field. | ||
| field, field_list_filter_class = list_filter | ||
| field, second_param = list_filter | ||
|
|
||
| if isinstance(second_param, str): | ||
| # Second parameter is an override for the default title | ||
| field_list_filter_class = FieldListFilter.create | ||
| title = second_param | ||
| else: | ||
| # This is a custom FieldListFilter class for a given field. | ||
| field_list_filter_class = second_param |
There was a problem hiding this comment.
So it feels odd that you can provide a title or a custom FieldListFilter class, but not both. I think it should be possible to provide a 3-tuple to allow (field, class, title). The disadvantage is that is it not obvious given (field, class) and (field, title) whether it ought to be (field, class, title) or (field, title, class), so...
...perhaps a dict with keys field, class and title would be better instead as it would allow for further additional options that might be required in the future. We could keep the 2-tuple with the previous behaviour for backward compatibility.
Perhaps you could take this to the mailing list to get approval for a sane API for this?
There was a problem hiding this comment.
Thanks for the review!
It might seem unintuitive at first to provide either title or a custom FieldListFilter since these are two very different things, but I guess it makes sense. If you don't like the default name of a field (which is the verbose_name) you pass a title along with it in a tuple (field, title). If you have created a custom FieldListFilter then you can define the title with the class attribute title of your custom FieldListFilter and add the tuple (field, class) to list_filter. Since you have this class attribute I hardly see a case where 3-tuple (field, class, title) would be necessary. Maybe if you have one custom filter that you want to use multiple times with different titles.
Dictionaries are more verbose than tuples to define, but of course, there is a point where things get more complex and they are justified by better readability and clarity. Keeping the existing options with 2-tuples and adding the option to supply a dict is also an option.
I will take this discussion to the mailing list and let see if we can find a consensus there.
There was a problem hiding this comment.
Yes a custom class can have the title hard coded, but if creating a reusable custom list filter it would be nice to allow changing title in the same way.
Also, Django has some built-in filter classes such as RelatedOnlyFieldListFilter and the upcoming EmptyFieldListFilter. Surely we should be able to set the title when using these?
There was a problem hiding this comment.
To me the idea of this ticket is to give developers an easy way of changing the title when this is all they want to do. i.e. when they are otherwise happy with the default filter. If they wanted to use a different filter class, then they automatically know which class that is and can easily create a little subclass to set the title there.
| self.lookup_choices = self.field_choices(field, request, model_admin) | ||
| if hasattr(field, 'verbose_name'): | ||
| self.lookup_title = field.verbose_name | ||
| else: | ||
| self.lookup_title = other_model._meta.verbose_name | ||
| self.title = self.lookup_title | ||
| self.title = title or self.lookup_title |
There was a problem hiding this comment.
WDYT about adding or self.title here?
self.title = title or self.title or self.lookup_titleThis would add support for the following:
class MyStaffFilter(BooleanFieldListFilter):
title = "By can haz admin" # no effect :( overwritten in __init__
@admin.register(User)
class UserAdmin(UserAdminBase):
list_filter = (
("is_staff", MyStaffFilter),
)There was a problem hiding this comment.
(actually meant to comment this under FieldListFilter class, but applies here too)
|
@pdecol Thanks for you efforts! However, I agree with Nick that proposed API is really confusing. There is already a clear and easy way to customize a |
Previous work was done by @dsanders11 in #9793 and by @baldychristophe in #10788
Unfortunately, it seems that all the work on this ticket was already done, but it was never merged. So I thought I'd finish it. As requested in the last pull request, I rebased the branch on master and updated the release notes. I also added the title parameter to the EmptyFieldListFilter that was added meanwhile. I hope everything is up to date and fine now.