272525 - UI inconsistancy for "Check new messages every X minutes" textfield

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[j.smith]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20041001 Firefox/0.10.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20041001 Firefox/0.10.1

In the "Account Settings" manager, if you click on "Server Settings" for each
account (in a mult-account setup), the "Check new messages every X minutes"
textfield is enabled even though it is unchecked.  This happens when you click
on "Server Settings" for one account and proceed to click on "Server Settings"
for another account.  If one account has the check messages enabled and if you
click on "Server Settings" on that account and then on another account, it will
show the "Check new messages..." textfield disabled on the account that does not
have it enabled.
NOTE:  I have not verified this on a single account setup.


Reproducible: Always
Steps to Reproduce:
1.  Go to "Tools -> Account Settings..."
2.  Click on "Server Settings" for each account

Actual Results:  
"Check new messages every X minutes" textfield is enabled when it is unchecked.
(at least one of them will be).


Expected Results:  
Should be disabled if "Check new messages..." is unchecked.


Machine info:
Windows NT 5.1 SP1; .NET CLR 1.1.4322.573

Thunderbird info:
version 0.9+ (20041122)

-----
Images of issue:
http://www11.brinkster.com/00zone00/tb.issue1.gif
http://www11.brinkster.com/00zone00/tb.issue2.gif
-----

Comment 1

[!Mike]

Confirmed using Mozilla Application Suite: Mozilla/5.0 (X11; U; Linux i686;
en-US; rv:1.7.5) Gecko/20050207. This happens only when selecting "Server
Settings" from a different account. If you stay inside the options of one
account it works.

Comment 2

[Magnus Melin [:mkmelin]]

Also: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b1) Gecko/20060807 Thunderbird/2.0a1 ID:2006080703

Comment 4

[Christian Eyrich]

It also affects others like "Messages larger than" in Disk Space and "Automatically delete junk mail older than" in Junk.
And that's a quite strange behaviour since it only affects those textboxes whose en-/disabling is implemented by adding/removing a disabled attribute of broadcasters and having an observes attribute on the textboxes.

In case of the latter of the above options the disabling doesn't work when switching between accounts as long the id's contain a dot like server.purgeSpam or even foo.purgeSpam. It works if they're e.g. server_purgeSpam or so. What's that?

Comment 5

[Christian Eyrich]

Attachment: patch for fixing 2/3 of the problem

Since in the cases of the server and offline panel only one element needs to be switched it can also be done by directly setting the disabled attribute for the element.
This avoids the problematic observer (and might be some cycles faster) and is simply simpler.

It doesn't solve the cause of the problem, but here's a patch in case someone is interested.

Comment 6

[Christian Eyrich]

Comment on attachment 267168 [details] [diff] [review]
patch for fixing 2/3 of the problem

Can we get this in to make things at least a bit better?

Comment 7

[Scott MacGregor]

Comment on attachment 267168 [details] [diff] [review]
patch for fixing 2/3 of the problem

Neil likes reviewing account manager bugs too.

Comment 8

[[e-mail]]

IIRC the problem with the broadcasters is that if someone sets the attribute directly on the observed element then you can't correct it by setting the attribute on the broadcaster, because the latter attribute is already "right".

Comment 9

[[e-mail]]

Comment on attachment 267168 [details] [diff] [review]
patch for fixing 2/3 of the problem

I think this is right, but just double-checking with IanN ;-)

Comment 10

[Ian Neal]

Comment on attachment 267168 [details] [diff] [review]
patch for fixing 2/3 of the problem

>Index: mailnews/base/prefs/resources/content/am-server.js
>===================================================================
>@@ -207,16 +207,14 @@ function secureSelect()
> 
> function setupBiffUI()
> function setupNotifyUI()
Both the above functions are near identical so could combine into a function similar to onCheckItem in am-offline.js

>Index: mailnews/base/prefs/resources/content/am-offline.xul
>===================================================================
>@@ -104,14 +100,14 @@
> 
>     <hbox align="center" class="indent" hidefor="none">
>         <checkbox  wsm_persist="true" id="offline.notDownload"
>-            label="&offlineNotDownload.label;" accesskey="&offlineNotDownload.accesskey;" oncommand="onCheckItem('bc_notDownload', 'offline.notDownload');"/>
>-        <textbox wsm_persist="true" id="offline.notDownloadMin" size="4" value="50"  observes="bc_notDownload"/> 
>+                   label="&offlineNotDownload.label;" accesskey="&offlineNotDownload.accesskey;" oncommand="onCheckItem('offline.notDownloadMin', 'offline.notDownload');"/>
The above line is over long, as you are touching it please split across two lines.
>+        <textbox wsm_persist="true" id="offline.notDownloadMin" size="4" value="50"/> 
>         <label value="&kb.label;" control="offline.notDownloadMin"/>
>     </hbox>
> 
>     <hbox align="center" class="indent" hidefor="movemail,pop3,imap,none">
>-        <checkbox wsm_persist="true" id="nntp.downloadMsg" label="&nntpDownloadMsg.label;" accesskey="&nntpDownloadMsg.accesskey;" oncommand="onCheckItem('bc_downloadMsg', 'nntp.downloadMsg');"/>
>-        <textbox wsm_persist="true" id="nntp.downloadMsgMin" size="2" value="30" observes="bc_downloadMsg"/>
>+      <checkbox wsm_persist="true" id="nntp.downloadMsg" label="&nntpDownloadMsg.label;" accesskey="&nntpDownloadMsg.accesskey;" oncommand="onCheckItem('nntp.downloadMsgMin', 'nntp.downloadMsg');"/>
The above line is over long, please split across two lines.
>+        <textbox wsm_persist="true" id="nntp.downloadMsgMin" size="2" value="30"/>
Indentation is incorrect, should line up with checkbox.

Comment 11

[Christian Eyrich]

Attachment: [checked in] 2/3 patch v2

This one should address Ians comment.

Comment 12

[Ian Neal]

Comment on attachment 270005 [details] [diff] [review]
[checked in] 2/3 patch v2

Looks good to me, r=me

Comment 13

[[e-mail]]

Comment on attachment 270005 [details] [diff] [review]
[checked in] 2/3 patch v2

I'm still catching up on bugmail (I've only just read IanN's review of the previous patch) so I'll punt this one to Scott ;-)

Comment 14

[[e-mail]]

Comment on attachment 270005 [details] [diff] [review]
[checked in] 2/3 patch v2

OK, I got there eventually ;-)

Comment 15

[Christian Eyrich]

Thanks Ian and Neil. Neil, would you be so kind to also check it in sometime or other?

Comment 16

[Phil Ringnalda (:philor)]

mailnews/base/prefs/resources/content/am-server.xul 1.119
mailnews/base/prefs/resources/content/am-server.js 1.71
mailnews/base/prefs/resources/content/am-offline.xul 1.28
mailnews/base/prefs/resources/content/am-offline.js 1.26

Comment 17

[Christian Eyrich]

Thanks Phil for checkin, marking fixed.