User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20030925 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20030925 I'd like to be able to move events and tasks from one calendar file to another, because I use multiple Calendar Files for purpose of grouping items. Reproducible: Always Steps to Reproduce: 1. Enter a new event or task. Calendar File can be chosen. 2. Modify event or task. Actual Results: "Calendar File" combo box is disabled. Expected Results: It would be nice that Mozilla Calendar would enable users to move items from one file to another. This entry has some corellation to my entry #220652, but as it is not just the same, I decided to add a separate entry.
*** Bug 259472 has been marked as a duplicate of this bug. ***
*** Bug 298598 has been marked as a duplicate of this bug. ***
Making this bug a blocker for 0.3a1. Currently, for me, if I switch the calendar an event belongs to in the edit-event dialog, then hit 'OK', the event just disappears. I'm not sure where exactly this should be fixed ('modify' command? event-dialog?), but losing events is definitely not cool. Worst case, we need to disable the menu-option again.
Created attachment 190907 [details] [diff] [review] switch action order This was easy once I looked at it more closely. Due to the fact that the event still has the same ID, you can't add it before you delete it without problems. Deleting first makes it work.
Comment on attachment 190907 [details] [diff] [review] switch action order r=mvl
patch checked in.
*** Bug 291243 has been marked as a duplicate of this bug. ***
*** Bug 209687 has been marked as a duplicate of this bug. ***
(In reply to comment #4) > Due to the fact that the event > still has the same ID, you can't add it before you delete it without problems. > Deleting first makes it work. What are the problems? I'm concerned this may lead to dataloss. Adding before deleting avoids dataloss, and switching this order may lead to dataloss if there is an error. 1. Deleting from calendar A before adding to calendar B mean there could be dataloss. (For example, the calendar B might be remote and may become unreachable. If calendar is deleted from A and then addition to B fails because of poweroutage or network failure, then item could be lost.) * should add to calendar B before deleting from calendar A, so there is no dataloss, even if program crashes when accessing one of the calendars. * the 'add' and 'delete' should be separate subtransactions, so when one throws an exception, the other is undone. (Note: these are undo/redo 'transactions', not distributed database transactions. See http://www.mozilla.org/editor/txmgr/transaction-manager.html#Rollback) 2. In any case, same fixes should be made to undoTransaction, just a few lines later. http://lxr.mozilla.org/mozilla/source/calendar/resources/content/calendar.js#1498
(In reply to comment #9) I'm happy to fix the undoTransaction part here. However, I suggest filing a separate bug on the fact that these should be undo/redo transactions, as that will be a more substantial patch and rather unrelated to the problems described in this bug. The current problem with adding before deleting was discussed a bit on IRC. The providers only like to have one instance of an event-ID. When you add and then delete, the same event tends to get acted on. (Try performing a 'move' transaction before this patch.) Deleting before adding prevents this from happening. This is the same problem that's holding up bug 300250, since copy+paste results in the same 2-instances-of-the-same-ID problem. We really need a method for copying all properties of an event, except the ID. However, while copy+paste should result in an event with a new ID, moving an event from one calendar to another probably should not. It's still the same event, and should therefore have the same ID. Therefore, I'd recommend keeping the delete and then add order, but with a proper implementation of transactions, to avoid the data loss.
why: These are undo/redo 'transactions', not atomic transactions as in a database. Unlike atomic transactions, they do not undo if the program crashes, or the computer fails (power outage). (In a database transaction, all the suboperations are performed/written only tentatively, and don't become permanent until the transaction commits. So if the system crashes, when the database restarts it throws away the uncommitted changes and reverts to the previous state. That way the transaction is atomic: it either performs all its operation, or none of them.) The undo/redo 'transactions' have no 'commit' operation, so they don't inform the provider when they are ready to commit, so they can't be atomic against program/computer crashes. Therefore the program should still add before delete to avoid dataloss in case of program/computer crashes. how: It sounds like maybe the old provider and the new provider are the same provider, and calendar is just a field. A. May be able to use a modify operation to change the calendar field, rather than delete/add, if on same provider. (Somewhat analogous to moving a file on the same disk: the filesystem modifies the name or path, it doesn't rewrite the file.) B. Or add a new item with a different id, then delete the old item, then modify the new item id to the old item id. (One standard approach used to prevent dataloss when saving files is to write the new file to a temporary name, and then once that succeeds rename the new file to the old filename.)
Created attachment 194227 [details] [diff] [review] fix transaction stuff The problem with duplicated id's only happens when you have two storage calendars in the same file (two 'local' calendars) Then the db gets confused when two events have the same ID. I'm not sure if that is more a storage-provider bug, but for now, we can workaround it. This patch makes the move a nested transaction of change id, add, delete, so should be more safe. (it doesn't leave the calendar in an unchanged state if the computer crashes halfway, but at least the data isn't gone. With all the other possible places for dataloss we have, i consider this good enough)
Comment on attachment 194227 [details] [diff] [review] fix transaction stuff add id2 to cal2 delete id1 from cal1 rename id2 to id1 in cal2 what patch implements: rename id1 to id2 in cal1 add id1 to cal2 delete id2 from cal1 This is subtle. As you noted, there is a disadvantage: it does not leave cal1 in the original state if there is a power failure or crash during the add. Is there an advantage of doing the rename operation first?
Also, how about changing the ID back, post-move?
Created attachment 194460 [details] [diff] [review] better fix for transaction stuff > Is there an advantage of doing the rename operation first? The advantage was that i actaully thought of it. I didn't thought of doing it the other way. The new patch does adding before modify or delete. Should be safe. (a crash during undo will leave the calendar modified, but that is less common)
Comment on attachment 194460 [details] [diff] [review] better fix for transaction stuff Looks ok, but crashes during redo (on w2k, using nightly 20050901): Local calendars: 0. default local calendar is "home" 1. create local calendar "home2" if necessary. 2. make sure "calendar" column is visible in event search list (unifinder). 3. create event "movetest" in calendar "home". ok. (event appears with calendar "home") 4. edit event "movetest", change calandar to "home2". ok. (event appears with calendar "home2") 5. menu edit | undo (BUG: calendar is still "home2". Expected calendar to become "home".) 6. menu edit | redo (BUG: CRASH) 7. restart sunbird ("movetest" appears twice in unifinder, both in "home2". Can't select second one? Deleting first deletes both.) ICS Calendars (local file) 0. create local ICS file calendar /temp/ICSMoveTest1.ics 1. create local ICS file calendar /temp/ICSMoveTest2.ics 2. make sure "calendar" column is visible in event search list (unifinder). 3. create event "icsmove" in calendar "ICSMoveTest1". ok. (event appears with calendar "ICSMoveTest1") 4. edit event "icsmove", change calandar to "ICSMoveTest2". ok. (event appears with calendar "ICSMoveTest2") 5. menu edit | undo (Calendar becomes "ICSMoveTest1", better than local calendars.) 8. menu edit | redo (BUG: CRASH) 9. restart sunbird ("icsmove" appears once in unifinder, in "ICSMoveTest2".) Can you find cause of these crashes?
Comment on attachment 194460 [details] [diff] [review] better fix for transaction stuff Doing this right turns out to be a major pain. - The ics (or better: memory) provider doesn't allow you to modify the id of an event. Could make sense, but hurts us here - To do undo right, you need to catch the errors that you get back in the listener, which is async. The txnmgr can't really handle that. So, i'm staying with the current code. That at least works. Yes, it might loose an event if something crashes at the wrong moment. But that can happen anyway when writing the ics file to disk. We should start with fixing that.
btw, i did found the reason for the crash: you shouldn't add a new transaction in redoTransaction. The 'move' action shouldn't do anything. The txnmgr will handle it, thanks to the child-transactions.
Created attachment 194694 [details] [diff] [review] switch undo move order From gekacheka's original comments, the order of undo's move should also be switched. We still have a chance for dataloss, but this will work correctly most of the time.
Comment on attachment 194694 [details] [diff] [review] switch undo move order r=mvl I suggest closing this bug, and filing a new one for the safer way to do the move.
Patch checked in. Closing bug in response to comment #20.
This bug is closed, in order to file a new bug to do this safer. Where is that bug? This feature is still not available in TB 1.5 extension 20051201. Is it fixed in Sunbird? Will that come to the TB stable extension anytime soon?
(In reply to comment #22) > This bug is closed, in order to file a new bug to do this safer. Where is that > bug? This feature is still not available in TB 1.5 extension 20051201. > > Is it fixed in Sunbird? Will that come to the TB stable extension anytime soon? > This bug is fixed in Sunbird 0.3a1 and nightly builds. The new bug for safer moving is bug 307556. The 'stable' extension is not being updated, only the 'experimental' extension and Lightning will contain this feature. (Hopefully the 'experimental' extension can eventually become stable enough to replace the stable version.)
Has the latest patch landed yet? I don't think so and I think that caused bug 425260.