Last Comment Bug 530142 - Account Settings copies "Reply-To Address" between accounts when selected
: Account Settings copies "Reply-To Address" between accounts when selected
Status: RESOLVED FIXED
[gs][gssolved]
:
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 13.0
Assigned To: :aceman
:
:
Mentors:
http://gsfn.us/t/r3sh
: 541730 558495 (view as bug list)
Depends on:
Blocks: qa-tb3.0rc1 810763
  Show dependency treegraph
 
Reported: 2009-11-20 11:09 PST by Nathan Tuggy (:tuggyne)
Modified: 2013-03-26 12:53 PDT (History)
10 users (show)
acelists: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (1.96 KB, patch)
2012-02-07 14:35 PST, :aceman
iann_bugzilla: review-
Details | Diff | Splinter Review
patch v2 (2.04 KB, patch)
2012-02-12 06:15 PST, :aceman
iann_bugzilla: review+
Details | Diff | Splinter Review
patch v3 (1.99 KB, patch)
2012-02-18 07:20 PST, :aceman
iann_bugzilla: review+
standard8: review+
Details | Diff | Splinter Review

Description Nathan Tuggy (:tuggyne) 2009-11-20 11:09:50 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5 (.NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.30729; .NET CLR 3.5.21022; .NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.5) Gecko/20091117 Thunderbird/3.0

The Account Settings dialog copies the Reply-To address setting from one account to another when the accounts are selected one at a time in the account pane.

Before:
Account        Reply-To
a@example.com  x@example.com
b@example.com  

After selecting a@example.com and b@example.com, in that order, in the Account Settings dialog:
Account        Reply-To
a@example.com  x@example.com
b@example.com  x@example.com

Reproducible: Always

Steps to Reproduce:
0. (requires multiple accounts set up)
1. Open the Account Settings dialog box (Tools -> Account Settings on Windows; not sure about other platforms)
2. Ensure one of your accounts is not set to use a reply-to address, and another is
3. Click OK to close Account Settings, then reopen the dialog
4. Click an account that has a reply-to set (from step 2)
5. Click another account that had a reply-to not set (also from step 2)
Actual Results:  
The second account had the reply-to address copied over from the first. When OK is clicked, the copied reply-to settings are persisted.

Expected Results:  
The second account should retain its reply-to settings. Or, at least, the visual glitch should not be persisted when OK is clicked.

Does not apparently copy blank Reply-To address settings over set Reply-To addresses, which is nice. (Otherwise, there'd be no real way of setting Reply-Tos at all.)
Comment 1 Nathan Tuggy (:tuggyne) 2009-11-20 11:13:11 PST
Loosely speaking, this bug was discovered testing RC1 build 2, although I was not intentionally running any specific tests at the time -- just ordinary usage. Setting blocking accordingly.

I'm fairly sure this is a regression, as I don't remember this behavior even from 3.0 beta 4, and certainly not from 2.0.0.x. Setting regression keyword accordingly.
Comment 2 Nathan Tuggy (:tuggyne) 2009-11-20 11:32:36 PST
Hmm, this is very strange: I can no longer reproduce after restarting Thunderbird. Perhaps the bug only surfaces if a new account has been created that session? (If so, it's obviously completely unimportant.)
Comment 3 Nathan Tuggy (:tuggyne) 2009-11-20 11:45:55 PST
I can reproduce the bug again now, after restarting the entire computer. Not sure what else has changed since the last time I tried and failed to reproduce.

So it's evidently a somewhat slippery bug.
Comment 4 Ludovic Hirlimann [:Usul] 2009-11-23 04:02:22 PST
Nathan, could you try running in -safe-mode for a while and see if the bug comes back after a few days ? (Ho and we got build3 for rc1).

Did you notice anything in Tools -> Error console when you got the issue ?
Comment 5 [:Aureliano Buendía] 2009-11-23 04:23:49 PST
I can confirm this issue with TB 3.0 rc1 build 3.

I can provide a STR (steps to reproduce) a little bit different from Nathan.

My situation:
I have 3 account: 1 (called [A]) that is "isolate" has a specific node in
folders pane, and others 2 (called [B],[C]) that store data in local folders.
None of them use "reply to" property in Account Settings (the textbox is
empty).

STR:
1. goto Account Settings and delete reply to field for accounts [A],[B], [C];
2. close TB;
3. start TB goto  Account Settings and in account [C] set reply to =
x@example.com as suggest Nathan.
4. close Account Settings;
5. reopening Account Settings: account [A] (but ramdomly also\or account [B])
has reply to field set to x@example.com.

It seems that reply to is override from one account to some hothers as Nathan
observe in comment #0.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.5) Gecko/20091121
Lightning/1.0pre Thunderbird/3.0 ID:20091121183158

Nathan could provide a regression-range?
Comment 6 [:Aureliano Buendía] 2009-11-23 04:27:25 PST
...I'm sorry I forgot: it happens also in safe-mode (overwrite reply to property in all my account except the first)
Comment 7 [:Aureliano Buendía] 2009-11-23 04:38:35 PST
not "overwrite" I'm wrong but "copies": it copies reply to field to others account (one or more) that have reply to empty
Comment 8 Nathan Tuggy (:tuggyne) 2009-11-23 08:58:27 PST
(In reply to comment #4)
> Did you notice anything in Tools -> Error console when you got the issue ?
Just checked for that; one error comes up when I open Account Settings (before selecting any accounts at all):
Error: childrenNode.childNodes[i]._account is undefined
Source File: chrome://messenger/content/AccountManager.js
Line: 171

Will check later in Safe Mode to see if this error reoccurs there; I suspect it may be unrelated.

BTW, the same problem occurs in two different profiles.

(In reply to comment #5)
> Nathan could provide a regression-range?
Working on it. May be a few days, though, before I've narrowed it down sufficiently -- though if my hunch is correct, and it's a regression from beta 4, it shouldn't be too bad.
Comment 9 Nathan Tuggy (:tuggyne) 2009-12-05 22:47:54 PST
All right, so my hunch was completely wrong. If this is a regression, it's not a regression from beta 4, or beta 1, or the pre-alphas from 2007, or even Thunderbird 2 -- all exhibit this bug (tested in a new clean profile for the purpose). Clearing keywords, version accordingly. Also, according to Aureliano in comment #5, this is reproducible on XP, so I believe it's not platform-related; setting platform to All/All.

Next steps, anyone? I've never looked at the code in question, so I'm not sure how much help I'll be to track it down, now that we don't have an obvious regression range.
Comment 10 [:Aureliano Buendía] 2010-02-02 02:20:08 PST
*** Bug 541730 has been marked as a duplicate of this bug. ***
Comment 11 Nathan Tuggy (:tuggyne) 2010-04-09 18:25:05 PDT
*** Bug 558495 has been marked as a duplicate of this bug. ***
Comment 12 WADA 2010-04-09 22:59:31 PDT
Confirmed with 3.2a1pre.
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100408 Shredder/3.2a1pre

(1) account1(id1),account2(id2),account3(id3),account4(id4) are defined.
    Config Editor : reset mail.identity.idX.reply_to (X=1 to 4)
(2) Open Account setting.
    (2-1) click account1(id1), reply-to=1@1.1.1
    (2-2) click account3(id3) => reply-to=1@1.1.1 is displayed
                    => change to reply-to=3@3.3.3
    (2-3) OK at account setting.
          mail.identity.id1.reply_to=1@1.1.1
          mail.identity.id2.reply_to is reset status(or doesn't exist)
          mail.identity.id3.reply_to=3@3.3.2
          mail.identity.id4.reply_to is reset status(or doesn't exist)
(3) Open Account setting again.
    (3-1) click account1(id1), reply-to=1@1.1.1 is displayed
    (3-2) click account2(id2), reply-to=1@1.1.1 (previous one is not cleared)
    (3-3) click account3(id3), reply-to=3@3.3.3 is displayed
    (3-4) click account4(id4), reply-to=3@3.3.3 (previous one is not cleared)
    (3-5) OK at account setting.
          mail.identity.id1.reply_to=1@1.1.1
          mail.identity.id2.reply_to=1@1.1.1
          mail.identity.id3.reply_to=3@3.3.2
          mail.identity.id4.reply_to=3@3.3.2

Problem looks:
Upon account switch, reply-to Address: field of UI is not cleared, if idX.reply_to doesn't exist.
Comment 13 :aceman 2012-02-07 08:18:06 PST
This is a general problem in the account manager. The fields are not properly cleared when switching panels, the code must initialize/clear each field "manually". I't looks like some architectural problem. I have played with this in bug 208628 comment 9.
I'll try to look into this. The problem is the Reply-To field does not have an "activating" checkbox like the BCC field has. I can't use it to decide whether to clear the field.
Comment 14 :aceman 2012-02-07 14:35:55 PST
Created attachment 595187 [details] [diff] [review]
patch

This is an attempt to fix the architectural problem.
I found out that setFormElementValue and getFormElementValue in AccountManager.js check for (type == "text") element. I don't know if such an element exists. What the XUL tag for that element would be. So I changed the check to "textbox", which does exist (and is the XUL tag for all those text fields).
Then I store formElement.value when saving a page and restore
both formElement.setAttribute("value",xxx) and formElement.value = xxx;

This seems to work fine for me and also fixes the same problem in BCC fields of bug 208628 comment 9.

This code is used for managing each textbox field in the account manager so please review thoroughly, it could produce regressions.
Comment 15 Ian Neal 2012-02-11 15:49:05 PST
Comment on attachment 595187 [details] [diff] [review]
patch

r- on testing
STR
1. Have one IMAP and news account.
2. Enter a@example.com in reply-to fields for both accounts.
3. Close application
4. Open application
5. Delete reply-to field from news account and click OK
6. Open account settings

Expected Result
1. Reply-to field empty

Actual Result
1. Reply-to field is still a@example.com
Comment 16 :aceman 2012-02-12 06:15:55 PST
Created attachment 596470 [details] [diff] [review]
patch v2

Thanks, try this.
Also check if other text fields work ok.
Comment 17 Ian Neal 2012-02-14 09:41:03 PST
Comment on attachment 596470 [details] [diff] [review]
patch v2


>+    if (type == "textbox") {
>+      var val = formElement.value;
>+      return val;
>     }
>     if ("value" in formElement) {
>       return formElement.value;
>     }
These could be combined into a single if statement (does "textbox" even need to be special-cased anymore?)

>+  else if (type == "textbox") {
>+    if (value == null || value == undefined) {
>+      formElement.setAttribute("value",null);
>+      formElement.value = null;
>+    } else {
>       formElement.setAttribute("value",value);
>+      formElement.value = value;
>+    }
This is unnecessarily complicated, just set variable to null when it is undefined. Do we really need to use both setAttribute and .value?

r=me though I would like to test the new patch.
Comment 18 :aceman 2012-02-14 12:28:08 PST
(In reply to Ian Neal from comment #17)
> Comment on attachment 596470 [details] [diff] [review]
> patch v2
> 
> 
> >+    if (type == "textbox") {
> >+      var val = formElement.value;
> >+      return val;
> >     }
> >     if ("value" in formElement) {
> >       return formElement.value;
> >     }
> These could be combined into a single if statement (does "textbox" even need
> to be special-cased anymore?)
I would leave it special cased as it also has a specialcased counterpart in the setFormElementValue function. Just for understandability.

> >+  else if (type == "textbox") {
> >+    if (value == null || value == undefined) {
> >+      formElement.setAttribute("value",null);
> >+      formElement.value = null;
> >+    } else {
> >       formElement.setAttribute("value",value);
> >+      formElement.value = value;
> >+    }
> This is unnecessarily complicated, just set variable to null when it is
> undefined.

Which variable?

> Do we really need to use both setAttribute and .value?
Removing that would bring us back to the original state which didn't work.
According to XUL docs setAttribute("value") and .value do not return identical values when read. I am not sure about writing to them. Maybe then one would suffice but I don't know which. So this is for safety.
Comment 19 Ian Neal 2012-02-14 13:55:19 PST
(In reply to :aceman from comment #18)
> (In reply to Ian Neal from comment #17)
> > Comment on attachment 596470 [details] [diff] [review]
> > patch v2
> > 
> > 
> > >+    if (type == "textbox") {
> > >+      var val = formElement.value;
> > >+      return val;
> > >     }
> > >     if ("value" in formElement) {
> > >       return formElement.value;
> > >     }
> > These could be combined into a single if statement (does "textbox" even need
> > to be special-cased anymore?)
> I would leave it special cased as it also has a specialcased counterpart in
> the setFormElementValue function. Just for understandability.
So just combine then:
if ((type == "textbox") || ("value" in formElement))
  return formElement.value

> > >+  else if (type == "textbox") {
> > >+    if (value == null || value == undefined) {
> > >+      formElement.setAttribute("value",null);
> > >+      formElement.value = null;
> > >+    } else {
> > >       formElement.setAttribute("value",value);
> > >+      formElement.value = value;
> > >+    }
> > This is unnecessarily complicated, just set variable to null when it is
> > undefined.
> 
> Which variable?
The variable value
> 
> > Do we really need to use both setAttribute and .value?
> Removing that would bring us back to the original state which didn't work.
> According to XUL docs setAttribute("value") and .value do not return
> identical values when read. I am not sure about writing to them. Maybe then
> one would suffice but I don't know which. So this is for safety.
Well you've changed the getter to use .value so the setter should be okay as .value
Comment 20 :aceman 2012-02-18 07:20:49 PST
Created attachment 598534 [details] [diff] [review]
patch v3

OK, it seems to work too.
Comment 21 Mark Banner (:standard8, afk until Dec) 2012-02-28 04:22:54 PST
Checked in: http://hg.mozilla.org/comm-central/rev/fea6ed5b3918
Comment 22 :aceman 2012-11-12 11:57:40 PST
Wayne, can you post to the GS topic and mark this [gssolved] when needed?
Comment 23 Wayne Mery (:wsmwk, NI for questions) 2012-11-12 12:01:35 PST
(In reply to :aceman from comment #22)
> Wayne, can you post to the GS topic and mark this [gssolved] when needed?

done. only one user in the topic
Comment 24 :aceman 2013-03-26 12:53:05 PDT
Test in bug 810763.

Note You need to log in before you can comment on or make changes to this bug.