Closed
Bug 71840
Opened 24 years ago
Closed 23 years ago
[RFE] Make comments referenceable
Categories
(Bugzilla :: User Interface, enhancement, P2)
Bugzilla
User Interface
Tracking
()
RESOLVED
FIXED
Bugzilla 2.16
People
(Reporter: ian, Assigned: jacob)
References
(Blocks 1 open bug, )
Details
Attachments
(2 files, 13 obsolete files)
2.79 KB,
patch
|
gerv
:
review+
justdave
:
review+
|
Details | Diff | Splinter Review |
573 bytes,
patch
|
justdave
:
review+
justdave
:
review+
|
Details | Diff | Splinter Review |
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 2•24 years ago
|
||
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?
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.16
Assignee | ||
Comment 5•23 years ago
|
||
Assignee | ||
Comment 6•23 years ago
|
||
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).
Priority: -- → P2
Comment 7•23 years ago
|
||
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.
Assignee: tara → jake
Keywords: review
Assignee | ||
Comment 8•23 years ago
|
||
Assignee | ||
Comment 9•23 years ago
|
||
In the intreast of brevity I opted for #cn
Comment 10•23 years ago
|
||
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...
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•23 years ago
|
Attachment #38476 -
Attachment is obsolete: true
Assignee | ||
Comment 11•23 years ago
|
||
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.
Component: Bugzilla → User Interface
Product: Webtools → Bugzilla
Version: other → unspecified
Reporter | ||
Comment 12•23 years ago
|
||
seconds since the era.
Reporter | ||
Comment 13•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
*** Bug 98308 has been marked as a duplicate of this bug. ***
Comment 17•23 years ago
|
||
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•23 years ago
|
||
Assignee | ||
Comment 19•23 years ago
|
||
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").
Attachment #51578 -
Flags: review-
Comment 20•23 years ago
|
||
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
Assignee | ||
Comment 21•23 years ago
|
||
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•23 years ago
|
||
Current version of patch needs work. Removing patch and review keywords.
Comment 23•23 years ago
|
||
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.
Assignee | ||
Comment 24•23 years ago
|
||
Assignee | ||
Comment 25•23 years ago
|
||
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.
Assignee | ||
Updated•23 years ago
|
Attachment #38485 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #51578 -
Attachment is obsolete: true
Assignee | ||
Comment 26•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #52310 -
Attachment is obsolete: true
Comment 27•23 years ago
|
||
With Jake's latest patch, all the things I think should be linkified are, and those I think shouldn't be aren't. Gerv
Summary: [RFE] Make comments referencable → [RFE] Make comments referenceable
Comment 28•23 years ago
|
||
Comment on attachment 52314 [details] [diff] [review] Slightly simplified Expression r=gerv
Attachment #52314 -
Flags: review+
Comment 29•23 years ago
|
||
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.
Assignee | ||
Comment 30•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #52314 -
Attachment is obsolete: true
Comment 31•23 years ago
|
||
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•23 years ago
|
||
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.
Attachment #52314 -
Flags: review-
Comment 33•23 years ago
|
||
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl new revision: 1.116; previous revision: 1.115 Gerv
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 34•23 years ago
|
||
Comment 35•23 years ago
|
||
Gerv, can you please check in something like this, too?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 36•23 years ago
|
||
/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.
Assignee | ||
Comment 37•23 years ago
|
||
Assignee | ||
Comment 38•23 years ago
|
||
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?
Status: REOPENED → ASSIGNED
Comment 39•23 years ago
|
||
> 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•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Comment 42•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #52510 -
Attachment is obsolete: true
Assignee | ||
Comment 43•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #53209 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #53250 -
Attachment is obsolete: true
Comment 44•23 years ago
|
||
Comment on attachment 53394 [details] [diff] [review] no, really r=gerv. Gerv
Attachment #53394 -
Flags: review+
Assignee | ||
Updated•23 years ago
|
Attachment #53393 -
Attachment is obsolete: true
Comment 45•23 years ago
|
||
> 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•23 years ago
|
||
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•23 years ago
|
||
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.
Attachment #53394 -
Flags: review-
Comment 48•23 years ago
|
||
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•23 years ago
|
||
Comment 50•23 years ago
|
||
This patch implements the more conservative scheme, and uses e.g. #c3 for the anchors. Gerv
Assignee | ||
Comment 51•23 years ago
|
||
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" :(
Attachment #54282 -
Flags: review-
Assignee | ||
Comment 52•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #53394 -
Attachment is obsolete: true
Comment 53•23 years ago
|
||
Comment on attachment 54292 [details] [diff] [review] Jake: v8 - I hope this is it r= justdave
Attachment #54292 -
Flags: review+
Comment 54•23 years ago
|
||
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
Attachment #54292 -
Flags: review-
Assignee | ||
Comment 55•23 years ago
|
||
Assignee | ||
Comment 56•23 years ago
|
||
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•23 years ago
|
||
Comment on attachment 54309 [details] [diff] [review] one uber-patch r=gerv. Gerv
Attachment #54309 -
Flags: review+
Comment 58•23 years ago
|
||
Comment on attachment 54309 [details] [diff] [review] one uber-patch r= justdave
Attachment #54309 -
Flags: review+
Comment 59•23 years ago
|
||
/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
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 60•23 years ago
|
||
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•23 years ago
|
||
Oh smeg. I forgot I didn't make the last patch! Doh. Thanks, Jake. Gerv
Comment 62•23 years ago
|
||
This fix doesn't seem to have made it to b.m.o. Or am I just blind?
Comment 63•23 years ago
|
||
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•23 years ago
|
||
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 -------
Assignee | ||
Comment 65•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
doh, he's right. It even says so in the last patch on this bug, which is what got checked in.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•23 years ago
|
Attachment #54282 -
Attachment is obsolete: true
Updated•23 years ago
|
Attachment #54292 -
Attachment is obsolete: true
Assignee | ||
Comment 68•23 years ago
|
||
Comment 69•23 years ago
|
||
Assignee | ||
Comment 70•23 years ago
|
||
*grumble* how'd I miss that one :(
Comment 71•23 years ago
|
||
Comment on attachment 56731 [details] [diff] [review] Bugfix 1 v1 r= justdave this is pretty obvious, no 2nd review needed.
Attachment #56731 -
Flags: review+
Comment 72•23 years ago
|
||
Comment on attachment 56732 [details] [diff] [review] Bugfix patch oops, duplicate patch.
Attachment #56732 -
Attachment is obsolete: true
Assignee | ||
Comment 73•23 years ago
|
||
Braindead mistake correction checked in...
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 74•23 years ago
|
||
*** Bug 68482 has been marked as a duplicate of this bug. ***
Comment 75•22 years ago
|
||
[RFE] is deprecated in favor of severity: enhancement. They have the same meaning.
Severity: normal → enhancement
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•