282349 - Comments are forced to being left-justified

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

[Jussi Sirpoma]

Bug 11901 improved bugzilla's output formatting substantially, but in the
process it lost the ability to insert peaces of code or other preformatted text
to comments. The below link shows a peace of bugzilla code copied to a comment:

<http://landfill.bugzilla.org/bugzilla-tip/show_bug.cgi?id=2383#c2>

To fix this, I would suggest adding a tag which could be used to turn off
wrapping when necessary. For example:

<verbatim>
    Preformatted text.
</verbatim>

Comment 1

[Max Kanat-Alexander]

(In reply to comment #0)
> Bug 11901 improved bugzilla's output formatting substantially, but in the
> process it lost the ability to insert peaces of code or other preformatted text
> to comments. 

  Nothing was lost. Bugzilla never had that ability, unless you hacked it.

  You can also now add a > to the front of a line to make it not wrap.

Comment 2

[Jussi Sirpoma]

(In reply to comment #1)
> Nothing was lost. Bugzilla never had that ability, unless you hacked it.
> You can also now add a > to the front of a line to make it not wrap.

You are right, but before output formatting bugzilla had kind of WYSIWYG comment
writing where everything was preformatted. So I was able to insert indented code
etc. without loosing readability.  See the link in comment 0 which contains a
peace of code copied from User.pm. If I would copy the same code here, it would
show up indented correctly.

Comment 3

[Max Kanat-Alexander]

Oh, that's a bug! :-)

Comment 4

[Max Kanat-Alexander]

Attachment: Go back to using Text::Wrap, with same functionality

OK, so basically, I discovered that this behavior of Text::Autoformat is not
changeable.

So I went back to how we used to wrap all attachment comments, but put that
code into the wrap_comment sub.

So this means that we're back to using Text::Wrap again, which I really like
more anyhow, and which I think is probably also faster.

Comment 5

[Max Kanat-Alexander]

This pretty much definitely blocks any release.

Here's a testing installation, and a link to a bug where I've posted many
test-case comments:

http://landfill.bugzilla.org/bz282349/show_bug.cgi?id=2323

I didn't want to make this a "back out bug 11901 and check it back in" because
this only affects a localized part of that patch.

Comment 6

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

Comment on attachment 174473 [details] [diff] [review]
Go back to using Text::Wrap, with same functionality

This seems ok if it doesn't regress functionality, but it should get second
review from someone who reviewed the fix for bug 11901 and is familiar with the
decision to go with Text::Autoformat in that patch.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 174473 [details] [diff] [review]
Go back to using Text::Wrap, with same functionality

OK, that would be Gerv.

Gerv, could you respond to Myk's comment above?

Comment 8

[Gervase Markham [:gerv]]

The reason we chose Text::Autoformat was issue 2 in bug 11901, comment 229.
However, you appear to have fixed that with WRAP_IGNORE in your patch (which,
BTW, should be hard coded rather than yet another global variable; it's _so_ not
going to change.)

I find it hard to believe that AutoFormat a) does what you say it does, and b)
isn't configurable in it. But if we've solve issue #2, then I have no particular
problem with switching back to Text::Wrap.

Until, of course, we find some more comments that _that_ can't cope with...

Gerv

Comment 9

[Max Kanat-Alexander]

It does indeed do what I say it does, and it's definitely not configurable. I
pored over the Text::Autoformat source for hours.

So if I switch WRAP_IGNORE to be a literal instead of a constant, is that a r+?

Comment 10

[Max Kanat-Alexander]

Oops. Gerv wasn't on CC. Gerv -- see my last comment.

Comment 11

[Gervase Markham [:gerv]]

Comment on attachment 174473 [details] [diff] [review]
Go back to using Text::Wrap, with same functionality

r=gerv with that change. I hope there's not a performance hit associated with
calling wrap for every line rather than once per comment...

Gerv

Comment 12

[Max Kanat-Alexander]

Attachment: v1.1

OK, carrying forward r+ from Gerv based on the comments above.

Comment 13

[Max Kanat-Alexander]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.356; previous revision: 1.355
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.23; previous revision: 1.22
done