html-to-plaintext replies have > on blank lines

VERIFIED FIXED in M18

Status

()

Core
Serializers
P4
normal
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: Akkana Peck, Assigned: Akkana Peck)

Tracking

({polish})

Trunk
x86
All
polish
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta3+][p:4] Fix in hand)

Attachments

(1 attachment)

(Assignee)

Description

18 years ago
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.
(Assignee)

Comment 1

18 years ago
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.
Status: NEW → ASSIGNED
Keywords: nsbeta3, polish
Target Milestone: --- → M19

Comment 2

18 years ago
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.
(Assignee)

Comment 3

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

Comment 4

18 years ago
see the dup bug for additional discussion.

Comment 5

18 years ago
setting to nsbeta3+
Whiteboard: nsbeta3+
Target Milestone: M19 → M18

Comment 6

18 years ago
setting priority in status whiteboard
Priority: P3 → P4
Whiteboard: nsbeta3+ → [nsbeta3+][p:4]

Comment 7

18 years ago
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.
(Assignee)

Comment 8

18 years ago
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

18 years ago
Created attachment 12257 [details] [diff] [review]
Simple fix

Updated

18 years ago
Keywords: review
Whiteboard: [nsbeta3+][p:4] → [nsbeta3+][p:4] Fix in hand

Comment 10

18 years ago
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

18 years ago
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.)
(Assignee)

Comment 12

18 years ago
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

18 years ago
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)
(Assignee)

Comment 14

18 years ago
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

18 years ago
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.
Keywords: review → approval

Comment 16

18 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 17

18 years ago
verified in 8/9 build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.