User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040616 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040616 The primary sort order key is not indicated in the buglist output. A prior versions of Bugzilla would italicize the column heading of the column used for the primary sort key in the output. The current version 2.18rc2 does not do this. Reproducible: Always Steps to Reproduce:

Reassigning bugs that I'm not actively working on to the default component owner in order to try to make some sanity out of my personal buglist. This doesn't mean the bug isn't being dealt with, just that I'm not the one doing it. If you are dealing with this bug, please assign it to yourself.

Adding a patch that shows the sort director by a small arrow next to the columns header. Clicking on the header toggles the sort direction, i.e. sorting descending becomes ascending after click etc. To install, please apply the attached patch to buglist.cgi and template/en/default/list/table.html and unpack the image tar ball in the bugzilla directory.

two small icons, simply unpack in the bugzilla directory

Comment on attachment 180373 [details] [diff] [review] Patch to show sort direction and allow toggling the sort direction Asking for review for you, as per your posting on @developers :)

The patch to table.html.tmpl has a reference to custom_field_descs, is this perhaps something from one of the custom fields patches that snuck in? One drawback that I noted, is that current behavior is for the sort order to be cumulative. That effectively allows specifying primary, secondary, etc... sort keys. This allows me to select the Priority, Severity, and then Target Milestone columns in that order, and results in a primary sort key of Target Milestone, secondary sort key of Severity, and third sort key of Priority. The current patch looses this behavior. I would not want to loose this without having some other suitable way to specify secondary and tertiary sort keys.

There's some enhancement bug somewhere that that patch would probably more appropriately fit on... (we filed a bug to be able to reverse the sort order, I thought...)

Is this a duplicate of bug 23473?

No, it isn't a dup because this addresses a bug that managed to get in somewhere between 2.16 and 2.18. Prior releases did indicate the sort order. The patch from Klaas fixes both this bug, and it also looks like bug 23473. If there is movement on getting this patch applied (hopefully fixing the problem I pointed out in comment #6) then it would make sense to consolidate the related bugs.

Note that you have *not* my vote for this patch. There are better ways to indicate the sort order of a list than using images IMHO.

(In reply to comment #10) > Note that you have *not* my vote for this patch. There are better ways to > indicate the sort order of a list than using images IMHO. > And the better ways to indicate sort order are?

(In reply to comment #11) > And the better ways to indicate sort order are? See bug 291397 comment 4, point 5. See also http://www.unicode.org/charts/PDF/U2190.pdf

Thanks for the link, I like the idea of prefering characters before images too. Will look into that... The benefit of images however is that they could be easily customised.

Comment on attachment 180373 [details] [diff] [review] Patch to show sort direction and allow toggling the sort direction Your identation is off in many places. The patch contains TABs. The testing suite fails due to the previous issues (you can run runtests.pl or runtests.sh to see the results; it accepts the --verbose option). Our style is: if ($condition) ... and we prefer that to: if ( $condition ) Nit: I really don't like the space (%20) thing that is added as a separator: + [% urlquerypart FILTER html %]&order=bugs.bug_id%20[% sortdirs.$col FILTER url_quote %] Couldn't that %20 be replaced with something like: &sortorder= or &sortdirs= ? It would remove the need for the special parser regexp: $fragment =~ /^(\S+)\s*(asc|desc)?$/ and it would preserve URL-compatibility with API that uses the order parameter from within the URL. By the way, I haven't understood this change: -$vars->{'urlquerypart'} = $::buffer; -$vars->{'urlquerypart'} =~ s/(order|cmdtype)=[^&]*&?//g; +my $tmpcgi = new Bugzilla::CGI($::buffer); +$tmpcgi->delete('order', 'cmdtype', 'query_based_on'); +$vars->{'urlquerypart'} = $tmpcgi->query_string(); Personally I don't like $tmpcgi, but that's irrelevant: it seems it's a change not related to this bug, and if it doesn't help with the implementation of the described feature, I'd leave that out for another bug.

Oh, by the way, I forgot: according to the MPL, you also need to add your name and email to the contributor(s) section at the beginning of the file.

Adding UI owner to CC list.

Comment on attachment 180374 [details] proposal arrow images Requesting UI review on images from UI owner.

Comment on attachment 180374 [details] proposal arrow images These are OK, although I'd prefer Unicode arrow characters, if universally supported, or even better, something other than an arrow which indicates sort order better (in my experience, it is difficult to infer order from arrow direction).

Adding a new patch here that is a bit reworked to address some of the issues Vlad that were brought up - thanks for that. testsuite: (comment #14) I was not able to satisfy the testsuite completely, sorry. I need help here. The problem that remains is not ok 40 - (en/default) template/en/default/list/table.html.tmpl has unfiltered directives: # 139: (column.name) # 141: (sortdirs.${column.name}) # 150: sortdirs.${column.name} and I tried several FILTER statements, but was not able to find out which FILTER is to use how. Space as separator: (comment #14) I do not like that really too, but this patch also contains it to be still compatible with the old code. Specifying asc or desc to the sort order was possible before this patch too, for example by editing the url. And the asc or desc was also added with a space separator, because it goes directly to the sort statement of the database. Thus having the space separator keeps us compatible with saved queries for example. $tmpcgi lines: (comment #14) The two deleted lines copy the entire buffer to hidden parameter and tries to delete the order and cmdtype value. I found problems with that (I think thats the reason for the growing url line described in bug #228053, but I have to admit that I do not remember exactly). I thought that using the CGI object is the future way to go and it provides an easy way to remove the parameters. Cumolative Sort Order (comment #5) This patch allows to sort after up to three sort criterias. The latest clicked is the first criteria. I personally find that somehow confusing because it is not obvious which is the primary sort criteria. For that I added a hash "sortsequence" to the template that says 1, 2 or 3 for the sorted field name and indicates if it is the first, second or third criteria. Note: The template patch does not yet show it because I did not find a good looking solution. Images or Unicode Characters (comment #12) I tried to replace the images by unicode arrows and was not successfull. The character-arrows are very much font dependant. While Firefox came up with a very thin, not really good looking arrow, Konqueror did not find something in his font base and as a result came up with a square. So I still left the images in this patch, however I still agree that images are not the very best solution. BTW I was not able to think of a better solution to indicate the sort order of a column than little arrows pointing to top or bottom. These arrows are IMO very common in GUIs like in listviews for example if one thinks of the sort order indicators in the columns of the message lists in mail clients like thunderbird.

Comment on attachment 186301 [details] [diff] [review] Buglist sort patch You're not a reviewer, so don't set the flag to either + or -. If you want your patch to be 'officially' reviewed, please set it to ?.

Comment on attachment 186301 [details] [diff] [review] Buglist sort patch Sorry for the wrong review setting. I hope that this one is correct now.

Comment on attachment 186301 [details] [diff] [review] Buglist sort patch Requesting for review without asking specifically. You've asked yourself to review your patch, which is not the way it works and, I gather, not what you want :)

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

Comment on attachment 186301 [details] [diff] [review] Buglist sort patch 1) First hunk of buglist.cgi patch failes. For testing this doesn't matter as it's only a comment change. 2) Runtests fail on test #8 like you know. (More comments below.) 3) You should move the two .png files under images subdirectory. 4) When more than one column is selected for sorting, trying to change sort order of a column removes all other columns from the sort. 5) When clicking on columns, sometimes URL gets infested with multiple %20 chars. >Index: buglist.cgi >=================================================================== >- $columns->{$id} = { 'name' => $name , 'title' => $title }; >+ $columns->{$id} = { 'name' => $name , 'title' => $title, 'sortdir' => 'asc' }; Is the sortdir member used anywhere? I couldn't find anything referencing it.. >+ $cnt++; >+ last if ($cnt == 3); # only allow three search criterias. I don't think I like the idea of allowing only three columns for sorting. I have used and needed more.. If there is no way to not limit this then atleast use bigger limit. Also, please fix the comment. >+# Add the sortdirs hash to the template database Maybe "Pass sort direction and sequence hashes to template" is better comment here? +$vars->{sortsequence} = \%sortsequence; It seems this is not used in the template at all? Ah, looks like you didn't yet update the template to use it. IMHO, it would be really cool to see the sequence in which the the sort columns are used. Perhaps with different kind of colored image or maybe just simply small number indicating the sequence. >-$vars->{'urlquerypart'} = $::buffer; >-$vars->{'urlquerypart'} =~ s/(order|cmdtype)=[^&]*&?//g; >+my $tmpcgi = new Bugzilla::CGI($::buffer); >+$tmpcgi->delete('order', 'cmdtype', 'query_based_on'); >+$vars->{'urlquerypart'} = $tmpcgi->query_string(); Why this change? I don't see what relevance this has to this patch. If it has none, please remove it from subsequent patches to this bug. You should, however, attach it to the bug it fixes or open a new one to handle the change. Besides, there is a better way to rewrite that using canonicalise_query (see the patch on bug 70907 that also does similar change). >Index: template/en/default/list/table.html.tmpl >=================================================================== >+ [% col='bugs.bug_id' FILTER url_quote %] This doesn't need FILTER as it's a constant. Also, please add spaces around = to give it more space, like this: [% col = 'bugs.bug_id' %] >+ [% IF order.search( 'bugs.bug_id' ) %] Nit: You have the name in col variable, please use it here too. >- <a href="buglist.cgi?[% urlquerypart FILTER html %]&amp;order= >- [% column.name FILTER url_quote FILTER html %] >- [% ",$qorder" FILTER html IF order %] >+ <nobr><a href="buglist.cgi?[% urlquerypart FILTER html %]&amp;order= >+ [% (column.name) %] Don't remove filter directives from this line. Probably no need to change this at all.. >+ [% IF ! sortdirs.search( column.name ) %] Remove extra spacing. >+ %20[% (sortdirs.${column.name}) %] Filters missing from this line. Also, intend this because it's no inside IF/END block. I don't think %20 works as expected here. >+ [% END %] >+ [% IF ! order.search( column.name ) %] Remove extra spacing. Actually, I don't think this IF/END block is needed at all? >+ ,[% order FILTER url_quote %] No need to change filtering on this line either. But do intend it more if it stays inside an IF/END block. >- [%- abbrev.$id.title || field_descs.$id || column.title -%]</a> >+ [%- abbrev.$id.title || field_descs.$id || column.title FILTER html -%] I don't think this needs this filter.

/me throws two more Unicode characters &#x25b2; and &#x25bc; (▲ and ▼) into the discussion and goes back into hiding

This bug is annoying me more and more. Let's fix it asap.

This bug is retargetted to Bugzilla 3.2 for one of the following reasons: - it has no assignee (except the default one) - we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1) - it's not a blocker If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0. If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug. If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

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 the "blocking3.2" flag to "?". Then, 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. This particular bug has not been touched in over eight months, and thus is being retargeted to "---" instead of "Bugzilla 4.0". If you believe this is a mistake, feel free to retarget it to Bugzilla 4.0.

Looks like bug 164009 has evolved to be a DUP of this with a patch that's more up to date.