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

RESOLVED FIXED in Thunderbird 13.0

Status

MailNews Core
Account Manager
--
minor
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Carsten Krueger, Assigned: aceman)

Tracking

(Blocks: 1 bug, {polish, ux-error-prevention})

Trunk
Thunderbird 13.0
polish, ux-error-prevention
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

4.76 KB, patch
Ian Neal
: review+
standard8
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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
(Assignee)

Comment 1

6 years ago
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?
Component: Account Manager → Account Manager
OS: Windows XP → All
Product: Thunderbird → MailNews Core
QA Contact: account-manager → account-manager
Hardware: x86 → All
Version: unspecified → Trunk
(Assignee)

Updated

6 years ago
Assignee: nobody → acelists
Keywords: ux-error-prevention
(Assignee)

Comment 2

6 years ago
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.
Attachment #598570 - Flags: ui-review?(bwinton)
Attachment #598570 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED

Comment 3

6 years ago
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-
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 728681
(Assignee)

Comment 6

6 years ago
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.
Attachment #598570 - Attachment is obsolete: true
Attachment #598660 - Flags: review?(iann_bugzilla)
Attachment #598570 - Flags: ui-review?(bwinton)

Updated

6 years ago
Attachment #598660 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Updated

6 years ago
Attachment #598660 - Flags: review?(mbanner)
Attachment #598660 - Flags: review?(mbanner) → review+
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/3104846ab504
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
(Assignee)

Updated

5 years ago
Blocks: 755454
You need to log in before you can comment on or make changes to this bug.