353567 - Wrong default alarm set on new events (after start instead of before)

Status: VERIFIED FIXED

(Calendar :: Alarms, defect)

Description

[Stefan Sitter [:ssitter]]

Default alarm settings don't work properly if 'More>>' section of item dialog was not shown (wrong alarm set)

Steps to Reproduce:
1. Start Sunbird with clean profile
2. Go to Preferences->Alarms and set default alarm to On, 5 min before
3. Select File->New Event and create new event. Do NOT open 'More>>' section of item dialog.
4. Wait until alarm is shown

Actual Results:
Alarm is triggert 5 minutes after event start time.

Expected Results:
Alarm is triggert 5 minutes before event start time.

Additional Information:
Console shows that wrong information is read from dialog:
  considering alarm for item:foo
   offset:PT5M, which makes alarm time:2006/09/20 21:55:00 UTC
  now is 2006/09/20 21:47:52 UTC
  alarm is in the future

Repeat step 3 and 4 but open 'More>>' section before saving the alarm. This time the correct value is read from dialog:
  considering alarm for item:bar
   offset:-PT5M, which makes alarm time:2006/09/20 21:45:00 UTC
  now is 2006/09/20 21:49:38 UTC

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060919 Calendar/0.3a2+

Comment 1

[Stefan Sitter [:ssitter]]

Happens also if event is created with drag and drop in day/week view.

Comment 2

[Matthew (lilmatt) Willis]

Attachment: inverts the pref that we read

Comment 3

[Matthew (lilmatt) Willis]

Attachment: does it the same way the event dialog does it.

Comment 4

[cmtalbert]

Comment on attachment 240347 [details] [diff] [review]
does it the same way the event dialog does it.

looks good

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 240347 [details] [diff] [review]
does it the same way the event dialog does it.

r2=dmose

Comment 6

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Comment 7

[Sebastian Schwieger]

verified fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060928 Sunbird/0.3RC1

Comment 8

[Damian Szczepanik]

Litmus testcase 2698 created

Comment 9

[Sven Giermann]

Should this be re-opened or newly filed???

I get exactly these results in Lighting 0.5 - in latest nightly (with new prototype dialog) it is even being displayed wrong (15 minutes after, when I select 15 minutes before)

So my question: has this been ever fixed for Lightning?

Comment 10

[Stefan Sitter [:ssitter]]

(In reply to comment #9)
> Should this be re-opened or newly filed???

This was fixed for the non-prototype dialog in Sunbird and Lightning. If you experience a similar issue with the new prototype dialog file a new bug.

Comment 11

[Sven Giermann]

Attachment: Corrected attachment 240347 to address default '15 mintues before', when pref cannot be read (now in calendar-item-editing.js)

I further researched the problem and found, that this occured, because my prefs were being set wrong (german locale, unit=Minuten) which lead to the situation, that the 'catch(ex)' part was executed.

So the correct patch would be to either set it to '-15' there, or to set 'isNegative' after all this, which I chose.

You can reproduce the problem by manually setting a wrong 'calendar.alarms.eventalarmunit' or 'calendar.alarms.todoalarmunit' and then create a new event/todo.

Comment 12

[Michael Büttner (no reviews TFN)]

Comment on attachment 274743 [details] [diff] [review]
Corrected attachment 240347 [details] [diff] [review] to address default '15 mintues before', when pref cannot be read (now in calendar-item-editing.js)

Sven, thanks for the patch.  Shifting review request to daniel as he's responsible for alarms (as well as philipp). Please see [1] for the list of possible reviewers.

[1] http://wiki.mozilla.org/Calendar:Module_Ownership

Comment 13

[Daniel Boelzle [:dbo]]

Comment on attachment 274743 [details] [diff] [review]
Corrected attachment 240347 [details] [diff] [review] to address default '15 mintues before', when pref cannot be read (now in calendar-item-editing.js)

>         try {
>             if (alarmsBranch.getIntPref("onforevents") == 1) {
>                 var alarmOffset = Components.classes["@mozilla.org/calendar/duration;1"]
>                                             .createInstance(Components.interfaces.calIDuration);
>                 try {
>                     var units = alarmsBranch.getCharPref("eventalarmunit");
>                     alarmOffset[units] = alarmsBranch.getIntPref("eventalarmlen");
>-                    alarmOffset.isNegative = true;
>                 } catch(ex) {
>                     alarmOffset.minutes = 15;
>                 }
>+                alarmOffset.isNegative = true;
>                 aItem.alarmOffset = alarmOffset;
>                 aItem.alarmRelated = Components.interfaces.calIItemBase.ALARM_RELATED_START;
>             }
>         } catch (ex) {
>             Components.utils.reportError(
>                 "Failed to apply default alarm settings to event: " + ex);
>         }

r=dbo for this one, although I am wondering whether we should apply a default at all if it could not be read out correctly. Why not just remove the inner try-catch (i.e. the fallback to 15 minutes)? The user currently doesn't get any feedback why his default value doesn't work, but always gets 15 minutes.

Comment 14

[Daniel Boelzle [:dbo]]

Attachment: don't silently swallow pref errors

IMO we shouldn't silently swallow the pref getter exception.

Comment 15

[Michael Büttner (no reviews TFN)]

Comment on attachment 277715 [details] [diff] [review]
don't silently swallow pref errors

The inner try-block doesn't make sense -> r=mickey.

Comment 16

[Sven Giermann]

(In reply to comment #15)
> (From update of attachment 277715 [details] [diff] [review])
> The inner try-block doesn't make sense -> r=mickey.

Totally right, and the outer one does report an error.
I would not have searched for the wrong sign of the alarm, if there would have been no alarm set... -> vote=Sven

Comment 17

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH.

Comment 18

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.8pre) Gecko/20070921 Calendar/0.7pre