325477 - Undo/Redo seems partially broken in new views

Status: VERIFIED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Stefan Sitter [:ssitter]]

Undo/Redo seems partially broken in new views.

For example:
Undo (Ctrl+Z) after moving an event via drag'n'drop works.
Undo (Ctrl+Z) after deleting an event via Del-key works.
Undo (Ctrl+Z) after cut and paste of an event does nothing.
Undo (menu)   after cut and paste results in two events, the old and new one.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060130 Mozilla Sunbird/0.3a1+

Comment 1

[Joey Minta]

See also bug 322386.  I think we need undo/redo in lightning first.

Comment 2

[Joey Minta]

Actually, this seems to me to be a problem with 'add' transactions, of which Paste is one example.  I can't undo any 'add' transactions at all.

Comment 3

[Joey Minta]

Attachment: part 1: switch to doTransaction everywhere

This patch doesn't finish the bug, but gets things into a state where further debugging/testing can occur.  It moves all of the modification code into doTransaction, so that undo/redo has access to it.  This means that Lightning now gets an undo/redo backend.  (There's an open bug on giving it undo/redo that can be used to figure out the UI details there.)  Unfortunately, I'm having a problem with many instances of undo/redo that come through the view-controller.  I'd like to get this patch in so that others can help me track that down.

Comment 4

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

Comment on attachment 216806 [details] [diff] [review]
part 1: switch to doTransaction everywhere

r=mvl

Comment 5

[Joey Minta]

Part 1 checked in.  This mean's that Sunbird's undo/redo system should *try* to work for all changes except inline editing of titles.  If testers could please put the code through its paces and report any failures, i'd appreciate it.

Comment 6

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

When trying to undo an addition, I get this error:
[JavaScript Error: "uncaught exception: [Exception... "'Failure' when calling method: [calICompositeCalendar::deleteItem]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://calendar/content/calendar-item-editing.js :: anonymous :: line 300"  data: no]"]

Comment 7

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

I guess the problem is that we are trying to delete the original item, the item that was created. But that isn't the item after it was added: it has no parent yet.
Without a parent, the composite can't delete.
http://lxr.mozilla.org/seamonkey/source/calendar/providers/composite/calCompositeCalendar.js#323

Comment 8

[Joey Minta]

(In reply to comment #7)
> I guess the problem is that we are trying to delete the original item, the item
> that was created. But that isn't the item after it was added: it has no parent
> yet.
> Without a parent, the composite can't delete.
> http://lxr.mozilla.org/seamonkey/source/calendar/providers/composite/calCompositeCalendar.js#323
> 
Bigger problem, the added item doesn't have an id!  This may essentially be a duplicate of bug 291220 at this point.

Comment 9

[Joey Minta]

Attachment: part 2: grab the returned item and save it

Patch adds a listener so we can store the proper returned event, and use it when we undo.  I *think* this solves the rest of the undo/redo problems.

Comment 10

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

Comment on attachment 216924 [details] [diff] [review]
part 2: grab the returned item and save it

This patch leaves room for race conditions: when you have a slowish calendar (caldav over a slow link) it might take some time for the new item to return. When trying to undo in that timeframe, things will get lost. Is that a problem we want to care about?

Comment 11

[Joey Minta]

(In reply to comment #10)
> (From update of attachment 216924 [details] [diff] [review] [edit])
> This patch leaves room for race conditions: when you have a slowish calendar
> (caldav over a slow link) it might take some time for the new item to return.
> When trying to undo in that timeframe, things will get lost. Is that a problem
> we want to care about?
> 
It's probably something we care about, but I'm not sure it's something that can be fixed in a low-risk enough way for 0.3a2.  Unless you see a solution that I don't?

Comment 12

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

There is a way around it: queue the undo-actions until the new item is available. But that will add a lot of code, and won't be easy to do.
I guess we have to live with this edge case (very quick undo on a slow caldav server) for 0.3a2.

Comment 13

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

Comment on attachment 216924 [details] [diff] [review]
part 2: grab the returned item and save it

For now, this will do. But please file a bug on the edge-case.
r=mvl

Comment 14

[Joey Minta]

Patch checked in and bug 332754 filed.  I don't think there are any additional issues, so I'm closing the bug.  Use your best judgment for any additional problems whether they should be here or in another bug.

Comment 15

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060909 Calendar/0.3a2+