463784 - Copy and multiple paste is broken, fails with DUPLICATE_ID error

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Stefan Sitter [:ssitter]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081107 Shredder/3.0b1pre (BuildID: 20081107031720)

Copy and multiple paste is broken, fails with DUPLICATE_ID error

Steps to Reproduce:
1. Create an event
2. Copy the event to clipboard
3. Paste the event to another day
4. Paste the event again to another day

Actual results:
The first Paste operation works. The second fails with DUPLICATE_ID error in console and the useless new MODIFICATION_FAILED dialog is displayed.

[[[
Warning: There has been an error reading data for calendar: Home.  However, this error is believed to be minor, so the program will attempt to continue. Error code: DUPLICATE_ID. Description: ID already exists for addItem

Error: An error occurred when writing to the calendar Home! Error code: MODIFICATION_FAILED. Description: 
Source File: file:///[...]/calendar-js/calCalendarManager.js Line: 944
]]]

Expected Results:
The event can be pasted as often as requested (like before).

Regression range: works in Lightning 1.0pre (20081105045801)
                  fails in Lightning 1.0pre (20081106051601)

Check-ins during regression range:
https://hg.mozilla.org/comm-central/pushloghtml?startdate=2008-11-05+04:58:01&enddate=2008-11-06+05:16:01

Most probably regressed by Bug 463060.

Comment 1

[Martin Schröder [:mschroeder]]

I added a newly generated uuid to the items which should be copied instead of leaving the UID field empty. But I also think the old behavior was not correct because if you copy the items into another application they have no UID set. Pasting items in Sunbird or Lightning generates an uuid, and sets the UUID field when adding them to a calendar provider.

Comment 2

[Daniel Boelzle [:dbo]]

Right Martin, the clipboard item needs the original UID. I think however that copying and pasting an item should duplicate it: We could check for calIItemBase::calendar whether the item has originated from a calendar. If it has, we could clean out the existing UID and add it.
This is related (but slightly different) to bug 450653 which heads for iTIP processing, too.

Comment 3

[Stefan Sitter [:ssitter]]

From the IRC discussion last week: In my opinion Copy/Cut to clipboard should preserve the original UID. Pasting from clipboard should set a new UID to the new event/task. This mimics the previous behavior and allows multiple Paste operations.

Comment 4

[Martin Schröder [:mschroeder]]

Attachment: Patch v1

I've fixed it as suggested by ssitter.

Comment 5

[Daniel Boelzle [:dbo]]

Comment on attachment 348166 [details] [diff] [review]
Patch v1

I think this fix is OK as an interim solution. IMO we actually have two valid, but contradicting scenarios:
- copy/pasting the same item multiple times (value semantics, new UID)
- importing an event via clipboard/dnd from other apps or mail attachment (bug 450653, reference semantics, needs to preserve UID to allow iTIP responses)

I think a feasible way would be to check whether METHOD is a simple PUBLISH (i.e. copy/paste scenarios), and generate a new UID in that case. We may check whether an item od the same UID already exists and preserve the UID; however latencies on pasting are annoying so IMO always using a fresh UID is forgivable.
For any other METHOD we parse the ics data as an iTIP message and process it (cal.itip.processItipItem(), needs further UI etc as mentioned in bug 450653).
This proceeding might be OK since users don't respond on published items. The only drawback it has is that we don't properly relate updates of published items, e.g. if those have been received via iMIP.

>     for each (let item in calendarItemArray) {
>-        // If we copy an item and paste it again, it will have the same ID as
>-        // the original. Therefore, give every item a new ID.
>         let newItem = item.clone();
>-        newItem.id = cal.getUUID();
>         calComp.addSubcomponent(newItem.icalComponent);
>     }

I don't see a reason any longer for cloning the item; please remove.

r=dbo

Comment 6

[Stefan Sitter [:ssitter]]

Comment on attachment 348166 [details] [diff] [review]
Patch v1

>             startBatchTransaction();
>             for each (let item in items) {
>                 let newItem = item.clone();
>+                newItem.id = null;
>                 if (item.startDate) {

I'd rather use newItem.id = cal.getUUID() here. It is more explicit about the purpose instead of relying on the fact that some internal component sets the new UID.

Comment 7

[Stefan Sitter [:ssitter]]

And maybe add a comment like "set new UID to allow multiple pasting of the same clipboard content"?

Comment 8

[Martin Schröder [:mschroeder]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/9413bd795016> (with review nits fixed)
-> FIXED

(In reply to comment #5)
> (From update of attachment 348166 [details] [diff] [review])
> I think this fix is OK as an interim solution. IMO we actually have two valid,
> but contradicting scenarios:
> - copy/pasting the same item multiple times (value semantics, new UID)
> - importing an event via clipboard/dnd from other apps or mail attachment (bug
> 450653, reference semantics, needs to preserve UID to allow iTIP responses)

I think, we should file a follow-up bug to solve these issues.

Comment 9

[Daniel Boelzle [:dbo]]

I think bug 450653 already serves well for it.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".