Closed
Bug 584962
Opened 14 years ago
Closed 13 years ago
automatic CC to self stripped on Reply All
Categories
(MailNews Core :: Composition, defect)
MailNews Core
Composition
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 10.0
People
(Reporter: walter.mozilla, Assigned: walter.mozilla)
Details
Attachments
(1 file, 1 obsolete file)
2.13 KB,
patch
|
Bienvenu
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/533.4 (KHTML, like Gecko) Chrome/5.0.375.125 Safari/533.4 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; nl; rv:1.9.2.4) Gecko/20100608 Thunderbird/3.1 I have automatic CC set to my own email address. When I hit Reply, my CC address is added like expected. When I hit Reply All, my CC address isn't. I'm not 100% sure, but I expect the problem to be in the code below. First my address is added to the CC list and then Reply All triggers the needToRemoveDup code below. That code ensures that my own e-mail address is stripped, even though I want it to be in there. http://hg.mozilla.org/releases/comm-1.9.2/file/c1186371ddee/mailnews/compose/src/nsMsgCompose.cpp 2431 NS_IMETHODIMP QuotingOutputStreamListener::OnStopRequest(nsIRequest *request, nsISupports *ctxt, nsresult status) ... 2521 if (type == nsIMsgCompType::ReplyAll) ... 2550 needToRemoveDup = PR_TRUE; ... ... 2722 if (mIdentity) 2723 { 2724 nsCString email; 2725 mIdentity->GetEmail(email); 2726 addressToBeRemoved.AppendLiteral(", "); 2727 addressToBeRemoved.Append(email); 2728 rv = parser->RemoveDuplicateAddresses(nsDependentCString(_compFields->GetTo()), 2729 email, resultStr); 2730 if (NS_SUCCEEDED(rv)) 2731 { 2732 if (type == nsIMsgCompType::ReplyAll && !mailFollowupTo.IsEmpty()) 2733 _compFields->SetTo(resultStr.get()); 2734 } 2735 } 2736 rv = parser->RemoveDuplicateAddresses(nsDependentCString(_compFields->GetCc()), 2737 addressToBeRemoved, resultStr); Reproducible: Always Steps to Reproduce: 1. Set your own email address in Account Settings -> .. -> CC to these addresses. 2. Find a mail sent to you. 3a. Click Reply: see your address in Cc and the original From in the To field. 3b. Click Reply All: (diverges) Actual Results: 3b. Click Reply All: see only the original From in the To field. Expected Results: 3b. Click Reply All: see your address in Cc and the original From in the To field. There is a different bug in which the Cc isn't added for regular replies either if the original mail contains a Reply-To. If you do not get your e-mail address Cc in step (3a) you might see that bug. Proposed fix: if the code is indeed broken in the place that I think it is, one could add a check against the auto-CC option there. An alternative fix is to add the auto-CC email address first after the above code has run. Regards, Walter Doekes OSSO B.V.
Assignee | ||
Comment 1•14 years ago
|
||
The different bug that I mention is actually the same: 2619 else if (! replyTo.IsEmpty()) 2620 { 2621 // default behaviour for messages without Mail-Reply-To 2622 compFields->SetTo(replyTo); 2623 needToRemoveDup = PR_TRUE; 2624 } needToRemoveDup is set to true if the Reply-To field is non-empty causing my CC-to-self to get stripped in the regular Reply case as well.
Component: Message Compose Window → Composition
Product: Thunderbird → MailNews Core
Version: unspecified → 1.9.2 Branch
Updated•14 years ago
|
QA Contact: message-compose → composition
Comment 2•14 years ago
|
||
Patch are welcome :-)
Comment 4•13 years ago
|
||
I confirm this bug is still present in v.7.0 b3
Assignee | ||
Comment 5•13 years ago
|
||
Ok. This bug is still present in hg tip. This patch should fix it. I tried to obey coding style of the rest of the file. I don't know if any review flags should be set. Regards, Walter Doekes OSSO B.V.
Updated•13 years ago
|
Assignee: nobody → walter+mozilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 6•13 years ago
|
||
Comment on attachment 563329 [details] [diff] [review] check whether myEmail is in automaticCCList and if so, don't remove it from CC >I tried to obey coding style of the rest of the file. I don't know if any review >flags should be set. Yes if you want the pacth to go into the tree.
Attachment #563329 -
Flags: review?(dbienvenu)
Updated•13 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Version: 1.9.2 Branch → Trunk
Comment 7•13 years ago
|
||
Comment on attachment 563329 [details] [diff] [review] check whether myEmail is in automaticCCList and if so, don't remove it from CC thx for the patch - it's a little out of date against the trunk. PRBool needs to be bool, PR_TRUE should be true, PR_FALSE false you don't need braces here: + if (ccListEmailAddresses.Find(myEmail) != kNotFound) + { + removeMyEmailInCc = PR_FALSE; + } this line looks like it's over 80 chars, so should be split up: + // Only remove own address from CC if it's not in automatic-CC-to-self (see bug 584962) + rv = parser->ExtractHeaderAddressMailboxes(ccList, + ccListEmailAddresses); + NS_ENSURE_SUCCESS(rv, rv); This would totally abort if the cc list is not parseable, and I'm not sure we currently do that. So better to warn, but try to keep going. Finally, why do you need the code inside the if (automaticCc) { }? Can't you just set removeMyMailInCc to !automaticCc? Or simply change if (removeMyEmailInCc) to if (!automaticCc) ?
Attachment #563329 -
Flags: review?(dbienvenu) → review-
Assignee | ||
Comment 8•13 years ago
|
||
> PRBool needs to be bool, PR_TRUE should be true, PR_FALSE false done > you don't need braces here: done > this line looks like it's over 80 chars, so should be split up: done > This would totally abort if the cc list is not parseable I just copied that from nsMsgCompose::CreateMessage. I have not bothered to check what it does. But it looks like Reply'ing to a message would abort too, if the parsing were to fail. So I'm guessing the parser is not that picky. > Finally, why do you need the code inside the if (automaticCc) There are three cases here: - user has no automaticCc - user has automaticCc without self - user has automaticCc with self removeMyEmailInCc was always true before this patch, so it should still be. We only change it to false when there is automaticCc *and* self is in it. I could reduce the use of two variables to a single one, but that would not make the code any clearer, IMO. That would become: if (doNotRemoveMyEmailInCc) { ... if (ccListEmailAddresses.Find(myEmail) == kNotFound) doNotRemoveMyEmailInCc = false; } if (!doNotRemoveMyEmailInCC) { addressToBeRemove.Append... } You see how ridiculous that looks.
Assignee | ||
Comment 9•13 years ago
|
||
> > This would totally abort if the cc list is not parseable > I just copied that from nsMsgCompose::CreateMessage. I have not bothered > to check what it does. But it looks like Reply'ing to a message would > abort too, if the parsing were to fail. So I'm guessing the parser is not > that picky. Oh wait. I'm wrong. I'll fix go that.
Comment 10•13 years ago
|
||
(In reply to Walter Doekes from comment #8) > > There are three cases here: > - user has no automaticCc > - user has automaticCc without self > - user has automaticCc with self > Ah, thx, I see, a comment to that effect would be helpful...
Assignee | ||
Comment 11•13 years ago
|
||
(Didn't test, got compile error in nsLDAPOperation.cpp.. but I don't think I made any typo's.)
Attachment #563329 -
Attachment is obsolete: true
Attachment #567522 -
Flags: review?(dbienvenu)
Comment 12•13 years ago
|
||
Comment on attachment 567522 [details] [diff] [review] changes after feedback from david bienvenu Thx, Walter, that looks good.
Attachment #567522 -
Flags: review?(dbienvenu) → review+
Comment 13•13 years ago
|
||
(In reply to David :Bienvenu from comment #12) > Comment on attachment 567522 [details] [diff] [review] [diff] [details] [review] > changes after feedback from david bienvenu > > Thx, Walter, that looks good. Adding checking-needed.
Keywords: checkin-needed
Comment 14•13 years ago
|
||
Checked in: http://hg.mozilla.org/comm-central/rev/b4ae149b9eb5
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 10.0
Assignee | ||
Comment 15•11 years ago
|
||
Did someone break this in Thunderbird 19? http://forums.mozillazine.org/viewtopic.php?f=29&t=2648633 """The "Cc" behavior was changed as part of bug 250187, which indeed became effective with the 19.0 cycle.""" https://bugzilla.mozilla.org/show_bug.cgi?id=250187 I'm not even sure I like that feature. Cheers, Walter
Comment 16•11 years ago
|
||
Yes, bug 917231.
You need to log in
before you can comment on or make changes to this bug.
Description
•