293183 - implement full exception support for recurrence

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

We need to implement the ability to have full overrides of any exception on any
recurring event, to allow you to do such things as change the times of one
meeting out of a recurring series, change the location/attendees, etc.  The item
should still stay linked to the original recurring event, so that if something
global changes (like if a new person gets added to a meeting) that is reflected
in all.

One issue is what to do about making global changes to a recurring event with
some occurrences that are in the past -- do you still modify those?  "Move all
future meetings to 2pm instead of 1pm" seems like you don't want to modify the
old ones.  Not sure what to do about that.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: early broken initial patch for moving all item properties into property bag

This patch was some of my early work in getting all item properties to go
through a property bag, so that there would be a clean way to implement
overrides in just one place.

Comment 2

[Mike Shaver (:shaver emeritus)]

I think we just decided here that we would not support exception changes that
change the time of an event.  It makes searching date ranges much more complex,
and the equivalent behaviour (mark a -ve exception at the old time, add a repeat
rule for the new time) is not extremely onerous.  To whatever degree the host
app wishes to, it can hide these steps from the user to simulate "moving" an
event.  (It was suggested that this is in fact the recommended method for
altering the times on one recurrence instance coming out of TC-RECUR at Calconnect.)

Comment 3

[Stuart Parmenter]

Attachment: Updated patch to trunk and fix some problems

I updated the previous patch to the trunk and fixed a few issues with the way
properties were being set.  I added a createProxy() call on calIItemBase that
would return a new proxy item...  The recurrence stuff still needs to be hooked
in...

Comment 4

[Stuart Parmenter]

Attachment: midly better pach

now i get random JS errors... "event.parent has no properties" and whatnot.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

taking this back

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: Volume 1 of the patch

This gargantuan patch does a bunch of things to prepare the way for proper
exception support:

1) Moves all item properties to the property bag, so they can be accessed and
overridden in a uniform way.

2) Exposes isPropertyPromoted in the interface, so that the providers can know
whether they need to serialize something as a property or not (some providers,
like ICS, can just work directly from the property bag and won't care.. storage
wants to be able to store some things directly in the tables for faster
searching)

3) Gets rid of calIItemOccurrence entirely, in favour of calIItemBase-based
proxy objects.	What were formerly occurrences are now returned as
calIItemBase's with a parentItem and a recurrenceId set.  (recurrenceId is a
calDateTime, in accordance with the ics spec!)

4) Reimplements calRecurrenceInfo in js instead of in C++.  No reason to have
it be in C++, as it doesn't interact with libical at all (RecurrenceRule does;
RecurrenceDate/DateSet can also be moved to js, but won't do that now because
they work fine and don't need any modification.

5) [small nit] Gets rid of hasAlarmTime from calIItemBase; instead, a null
alarmTime means there's no alarm time, and non-null means there is.

6) [small nit] Causes event boxes to wrap their label, and also to force the
size to be the size of the event (the label gets cropped).

7) Adds calIErrors.idl, which is an empty file where we can put in
calendar-specific error codes, to avoid NS_ERROR_FAILURE everywhere.  The patch
also changes a bunch of NS_ERROR_FAILURE to NS_ERROR_ITEM_IS_IMMUTABLE.

8) Expands out the exception interface in calIRecurrenceInfo in preparation of
implementation.

I realize this patch is fairly huge and that there's a lot of only
peripherally-related stuff here.  However, most of it is moving
calRecurrenceInfo to js, and splitting out the other bits would take a
significant amount of time (because the files have changes related to multiple
items from above.)

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 184747 [details] [diff] [review]
Volume 1 of the patch

new patch soon

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: calendar-exceptions.patch

Patch.

One semi-outstanding issue is that when you change an exception occurrence, you
have to call ex.parentItem.recurrenceInfo.modifyException(ex) to actually stick
it on the base item, otherwise it won't show up if you call
ex.parentItem.getExceptionFor(ex.recurrenceId) or similar.  I put this code in
the provider for now; I have an idea of how to fix it, but in practice it
shouldn't affect anything for now, and I'd rather not make this patch bigger
than it already is (the fix is to keep a list of all occurrences handed out and
always hand back the same object for any given recurrence id).

Comment 9

[Mike Shaver (:shaver emeritus)]

Comment on attachment 185710 [details] [diff] [review]
calendar-exceptions.patch

r=shaver with changes from f2f review.

Comment 10

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

Attachment: fix regressions

This checkin caused a number regressions with the old views. This patch should
fix them. (as long as there is no new monthview etc, i'd like to keep the old
stuff working)

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 185972 [details] [diff] [review]
fix regressions

whoops, sorry.. I missed some stuff while updating, I guess =/

r=me

Comment 12

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

patch checked in. (and assigning back to vlad. Must have checked a box i didn't
want to check)

Comment 13

[hcg]

The patches probably doesn't apply anymore, but does this need anything else than updated patches?

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

I think this bug is open from comment 8. The patches on the bug were actually cheked in as per comment 12 and comment 9 and 3/4. It doesn't look like there is anything important left to do though, if we notice issues we can always open a new bug. Thanks for the comment!