Closed Bug 178328 Opened 22 years ago Closed 21 years ago

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

Categories

(SeaMonkey :: MailNews: Account Configuration, defect)

Product:
SeaMonkey
Component:
MailNews: Account Configuration
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.4alpha

People

(Reporter: jglick, Assigned: shliang)

Details

(Whiteboard: [ish][ue])

Attachments

(2 files, 4 obsolete files)

Add the correct attachment this time (ignore previous attach)
27.15 KB, image/gif
Details
patch
11.85 KB, patch
sspitzer
: review+
Details | Diff | Splinter Review 
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.
Attached image example (obsolete) — 

    
    

    
  
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

    
  
Keywords: nsbeta1
Whiteboard: [ish]

    
  
Whiteboard: [ish] → [ish][ue]

    
    

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

    
  
Target Milestone: mozilla1.3beta → mozilla1.4alpha

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

    
  

        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-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #117889 -
        Attachment is obsolete: true

    
  

        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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #118159 -
        Attachment is obsolete: true

    
  

        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-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  

        Attachment #118252 -
        Attachment is obsolete: true

    
  

        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 on attachment 118262 [details] [diff] [review]
patch

r=cavin.

    
    

    
  
resolving
Status: ASSIGNED → RESOLVED
Closed: 21 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.

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: