Closed Bug 737014 Opened 9 years ago Closed 9 years ago

Plaintext signatures are not received properly due to lack of BR nodes.

Categories

(Thunderbird :: Message Compose Window, defect)

13 Branch
defect
Not set
normal

Tracking

(thunderbird13 fixed, thunderbird14 verified)

VERIFIED FIXED
Thunderbird 14.0
Tracking Status
thunderbird13 --- fixed
thunderbird14 --- verified

People

(Reporter: ohghoo8n.mzl.al, Assigned: mconley)

References

Details

(Keywords: regression)

Attachments

(5 files, 3 obsolete files)

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: me@company.tld
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:me@company.tld
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: me@company.tld
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: me@company.tld 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.
Severity: normal → minor
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
Keywords: regression
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
Blocks: BigFiles
OS: Windows 7 → All
Severity: minor → normal
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.
Attached patch Patch v1 (obsolete) — Splinter Review
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?
Attachment #607252 - Attachment is obsolete: true
Attachment #607309 - Flags: review?(dbienvenu)
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-
Attached patch parse string inline (obsolete) — Splinter 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...
Attached patch Patch v2Splinter Review
Thanks for the assist on this, David - I've removed SplitString in nsMsgCompose.h/.cpp as suggested.
Attachment #607309 - Attachment is obsolete: true
Attachment #607443 - Attachment is obsolete: true
Attachment #607532 - Flags: review?(dbienvenu)
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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
Seems fine now with:
Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20120321 Thunderbird/14.0a1
Severity: normal → minor
Status: RESOLVED → VERIFIED
OS: All → Windows 7
Target Milestone: Thunderbird 14.0 → ---
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).
Severity: minor → normal
OS: Windows 7 → All
Hardware: x86 → All
Target Milestone: --- → Thunderbird 14.0
Version: 14 → 13
Works for me after applying update. Thanks guys.
Severity: normal → minor
OS: All → Windows 7
Hardware: All → x86
Version: 13 → 14
(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... :-\
Severity: minor → normal
OS: Windows 7 → All
Hardware: x86 → All
Version: 14 → 13
+  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.
Attachment #608054 - Flags: review?(mconley)
Comment on attachment 608054 [details] [diff] [review]
fix typo in kSeperator

Great, thanks!
Attachment #608054 - Flags: review?(mconley) → review+
Keywords: checkin-needed
Whiteboard: [checkin patch 608054]
aceman:

Thanks, follow-up checked in to comm-central as http://hg.mozilla.org/comm-central/rev/8a8a82bf4a24

-Mike
Keywords: checkin-needed
Whiteboard: [checkin patch 608054]
Attachment #609355 - Flags: review?(dbienvenu) → review+
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.