Copies & Folders: Combine bcc settings & disable text field appropriately

VERIFIED FIXED in mozilla1.4alpha

Status

SeaMonkey
MailNews: Account Configuration
VERIFIED FIXED
15 years ago
13 years ago

People

(Reporter: jglick, Assigned: shliang)

Tracking

Trunk
mozilla1.4alpha
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ish][ue])

Attachments

(2 attachments, 4 obsolete attachments)

27.15 KB, image/gif
Details
11.85 KB, patch
(not reading, please use seth@sspitzer.org instead)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
Account Settings: Copies & Folder Pane.

1. Combine the two bcc settings to save vertical space.
2. The bcc text field should be disabled if its checkbox is not checked.
(Reporter)

Comment 1

15 years ago
Created attachment 105106 [details]
example
(Reporter)

Comment 2

15 years ago
Comment on attachment 105106 [details]
example

><HTML><BODY><P><IMG src="http://bugzilla.mozilla.org/attachment.cgi?id=105106&amp;action=view" alt="The image “http://bugzilla.mozilla.org/attachment.cgi?id=105106&amp;action=view” cannot be displayed, because it contains errors."/></P></BODY></HTML>
Attachment #105106 - Attachment is obsolete: true
(Reporter)

Comment 3

15 years ago
Created attachment 105110 [details]
Add the correct attachment this time (ignore previous attach)
(Reporter)

Updated

15 years ago
Keywords: nsbeta1
Whiteboard: [ish]
(Reporter)

Updated

15 years ago
Whiteboard: [ish] → [ish][ue]

Comment 4

15 years ago
reassigning to shliang
Assignee: racham → shliang
Keywords: nsbeta1 → nsbeta1+
Target Milestone: --- → mozilla1.3beta

Updated

15 years ago
Target Milestone: mozilla1.3beta → mozilla1.4alpha
(Assignee)

Comment 5

15 years ago
Created attachment 117889 [details] [diff] [review]
patch
(Assignee)

Updated

15 years ago
Attachment #117889 - Flags: superreview?(sspitzer)
this is just a fix for the UI.

but what about these issues?

1)
what if a user had both bcc self [from the first checkbox] and bcc others set
[with the second checkbox]

or just bcc self [from the first checkbox].

when they move to this build, the UI will have "lost data".

2) 

what about the back end?  if I used to bcc myself, after your fix, I would still
be doing it, but I would have no way in the UI to disable it.
Status: NEW → ASSIGNED
Comment on attachment 117889 [details] [diff] [review]
patch

the migration and backend parts of this patch still need to be done.
Attachment #117889 - Flags: superreview?(sspitzer) → superreview-
(Assignee)

Comment 8

15 years ago
Created attachment 118159 [details] [diff] [review]
patch
Attachment #117889 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #118159 - Flags: review?(sspitzer)
Comment on attachment 118159 [details] [diff] [review]
patch

sorry I was so sparse on details.

notice your fix doesn't play nice with cancel.

here's what I suggest:

add a comment to mailnews.js:

// keep these defaults for backwards compatability and migration
// but .doBcc and .doBccList are the right ones from now on.
305 pref("mail.identity.default.bcc_self",false);
306 pref("mail.identity.default.bcc_others",false);
307 pref("mail.identity.default.bcc_list","");

are you are trying to map this into a bool pref and a string pref

I suggest this:

depreciate these attributes of nsIMsgIdentity:

// don't call bccSelf, bccOthers, and bccList directly
// only used for migration, and backward compatability
// instead use doBcc and doBccList
attribute boolean bccSelf;
attribute boolean bccOthers;
attribute string bccList;
attribute boolean doBcc;
attribute string doBccList;

Then, fix all the callers (mozilla and commercial tree, C++ and JS)
who previously called .bccSelf .bccOthers and .bcc list to use these new ones.
(except the callers in migration code.)

The setters are easy:

NS_IMETHODIMP
nsMsgIdentity::SetDoBcc(PRBool aValue)
{
  return SetBoolAttribute("doBcc", aValue);
}

NS_IMETHODIMP
nsMsgIdentity::SetDoBccList(const char *aValue)
{
  return SetCharAttribute("doBccList", aValue);
}

the getters are fun:

NS_IMETHODIMP
nsMsgIdentity::GetDoBcc(PRBool *aValue)
{
  nsresult rv = GetBoolAttribute("doBcc", aValue);
  // will fail the first time, since doBcc is not defaulted in mailnews.js
  if (NS_SUCCEEDED(rv))
    return rv;

  // pseudo code:
  *aValue = (GetBccOthers() && GetOtherList() is not empty) || GetBccSelf;
  // set it, so the next call to GetDoBcc() "works"
  rv = SetDoBcc(*aValue);
  return rv;
}

NS_IMETHODIMP
nsMsgIdentity::GetDoBccList(const char **aValue)
{
  nsresult rv = GetCharAttribute("doBccList", aValue);
  // will fail the first time, since doBccList is not defaulted in mailnews.js
  if (NS_SUCCEEDED(rv))
    return rv;

  nsCAutoString result;

  // pseudo code:
  self = GetBccSelf();
  others = GetBccOthers();
  othersList = GetBccOthersList();
  if (self)
    result = (get emal address);

  if (others && otherlist is not empty) {
    if (self)
      result += ","
    result += otherList;
  }

  // set it, so the next call to GetDoBcc() "works"
  rv = SetDoBccList(result);
  return rv;
}

In the past it was possible to bcc multiple addresses at a time:
  yourself and others

So I also think we want the text to be:

"Bcc these email addresses:"

note, we already support a comma separated list, so this should work in the
back end.

For the pref UI, use doBcc and doBccList.

See http://lxr.mozilla.org/mozilla/search?string=BccOthers
http://lxr.mozilla.org/mozilla/search?string=BccSelf
Attachment #118159 - Flags: review?(sspitzer) → review-
"Bcc these email addresses (comma separated)"

probably best to ping robinf and/or jglick for suggestions on wording.
(Assignee)

Comment 11

15 years ago
Created attachment 118252 [details] [diff] [review]
patch
Attachment #118159 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #118252 - Flags: review?(sspitzer)
Comment on attachment 118252 [details] [diff] [review]
patch

1)

please ad a comment to nsIMsgIdentity.idl

to indicate that these are still needed for migration and backward
compatability
(with existing profiles), and with ispdata .rdf files (lxr for nswebmail.rdf
and aol.rdf)
(so we can't remove them), but you shouldn't using them, instead using doBcc
and doBccList

attrobite boolean bccSelf;
attribute boolean bccOthers;
attribute string bccList;

2)

don't make this change, as it will break 4.x migration

-  MIGRATE_SIMPLE_BOOL_PREF(PREF_4X_NEWS_CC_SELF,identity,SetBccSelf)
-  MIGRATE_SIMPLE_BOOL_PREF(PREF_4X_NEWS_USE_DEFAULT_CC,identity,SetBccOthers)
-  MIGRATE_SIMPLE_STR_PREF(PREF_4X_NEWS_DEFAULT_CC,identity,SetBccList)
+  MIGRATE_SIMPLE_BOOL_PREF(PREF_4X_NEWS_USE_DEFAULT_CC,identity,SetDoBcc)
+  MIGRATE_SIMPLE_STR_PREF(PREF_4X_NEWS_DEFAULT_CC,identity,SetDoBccList)

for migration to work, we need to migrate these the "old way".

this code migrates 4.x to "old way", and then the new getters will migrate "old
way" to "new way".


3)


+NS_IMETHODIMP
+nsMsgIdentity::GetDoBcc(PRBool *aValue)
+{
+  nsresult rv = getPrefService();
+  if (NS_FAILED(rv)) 
+    return rv;
+  
+  char *prefName = getPrefName(m_identityKey, "doBcc");
+  rv = m_prefBranch->GetBoolPref(prefName, aValue);
+  PR_Free(prefName);
+  
+  if (NS_SUCCEEDED(rv)) {
+    rv = GetBoolAttribute("doBcc", aValue);
+    return rv;
+  }

why can't you just do

+    nsresult rv = GetBoolAttribute("doBcc", aValue);
+    if (NS_SUCCEEDED(rv))
       return rv;

that calls nsMsgIdentity::getBoolPref(), which is similar to the code you
wrote.

4a)

+  GetBccSelf(&bccSelf);

instead,

+  rv = GetBccSelf(&bccSelf);
+  NS_ENSURE_SUCCESS(rv,rv);

4b)

+  GetBccOthers(&bccOthers);

instead,

+  rv = GetBccOthers(&bccOthers);
+  NS_ENSURE_SUCCESS(rv,rv);


4c)

+  GetBccList(getter_Copies(others));

instead,

+  rv = GetBccList(getter_Copies(others));
+  NS_ENSURE_SUCCESS(rv,rv);

a,b,c will not (at least, should not) fail, as we specify default values in
mailnews.js


5)

+  *aValue = (bccOthers && (PL_strcmp((const char*) others, "") != 0)) ||
bccSelf;

instead:

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

6)

+  rv = SetDoBcc(*aValue);
+  return rv;

instead,

return SetDoBcc(*aValue);


7)


+NS_IMETHODIMP
+nsMsgIdentity::GetDoBccList(char **aValue)
+{
+  nsresult rv = getPrefService();
+  if (NS_FAILED(rv)) 
+    return rv;
+
+  char *prefName = getPrefName(m_identityKey, "doBccList");
+  rv = m_prefBranch->GetCharPref(prefName, aValue);
+  PR_Free(prefName);
+
+  if (NS_SUCCEEDED(rv)) {
+    rv = GetCharAttribute("doBccList", aValue);
+    return rv;
+  }

see comment #3

8)

+  GetBccSelf(&bccSelf);

see comment 4abc.

9)

+    result.Append(email);

I think 

result += email;

is preferred, according to the mozilla string guide.

10)


+  GetBccOthers(&bccOthers);

see comment 4abc

11)

+  GetBccList(getter_Copies(others));

see comment 4abc

12)

+  if (bccOthers && (PL_strcmp((const char*) others, "") != 0)) {

instead

+  if (bccOthers && (!others.IsEmpty)) {

13)

+    result.Append(others);  

see comment 9.

14)

+  *aValue = ToNewCString(result);
+  
+  rv = SetDoBccList(ToNewCString(result));
+  return rv;

ToNewCString allocates, so you just leaked result.

instead:

+  *aValue = ToNewCString(result);
+  return SetDoBccList(*aValue);

15)

doBcc and doBccList really makes the code in MsgComposeCommands.js and
nsMsgCompose.cpp cleaner.  nice!
Attachment #118252 - Flags: review?(sspitzer) → review-
(Assignee)

Comment 13

15 years ago
Created attachment 118262 [details] [diff] [review]
patch
Attachment #118252 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #118262 - Flags: review?(sspitzer)
Comment on attachment 118262 [details] [diff] [review]
patch

two comments, make these changes locally before checking in:

1)

// ...Use doBcc
// doBcc and doBccList instead.

(doBcc repeated)

2)

can you just remove this commented out code?

+      //m_identity->GetBccSelf(&bccSelf);

r/sr=sspitzer
Attachment #118262 - Flags: review?(sspitzer) → review+
note to shuehan / QA, here's the areas to make sure to test:

1)  4.x migration (of the news and mail bcc values)
2)  existing profiles (for bcc self, bcc others values)
3)  sending

for completeness, it also affects

4)  isp datasources (see
http://lxr.mozilla.org/mozilla/source/mailnews/base/ispdata/example.rdf#32)

but don't worry about that, as I think we support it, but no one (I know) is
using it.
other areas this affects:

5)  compose window (we should be adding the right bcc fields, see change to
nsMsgCompose.cpp)
6)  switching identities in the compose window (should update the bcc fields,
see change to MsgComposeCommands.js)

Comment 17

15 years ago
Comment on attachment 118262 [details] [diff] [review]
patch

r=cavin.
(Assignee)

Comment 18

15 years ago
resolving
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
shuehan the bug I was talking about last night is bug #199361 (might be a dup)

but it was existing before you change, so it isn't a regression.

Comment 20

15 years ago
Trunk build 2003-03-27: WinXP, Mac 10.1.5, Linux RH 8
Verified Fixed. 

I found a bcc bug but it was occuring in 7.02 so it's not a regression (bug# 199582)
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.