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...
Status: RESOLVED FIXED
: 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
:
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
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 Carsten Krueger 2009-12-02 06:54:05 PST
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 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 :aceman 2012-02-18 12:50:33 PST
Created attachment 598570 [details] [diff] [review]
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 2012-02-18 15:37:48 PST
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 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 :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 :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 Mark Banner (:standard8, limited time in Dec) 2012-02-28 04:23:11 PST
Checked in: http://hg.mozilla.org/comm-central/rev/3104846ab504

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