532391 - Check for new messages every x minutes could be set to 0 without checkbox disabled

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Carsten Krueger]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.5) Gecko/20091130 Thunderbird/3.0

Account Settings - Server settings
Set "check for new messages every x minutes" to 0. The checkbox is correctly disabled [ ]. Enable checkbox [x] but every minute stays 0.

Reproducible: Always

Steps to Reproduce:
Account Settings - Server settings
Set "check for new messages every x minutes" to 0. The checkbox is correctly disabled [ ]. Enable checkbox [x] but every minute is furthermore 0.
Actual Results:  
every minute is 0

Expected Results:  
every minute is 1

Might be a good idea to not allow 0 at all.

Comment 1

[:aceman]

I think I can do this. But does it have any bad effects? When does TB check for new mail if the interval stays on 0?

Comment 2

[:aceman]

Attachment: patch

This is the proposed fix.
It also updates nntp.maxArticles in the same way as these 2 fields share the checking functions.

I added some more checks and value sanitization (if user enters wrong value in prefs.js) and minimum allowed value. The side-effect is that it is no longer possible to enter 0 to disable the field (the corresponding checkbox). If you want to preserve this functionality I can do it.

I have seen other fields where a 0 value should not be allowed when checked (like 'do not download messages larger than' in Disk space pane or RSS checking interval) that could also use all these checks but currently do not have them. I can consolidate all these similar fields in another bug.

Comment 3

[Ian Neal]

Comment on attachment 598570 [details] [diff] [review]
patch


> function onCheckItem(changeElementId, checkElementId)
> {
>     var element = document.getElementById(changeElementId);
>     var notify = document.getElementById(checkElementId);
>+
>+    onBiffMinChange(checkElementId, element);
>     var checked = notify.checked;
> 
>+    if (checked && !getAccountValueIsLocked(notify)) {
Nit: You can inline checked, so just use notify.checked here.

>       element.removeAttribute("disabled");
>+      if (parseInt(element.value, 10) < parseInt(element.getAttribute("min"), 10))
Haven't we just checked this in onBiffMinChange, pity it is not passed back.

> /**
>  * Called when someone changes the biff-minutes value.  We'll check whether it's
>+ * less than allowed minimum, and if so, disable the biff checkbox as well,
Not true anymore, as you use this function for articles too now.

>+function onBiffMinChange(aCheckElementId, aValueElement)
You will need to change the function name.
> {
>+  if (parseInt(aValueElement.value, 10) < parseInt(aValueElement.getAttribute("min"), 10))
>+    document.getElementById(aCheckElementId).checked = false;
> }

>       <textbox wsm_persist="true" id="server.biffMinutes" size="3"
>                aria-labelledby="server.doBiff server.biffMinutes biffEnd"
>+               preftype="int" type="number" min="156" increment="1"
Do you really want a min of 156?
>+               onchange="onBiffMinChange('server.doBiff', this);"
>                prefstring="mail.server.%serverkey%.check_time"/>

>       <textbox wsm_persist="true" id="nntp.maxArticles"
>                size="4" type="number" min="1" increment="10"
>                aria-labelledby="nntp.notifyOn nntp.maxArticles maxMessagesEnd"
>                preftype="int"
>+               onchange="onBiffMinChange('nntp.maxArticles', this);"
This first argument should be 'nntp.notifyOn' shouldn't it?
>                prefstring="mail.server.%serverkey%.max_articles"/>

Comment 4

[:aceman]

Sorry, so many bugs ... :(
On the other hand, this new model would allow nntp.maxArticles to be set to 0 (if I set min="0" as 0 is no longer a special value) and still allow the warning checkbox to be enabled. That could be a valid configuration somebody would like (warn before downloading any messages).
But I am not sure if the backend would handle it fine.

Comment 5

[:aceman]

It seems just setting min= on the element makes the widget sanitize the value automatically so those "min" checks are not necessary. I'll rework it all.

Comment 6

[:aceman]

Attachment: patch v2

Simplified checks. I have described how this is supposed to operate in bug 728681.
I tested that backend does not warn when "ask me before downloading X messages" is set to 0. So I left min at 1 there.

Comment 7

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/3104846ab504