Last Comment Bug 329642 - 'All occurrences' vs. 'This occurrence' dialog should be cancelable
: 'All occurrences' vs. 'This occurrence' dialog should be cancelable
Status: VERIFIED FIXED
: dataloss
Product: Calendar
Classification: Client Software
Component: Internal Components (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Robin Edrenius
:
Mentors:
Depends on:
Blocks: 339487
  Show dependency treegraph
 
Reported: 2006-03-07 10:14 PST by Stefan Sitter
Modified: 2006-06-30 13:02 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 (2.31 KB, patch)
2006-03-07 13:18 PST, Robin Edrenius
no flags Details | Diff | Review
Patch v3 (7.39 KB, patch)
2006-03-07 15:25 PST, Robin Edrenius
no flags Details | Diff | Review
Patch v4 (7.42 KB, patch)
2006-03-08 08:11 PST, Robin Edrenius
no flags Details | Diff | Review
Patch v5 (7.43 KB, patch)
2006-03-08 09:43 PST, Robin Edrenius
dmose: first‑review-
Details | Diff | Review
Screenshot showing cancel button (58.89 KB, image/png)
2006-05-01 23:37 PDT, Robin Edrenius
no flags Details
Patch v6 (8.62 KB, patch)
2006-05-01 23:43 PDT, Robin Edrenius
dmose: first‑review+
Details | Diff | Review
patch v7 (8.69 KB, patch)
2006-05-02 13:15 PDT, Robin Edrenius
robin.edrenius: first‑review+
Details | Diff | Review
remove global delete (7.24 KB, patch)
2006-06-14 11:22 PDT, Joey Minta
mvl: first‑review+
Details | Diff | Review

Description Stefan Sitter 2006-03-07 10:14:23 PST
'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.
Comment 1 Robin Edrenius 2006-03-07 13:09:45 PST
I'll make a try at this.

Assigning to myself
Comment 2 Robin Edrenius 2006-03-07 13:18:48 PST
Created attachment 214355 [details] [diff] [review]
Patch v1

This patch makes the dialog cancelable.
Comment 3 Joey Minta 2006-03-07 14:12:36 PST
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
Comment 4 Robin Edrenius 2006-03-07 15:25:33 PST
Created attachment 214367 [details] [diff] [review]
Patch v3

(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)
Comment 5 gekacheka 2006-03-07 16:49:08 PST
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
Comment 6 gekacheka 2006-03-08 05:16:24 PST
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;
  }
)
Comment 7 Robin Edrenius 2006-03-08 08:11:24 PST
Created attachment 214434 [details] [diff] [review]
Patch v4

(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 8 Stefan Sitter 2006-03-08 09:26:53 PST
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.
Comment 9 Robin Edrenius 2006-03-08 09:43:24 PST
Created attachment 214446 [details] [diff] [review]
Patch v5

> 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!)
Comment 10 Andrew N Dowden 2006-04-02 14:38:42 PDT
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?
Comment 11 Joey Minta 2006-04-02 14:42:55 PDT
(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.
Comment 12 Andrew N Dowden 2006-04-02 16:07:23 PDT
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).
Comment 13 Jason Brewer 2006-04-11 18:34:53 PDT
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.
Comment 14 Martin Schröder [:mschroeder] 2006-04-12 04:22:36 PDT
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.
Comment 15 Stefan Sitter 2006-04-12 04:36:49 PDT
(In reply to comment #13 and #14)
Use the Delete keyboard button in Sunbird istead of the toolbar or menu command.
See Bug 332266.
Comment 16 Dan Mosedale (:dmose) 2006-04-27 17:35:17 PDT
The behavior described in comments 10 through 12 should be taken to a different bug, if someone wishes to pursue it further.
Comment 17 Dan Mosedale (:dmose) 2006-05-01 18:16:11 PDT
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!
Comment 18 Robin Edrenius 2006-05-01 23:37:24 PDT
Created attachment 220489 [details]
Screenshot showing cancel button
Comment 19 Robin Edrenius 2006-05-01 23:43:12 PDT
Created attachment 220490 [details] [diff] [review]
Patch v6

(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 20 Dan Mosedale (:dmose) 2006-05-02 12:50:20 PDT
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.
Comment 21 Robin Edrenius 2006-05-02 13:15:30 PDT
Created attachment 220547 [details] [diff] [review]
patch v7

The nits dmose pointed out is addressed
Comment 22 Robin Edrenius 2006-05-02 13:17:23 PDT
Bug 336300 submitted.
Comment 23 Matthew (lilmatt) Willis 2006-05-14 19:57:07 PDT
patch checked in on trunk and MOZILLA_1_8_BRANCH
Comment 24 Stefan Sitter 2006-05-16 13:15:14 PDT
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+
Comment 25 Joey Minta 2006-06-14 11:22:22 PDT
Created attachment 225599 [details] [diff] [review]
remove global delete

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 26 Michiel van Leeuwen (email: mvl+moz@) 2006-06-17 05:03:42 PDT
Comment on attachment 225599 [details] [diff] [review]
remove global delete

r=mvl
Comment 27 Joey Minta 2006-06-19 10:04:16 PDT
Patch checked in.
Comment 28 Stefan Sitter 2006-06-30 13:02:01 PDT
Verified with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1)
Gecko/20060630 Sunbird/0.3a2+.

Note You need to log in before you can comment on or make changes to this bug.