Closed
Bug 509152
Opened 15 years ago
Closed 15 years ago
line breaks erroneously present in bug xml
Categories
(Bugzilla :: Query/Bug List, defect, P1)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.4
People
(Reporter: rob.elves, Assigned: thomas.ehrnhoefer)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(1 file, 3 obsolete files)
2.35 KB,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Build Identifier: 3.4.1
Upon upgrading to 3.4.1, the text returned for description is different, resulting in false incoming notification in Mylyn (http://eclipse.org/mylyn). This difference is due to line breaks being present in the xml for the description (possibly other fields as well). There were no line breaks present upon original submit (at look at the raw data in the database also reveals no additional line breaks). It appears as though the line wrapping being applied for html presentation is being applied to the xml output as well.
Reproducible: Always
Steps to Reproduce:
1.Post a long line as a comment to a 3.2 repository
2.Upgrade the repository to 3.4.1
3.Open the task in Mylyn and see false incoming on description due to wrapping in xml
Expected Results:
Presentation wrapping should not be applied to XML output.
Reporter | ||
Updated•15 years ago
|
Blocks: bz-clients
Priority: -- → P1
Reporter | ||
Updated•15 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 1•15 years ago
|
||
Which fields are affected?
Comment 2•15 years ago
|
||
Yes, this is happening because Bugzilla::Bug uses format_comment, which wraps text. format_comment should not wrap the text, I think.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•15 years ago
|
Target Milestone: --- → Bugzilla 3.4
Assignee | ||
Comment 3•15 years ago
|
||
@Frederic: only comments are affected
@Max: Thanks for your hint. Based on that I tried this patch and it works fine for us. Comments are not getting wrapped in the XML, but still in the WebUI. Not sure though if that is the best way to do it.
Comment 4•15 years ago
|
||
Comment on attachment 393634 [details] [diff] [review]
suggested patch
No, this is incorrect. You'll want to look at the patch that originally implemented format_comment to see what you have to revert as far as where wrapped_comment goes and where you have to wrap comments.
Also, check out our development process so you can be sure that if you submit another patch, it gets reviewed:
http://wiki.mozilla.org/Bugzilla:Developers
Attachment #393634 -
Flags: review-
Assignee | ||
Comment 5•15 years ago
|
||
Thanks for your looking into this Max.
Excuse me if I am wrong, but I am totally new to the Bugzilla sources and tried my best to understand what was and is going on in regards to the comment wrapping...
As for the patch that implemented the new format_comment.txt.tmpl, it seems to me that the formatting (adding the "duplicate of" and such comments) of comments was done in code (where format_comment is called now), and there was no wrapping done there (at least non that I could find). That's why I thought by removing the wrapping from the template I basically reverted the changes (but leaving the added comments as is)
In detail:
h3. version from 3.3.4
bc..
# Format language specific comments. This routine must not update
# $comment{'body'} itself, see BugMail::prepare_comments().
sub format_comment {
my $comment = shift;
my $body;
if ($comment->{'type'} == CMT_DUPE_OF) {
$body = $comment->{'body'} . "\n\n" .
get_text('bug_duplicate_of', { dupe_of => $comment->{'extra_data'} });
}
elsif ($comment->{'type'} == CMT_HAS_DUPE) {
$body = get_text('bug_has_duplicate', { dupe => $comment->{'extra_data'} });
}
elsif ($comment->{'type'} == CMT_POPULAR_VOTES) {
$body = get_text('bug_confirmed_by_votes');
}
elsif ($comment->{'type'} == CMT_MOVED_TO) {
$body = $comment->{'body'} . "\n\n" .
get_text('bug_moved_to', { login => $comment->{'extra_data'} });
}
else {
$body = $comment->{'body'};
}
return $body;
}
p.
As you can see, there was no wrapping in the format_comment, and I could not find it whereever format_comment is called either.
Comment 6•15 years ago
|
||
Hey thomas. That's right, you have to find where wrap_comment was removed and put it back.
Assignee | ||
Comment 7•15 years ago
|
||
Ah, I get it. It seems bug 457657 introduced this new formatting, and was just intended for emails. However, the change also affected XML content.
Pretty sure there is a smarter way about this, but with my limited perl knowledge, this is all I can do to fix it.
With my previous patch, HTML was wrapped, XML was not (as intended), but also Email was not wrapped (that is what you meant by "find where it was removed
Assignee | ||
Comment 8•15 years ago
|
||
sorry, hit the wrong key...., continuing comment 7:
...
With my previous patch, HTML was wrapped, XML was not (as intended), but also Email was not wrapped (that is what you meant by "find where it was removed" I guess). With the v2 patch email now gets wrapped again.
I know this is not pretty, but it fixes the regression introduced.
Assignee | ||
Updated•15 years ago
|
Attachment #393815 -
Flags: review?(gerv)
Assignee | ||
Comment 9•15 years ago
|
||
Me again. didn't submit the previous patch correctly, here it goes. Previous 2 comments still apply to this patch.
Attachment #393634 -
Attachment is obsolete: true
Attachment #393815 -
Attachment is obsolete: true
Attachment #393816 -
Flags: review?(gerv)
Attachment #393815 -
Flags: review?(gerv)
Comment 10•15 years ago
|
||
I don't think Gerv is the right reviewer. Maybe one of the people who wrote or reviewed the original patch would be better.
Assignee | ||
Updated•15 years ago
|
Attachment #393816 -
Flags: review?(gerv) → review?(wurblzap)
Assignee | ||
Comment 11•15 years ago
|
||
Comment on attachment 393816 [details] [diff] [review]
patch v3
:) I used the linked document to find the reviewer for BugMail.pm. I will try Marc, who provided the original patch.
Comment 12•15 years ago
|
||
Comment on attachment 393816 [details] [diff] [review]
patch v3
This looks really good to me at first glance. I'll take a closer look as soon as possible.
Reporter | ||
Comment 13•15 years ago
|
||
Hi Marc, Eclipse.org will be moving to 3.4.1 this weekend. Any chance of iterating on the patch this week?
Updated•15 years ago
|
Attachment #393816 -
Flags: review?(wurblzap) → review-
Comment 14•15 years ago
|
||
Comment on attachment 393816 [details] [diff] [review]
patch v3
I can already tell you that this will break everywhere that we were previously using comment wrapping--you'll want to check the original patch and revert the correct sections.
Reporter | ||
Comment 15•15 years ago
|
||
We're running with this patch here and wrapping still works in the web ui and in emails but the xml output is fixed. Of course there may be other scenarios that I'm unaware of, Marc?
Comment 16•15 years ago
|
||
Wrapping is only working in the UI because you are depending on the CSS to do it for you. It is actually going to be much wider than you're used to and will vary between browsers.
Comment 17•15 years ago
|
||
Fixing this bug may also fix bug 514703, or vice versa.
Comment 18•15 years ago
|
||
(In reply to comment #16)
> Wrapping is only working in the UI because you are depending on the CSS to do
> it for you. It is actually going to be much wider than you're used to and will
> vary between browsers.
I don't think this is actually a problem.
Bug 457657 is the bug where the wrapping was introduced to format_comment. (attachment 379597 [details] [diff] [review] is very close to what was committed to CVS.)
The only place where wrapping was removed as in BugMail.pm - which Thomas's patch reintroduces.
Wrapping is still done for the web UI in ./template/en/default/bug/comments.html.tmpl
The handling for email will be slightly different after this patch - because any text introduced in a long-winded translation of the format_comment template will be also be wrapped. But if anything, that seems like a change for the better.
(Don't know if spontaneous reviews from random non-developers are appreciated, but the patch looks correct to me; I don't see any problems in it.)
Comment 19•15 years ago
|
||
Comment on attachment 393816 [details] [diff] [review]
patch v3
I like patch v3. I like the idea of handling comments generally unwrapped, and do the wrapping only when it needs to happen. And as you say, show_bug.cgi and bugmail are already covered.
Unfortunately, the patch doesn't apply to HEAD any more, so I can't test; please upload an unrotted patch. I'll see whether I come accross other places in need of wrapping.
Assignee | ||
Updated•15 years ago
|
Attachment #393816 -
Attachment is obsolete: true
Assignee | ||
Comment 20•15 years ago
|
||
Hi Marc
Thanks for looking at this. Here is an updated version that works against HEAD again.
Attachment #403812 -
Flags: review?(wurblzap)
Updated•15 years ago
|
Flags: blocking3.4.3+
Updated•15 years ago
|
Assignee: query-and-buglist → thomas.ehrnhoefer
Status: NEW → ASSIGNED
Comment 21•15 years ago
|
||
As we got no comment from Marc for almost a month, I'm going to review this patch.
Comment 22•15 years ago
|
||
Comment on attachment 403812 [details] [diff] [review]
patch v.4
>Index: webtools/bugzilla/Bugzilla/BugMail.pm
>+ $comment->{body} = ($comment->{'already_wrapped'} ? $comment->{body} : wrap_comment($comment->{body}));
Nit: parentheses are useless and can be removed on checkin (I know they were present in the original code; that's not a big deal).
The patch is correct and is working fine, for bugmails, XML files and bugs displayed in the web browser (including format for printing, aka long format). r=LpSolit
And yes, the wrapping happens independently of CSS; comments.html.tmpl handles this correctly.
Attachment #403812 -
Flags: review?(wurblzap) → review+
Updated•15 years ago
|
Flags: approval3.4+
Flags: approval+
Comment 23•15 years ago
|
||
tip:
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v <-- BugMail.pm
new revision: 1.129; previous revision: 1.128
done
Checking in template/en/default/bug/format_comment.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/format_comment.txt.tmpl,v <-- format_comment.txt.tmpl
new revision: 1.3; previous revision: 1.2
done
3.4.2:
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v <-- BugMail.pm
new revision: 1.124.2.5; previous revision: 1.124.2.4
done
Checking in template/en/default/bug/format_comment.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/format_comment.txt.tmpl,v <-- format_comment.txt.tmpl
new revision: 1.1.2.3; previous revision: 1.1.2.2
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 24•15 years ago
|
||
Thanks Frederic for reviewing this and applying the fix!
Comment 27•15 years ago
|
||
(In reply to comment #26)
> No problem. Why resetting the version field??
I don't know how this happened. It was not intentional.
You need to log in
before you can comment on or make changes to this bug.
Description
•