Code

Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#11868 closed New feature (fixed)

Multiple sort in admin changelist

Reported by: bendavis78 Owned by: bendavis78
Component: contrib.admin Version: master
Severity: Normal Keywords: admin sort multisort ordering order
Cc: andy@…, patrickk, idan@…, andybak Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

Description

I've made an attempt at adding multiple sort columns to the admin changelist. I'm basically putting this up here as an initial proposal of what I think would be a good starting point for a UI for multiple column sorting.

The idea is that each time you click on a header, it toggles between three states, "ascending", "descending", and "cleared". The order in which multiple columns are sorted are denoted by a small number next to the up/down icon. So (for example) to sort by ('last_name', 'first_name'), you click on the "last name" column, then the "first name" column. To change that to ('-last_name', 'first_name') you would click once more on the "last name" column. Clicking "last name" again would set the ordering to just ('first_name').

This patches cleanly against r10843. Testers are always appreciated.

Attachments (10)

django_admin_multisort.patch (8.9 KB) - added by bendavis78 5 years ago.
admin-multisort-1.2-beta-SVN-12755.diff (8.5 KB) - added by bendavis78 4 years ago.
Updated for 1.2-beta
11868.diff (8.5 KB) - added by jacob 3 years ago.
Updated to trunk (r16060)
11868_luke1.diff (11.7 KB) - added by lukeplant 3 years ago.
Part way implementation of latest proposal
icon_primary_sort.png (187 bytes) - added by lukeplant 3 years ago.
Icon needed for the above
11868_luke2.diff (13.1 KB) - added by lukeplant 3 years ago.
i18n and docs
11868_luke3.diff (15.3 KB) - added by lukeplant 3 years ago.
Fixed some existing tests, and bugs with handling columns invalid for sorting
11868_enhancements.diff (7.5 KB) - added by lukeplant 3 years ago.
Possible enhancements for review
django-changelist-header-bug.png (63.0 KB) - added by julien 3 years ago.
Changelist headers break when using custom ModelAdmin method with admin_order_field
Screenshot-Sortheaders.png (210.9 KB) - added by kmtracey 3 years ago.
Hopefully previewable version

Download all attachments as: .zip

Change History (53)

Changed 5 years ago by bendavis78

comment:1 Changed 5 years ago by bendavis78

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

Also, this is related to the following tickets; #389, #2234, #4926, #5673, #8682

comment:2 Changed 5 years ago by bendavis78

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

comment:3 Changed 4 years ago by jacob

  • Triage Stage changed from Unreviewed to Design decision needed

Changed 4 years ago by bendavis78

Updated for 1.2-beta

comment:4 Changed 3 years ago by andybak

  • Cc andy@… added

comment:5 Changed 3 years ago by patrickk

  • Cc patrickk added

comment:6 Changed 3 years ago by baumer1122

  • Severity set to Normal
  • Type set to New feature

comment:7 Changed 3 years ago by julien

  • Needs tests set
  • Patch needs improvement set

#11695 and #4926 were closed as duplicates.

The reason why the admin currently forces the use of just one field for ordering is merely for a lack of a better UI. And I think that the UI approach suggested here is the right one.

The patch needs to be updated to work with current trunk. It is also important to make it work if the ordering is changed via overriding the ModelAdmin queryset (see #7309).

comment:8 Changed 3 years ago by julien

  • Triage Stage changed from Design decision needed to Accepted

Changed 3 years ago by jacob

Updated to trunk (r16060)

comment:9 Changed 3 years ago by jacob

  • Easy pickings unset

I've updated this patch to apply to trunk, but only looked at it lightly. It still needs review, and tests, and maybe a glace at the CSS to make sure it'll work right cross-browser.

comment:10 Changed 3 years ago by jacob

After playing with this a bit more, I'm not entirely sold on the UX... clicking around I can't quite predict what my next click will do. I Like being able to sort by multiple fields, but it's very weird to me that a click on a second column doesn't resort.

I'm not sure about a better approach, but this a bit confusing as it stands. It *looks* OK, but it doesn't *act* right, at least to me.

comment:11 Changed 3 years ago by jacob

After some discussion on IRC, I have an alternate proposal:

If the developer specifies multiple ordering fields in your ModelAdmin, then you get multiple ordering with the UI as-is here. But if a user clicks on a header, he's back back to single ordering as it worked before. Additionally, there's a "restore default ordering" button (please, come up with a better name!) somewhere to bring back the original ordering.

This doesn't solve the issue completely, but it *does* fix the bug (multiple orderings are ignored), and it does it in a way that a future patch could add a more complex UI, perhaps cribbed from something like Excel or Numbers.

comment:12 Changed 3 years ago by idangazit

I'm happy with Jacob's proposal above. The main problem I have with the current patch is the header clicking.

To clarify Jacob's coment about the UI, if multiple column ordering is specified in ModelAdmin, the small-numbers-in-column-headers should be displayed. A "Reset sort ordering" button would need to be displayed for any model with a specified ordering, multicolumn or not. I'm not sure whether this button deserves a whole horizontal bar to itself below the search and admin actions, on one hand it is a waste of vertical space, on the other hand it feels kind of arbitrary to shoehorn it in with either search or admin actions.

comment:13 Changed 3 years ago by idangazit

  • Cc idan@… added

comment:14 Changed 3 years ago by julien

There is another approach illustrated in http://tablesorter.com/docs/#Examples

Basically, column headers only toggle between ascending/descending. You can then *select* multiple sorting columns by clicking the headers while pressing the shift key. The selected headers then have a different background colour. The big difference between this example and the admin is that in the admin the page would likely refresh after each click, which may be a bit disturbing. But maybe that could still work. What do you think?

comment:15 Changed 3 years ago by andybak

Is there a good reason not to follow existing conventions here? This might be my Windows background speaking but I always found Explorer's UI for this to be intuitive, discoverable and non-intrusive.

  1. Maintain a list of sort columns. i.e. [date, name, size]
  2. Every click on a column heading adds that item to the front of the queue (and removes it if it's already in the list)

From the users point of view, every click sorts in the way they would expect. Power users quickly learn that the previous sort is also preserved as the secondary sorting column. The UI only needs to indicate the primary sorting column.

comment:16 Changed 3 years ago by andybak

  • Cc andybak added

comment:17 Changed 3 years ago by lukeplant

@andybak: I think that suggestion is good in general, but has some problems:

1) You pay for additional sorting in terms of performance even if you don't need it. This is probably a minor consideration, I would live with it.

2) There needs to be a way to reset to the original sorting. If, on first view, it is sorted by something sensible (e.g. last name, first name), the user might change it, but then want to restore the original sorting without having to go back n pages and start again (especially if they have also done other filtering and searching). But with this proposal, it might not be obvious to them how to restore the sorting.

I think we do need the 'restore original sorting' link - but I think we need to find some UI space for it.

A suggestion for UI space: on the primary sorting column, we currently have an up/down arrow indicating sorting. We could add an additional icon to the left of this one which shows the advanced controls when you click on it. With andybak's suggestion, the advanced controls could just have:

  • a list showing the currently used sort fields
  • a 'restore original sort' link.

We could make it more advanced if necessary, but that would suffice for now.

I can't think of anywhere else to find space for this in the UI, because of the way we build up rows of controls i.e. search bar, date hierarchy bar, actions bar, and I really don't want a whole new bar just for this.

comment:18 Changed 3 years ago by idangazit

OK, Andy's sorting mechanic sounds fine (and better than the previous idea).

I also like Luke's UI suggestion -- it's more compact, and it doesn't feel shoehorned into the UI somewhere, plus it provides some explicit information about the sorting. No need to make it any more complex / advanced than this for now.

Changed 3 years ago by lukeplant

Part way implementation of latest proposal

Changed 3 years ago by lukeplant

Icon needed for the above

comment:19 Changed 3 years ago by lukeplant

I've added a patch which is nearly there. You'll need the icon_primary_sort.png in django/contrib/admin/media/img/admin/

I tried it without the numbers indicating sorting, but it was more confusing, so I left them in.

Some things that need fixing:

  • A better icon! I'm not brilliant at icon design at the best of times, and this was not the best of times...
  • The styling on the popup could probably be improved.
  • There is a tricky bug that is apparent if you, for example, go to the admin page for auth.Group and click on the header - you get a crash. The root cause seems to be that __str__ is being passed in somewhere, and it can't be resolved as a field. I've run out of time to debug this.
  • Possibly we'd want to change the popup so it showed '(ascending)' or '(descending)' next to each field. That would require adding in some translated strings somewhere, and also passing the information through from admin_list.py to the template.

I'm not sure what we want to do in terms of tests. The vast majority of this has to be tested by hand, or using some proper browser based testing like Selenium, for which we've got no infrastructure. The kind of unit tests we can write are a pain in the neck to write and add little value.

Changed 3 years ago by lukeplant

i18n and docs

Changed 3 years ago by lukeplant

Fixed some existing tests, and bugs with handling columns invalid for sorting

comment:20 follow-up: Changed 3 years ago by lukeplant

  • milestone set to 1.4
  • Needs tests unset
  • Patch needs improvement unset

The bug I mentioned turned out to be a separate bug in existing functionality, fixed in [16312]. I've uploaded an updated patch that addresses various issues neglected in my first patch.

Regarding showing 'ascending' or 'descending' on the popup, I don't know if I can come up with a method that is i18n friendly. I don't think we can assume that doing:

 * Field name (ascending)

is going to look right in all languages (does Arabic use brackets like that, for instance?). So it's probably best to leave that for now.

I've have tried and failed to write some sensible tests. You can write tests that test the implementation, rather than functionality, but that is not useful. If you try to test functionality i.e. click link on first column, then second column, you get into a regex parsing nightmare, or a DOM traversal nightmare. To code "click on the link inside the header than contains the text 'Field name'" succinctly and robustly, you need a library/infrastructure that Django's test suite just doesn't provide. I already found in [16313] that I had to remove a test because of fragility.

So, I'm removing 'needs tests'. Just awaiting some UI review now, and a better icon - help appreciated!

Last edited 3 years ago by lukeplant (previous) (diff)

comment:21 in reply to: ↑ 20 Changed 3 years ago by jezdez

Replying to lukeplant:

Regarding showing 'ascending' or 'descending' on the popup, I don't know if I can come up with a method that is i18n friendly. I don't think we can assume that doing:

 * Field name (ascending)

is going to look right in all languages (does Arabic use brackets like that, for instance?). So it's probably best to leave that for now.

Something like this would work (pseudo code, assuming there would be a way to get the order in the templates):

{% if header.asc %}
    {% blocktrans with fieldname=header.fieldname %}{{ fieldname }} (ascending){% endblocktrans %}
{% else %}
    {% blocktrans with fieldname=header.fieldname %}{{ fieldname }} (descending){% endblocktrans %}
{% endif %}

or (but this is less preferable since 'ascending' and 'descending' might be translated differently depending on the context):

{% blocktrans with fieldname=header.fieldname order=header.order %}{{ fieldname }} ({{ order }}){% endblocktrans %}

Either way the translators can manipulate the actual translation and RTL languages can handle it on their own.

Which reminds me, you might want to extend rtl.css to cope with the right aligning.

comment:22 Changed 3 years ago by julien

This is looking good. Could we get away by displaying the up/down arrow icons in the popup as well, instead of "(ascending) / (descending)" text? If not then the first approach mentioned by Jannis would be best.

For the icon, a cog might possibly do (it's commonly used for displaying extra options in a popup menu). You can find a sample in http://www.famfamfam.com/lab/icons/silk/

As for the tests, I think we should still add some to verify that Model.Meta.ordering and ModelAdmin.ordering are fully honoured.

Also, I haven't tested this thoroughly yet, but how would this all fit with custom querysets (see #7309)?

comment:23 Changed 3 years ago by lukeplant

Regarding RTL, I discovered #16144, and need to fix that first. (Review appreciated on that, to ensure I haven't got a broken setup here).

comment:24 Changed 3 years ago by lukeplant

OK, thanks for input from everyone. I believe I've got a patch that addresses all concerns raised:

  • I've gone with Jannis' suggestion regarding 'ascending/descending', because I don't think using icons in that popup is really helpful.
  • I've fixed it in RTL mode
  • I've added a basic test for multiple sorting. It doesn't test the URLs generated for each column header, for the reasons of parsing HTML that I described earlier.
  • Added tests for ModelAdmin and Meta being respected, in the right order.
  • #7309 - my patch now addresses that too, also adding tests for it - thanks for pointing out Julien.
  • I found a suitable cog image, good suggestion. The one I've used is perhaps a tad big, but I couldn't find one smaller, and it does give a better target for hitting than the dot I had before, and makes the feature more obvious.

BTW, I used a '.' instead of ',' in the query string, because the former is not escaped, and so looks much nicer in the address bar.

I'll commit shortly. Any other tweaks to UI styling can then be done more easily.

comment:25 Changed 3 years ago by lukeplant

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

In [16316]:

Fixed #11868 - Multiple sort in admin changelist.

Many thanks to bendavis78 for the initial patch, and for input from others.

Also fixed #7309. If people were relying on the undocumented default ordering
applied by the admin before, they will need to add 'ordering = -pk?' to
their ModelAdmin.

comment:26 Changed 3 years ago by jezdez

Ough, that icon is rather aweful! It really blows up the table header vertically, didn't you find a better image?

comment:27 Changed 3 years ago by lukeplant

Hmm, in the testing I thought it left the table header the same size.

Finding a nice icon is tricky - you can't just resize them at this small size, and it also has to be gif, for transparency support on IE6.

Here are a couple of alternatives I found/made:

http://lukeplant.me.uk/uploads/cog_icon.gif

http://lukeplant.me.uk/uploads/cog_icon2.gif

They are a bit smaller and hard to hit with the mouse, but look a bit nicer. I made them a bit wider than need to be, to increase the chance of hitting them.

comment:28 follow-up: Changed 3 years ago by apollo13

Is it just me or can I no longer filter by just the second column in the display, reset always resets to the first one, clicking on the second column then gives me a sorting by two columns…

comment:29 in reply to: ↑ 28 Changed 3 years ago by julien

Replying to apollo13:

Is it just me or can I no longer filter by just the second column in the display, reset always resets to the first one, clicking on the second column then gives me a sorting by two columns…

I presume that "Reset sorting" will reset the default sorting, which means it's no longer possible to sort by a single column other than the default one. Perhaps there should also be an option "Clear sorting" that would clear all sorting options including the default one.

Now that I've used it a bit more, I think one more way to improve the user experience is to also add a "Close" link/button in the popup, and also to make the popup close when pressing the "Esc" key.

I like the two small icons that you've suggested, Luke. I've got a slight preference for "cog_icon2.gif" as it's a bit more subtle.

comment:30 Changed 3 years ago by lukeplant

As I mentioned to Florian on IRC, there isn't really a need to sort on one column, as long as you can sort by your primary column. But I agree it could be a distraction/annoyance.

A 'clear sorting' option would be tricky in terms of how it would work, and easy to confuse with 'reset sorting'. An alternative is that in the popup there is an option to explicitly remove a field from the sorting. We could also add an option to explicitly toggle any of the existing fields. I'm not sure if these are refinements we can live without for now. With the current code, however, it isn't that hard to add them, now that I've got this in my head.

Changed 3 years ago by lukeplant

Possible enhancements for review

comment:32 Changed 3 years ago by lukeplant

I don't want to keep messing around with this, so I'll try to get feedback on this latest patch and then hopefully be done with it.

On top of the committed changes, this patch:

  1. Implements the ESC handling and Cancel button suggested by Julien
  2. Adds 'toggle' and 'remove' buttons for each field in the sorting list, thereby allowing to sort on just the desired columns (or no sorting,
  3. Switches to a table layout for the popup. This has the consequence of simplifying the i18n issues
  4. Adds styling consistent with the date picker popup (actually, got this almost for free with the table markup).

comment:33 Changed 3 years ago by julien

Wow, this is awesome! The popup looks great to me. I've found two issues though:

  • The popup doesn't show up when clicking on the cog in IE7 -- however this might be due to my crappy IE7 emulator on the Mac. It'd be worth testing on proper IE 6&7 setups.
  • I'm not sure if it's directly related to this patch, but the headers break when using a custom ModelAdmin method with the admin_order_field attribute. See the attached screenshot. Here's the code I've used:
from django.db import models

class Person(models.Model):
    name = models.CharField(max_length=100)
    age = models.PositiveIntegerField()
    is_employee = models.NullBooleanField()
from django.contrib import admin

from models import Person

class PersonAdmin(admin.ModelAdmin):
    list_display = ('name', 'age', 'is_employee', 'colored_name')

    def colored_name(self, obj):
        return '<span style="color: #%s;">%s</span>' % ('ff00ff', obj.name)
    colored_name.allow_tags = True
    colored_name.admin_order_field = 'name'
    
admin.site.register(Person, PersonAdmin)

Changed 3 years ago by julien

Changelist headers break when using custom ModelAdmin method with admin_order_field

comment:34 Changed 3 years ago by lukeplant

In [16319]:

Fixed various bugs related to having multiple columns in admin list_display with the same sort field

Thanks to julien for the report

Refs #11868

comment:35 Changed 3 years ago by lukeplant

In [16320]:

Improved UI for advanced sorting controls.

Now allows individual fields to be removed/toggled.

Refs #11868

comment:36 Changed 3 years ago by lukeplant

There was already a bug with multiple columns that have the same sort field, but the changes made it much worse. Fixing it properly requires one bit of code getting uglier, but another getting nicer. Done in [16319]

I've tested in proper IE6 and 7, and ironed out bugs (with some hacks). It's not exactly the same as other browsers, but good enough. Since these were major bugs, I just committed straight away - [16321]. Testing in IE8 and 9 would be appreciated - I don't have VMs for them.

I've also committed the more advanced UI - [16320]

Changed 3 years ago by kmtracey

Hopefully previewable version

comment:37 Changed 3 years ago by kmtracey

If the window is narrow enough that the added sort icons don't fit without wrapping the column header, I'm seeing a darker gray bar at the top of the header (see attached screenshot)...is that intentional? It looks odd to me...

comment:38 Changed 3 years ago by lukeplant

I can't reproduce that with most recent Firefox, Opera or Chrome. It looks like you are using Chrome - what version? I tested Chrome 11 and 13, and couldn't reproduce that issue. That makes it seem like an old Chrome bug that is fixed in recent versions. I'm not sure we'll be able to do anything about that, unless there is an obvious work around.

comment:39 Changed 3 years ago by lukeplant

Cancel that - with a larger icon I can reproduce this, on several browsers, so it is an issue.

comment:40 Changed 3 years ago by kmtracey

Hmm, screenshot was from Chromium 12.0.742.68 (86550) Ubuntu 10.04 but I am seeing similar behavior on Firefox (both 3.6.17 and 4.0.1). On further playing with it the behavior is not as consistent as I first thought -- the headers can wrap without the bar appearing but then when I add another sort column the bar may appear, it may disappear if I remove one or may not...I've not yet figured out the pattern to what makes it show up.

comment:41 Changed 3 years ago by lukeplant

Fixed in [16322]. The bug was really an old one - the existing way of changing the colour of those headers was a hack that worked if the cell wasn't too tall. I've done it properly now.

comment:42 Changed 3 years ago by kmtracey

Thanks Luke!

comment:43 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.