Alarm is not fired when dismissed and later set reminder again

RESOLVED FIXED in 2.6

Status

Calendar
Alarms
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: Damian Szczepanik, Assigned: Fallen)

Tracking

Trunk
x86
Windows XP
Bug Flags:
blocking-calendar1.0 +

Details

(Whiteboard: [needed beta][no l10n impact])

Attachments

(1 attachment)

(Reporter)

Description

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

Updated

8 years ago
Flags: blocking-calendar1.0+
Whiteboard: [not needed beta][no l10n impact]
Created attachment 547143 [details] [diff] [review]
Fix - v1

This should take care, I hope it covers all special cases. Please test.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #547143 - Flags: review?(matthew.mecca)
(Assignee)

Updated

7 years ago
Whiteboard: [not needed beta][no l10n impact] → [not needed beta][no l10n impact][needs review]
Comment on attachment 547143 [details] [diff] [review]
Fix - v1

Looks good. r=mmecca
Attachment #547143 - Flags: review?(matthew.mecca) → review+
Hmm I may have done something wrong with how recurring snooze props are handled, I'll take another look before checkin.
Whiteboard: [not needed beta][no l10n impact][needs review] → [needed beta][no l10n impact][needs patchwork]

Comment 4

6 years ago
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 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.
Attachment #547143 - Flags: approval-calendar-release?(philipp)
Attachment #547143 - Flags: approval-calendar-beta?(philipp)
Attachment #547143 - Flags: approval-calendar-aurora?(philipp)
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
Attachment #547143 - Flags: approval-calendar-release?(philipp)
Attachment #547143 - Flags: approval-calendar-release-
Attachment #547143 - Flags: approval-calendar-beta?(philipp)
Attachment #547143 - Flags: approval-calendar-beta+
Attachment #547143 - Flags: approval-calendar-aurora?(philipp)
Attachment #547143 - Flags: approval-calendar-aurora+
Bug 861594 will likely change the way this will need to be fixed.
Depends on: 861594
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?
(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.
No longer depends on: 861594
(Assignee)

Updated

5 years ago
Attachment #547143 - Flags: approval-calendar-release-
Attachment #547143 - Flags: approval-calendar-beta+
Attachment #547143 - Flags: approval-calendar-aurora+
Pushed to comm-central changeset 7ac16d3b7c0f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.6
Whiteboard: [needed beta][no l10n impact][needs patchwork] → [needed beta][no l10n impact]
You need to log in before you can comment on or make changes to this bug.