If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

MailNews Core
Composition
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: Hideo Oshima, Assigned: Hiro)

Tracking

Bug Flags:
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
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)
(Assignee)

Comment 2

11 years ago
Created attachment 244725 [details] [diff] [review]
Proposal patch

Blanks are deleted and compared from the address list.

Comment 3

11 years ago
reproduce with SeaMonkey/2006110408-trunk/WinXP.

this bug same as bug 63639?
(Assignee)

Comment 4

11 years ago
Mac version Thunderbird reproduces.
OS: Linux → All
Hardware: PC → All
(Reporter)

Comment 5

11 years ago
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
(Assignee)

Updated

11 years ago
Attachment #244725 - Attachment is obsolete: true
(Assignee)

Comment 6

11 years ago
Created attachment 246580 [details] [diff] [review]
Proposal patch-v1

Update proposal patch.
Assignee: nobody → sugar.waffle
Flags: blocking-thunderbird2?
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
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: superreview?(mscott)

Updated

11 years ago
Attachment #246580 - Flags: review?(mscott) → review?(neil)

Comment 8

11 years ago
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+
(Assignee)

Updated

11 years ago
Attachment #246580 - Attachment is obsolete: true
Attachment #246580 - Flags: superreview?(mscott)
(Assignee)

Comment 9

11 years ago
Created attachment 256293 [details] [diff] [review]
patch-v2
Attachment #256293 - Flags: superreview?(mscott)
Attachment #256293 - Flags: review+

Comment 10

11 years ago
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?
(Assignee)

Updated

11 years ago
Attachment #256293 - Attachment is obsolete: true
Attachment #256293 - Flags: superreview?(mscott)
(Assignee)

Comment 12

11 years ago
Created attachment 257217 [details] [diff] [review]
Update patch(comment#10)

RemoveDuplicateAddresses() was used. 
http://lxr.mozilla.org/mozilla/source/mailnews/compose/src/nsMsgCompose.cpp#151
Attachment #257217 - Flags: superreview?(mscott)
Attachment #257217 - Flags: review?(neil)

Comment 13

11 years ago
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-
(Assignee)

Updated

11 years ago
Attachment #257217 - Attachment is obsolete: true
Attachment #257217 - Flags: superreview?(mscott)
(Assignee)

Comment 14

11 years ago
Created attachment 257343 [details] [diff] [review]
update patch(comment#13)
Attachment #257343 - Flags: review?(neil)

Comment 15

11 years ago
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)

Comment 16

11 years ago
Moving off bugs that didn't make the deadline for Thunderbird 2. 
Flags: blocking-thunderbird2? → blocking-thunderbird2-
(Assignee)

Comment 17

11 years ago
(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.

Comment 18

11 years ago
Hiro-san, were you going to put together a new patch based on your discussion with Neil?
(Assignee)

Comment 19

11 years ago
(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. 
Created attachment 328343 [details] [diff] [review]
Updated for bitrot
Attachment #257343 - Attachment is obsolete: true
Created attachment 328344 [details] [diff] [review]
For ease of review

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 22

9 years ago
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.

Comment 25

9 years ago
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?
testcase is https://litmus.mozilla.org/show_test.cgi?id=7646

closing FIXED per comment 25
Status: ASSIGNED → RESOLVED
Last Resolved: 9 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.