71840 - [RFE] Make comments referenceable

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement, P2)

Description

[Hixie (not reading bugmail)]

Comments cannot be referenced individually. It would be great if they could. 
Here is a proposed patch (the line which changes is the one with lots of "---" 
on it. I add a <A NAME=""> bit).

http://www.hixie.ch/bugzilla/ is currently running with this change.


***** globals.pl before *****

    $query .= "ORDER BY longdescs.bug_when";
    SendSQL($query);
    while (MoreSQLData()) {
        my ($who, $email, $when, $text) = (FetchSQLData());
        $email .= Param('emailsuffix');
        if ($count) {
            $result .= "<BR><BR><I>------- Additional Comments From ";
              if ($who) {
                  $result .= qq{<A HREF="mailto:$email">$who</A> } .
                      time2str("%Y-%m-%d %H:%M", str2time($when)) .
                          " -------</I><BR>\n";
              } else {
                  $result .= qq{<A HREF="mailto:$email">$email</A> } .
                      time2str("%Y-%m-%d %H:%M", str2time($when)) .
                          " -------</I><BR>\n";
              }
        }
        $result .= "<PRE>" . quoteUrls(\%knownattachments, $text) . "</PRE>\n";
        $count++;
    }


***** globals.pl after *****

    $query .= "ORDER BY longdescs.bug_when";
    SendSQL($query);
    while (MoreSQLData()) {
        my ($who, $email, $when, $text) = (FetchSQLData());
        $email .= Param('emailsuffix');
        if ($count) {
            $result .= "<BR><BR><I>------- <A NAME='$count'>Additional Comments 
From</A> ";
              if ($who) {
                  $result .= qq{<A HREF="mailto:$email">$who</A> } .
                      time2str("%Y-%m-%d %H:%M", str2time($when)) .
                          " -------</I><BR>\n";
              } else {
                  $result .= qq{<A HREF="mailto:$email">$email</A> } .
                      time2str("%Y-%m-%d %H:%M", str2time($when)) .
                          " -------</I><BR>\n";
              }
        }
        $result .= "<PRE>" . quoteUrls(\%knownattachments, $text) . "</PRE>\n";
        $count++;
    }

Comment 1

[Hixie (not reading bugmail)]

Since I give the fix above, marking 'patch'.

Comment 2

[Stephan Niemz]

Good idea, I also missed this already, but was too lazy to file a bug. :)

I think it would be even better to also insert an <a href> there to make it
easy to "copy link location" to paste it in somewhere when making a reference,
like LXR does it, e.g.:
http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/globals.pl#826

Targetting for 2.16 because I think that shouldn't be a big deal.  (Perhaps this
can even get in earlier?)  By the way, could you please attach the patch again
in "diff -u" format?

Comment 3

[Jesse Ruderman]

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

Comment 4

[Andreas Franke (gone)]

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

Comment 5

[Jacob Steenhagen]

Attachment: diff based on Hixie's suggestion

Comment 6

[Jacob Steenhagen]

The patch I just attached started with what Hixie submitted, but expands by
adding what faniz suggested (name and href) and also makes it so #0 will
reference the first comment.

http://bugzilla.mozilla.org/show_bug.cgi?id=71840#6 will reference this comment
after this patch lands (assuming I counted correctly).

Comment 7

[Myk Melez [:myk] [@mykmelez]]

It's all yours Jake.  My only suggestion is to make the anchors look like
#commentn or #cn (where n is the number) just in case we ever start adding other
kinds of numbered anchors to bugs.

Comment 8

[Jacob Steenhagen]

Attachment: patch v2

Comment 9

[Jacob Steenhagen]

In the intreast of brevity I opted for #cn

Comment 10

[Dave Miller [:justdave]]

My only comment on this:

It's possible to delete a comment from the DB backend, and we actually have a bug 
that's requesting the ability to do this from the front end (bug 540 I think).

If a comment gets deleted and any comment after it in the list has been 
referenced elsewhere, that reference will now point to the wrong comment using 
this $count method.  Would be better to reference the datestamp or something...

Comment 11

[Jacob Steenhagen]

The one thing I don't like about using dates instead of integers is dates are
ugly and harder to type :)  We'd also have to come up with a URL safe date format.

Comment 12

[Hixie (not reading bugmail)]

seconds since the era.

Comment 13

[Hixie (not reading bugmail)]

although, isn't the ID fixed even if you remove a comment? In BZ3, each comment
has a bug id and a comment id, so even if you shuffle them delete them and edit
them, the ID would stay the same.

Comment 14

[Gervase Markham [:gerv]]

If you were worried about numbers changing, comment deletion could be
implemented using a special record in the database which counted for 1 but
didn't display at all. 

The anchors should be #<num>. In the extremely unlikely event that bugs acquire
other sorts of anchors, then those can have letter abbreviations.

Lastly, we should hyperlink "comment 3" and comment #3" in the same way we do
bug 12345 . For bonus points, we should hyperlink "comment #7 in bug 12345"
correctly as well. 

timeless has a patch for some of this. It may well be the bit hixie's done, though.

Gerv

Comment 15

[Gervase Markham [:gerv]]

I fixed this, together with auto-hyperlinking of comment references, over in bug
98308. Oops. Ah well - that patch is up for review :-)

Gerv

Comment 16

[Dave Miller [:justdave]]

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

Comment 17

[Gervase Markham [:gerv]]

The problem with this patch is that it doesn't make the comment numbering
visible; so if someone wants to reference a comment, they have to mouseover
"Additional Comments" to work out the number to reference.

I'm attaching my patch from the duped bug 98308, which does the following e.g.:

------ Additional Comments From Stephan Niemz [faniz] 2001-03-14 08:47 (#5) ------

and also linkifies a variety of strings of the form:
bug #3, comment 6
etc. (see that bug for a semi-complete list.)

Gerv

Comment 18

[Gervase Markham [:gerv]]

Attachment: Gerv's patch v.1

Comment 19

[Jacob Steenhagen]

Comment on attachment 51578 [details] [diff] [review]
Gerv's patch v.1

Over in bug 98308 you said 

------------
Within a bug:
comment 1
comment #1
com 1
(basically, the same regexp as bug, but with com(ment)? instead.)

Between bugs:
bug 30000#1
bug #30000 #4.
bug 30000 comment 1
bug 30000, com 4
etc.

but _not_:
bug 30000, #5
bug #30000, #6
because I think this would get the wrong result some times.
--------------

In the middle list, the top 2 work, but the bottom 2 don't (and neither does
"bug #3, comment 6").

Comment 20

[Gervase Markham [:gerv]]

Hmm. Can you see what's wrong with my regexp? It looks right to me, and I'm
pretty sure it worked when I tried it...

Gerv

Comment 21

[Jacob Steenhagen]

Gerv, looks like you forgot a set of ()'s

/\bbug(\s|%\#)*(\d+)((,?\s*\bcom(ment)?\s*%\#?)|(\s*|%\#))\s*(\d+)/
                                                ^   ^   ^
Making that so it looks the same of all the other instances of it seems to work.

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Current version of patch needs work.  Removing patch and review keywords.

Comment 23

[Myk Melez [:myk] [@mykmelez]]

Testing Gerv's latest patch along with Jake's corrections to the regexp:

bug 30000#1 -> http://localhost/bz214/show_bug.cgi?id=30000##
bug #30000 #4. -> http://localhost/bz214/show_bug.cgi?id=3000#
(only "bug #30000" is linked)
bug 30000 -> http://localhost/bz214/show_bug.cgi?id=3000#

still needs work.

Comment 24

[Jacob Steenhagen]

Attachment: Another Revision

Comment 25

[Jacob Steenhagen]

OK, I've tested attachment 52310 [details] [diff] [review] w/all of Gerv's samples (the yes and no list)
as well as Myk's tests.  I also moved the link to in front of the user name
instead of behind it so the lenght of the user's name doesn't effect the
horizontal position of the command anchor link.

Comment 26

[Jacob Steenhagen]

Attachment: Slightly simplified Expression

Comment 27

[Gervase Markham [:gerv]]

With Jake's latest patch, all the things I think should be linkified are, and
those I think shouldn't be aren't.

Gerv

Comment 28

[Gervase Markham [:gerv]]

Comment on attachment 52314 [details] [diff] [review]
Slightly simplified Expression

r=gerv

Comment 29

[Andreas Franke (gone)]

You may want to take
	time2str("%Y-%m-%d %H:%M", str2time($when))
out of the if-then-else, too.

Also, is there a reason to not drop the "Additional" ? IMO, it just takes up
space.

Comment 30

[Jacob Steenhagen]

Attachment: w/Anreas' Suggestions

Comment 31

[Gervase Markham [:gerv]]

I have too much mental stability invested in the phrase "Additional Comments".
This may seem strange, but I'd much prefer "Additional" to stay.

Gerv

Comment 32

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 52314 [details] [diff] [review]
Slightly simplified Expression

>Index: bug_form.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/bug_form.pl,v
>retrieving revision 1.73
>diff -u -r1.73 bug_form.pl
>--- bug_form.pl	2001/10/02 01:20:52	1.73
>+++ bug_form.pl	2001/10/06 00:43:38
>@@ -569,7 +569,7 @@
> print "<BR></FORM>";
> 
> print "
>-<table><tr><td align=left><B>Description:</B></td>
>+<table><tr><td align=left><B><a name='0' href='#0'>Description:</a></B></td>

My suggestion would be to be consistent with the quotes you use in HTML, rather
than being inconsistent for the sake of a small increase in Perl readability,
or have your cake and eat it too with the generic quoting delimiters q and qq
(f.e. qq|<a href="foo">He said, "She said, '$bar!'"</a>| ).

>+    # Either a com(ment)? string or no comma and a compulsory #.

"com" is confusing and redundant, and the comma could cause problems in
some cases.  We could do without them.

Comment 33

[Gervase Markham [:gerv]]

/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.116; previous revision: 1.115

Gerv

Comment 34

[Andreas Franke (gone)]

Attachment: avoid duplicate time2str call, it's the same in both cases

Comment 35

[Andreas Franke (gone)]

Gerv, can you please check in something like this, too?

Comment 36

[Dave Miller [:justdave]]

/cvsroot/mozilla/webtools/bugzilla/bug_form.pl,v  <--  bug_form.pl
new revision: 1.76; previous revision: 1.75

/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.117; previous revision: 1.116

Gerv's checkin has been backed out.

We have this procedure in place for a reason, please follow it.  Your checkin
notice claims that Myk r='d it, however, the last review from Myk on this bug
was a 'needs-work' and Myk says what actually got landed did not adequately
satisfy his concerns.

Revised patches should get posted to the bug before being checked in, not just
for review, but so that people who want to apply this on their own without CVS
can download it from the bug and get the same patch.

Comment 37

[Jacob Steenhagen]

Attachment: this round's on me

Comment 38

[Jacob Steenhagen]

Andreas, I'm not sure what the point of your attachment to clear out the
duplicate time2str thing was for, it's included in my last patch(es)...

Myk, I have some questions on you second comment...

>"com" is confusing and redundant, and the comma could cause problems in
>some cases.  We could do without them.

I agree that "com" is/can be confusing (even though I didn't take it out of the
lastest version), but shouldn't there be some kind of shorthand for "comment"?

Regarding the comma: "bug xxx, comment y" would be the proper english way to
refer to comment y on bug xxx (second only to what I just said, which we don't
detect)... I can't think of any instances where having that in the expression
would cause problems, can you point out a specific case?

Comment 39

[Gervase Markham [:gerv]]

> We have this procedure in place for a reason, please follow it.  Your checkin
> notice claims that Myk r='d it, however, the last review from Myk on this bug
> was a 'needs-work' 

I sit across the room from Myk. He told me to make the changes, and he'd be
happy with it. I'm not sure why he didn't say so explicitly in the bug as well.

> and Myk says what actually got landed did not adequately
> satisfy his concerns.

In what way? I made the changes he requested.

Gerv

Comment 40

[Myk Melez [:myk] [@mykmelez]]

Jake wrote:

>I agree that "com" is/can be confusing (even though I didn't take it out of the
>lastest version), but shouldn't there be some kind of shorthand for "comment"?

I don't think so, for several reasons.  First of all, we don't have a shorthand
for "attachment," which is much longer than "comment," and I am not aware of a
single complaint about this.  Second of all, "com" is not a common abbreviation
of comment.  I suspect people will waste more time learning about their option
to use "com" and figuring out what it means when they see it than they will save
by being able to use it.

>Regarding the comma: "bug xxx, comment y" would be the proper english way to
>refer to comment y on bug xxx (second only to what I just said, which we don't
>detect)... I can't think of any instances where having that in the expression
>would cause problems, can you point out a specific case?

Looking at it again, it makes sense to linkify "bug xxx, comment y".

> I sit across the room from Myk. He told me to make the changes, and he'd be
> happy with it. I'm not sure why he didn't say so explicitly in the bug as 
> well.

I didn't say I'd be happy with it, I said I wanted you to make those two
changes.  I didn't do a complete review after I found those two problems because
I knew I was going to have to re-review anyway after you fixed them.  I should
have said this in my review (or done the complete review up-front).

> > and Myk says what actually got landed did not adequately
> > satisfy his concerns.

> In what way? I made the changes he requested.

That's true, now that I look at the Bonsai check-in.  My mistake for not looking
at it before.  Still, the last patch on the bug should be the one checked in so
installations can apply just that patch.

Comment 41

[Myk Melez [:myk] [@mykmelez]]

And just in case anyone misunderstood the attributions in my previous comment,
"I sit across the room from Myk..." was written by Gerv, not Jake.

Comment 42

[Jacob Steenhagen]

Attachment: this should be it...

Comment 43

[Jacob Steenhagen]

Attachment: no, really

Comment 44

[Gervase Markham [:gerv]]

Comment on attachment 53394 [details] [diff] [review]
no, really

r=gerv.

Gerv

Comment 45

[Andreas Franke (gone)]

> Andreas, I'm not sure what the point of your attachment to clear out the
> duplicate time2str thing was for, it's included in my last patch(es)...

That's true, it just wasn't in the patch that was checked in by Gerv.

Comment 46

[Myk Melez [:myk] [@mykmelez]]

So what happens when the fix for bug 11368 lands and people want to be able to
reference history items by number in addition to comments?

Comment 47

[Dave Miller [:justdave]]

Comment on attachment 53394 [details] [diff] [review]
no, really

I'll stamp it now so it doesn't accidently get checked in.  This was
brought up before, and seems to have been ignored.  Need to use
something in addition to numbers for the anchors because there's a real
good possibility that we might introduce something else that needs to 
be referenced by number. maybe use "#c0" "#c1" or something.

Comment 48

[Gervase Markham [:gerv]]

We just had a long conversation about this, and decided to make it more
conservative.
"http://bugzilla.mozilla.org/show_bug.cgi?id=71840#comment6" and "bug 34567,
comment 42" it is.

Gerv

Comment 49

[Gervase Markham [:gerv]]

Attachment: Gerv v.2

Comment 50

[Gervase Markham [:gerv]]

This patch implements the more conservative scheme, and uses e.g. #c3 for the
anchors.

Gerv

Comment 51

[Jacob Steenhagen]

Comment on attachment 54282 [details] [diff] [review]
Gerv v.2

>Index: globals.pl

>+    while ($text =~ s/\bbug(\s|%\#)*(\d+),?\s*comment\s*(\s|%\#)(\d+)/"##$count##"/ei) {

This regular expression is different than the one in my last patch.  It now
fails to linkify "bug 30000 #3".

>+            $result .= "<BR><BR><I>------- Additional Comment <a name='c$count' href='#c$count'>#$count</a> From ";

Myk mentioned before that we should use real quotes ("") within our HTML tags.
This line wasn't specifically flagged, but it does suffer from the same problem.

>+            if ($who) {
>+                $result .= qq{<A HREF="mailto:$email">$who</A> } .
>+                    time2str("%Y-%m-%d %H:%M", str2time($when));
>+            } else {
>+                $result .= qq{<A HREF="mailto:$email">$email</A> } .
>+                    time2str("%Y-%m-%d %H:%M", str2time($when));
>+            }

As was mentioned by Andreas, both time2str() calls are the same and can be
factored out of the if construct (as was done in my last patch)

Unfortunately, I'm gonna gave to flag this "needs-work" :(

Comment 52

[Jacob Steenhagen]

Attachment: Jake: v8 - I hope this is it

Comment 53

[Dave Miller [:justdave]]

Comment on attachment 54292 [details] [diff] [review]
Jake: v8 - I hope this is it

r= justdave

Comment 54

[Gervase Markham [:gerv]]

Comment on attachment 54292 [details] [diff] [review]
Jake: v8 - I hope this is it

> This regular expression is different than the one in my last patch.  It now
> fails to linkify "bug 30000 #3".

Yes, this is on purpose. Myk, Dawn, Asa and I had a chat and decided that we should not be using bare numbers, because that might make things difficult if we integrate bug_activity items into the bug. 

The policy is that we linkify "comment X" or "comment #X", or the "bug foo comment X" variants thereof, only. The other change is to use "cXXX" as the anchors rather than "XXX".

I'm afraid I'm going to have to flag this 'needs-work'. ;-)

Gerv

Comment 55

[Jacob Steenhagen]

Attachment: one uber-patch

Comment 56

[Jacob Steenhagen]

Talked to Gerv on IRC and I now agree that we should only link to a specific
comment if the word "comment" appears.  So, attachment 54309 [details] [diff] [review] is the combination
of Gerv's v.2 and my v8.

Comment 57

[Gervase Markham [:gerv]]

Comment on attachment 54309 [details] [diff] [review]
one uber-patch

r=gerv.

Gerv

Comment 58

[Dave Miller [:justdave]]

Comment on attachment 54309 [details] [diff] [review]
one uber-patch

r= justdave

Comment 59

[Gervase Markham [:gerv]]

/cvsroot/mozilla/webtools/bugzilla/bug_form.pl,v  <--  bug_form.pl
new revision: 1.79; previous revision: 1.78
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.121; previous revision: 1.120
done

Gerv

Comment 60

[Jacob Steenhagen]

Attachment 54282 [details] [diff] got checked in by accident.  Backed that out and checked in
attachment 54309 [details] [diff] [review].

Checking in bug_form.pl;
/cvsroot/mozilla/webtools/bugzilla/bug_form.pl,v  <--  bug_form.pl
new revision: 1.81; previous revision: 1.80
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.123; previous revision: 1.122
done

Comment 61

[Gervase Markham [:gerv]]

Oh smeg. I forgot I didn't make the last patch! Doh. Thanks, Jake.

Gerv

Comment 62

[Stephen Rasku]

This fix doesn't seem to have made it to b.m.o.  Or am I just blind?

Comment 63

[Dave Miller [:justdave]]

this just got checked in this morning.  of course b.m.o hasn't picked it up yet.
:)   b.m.o only updates every few weeks at this point (usually stalling to get
certain things checked in that they want)

Comment 64

[xyzzy]

AFAICT, this needs one more small change.  The anchor number is not exposed to 
the user.  For long bug reports, this is a problem as the user must count which 
bug number it is or view source...

The change could easily be done on the Additional Comments line, for example:

------- Additional Comments (#472) From Foo [bar] 2001-01-01 00:00 -------

Comment 65

[Jacob Steenhagen]

It's exposed to the user... take a look at
http://landfill.tequilarista.org/bugzilla-tip/show_bug.cgi?id=132

It only has one comment on it, but I'm sure you can see how it'll look when
there's more :)

Comment 66

[Jesse Ruderman]

Jake, at http://landfill.tequilarista.org/bugzilla-tip/show_bug.cgi?id=132, 
description is a link to #c0 but the named anchor corresponding to the 
description is #0.  I think they should both be #c0.

Comment 67

[Dave Miller [:justdave]]

doh, he's right.  It even says so in the last patch on this bug, which is what
got checked in.

Comment 68

[Jacob Steenhagen]

Attachment: Bugfix 1 v1

Comment 69

[Dave Miller [:justdave]]

Attachment: Bugfix patch

Comment 70

[Jacob Steenhagen]

*grumble* how'd I miss that one :(

Comment 71

[Dave Miller [:justdave]]

Comment on attachment 56731 [details] [diff] [review]
Bugfix 1 v1

r= justdave
this is pretty obvious, no 2nd review needed.

Comment 72

[Dave Miller [:justdave]]

Comment on attachment 56732 [details] [diff] [review]
Bugfix patch

oops, duplicate patch.

Comment 73

[Jacob Steenhagen]

Braindead mistake correction checked in...

Comment 74

[Bradley Baetz (:bbaetz)]

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

Comment 75

[Brant Gurganus]

[RFE] is deprecated in favor of severity: enhancement.  They have the same meaning.