25473 - 0 should be not possible in "Check for new mail every x minutes"

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect, P3)

Description

[Henrik Gemal]

If you enter 0 in the "Check for new mail every x minuts" Mozilla goes crazy and 
checks the server all the time...
It should be possible to enter 0 in the option.

Comment 1

[Alec Flett]

Ah yes, this bit me many times.
reassign to biff guru, scottip.
scott, we should probably be dropping 0-minute biffs... adding myself to CC in
case I decide to fix this myself :)

Comment 2

[scottputterman]

ok.  this seems pretty important.  I hope I'm doing this right by adding the 
beta1 keyword.

Comment 3

[Henrik Gemal]

Forgot the "not" word...

Comment 4

[leger]

Putting on PDT+ radar for beta1.

Comment 5

[scottputterman]

checked in the fix.  If 0 minutes then it doesn't get biffed.

Comment 6

[fenella]

Linux (2000-02-03-08 M14)
Win32 (2000-02-03-10 M14)
Mac (2000-02-03-15 M14)
"If 0 minutes then it doesn't get biffed" is working on all platforms.

Comment 7

[jglick]

According to Fenella, the user could have the "Check for new mail every <X> 
minutes" box checked, and since 0 is entered in the text field, no biff will 
occur at all.  This is kinda confusing.  User may wonder why they have this box 
selected, but biff is not working. 

Some potential solutions:
1. Disallow the user to enter 0 in the "Check for new mail every X minutes"  
field.  If 0 is entered, it is replaced by a 1 (or smallest valid #), when the 
user tabs out of the field or closes the dialog.
2. When the user enter 0 in this field, the application automatically unchecks 
the checkbox.
3.  If a user enters 0, a dialog pops up and informs the user that 0 is not an 
valid number.  

Personally, I think #1 is the best way to go.  What the user was trying to do 
was turn on biff and set the shortest possible interval.  By automatically 
setting it to the smallest allowable interval if a 0 is entered, the user gets 
the behavior closes to what they wanted.  

Automatically  unchecking the box.  Users may not notice this and wonder why 
biff isn't working. "I keep going back to that dialog and turning the feature 
on, but it keeps turning it self off!"

And its nice to avoid the dialog is possible.

Comment 8

[fenella]

I am going to open a new bug to track the discussion.

Comment 9

[fenella]

Please see bug 26884

Comment 10

[Henrik Gemal]

Regression:
Now entering 0 just saves 0.

btw it's also possible to enter fx -200

Comment 11

[scottputterman]

Henrik, 

what do you mean?  Are you seeing something different than 26884?

Comment 12

[Henrik Gemal]

Isn't bug 26884 about biff and stuff?
I'm just saying that it should not be possible to enter 0 and negative numbers 
into the "Check for new mail every x minuts" input field. Isn't that what this 
bug is about?

Comment 13

[scottputterman]

it eventually got changed to be about what this bug describes.  however, since
this came first, I'll mark that one as a dup of this one and reassing this to alecf.

Comment 14

[scottputterman]

*** Bug 26884 has been marked as a duplicate of this bug. ***

Comment 15

[Alec Flett]

all that's left is minor. pushing off till later.

Comment 16

[scottputterman]

moving to future milestone.

Comment 17

[Alec Flett]

massive reassign of account manager bugs -> sspitzer
please feel free to put me back on the CC if you have any questions/comments

Comment 18

[Daniel Bratell]

I made a small attempt at this, but I can't get event handlers to fire in XUL. 
Neither onBlur, onFocus, onChange, onKeyUp, onKeyDown or onKeyPress fired. I 
don't know XUL but I thought this should work:

Index: content/am-server.xul
===================================================================
RCS file: /cvsroot/mozilla/mailnews/base/prefs/resources/content/am-server.xul,v
retrieving revision 1.51
diff -u -r1.51 am-server.xul
--- am-server.xul       2000/09/14 05:17:54     1.51
+++ am-server.xul       2000/12/02 13:07:34
@@ -56,7 +56,8 @@
 -->
   <box orient="horizontal" autostretch="never">
     <checkbox wsm_persist="true" id="server.doBiff" value="&biffStart.label;"/>

-    <textfield wsm_persist="true" id="server.biffMinutes" size="3"/>
+    <textfield wsm_persist="true" id="server.biffMinutes" size="3"
+            onBlur="verifyAndFixBiffTime(this)"/>
     <text class="label" for="server.biffMinutes" value="&biffEnd.label;"/>
   </box>
   <checkbox style="margin-left: 2em" hidefor="imap,nntp"




Index: content/am-server.js
===================================================================
RCS file: /cvsroot/mozilla/mailnews/base/prefs/resources/content/am-server.js,v
retrieving revision 1.22
diff -u -r1.22 am-server.js
--- am-server.js        2000/09/01 00:47:22     1.22
+++ am-server.js        2000/12/02 13:07:34
@@ -176,3 +176,21 @@

     return box;
 }
+
+// Removes strange input from the field and replaces with
+// a number. If the input couldn't be parsed as a number
+// we put 10 in the field. If it's a negative number or 0,
+// we put 1 in it. Otherwise we just put the numerical
+// representation of the input in it.
+function verifyAndFixBiffTime(biffField)
+{
+    var inputMinutes = parseInt(biffField.value);
+
+    if (isNaN(inputMinutes))
+        biffField.value = 10;
+    else if (inputMinutes <= 0)
+        biffField.value = 1;
+    else
+        biffField.value = inputMinutes;
+}

Comment 19

[Blake Ross]

XUL node attributes are case sensitive, so `onblur' (not `onBlur') should fire.

Comment 20

[Blake Ross]

but why would we want to remove it onblur?  Can't we just disallow it 
onkeydown?  Seems confusing to erase it after they've typed it in and the field 
loses focus.

Comment 21

[Daniel Bratell]

Thanks. It works now. That explains why I found so few onBlurs in the tree 
(but there are some). 

The reason for doing it at blur instead of after key press is that it can be 
more confusing when you can't type what you want. For instance, change from 10 
to 20 minutes by deleting the "1" (disallowed since that would leave "0" in the 
field) and inserting a "2". Sometimes it's nice to be allowed to pass through an 
illegal state between two legal states, and I don't think it will be confusing 
anyway. 

Also, not least, I find it difficult to implement that, with regard to 
selections, cut'n'paste, caret positions, deletions, and probably many other 
things i'm missing. 

I'm attaching the correct fix. Blake, if you accept my arguments above, could 
you review the fix?

Comment 22

[Daniel Bratell]

Attachment: Proposed fix to disallow nonpositive numbers in the UI.

Comment 23

[Daniel Bratell]

Forget what I said about wrong case on onBlur. None of that was in a xml file.

Comment 24

[Blake Ross]

Sure, I can r=, but cc'ing matthew first for some input.  He's been asking for 
something similar to this, but (I believe) to completely ignore certain keys 
(and c&p which, you're right, would be difficult).  But I do agree that in this 
case, we really don't want something like that.  I'm still not sure that onblur 
is the best solution, although it is non-intrusive.   Matthew, what do you 
think?  Daniel, want to take this bug?

Comment 25

[Daniel Bratell]

Yes. Here's me stealing the bug. I hope noone minds.

Comment 26

[(not reading, please use [e-mail] instead)]

daniel, thanks for taking this one.

don't default the time to 1 if the user enters a number <= 0.
default it to 10, like you do if it is NaN.
default values of 1 for biff put a load on servers.

as far as the textfield goes, you can use oninput="" to fire when the user does
anything, including copy and paste.

please let me review before you check in.

Comment 27

[Blake Ross]

But I believe Daniel wants this validation to happen when focus leaves the 
textfield, so oninput wouldn't be appropriate for that.  (unless you were 
talking about the desire to be able to prevent certain keys from entering, as I 
mentioned for another bug, but in that case...will oninput let you prevent the 
keystroke before it even has a chance to appear?  in other words, does it fire 
onkeydown or onkeypress?)

Also, I understand the point about the load on the servers, but if the user 
enters (for example) -1 or 0, shouldn't we assume that they'd prefer 1 over 10 
(it being the closest acceptable value to their input)?

Comment 28

[(not reading, please use [e-mail] instead)]

actually, the more I think about this, I think we should just handle it in the
back end.

if the interval value is 0 or -1, we just do the default which is every ten
minutes (see mailnews.js).

It should be the back end's responsibility not to pound on the server and do the
right thing.

comments?

Comment 29

[Blake Ross]

Shouldn't the UI reflect what you're doing behind the scenes?  Probably 
shouldn't let the user think he can get new mail every 0 minutes if he can't...

Comment 30

[Daniel Bratell]

Ouch. I thought this was a trivial one.

I compromise (I am swedish you know :-) ) which I find reasonable is to replace
anything that's not a positive number with 10. If the user writes 0 we replace
it with "1". The rational behind that is found in jglick's comment 2000-02-07
11:42. Íf the user writes 0 it's probably because she wants to check as often as
possible. Therefore it's perfecty reasonable to replace it with the lowest legal
number "1".

The backend is already handling this in the way that an illegal value cause biff
to be turned off. Combined with the front end patch I'm about to attach, I think
it's good enough. (Preferably we would enter each users mind with telepathy and
learn them all about what they are supposed to do)

I'm still looking for reviews, but I can wait if you want input from some UI expert.

Comment 31

[Daniel Bratell]

Attachment: Patch take #2

Comment 32

[Henrik Gemal]

Attachment: Screenshot: Would this help? Options taken from Outlook Express

Comment 33

[Daniel Bratell]

That's better but I don't think we have the tools in XUL to do it with a spin
(without lots and lots of work). Disabling the field when deselcting the
checkbox would be possible though, but that's not in the scope of this bug. (Yet?)

Comment 34

[Matthew T (active 1999-2002)]

Somebody file an RFE for a spin box widget, we'll need it for other stuff later 
anyway. But even if we used it here, that wouldn't solve the original problem, 
because the user would still be able to enter `0' in the text field.

In this case, would it be any great loss if we just provided a popup menu with a 
selection of useful options? Will anyone really mind if they can't set Mozilla to 
check mail every 7 minutes, and have to choose between every 5 minutes or every 
10 minutes instead?

[/] Automatically check for new messages every: [30 minutes    :^]

Options: minute, 5 minutes, 10 minutes, 30 minutes, hour, two hours, day. (If 
someone wants to check their mail between every two hours and every day, or less 
often than every day, they're extremely likely to be doing it manually anyway.)

Comment 35

[Daniel Bratell]

I don't think people are finding the textfield difficult. It's even prefilled by
default so people can both see an example of input and don't need to do anything
at all if they don't care/dare. Therefore I don't see the need to limit the user
to certain discrete choices. Experienced users are also used to be able to
select a number of their choice (after careful tuning I have 3 minutes at work,
13 minutes at home for my primary account and 20 for my secondary).

Also, I'm impressed by all UI opinions but I also want to ask, do we want this
fixed today or some time in the future. There is a small patch in this bug that
takes care of the issues we had. I think it's good and I don't think anything
better will be produced soon. We don't have a spin box and it might be long
before we do. To restrict the user while typing will be very hard to implement.
If nobody can come up with some really good arguments why this is a conceptually
bad solution I want the code reviews and when that is done I want to check it in.

Comment 36

[jglick]

If a user enters an invalid number (negative, zero, too high):
1. Once focus leaves the text field, replace the value with the closes allowable 
value. For negative numbers and 0, this would be 1. For numbers too large, this 
would be the max number allowed.
OR
2. Once focus leaves the text field a dialog pops up and informs the user that 
"The value for this field must be between X and Y." Users ok's dialog and focus 
goes back to text field.

Comment 37

[Henrik Gemal]

If a user enters 0, isn't that because he wants to disable the checking?

Comment 38

[jglick]

Some users may think that. Others may think entering 0 means they want to check 
for mail continually. Which makes me lean more towards suggestion 2 (Once focus 
leaves the text field a dialog pops up and informs the user that "The value for 
this field must be between X and Y." Users ok's dialog and focus goes back to 
text field.).

Comment 39

[Henrik Gemal]

"Somebody file an RFE for a spin box widget"...
did anybody do this?

Comment 40

[racham]

Adding mail6 keyword.

Comment 41

[fenella]

Assign to Sheela.

Comment 42

[Sheela Ravindran]

tagging this to biff tracking bug.

Comment 43

[Daniel Bratell]

I'm not working on this. If someone else wants to make a new patch, be my guest.

Comment 44

[Henrik Gemal]

This bug can now be fixed using the new spinbutton widget!

See http://bugzilla.mozilla.org/show_bug.cgi?id=90314

Comment 45

[(not reading, please use [e-mail] instead)]

before you go creating a fix for this, let's give jglick a chance to spec it out.

http://www.mozilla.org/mailnews/specs/accounts/#AccountSettings

jglick is overloaded, so it will be a while.

Comment 46

[jglick]

The spin widget is great, but given that the user can still manually enter a 
value (and hence enter invalid values), the problem for which this bug was 
written has not been solved.

As mentioned 12-04-2000, options fixing this problem are:

If a user enters an invalid number (negative, zero, too high):
1. Once focus leaves the text field, replace the value with the closes allowable 
value. For negative numbers and 0, this would be 1. For numbers too large, this 
would be the max number allowed.
OR
2. Once focus leaves the text field a dialog pops up and informs the user that 
"The value for this field must be between X and Y." Users ok's dialog and focus 
goes back to text field.

As  Henrik Gemal mentions though, if a user enters 0, we don't know if that 
is because they wanted to Never have biff occur, or they wanted to continually 
have biff occuring. Hence, option 2 above makes the user decide.

Outlook Express uses the spin widget. The use of the "-" sign is not allowed 
(keyboard entry is ignored). If a positive value not within the allowable range 
is entered, a dialog opens informing users of the allowable range. I know mpt 
hates dialogs, since this situation is probably pretty rare, I think a dialog is 
ok.  

Recommending option 2 above, plus not allowing the negative sign "-" if 
possible.

Comment 47

[Daniel Bratell]

Oh, was this assigned to me? Well, since my hack wasn't good enough, I'm leaving
this bug back to the default owners.

Comment 48

[Kai Engert [:KaiE:]]

Changing my e-mail address.

Comment 49

[Kai Engert [:KaiE:]]

Removing old e-mail address.

Comment 50

[Sebastian Späth]

I loved Daniel's proposal. Please don't make this bug appear more difficult than
it really is:

- check the value only on "onblur"
  (I e.g. would go from 20 to 10 e.g. by deleting the 2 
   resulting in a temporary illegal 0)
- change an entry smaller than 1 to 1 when leaving the box.

If the user intended to turn biff off, he will notice now at latest that there
is a checkbox better suited to his purposes.
If the user wanted to check as often as possible, he will still be doing that.
It should be fixed at the Frontend side: Allowing 0 and negative values for a
time pref is a bad thing IMHO.
Therefore let's take Daniels patch and check it in (if it hasn't rotten by now)

Comment 51

[Tuukka Tolvanen (sp3000)]

*** Bug 168819 has been marked as a duplicate of this bug. ***

Comment 52

[Tuukka Tolvanen (sp3000)]

Oh, the back-end fix (comment 5) seems to have regressed (bug 168819) --
continuous mail-checking does happen with biff time set to 0, although not
immediately after starting mail.

Comment 53

[(not reading, please use [e-mail] instead)]

mass re-assign.

Comment 54

[Greg Ubben]

This bug just bit me.  Subtle variation...  While testing Mozilla 1.6
with IMAP, I tried setting the check time to .5 minutes, wondering if
this would biff every 30 seconds.  Mozilla silently turned this into
0 minutes and proceded to spin, eating CPU and memory until killed.

Maybe this will get this back on someone's radar.  After 4 years, we
need at least some basic fix to prevent a hang by entering 0 or a
fraction... can do it "right" in a subsequent patch.  If the easiest
fix is in the back end, then even that would be better than nothing
for now.

Comment 55

[Joey Minta]

Attachment: patch v1

This patch converts the textbox to be a numeric spin-box, so we'll get input validation, and then unchecks the biff checkbox if someone sets the value to 0.  That seems, to me, to be a reasonable solution to the problems described in this bug.

Comment 56

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 306187 [details] [diff] [review]
patch v1

r+sr=dmose

Comment 57

[Joey Minta]

I've checked in the patch, which I believe qualifies this bug to be marked FIXED.  (0 is still a possible input, but it disables the checkbox, which makes sense.)

Checking in mailnews/base/prefs/resources/content/am-server.js;
/cvsroot/mozilla/mailnews/base/prefs/resources/content/am-server.js,v  <--  am-server.js
new revision: 1.73; previous revision: 1.72
done
Checking in mailnews/base/prefs/resources/content/am-server.xul;
/cvsroot/mozilla/mailnews/base/prefs/resources/content/am-server.xul,v  <--  am-server.xul
new revision: 1.122; previous revision: 1.121
done