328084 - mixing modify 'single ocurrence' and 'all ocurrences' plays havok

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Michael Büttner (no reviews TFN)]

steps to reproduce:

1) create a new recurring event, say daily for 5 days
2) move one of the ocurrences to another start-time, answer 'this ocurrence only' to the following dialog.
3) move another ocurrence to some other start-time, answer 'all ocurrences' to the following dialog.

all ocurrences will instantly show up at the time you moved the second ocurrence to, but the first ocurrence will stick to where it was. effectively a duplicated ocurrence has been created.

Comment 1

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

modifying the startDate of a recurring event did not take into account already existing exceptions for that series. this effectively invalidated exceptions, since the recurrenceId does no longer match the date of the corrosponding ocurrence.
this patch also modifies the given startDate to enable the situation if the user drags an arbitrary ocurrence from the series but selects 'all ocurrences' in the dialog.
the question whether to modify all or just the single ocurrence won't show up in case the user modified an item which is already an exception.

Comment 2

[Joey Minta]

Comment on attachment 212750 [details] [diff] [review]
patch v1

+    // if the user wants to edit an occurrence which is already
+    // an exception, always edit this single item.

I'm not clear on the reasons for the introduction of this clause.  To me, it completely clashes with the user's mental model of a recurring event.  In my mind, if I create a recurring event, and change the title on that event, I still consider it part of the series.  I would expect it to behave exactly like any non-altered item in the series.  Can you explain further your reasoning here?

Comment 3

[Michael Büttner (no reviews TFN)]

Attachment: patch v2

the previous patch did not take RDATE's and EXDATE's into account, this is now fixed in the new patch. to verify this, delete single ocurrences from the series and drag'n'drop the whole series (modify all) to a new date. since the first patch did not care about EXDATE's the previously deleted items appeared again, but as already said, this is now fixed.

Comment 4

[Joey Minta]

Comment on attachment 212774 [details] [diff] [review]
patch v2

+        var exceptions = rec.getExceptionIds({});
+        for each (var exid in exceptions) {
+            if (exid.compare(occurrence.recurrenceId) == 0) {
+                return occurrence;
+            }
+        }
This looks like the perfect case for using .some(), and it ought to be faster.

+        // changing the startdate of an item needs to take exceptions into account.
+        // in case we're about to modify a parentItem (aka 'folded' item), we need
Just out of curiousity, where is it called 'folded'?  I've never heard that term.

+            var oldStart = this.startDate;
+            if(oldStart != null) {
js> var ev = Components.classes["@mozilla.org/calendar/event;1"].createInstance(Components.interfaces.calIEvent);
js> ev.startDate
0000/01/00 00:00:00 UTC
js> ev.startDate == null
false
Events are required to have startDates, so I don't see how this could ever be null.

I stopped reviewing here after comparing this code with calTodo...

The code here is almost identical in calevent.js and calTodo.js and whenever I see this much duplication, it throws up a warning flag in my mind.  At the very least, we probably want a helper function in calItemBase.js, but really, we may want to make an adjustStartingDate(newDate, oldDate) method in calRecurrenceInfo.

My questions are therefore:
(1) Are there reasons for not combining this code into a single method in a common location?  If there are, then I'll continue with the review.
(2) If not, where would you recommend the best place to move this code would be?  (Feel free to discuss on IRC).

Lastly, for anyone reading this bug, after discussion on IRC, mconnor and mickey convinced me that modifying an event, so that it is an exception, is sufficient to distinguish it in the user's mind.  Therefore, my original question about not prompting for exceptions shouldn't be a concern.

Comment 5

[Michael Büttner (no reviews TFN)]

Attachment: patch v3

new iteration of the patch. we also found another problem in the previously supplied patch. if during the adjustment of the recurrenceIds we end up with two exceptions having the same id, removeExceptionFor() removes all of them (it's not always a good idea to use prebaked algorithms such as filter(), but i didn't touch that), effectively deleting the second. this can't happen any longer because the new iteration of the patch first collects all modifed exceptions and removing the existing ones as we go. the last step of the adjustment puts all exceptions back again into the list of exceptions.
i also didn't heard of 'folded' items before, it's just my mental model to differentiate between the semantic unit of an item as a whole and the single occurrences this item can be 'unfolded' into.

Comment 6

[Joey Minta]

Comment on attachment 213026 [details] [diff] [review]
patch v3

+    if (rec != null) {
This can just be |if (rec) {|. (same in a couple other places)

+        if(this.parentItem == this) {
Style, you know. :-)

+        if(this.parentItem == this) {
+            var rec = this.recurrenceInfo;
+            if (rec != null) {
+                rec.onStartDateChange(value,this.startDate);
+            }
+        }
A comment here explaining what you're doing and why you're doing it would be most helpful. (same in calTodo.js)

+        for each (var ex in modifiedExceptions) {
+            this.modifyException(ex);
+        }
The original declaration of var ex (in the previous loop), is still within this scope, so you need to rename this variable.

+        if (exceptions.some(function (exid) { return exid.compare(occurrence.recurrenceId) == 0; })) {
Anonymous functions make profiling/debugging difficult.  Also, be sure to wrap at 80chars, so you may need to break this out into a separate declaration.

+   * changing the starttime of the associated item invalidates the internal
+   * structure of the recurrenceinfo, since the backward connection from
+   * e.g. exceptions to an ocurrence would break if we wouldn't adjust it.
+   * currently this method will be called from the setter for DTSTART, no
+   * need to call this method manually after adjusting the starttime of an item.
The capitalization/grammer here isn't very good, making this comment very difficult to understand.

+        for (i in ritems) {
That looks like a strict warning for undeclared i.

+            if (ritem instanceof kCalIRecurrenceDate) {
+                ritem = ritem.QueryInterface(kCalIRecurrenceDate);
+                ritem.date.addDuration(timeDiff);
js> var rd = Components.classes["@mozilla.org/calendar/recurrence-date;1"].createInstance(Components.interfaces.calIRecurrenceDate);
js> rd.date
2006/02/25 02:48:27 UTC
js> rd.icalProperty.valueAsIcalString
20060225T024827Z
js> rd.date.month = 14
14
js> rd.icalProperty.valueAsIcalString
20061525T024827Z
js> rd.date.normalize()
js> rd.icalProperty.valueAsIcalString
20070325T024827Z

With all of these addDuration() calls, it seems you need to also normalize(), otherwise I think you'll end up with wacky ics files.

Almost there!  These are all pretty minor.  The idl-comment and the normalize() are the two biggest issues.

Comment 7

[Michael Büttner (no reviews TFN)]

Attachment: patch v4

new iteration as required.

Comment 8

[gekacheka]

I think the duplication shows startDate should be part of calIItemBase.
VEVENT, VTODO, VJOURNAL, and VFREEBUSY all have DTSTART.

Comment 9

[Joey Minta]

Comment on attachment 213306 [details] [diff] [review]
patch v4

-/* -*- Mode: javascript; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+    /* -*- Mode: javascript; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
That change doesn't really seem related.  Should probably be removed before checkin?

Awesome otherwise. r=jminta.  Holding off on landing until the final decision about recurrence is made.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Fix checked in, without the inadvertant modeline change.  Thanks for the patch!

Comment 11

[Damian Szczepanik]

I could reproduce it once but not any more, so

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