Last Comment Bug 532391 - Check for new messages every x minutes could be set to 0 without checkbox disabled
: Check for new messages every x minutes could be set to 0 without checkbox dis...
: polish, ux-error-prevention
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
-- minor (vote)
: Thunderbird 13.0
Assigned To: :aceman
Depends on:
Blocks: 728681 755454
  Show dependency treegraph
Reported: 2009-12-02 06:54 PST by Carsten Krueger
Modified: 2012-05-15 12:38 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch (4.74 KB, patch)
2012-02-18 12:50 PST, :aceman
iann_bugzilla: review-
Details | Diff | Splinter Review
patch v2 (4.76 KB, patch)
2012-02-19 07:10 PST, :aceman
iann_bugzilla: review+
standard8: review+
Details | Diff | Splinter Review

Description User image Carsten Krueger 2009-12-02 06:54:05 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: 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: 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 User image :aceman 2012-02-11 17:48:05 PST
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 User image :aceman 2012-02-18 12:50:33 PST
Created attachment 598570 [details] [diff] [review]

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 User image Ian Neal 2012-02-18 15:37:48 PST
Comment on attachment 598570 [details] [diff] [review]

> 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 User image :aceman 2012-02-18 16:53:12 PST
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 User image :aceman 2012-02-19 05:47:57 PST
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 User image :aceman 2012-02-19 07:10:00 PST
Created attachment 598660 [details] [diff] [review]
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 User image Mark Banner (:standard8) 2012-02-28 04:23:11 PST
Checked in:

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