Closed Bug 359187 Opened 18 years ago Closed 16 years ago

The BCCs of default settings are duplicated whenever which is saved to draft

Categories

(MailNews Core :: Composition, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: hidenosuke, Assigned: sugar.waffle)

Details

Attachments

(2 files, 5 obsolete files)

When BCC addresses are in "Bcc these addresses:" in the Preferences, Bcc address are addred whenever you save the message as draft. If only one address in "Bcc these address:", there isn't this problem. Steps to reproduce: 1. Open Preferences > Copies & Folders 2. Add two address in the field 3. Click OK 4. Open Compose window 5. Write Subject or something 6. Save as draft 7. Open the message by "Edit draft" Actual result: Bcc address are added doublly. Bug 63639 may be related, but that is fixed.
It tried with debug build. 'bccStr' and 'bccList' are the following values. FindInReadable() is false because there is a blank. Value set with Preferences: test1@test,test2@test Result: debug:bccStr->test1@test, test2@test debug:bccList->test1@test,test2@test http://lxr.mozilla.org/mozilla1.8/source/mailnews/compose/src/nsMsgCompose.cpp#1520 1520 /* Setup bcc field */ 1521 PRBool doBcc; 1522 m_identity->GetDoBcc(&doBcc); 1523 if (doBcc) 1524 { 1525 nsXPIDLCString bccStr; 1526 bccStr.Assign(m_compFields->GetBcc()); 1527 1528 bccStr.BeginReading(start); 1529 bccStr.EndReading(end); 1530 1531 nsXPIDLCString bccList; 1532 m_identity->GetDoBccList(getter_Copies(bccList)); printf("debug:bccStr->%s\n",bccStr.get()); printf("debug:bccList->%s\n",(char *)bccList); 1533 if (FindInReadable(bccList, start, end) == PR_FALSE) { 1534 if (bccStr.Length() > 0) 1535 bccStr.Append(','); 1536 bccStr.Append(bccList); 1537 } Perhaps, the blank is set in the following places. http://lxr.mozilla.org/mozilla1.8/source/mailnews/compose/src/nsSmtpUrl.cpp#103 103 if (!nsCRT::strcasecmp (token, "bcc")) 104 { 105 if (!m_bccPart.IsEmpty()) 106 { 107 m_bccPart += ", "; 108 m_bccPart += value; 109 } 110 else 111 m_bccPart = value; 112 } The correspondence of the comparison by the form that removes the blank etc. is necessary. And as for this problem, "Reply-to:" reproduces. (Preferences > Account name > Reply-to Address)
Attached patch Proposal patch (obsolete) — Splinter Review
Blanks are deleted and compared from the address list.
reproduce with SeaMonkey/2006110408-trunk/WinXP. this bug same as bug 63639?
Mac version Thunderbird reproduces.
OS: Linux → All
Hardware: PC → All
I think this is a core problem.
Assignee: mscott → nobody
Component: Message Compose Window → MailNews: Composition
Product: Thunderbird → Core
QA Contact: message-compose → composition
Version: unspecified → Trunk
Attachment #244725 - Attachment is obsolete: true
Attached patch Proposal patch-v1 (obsolete) — Splinter Review
Update proposal patch.
Assignee: nobody → sugar.waffle
Flags: blocking-thunderbird2?
Status: NEW → ASSIGNED
Attachment #246580 - Flags: review?(mscott)
mscott: Hi, I checked the patch in bugzilla-jp. This patch looks good for me. Would you check the patch?
Summary: Add BCCs when save as draft → The BCCs of default settings are duplicated whenever which is saved to draft
Attachment #246580 - Flags: review?(mscott) → review?(neil)
Comment on attachment 246580 [details] [diff] [review] Proposal patch-v1 >+ aResultStr.Truncate(); I'm tempted to say that aSrcList should be an in/out parameter; you should at least assign the result with the aSrcList here rather than in each caller (and possibly rename the method MergeAddressList). This saves the caller having to fiddle around with whether either string is empty or not. r=me with these fixes. >+ if (aFindList.IsEmpty()) >+ return; This is an internal method, so you can comment it if you like but all your callers already check this for you so you don't need to repeat yourself. >+ nsCAutoString findList(aFindList), srcList(aSrcList); For an internal method it's fine to make the parameter types nsXPIDLCString& >+ findListArray.ParseString(findList.get(), ","); I don't know how safe this is - Scott, are only email addresses allowed here or are full addresses allowed? >+ findListArray[index]->CompressWhitespace(); I think you should use Trim rather than CompressWhitespace. >+ /* Setup reply-to field */ >+ nsXPIDLCString replyTo; >+ m_identity->GetReplyTo(getter_Copies(replyTo)); >+ if (replyTo && *(const char *)replyTo) You could use replyTo.IsEmpty() here! Scott, do we allow multiple reply-tos?
Attachment #246580 - Flags: review?(neil) → review+
Attachment #246580 - Attachment is obsolete: true
Attachment #246580 - Flags: superreview?(mscott)
Attached patch patch-v2 (obsolete) — Splinter Review
Attachment #256293 - Flags: superreview?(mscott)
Attachment #256293 - Flags: review+
Do we need to use our email address parser: nsIMsgHeaderParser instead of .ParseString? Can findList have values that include both the name & the address? i.e. "MacGregor, Scott" <mscott@mozilla.org>. If it can have that as a value, then parsing the string based on a comma won't work.
It's a good point! That is not in our way of thinking. should we use nsIMsgHeaderParser::parseHeadersWithArray instead of ParseString?
Attachment #256293 - Attachment is obsolete: true
Attachment #256293 - Flags: superreview?(mscott)
Attached patch Update patch(comment#10) (obsolete) — Splinter Review
Attachment #257217 - Flags: superreview?(mscott)
Attachment #257217 - Flags: review?(neil)
Comment on attachment 257217 [details] [diff] [review] Update patch(comment#10) I've looked at reply-to but similar comments apply to bcc of course. >+ nsXPIDLCString replyToStr; >+ replyToStr.Assign(m_compFields->GetReplyTo()); I think you should use a const char* at this point. > >+ char *resultStr = nsnull; >+ rv = RemoveDuplicateAddresses(replyToStr.get(), replyTo.get(), >+ PR_TRUE, &resultStr); >+ if (NS_SUCCEEDED(rv) && resultStr) > { Here I think you should check whether the resultStr is empty and if not then you can concatenate the two strings together and set the reply to. (Note that the replyToStr can be reused here to perform the concatenation. Note also that you still need to free the resultStr even if it's pointing to an empty string.)
Attachment #257217 - Flags: review?(neil) → review-
Attachment #257217 - Attachment is obsolete: true
Attachment #257217 - Flags: superreview?(mscott)
Attached patch update patch(comment#13) (obsolete) — Splinter Review
Attachment #257343 - Flags: review?(neil)
Comment on attachment 257343 [details] [diff] [review] update patch(comment#13) Hmm, I'm not sure the logic is optimal here. Let me try again. 1. If the identity has no reply-to(bcc) then there's nothing to do. 2. If the compose fields has no reply-to(bcc) then we can just set the reply-to(bcc) from the identity. 3. Otherwise we have to remove duplicate addresses. We remove those from the identity that are in the compose fields. 4. If there are none left then there's nothing to do, because the compose fields already have the right addresses. 5. Otherwise, concatenate the remaining addresses. Can you see if you can implement that?
Attachment #257343 - Flags: review?(neil)
Moving off bugs that didn't make the deadline for Thunderbird 2.
Flags: blocking-thunderbird2? → blocking-thunderbird2-
(In reply to comment #15)> > 1. If the identity has no reply-to(bcc) then there's nothing to do. Yes. > 2. If the compose fields has no reply-to(bcc) then > we can just set the reply-to(bcc) from the identity. Yes. > 3. Otherwise we have to remove duplicate addresses. > We remove those from the identity that are in the compose fields. Yes. The one that exists in identity is removed from compose fields. > 4. If there are none left then there's nothing to do, > because the compose fields already have the right addresses. Yes. > 5. Otherwise, concatenate the remaining addresses. Yes.
Hiro-san, were you going to put together a new patch based on your discussion with Neil?
(In reply to comment #18) > Hiro-san, were you going to put together a new patch based on your discussion > with Neil? > No. attachment (id=257343) is being written like comment#17.
Attachment #257343 - Attachment is obsolete: true
Marking r+ because I've decided I like this way after all. I didn't check my bitrot updates too carefully but it compiles with VC7.1
Attachment #328344 - Flags: superreview?(bienvenu)
Attachment #328344 - Flags: review+
Comment on attachment 328344 [details] [diff] [review] For ease of review looks good, except you don't really need a local var for replyToStr or bccStr, if I'm reading the code correctly, because they're each only used in one place.
Attachment #328344 - Flags: superreview?(bienvenu) → superreview+
Fix checked in with bienvenu's comments addressed.
Product: Core → MailNews Core
Can someone test this? I can't repro with version 3.0a1pre (2007100604), so I can't verify the effectiveness of the patch.
version 2.0.0.16 (20080708) CAN reproduce if test1@test,test2@test [comma] NOT if test1@test, test2@test [comma + space] version 3.0a2 (2008072418) WFM, [comma with or without space]
agree Fixed should there be a litmus test?
Flags: in-litmus?
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-litmus? → in-litmus-
Resolution: --- → FIXED
Flags: in-litmus-
Flags: in-litmus+
Flags: blocking-thunderbird2-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: