The default bug view has changed. See this FAQ.

'All occurrences' vs. 'This occurrence' dialog should be cancelable

VERIFIED FIXED

Status

Calendar
Internal Components
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Stefan Sitter, Assigned: Robin Edrenius)

Tracking

({dataloss})

Trunk
dataloss

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

11 years ago
'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.
(Assignee)

Comment 1

11 years ago
I'll make a try at this.

Assigning to myself
Assignee: base → robin.edrenius
(Assignee)

Comment 2

11 years ago
Created attachment 214355 [details] [diff] [review]
Patch v1

This patch makes the dialog cancelable.
Attachment #214355 - Flags: first-review?(dmose)

Comment 3

11 years ago
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
(Assignee)

Comment 4

11 years ago
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)
Attachment #214355 - Attachment is obsolete: true
Attachment #214367 - Flags: first-review?(dmose)
Attachment #214355 - Flags: first-review?(dmose)

Comment 5

11 years ago
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

11 years ago
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;
  }
)
(Assignee)

Comment 7

11 years ago
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.
Attachment #214367 - Attachment is obsolete: true
Attachment #214434 - Flags: first-review?(dmose)
Attachment #214367 - Flags: first-review?(dmose)
(Reporter)

Comment 8

11 years ago
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.
(Assignee)

Comment 9

11 years ago
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!)
Attachment #214434 - Attachment is obsolete: true
Attachment #214446 - Flags: first-review?(dmose)
Attachment #214434 - Flags: first-review?(dmose)

Updated

11 years ago
Whiteboard: [cal relnote]

Comment 10

11 years ago
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

11 years ago
(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

11 years ago
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

11 years ago
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.
(Reporter)

Comment 15

11 years ago
(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!
Attachment #214446 - Flags: first-review?(dmose) → first-review-
(Assignee)

Comment 18

11 years ago
Created attachment 220489 [details]
Screenshot showing cancel button
(Assignee)

Comment 19

11 years ago
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.
Attachment #214446 - Attachment is obsolete: true
Attachment #220490 - Flags: first-review?(dmose)
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.
Attachment #220490 - Flags: first-review?(dmose) → first-review+
(Assignee)

Comment 21

11 years ago
Created attachment 220547 [details] [diff] [review]
patch v7

The nits dmose pointed out is addressed
Attachment #220490 - Attachment is obsolete: true
Attachment #220547 - Flags: first-review+
(Assignee)

Comment 22

11 years ago
Bug 336300 submitted.
Whiteboard: [cal relnote] → [needs landing]
patch checked in on trunk and MOZILLA_1_8_BRANCH
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
(Reporter)

Comment 24

11 years ago
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+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

11 years ago
Keywords: dataloss

Comment 25

11 years ago
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.
Attachment #225599 - Flags: first-review?(mvl)

Updated

11 years ago
Blocks: 339487
Comment on attachment 225599 [details] [diff] [review]
remove global delete

r=mvl
Attachment #225599 - Flags: first-review?(mvl) → first-review+

Comment 27

11 years ago
Patch checked in.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Reporter)

Comment 28

11 years ago
Verified with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1)
Gecko/20060630 Sunbird/0.3a2+.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.