46306 - html-to-plaintext replies have > on blank lines

Status: VERIFIED FIXED

(Core :: DOM: Serializers, defect, P4)

Description

[Akkana Peck]

When someone sends me a plaintext message, and I reply to it, the result comes
out looking like this:

> Akkana, how is Mozilla coming along?
>
It's looking better!

In other words, there's no blank line between the quoted text and the new text,
but instead, there's a line consisting of only the quote character.  This looks
wrong and is harder to read than the standard of a blank line.

Comment 1

[Akkana Peck]

Plaintext mail is fundamental; we should do the simple stuff right.

Cc'ed Ben since I'm not sure whether this is something he's been looking at or
not; Ben, feel free to take this bug if you want it.  I just wanted to make sure
it was in the system and we both knew about it.

Comment 2

[Ben Bucksch (:BenB)]

Yes, please fix this bug. IMO, we can't release the product, if it outputs such
nonsense. Yes, it's an esthetical problem, but a very visible one. For me, this
is a reason to drop a mailer.

Akk, just make sure your fixes don't collide with my work (e.g. on bug 44439).

It would be very kind, if you could check the result, if
"user_pref("mail.quoteasblock", true);" is sat, as well.

Comment 3

[Akkana Peck]

*** Bug 38492 has been marked as a duplicate of this bug. ***

Comment 4

[Ben Bucksch (:BenB)]

see the dup bug for additional discussion.

Comment 5

[rubydoo123]

setting to nsbeta3+

Comment 6

[rubydoo123]

setting priority in status whiteboard

Comment 7

[Daniel Bratell]

I can fix this if nobody else has started working on it (speak up or forever 
etc...). It's quite simple but I need some advise on what way is best. My 
options right now are:

1. Change the citers (plaintext) to strip trailing newlines when doing the
   insertion of '>'. This means the same code in the AOL citer and the Internet
   citer. The drawback is that the output will be different between plaintext
   replies and html replies since the html reply doesn't strip trailing empty
   lines.

2. Change InsertAsQuotation to strip the trailing newlines. Then the output will
   be the same for all citers with the code on only one place. The bad part here 
   is that the text is |const| so I had to do a copy of the text before
   stripping newlines. With the strings we have today that's bad for
   performance and bad for memory usage.

3. The same as two, but strip the const from the InsertAsQuotation and let the
   function modify the passed string. This is ugly, but would probably work.

4. Pass a length parameter to InsertAsPlaintextQuotation and
   InsertAsCitedQuotation so that they only quote the first |n| characters in
   the text. This requires the citer to change also so that it takes a length
   parameter. 

In all cases InsertAsPlaintextQuotation adds an empty line after the text has 
been quoted.

I've implemented 2. and it works fine, but my favorite is number 4. Comments?

I've not made anything to check how it collides with Ben's work but it's (as 
I said) simple changes.

Comment 8

[Akkana Peck]

It would be great if you could work on this!

My favorite is 1 (the citers are only used for mail quoting anyway, so changing
them to work more conveniently is no problem), but if you like 4 best, that
would be fine with me.  I agree that 2 is not really worth the performance hit.

If you decide 4 is the best approach, you should probably allow for a length
parameter of 0 to mean "quote the whole string".

Comment 9

[Daniel Bratell]

Attachment: Simple fix

Comment 10

[Daniel Bratell]

I've attached my fix. It strips trailing newlines in the Internet Citer and adds 
an empty line after quoted text so that the caret won't end up next to the 
quoted text.

Akkana, what do you think? It does what it's supposed to do.

I don't know the procedure for non-Netscape people working on nsbeta3+ bugs. Do 
I still need a review and then an approval? Or maybe you could do it.

Comment 11

[Ben Bucksch (:BenB)]

Daniel, tnx for fixing. Patch doesn't collide with any of my work. See tinderbox
for checkin rules: You're not a NS employee, so the nsbeta3+ is meaningless for
you. Technically, you need mozilla.org approval. (Although I consider this being
dumb.)

Comment 12

[Akkana Peck]

The patch looks great, except ... where are the symbols CR and LF coming from,
are they XP, and are they unichar?  I see a PRUnichar newline earlier in that
function ... wouldn't it be safer to use that, and have a new PRUnichar cr to
compare against?

Comment 13

[Daniel Bratell]

Well, as a matter of fact, I'm not sure. I started defining my own CR and LF but 
got warnings because of some collision so I used the existing ones. They work, 
that's all I know (not very reassuring :-) ). I expect CR to be just that 
(Carriage Return ASCII 10) and LF to be Line Feed (ASCII 13). They could come 
from nsCRT which defines them 
<http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsCRT.h#32> as '\015' and 
'\012'. 

But since you bring it up, I'll switch to a cr locally defined and the existing 
newline. If it's ok with you, I'll seek approval from higher powers. :-)

Index: nsInternetCiter.cpp
===================================================================
RCS file: /cvsroot/mozilla/editor/base/nsInternetCiter.cpp,v
retrieving revision 1.10
diff -u -r1.10 nsInternetCiter.cpp
--- nsInternetCiter.cpp 2000/04/21 09:33:32     1.10
+++ nsInternetCiter.cpp 2000/08/04 19:55:55
@@ -76,10 +76,20 @@
 nsInternetCiter::GetCiteString(const nsString& aInString, nsString& aOutString)

 {
   PRUnichar newline ('\n');      // Not XP!
+  PRUnichar cr('\r');
   PRInt32 i = 0;
   PRInt32 length = aInString.Length();
   aOutString.SetLength(0);
   PRUnichar uch = newline;
+
+  // Strip trailing new lines which will otherwise turn up
+  // as ugly quoted empty lines.
+  while(length > 0 &&
+        (aInString[length-1] == cr ||
+         aInString[length-1] == newline))
+  {
+    --length;
+  }

   // Loop over the string:
   while (i < length)

Comment 14

[Akkana Peck]

Oh, if it's in nsCRT.h (love that namespace pollution!  I would have thought
anything there would be nsCRT::CR or nsCR), then that's okay.  So actually,
either version of your patch is fine with me -- whichever one you prefer.  Guess
I don't need that "newline" variable there, either, and ought to yank it at some
point.

Comment 15

[Daniel Bratell]

Ok, than I take, ah, number two. I'm more confident that it won't bring any 
problems in the future than the previous. Just waiting for appropval from some 
mozilla.org-people.

Comment 16

[Daniel Bratell]

Fix checked in.

Comment 17

[sujay]

verified in 8/9 build.