Closed Bug 419807 Opened 12 years ago Closed 11 years ago

After closing task dialog now save-messagebox pops up

Categories

(Calendar :: Provider: WCAP, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: berend.cornelius09, Assigned: dbo)

Details

Attachments

(2 files, 1 obsolete file)

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.
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.
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?
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...
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.
Attached patch patch v. #1 (obsolete) — Splinter Review
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.
Berend, I could no more reproduce this with Ltn-2008-06-03-19 - resolve as WFM ?
Daniel knows best.... This is a backend-problem.
(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.
(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.
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 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");
(In reply to comment #10)
filed bug 437400 for that
Assignee: nobody → Berend.Cornelius
Status: NEW → ASSIGNED
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.
Attachment #309427 - Attachment is obsolete: true
Attachment #323872 - Flags: review?(daniel.boelzle)
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
Attachment #323872 - Flags: review?(daniel.boelzle) → review+
patch checked in on trunk and MOZILLA_1_8_BRANCH

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

Berend, can this bug be closed?
Target Milestone: --- → 0.9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.
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.  
Status: RESOLVED → REOPENED
Component: Calendar Views → Provider: WCAP
Flags: wanted-calendar0.9+
Resolution: FIXED → ---
changed component and taking
Assignee: Berend.Cornelius → daniel.boelzle
Status: REOPENED → NEW
QA Contact: views → wcap-provider
Attachment #323872 - Attachment description: patch v. #2 → [checked in] patch v. #2
Target Milestone: 0.9 → 1.0
Flags: wanted-calendar0.9+ → wanted-calendar1.0+
Attached patch fix - v1Splinter Review
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.
Attachment #349197 - Flags: review?(philipp)
Status: NEW → ASSIGNED
Comment on attachment 349197 [details] [diff] [review]
fix - v1

r=philipp
Attachment #349197 - Flags: review?(philipp) → review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/be6acef7ccb8>

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Checked with lightning build 20081130 -> VERIFIED.
Status: RESOLVED → VERIFIED
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".
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.