302588 - sunbird freezes if i recur an event without specifying the interval

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

When I set Sunbird to recur an event by clicking the "Repeat every" checkbox on
the "Recurrence" tab, if I don't specify an interval like "1" in the interval
text field next to the checkbox, Sunbird freezes, and my drive whirs like a
banshee off and on.  If I force quit Sunbird, and then restart it, it stays
frozen.  I have to start a fresh profile to get it working again.

Sunbird shouldn't freeze in any case, but it should assume a sensible value for
that field if none is provided, f.e. "1".  "1" is is what I was intending when I
left that field blank.  I read "Repeat every <blank> weeks" as "Repeat every
week" and assumed Sunbird would interpret it the same.

Comment 1

[Joey Minta]

Attachment: add more warnings

Rather than guess at the desired numbers (because guessing requires
preferences, etc and we have enough of those), this patch adds new warning
messages that make it so that 'OK' cannot be chosen if a needed field is empty.


Myk: Thanks for the bug!  This is exactly the kind of testing we need more of.

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 190937 [details] [diff] [review]
add more warnings

>+    if (getElementValue("repeat-numberoftimes-radio", "selected") &&
>+       getElementValue("repeat-numberoftimes-textbox") == "") {

Can you change that to check if the textbox actually contains a number? If i
fill in a space, that should be an error too.

>+    }
>+    else
>+        setElementValue("repeat-numberoftimes-warning", true, "hidden");

I prefer to add {} around the else block, when the if block had them.

>+        if (getElementValue("repeat-length-field") == "") {
Same think about the number check here. (There should be a xul element that
only accepts number, and maybe has increase/decrease buttons....)

r- for now, because a space will cause the same problem, just less noticable.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

> Rather than guess at the desired numbers (because guessing requires
> preferences, etc and we have enough of those),

Not really, since guessing wrong doesn't cost much (whether you don't guess or
guess wrong I still have to type a number into that field), and guessing right
most of the time (which I'll wager you would do if you assumed the value "1"
when that field is empty) saves a lot of work.


> Myk: Thanks for the bug!  This is exactly the kind of testing we need more of.

I need a calendar, and I hope it can be Sunbird.  I'll keep filing bugs as I run
into issues.

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #1)
> Rather than guess at the desired numbers (because guessing requires
> preferences, etc and we have enough of those)

Why does guessing require a pref? guessing 1 when nothing was filled in doesn't
need a pref afaics.
(But still, we need to check for only numbers. a space or a letter isn't valid,
and shouldn't be accepted. So we still need a least parts of the patch)

(In reply to comment #0)
> If I force quit Sunbird, and then restart it, it stays
> frozen.

That means there is invalid data in a calendar. I wonder if importing an invalid
ics file can cause the same behaviour. That wouldn't be good.

Comment 5

[Joey Minta]

Attachment: add more warnings v2

Now only works for positive integer numbers.  The function that checks it is in
applicationUtil.js, so we should probably start using it for all the other
textboxes that need similar input-checking.  Also guesses 1 for the value, but
I still think we're going to get people asking for a way to change this guess.

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 191404 [details] [diff] [review]
add more warnings v2

>Index: mozilla/calendar/resources/content/applicationUtil.js
>+function hasPositiveIntegerValue(elementId)
>+{
>+    var value = document.getElementById(elementId).value;
>+    if (value && value.match(/^\d+$/) && value > 0)

parseInt(value) might come in handy, and will eliminate the regexp.

r=mvl with that changed.

Comment 7

[Joey Minta]

patch checked in

Comment 8

[Myk Melez [:myk] [@mykmelez]]

I can't recur an event without specifying the interval anymore, and Sunbird
guesses "1" by default.