Note: There are a few cases of duplicates in user autocompletion which are being worked on.

alarms on recurring items don't really work

RESOLVED FIXED

Status

Calendar
Lightning Only
--
major
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Nidheesh Dubey, Assigned: Joey Minta)

Tracking

({dataloss})

Details

(Whiteboard: [swag:2d])

Attachments

(1 attachment)

(Reporter)

Description

12 years ago
Double click an open an even from agenda pane for any event that has alarm set to go off say 30 min before event. The event detail does not show this alarm info though calendar event shows it. Weird since I would imagine both to be pulling the info from same store.

Comment 1

12 years ago
I can reproduce this; it happens only on recurring events.  Furthermore, it happens in the views too, but if you edit the individual occurrence and not all events.

Comment 2

12 years ago
This is a side-effect of not using makeMemberAttr for alarmOffset and friends.  Requests for properties generated with makeMemberAttr pass through to the parent if they are not present on the proxy by virtue of using explicit getter and setter methods.  Just using makeMemberAttr is not enough to fix this, however, as that only does pass-through on things that are in the property bag, which the alarm stuff is not.  

The possible fixes that I can think of seem relatively risky to me, so at this point, my inclination is to relnote this for 0.1.  This is unfortunate, since it means that any exceptions created via the event dialog will lose their alarm.

I am still not clear on why the calAlarmService seems to fire alarms for occurrences despite the existence of this bug, so my understanding of whats going on here is not yet complete.
Severity: normal → major
Keywords: dataloss
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [cal relnote]

Comment 3

12 years ago
After discussion with Joey, we've concluded that in addition to the makeMemberAttr problem, it's also the case the onAddItem incorrectly uses the parent rather than the occurrences to fire alarms.  This will really be solved when the current infrastructure is replaced with calIAlarm.
Summary: Agenda pane event does not show alarm correctly → alarms on recurring items don't really work
(Assignee)

Updated

11 years ago
Flags: blocking0.3+
(Assignee)

Updated

11 years ago
Whiteboard: [cal relnote] → [cal relnote][swag:2d]
(Assignee)

Comment 4

11 years ago
Created attachment 232653 [details] [diff] [review]
occurrences and parents

There are 2 parts to this patch.  First, we need to expand items to occurrences, if a new recurring item is added. Second, we need to check both the item and its parent for alarm information, since those properties aren't automatically forwarded on to child-items.
Assignee: nobody → jminta
Status: NEW → ASSIGNED
Attachment #232653 - Flags: second-review?(dmose)
Attachment #232653 - Flags: first-review?(mattwillis)

Updated

11 years ago
Blocks: 344919
Comment on attachment 232653 [details] [diff] [review]
occurrences and parents

"jminta: any time you have 2 instances of the same parent with alarms in the same 6hr window you're in trouble"

r=lilmatt with a followup filed to fix the above case per our IRC discussion (and remove the dumps)
Attachment #232653 - Flags: first-review?(mattwillis) → first-review+
(Assignee)

Comment 6

11 years ago
Comment on attachment 232653 [details] [diff] [review]
occurrences and parents

+            if (!aItem.alarmOffset &&
+                !(aItem.parentItem && aItem.parentItem.alarmOffset)) {
+                return;
+            }
dmose: this can be factored into a helper function.
jminta: this will need to be done for snooze, since there will be extra checks.
dmose: ok.

+                start.month -= 2;
Comment here, or at least somewhere useful.

+        var offset = aItem.alarmOffset;
+        if (!offset) {
+            offset = aItem.parentItem.alarmOffset;
+        }
This should just be an ||.

+            var lastAck = aItem.alarmLastAck || aItem.parentItem.alarmLastAck
semicolon would be nice.

r=dmose with that.
Attachment #232653 - Flags: second-review?(dmose) → second-review+
(Assignee)

Comment 7

11 years ago
patch checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 8

11 years ago
(In reply to comment #6)
> (From update of attachment 232653 [details] [diff] [review] [edit])
> +            if (!aItem.alarmOffset &&
> +                !(aItem.parentItem && aItem.parentItem.alarmOffset)) {
> +                return;
> +            }
> dmose: this can be factored into a helper function.

Why not implement this right in calItemBase once for all, like proxied items do for attendees, organizer etc...?
(Assignee)

Comment 9

11 years ago
(In reply to comment #8)
> Why not implement this right in calItemBase once for all, like proxied items do
> for attendees, organizer etc...?
> 
You're welcome to write that patch.  mvl also mentioned he may want to refactor calItemBase to make it a bit more sane.  I don't know if the intended to address this issue or not in that patch.
(In reply to comment #9)
> You're welcome to write that patch.  mvl also mentioned he may want to refactor
> calItemBase to make it a bit more sane.  I don't know if the intended to
> address this issue or not in that patch.

Thinking further about that "alarmOffset = item.alarmOffset || item.parentItem.alarmOffset" mimic, I think that it is a bug: People might want to define exception items explicitly switching off the alarm. Currently they will get the parent's alarm setting.
So the fix IMO tends to be in calItemBase::initializeProxy(): Setting the parent's alarmOffset, alarmRelated[, alarmLastAck] and reviewing all code with the stated mimic.
What do you think?
Whiteboard: [cal relnote][swag:2d] → [swag:2d]

Comment 11

10 years ago
Bug 368976 was filed apparently for the issue mentioned in Comment #10.
You need to log in before you can comment on or make changes to this bug.