328510 - Deleting an exception doesn't really delete, but only adds an exdate

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

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

When you delete an exception from a recurring set, the exception is still in memory, and will be serialized to an ics file. The item disapears, because an EXDATE is added.

steps to reproduce:
1. create a recurring event
2. change the starttime of on if the items in the set
3. delete that item
4. look at the ics file

Comment 1

[Michael Büttner (no reviews TFN)]

one could argue that this behavior could serve the purpose to restore a previously deleted exception, therefore we should postpone this discussion and remove this bug from the 0.1 blocker list.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #1)
> one could argue that this behavior could serve the purpose to restore a
> previously deleted exception, therefore we should postpone this discussion and
> remove this bug from the 0.1 blocker list.

I don't understand this.  Can you explain what you mean in more detail?

Comment 3

[Michael Büttner (no reviews TFN)]

sorry for not making the above statement clearer. generally an exception is an exception from the rule, and an EXDATE means that some specific occurrence should not occur. there's nothing wrong with deleting an exception by adding an EXDATE, since it's the regular behavior of how seperate occurrences are deleted. it's even possible to restore an exception by deleting the EXDATE. of course it's a waste of precious resources to keep the exception instead of really delete it. but it should be clear that if we allow to remove previously created EXDATES the deleted occurrence would be restored at its original timeslot (assuming you created the exception by assigning a new DTSTART to it), since the information would be lost in that case.
what i basically want to say is, there are no hard rules of how this has to be done and the way it's currently solved is not wrong, but we could decide that we would like to handle such cases in a different way. and i don't think this discussion needs to happen before the 0.1 lightning release and should therefore be removed from the blocker list.

Comment 4

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

For other deletions, the items are really deleted from the datafile. There is an undo machanism (at least in sunbird, and lightning should get one too) that allows to undo those actions.
Deleting an exception should be treated the same. Delete the information from the data store, and use nsITransactionManager for undo. No need to create a new mechanism.
So just for consistency, the exception should really be deleted.

Also, some people might consider this a privacy bug. You deleted something, but it is still in the file. And I agree with that. Deleting should delete, nog just make it invisible.

Comment 5

[Michael Büttner (no reviews TFN)]

(In reply to comment #4)

agreed, you're totally right, i just wanted to point out that i wouldn't rate this behavior as critical and therefore suggested to remove it from the blocker list. i didn't want to use this as a replacement for undo/redo which definitely needs to be handled by some authorative instance. so, what's the conclusion, fix it before or after the 0.1 release? dan, what's your opinion?

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

I agree that this isn't particularly serious.  One thing Joey pointed out is that if we don't fix this bug for 0.1, then we'll need to try a little harder to cope with calendars that have this sort of structure (both ICS and storage) in the future so that we don't cause unnecessary pain for users of 0.1.

Now if we think that there are likely to be ICS files floating around in the wild with this structure anyway, then we'll need that code regardless.  It'd be interesting to know if there are already generators of that sort of ICS file (especially Sunbird 0.2/0.3a1, but also other vendors).  It would also be helpful to know how hard it would be to migrate old files to the more canonical "actually get rid of the exceptions".  Without really knowing that code myself, it seems like it shouldn't be hard.

Depending on how hard the fix is, it might actually be easier to patch this than to try and figure out the answers to the questions above.  

Thoughts?

Comment 7

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

this patch removes those wacky exceptions. please note that this triggers a maybe undesired behavior with exception handling. assume the following list of actions:

1) create a new recurring event (say 5 days)
2) move one occurrence to some arbitrary startdate
3) delete this modified occurrence
4) open the dialog and remove the listed exception
5) the deleted occurrence will appear at its original (unmodified) startdate.

this could easily be treated as a bug, though it's absolutely correct what happens under the hood with the supplied patch in place. we maybe should think about not allowing to restore exceptions in order to get around this problem.

any opinions?

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

The scenario you describe is sounds like the program is would be doing exactly what the user requested of it.  That said, I agree that the user might not understand exactly what they'd requested.  But this seems to me to be a relatively unusual scenario, so I wouldn't object to taking this fix and filing another bug on figuring out how to make the UI more clear in the future.

Comment 9

[Michael Büttner (no reviews TFN)]

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

requesting review.

Comment 10

[Michael Büttner (no reviews TFN)]

(In reply to comment #8)
> so I wouldn't object to taking this fix and filing
> another bug on figuring out how to make the UI more clear in the future.

see https://bugzilla.mozilla.org/show_bug.cgi?id=324657 for a new proposal. there's also a new version of this proposal on its way which should be submitted until end of this week.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

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

r=dmose

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Fix checked in; thanks for the patch, Michael!