220655 - Enable moving items (such as events) from one Calendar File to another

Status: RESOLVED FIXED

(Calendar :: General, enhancement)

Description

[Matija Polajnar]

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.

Comment 1

[gekacheka]

*** Bug 259472 has been marked as a duplicate of this bug. ***

Comment 2

[rabbitambulance]

*** Bug 298598 has been marked as a duplicate of this bug. ***

Comment 3

[Joey Minta]

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.

Comment 4

[Joey Minta]

Attachment: 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 5

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

Comment on attachment 190907 [details] [diff] [review]
switch action order

r=mvl

Comment 6

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

patch checked in.

Comment 7

[Joey Minta]

*** Bug 291243 has been marked as a duplicate of this bug. ***

Comment 8

[Joey Minta]

*** Bug 209687 has been marked as a duplicate of this bug. ***

Comment 9

[gekacheka]

(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

Comment 10

[Joey Minta]

(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.

Comment 11

[gekacheka]

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.)

Comment 12

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

Attachment: 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 13

[gekacheka]

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?

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Also, how about changing the ID back, post-move?

Comment 15

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

Attachment: 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 16

[gekacheka]

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 17

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

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.

Comment 18

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

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.

Comment 19

[Joey Minta]

Attachment: 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 20

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

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.

Comment 21

[Joey Minta]

Patch checked in.  Closing bug in response to comment #20.

Comment 22

[Crystle Numan]

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?

Comment 23

[Joey Minta]

(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.)

Comment 24

[Tom Aratyn]

Has the latest patch landed yet?

I don't think so and I think that caused bug 425260.