'All occurrences' vs. 'This occurrence' dialog should be cancelable If you try to delete or edit an recurring event the 'All occurrences' vs. 'This occurrence' dialog is displayed. There is no way to cancel the dialog respectively the scheduled action. Pressing Escape-key or hiting the X-icon is interpreted as a selection of 'This occurrence' and scheduled action is performed. History: I just hit the delete key accidentally. But then there was no way to cancel this delete action. Because Lightning does not have a undo function the event is gone. (I know that you can undo this by editing 'All occurrences', go to Repeat dialog and remove the exception. But that is a very long winded procedure for average user) Expected behaviour: I can cancel the dialog and the scheduled action is not performed.

I'll make a try at this. Assigning to myself

This patch makes the dialog cancelable.

Comment on attachment 214355 [details] [diff] [review] Patch v1 It seems to me that there are several other areas of the code that are expecting something to be returned here. I think we may need to do some additional null-checking in the callers of getOccurrenceOrParent() http://lxr.mozilla.org/mozilla/search?string=getOccurrenceOrParent

(In reply to comment #3) > (From update of attachment 214355 [details] [diff] [review] [edit]) > It seems to me that there are several other areas of the code that are > expecting something to be returned here. I think we may need to do some > additional null-checking in the callers of getOccurrenceOrParent() > > http://lxr.mozilla.org/mozilla/search?string=getOccurrenceOrParent > Some null-checks is added. Bare in mind that I'm not really sure about how to do that properly. (Fixed some indenting in the code I touched as well)

This part calls dialog twice? (in both editEvent and editToDo) https://bugzilla.mozilla.org/attachment.cgi?id=214367&action=diff#mozilla/calendar/resources/content/calendar.js_sec1

Also, getOccurrenceOrParent should return null if cancelled https://bugzilla.mozilla.org/attachment.cgi?id=214367&action=diff#mozilla/calendar/base/content/calendar-item-editing.js_sec2 (avoids "function doesn't always return a value" warning when preference javascript.options.strict is true) (optional: A switch might be a little more concise: switch(choice) { case 0: return occurrence.parentItem; case 2: return occurrence; default: return null; } )

(In reply to comment #6) > (optional: A switch might be a little more concise: > switch(choice) { > case 0: return occurrence.parentItem; > case 2: return occurrence; > default: return null; > } > ) > Thanks for your comments! Changes made to reflect this.

Comment on attachment 214434 [details] [diff] [review] Patch v4 > function editEvent() >+ if (!modifyEventWithDialog(getOccurrenceOrParent(calendarEvent))) >+ { >+ return; >+ } >+ modifyEventWithDialog(getOccurrenceOrParent(calendarEvent)); This looks curious. I think it would be better to do the null check like: var aItem = getOccurrenceOrParent(calendarEvent); if (aItem) { modifyEventWithDialog(aItem); } Otherwise you might end up calling modifyEventWithDialog twice. > function editToDo(task) { >+ if (!modifyEventWithDialog(getOccurrenceOrParent(task))) >+ { >+ return; >+ } > modifyEventWithDialog(getOccurrenceOrParent(task)); Same here.

> This looks curious. I think it would be better to do the null check like: > > var aItem = getOccurrenceOrParent(calendarEvent); > if (aItem) { > modifyEventWithDialog(aItem); > } > > Otherwise you might end up calling modifyEventWithDialog twice. Thanks for your input! Here's a new iteration of the patch. (btw, I just realized that I skipped a version 2 of the patch, doh!)

With recent build (03-30), if you try edit an event you do NOT always get prompted with the 'All occurrences' vs. 'This occurence' dialog. If you edit and event, and choose 'This occurrence', then re-editing this event will sometimes (can NOT find exact pattern) just open this event only. Re-trying this with 3-4 events, as All and This occurrence', did not produce reproducable pattern. Is this a NEW bug? Is this caused by recent changes to code?

(In reply to comment #10) > With recent build (03-30), if you try edit an event you do NOT always get > prompted with the 'All occurrences' vs. 'This occurence' dialog. That should have been fixed yesterday I think. > > If you edit and event, and choose 'This occurrence', then re-editing this event > will sometimes (can NOT find exact pattern) just open this event only. > > Re-trying this with 3-4 events, as All and This occurrence', did not produce > reproducable pattern. > > Is this a NEW bug? Is this caused by recent changes to code? > My guess for what is happening here: When you modify a single 'this occurrence event, you're effectively pulling the event out of the series. Since it's independent, editing this single event is your only option. This is by design, although over my objections as I felt it would confuse users just like this. The UE guys told me otherwise.

I tested this again with overnight (04-02). No change. Your logic seems correct, it is where the event is 'different' than ALL. But that doesn't make total sense. You are able to change: Category, Privacy, Status, and duration (can change end-date). Changing duration/end-date should force 'EXDATE, RDATE' additions pairs to the equivalent iCal data. (for full functionality).

The current trunk provides no choice to delete "all occurrences" of a recurring event. Deleting a recurring event simply adds an exception to the VEVENT code. While it's nice to be able so easily add an exception, I would like to be able to delete an entire recurring event without using a text editor. This may merit a new bug report, but I thought this one was very relevant.

The 'All occurences' vs. 'This occurence' dialog is not showed when using the delete button on toolbar or the 'Delete selected event' context menu item. I think you can file a new bug report because I couldn't find an existing one for this problem.

(In reply to comment #13 and #14) Use the Delete keyboard button in Sunbird istead of the toolbar or menu command. See Bug 332266.

The behavior described in comments 10 through 12 should be taken to a different bug, if someone wishes to pursue it further.

Comment on attachment 214446 [details] [diff] [review] Patch v5 Apologies for the review delay. In general, this looks good, just some minor stuff to be addressed: * Be sure to add your name and email address to the license boilerplate of any file you touch, if it's not already there. * I notice that unifinder doesn't use getOccurrenceOrParent() for deleting items. Should it? * Why do the null-checks in editEvent and editTodo? They just call modifyEventWithDialog, which you've changed to already do one. Also, can you post a screenshot of the modified dialog? Thanks!

(In reply to comment #17) > * Be sure to add your name and email address to the license boilerplate of any > file you touch, if it's not already there. > Done. > * I notice that unifinder doesn't use getOccurrenceOrParent() for deleting > items. Should it? > I'll leave that decision for someone else =) > * Why do the null-checks in editEvent and editTodo? They just call > modifyEventWithDialog, which you've changed to already do one. > Removed them. > Also, can you post a screenshot of the modified dialog? Thanks! > Done.

Comment on attachment 220490 [details] [diff] [review] Patch v6 Looks good. I noticed a couple of tiny nits that need fixing, so if you could upload a new version of the patch, then you can carry the r+ forward and add [needs landing] to the status whiteboard. Also, could you file a new bug to decide what to do about the unifinder and recurrence? Thanks! >@@ -144,17 +145,19 @@ function modifyEventWithDialog(item) > if (!originalItem.calendar || > (originalItem.calendar.uri.equals(calendar.uri))) > doTransaction('modify', item, item.calendar, originalItem, null); > else { > doTransaction('move', item, calendar, originalItem, null); > } > } > >+ if (item) { > openEventDialog(item, item.calendar, "modify", onModifyItem); >+ } > } The openEventDialog line needs to be indented. >+ var choice = promptService.confirmEx(null, promptTitle, promptMessage, flags, >+ buttonLabel1,null , buttonLabel2, null, {}) The above line should have a ; after it.

The nits dmose pointed out is addressed

Bug 336300 submitted.

patch checked in on trunk and MOZILLA_1_8_BRANCH

This fix seems to be incomplete: 1. Create recurring event in Sunbird 2. Select one of the events, press Delete key on keyboard 3. In the 'All' vs. 'This' dialog press cancel --> event is deleted anyway, not as expected Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060516 Mozilla Sunbird/0.3a2+

We currently have a global listener for the delete key, that always applies to the selected events. This feels fairly wrong, since if focus is in, for example, the calendar-list-box, pressing delete shouldn't delete events. This patch removes the global delete key command. The bug: The views currently handle key-events on their own. The cancel part of the dialog works fine there. The problem was that the global delete key was then also being triggered, so we also deleted the event. Simply removing the global listener fixes the bug, but this would regress the ability to delete while the unifinder has focus. Therefore, I've also added a keypress listener to the unifinder.

Comment on attachment 225599 [details] [diff] [review] remove global delete r=mvl

Patch checked in.

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060630 Sunbird/0.3a2+.