323477 - Cannot delete recurring event from multiweek/month view

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Calum Mackay]

Linux/x86 my own build from CVS; 20060113

I find that I cannot delete a weekly-recurring event from within the multiweek view. Neither the Del key, nor the entry from the event's context menu, nor the entry on the Edit menu, do anything at all. No errors on cmdline nor on JS console. I can delete the event in the Week view.

I can delete single non-recurring events in the multiweek view.

To delete a weekly recurring event, from within the multiweek view, I have to either disable the recurrence and then delete the resulting single event, or switch to a different view.

Comment 1

[Stefan Sitter [:ssitter]]

Did some testing:

In the Sunbird/0.3a1+ (win32-2006-01-12-07-trunk) build deleting of recurring events works (via Del-key or context menu). If I delete any event the whole event series is deleted (all views).

In the Sunbird/0.3a1+ (win32-2006-01-13-07-trunk) build I can delete single events out of event series using the Del-key in Day/Week view. Deleting via context menu does not work. In the Multiweek/Month view deleting does not work at all.

Problem applies to all recurring events, not only weekly-recurring events.

In the latest build (win32-2006-01-21-01-trunk) the problem still exist.

(Sometimes I was able to delete events in the Multiweek/Month view. But that happened only if Day/Week view was seelcted before. See Bug 320175 Comment #3 for this)

Checkins to mozilla/calendar between 2006-01-12 06:00 and 2006-01-13 08:00:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=CalendarClient&branch=HEAD&branchtype=match&dir=mozilla%2Fcalendar&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-01-12+06%3A00&maxdate=2006-01-13+08%3A00&cvsroot=%2Fcvsroot

Comment 2

[Joey Minta]

(In reply to comment #1)
> Did some testing:
> ...
Great work.  The selection changes seem the most suspicious.  I'd be curious what the selectionManager is doing when you select things, and whether it's getting called at all the proper times.  What event is being placed in the selectedEvents array? the parent? the occurrence? neither?

Comment 3

[Stefan Sitter [:ssitter]]

I added some dumps to see what happens. From what I see this is not a problem with the selection. Pressing Del-Key in Day View calls:

    calViewController.deleteOccurrence called
        calRecurrenceInfo.removeOccurrenceAt() called 
            with aRecurrenceId=2006/01/31 11:00:00 floating
        calRecurrenceInfo.removeOccurrenceAt() finished
    calViewController.deleteOccurrence finished

    calTransaction.doTransaction('delete') called
        StorageProvider.deleteItem() called
          calRecurrenceInfo.removeExceptionFor() called 
              with aRecurrenceId=2006/01/31 11:00:00 floating
          calRecurrenceInfo.removeExceptionFor() finished
        StorageProvider.deleteItem() finished
    calTransaction.doTransaction('delete') finished

Using Delete from menu or context menu in Day View just calls:

    calTransaction.doTransaction('delete') called
        StorageProvider.deleteItem() called
          calRecurrenceInfo.removeExceptionFor() called 
              with aRecurrenceId=2006/01/31 11:00:00 floating
          calRecurrenceInfo.removeExceptionFor() finished
        StorageProvider.deleteItem() finished
    calTransaction.doTransaction('delete') finished

So removeOccurrenceAt() makes it working in the first case. What is removeExceptionFor() suppossed to to?. That does not seem to work.

Comment 4

[Joey Minta]

removeExceptionFor is used for returning a previously listed exception to a recurring event back into the normal series of events.  I'm fairly certain that the call in the storage provider should be removeOccurrenceAt instead.

Comment 5

[Stefan Sitter [:ssitter]]

Just tested with the ics provider (using file:/// url): Deleting of recurring
event via Del-key and menu works in month view, but the whole series is deleted.

Seems there a big differences between storage and ics provider.

Comment 6

[Michael Büttner (no reviews TFN)]

investigating...

Comment 7

[Michael Büttner (no reviews TFN)]

the root cause of this issue is related to the focus-handling of the different views. each view listens for the del/backspace-key, but this event is obviously only received by the view which currently has the focus. since the focus-handling is not performed in a consistent way, we run into all sorts of incorrect behaviors. to make things even worse, each view attachs itself as observer to the compositeCalendar, which is why the behavior is different depending on the order in which the views were selected.

any suggestions how to perform a consistent focus-handling? would it be enough for the month-view to just grab the focus if it is activated? currently it never gets the focus, which is why it doesn't work to delete events in the month-view.

besides the problem with the focus-handling there is still another problem with recurring events. if we delete for example the second occurrence, and afterwards the third occurence, the second one suddenly reappears. this is a seperate issue i'm still looking for.

patch in progress...

Comment 8

[Michael Büttner (no reviews TFN)]

Attachment: patch for this task

the cause of this issue was indeed related to the focus-handling of the views, but at the end of the day it was just a missing css-style. the month-view never got the focus, which is why the del-key didn't work...

Comment 9

[Simon Paquet [:sipaq]]

Comment on attachment 209837 [details] [diff] [review]
patch for this task

Michael, please see to it that you always request review from a person. Otherwise the review request might fall through the cracks.

Comment 10

[Stefan Sitter [:ssitter]]

Comment on attachment 209837 [details] [diff] [review]
patch for this task

After a quick test: This patch seems to fix Bug 320175. But it is still not possible to delete recurring events via toolbar or context menu.

Comment 11

[Michael Büttner (no reviews TFN)]

(In reply to comment #9)
aeh, I thought I did exactly that? Isn't it correct to enter the mail-address of the person I request for the review? sorry for the dumb question, this is my first patch...

Comment 12

[Michael Büttner (no reviews TFN)]

(In reply to comment #10)
> (From update of attachment 209837 [details] [diff] [review] [edit])
> After a quick test: This patch seems to fix Bug 320175. But it is still not
> possible to delete recurring events via toolbar or context menu.

tried it with lightning only, stupid me ;-) I'll go for sunbird, too...

Comment 13

[Joey Minta]

Comment on attachment 209837 [details] [diff] [review]
patch for this task

OK, so I'm a bit confused given the last few comments, especially the one by ssitter and the fact that I now have 2 requests to review the same patch on 2 different bugs.

As I recall, there were some problems identified with the storage provider in deleting instances of recurring events.  Michael, can you please clarify
(1) What bug(s) this patch is designed to fix.
(2) What work still remains to be done on this bug and others if this patch is checked in.

That will make reviewing this much easier.  Thanks.

Comment 14

[Michael Büttner (no reviews TFN)]

(In reply to comment #13)

> As I recall, there were some problems identified with the storage provider in
> deleting instances of recurring events.  Michael, can you please clarify
> (1) What bug(s) this patch is designed to fix.
> (2) What work still remains to be done on this bug and others if this patch is
> checked in.

sorry for the confusion ;-)

1) the submitted patch resolves the problem with deleting events from the month-view (in lightning as well as in sunbird) with the del-key. it doesn't matter if the event has a recurrence rule or not, the problem is that the month-view never got the focus, therefore the appropriate handler was never called.

2) regarding the remaining work, the patch fixes the problem described in bug #320175 (which ssitter pointed me to after i already submitted the patch to bug #323477). bug #320175 could be set to resolved after integration of the patch. for bug #323477 the remaining work is to delete recurring events via toolbar or context menu in sunbird, this needs to be resolved by some other patch i'm currently working on.

regarding the problems with the storage provider not correctly deleting instances of recurring events, i'm currently investigating what's going on there, but it is not related to this patch.

Comment 15

[Joey Minta]

Comment on attachment 209837 [details] [diff] [review]
patch for this task

Cancelling request.  This patch went in on bug 320175.

Comment 16

[Michael Büttner (no reviews TFN)]

Attachment: proposed patch

this patch will now fix the right bug ;-)

the problem was that 'deleteItems' in calendar.js did not handle recurring events at all, i added the appropriate codepath. please note that in case of a recurring event i'm passing the proxy of the occurrence to 'doTransaction', which is consitent with how lightning handles this.

Comment 17

[Michael Büttner (no reviews TFN)]

Attachment: reworked patch

changed the last patch to reflect the result of our discussion regarding bug #325215

Comment 18

[Joey Minta]

Comment on attachment 211127 [details] [diff] [review]
reworked patch

r=jminta Thanks for the patch!

Comment 19

[Joey Minta]

patch checked in.