1392052 - Improve text to HTML conversion when shift modifier is used

Status: RESOLVED FIXED

(MailNews Core :: Composition, enhancement)

Description

[Jorg K (CEST = GMT+2)]

In bug 731688 we implemented the shift modifier for "Edit draft", "Edit as new", "New message from template".

Plain text messages can be upgraded to HTML in convert_plaintext_body_to_html() in mimedrft.cpp.

This function sticks the message into a <pre> block. That's quite suboptimal. What should happen instead is described in bug 731688 comment #42 (and discussion above). Basically we just want to convert text to HTML as we do for a HTML reply of a flowed plaintext message. It's yet to be determined what should happen when upgrading non-flowed plaintext messages. Maybe where we do the upgrade we might not have that information any more.

If you research is correct, the reply processing is done in
https://dxr.mozilla.org/comm-central/rev/d2b81af84925c478d9a7c2ef888351e1ba9b5fce/mailnews/mime/src/mimetpla.cpp#365
https://dxr.mozilla.org/comm-central/rev/d2b81af84925c478d9a7c2ef888351e1ba9b5fce/mailnews/mime/src/mimetpfl.cpp#413
and it would be quite hard to understand that logic and port it to convert_plaintext_body_to_html().

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: Test e-mail non-flowed

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1392052-text-to-html.patch (v1) WIP

This detects quotes and turns text into HTML in non-flowed cases.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: Test e-mail non-flowed (v2).

Removed trailing space not expected in non-flowed e-mail.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: Test e-mail non-flowed (v3).

With signature.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: Test e-mail flowed (v1).

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1392052-text-to-html.patch (v2)

This should do it.

The test is to load the two test messages, view them, then "edit as new" in a HTML composition, then send with auto-downgrade. The sent result should be the same at the original. You can also compare to a reply, but then of course, the quote level is one higher.

There is one quirk. This line in the flowed message
  > Level 0
gets into the function as
 > Level 0
so it looks like flowed MIME processing stripped the leading space.
If you start with | > Level 0| the function sees |> Level 0| and that is detected as quote.

So for that line, the result differs, but there's nothing I can do about it, what doesn't get to me, I can't process.

I think this is pretty good and much better than whacking it all into a <pre>.

Note that class="moz-quote-pre" is used here just like in bug 1392064.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1392052-text-to-html.patch (v2b)

Fixed comment.

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8899297 [details] [diff] [review]
1392052-text-to-html.patch (v2b)

Actually, that isn't right. Flowed shouldn't insert the <pre>. I'll fix it tonight.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1392052-text-to-html.patch (v3).

Now only using <pre> for non-flowed messages.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1392052-text-to-html.patch (v3b).

Refreshed after bug 1392529 ripped right through it.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1392052-text-to-html.patch (v3c).

Sorry about the noise, I had to refresh it again. Also now using ToNewCString() instead of strdup().

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1392052-text-to-html.patch (v4).

Removed the clumsy copying of the escaped string. We can just traverse it instead instead of copying it.

Comment 13

[:aceman]

Comment on attachment 8900023 [details] [diff] [review]
1392052-text-to-html.patch (v4).

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

OK, I think I see the effect now.
For flowed messages the result is the same, except for some lost spaces before > .
For non-flowed messages, the indentation in the resulting message (opened sample, edit as new in HTML, send to a recipient that needs plain text) seems better, as rendered with the color bars inside TB message preview.

::: mailnews/mime/src/mimedrft.cpp
@@ +1150,5 @@
> +    // Eat space following quote character, for non-flowed already eaten above.
> +    if (quoteLevel > 0 && isFlowed && *p == ' ')
> +      p++;
> +
> +    // Close any open sigantures if we find a quote. Strange, that shouldn't happen.

signatures

@@ +1220,5 @@
> +      newBody.AppendLiteral("</pre></blockquote>");
> +    prevQuoteLevel--;
> +  }
> +
> +  // Close any open sigantures.

signatures

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #13)
> For flowed messages the result is the same, except for some lost spaces
> before > .
Yes, as explained in comment #6. The function is called with the space already removed. In fact, you can see this without the patch. Edit the flowed test message as new which has |  > Level 0|. You'll see this in ThunderHTMLedit:

<pre>Level 0
 &gt; Level 0

So the text delivered to the function is missing the space already. Nothing the function can to to fix this.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5a4764a25a0e
Improve text to HTML conversion for 'Edit as new message' and other commands. r=aceman

Comment 16

[Magnus Melin [:mkmelin]]

Uplift to tb 52? x-ref bug 1411741

Comment 17

[Jorg K (CEST = GMT+2)]

No way. That changes behaviour drastically and comes in a whole package of bug 731688 and friends, bug 1389771 and bug 1389083, although the latter two might not be required.