Opened 15 years ago

Closed 13 years ago

Last modified 13 years ago

#11868 closed New feature (fixed)

Multiple sort in admin changelist

Reported by: Ben Davis Owned by: Ben Davis
Component: contrib.admin Version: dev
Severity: Normal Keywords: admin sort multisort ordering order
Cc: andy@…, Patrick Kranzlmueller, idan@…, Andy Baker Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

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 Ben Davis 15 years ago.
admin-multisort-1.2-beta-SVN-12755.diff (8.5 KB ) - added by Ben Davis 15 years ago.
Updated for 1.2-beta
11868.diff (8.5 KB ) - added by Jacob 14 years ago.
Updated to trunk (r16060)
11868_luke1.diff (11.7 KB ) - added by Luke Plant 13 years ago.
Part way implementation of latest proposal
icon_primary_sort.png (187 bytes ) - added by Luke Plant 13 years ago.
Icon needed for the above
11868_luke2.diff (13.1 KB ) - added by Luke Plant 13 years ago.
i18n and docs
11868_luke3.diff (15.3 KB ) - added by Luke Plant 13 years ago.
Fixed some existing tests, and bugs with handling columns invalid for sorting
11868_enhancements.diff (7.5 KB ) - added by Luke Plant 13 years ago.
Possible enhancements for review
django-changelist-header-bug.png (63.0 KB ) - added by Julien Phalip 13 years ago.
Changelist headers break when using custom ModelAdmin method with admin_order_field
Screenshot-Sortheaders.png (210.9 KB ) - added by Karen Tracey 13 years ago.
Hopefully previewable version

Download all attachments as: .zip

Change History (53)

by Ben Davis, 15 years ago

comment:1 by Ben Davis, 15 years ago

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

comment:2 by Ben Davis, 15 years ago

Owner: changed from nobody to Ben Davis
Status: newassigned

comment:3 by Jacob, 15 years ago

Triage Stage: UnreviewedDesign decision needed

by Ben Davis, 15 years ago

Updated for 1.2-beta

comment:4 by Andy Baker, 14 years ago

Cc: andy@… added

comment:5 by Patrick Kranzlmueller, 14 years ago

Cc: Patrick Kranzlmueller added

comment:6 by Peter Baumgartner, 14 years ago

Severity: Normal
Type: New feature

comment:7 by Julien Phalip, 14 years ago

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 by Julien Phalip, 14 years ago

Triage Stage: Design decision neededAccepted

by Jacob, 14 years ago

Attachment: 11868.diff added

Updated to trunk (r16060)

comment:9 by Jacob, 14 years ago

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 by Jacob, 14 years ago

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 by Jacob, 14 years ago

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 by Idan Gazit, 14 years ago

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 by Idan Gazit, 14 years ago

Cc: idan@… added

comment:14 by Julien Phalip, 14 years ago

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 by Andy Baker, 14 years ago

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 by Andy Baker, 14 years ago

Cc: Andy Baker added

comment:17 by Luke Plant, 14 years ago

@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 by Idan Gazit, 14 years ago

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.

by Luke Plant, 13 years ago

Attachment: 11868_luke1.diff added

Part way implementation of latest proposal

by Luke Plant, 13 years ago

Attachment: icon_primary_sort.png added

Icon needed for the above

comment:19 by Luke Plant, 13 years ago

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.

by Luke Plant, 13 years ago

Attachment: 11868_luke2.diff added

i18n and docs

by Luke Plant, 13 years ago

Attachment: 11868_luke3.diff added

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

comment:20 by Luke Plant, 13 years ago

milestone: 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 13 years ago by Luke Plant (previous) (diff)

in reply to:  20 comment:21 by Jannis Leidel, 13 years ago

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 by Julien Phalip, 13 years ago

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 by Luke Plant, 13 years ago

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 by Luke Plant, 13 years ago

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 by Luke Plant, 13 years ago

Resolution: fixed
Status: assignedclosed

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 by Jannis Leidel, 13 years ago

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

comment:27 by Luke Plant, 13 years ago

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 by Florian Apolloner, 13 years ago

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…

in reply to:  28 comment:29 by Julien Phalip, 13 years ago

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 by Luke Plant, 13 years ago

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.

by Luke Plant, 13 years ago

Attachment: 11868_enhancements.diff added

Possible enhancements for review

comment:32 by Luke Plant, 13 years ago

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 by Julien Phalip, 13 years ago

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)

by Julien Phalip, 13 years ago

Changelist headers break when using custom ModelAdmin method with admin_order_field

comment:34 by Luke Plant, 13 years ago

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 by Luke Plant, 13 years ago

In [16320]:

Improved UI for advanced sorting controls.

Now allows individual fields to be removed/toggled.

Refs #11868

comment:36 by Luke Plant, 13 years ago

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]

by Karen Tracey, 13 years ago

Attachment: Screenshot-Sortheaders.png added

Hopefully previewable version

comment:37 by Karen Tracey, 13 years ago

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 by Luke Plant, 13 years ago

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 by Luke Plant, 13 years ago

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

comment:40 by Karen Tracey, 13 years ago

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 by Luke Plant, 13 years ago

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 by Karen Tracey, 13 years ago

Thanks Luke!

comment:43 by Jacob, 13 years ago

milestone: 1.4

Milestone 1.4 deleted

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