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)
MailNews Core
Composition
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: hidenosuke, Assigned: sugar.waffle)
Details
Attachments
(2 files, 5 obsolete files)
3.34 KB,
patch
|
Details | Diff | Splinter Review | |
2.99 KB,
patch
|
neil
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
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)
reproduce with SeaMonkey/2006110408-trunk/WinXP.
this bug same as bug 63639?
Mac version Thunderbird reproduces.
OS: Linux → All
Hardware: PC → All
Reporter | ||
Comment 5•18 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
Attachment #244725 -
Attachment is obsolete: true
Updated•18 years ago
|
Assignee: nobody → sugar.waffle
Flags: blocking-thunderbird2?
Updated•18 years ago
|
Status: NEW → ASSIGNED
Attachment #246580 -
Flags: review?(mscott)
Comment 7•18 years ago
|
||
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
Updated•18 years ago
|
Attachment #246580 -
Flags: superreview?(mscott)
Updated•18 years ago
|
Attachment #246580 -
Flags: review?(mscott) → review?(neil)
Comment 8•18 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+
Attachment #246580 -
Attachment is obsolete: true
Attachment #246580 -
Flags: superreview?(mscott)
Attachment #256293 -
Flags: superreview?(mscott)
Attachment #256293 -
Flags: review+
Comment 10•18 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.
Comment 11•18 years ago
|
||
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)
Assignee | ||
Comment 12•18 years ago
|
||
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•18 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-
Attachment #257217 -
Attachment is obsolete: true
Attachment #257217 -
Flags: superreview?(mscott)
Assignee | ||
Comment 14•18 years ago
|
||
Attachment #257343 -
Flags: review?(neil)
Comment 15•18 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•18 years ago
|
||
Moving off bugs that didn't make the deadline for Thunderbird 2.
Flags: blocking-thunderbird2? → blocking-thunderbird2-
Assignee | ||
Comment 17•18 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•18 years ago
|
||
Hiro-san, were you going to put together a new patch based on your discussion with Neil?
Assignee | ||
Comment 19•18 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.
Comment 20•17 years ago
|
||
Attachment #257343 -
Attachment is obsolete: true
Comment 21•17 years ago
|
||
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•17 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+
Comment 23•17 years ago
|
||
Fix checked in with bienvenu's comments addressed.
Updated•17 years ago
|
Product: Core → MailNews Core
Comment 24•16 years ago
|
||
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•16 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]
Comment 27•16 years ago
|
||
testcase is https://litmus.mozilla.org/show_test.cgi?id=7646
closing FIXED per comment 25
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-litmus? → in-litmus-
Resolution: --- → FIXED
Updated•16 years ago
|
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.
Description
•