419807 - After closing task dialog now save-messagebox pops up

Status: VERIFIED FIXED

(Calendar :: Provider: WCAP, defect)

Description

[Berend Cornelius]

Go to calendar view and edit a task from a series of tasks
Messagebox "Editing a repeating item" pops up ->click "This occurrence only"

event dialog pops up
close the dialog without modifications

-> "Save Task" messagebox pops up
Expected behaviour:
messagebox should not pop up.

Comment 1

[Berend Cornelius]

I could only reproduce this issue with recurring tasks and debugged into it. 
In line 226 of sun-calendar-event-dialog.js it is checked if the item has changed by:
...
    if (newItem.icalString == oldItem.icalString) {
...
I could clearly see that the icalString has been changed within the property "LAST-MODIFIED".
I started to wonder why this error did not occur with events and started the same procedure with an event. This is the output in the javascript debugger I retrieved with :

0003: oldItem.icalString
$[1] = [string] "BEGIN:VCALENDAR\r\nPRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN\r\nVERSION:2.0\r\nBEGIN:VTIMEZONE\r\nTZID:/mozilla.org/20071231_1/Africa/Ceuta\r\nX-LIC-LOCATION:Africa/Ceuta\r\nBEGIN:DAYLIGHT\r\nTZOFFSETFROM:+0100\r\nTZOFFSETTO:+0200\r\nTZNAME:CEST\r\nDTSTART:19700329T020000\r\nRRULE:FREQ=YEARLY;INTERVAL=1;BYDAY=-1SU;BYMONTH=3\r\nEND:DAYLIGHT\r\nBEGIN:STANDARD\r\nTZOFFSETFROM:+0200\r\nTZOFFSETTO:+0100\r\nTZNAME:CET\r\nDTSTART:19701025T030000\r\nRRULE:FREQ=YEARLY;INTERVAL=1;BYDAY=-1SU;BYMONTH=10\r\nEND:STANDARD\r\nEND:VTIMEZONE\r\nBEGIN:VEVENT\r\nCREATED:20080213T161140Z\r\nLAST-MODIFIED:20080226T170601Z\r\nDTSTAMP:20080226T170601Z\r\nUID:a3b4a388-0650-465d-acc0-f95b0b517f6b\r\nSUMMARY:New Home-Event\r\nDTSTART;TZID=/mozilla.org/20071231_1/Africa/Ceuta:20080213T180000\r\nDTEND;TZID=/mozilla.org/20071231_1/Africa/Ceuta:20080213T190000\r\nTRANSP:OPAQUE\r\nEND:VEVENT\r\nEND:VCALENDAR\r\n"
0003: newItem.icalString
$[2] = [string] "BEGIN:VCALENDAR\r\nPRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN\r\nVERSION:2.0\r\nBEGIN:VTIMEZONE\r\nTZID:/mozilla.org/20071231_1/Africa/Ceuta\r\nX-LIC-LOCATION:Africa/Ceuta\r\nBEGIN:DAYLIGHT\r\nTZOFFSETFROM:+0100\r\nTZOFFSETTO:+0200\r\nTZNAME:CEST\r\nDTSTART:19700329T020000\r\nRRULE:FREQ=YEARLY;INTERVAL=1;BYDAY=-1SU;BYMONTH=3\r\nEND:DAYLIGHT\r\nBEGIN:STANDARD\r\nTZOFFSETFROM:+0200\r\nTZOFFSETTO:+0100\r\nTZNAME:CET\r\nDTSTART:19701025T030000\r\nRRULE:FREQ=YEARLY;INTERVAL=1;BYDAY=-1SU;BYMONTH=10\r\nEND:STANDARD\r\nEND:VTIMEZONE\r\nBEGIN:VEVENT\r\nCREATED:20080213T161140Z\r\nLAST-MODIFIED:20080226T170614Z\r\nDTSTAMP:20080226T170614Z\r\nUID:a3b4a388-0650-465d-acc0-f95b0b517f6b\r\nSUMMARY:New Home-Event\r\nDTSTART;TZID=/mozilla.org/20071231_1/Africa/Ceuta:20080213T180000\r\nDTEND;TZID=/mozilla.org/20071231_1/Africa/Ceuta:20080213T190000\r\nTRANSP:OPAQUE\r\nEND:VEVENT\r\nEND:VCALENDAR\r\n"
0003: newItem.icalString == oldItem.icalString
$[3] = [boolean] true

As can be seen the icalstrings again differ in their property "LAST-MODIFIED" but Javascript does not seem to notice this and finds both strings are same.
I am a bit at a loss why the string comparison delivers a wrong return value once in a while and second I wonder how to solve the problem with the "LAST-MODIFIED" property in an elegant way.

Comment 2

[Sven Giermann]

I had this same issue after upgrading from 0.5!

It seems, that some properties were changed in the background, because I had to save it once and then the message did not come up again.

Apart from that, I cannot reproduce on a current build - did you try to use a clean profile?
What happens, when you first create a new repeating event?

Comment 3

[Sven Giermann]

Sorry, I did not read to the end...

This only happens with Tasks and I can reproduce.

My workaround for events further confuses the situation:
After saving a single instance of a repeating task, I get no question (all/this occurence) when I try to open it again, but the drop-down for setting the repeat interval is still grayed out - so it seems to force editing of this single occurence of the repeating task.

And one more issue: when I edit "all occurences" to another repeat interval, the former edited occurence still exists and shows the new repeat interval...

Comment 4

[Berend Cornelius]

I talked with Daniel (dbo) about the problem. He said that querying the icalstring also might modify the item itself which could be the reason why the string comparison delivers false. In accord with him I implemented the patch attached that deletes the relevant property substrings ("LAST-MODIFIED" and "DTSTAMP" from the icalStrings), but the problem still ocurred. Further investigation made me aware that cloning the window.calendarItem did not create a real copy of the item. It seems like the parent item is cloned which is why the dueDate differs from the original item. Daniel will set up an issue for this.

Comment 5

[Berend Cornelius]

Attachment: patch v. #1

The patch attached introduces a method to compare items. This is not really a clean solution and I don't know if it's really worth being integrated. In the long run we should probably add a member function to calItemBase approximately called "equals" that is able to consider various levels of comparison e.g. consider the calendar of the item or not.

Comment 6

[Sven Giermann]

Berend, I could no more reproduce this with Ltn-2008-06-03-19 - resolve as WFM ?

Comment 7

[Berend Cornelius]

Daniel knows best.... This is a backend-problem.

Comment 8

[Daniel Boelzle [:dbo]]

(In reply to comment #4)
> I talked with Daniel (dbo) about the problem. He said that querying the
> icalstring also might modify the item itself which could be the reason why the
> string comparison delivers false. In accord with him I implemented the patch
> attached that deletes the relevant property substrings ("LAST-MODIFIED" and
> "DTSTAMP" from the icalStrings), but the problem still ocurred. Further
The cause is here:
<http://mxr.mozilla.org/mozilla1.8/source/calendar/base/src/calItemBase.js#685>
Probably worth a fix IMO, because reading out the ical component shouldn't modify the object.

> investigation made me aware that cloning the window.calendarItem did not create
> a real copy of the item. It seems like the parent item is cloned which is why
> the dueDate differs from the original item. Daniel will set up an issue for
> this.
This has been fixed with bug 368976.

(In reply to comment #6)
> Berend, I could no more reproduce this with Ltn-2008-06-03-19 - resolve as WFM
> ?
Could be the case, because it heavily depends on timing: DTSTAMP has a resolution in seconds.

Comment 9

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

(In reply to comment #8)
> (In reply to comment #4)
> The cause is here:
> <http://mxr.mozilla.org/mozilla1.8/source/calendar/base/src/calItemBase.js#685>
> Probably worth a fix IMO, because reading out the ical component shouldn't
> modify the object.

My reading of rfc 2445 is that the dtstamp should be the current time when creating the ical string. I'm not sure if the calIItemBase dtstamp should be updated, though.

rfc 2445 4.8.7.2:
   This property is different than the "CREATED" and "LAST-MODIFIED"
   properties. These two properties are used to specify when the
   particular calendar data in the calendar store was created and last
   modified. This is different than when the iCalendar object
   representation of the calendar service information was created or
   last modified.

(according to rfc2445bis, this only holds if no METHOD is specified.)


Anyway, I never liked the idea of comparing items based on the ical string serialization being equal. One item can have many different serializations.

Comment 10

[Daniel Boelzle [:dbo]]

Yes, my reading of 2445 is the same. I am only arguing from an API POV that reading an attribute shouldn't modify the object, i.e. reading twice should return the same data. And yes, comparing the ical strings is more a hack than something clean.

Comment 11

[Daniel Boelzle [:dbo]]

Comment on attachment 309427 [details] [diff] [review]
patch v. #1

>+function compareItemContent(aFirstItem, aSecondItem) {
>+    function getOverworkedIcalString(aItem) {
>+        var icalString = aItem.icalString;
>+        icalString = icalString.replace(/\r\nLAST-MODIFIED:.+/, "");
>+        return icalString.replace(/\r\nDTSTAMP:.+/, "");
Berend, you should probably add:
var propStrings = icalString.split("\n");
propStrings.sort();
return propStrings.join("\n");

Comment 12

[Daniel Boelzle [:dbo]]

(In reply to comment #10)
filed bug 437400 for that

Comment 13

[Berend Cornelius]

Attachment: [checked in] patch v. #2

I could not reproduce the failure anymore - with or without my patch. But maybe the patch adds some clarity to the sourcecode and we can take it anyhow.

Comment 14

[Daniel Boelzle [:dbo]]

Comment on attachment 323872 [details] [diff] [review]
[checked in] patch v. #2

>+ 
>+function compareItemContent(aFirstItem, aSecondItem) {
>+    function getHashItem(aItem) {
>+        var icalString = aItem.icalString;
>+        icalString = icalString.replace(/\r\nLAST-MODIFIED:.+/, "");
>+        icalString = icalString.replace(/\r\nDTSTAMP:.+/, "");
>+        var propStrings = icalString.split("\n");
>+        propStrings.sort();
>+        return propStrings.join("\n");
>+    }
just call it hashItem, meaning it gets a hash for an item.

r=dbo

Comment 15

[Berend Cornelius]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> fixed

Comment 16

[Martin Schröder [:mschroeder]]

(In reply to comment #15)
> patch checked in on trunk and MOZILLA_1_8_BRANCH
> 
> -> fixed
> 

Berend, can this bug be closed?

Comment 17

[Berend Cornelius]

I am actually not quite sure if this bug is really fixed as my patch solved the problem in a rather superficial way (after consultation with daniel). I'd rather like to ask daniel if we should keep this issue open. However Andreas told me he could not reproduce the problem anymore either.

Comment 18

[Andreas Treumann]

I checked this issue with different providers and this issue is fixed for local storage calendars, webdav- and caldav-calendars. But on WCAP calendars this issue still exists -> I cc'ed Daniel.  

Comment 19

[Daniel Boelzle [:dbo]]

changed component and taking

Comment 20

[Daniel Boelzle [:dbo]]

Attachment: fix - v1

The problem seems to be that the WCAP server always sends a PERCENTAGE-COMPLETE property although we haven't specified one. This patch handles existing PERCENTAGE-COMPLETE properties gracefully.

Comment 21

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 349197 [details] [diff] [review]
fix - v1

r=philipp

Comment 22

[Daniel Boelzle [:dbo]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/be6acef7ccb8>

-> FIXED

Comment 23

[Andreas Treumann]

Checked with lightning build 20081130 -> VERIFIED.

Comment 24

[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".