User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6 Build Identifier: Thunderbird 2.0.0.12 When choosing "start my reply above the quote" and "place my signature below my reply (above the quote)", then two blank lines are inserted between the signature and the quoted text (as opposed to only one empty before the quoted text when no signature is used at all). I manually have to delete the two lines (or at least one of them) every time. Now one might argue, that that is a question of design and preference, but actually it is NOT, because if Thunderbird inserts only one empty line after the signature or no line at all, the user CAN add line breaks to his/her signature, so that they are added automatically. But the way it's currently working the user has NO possibility to remove those lines automatically - there is nothing the user can add to the signature for that purpose. The user is obliged to delete the lines manually. So to empower the user and give him/her the choice how he/she wants to send the email, it would be good not to take away his/her freedom by automatically inserting things the user has no way to get rid of. Reproducible: Always

[quote=http://forums.mozillazine.org/viewtopic.php?p=3331974#3331974] rsx11m: "The main action seems to be happening in nsMsgCompose.cpp, where a couple of different functions handle the signature. The ones that caught my attention were ConvertAndLoadComposeWindow (http://mxr.mozilla.org/mailnews/source/mailnews/compose/src/nsMsgCompose.cpp#538) and SetSignature (http://mxr.mozilla.org/mailnews/source/mailnews/compose/src/nsMsgCompose.cpp#5003), where it appears that the second line break you see may actually come from the omitted "-- " separator when replying above the quote." [/quote]

Did you tried latest trunk?

Yes, I tried it on TB 2.0.0.12 Two other users participating in the abovementioned discussion were able to reproduce the bug: "I can reproduce the double-line with both plain-text and HTML composition as long as the quote is below the reply and the signature above the quote." rsx11m narrowed it down to: http://mxr.mozilla.org/mailnews/source/mailnews/compose/src/nsMsgCompose.cpp#2121 "With 'reply on top' two lines are inserted before the quote prefix. This is hard coded in Thunderbird." Code: if (reply_on_top == 1) mCitePrefix.AppendLiteral("\n\n");

I'm talking about trunk nightly of TB3 from here ftp://ftp.mozilla.org/pub/thunderbird/nightly/latest-trunk Could you test in it and see if it happens for you.

At the moment I cannot establish an FTP connection. I will try later on. But rsx11m said the code (I cited the link above) is from the latest nightly. rsx11m: "[...] version of TB 3.0a1pre and verify if the problem persists there. The code I cited was trunk code, thus it should unless something else has changed that overrides it."

Ok Nikolay, I installed TB3.01a1pre and can confirm that the problem persists.

Sorry for getting here a bit late, I wasn't aware that a bug was already filed and didn't get to it until now. BTW: The actual "\n\n" sequence was found by bkennelly in that forum thread, just to get the credits right. I too see this on Thunderbird 3.0a1pre (Windows/2008040903) using a plain-text signature, and also with SeaMonkey (to be expected as this is Core code). With HTML composition, The two extra lines are formatted as "Body Text" compared to "Preformat" for the signature, if "start my reply above the quote" and "place my signature below my reply" are chosen. The two lines are inserted to separate the reply (and here also the signature) from the quote, thus it makes sense to have some blank space at this location. The issue at question is if it makes sense to have it hard-coded to two blank lines (or one line), or if this should be configurable by some preference. chrizoo, I hope I summarized this correctly, otherwise please comment.

First, the issue is not lines added after the signature, it is lines added before the quotation. The problem is that two blank lines are appropriate when there is no signature, because the first will be the entry line and will contain the reply text, but not when there is a signature. So, the request is to insert only a single line before the quotation, if there is a signature on top, otherwise two.

Thanks for summing this up rsx11m. Brian, yes, the (courteous) request would be to rethink the adding of blank lines. The point is that the user can very easily add as many blank lines as he/she wishes by adding them to his/her signature. But it is impossible for the user to get rid of them, if TB adds them. So by not adding those blank lines, nobody has a disadvantage, because those who want them can add them in the signature. But the way it is currently handled, those who don't want as many blank lines are disadvantaged because they have to manually delete them all the time and there doesn't seem to be a workaround. (For me personally, one blank line between signature and quotation would be okay, but even that one line can be created via the signature.) My suggestion to handle the blank lines would be: without signature: BL BL QUOTE with signature: BL BL SIG BL QUOTE so combined we have: BL BL (SIG BL) QUOTE Since the blank line between signature and the quotation is only necessary if there is a signature, so these two should be treated together as a block. This scheme would also assure that the number of blank lines in the beginning is always 2, regardless of there being a signature afterwards or not.

Ok, the problem is that QuotingOutputStreamListener() always adds two lines when reply_on_top on set, regardless of whether or not a signature is present and positioned above the quote. Thus, only one line should be added if the signature is above the quote, and two lines if it's below the quote or no signature is present. This probably can be safely hardwired. I'm confirming this, seen in branch and trunk on Windows and Linux.

The design must accomodate all the possibilities, not just top-posters. There are three elements: T: the text entry field S: the signature (optional) Q: the quoted text (optional) With NL as separator, and quotes around optional parts, there are three possibilities: (Q) T (NL S) T (NL Q) (NL S) T (NL S) (NL Q) The only invariant is that, if a signature is included, it is preceded by a NL. With the reply on top, the quotation is always preceded by a NL, so that is a useful construction. With T initialised to NL, the first and second patterns are currently being handled correctly, but the third is being written as T (NL S) (NL NL Q).

Taking. This patch extents the logic of how many newlines are added before the quote, according to Brian's analysis. Only the reply formatting for which the reply is above the quote has been changed, no empty lines are added if the reply comes below the quote (as before). Following cases that have to be distinguished: 1) no signature is present, two newlines are added before the quote (one for the reply, the other to separate the quote from the text); 2) the signature is present and below the quote, again two newlines; 3) the signature is present and above the quote, only one newline is added now as the reply text comes before the signature, and only one newline is needed to separate the signature from the quote (the no-newline case looks odd, especially for plain-text replies, thus I think we should have one here). Note that I'm not checking if a signature actually exists, i.e., a non-empty signature file referred to, this would probably overdo it. Tested for both HTML and plain-text replies in all signature modes.

Comment on attachment 321493 [details] [diff] [review] Proposed fix Personally I don't think this is the way to go; I'd suggest removing any trailing newlines from the signature before it is pasted in to the message.

(In reply to comment #13) > (From update of attachment 321493 [details] [diff] [review]) > Personally I don't think this is the way to go; I'd suggest removing any > trailing newlines from the signature before it is pasted in to the message. > Neil, as you can read above, the problem was not trailing newlines in the signature, but extra spacing lines before the quote, when it is preceded by the signature. (A signature with no trailing newlines would still be separated from the quote by two blank lines.) Besides that, if the user includes trailing newlines in his signature, Thunderbird should respect his decision and retain them.

Correct, this patch is not about touching the signature in any way, it is just intended to remove an inconsistency in the placement of newlines. before the quoted text based on where the signature is placed. Please reconsider sr based on this.

I've canceled this patch for now after some discussion with Neil by e-mail. While addressing a valid issue, it was not clear from comment #13 in which context this was meant. The suggestion was that, rather than adding one or two newlines before the quote, to just remove a (single) trailing '\n' from the signature itself to achieve the same effect. Thus, a signature "Line1\nLine2" would result in Line1\n Line2\n \n as desired. However, this only works with plain-text signatures in plain-text composition mode. In HTML composition, this would be expanded to <pre class="moz-signature" cols="72">Line1<br>Line2</pre> <br> <br> where the </pre> implies a line break with or without a trailing <br> of Line2. All other combinations, including image signatures, result in two blank lines between the signature and the quote. Nevertheless, plain-text signatures should be normalized with regard to a '\n' at the end, to avoid an ambiguity here and to have it consistent. In agreement with comment #14, no other '\n' should be touched as those were placed on purpose by the user.

This revised patch addresses the issues raised by Neil during the review of the first patch. In General, it retains the initial idea of making the number of newlines before the quote depend on the signature placement, with a couple of additions: (1) The logic on how many newlines to place is still located in the constructor of QuotingOutputStreamListener, however also considering the presence of a signature provided in the new "htmlSigText" preference of the identity (missed that one in the first patch). (2) Plain-text signatures are now verified for the existence of a trailing CR, LF, or CRLF. If it is missing, one is added in order to have a uniform appearance (comment #16). (3) To fix another nit, a newline is added *before* the signature if we are in plain-text mode and the signature is placed between the reply and the quote, otherwise a newline was missing to indicate the start of the reply text. A couple of further comments for (1): (a) I ran into bug 435587 while testing, causing a crash for the htmlSigText pref handling. This part is not included in the patch here (as of now, no overlap in the code), thus before you can test the pref, you have to apply the patch in attachment 322383 [details] [diff] [review]. (b) Neil suggested to distinguish between HTML and plain-text editor modes when adding newlines before the quote. One problem is that m_composeHTML does not seem to be available in the QuotingOutputStreamListener class, thus I'd have to do this in ProcessSignature(). I'm not convinced that this is a good way to go. Having no newline before the quote after a plain-text signature in HTML composition (it would still be added in plain-text composition) looks fine and is separated by a newline due to the trailing </pre> if the message goes out as plain text then. However, with HTML signatures (no implied line break for either HTML or plain-text sending) the signature sits directly on top of the "author wrote:" string. Thus, we would have to check whether the signature comes with its own formatting before adding a newline - lots of cases to be distinguished. I don't think that this is solvable easily, given the variety of HTML formatting options. Similarly, image signatures would sit directly on top of the "author wrote:", same problem here. I'm putting this up for review again, please try to be as specific as possible if you want further changes.

Comment on attachment 322396 [details] [diff] [review] Proposed fix (v2) The problem here is that when the signature is at the bottom you're now forcing an extra blank line right at the very end.

Hmm, I'm not aware that I've changed anything with the handling of the signature if it is placed at the bottom (either for reply after the quote or when replying before the quote but the signature is placed at the end). Everything added is within a "reply_on_top == 1 && !sig_bottom" context. Are you referring to the handling of the plain-text signature ending? This is still the same if somebody has the signature end in a CR/LF.

Comment on attachment 322396 [details] [diff] [review] Proposed fix (v2) Hmm... it looks as if the htmlSigText already suffers from the same issue anyway.

Comment on attachment 322396 [details] [diff] [review] Proposed fix (v2) You'll need a reviewer from http://www.mozilla.org/owners.html#mail-and-news-backend for that

Comment on attachment 322396 [details] [diff] [review] Proposed fix (v2) Mixed up the modules, reassigning...

Thanks Mark, check-in for trunk please.

Checking in mailnews/compose/src/nsMsgCompose.cpp; /cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v <-- nsMsgCompose.cpp new revision: 1.566; previous revision: 1.565 done