Closed Bug 532391 Opened 10 years ago Closed 8 years ago

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

Categories

(MailNews Core :: Account Manager, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 13.0

People

(Reporter: carsten, Assigned: aceman)

References

(Blocks 1 open bug)

Details

(Keywords: polish, ux-error-prevention)

Attachments

(1 file, 1 obsolete file)

4.76 KB, patch
iann_bugzilla
: review+
standard8
: review+
Details | Diff | Splinter Review
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: polish
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?
OS: Windows XP → All
Product: Thunderbird → MailNews Core
QA Contact: account-manager → account-manager
Hardware: x86 → All
Version: unspecified → Trunk
Assignee: nobody → acelists
Attached patch patch (obsolete) — Splinter 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.
Attachment #598570 - Flags: ui-review?(bwinton)
Attachment #598570 - Flags: review?(iann_bugzilla)
Status: NEW → ASSIGNED
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"/>
Attachment #598570 - Flags: review?(iann_bugzilla) → review-
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.
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.
Blocks: 728681
Attached patch patch v2Splinter Review
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.
Attachment #598570 - Attachment is obsolete: true
Attachment #598660 - Flags: review?(iann_bugzilla)
Attachment #598570 - Flags: ui-review?(bwinton)
Attachment #598660 - Flags: review?(iann_bugzilla) → review+
Attachment #598660 - Flags: review?(mbanner)
Attachment #598660 - Flags: review?(mbanner) → review+
Checked in: http://hg.mozilla.org/comm-central/rev/3104846ab504
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
Blocks: 755454
You need to log in before you can comment on or make changes to this bug.