Bug 11901 : Change Bugzilla comments line-wrapping policy

Patch by Max Kanat-Alexander <mkanat@kerio.com>  r=gerv,justdave  a=justdave

14 files changed, 95 insertions, 47 deletions

diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm
index 768b0e684..3ef3cc634 100644
--- a/Bugzilla/Constants.pm
+++ b/Bugzilla/Constants.pm
@@ -64,6 +64,8 @@ use base qw(Exporter);
 
     DEFAULT_COLUMN_LIST
     DEFAULT_QUERY_NAME
+
+    COMMENT_COLS
 );
 
 @Bugzilla::Constants::EXPORT_OK = qw(contenttypes);
@@ -212,4 +214,7 @@ use constant DEFAULT_COLUMN_LIST => (
 # for the default settings.
 use constant DEFAULT_QUERY_NAME => '(Default query)';
 
+# The column length for displayed (and wrapped) bug comments.
+use constant COMMENT_COLS => 80;
+
 1;
diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm
index d4fcbe27c..aea32a2b3 100644
--- a/Bugzilla/Template.pm
+++ b/Bugzilla/Template.pm
@@ -378,6 +378,9 @@ sub create {
                      1
                      ],
 
+            # Wrap a displayed comment to the appropriate length
+            wrap_comment => \&Bugzilla::Util::wrap_comment,
+
             # We force filtering of every variable in key security-critical
             # places; we have a none filter for people to use when they 
             # really, really don't want a variable to be changed.
diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
index 7bb173e05..6175e0ab2 100644
--- a/Bugzilla/Util.pm
+++ b/Bugzilla/Util.pm
@@ -22,6 +22,7 @@
 #                 Jacob Steenhagen <jake@bugzilla.org>
 #                 Bradley Baetz <bbaetz@student.usyd.edu.au>
 #                 Christopher Aillon <christopher@aillon.com>
+#                 Max Kanat-Alexander <mkanat@kerio.com>
 
 package Bugzilla::Util;
 
@@ -32,14 +33,37 @@ use base qw(Exporter);
                              html_quote url_quote value_quote xml_quote
                              css_class_quote
                              lsearch max min
-                             trim diff_strings
+                             trim diff_strings wrap_comment
                              format_time format_time_decimal
                              file_mod_time);
 
 use Bugzilla::Config;
 use Bugzilla::Error;
+use Bugzilla::Constants;
 use Date::Parse;
 use Date::Format;
+use Text::Autoformat qw(autoformat break_wrap);
+
+our $autoformat_options = {
+    # Reformat all paragraphs, not just the first one.
+    all        => 1,
+    # Break only on spaces, and let long lines overflow.
+    break      => break_wrap,
+    # Columns are COMMENT_COLS wide.
+    right      => COMMENT_COLS,
+    # Don't reformat into perfect paragraphs, just wrap.
+    fill       => 0,
+    # Don't compress whitespace.
+    squeeze    => 0,
+    # Lines starting with ">" are not wrapped.
+    ignore     => qr/^>/,
+    # Don't re-arrange numbered lists.
+    renumber   => 0,
+    # Keep short lines at the end of paragraphs as-is.
+    widow      => 0,
+    # Even if a paragraph looks centered, don't "auto-center" it.
+    autocentre => 0,
+};
 
 # This is from the perlsec page, slightly modifed to remove a warning
 # From that page:
@@ -178,6 +202,11 @@ sub diff_strings {
     return ($removed, $added);
 }
 
+sub wrap_comment ($) {
+    my ($comment) = @_;
+    return autoformat($comment, $autoformat_options);
+}
+
 sub format_time {
     my ($time) = @_;
 
@@ -277,6 +306,7 @@ Bugzilla::Util - Generic utility functions for bugzilla
   # Functions for manipulating strings
   $val = trim(" abc ");
   ($removed, $added) = diff_strings($old, $new);
+  $wrapped = wrap_comment($comment);
 
   # Functions for formatting time
   format_time($time);
@@ -402,6 +432,16 @@ compared to the old one. Returns a list, where the first entry is a scalar
 containing removed items, and the second entry is a scalar containing added
 items.
 
+=item C<wrap_comment($comment)>
+
+Takes a bug comment, and wraps it to the appropriate length. The length is
+currently specified in C<Bugzilla::Constants::COMMENT_COLS>. Lines beginning
+with ">" are assumed to be quotes, and they will not be wrapped.
+
+The intended use of this function is to wrap comments that are about to be
+displayed or emailed. Generally, wrapped text should not be stored in the
+database.
+
 =back
 
 =head2 Formatting Time
diff --git a/attachment.cgi b/attachment.cgi
index a5407e8b0..d58395efc 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -893,11 +893,6 @@ sub insert
   my $comment = "Created an attachment (id=$attachid)\n$::FORM{'description'}\n";
   $comment .= ("\n" . $::FORM{'comment'}) if $::FORM{'comment'};
 
-  use Text::Wrap;
-  $Text::Wrap::columns = 80;
-  $Text::Wrap::huge = 'overflow';
-  $comment = Text::Wrap::wrap('', '', $comment);
-
   AppendComment($::FORM{'bugid'}, 
                 Bugzilla->user->login,
                 $comment,
@@ -1151,35 +1146,17 @@ sub update
   # add the comment to the bug.
   if ( $::FORM{'comment'} )
   {
-    use Text::Wrap;
-    $Text::Wrap::columns = 80;
-    $Text::Wrap::huge = 'wrap';
-
     # Prepend a string to the comment to let users know that the comment came from
     # the "edit attachment" screen.
     my $comment = qq|(From update of attachment $::FORM{'id'})\n| . $::FORM{'comment'};
 
-    my $wrappedcomment = "";
-    foreach my $line (split(/\r\n|\r|\n/, $comment))
-    {
-      if ( $line =~ /^>/ )
-      {
-        $wrappedcomment .= $line . "\n";
-      }
-      else
-      {
-        $wrappedcomment .= wrap('', '', $line) . "\n";
-      }
-    }
-
     # Get the user's login name since the AppendComment function needs it.
     my $who = DBID_to_name($::userid);
     # Mention $::userid again so Perl doesn't give me a warning about it.
     my $neverused = $::userid;
 
     # Append the comment to the list of comments in the database.
-    AppendComment($bugid, $who, $wrappedcomment, $::FORM{'isprivate'}, $timestamp);
-
+    AppendComment($bugid, $who, $comment, $::FORM{'isprivate'}, $timestamp);
   }
   
   # Define the variables and functions that will be passed to the UI template.
diff --git a/checksetup.pl b/checksetup.pl
index e79598029..3655087dd 100755
--- a/checksetup.pl
+++ b/checksetup.pl
@@ -300,9 +300,9 @@ my $modules = [
         version => '2.08' 
     }, 
     { 
-        name => 'Text::Wrap', 
-        version => '2001.0131' 
-    },
+        name => 'Text::Autoformat', 
+        version => '0' 
+    }, 
     { 
         name => 'Mail::Mailer', 
         version => '1.65'
@@ -1262,6 +1262,7 @@ END
                 csv => sub { return $_; },
                 unitconvert => sub { return $_; },
                 time => sub { return $_; },
+                wrap_comment => sub { return $_; },
                 none => sub { return $_; } ,
                },
            }) || die ("Could not create Template Provider: "
@@ -1836,6 +1837,7 @@ $table{longdescs} =
     work_time decimal(5,2) not null default 0,
     thetext mediumtext,
     isprivate tinyint not null default 0,
+    already_wrapped tinyint not null default 0,
     index(bug_id),
     index(who),
     index(bug_when),
@@ -4238,6 +4240,22 @@ AddField("profiles", "extern_id", "varchar(64)");
 AddField('flagtypes', 'grant_group_id', 'mediumint null');
 AddField('flagtypes', 'request_group_id', 'mediumint null');
 
+# 2005-01-29 - mkanat@kerio.com
+if (!GetFieldDef('longdescs', 'already_wrapped')) {
+    AddField('longdescs', 'already_wrapped', 'tinyint not null default 0');
+    # Old, pre-wrapped comments should not be auto-wrapped
+    $dbh->do('UPDATE longdescs SET already_wrapped = 1');
+    # If an old comment doesn't have a newline in the first 80 characters,
+    # (or doesn't contain a newline at all) and it contains a space,
+    # then it's probably a mis-wrapped comment and we should wrap it
+    # at display-time.
+    print "Fixing old, mis-wrapped comments...\n";
+    $dbh->do(q{UPDATE longdescs SET already_wrapped = 0
+                WHERE ( POSITION('\n' IN thetext ) > 80
+                        OR POSITION('\n' IN thetext ) = 0 )
+                  AND SUBSTRING(thetext FROM 1 FOR 80) LIKE '% %'});
+}
+
 # If you had to change the --TABLE-- definition in any way, then add your
 # differential change code *** A B O V E *** this comment.
 #
diff --git a/globals.pl b/globals.pl
index d793a4659..c3dde532f 100644
--- a/globals.pl
+++ b/globals.pl
@@ -970,7 +970,8 @@ sub GetLongDescriptionAsText {
     my $count = 0;
     my $anyprivate = 0;
     my ($query) = ("SELECT profiles.login_name, DATE_FORMAT(longdescs.bug_when,'%Y.%m.%d %H:%i'), " .
-                   "       longdescs.thetext, longdescs.isprivate " .
+                   "       longdescs.thetext, longdescs.isprivate, " .
+                   "       longdescs.already_wrapped " .
                    "FROM   longdescs, profiles " .
                    "WHERE  profiles.userid = longdescs.who " .
                    "AND    longdescs.bug_id = $id ");
@@ -989,7 +990,8 @@ sub GetLongDescriptionAsText {
     $query .= "ORDER BY longdescs.bug_when";
     SendSQL($query);
     while (MoreSQLData()) {
-        my ($who, $when, $text, $isprivate, $work_time) = (FetchSQLData());
+        my ($who, $when, $text, $isprivate, $work_time, $already_wrapped) = 
+            (FetchSQLData());
         if ($count) {
             $result .= "\n\n------- Additional comment #$count from $who".Param('emailsuffix')."  ".
                 Bugzilla::Util::format_time($when) . " -------\n";
@@ -997,7 +999,7 @@ sub GetLongDescriptionAsText {
         if (($isprivate > 0) && Param("insidergroup")) {
             $anyprivate = 1;
         }
-        $result .= $text;
+        $result .= ($already_wrapped ? $text : wrap_comment($text));
         $count++;
     }
 
diff --git a/t/004template.t b/t/004template.t
index 1e7846cd7..7f83c8702 100644
--- a/t/004template.t
+++ b/t/004template.t
@@ -107,6 +107,7 @@ foreach my $include_path (@include_paths) {
             csv       => sub { return $_ } ,
             unitconvert => sub { return $_ },
             time      => sub { return $_ } ,
+            wrap_comment => sub { return $_ },
             none      => sub { return $_ } ,
             ics       => [ sub { return sub { return $_; } }, 1] ,
         },
diff --git a/template/en/default/bug/comments.html.tmpl b/template/en/default/bug/comments.html.tmpl
index 2f82b46dc..bf41200b7 100644
--- a/template/en/default/bug/comments.html.tmpl
+++ b/template/en/default/bug/comments.html.tmpl
@@ -17,6 +17,7 @@
   # Rights Reserved.
   #
   # Contributor(s): Gervase Markham <gerv@gerv.net>
+  #                 Maxwell Kanat-Alexander <mkanat@kerio.com>
   #%]
 
 [% DEFAULT start_at = 0 mode = "show" %]
@@ -82,8 +83,13 @@
 [%# Don't indent the <pre> block, since then the spaces are displayed in the
   # generated HTML
   #%]
+[% IF comment.already_wrapped %]
+    [% wrapped_comment = comment.body %]
+[% ELSE %]
+    [% wrapped_comment = comment.body FILTER wrap_comment %]
+[% END %]
 <pre[% ' id="comment_text_' _ count _ '"' IF mode == "edit" %]>
-  [%- comment.body FILTER quoteUrls -%]
+  [%- wrapped_comment FILTER quoteUrls -%]
 </pre>
     </div>
   [% END %]
diff --git a/template/en/default/bug/create/create-guided.html.tmpl b/template/en/default/bug/create/create-guided.html.tmpl
index 47454d303..a4256a6d5 100644
--- a/template/en/default/bug/create/create-guided.html.tmpl
+++ b/template/en/default/bug/create/create-guided.html.tmpl
@@ -323,8 +323,7 @@ function PutDescription() {
       <b>Details</b>
     </td>
     <td valign="top">
-      <textarea rows="6" cols="80" name="comment"
-                wrap="hard"></textarea>
+      <textarea rows="6" cols="80" name="comment"></textarea>
       <p>
         Expand on the Summary. Please be
         as specific as possible about what is wrong.
@@ -372,7 +371,7 @@ function PutDescription() {
       <b>Steps to Reproduce</b>
     </td>
     <td valign="top">
-<textarea rows="4" cols="80" name="reproduce_steps" wrap="hard">
+<textarea rows="4" cols="80" name="reproduce_steps">
 1.
 2.
 3.
@@ -389,8 +388,7 @@ function PutDescription() {
       <b>Actual Results</b>
     </td>
     <td valign="top">
-      <textarea rows="4" cols="80" name="actual_results"
-                wrap="hard"></textarea>
+      <textarea rows="4" cols="80" name="actual_results"></textarea>
       <p>
         What happened after you performed the steps above?
       </p>
@@ -402,8 +400,7 @@ function PutDescription() {
       <b>Expected Results</b>
     </td>
     <td valign="top">
-      <textarea rows="4" cols="80" name="expected_results"
-                wrap="hard"></textarea>
+      <textarea rows="4" cols="80" name="expected_results"></textarea>
       <p>
         What should the software have done instead?
       </p>
@@ -415,8 +412,7 @@ function PutDescription() {
       <b>Additional Information</b>
     </td>
     <td valign="top">
-      <textarea rows="8" cols="80" name="additional_info"
-                wrap="hard"></textarea>
+      <textarea rows="8" cols="80" name="additional_info"></textarea>
       <p>
         Add any additional information you feel may be
         relevant to this [% terms.bug %], such as the <b>theme</b> you were
diff --git a/template/en/default/bug/create/create.html.tmpl b/template/en/default/bug/create/create.html.tmpl
index a5d5e213c..013a05f24 100644
--- a/template/en/default/bug/create/create.html.tmpl
+++ b/template/en/default/bug/create/create.html.tmpl
@@ -251,7 +251,7 @@ function set_assign_to() {
 
   <tr><td align="right" valign="top"><strong>Description:</strong></td>
     <td colspan="3">
-      <textarea wrap="hard" name="comment" rows="10" cols="80">
+      <textarea name="comment" rows="10" cols="80">
         [% IF cloned_bug_id %]
 +++ This [% terms.bug %] was initially created as a clone of [% terms.Bug %] #[% cloned_bug_id %] +++
 
diff --git a/template/en/default/bug/edit.html.tmpl b/template/en/default/bug/edit.html.tmpl
index cf2ed5d7b..613bfb4d4 100644
--- a/template/en/default/bug/edit.html.tmpl
+++ b/template/en/default/bug/edit.html.tmpl
@@ -481,7 +481,7 @@
   [% END %]
   <br>
   <a name="add_comment"></a>
-  <textarea wrap="hard" name="comment" id="comment" rows="10" cols="80"
+  <textarea name="comment" id="comment" rows="10" cols="80"
             accesskey="c"></textarea>
 
   [% IF NOT bug.cc || NOT bug.cc.contains(user.login) %]
diff --git a/template/en/default/list/edit-multiple.html.tmpl b/template/en/default/list/edit-multiple.html.tmpl
index b705ba574..c2c8bf4d8 100644
--- a/template/en/default/list/edit-multiple.html.tmpl
+++ b/template/en/default/list/edit-multiple.html.tmpl
@@ -186,7 +186,7 @@
 </table>
 
 <label for="comment"><b>Additional Comments:</b></label><br>
-<textarea id="comment" name="comment" rows="5" cols="80" wrap="hard"></textarea><br>
+<textarea id="comment" name="comment" rows="5" cols="80"></textarea><br>
 
 [% IF groups.size > 0 %]
 
diff --git a/template/en/default/pages/linked.html.tmpl b/template/en/default/pages/linked.html.tmpl
index fcb5ee9d0..bd5933094 100644
--- a/template/en/default/pages/linked.html.tmpl
+++ b/template/en/default/pages/linked.html.tmpl
@@ -32,7 +32,7 @@
 
 <p>
 <pre>
-[%- cgi.param("text") FILTER quoteUrls FILTER html -%]
+[%- cgi.param("text") FILTER wrap_comment FILTER quoteUrls FILTER html -%]
 </pre>
 </p>
 
@@ -47,7 +47,7 @@
 
 <p>
 <pre>
-[%- cgi.param("text") FILTER quoteUrls -%]
+[%- cgi.param("text") FILTER wrap_comment FILTER quoteUrls -%]
 </pre>
 </p>
 
diff --git a/template/en/default/pages/linkify.html.tmpl b/template/en/default/pages/linkify.html.tmpl
index 40cda81cb..896f7d41f 100644
--- a/template/en/default/pages/linkify.html.tmpl
+++ b/template/en/default/pages/linkify.html.tmpl
@@ -30,7 +30,7 @@
 </p>
 
 <form action="page.cgi" method="post">
-  <textarea cols="80" rows="20" name="text" wrap="hard"></textarea>
+  <textarea cols="80" rows="20" name="text"></textarea>
   <br>
   <input type="hidden" name="id" value="linked.html">
   <input value="Linkify" type="submit">