Closed Bug 269728 Opened 15 years ago Closed 10 years ago

When sending email automatically add emails to CC list (instead of BCC)

Categories

(Thunderbird :: Account Manager, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.3a1

People

(Reporter: mozilla, Assigned: zcrendel)

Details

Attachments

(1 file, 15 obsolete files)

20.94 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041113 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041113 Firefox/1.0

Into account settings/copies & folders there is an option "automatically BB this
email addresses). You also need to add an option for CC.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.
Severity: normal → enhancement
Summary: When sending email automatically add emails to CC → When sending email automatically add emails to CC list (instead of BCC)
Version: unspecified → Trunk
Component: Preferences → Account Manager
This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/
This bug has been automatically resolved after a period of inactivity (see above
comment). If anyone thinks this is incorrect, they should feel free to reopen it.
Status: UNCONFIRMED → RESOLVED
Closed: 14 years ago
Resolution: --- → EXPIRED
Confirming the need for this option - the company I'm working for requests me to mail specific mails to the whole department via CC and it's a very annoying problem that Thunderbird can only BCC (which the company denies and requests CC)..
reopening
Status: RESOLVED → UNCONFIRMED
Resolution: EXPIRED → ---
Assignee: mscott → nobody
QA Contact: account-manager
This (missing) feature is the only thing blocking me from using Thunderbird.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Magnus, I'm sure you didn't miss that this bug is ENH.  In confirming this, are you think of this in terms of wanted-thunderbird3?
I wouldn't say it's a priority. I just confirmed it because it's a valid enh request.
probably easy too - just clone the bcc UI and code
Whiteboard: [good first bug]
Though for this kind of ting, default templates could be really nice.
I tried to set mail.default_cc in the hopes that would be used globaly, but that preference is ignored.
I have used oDesk.com to hire Michał Ściubidło to fix this bug.  Please assign this bug to me.  If Michał has a BMO account, I may ask to have this bug reassigned to him.
Attached patch proposed path for 3.x (obsolete) — Splinter Review
Adds option in account settings/copies & folders "automatically Cc this email addresses". In new mail automatically add Cc fields.
Adds option in account settings/copies & folders "automatically Cc this email addresses". In new mail automatically add Cc fields.
Michał Ściubidło: please follow the steps outlined under <https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch>.
Thanks Michal!  I am reviewing your patches.  Note:  the English word is “patch”, but you wrote “path” in the title of both patches.  “patch” and “path” are different words.

In attachment 387077 [details] [diff] [review]:

1. You misspelled “mercurial” (that is the correct spelling) in the patch filename: “mercirial_trunk.patch” (that is the incorrect spelling).

2. In the last hunk, there is a comment:

/* Setup bcc field */

This should say “cc” instead of “bcc”.
Attached patch proposed patch for 3.x (obsolete) — Splinter Review
Includes changes proposed by Brolin Empey in comment #15.
Attachment #387077 - Attachment is obsolete: true
Comment on attachment 387134 [details] [diff] [review]
proposed patch for 3.x

Adds option in account settings/copies & folders "automatically Cc this email
addresses". In new mail automatically add Cc fields.
Attachment #387134 - Attachment description: proposed path for 3.x → proposed patch for 3.x
Attachment #387134 - Flags: review?(account-manager)
Comment on attachment 387134 [details] [diff] [review]
proposed patch for 3.x

Setting a more probable reviewer.
Attachment #387134 - Flags: review?(account-manager) → review?(bienvenu)
Attachment #387134 - Flags: ui-review?(clarkbw)
Attachment #387134 - Flags: review?(bienvenu)
Attachment #387134 - Flags: review-
Comment on attachment 387134 [details] [diff] [review]
proposed patch for 3.x

thx for the patch.

we'll need a ui review for this...

some of the patch seems to use 4 space indent - can you make the code you added use 2 space consistently?

this looks wrong: it looks like you cloned GetDoBccList and removed the code in the middle, but that doesn't really make sense.


+nsMsgIdentity::GetDoCcList(nsACString& aValue)

+{
+    nsCString val;
+    nsresult rv = mPrefBranch->GetCharPref("doCcList", getter_Copies(val));
+    aValue = val;
+    if (NS_SUCCEEDED(rv))
+        return rv;
+
+    return SetDoCcList(aValue);
+}


I think:

+    if (NS_SUCCEEDED(rv))
+        return rv;
+
+    return SetDoCcList(aValue);

just wants to be 

return rv;

similarly for GetDoCc

it can just be

return mPrefBranch->GetBoolPref("doCc", aValue);
Assignee: nobody → michal.sciubidlo
Attached patch proposed patch for 3.x (obsolete) — Splinter Review
Includes changes proposed by David Bienvenu in Comment #19.
Attachment #387134 - Attachment is obsolete: true
Attachment #387215 - Flags: ui-review?(clarkbw)
Attachment #387215 - Flags: review?(bienvenu)
Attachment #387134 - Flags: ui-review?(clarkbw)
Attached patch proposed patch for 3.x (obsolete) — Splinter Review
Includes changes proposed by David Bienvenu in Comment #19 and is proper mercurial patch.
Attachment #387215 - Attachment is obsolete: true
Attachment #387220 - Flags: ui-review?(clarkbw)
Attachment #387220 - Flags: review?(bienvenu)
Attachment #387215 - Flags: ui-review?(clarkbw)
Attachment #387215 - Flags: review?(bienvenu)
Comment on attachment 387220 [details] [diff] [review]
proposed patch for 3.x

Can you put the CC field above the Bcc field; that's the natural order we commonly set them.

Also, could add some emptytext in the input for the format email addresses are supposed to be entered that would be really helpful. (both bcc and cc while you're there)

e.g. "Separate emails with commas"

ui-r+ with those
Attachment #387220 - Flags: ui-review?(clarkbw) → ui-review+
Attached patch proposed patch for 3.x (obsolete) — Splinter Review
Includes changes from Bryan W Clark, Comment #22.
Attachment #387220 - Attachment is obsolete: true
Attachment #387242 - Flags: ui-review?(clarkbw)
Attachment #387242 - Flags: review?(bienvenu)
Attachment #387220 - Flags: review?(bienvenu)
Comment on attachment 387242 [details] [diff] [review]
proposed patch for 3.x

great, thanks!
Attachment #387242 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 387242 [details] [diff] [review]
proposed patch for 3.x

thx for the new patch.

to avoid breaking SeaMonkey, you need to add the same strings to the equivalent suite file:

+++ b/mail/locales/en-US/chrome/messenger/am-copies.dtd

which is suite/locales/en-US/chrome/mailnews/pref


It would be good to tweak the suite equivalent of MsgComposeCommands.js as well, which is in suite/mailnews/compose.

Otherwise, this looks good to me, though I haven't tried running with it yet. Since this can't land until after b3, because of the string changes, there's no particular hurry...
Includes changes proposed by David Bienvenu in Comment #25.

Function GetDoCc and GetDoCcList have to return SetDoCC and GetDoCcList if there is no saved data. Otherwise no data is ever saved.
Attachment #387242 - Attachment is obsolete: true
Attachment #388155 - Flags: ui-review?(clarkbw)
Attachment #388155 - Flags: review?(bienvenu)
Attachment #387242 - Flags: review?(bienvenu)
Attachment #387078 - Attachment is obsolete: true
Attachment #388155 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 388155 [details] [diff] [review]
proposed patch for thunderbird 3.x and seamonkey

you can just carry this forward unless there are changes to the ui/strings.

Thanks for everything, this looks great!  Sorry I've been away for a little while.
Attachment #388155 - Flags: review?(bienvenu) → review-
Comment on attachment 388155 [details] [diff] [review]
proposed patch for thunderbird 3.x and seamonkey

ah, you mean we need to call setDoCC/setDoCCList in the case that getting the pref fails. A couple problems, then:

you're making GetDoCcList call setDoBccList - that doesn't seem right, and since you're calling what seems to be the wrong function, I'm not sure how it's helping you.

And in GetDoCC, you're essentially calling SetDoCc with an uninitialized value:

+  nsresult rv = mPrefBranch->GetBoolPref("doCc", aValue);
+  if (NS_SUCCEEDED(rv))
+    return rv;
+
+  return SetDoCc(*aValue);
+}

If GetBoolPref fails, there's no guarantee that aValue has been set, and the caller of GetDoCc doesn't set it, so that also seems wrong. So I suspect what's important is that you not return an error, not that you call SetDoCc/CcList. I think perhaps in the case of errors getting the pref, you set the out param to a reasonable default (false/empty string), and return NS_OK. 

The other thing I noticed is that the left side of the text box for the cc list doesn't line up with the text box for the bcc list. I think it would look a lot nicer if they lined up - what do you think, Bryan?
(In reply to comment #28)
> you're making GetDoCcList call setDoBccList - that doesn't seem right, and
> since you're calling what seems to be the wrong function, I'm not sure how it's
> helping you.

It just treats aValue as default if it isn't there. Same as in GetDoBccList and GetDoBcc.

> And in GetDoCC, you're essentially calling SetDoCc with an uninitialized value:

> 
> +  nsresult rv = mPrefBranch->GetBoolPref("doCc", aValue);
> +  if (NS_SUCCEEDED(rv))
> +    return rv;
> +
> +  return SetDoCc(*aValue);
> +}
> 
> If GetBoolPref fails, there's no guarantee that aValue has been set, and the
> caller of GetDoCc doesn't set it, so that also seems wrong. So I suspect what's
> important is that you not return an error, not that you call SetDoCc/CcList. I
> think perhaps in the case of errors getting the pref, you set the out param to
> a reasonable default (false/empty string), and return NS_OK. 

Bcc code works same way it should be changed too then?
(In reply to comment #29)
> 
> Bcc code works same way it should be changed too then?

No, the bcc code is different:

  nsresult rv = mPrefBranch->GetBoolPref("doBcc", aValue);
  if (NS_SUCCEEDED(rv))
    return rv;

if the GetBoolPref call fails, we don't use the uninitialized aValue because of the code that you removed when cloning this function:

  PRBool bccSelf = PR_FALSE;
  GetBccSelf(&bccSelf);

  PRBool bccOthers = PR_FALSE;
  GetBccOthers(&bccOthers);

  nsCString others;
  GetBccList(others);

  *aValue = bccSelf || (bccOthers && !others.IsEmpty());

  return SetDoBcc(*aValue);

Hope that makes sense.
(In reply to comment #30)
> (In reply to comment #29)
> Hope that makes sense.

Yes, it does :)

I will wait for Bryan comment on bcc/cc text box alignment before posting another patch.
Whiteboard: [good first bug] → [good first bug] [waits for coments from clarkbw]
Attached patch proposed patch (obsolete) — Splinter Review
Includes changes proposed in comment 28 (with UI change).

A assigned account-manager@thunderbird.bugs for UI review because i don't know if Bryan is available.

With Target Milestone can I assign with this bug?
Attachment #388155 - Attachment is obsolete: true
Attachment #396745 - Flags: ui-review?(account-manager)
Attachment #396745 - Flags: review?(bienvenu)
(In reply to comment #32)
> Created an attachment (id=396745) [details]
> proposed patch
> 
> Includes changes proposed in comment 28 (with UI change).
> 
> A assigned account-manager@thunderbird.bugs for UI review because i don't know
> if Bryan is available.

Only bryan does ui-reviews AFAIK.

> With Target Milestone can I assign with this bug?

It will be added when this lands - which could be b4 if you get good and fast reviews.
(In reply to comment #33)
> (In reply to comment #32)
> > With Target Milestone can I assign with this bug?
> 
> It will be added when this lands - which could be b4 if you get good and fast
> reviews.

At first, I thought you meant “before” instead of “beta 4”, but I decided you must must have meant “beta 4” because you did not use any other chat abbreviations for single words. :)  You used “AFAIK”, but that is an abbreviated phrase, not a single word.
Comment on attachment 396745 [details] [diff] [review]
proposed patch

You'll need sr as well, so might as well set that up now.
Attachment #396745 - Flags: ui-review?(clarkbw)
Attachment #396745 - Flags: ui-review?(account-manager)
Attachment #396745 - Flags: superreview?(neil)
Comment on attachment 396745 [details] [diff] [review]
proposed patch

(In reply to comment #28)
> The other thing I noticed is that the left side of the text box for the cc list
> doesn't line up with the text box for the bcc list. I think it would look a lot
> nicer if they lined up - what do you think, Bryan?

Oh, nice catch it does look a lot better when lined up in a grid like format.

I'm assuming that's the only change I'm supposed to be looking at so ui-r+
Attachment #396745 - Flags: ui-review?(clarkbw) → ui-review+
(In reply to comment #36)
> I'm assuming that's the only change I'm supposed to be looking at so ui-r+

Yes, it's the only one. Thanks for review.
Whiteboard: [good first bug] [waits for coments from clarkbw] → [need r bienvenu, sr neil]
Comment on attachment 396745 [details] [diff] [review]
proposed patch

>       <hbox align="center">
>-        <checkbox wsm_persist="true" id="identity.doBcc" label="&bccAddress.label;"
>+      <vbox>
Please use the <grid> and related tags for grids.

>+			<checkbox wsm_persist="true" id="identity.doCc" label="&ccAddress.label;"
Please use spaces, not tabs.

As requested in a previous review comment, you correctly put the Cc field above the Bcc field in the display. However I would appreciate it if you could move the Cc code above the Bcc code too! At the very least, I think this should be done in MsgComposeCommands.js so that the Cc entries appear first.

>diff -r 5037fd3fea18 mailnews/base/public/nsIMsgIdentity.idl
You need to change the uuid when you change an idl.

>+nsMsgIdentity::GetDoCc(PRBool *aValue)
>+nsMsgIdentity::SetDoCc(PRBool aValue)
>+nsMsgIdentity::GetDoCcList(nsACString& aValue)
>+nsMsgIdentity::SetDoCcList(const nsACString& aValue)
Don't bother with these, just use NS_IMPL_IDPREF_BOOL/STR.
(Compare BccOthers/BccList).

>diff -r 5037fd3fea18 suite/locales/en-US/chrome/mailnews/pref/am-copies.dtd
>--- a/suite/locales/en-US/chrome/mailnews/pref/am-copies.dtd	Fri Jul 10 19:37:03 2009 +0100
>+++ b/suite/locales/en-US/chrome/mailnews/pref/am-copies.dtd	Tue Aug 25 19:09:12 2009 +0200
>@@ -4,16 +4,21 @@
> <!ENTITY sendingPrefix.label "When sending messages, automatically: ">
> <!ENTITY fccMailFolder.label "Place a copy in:">
> <!ENTITY fccMailFolder.accesskey "P">
> <!ENTITY fccReplyFollowsParent.label "Place replies in the folder of the message being replied to">
> <!ENTITY fccReplyFollowsParent.accesskey "c">
> <!-- LOCALIZATION NOTE (bccAddress.label): do not translate "Bcc" in below line -->
> <!ENTITY bccAddress.label "Bcc these email addresses:">
> <!ENTITY bccAddress.accesskey "B">
>+<!ENTITY bccAddressList.emptytext "Separate emails with commas">
>+<!-- LOCALIZATION NOTE (ccAddress.label): do not translate "Cc" in below line -->
>+<!ENTITY ccAddress.label "Cc these email addresses:">
>+<!ENTITY ccAddress.accesskey "C">
>+<!ENTITY ccAddressList.emptytext "Separate emails with commas">
For the emptytexts I would prefer "Use commas to separate multiple addresses"
Attachment #396745 - Flags: superreview?(neil) → superreview-
Comment on attachment 396745 [details] [diff] [review]
proposed patch

marking obsolete based on Neil's comments. We're going to have our b4 string freeze really soon (and SM is already frozen) so if this is to go into TB 3, we'd need a new patch soon.
Attachment #396745 - Attachment is obsolete: true
Attachment #396745 - Flags: review?(bienvenu)
Attached patch proposed patch (obsolete) — Splinter Review
Changes proposed in comment #38
Attachment #397920 - Flags: superreview?(neil)
Attached patch proposed patch (obsolete) — Splinter Review
Changes proposed in comment #38 (ignore previous patch :)
Attachment #397920 - Attachment is obsolete: true
Attachment #397923 - Flags: superreview?(neil)
Attachment #397920 - Flags: superreview?(neil)
Attached patch proposed patch (obsolete) — Splinter Review
Moved cc code above bcc in suite.
Attachment #397923 - Attachment is obsolete: true
Attachment #397932 - Flags: superreview?(neil)
Attachment #397923 - Flags: superreview?(neil)
Comment on attachment 397932 [details] [diff] [review]
proposed patch

Something strange happens with this patch. Let's say I have a message sent to user@example.com copied to me, and all my outgoing messages get copied to user@example.com, then when I "Reply" to the message, it correctly gets copied to user@example.com with the patch, but "Reply All" does not copy it, although it used to before the patch...

>diff -r 5037fd3fea18 mail/components/compose/content/MsgComposeCommands.js
You're using a very old working directory. Some files have moved since. You should use hg pull && hg update -r default to merge your changes.

>+<!ENTITY ccAddressList.emptytext "Use commas to separate multiple addresses">
Sorry, but it turns out that this is too long to fit in the window. Please change this to "Separate address with commas". Please change suite's file too!

>+      <grid align="center">
This is not quite right. You need to put align="center" on each <row> instead.
Comment on attachment 397932 [details] [diff] [review]
proposed patch

Looks like QuotingOutputStreamListener::OnStopRequest assumes there's no existing CC list and just clobbers it in the Reply All case :-(
Attachment #397932 - Flags: superreview?(neil) → superreview-
(In reply to comment #43)
> >+<!ENTITY ccAddressList.emptytext "Use commas to separate multiple addresses">
> Sorry, but it turns out that this is too long to fit in the window. Please
> change this to "Separate address with commas". Please change suite's file too!

Did you mean "Separate addresses with commas"?  If there is only 1 address, there is no need for separators. :)
(In reply to comment #45)
> (In reply to comment #43)
> > >+<!ENTITY ccAddressList.emptytext "Use commas to separate multiple addresses">
> > Sorry, but it turns out that this is too long to fit in the window. Please
> > change this to "Separate address with commas". Please change suite's file too!
> Did you mean "Separate addresses with commas"?  If there is only 1 address,
> there is no need for separators. :)
Yeah, silly typo, sorry!
Attached patch proposed patch (obsolete) — Splinter Review
>Something strange happens with this patch. Let's say I have a message sent to
>user@example.com copied to me.

So it's send from otheruser@example.com and your address is in cc?

>and all my outgoing messages get copied to
>user@example.com, then when I "Reply" to the message, it correctly gets copied
>to user@example.com with the patch, but "Reply All" does not copy it, although
>it used to before the patch...

user@example.com is setted up as cc address? Can you give me more details about your setup and this mail?
Attachment #397932 - Attachment is obsolete: true
Attachment #401625 - Flags: superreview?(neil)
(In reply to comment #47)
>>Something strange happens with this patch. Let's say I have a message sent to
>>user@example.com copied to me.
> So it's send from otheruser@example.com and your address is in cc?
It doesn't matter who it's from.

>>and all my outgoing messages get copied to
>>user@example.com, then when I "Reply" to the message, it correctly gets copied
>>to user@example.com with the patch, but "Reply All" does not copy it, although
>>it used to before the patch...
>user@example.com is setted up as cc address? Can you give me more details about
>your setup and this mail?
user@example.com is a mailing list. I want all my outgoing mail to be copied to the mailing list by default, which I can do with this patch by adding user@example.com to the automatic CC list. But if I use "Reply All", then it does not get automatically added to the CC list, even though user@example.com was one of the recipients, and so without the patch "Reply All" used to add it.
What is the status of this bug?  I guess we are waiting for Michal Sciubidlo’s reply?
(In reply to comment #48)
> >>Something strange happens with this patch. Let's say I have a message ....

Brolin hired me for complete fixing for this bug. I retrieved last trunk sources, built thunderbird, but can't reproduce such behaviour - all works as expected.

What I did to test:
1) I've got 3 mail boxes (and accounts is thunderbird), let's say: 
   mail_1@example.com, mail_2@example.com, mail_3@example.com
where:
   mail_1@example.com account with automated CC list: 'mail_3@example.com'
   mail_3@example.com account with automated CC list: 'mail_1@example.com'

2) I sent mail from mail_3 to mail_1 (it copied to mail_2)
Tried Reply and reply all - all worked fine. After that

3) I sent mail from mail_1 to mail_2 (it copied to mail_3)
Tried Reply and reply all - all worked fine.

I'm confused - what's need to be fixed? I've just attached a very little modified patch which match last thunderbird trunk. 

Dear nail, help me to reproduce bug step-by-step if it still exists or say what all work fine for you.
Flags: wanted1.8.1.x?
Attached patch update patch for last trunk (obsolete) — Splinter Review
Seems all works fine with last patch, please review.
Attachment #416630 - Flags: superreview?
Grogory: please request review (and superreview) from a specific person.
https://developer.mozilla.org/en/Getting_your_patch_in_the_tree#Getting_the_patch_reviewed
Assignee: michal.sciubidlo → zcrendel
Status: NEW → ASSIGNED
Attachment #401625 - Attachment is obsolete: true
Attachment #401625 - Flags: superreview?(neil)
Only security fixes for 1.8.1..
Flags: wanted1.8.1.x?
Attachment #416630 - Flags: superreview? → superreview?(neil)
Many apologies, but I managed to confuse myself so thoroughly in comment #43 that I had you all confused too.

The problem with the patch is in fact much simpler.

When you use Reply, you see the automatic CC as you expect. But when you use Reply All, then you see the same CC as without the patch. In fact comment #44 correctly noticed that the automatic CC seems to get overwritten.
Hello!

Anybody, please help me to confirm the status of last patch. Cause no answer from Neil for about a month
(In reply to comment #55)
> Hello!
> 
> Anybody, please help me to confirm the status of last patch. Cause no answer
> from Neil for about a month

You also need to ask for review.
He did, in case you didn't notice.

Anyway, the problem is that the auto-CC is ignored in a Reply All.

I unfortunately confused the issue by providing incorrect steps to reproduce.
> I unfortunately confused the issue by providing incorrect steps to reproduce.

Please, take a time to provide correct steps to reproduce. Cause I can't catch the problem and therefore cannot fix it. Thank you.
Sorry for not getting back to this bug sooner.

Steps to reproduce:
1. mail_1@example.com sends email to mail_2@example.com CC mail_3@example.com
2. mail_2@example.com does reply all
3. Resulting email is to mail_1@example.com CC mail_3@example.com
4. Now add mail_4@example.com to the auto-CC list.
5. Try to reply all to one of the two above emails.
6. Note that mail_4@example.com does not get added to the auto-CC list.

Additional test (nice, but not essential):
7. Set the auto-CC list to all four above addresses.
8. Reply all to one of the two above emails.
9. Check that each address appears exactly once in the auto-CC list.
Thank you. Now I've got it.
I've reconstructed patch to meet the actual trunk version. That patch contains a fix of the last bug. I've tested it with success. Asking for a superreview to Neil.

Best regards,
Grigory
Attachment #416630 - Attachment is obsolete: true
Attachment #432327 - Flags: superreview?(neil)
Attachment #416630 - Flags: superreview?(neil)
Comment on attachment 432327 [details] [diff] [review]
patch contains 'automatically add emails to CC list' feature for current trunk

I've reconstructed patch to meet the actual trunk version. That patch contains a fix of the last bug. I've tested it with success. Asking for a superreview to Neil.

Best regards,
Grigory
Attachment #432327 - Attachment is patch: true
Attachment #432327 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 432327 [details] [diff] [review]
patch contains 'automatically add emails to CC list' feature for current trunk

>+            nsAutoString outCCListString;
>+            compFields->GetCc(outCCListString);
>+            if (!replyCompValue.IsEmpty() && !outCCListString.IsEmpty())
>+            {
>+              replyCompValue.AppendLiteral(", ");
>+              replyCompValue.Append(outCCListString);
>+            }
>             compFields->SetCc(replyCompValue);
If you look at where replyCompValue is actually generated, you'll see that it's incorrect to have the Append inside the if block. sr=me with this fixed.
In fact the replyCompValue is generated for the SetAllReply part of bug 248681, although it transpires in bug 525070 that the backend code doesn't quite work so the good news is that it's not so bad if this makes things worse ;-)

> <!ENTITY fccReplyFollowsParent.label "Place replies in the folder of the message being replied to">
> <!ENTITY fccReplyFollowsParent.accesskey "c">
>+<!-- LOCALIZATION NOTE (ccAddress.label): do not translate "Cc" in below line -->
>+<!ENTITY ccAddress.label "Cc these email addresses:">
>+<!ENTITY ccAddress.accesskey "C">
You'll need to change the fccReplyFollowsParent accesskey (I somehow think you don't want to change the ccAddress access key!). sr=me with this fixed.
Attachment #432327 - Flags: superreview?(neil) → superreview+
(In reply to comment #63)
> (From update of attachment 432327 [details] [diff] [review])
> >+            nsAutoString outCCListString;
> >+            compFields->GetCc(outCCListString);
> >+            if (!replyCompValue.IsEmpty() && !outCCListString.IsEmpty())
> >+            {
> >+              replyCompValue.AppendLiteral(", ");
> >+              replyCompValue.Append(outCCListString);
> >+            }
> >             compFields->SetCc(replyCompValue);
> If you look at where replyCompValue is actually generated, you'll see that it's
> incorrect to have the Append inside the if block. sr=me with this fixed.
> In fact the replyCompValue is generated for the SetAllReply part of bug 248681,
> although it transpires in bug 525070 that the backend code doesn't quite work
> so the good news is that it's not so bad if this makes things worse ;-)
I'm not sure about the text above. I've reviewed that bugs, but didn't understand what you wanna say. I've placed Append inside 'if' block only in reason, what the line:
    compFields->SetCc(replyCompValue);
vanishing already prepared CC list. It does exactly in that place, so what's why Append is there. Did you say about what this code in if block close connected to the bugs above and that block maybe changed? 

Anyway, what's your recommendation? I have to get that 'Append' one level up from if block?

> > <!ENTITY fccReplyFollowsParent.label "Place replies in the folder of the message being replied to">
> > <!ENTITY fccReplyFollowsParent.accesskey "c">
> >+<!-- LOCALIZATION NOTE (ccAddress.label): do not translate "Cc" in below line -->
> >+<!ENTITY ccAddress.label "Cc these email addresses:">
> >+<!ENTITY ccAddress.accesskey "C">
> You'll need to change the fccReplyFollowsParent accesskey (I somehow think you
> don't want to change the ccAddress access key!). sr=me with this fixed.
I will change this to 'a', cause I've seen what it's a free access key on that page. Is it ok?

Thank you for a quick reply!
Regards
(In reply to comment #64)
>(In reply to comment #63)
>>(From update of attachment 432327 [details] [diff] [review])
>>>+            nsAutoString outCCListString;
>>>+            compFields->GetCc(outCCListString);
>>>+            if (!replyCompValue.IsEmpty() && !outCCListString.IsEmpty())
>>>+            {
>>>+              replyCompValue.AppendLiteral(", ");
>>>+              replyCompValue.Append(outCCListString);
>>>+            }
>>>             compFields->SetCc(replyCompValue);
>>If you look at where replyCompValue is actually generated, you'll see that it's
>>incorrect to have the Append inside the if block. sr=me with this fixed.
>>In fact the replyCompValue is generated for the SetAllReply part of bug 248681,
>>although it transpires in bug 525070 that the backend code doesn't quite work
>>so the good news is that it's not so bad if this makes things worse ;-)
>I'm not sure about the text above. I've reviewed that bugs, but didn't
>understand what you wanna say. I've placed Append inside 'if' block only in
>reason, what the line:
>     compFields->SetCc(replyCompValue);
>vanishing already prepared CC list. It does exactly in that place, so what's
>why Append is there. Did you say about what this code in if block close
>connected to the bugs above and that block maybe changed?
The problem is that replyCompValue.Append(outCCListString); won't get executed if replyCompValue is empty, so that the outCCListString data is lost.

>>> <!ENTITY fccReplyFollowsParent.label "Place replies in the folder of the message being replied to">
>>> <!ENTITY fccReplyFollowsParent.accesskey "c">
>>>+<!-- LOCALIZATION NOTE (ccAddress.label): do not translate "Cc" in below line -->
>>>+<!ENTITY ccAddress.label "Cc these email addresses:">
>>>+<!ENTITY ccAddress.accesskey "C">
>>You'll need to change the fccReplyFollowsParent accesskey (I somehow think you
>>don't want to change the ccAddress access key!). sr=me with this fixed.
>I will change this to 'a', cause I've seen what it's a free access key on that
>page. Is it ok?
Oops, this turned out to be harder than it looks. The keys I see as already being in use are a, b, d, e, f, h, m, n, o, p, r, s, t, v, w (note that a f h and r are used by SeaMonkey's buttons). This just leaves g (bad because it has a descender), i and l (bad because they are so thin). Out of the three I'd say that l is the least worst.
(In reply to comment #65)
> The problem is that replyCompValue.Append(outCCListString); won't get executed
> if replyCompValue is empty, so that the outCCListString data is lost.

Oops, you're right! Thanks for attention!

> Oops, this turned out to be harder than it looks. The keys I see as already
> being in use are a, b, d, e, f, h, m, n, o, p, r, s, t, v, w (note that a f h
> and r are used by SeaMonkey's buttons). This just leaves g (bad because it has
> a descender), i and l (bad because they are so thin). Out of the three I'd say
> that l is the least worst.

This makes sense. 

Patch is on the way.
Regards
last issues fixed
Attachment #432327 - Attachment is obsolete: true
Attachment #432607 - Flags: superreview?(neil)
Attachment #432607 - Flags: superreview?(neil) → superreview+
Comment on attachment 432607 [details] [diff] [review]
patch contains 'automatically add emails to CC list' feature for current trunk ver2

please approve the patch
Attachment #432607 - Flags: superreview+ → superreview?(bugzilla)
Attachment #432607 - Flags: review?(neil)
Attachment #432607 - Flags: review?(neil) → review+
Attachment #432607 - Flags: superreview?(bugzilla) → superreview?(bienvenu)
Whiteboard: [need r bienvenu, sr neil] → [need sr bienvenu, r neil]
Target Milestone: --- → Thunderbird 3.2a1
Comment on attachment 432607 [details] [diff] [review]
patch contains 'automatically add emails to CC list' feature for current trunk ver2

thx for the patch, sr=bienvenu, with nit:
+            if (!replyCompValue.IsEmpty() && !outCCListString.IsEmpty())
+            {
+              replyCompValue.AppendLiteral(", ");
+            }

no need for braces there.

This work predates our unit test requirement so I'm not going to require a mozmill test for this, but once we start fleshing out the compose window mozmill tests, we should keep this feature in mind when adding tests.
Attachment #432607 - Flags: superreview?(bienvenu) → superreview+
braces removed, please approve
Attachment #432607 - Attachment is obsolete: true
Attachment #435683 - Flags: superreview?(bienvenu)
Attachment #435683 - Flags: review?(neil)
Comment on attachment 435683 [details] [diff] [review]
patch contains 'automatically add emails to CC list' feature for current trunk ver3 [Checkin: comment 72]

sorry, you didn't need to re-request review - I should have mentioned that. Carrying forward reviews.
Attachment #435683 - Flags: superreview?(bienvenu)
Attachment #435683 - Flags: superreview+
Attachment #435683 - Flags: review?(neil)
Attachment #435683 - Flags: review+
Whiteboard: [need sr bienvenu, r neil] → checkin-needed
Comment on attachment 435683 [details] [diff] [review]
patch contains 'automatically add emails to CC list' feature for current trunk ver3 [Checkin: comment 72]

http://hg.mozilla.org/comm-central/rev/5e3d80a05323
Attachment #435683 - Attachment description: patch contains 'automatically add emails to CC list' feature for current trunk ver3 → patch contains 'automatically add emails to CC list' feature for current trunk ver3 [Checkin: comment 72]
Pro Tip: checkin-needed usually goes to Keywords, not Whiteboard. You're lucky I used Bugzilla Quicksearch. ;-) Thanks for the patch!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: checkin-needed
(In reply to comment #73)
> Pro Tip: checkin-needed usually goes to Keywords, not Whiteboard. You're lucky
> I used Bugzilla Quicksearch. ;-) Thanks for the patch!

Thanks! :-)
You need to log in before you can comment on or make changes to this bug.