528329 - Alarm is not fired when dismissed and later set reminder again

Status: RESOLVED FIXED

(Calendar :: Alarms, defect)

Description

[Damian Szczepanik]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; 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.6pre) Gecko/20091112 Calendar/1.0pre

I set the alarm for event but then decided that don't want to have reminder. However later I changed decision and set reminder again. It did not fire.

Reproducible: Always

Steps to Reproduce:
1. Create new event for tomorrow with reminder 2 days before the action.
2. Alarm fires once you save the event.
3. Dismiss the alarm.
4. Reopen the event and change the alarm for 1 day before the action.
Actual Results:  
Alarm does not fire when event is closed.

Expected Results:  
Alarm should be fired because reminder has been set again.

Confirmed with local and google calendar.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

This should take care, I hope it covers all special cases. Please test.

Comment 2

[Matthew Mecca [:mmecca]]

Comment on attachment 547143 [details] [diff] [review]
Fix - v1

Looks good. r=mmecca

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Hmm I may have done something wrong with how recurring snooze props are handled, I'll take another look before checkin.

Comment 4

[vknowles2000]

Quick comment: I apologize for not being familiar with this system. Not sure if this is the appropriate forum. I am using Lightning 1.2.3 in Thunderbird 10.0.2 under WinXP. 

I noticed this same behavior--if I dismiss an alarm, then even if I open the event and change the reminder setting, I cannot get the alarm back. This is also true if I turn off the alarm, save the event, then turn the alarm back on.

Comment 5

[Matthew Mecca [:mmecca]]

Comment on attachment 547143 [details] [diff] [review]
Fix - v1

The only issue I can see with this version of the patch as that if a single exception is modified on a repeating item, and an alarm is modified for that exception only, and that exception's original alarm has already been dismissed or snoozed, then the new alarm won't fire (because the X-MOZ-SNOOZE-TIME-[RID] prop needs to be cleared from the parent item).

Since that is a special case and it's effect isn't any worse than the behavior without the patch, I'd suggest spinning that part off to a new bug and taking this patch for 1.9.2.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 547143 [details] [diff] [review]
Fix - v1

Review of attachment 547143 [details] [diff] [review]:
-----------------------------------------------------------------

The code is quite old, so here a few review comments on my own code :-P

I guess we could give it a try for 1.9.2, I'd like to see this bake at least in a beta build before we do though. Please re-request approval when its been in the beta for a while. Luckily the merge has already happened.

::: calendar/base/content/dialogs/calendar-dialog-utils.js
@@ +680,5 @@
> +       }
> +    }
> +
> +    // If the alarms differ, clear the snooze/dismiss properties
> +    if (oldAlarmMap.toSource() != "({})") {

Object.keys(oldAlarmMap).length > 0

@@ +687,5 @@
> +
> +        // Recurring item alarms potentially have more snooze props, remove them
> +        // all.
> +        while (propEnum.hasMoreElements()) {
> +            let prop = propEnum.getNext().QueryInterface(Components.interfaces.nsIProperty);

Probably best use fixIterator for this

Comment 7

[Matthew Mecca [:mmecca]]

Bug 861594 will likely change the way this will need to be fixed.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Matt, what should we do with this patch? Given the next ESR is not so far away I think it should be OK to just push it on c-c. Will you be fixing bug 861594 this cycle?

Comment 9

[Matthew Mecca [:mmecca]]

(In reply to Philipp Kewisch [:Fallen] from comment #8)
> Matt, what should we do with this patch? Given the next ESR is not so far
> away I think it should be OK to just push it on c-c. Will you be fixing bug
> 861594 this cycle?

I think we can take this patch now, I can work around it in Bug 861594, I'm not sure if that will be ready for the next ESR.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central changeset 7ac16d3b7c0f