688205 - quoted text inside comments should wrap

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[:glob ✱]

long lines of quoted text are not wrapped to fit within the comment box boundary, resulting in a messy looking layout.

eg.

> * Combine checkForCrashes and cleanupAndCheckForCrashes into one API on the larger MozProcess API - just call it check_for_crashes and have it optionally perform cleanup steps, based on some parameter that it is passed.

removing the "white-space: pre" style from ".bz_comment_text span.quote" fixes this, however the "> " prefix only appears on the first line of quoted text, rather than on all lines.

Comment 1

[Frédéric Buclin]

This is intentional, to not break the UI when pasting reviews (you have code included).

Comment 2

[Max Kanat-Alexander]

This is indeed intentional; perhaps we should wait for Pretty before we make any decisions about this.

Comment 6

[teo8976]

- Are you handling quoted text and code snippets as if they were the same thing?? If that is the case, then it's a really bad design. 
Being able to not wrap code snippets (provided that they get boxed within a horizontally scrollable container, and that the option to wrap is available) is fine. Not wrapping quoted text doesn't make any sense. 

If a specific markup for code does exist, then it doesn't make sense to treat quoted text under the assumption that it probably contains code, because it would be like adjusting the behavior of something to a bad use of it.

- The wrap/unwrap option, that has been added as per #743090, should be per-code-snippet, not per-comment. While it's per-comment, it should not be applied to quotes, only to code. If made available per-item (as it should), then it may or may not be available for quoted texts (it's a mostly useless option for a quote, but the possibility to choose cannot do any damage), but certainly quoted text should be wrapped by default

- I notice that the unwrapped quotes here are boxed in a container with a scrollbar. That's "fine" (I mean, once accepted that they are not wrapped, which is wrong, it degrades in a graceful way). On other instances of Bugzilla (e.g. at Wireshark, where they say they are using the latest version) I've seen something much worse and definitely unacceptable: a long line that overflows the container. Which is the current version? I hope it's the one we're seing in this very page (the box with the horizontal scrollbar)

Comment 7

[Albert Ting]

Attachment: v1

Suggested patch that wraps quoted text, but only when not in markdown mode.  Ticket 1139150 addressed wrapping quoted text in Markdown mode.

Comment 8

[Albert Ting]

Attachment: v2

Updated version, where it retains the number of ">".  Also updates WebService::Bug::render_comment to also wrap cited text. 

Note, posted comments uses comment.body_full, preview just uses Bug.render_comment(text). Not sure there's a better way to consolidate the two calls.

Comment 9

[Frédéric Buclin]

If quoted text is unconditionnally wrapped, then you must also revert bug 524915. It doesn't make sense to keep both I think.

Comment 10

[:glob ✱]

Comment on attachment 8591830 [details] [diff] [review]
v2

i really wanted to review this, but it's increasingly unlikely i'll have time.  opening up the review for others.

Comment 11

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8591830 [details] [diff] [review]
v2

Review of attachment 8591830 [details] [diff] [review]:
-----------------------------------------------------------------

Fails tests due to undocumented function. Also fix the other small errors and should be a quick r+

t/011pod.t ........... 214/380 
#   Failed test 'Bugzilla/Util.pm POD coverage is 97%. Undocumented methods: wrap_cite'
#   at t/011pod.t line 109.
# Looks like you failed 1 test of 380.

::: Bugzilla/Util.pm~
@@ +472,5 @@
> +    # Don't mess with tabs.
> +    local $Text::Wrap::unexpand = 0;
> +
> +    foreach my $line (split(/\r\n|\r|\n/, $comment)) {
> +      if ($line =~ qr/^(>+ *)/) {

it is more conventional to use $line =~ /^(>+ *)/ here

@@ +473,5 @@
> +    local $Text::Wrap::unexpand = 0;
> +
> +    foreach my $line (split(/\r\n|\r|\n/, $comment)) {
> +      if ($line =~ qr/^(>+ *)/) {
> +        $wrappedcomment .= (wrap('', $1, $line) . "\n");

remove the extra parenthesis. (lisp programmer detected)

@@ +475,5 @@
> +    foreach my $line (split(/\r\n|\r|\n/, $comment)) {
> +      if ($line =~ qr/^(>+ *)/) {
> +        $wrappedcomment .= (wrap('', $1, $line) . "\n");
> +      } else {
> +        $wrappedcomment .= ($line . "\n");

these parens are also not needed.

Comment 12

[Albert Ting]

Attachment: v3

Updated patch to address concerns in comment #11. And yup, was paid to do scheme programming for many years.

Comment 13

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8702763 [details] [diff] [review]
v3

Review of attachment 8702763 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan. Code works and helped me discover a related bug.

::: Bugzilla/WebService/Bug.pm~
@@ +353,4 @@
>      my $bug = $params->{id} ? Bugzilla::Bug->check($params->{id}) : undef;
>  
>      my $markdown = $params->{markdown} ? 1 : 0;
> +    my $tmpl = $markdown ? '[% text FILTER markdown(bug, { is_markdown => 1 }) %]' : '[% text FILTER wrap_cite FILTER markdown(bug) %]';

this line is too long -- I'll fix it on commit.

Comment 14

[Albert Ting]

FYI, I'm using this patch for 5.0

Comment 15

[Frédéric Buclin]

(In reply to Albert Ting from comment #14)
> FYI, I'm using this patch for 5.0

This is not a bug fix, but an enhancement. 5.0 is locked for security and bug fixes only.

Comment 16

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/bugzilla/bugzilla.git
   85ca54b..8f405c0  master -> master