349354 - Editing recurring event changes single occurrence edited before

Status: VERIFIED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Gary Sanders]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060507 Mozilla Sunbird/0.3a2

If I setup a calendar entry that repeats for 7 days, For example a 7 day vacation.
Then I go into  wednesday to update the entry, I am ask if I want to change all occurances or this occurance. If I make a change a single occurance  "wednesday". I later go back and try to update friday and select all occurances, All my updates for wednesday are reset. There should be some warning that data will be changed. 

Should you allow all occurances if one has already been changed? Should there be some other option to allow, disallow or warn that all will be changed


Reproducible: Always

Steps to Reproduce:
1.create re-occuring date.
2. edit 1 date but selct this occurances.
3. edit another but select all occurances.

Actual Results:  
all date for that calendar entry are reset.

Expected Results:  
some warning that another date has been  changed.

Comment 1

[Martin Schröder [:mschroeder]]

After editing the single occurrence, it is internally still member of the recurring event. So the real problem is how this situation should be ideally handled (not only a warning dialog) and if such an occurrence still belongs to the recurrence. A possible solution for the UI could be: only fields that are equal for all occurrences should be editable.

Comment 2

[Joey Minta]

This shouldn't happen.  Changes to occurrences ought to be saved and not discarded when the normal pattern is edited.

There's a bug in here somewhere that's causing us to hold a stale copy of the parent item.  Can you go into exact detail about where the events were displayed, where you clicked on them, and how you opened the edit dialog, etc. to help pin down where that stale copy is getting stuck?

Comment 3

[Joey Minta]

My guess is that this is caused by stale selection, but we need better steps to reproduce to be sure.  If it is caused by stale selection, then it should be fixed by bug 321384.

Comment 4

[Bruno Browning]

I ran into this while working on bug 328576. Steps to reproduce:
1. Create recurring event, say at 18:00 every Wednesday in August. Set the Description to "each occurrence"
2. Doubleclick first instance, click "This occurrence only", set the Description to "first occurrence"
3. Doubleclick second instance, click "This occurrence only", set the Description to "second occurrence"
4. Doubleclick third instance, click "This occurrence only", set the date to the preceding day, a Tuesday
5. Doubleclick last instance, click "This occurrence only", set the Description to "last occurrence"
6. Exit/restart

Result: On restarting, only that last edit (step 5) will have "held": the other exceptions will have been lost. While Sunbird is running, all these edits hold: you only see the dataloss after a restart. Also, if you exit/restart after e.g. step 2, that first exception will "hold" into the next Sunbird session and will not be effected by this bug. In general, if an item has, at Sunbird startup, N exceptions, those exceptions will not be effected by this bug, but at the next start the item will have at most N+1 exceptions, with that last one representing the last one created.

This on Linux with the 2006-08-29 nightly, with both storage calendar and with CalDAV (using my slightly hacked calDavCalendar.js).

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

As Joey says, the bug here is that this can happen at all, so we shouldn't need to change the UI to accommodate this.

Comment 6

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

I can't reproduce this bug. Is it worksforme now, or is it just hard to reproduce?

Comment 7

[Bruno Browning]

I still see it with the current Linux nightly. I don't find it that hard to reproduce, but probably should not have cut off my procedure in comment #4. Should have continued:
7. Examine first instance, note that Description reads 'each occurrence' rather than 'first occurrence
8. Repeat for second and third instances, both of which will have the original Description field rather than the one set in steps 3 and 4.
9. Examine last instance, note that Description field reads 'last occurrence' as expected.

Step 6 is essential. During a single Sunbird session you won't see any evidence of this bug in the UI.

This occurs with the storage provider but *not* with the ICS provider. Interestingly, you can produce an almost identical bug in the ICS provider by commenting out line #270 of calMemoryCalendar.js

Comment 8

[Sebastian Schwieger]

I think I found steps to reproduce this:

DO EVERYTHING IN MONTH VIEW OR MULTIWEEKVIEW.

1. create recurring event (daily for 5 times)
2. change time of only first occurrence
3. change time of all occurrances by clicking on second occurrance

Result: first occurrance is changed as well.
The same happens if you change titles instead of times.

This is not reproducible in week view! 

Done using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060908 Calendar/0.3a2+

Comment 9

[Stefan Sitter [:ssitter]]

Maybe this is the same underlying problem as in Bug 345881?
See Bug 345881 Comment #3.

Comment 10

[Bruno Browning]

That seems likely. No matter how many exceptions you add to an item, modifyItem is called with an aNewItem whose .parentItem.recurrenceInfo has only one exception (with an exceptionId of the first exception created). 

The event as stored in mItemCache is correct, and it's possible to alter modifyItem to use that when doing modifyException(aNewItem). Doing so "fixes" this bug in both its "edit-all after creating exception nukes exception" and "cannot create more than one exception" aspects. But the proper fix is to make sure modifyItem is called with correct args.

This on storage calendar, current Linux nightly.

Comment 11

[Michael Büttner (no reviews TFN)]

This is obviously another evidence that the cloning of events results in dangling references. I wasn't able to reproduce this bug in the week view but it happened instantly in the month view, current windows nightly. I'll take this one, patch in progress.

Comment 12

[Michael Büttner (no reviews TFN)]

this is exactly the same problem we had before the 0.1 release with dangling references in the views. the observer gets called through onModifyItem(aNewItem,aOldItem). in case an exception was generated the 'new item' refers to the exception itself, specifically to the occurrence. the 'old item' refers to the parent item, without the exception. the view iterates both sets, the old item as well as the new item. for the set of 'aOldItem' each item is deleted, for the set of 'aNewItem' each item is added to the view. obviously this leads to dangling references since just the modified occurrence gets deleted, while all the other items are kept in their previous state. if the user continues to modify other items we're internally using the item as if the exception was never created.

the whole problem exists because the core clone()'s the items, which leads to the above described problem. all of this has already been evaluted after the 0.1 lightning release, see http://wiki.mozilla.org/Calendar:Architecture for further details on this topic.

without changing the existing architecture, there are basically two possible approaches to eliminate this problem (within the scope of the views, that is).

a) in onModifyItem() see if the 'old item' is an occurrence. if this is the case  use its parentItem instead of the proxy which has been passed to the method. this removes all occurrences of the series and adds them back to the views during iteration of the 'new item', which is (by convention) always the parentItem.

b) don't change anything in onModifyItem(), but make sure that while iterating the 'new item' we delete the previous incarnation of the occurrences the view holds.

from a performance point of view both options basically are the same, i personally feel that option a) is a bit cleaner. but in my opinion the real fix would be to not clone() the items in the first place, which would also be a wise decision in terms of performance (we'd avoid the costly and unnecessary delete/add/relayout stuff in the views). the week view implements option b) while the month view just filters known items in addItem() out and therefore suffers from the dangling reference problem.

Comment 13

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

see my previous comment on this issue. this patch implements option b) for the month view.

Comment 14

[Michael Büttner (no reviews TFN)]

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

thomas is sick, won't be in town for some days -> removed review request.

Comment 15

[cmtalbert]

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

This looks fine to me. Dan asked me to do a first review on this bug, and then he'd do a second. So, I've switched around the reviewing.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

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

r=dmose.  If I recall correctly from Toronto, the plan was that someone was going to try and figure out the most minimally invasive way to switch to that architecture and try and characterize how disruptive that would be.  Did that ever happen?

Comment 17

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRACH and trunk.

-> FIXED

(but dmose's question still stands)

Comment 18

[Michael Büttner (no reviews TFN)]

(In reply to comment #16)
> If I recall correctly from Toronto, the plan was that someone was
> going to try and figure out the most minimally invasive way to switch to that
> architecture and try and characterize how disruptive that would be.  Did that
> ever happen?
You're right, that did not happen until now. I'll do this within the next weeks.