I cannot sort hierarchically by "Assignee", "Priority", "Status". Currently, when selecting e.g. "Sort results by: Assignee" on the query page, this gives a hierarchically sorted bug list. The hierarchy is remembered in the cookie "LASTORDER", which is set to "map_assigned_to.login_name%2C%20bugs.bug_status%2C%20bugs.priority%2C%20bugs.bug_id". I could now probably edit the sorting hierarchy by editing the cookie file, but really great would be an UI in query.cgi, like: "Sort results by: <field1>, then <field2>, ..." This UI would show the current cookie content and allow to edit it.

The UI can be tied to the Column Selection UI. If there was a set of pairs of drop-down boxes provided with all the fields, so that one could select first by Priority Ascending second by Target Milestone Ascending third by Severity Ascending etc. And provide for upto 10 sort columns. It would be also nice if a similar set of drop-down boxes was added to the Query page, so that one could store a default sort order for each query.

Enhancements which don't currently have patches on them which are targetted at 2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. Consideration will be taken for moving items back to 2.18 on a case-by-case basis (but is unlikely for enhancements)

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004. Anything flagged enhancement that hasn't already landed is being pushed out. If this bug is otherwise ready to land, we'll handle it on a case-by-case basis, please set the blocking2.20 flag to '?' if you think it qualifies.

The trunk is now frozen to prepare Bugzilla 2.22. Only bug fixes are accepted, no enhancement bugs. As this bug has a pretty low activity (especially from the assignee), it's retargetted to ---. If you want to work on it and you think you can have it fixed for 2.24, please retarget it accordingly (to 2.24).

*** Bug 342835 has been marked as a duplicate of this bug. ***

The code added in buglist.cgi is required because we have to convert column IDs to their name if we want Search.pm to order the list correctly, e.g. when sorting based on realnames. If a SQL fragment is stored in the cookie, that's not a big problem as Search.pm will extract its alias anyway, but I added some code to extract and store it ourselves. The remaining code is pretty straightforward. Note that due to the fact that DefineColumn() is located in buglist.cgi, I cannot use the LASTORDER cookie in colchange.cgi to set the default sortkeys as the meaning of aliases and column names is not accessible. We will be able to do that as soon as we have moved this function out of buglist.cgi (Field.pm should be used instead IMO).

The URL field contains a link to a screenshot very similar to what this patch implements. I updated the text at the top of the page meanwhile, but the functionality and the form remain the same.

Comment on attachment 234342 [details] [diff] [review] patch, v1 Okay, sorry for taking a while to review this. I thought this was a patch to change the order that columns appear in buglist.cgi, but in fact it's actually about sort orders of buglist results. Okay, instead of a complex UI in colchange.cgi, we just need two things: (1) Arrows next to each column on the buglist, showing if the list is currently sorted on that field, and if the sort is ascending or descending. (2) A way to reset the sort order so that the bugs are unsorted, or just making it so that clicking a third time on a column will make the sort no longer take that column into account. There are already patches that do this somewhere on a bug in Bugzilla. There are also patches that sort the buglist in JavaScript, so that we don't have to send it back to the server every time.

(In reply to comment #9) > (1) Arrows next to each column on the buglist, showing if the list is currently > sorted on that field, and if the sort is ascending or descending. This is only a partial solution, assuming you only want to sort against one column. This doesn't work if you want to sort your list based on several columns. And I don't like having a pure JS fix for this bug (we should always have a fix for browsers not using JS).

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set "blocking3.2" tp "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

Here is a patch that allows multiple-column bidirectional sorting by clicking on the column labels, similar to sorttable.js but done on the server. Sorting by bug id resets the order. It also addresses an issue with sorting by a field whose name is a substring of another field.

(In reply to comment #14) > Here is a patch that allows multiple-column bidirectional sorting by clicking > on the column labels, similar to sorttable.js but done on the server. We already have that upstream...don't we?

I'm curious about the status on this. It sounds like there is a fix. But the target milestone is v4.0. In Bugzilla 2.18, the search page would automatically 'accumulate' sort columns. For example if I sorted by Priority, and then sorted by Assignee, the search results would be sorted by Assignee then Priority. In Bugzilla 3.2, it seems the searching has gotten less powerful. It now sorts only on a single column. I'm not sure if this is a known bug, or part of this bug, or perhaps it is working as designed? I'm looking for some way to get the Bugzilla 2.18 behavior in my Bugzilla 3.2 installation. The Bugzilla 2.18 behavior supported multiple search columns, which was nice.

I've reported this issue as bug 470214.

(In reply to comment #15) > We already have that upstream...don't we? What we have upstream (Bugzilla 3.2, see bug 23473) is the ability to sort asc/desc by clicking the column title, but there is currently no visual indication (in Jesse's patch: arrows) which shows you the way columns are sorted. So this patch adds arrows, not the functionality itself. Jesse: IMO, gray arrows should have a number on their right indicating which position each column has to decide how to sort the buglist. When you have to gray arrows, you don't know which one takes precedence. pyrzak, what do you think?

Comment on attachment 345641 [details] [diff] [review] v2 Does this still apply? I'm sorry it took me so long to get to this review, I didn't really understand what the bug summary meant. :-) Anyhow, at the very least, don't use style="", use CSS classes.

I made sure Jesse's patch applies and works as intended while addressing mkanat's comment about style attribute. This also fixes bug 470214 so would be good thing to get in sooner than later.

Comment on attachment 376164 [details] [diff] [review] Add arrows to indicate sort order columns and direction, V2.1 I haven't tested this yet, but: >+[% BLOCK order_arrow %] >+ [% IF order.match("^${column.sortalias} desc") %] >+ &#x25BC; >+ [% ELSIF order.match("^${column.sortalias}(,|\$)") %] >+ &#x25B2; These should at least have some sort of span style, to indicate that they're the primary sort order--people might want to style them, even if we don't style them.

As you wish.

Comment on attachment 376191 [details] [diff] [review] Add arrows to indicate sort order columns and direction, V2.2 bitrot, unfortunately.

Unrotted and added removing of whitespace and a comment to the remove regexp inspired by LpSolit (the patch in bug 491467).

Comment on attachment 389325 [details] [diff] [review] Add arrows to indicate sort order columns and direction, V2.3 >Index: template/en/default/list/table.html.tmpl >+[% BLOCK new_order %] >+ [% SET desc = '' %] >+ [% IF (om = order.match("${id}( DESC)?")) %] >+ [% SET desc = ' DESC' IF om.0 != ' DESC' %] >+ [% END %] Nit1: SET is optional. Nit2: ${id} can simply be written as $id, that's cleaner. There is no reason to test om.0 against ' DESC'. If it exists, then it's necessarily ' DESC' as that's what we use in the regexp. Please just test for IF NOT om.0, that's clearer (else one may think that om.0 could be another string, which is wrong). Also, if om.0 is undefined, we may generate a "undef value" warning when used with != (despite I didn't see it in the log). >+ [% ELSIF order.match("${id} DESC") %] >+ <span class="bz_sort_order_secondary">&#x25BC;</span> >+ [% ELSIF order.match("${id}(,\s*|\$)") %] I'm pretty sure this is the reason of the error I'm seeing with your patch. You look for "$id something" (here too, please remove {}, they are useless), but you don't make sure there is a comma before it, and so you could catch a substring. To reproduce the problem, click on the Summary column header, then twice on the Comp header. You get a message that the "hort_desc" column doesn't exist. Otherwise, your patch looks good.

(In reply to comment #26) > I'm pretty sure this is the reason of the error I'm seeing with your patch. This problem has been fixed in bug 510944. You will have to update your patch accordingly.

Thanks for this patch! It is very useful. Also fixes Bug 515018 or Bug 510944 - so this is repair and tuning together. :)

wicked: ping? ETA for an updated patch?

Here you go, finally.

Once this passes code review (or possibly even before), could we see a screenshot of it before it gets approved?

(In reply to comment #31) > Once this passes code review (or possibly even before), could we see a > screenshot of it before it gets approved? Well, apply the patch and see yourself. :)

Comment on attachment 401717 [details] [diff] [review] Add arrows to indicate sort order columns and direction, V2.4 >Index: template/en/default/list/table.html.tmpl >+ [% PROCESS order_arrow id='bug_id' ~%] >+ [% PROCESS order_arrow ~%] Unless there is some TT syntax I don't know about, ~%] should be %] alone. >+ [% ELSIF order.match("$id DESC") %] >+ <span class="bz_sort_order_secondary">&#x25BC;</span> >+ [% ELSIF order.match("$id(,\\s*|\$)") %] >+ <span class="bz_sort_order_secondary">&#x25B2;</span> >+ [% END %] As I said in my previous review, these two regexps must start with \\b$id instead of $id to skip false positives in substrings. A good example is when both short_desc and short_short_desc columns are displayed. r=LpSolit with these two comments fixed.

(In reply to comment #33) > Unless there is some TT syntax I don't know about, ~%] should be %] alone. OK, mkanat pointed me to the TT doc where it indeed mentions ~%] as being valid. So forget this comment and just fix the two regexps on checkin.

(In reply to comment #33) > As I said in my previous review, these two regexps must start with \\b$id > instead of $id to skip false positives in substrings. A good example is when > both short_desc and short_short_desc columns are displayed. I tried to copy the relevant parts but couldn't see anything wrong happening without \\b check for these regexps when I was updating my patch. I just tried again and you are right, sometimes the order arrows are pointing wrong way after clicking a column when I don't have the \\b check. I also found out that sometimes the code gets ASC vs. DESC wrong for new sort order when the first order.match doesn't have that \\b check. Phew, regexps sure are simple to code. :) So I added \\b to all three order.match regexps before committing to tip: Checking in skins/standard/buglist.css; /cvsroot/mozilla/webtools/bugzilla/skins/standard/buglist.css,v <-- buglist.css new revision: 1.17; previous revision: 1.16 done Checking in template/en/default/list/table.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/list/table.html.tmpl,v <-- table.html.tmpl new revision: 1.48; previous revision: 1.47 done

Added to the release notes in bug 547466.