199048 - Preference option to reverse sort the comments stack

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement, P1)

Description

[Jason Pyeron]

User-Agent:       Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0; .NET CLR 1.0.3705)
Build Identifier: 

I would like to see:


the following options
forwards:
report, 1, 2, 3, 4, MostRecentComment [current setup]
R, MRC, 1, 2, 3, 4
MRC, R, 1, 2, 3, 4

reverse:
MRC, 4, 3, 2, 1, R [my favorite]
R, MRC, 4, 3, 2, 1



Reproducible: Always

Steps to Reproduce:

Comment 1

[Gervase Markham [:gerv]]

Ooh, no. This would be horribly confusing. At the moment, you can:
a) Rely on any view of Bugzilla, including the one on your mate's desk, to work
   the same
b) Say "the comment above" in a comment and know that it's always right.
c) Use Ctrl-End to get to the last comment written.

All of these would break if this option was implemented. It would also be Yet
More Configuration. And those sites who want it can achieve it using custom
templates if they are masochists :-)

Gerv

Comment 2

[Jason Pyeron]

You make me laugh, Point taken, Then I would settle for REVERSE only.

I want to see the last few updates at the begining, when posting a reply I hate 
scroll up, type, scroll down read, REPEAT until dizzy.

Also my laptop has horribly slow (19.2 Max) cell connection, and pages which 
are very long bore me.

It is fun to do QA in the Park :)

Comment 3

[Thomas Schmidt]

if not configurable, i would prefer REVERSE order, too.
most of the time newer comments or attachments are more interesting than older 
ones!

Comment 4

[Albert Ting]

How about making this a user definable option?  It'll probably require some
type of "commentflags" field in the profiles table.  

Comment 5

[Dave Miller [:justdave]]

Good idea.  That depends on bug 98123, though, because we're shoving too much
stuff in the profiles table.

Comment 6

[Jason Pyeron]

use a cookie?
maybe like the suggestion on bug 262592

Comment 7

[Shane H. W. Travis]

I have a patch just about ready for this; I hope to get it in before the freeze.

Comment 8

[Albert Ting]

I don't know if there's a separate ticket for this, but I think it belongs with
this ticket.

If we're going to put the effort to support reverse the comments on a per user
basis, can there also be a flag to email *all* the comments whenever somebody
makes a change?  

I myself hate having all the comments resent. But I do have co-workers who
consider Bugzilla "inferior" because it doesn't include cited text from previous
comments, similar to what you would get via email. 

Comment 9

[Shane H. W. Travis]

(In reply to comment #8)
> I don't know if there's a separate ticket for this, but I think it belongs
> with this ticket.

No, it definitely doesn't. It's a fine idea deserving of consideration, but 
"No (preference) option to reverse sort the comments stack"
does not have anything much to do with
No (preference) option to mail me all comments from a bug every time".

It's true that they both deal with comments, but it would be MORE accurate to 
say that one deals with how comments are displayed (show_bug.cgi) and the other 
deals with how comments are mailed (BugMail.pm). It's also true that they both 
deal with user preferences, but that's irrelevant now bug 98123 has landed and 
the infrastucture now exists to add user preferences for just about anything.

Not trying to shoot down your idea, Albert; I think it's worthy of 
consideration. Please check that no dupe already exists, and if not then open 
up a new one and set it as being blocked by bug 98123. (That's how I'm keeping 
track of User Preferences, which I'm pretty much the one responsible for just 
now.)


> If we're going to put the effort to support reverse the comments on a per user
> basis, can there also be a flag to email *all* the comments whenever somebody
> makes a change?  
> I myself hate having all the comments resent. But I do have co-workers who
> consider Bugzilla "inferior" because it doesn't include cited text from 
previous
> comments, similar to what you would get via email. 

Comment 10

[Albert Ting]

(In reply to comment #9)

Thanks Shane.  Found an existing ticket, bug #116863.  Have added the dependency. 

Comment 11

[Shane H. W. Travis]

Attachment: Code patch for tip

Of the methods mentioned in the initial description, only the following three
are implemented by this patch:

D= Description
MRC = Most recent comment

D, 1, 2, 3, 4, MRC   as "oldest_to_newest"  [this is the current setup]
MRC, 4, 3, 2, 1, D   as "newest_to_oldest"
D, MRC, 4, 3, 2, 1   as "newest_to_oldest_desc_first"

Just for the record... a version of this patch (without the modifications for
98123, so basically just a hack job) has been on our site for the last year at
least; it is from that mod that I learned that I needed to fix up the midair
collisions to *ignore* a user's preferences... something I missed the first
time I tried to code it.

Comment 12

[Shane H. W. Travis]

Attachment: Code patch for tip, take 2

Fixed patch.

Comment 13

[Albert Ting]

Probably a nit, but shouldn't BugMail.pm be updated as well?  For cases where
there is a delay in sending emails, I believe Bugzilla would email multiple
comments.  

Comment 14

[Shane H. W. Travis]

Albert, are you on the right bug? I thought we agreed that this one was about 
how the comments looked to people when displayed by show_bug.cgi, and had 
nothing to do with BugMail at all?

Comment 15

[Albert Ting]

> Albert, are you on the right bug? I thought we agreed that this one was about 
> how the comments looked to people when displayed by show_bug.cgi, and had 
> nothing to do with BugMail at all?

Yup, I'm on the right bug... I think.... :)

My point is that the comment stack ought to be consistent for both show_bug and
BugMail.pm.  Granted, you normally only get 1 comment from BugMail but if the
sendmail flag is turned off, you could get multiple comments from a single
email.  I think those comments ought to be sorted the same way. 

This is not to be confused with making BugMail send all the comments.  I'm aware
that's a different issue marked in Bug #116863.  

Comment 16

[Jason Pyeron]

no, this bug is ONLY for the option, if a new bug [bug 116863?] is to be filed 
on the bugmail to make use of it then so beit.

Comment 17

[Albert Ting]

(In reply to comment #16)
> no, this bug is ONLY for the option, if a new bug [bug 116863?] is to be filed 
> on the bugmail to make use of it then so beit.

Understood.  I've added an initial patch to bug #116863.  Needs more work, but
it'll do.

Comment 18

[Max Kanat-Alexander]

Comment on attachment 177429 [details] [diff] [review]
Code patch for tip, take 2

>-        $vars->{'comments'} = Bugzilla::Bug::GetComments($id);
>+
>+        # Always sort midair collision comments oldest to newest,
>+        # regardless of the user's personal preference.
>+        $vars->{'comments'} = Bugzilla::Bug::GetComments($id, "oldest_to_newest");

  I'm not a huge fan of passing around fixed strings, but I suppose I'll live.

>Index: Bugzilla/Bug.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v
>retrieving revision 1.67
>diff -u -r1.67 Bug.pm
>--- Bugzilla/Bug.pm	10 Mar 2005 22:34:06 -0000	1.67
>+++ Bugzilla/Bug.pm	14 Mar 2005 22:32:02 -0000
>@@ -352,12 +352,27 @@
> }
> 
> sub longdescs {
>-    my ($self) = @_;
>+    my ($self, $comment_sort_order) = @_;
> [snip] 

    I'm fairly sure that this change and your comments.html.tmpl change are
redundant. I belive that comments.html.tmpl is usually getting its data from
this longdescs sub, here. And anywhere that isn't, ought to be. I don't
understand why we have to change the sort and then reverse it again in the
template.

  Also, just put these changes into GetComments, and change the actual sort
order of the returned SQL data. Then it should be easy to get the desc and put
it up front if needed, because it will be at the end of the returned data. It'd
just be a unshift(@array, pop @array).

  Oh, wait... OK, now I kind of understand the comments.html.tmpl code. That
seems like a lot of logic to have in a template... is there any way to simplify
that logic?

  Also, why do we grant a special exte

>+      [% IF is_description %]
>+        <table>
>+          <tr>
>+            <td align="left">
>+              <b><a name="c0" href="#c0">Description</a>:</b>&nbsp;&nbsp;<script 

  Couldn't we just add a description_at_end thing to the interface of
comments.html.tmpl, and simplify this somewhat? And maybe also a
has_description, although I'm not certain about that.

>+   "newest_to_oldest_desc_first"      => "Newest to Oldest, but keep Description at the top",

  That will really be too long for the <select> box. Maybe just "Newest To
Oldest, Description on Top"? Even that's kinda long, but it's shorter, at
least.

Comment 19

[Shane H. W. Travis]

Attachment: Code patch for tip, take 3

(In reply to comment #18)

> I don't understand why we have to change the sort and then reverse it
> again in the template.

I don't. The code here is about what order the comments are in the array when
they are passed to comments.html.tmpl. When they get there, the code in that
section is ONLY about putting the right 'Comment #X' on the comment line.

A parameter is created for this subroutine to allow code to override the user's
preference, which is necessary for midair collisions.

> Also, just put these changes into GetComments, and change the actual sort
> order of the returned SQL data.

Yeah, okay. I had a valid reason for not doing it that way at one time (namely
the interaction of this userpref with another one, and the second one heavily
modified the GetComments routine) but with the new Setting stuff most of the
things I had to worry about are gone now. Done.


> That seems like a lot of logic to have in a template... is there any way
> to simplify that logic?

Not really, but maybe a little. Look at this version and see if you like it any
better.

> Couldn't we just add a description_at_end thing to the interface of
> comments.html.tmpl, and simplify this somewhat? And maybe also a
> has_description, although I'm not certain about that.

I have NO idea what you're talking about here; every bug 'has_description', and
it isn't until we get to comments that we know the sort order and can tell if
the description is at the top or the bottom.

This code is taken directly from edit.html.tmpl (where it used to reside) and
brought here unchanged. Prior to this patch, one could count on the Description
always being the top comment, so it was fine to have the HTML for the
Description line before the call to comments.html.tmpl. Now, Description could
be either at the top or the bottom, and only comments.html.tmpl knows/cares
which it is, which is why that stuff had to be moved.

> >+   "newest_to_oldest_desc_first"	  => "Newest to Oldest, but keep
Description at the top",
>   That will really be too long for the <select> box.

Looks fine in mine. On what grounds are you saying 'too long'? (The
32-character restriction is only for the database...)

> Maybe just "Newest To Oldest, Description on Top"?

Doesn't shorten it that much, and obfuscates the meaning IMHO.

Comment 20

[Max Kanat-Alexander]

Comment on attachment 177759 [details] [diff] [review]
Code patch for tip, take 3

This works properly, and it looks much better. :-) 

But for some reason I now only have one setting (this one) on my General
Settings page, even though the Quip pref is clearly Enabled.

Comment 21

[Albert Ting]

(In reply to comment #20)
> But for some reason I now only have one setting (this one) on my General
> Settings page, even though the Quip pref is clearly Enabled.

See bug #286357.

Comment 22

[Shane H. W. Travis]

Midaired with Albert(In reply to comment #20)
> But for some reason I now only have one setting (this one) on my General
> Settings page, even though the Quip pref is clearly Enabled.

Midaired with Albert about to say the same thing; what you're seeing is a side-
effect of the issue raised in bug 286357 and has nothing to do with this patch.

Comment 23

[Max Kanat-Alexander]

Comment on attachment 177759 [details] [diff] [review]
Code patch for tip, take 3

Oh, OK. :-)

Comment 24

[Frédéric Buclin]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.387; previous revision: 1.386
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.249; previous revision: 1.248
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.73; previous revision: 1.72
done
Checking in template/en/default/bug/comments.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/comments.html.tmpl,v
 <--  comments.html.tmpl
new revision: 1.17; previous revision: 1.16
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--
 edit.html.tmpl
new revision: 1.58; previous revision: 1.57
done
Checking in template/en/default/global/setting-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/setting-descs.none.tmpl,v
 <--  setting-descs.none.tmpl
new revision: 1.3; previous revision: 1.2
done

Comment 25

[Frédéric Buclin]

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