Closed Bug 737014 Opened 9 years ago Closed 9 years ago
Plaintext signatures are not received properly due to lack of BR nodes
A sample (redacted) showing a received message where the newlines have been stripped from my signature, and stay on line after the "--".
8.17 KB, image/png
100.63 KB, image/png
5.30 KB, patch
|Details | Diff | Splinter Review|
1.80 KB, patch
|Details | Diff | Splinter Review|
1.33 KB, patch
|Details | Diff | Splinter Review|
Using Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20120318 Thunderbird/14.0a1 ID:20120318030022 for a standard account on a Mirapoint IMA4rev1 server. I have noticed the following issue where a newline is not inserted between the "--" and the signature, or that and every line of the signature does not have their respective newlines. Signature format: Firstname A Lastname My Work Position My Department My Institution Email: firstname.lastname@example.org Office: +123 12345678 Cell: +123 12345678 My vCard format (snipped from the problematic message source): --------------020207090705040603090106 Content-Type: text/x-vcard; charset=utf-8; name="myworkemailhandle.vcf" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="myworkemailhandle.vcf" begin:vcard fn:Firstname A Lastname n:Lastname;Firstname org:My Institution;My Department adr:My Corporate Campus;;P.O. Box 12345;City;;12345;Country email;internet:email@example.com title:My Work Position tel;work:+123 12345678 tel;cell:+123 12345678 url:http://www.linkedin.com/in/myuserprofile version:2.1 end:vcard --------------020207090705040603090106-- Steps to recreate the problem: 1) Click Ctrl+N to compose a new message. 2) Enter message recipient. 3) Enter message subject. 4) Type message body content before the --, where the cursor is placed by default. After the --, Daily has automatically injected my text-only signature (which has defined per-account in Tools > Accounts Settings > Account Name > Signature text; additionally, a vCard is specified and configured to automatically attach appropriately). The signature at this point *appears correctly*. 5) Click Ctrl+Enter to send. Desired Result: Message has appropriately formatted signature, looking like so. -- Firstname A Lastname My Work Position My Department My Institution Email: firstname.lastname@example.org Office: +123 12345678 Cell: +123 12345678 Actual result: The newlines are stripped or never added. (See attached screenshot, signature.newline.1.png) -- Firstname A Lastname My Work Position My Department My Institution Email: email@example.com Office: +123 12345678 Cell: +123 12345678 Notes: I had not reported this for a few days because it is actually difficult to recreate. It is not happening consistently, and I am not particularly sure why.
Confirming with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120319 Thunderbird/14.0a1 with a minimal example (1-line signature "test", no vCard). Content-Type: text/plain; charset=ISO-8859-1; format=flowed. Using plain-text editor. Last bugs affecting signature spacing were bug 698925 (BigFiles) with a regression resolved in bug 735380.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Thunderbird Daily Does Not Add Newline Between -- And Signature → Thunderbird Daily Does Not Add Newline Between -- And Signature (plain text composition)
Assignee: nobody → mconley
Summary: Thunderbird Daily Does Not Add Newline Between -- And Signature (plain text composition) → Plaintext signatures are not received properly due to lack of BR nodes.
Sorry this took me so long - string conversion in C++ is not something I'm really used to. Here's my first run at a solution. I've sent a plaintext message to myself with a multi-line plaintext signature and it seems to work. I had to update a test to make it pass properly. I'm splitting the string based on the "\n" character...am I missing any corner cases?
Some code in nsMsgCompose::ProcessSignature() compares on all platform variants of line terminators, i.e., "\n", "\r", and "\r\n" (the first form applies to Linux but not necessary to Mac OSX and Windows, I don't know though if any automated conversion is applied by now, e.g., when reading the signature from a file). There was a discussion I had with Neil back in bug 428040 that signatures are allowed to either have or have not a trailing newline at the end, where both cases are considered equal to produce the same result (hence the handling of CRLF cases in the resulting fix there, which to some extent were removed in bug 698925 or bug 735380; you may want to verify that the logic envisioned there still applies after the various changes in the wake of the BigFiles implementation).
Comment on attachment 607309 [details] [diff] [review] Patch v1 so, I think either SplitString should be in nsMsgUtils.cpp/h where other code could use it (and probably take a PRUnichar delimiter), or we should just do it inline, like the patch I'm about to attach.
Attachment #607309 - Flags: review?(dbienvenu) → review-
Comment on attachment 607443 [details] [diff] [review] parse string inline and remove SplitString, of course :-) This is the approach I was trying to describe over IRC...
Thanks for the assist on this, David - I've removed SplitString in nsMsgCompose.h/.cpp as suggested.
Comment on attachment 607532 [details] [diff] [review] Patch v2 great, thx.
Attachment #607532 - Flags: review?(dbienvenu) → review+
Comment on attachment 607532 [details] [diff] [review] Patch v2 [Approval Request Comment] Regression caused by (bug #): bug 698925 User impact if declined: Plaintext signatures will not be received correctly. Testing completed (on c-c, etc.): comm-central. Risk to taking this patch (and alternatives if risky): None that I can think of - the patch is pretty straight-forward.
Attachment #607532 - Flags: approval-comm-aurora?
Attachment #607532 - Flags: approval-comm-aurora? → approval-comm-aurora+
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/342f2874aa70 Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/62d1ea4c543c
Seems fine now with: Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20120321 Thunderbird/14.0a1
Joe: Awesome, thanks! -Mike
Looks like bugzilla screwed up the flags with comment #13, restoring them except for tracking-thunderbird13+ which I can't set (but hopefully no longer relevant).
Works for me after applying update. Thanks guys.
(In reply to rsx11m from comment #15) > Looks like bugzilla screwed up the flags with comment #13, restoring them > except for tracking-thunderbird13+ which I can't set (but hopefully no > longer relevant). Ugh. I am an ass and mashed buttons quickly. I did not mean to reset flags specifically after you mentioned it. Apologies and mea culpa. Let me know if you want me to do anything.
Easy to "re-fix" but I'm wondering why this happened twice in a row... :-\
Duplicate of this bug: 737736
+ const kSeperator = "-- "; This probably wanted to be kSeparator?
(In reply to :aceman from comment #20) > + const kSeperator = "-- "; > > This probably wanted to be kSeparator? Good catch! Feel like throwing together a patch?
Surely. Can I attach it here?
Sure - and then I'll commit it as a followup.
Comment on attachment 608054 [details] [diff] [review] fix typo in kSeperator Great, thanks!
Attachment #608054 - Flags: review?(mconley) → review+
aceman: Thanks, follow-up checked in to comm-central as http://hg.mozilla.org/comm-central/rev/8a8a82bf4a24 -Mike
Whiteboard: [checkin patch 608054]
Comment on attachment 609355 [details] [diff] [review] Use ints instead of iterators for external API compatibility Pushed changeset 6208e3c32bad to comm-central.
You need to log in before you can comment on or make changes to this bug.