Closed Bug 325477 Opened 14 years ago Closed 14 years ago

Undo/Redo seems partially broken in new views

Categories

(Calendar :: Sunbird Only, defect)

x86
Windows 2000
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: ssitter, Assigned: jminta)

References

Details

Attachments

(2 files)

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+
See also bug 322386.  I think we need undo/redo in lightning first.
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.
Depends on: 322386
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.
Attachment #216806 - Flags: first-review?(mvl)
Comment on attachment 216806 [details] [diff] [review]
part 1: switch to doTransaction everywhere

r=mvl
Attachment #216806 - Flags: first-review?(mvl) → first-review+
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.
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]"]
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
(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.
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.
Assignee: nobody → jminta
Status: NEW → ASSIGNED
Attachment #216924 - Flags: first-review?(mvl)
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?
(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?
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 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
Attachment #216924 - Flags: first-review?(mvl) → first-review+
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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
No longer depends on: 322386
verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060909 Calendar/0.3a2+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.