Last Comment Bug 317786 - when an event has duration and no dtend, the duration is replaced by dtend on serializing (upload/export)
: when an event has duration and no dtend, the duration is replaced by dtend on...
Status: VERIFIED FIXED
: dataloss
Product: Calendar
Classification: Client Software
Component: Internal Components (show other bugs)
: unspecified
: All All
: -- normal (vote)
: 0.8
Assigned To: Daniel Boelzle [:dbo]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-11-25 11:41 PST by Michiel van Leeuwen (email: mvl+moz@)
Modified: 2008-02-04 01:20 PST (History)
6 users (show)
bugzilla: blocking‑calendar0.8+
mschroeder: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
add getter/setter (5.71 KB, patch)
2006-06-14 10:34 PDT, Joey Minta
no flags Details | Diff | Splinter Review
rev1 - Updates jminta's patch with necessary stuff (18.54 KB, patch)
2006-09-23 10:33 PDT, Matthew (lilmatt) Willis
cmtalbert: first‑review+
Details | Diff | Splinter Review
rev2 - removes the unrelated cmd line handler change from patch (17.57 KB, patch)
2006-09-24 11:01 PDT, Matthew (lilmatt) Willis
mattwillis: first‑review+
Details | Diff | Splinter Review

Description Michiel van Leeuwen (email: mvl+moz@) 2005-11-25 11:41:24 PST
After bug 310221 is fixed, vevents with a duration and no dtend will get serialized to a dtend, and the duration information is lost (This can be different, for example on daylight saving switches)
Comment 1 Joey Minta 2006-06-14 10:34:05 PDT
Created attachment 225594 [details] [diff] [review]
add getter/setter

This patch moves to using an explicit getter/setter for endDate.  This allows us to compute, behind the scenes, the proper endDate to return, without actually creating data that will be serialized.
Comment 2 Joey Minta 2006-06-14 10:56:09 PDT
Comment on attachment 225594 [details] [diff] [review]
add getter/setter

hmm, found a regression caused by having this patch in my tree, removing request.
Comment 3 Joey Minta 2006-06-14 10:58:15 PDT
We need to kill some js-stuff to do this cleanly.  The patch in bug 329582 ought to be sufficient.
Comment 4 Dan Mosedale (:dmose) 2006-09-12 15:50:46 PDT
Given the daylight savings issue, I think this should block.  That said, if it's truly necessary to take something as gnarly as the patch in bug 329582 to fix this, then I could pretty easily be talked out of letting this block.
Comment 5 Matthew (lilmatt) Willis 2006-09-13 20:36:03 PDT
(In reply to comment #3)
> We need to kill some js-stuff to do this cleanly.  The patch in bug 329582
> ought to be sufficient.

jminta:
What part of that patch? Clearly, we don't want to take that whole thing at this late date.

Comment 6 Joey Minta 2006-09-14 13:26:26 PDT
(In reply to comment #5)
> jminta:
> What part of that patch? Clearly, we don't want to take that whole thing at
> this late date.
If I remember correctly, when I tried to fix this bug, I ran into problems in places where we initialized start/end dates via assignment to .jsDate.  So, we'd need the fixes to newEvent() and newTodo() in the patch referenced.  Alternatively, the patch in Bug 346934 might also be sufficient, if it hasn't likewise bit-rotted.

Comment 7 Matthew (lilmatt) Willis 2006-09-23 10:33:09 PDT
Created attachment 239805 [details] [diff] [review]
rev1 - Updates jminta's patch with necessary stuff
Comment 8 cmtalbert 2006-09-23 13:36:06 PDT
Comment on attachment 239805 [details] [diff] [review]
rev1 - Updates jminta's patch with necessary stuff

Looks good.
Comment 9 cmtalbert 2006-09-23 13:39:17 PDT
Comments from my review of the patch:
<ctalbert>	in calEvent.clone, why do we clone the duration but not the start and end date objects?
<lilmatt>	um. cloning's expensive
<lilmatt>	?
<ctalbert>	But if it's important enough to clone the duration, it would seem like you should also clone the others too. Maybe there's something I'm missing.
<lilmatt>	jminta?
<ctalbert>	Or, if you don't need to clone the others, then maybe you don't need to clone duration.
<ctalbert>	lilmatt: in get duration, this statement: this.mDuration || this.endDate.subtractDate(this.startDate);
<ctalbert>	Does that work in JS?
<ctalbert>	In C this would return either TRUE or FALSE
<ctalbert>	not the duration or the endDate subtraced from the startdate
<lilmatt>	Ir should
<jminta>	the cloning of those other bits should be done elsewhere
<jminta>	and yes || works cool in js
<ctalbert>	Didn't know about the ||
<ctalbert>	I'll have to use that
<jminta>	if the first one is null/false, it'll return the second
<ctalbert>	awesome
<jminta>	use !!(a || b) to get the C behavior
<ctalbert>	lilmatt: I'll bet that dmose will ask why in geticalComponent duration is not part of the property bag.
<lilmatt>	hmm
<ctalbert>	At line 319:
<ctalbert>	if (!this.startDate.isDate) {
<ctalbert>	return new CalDateTime();
<ctalbert>	This is inside getEndDate
<lilmatt>	ok?
<lilmatt>	Waht do we want to do there
<ctalbert>	How do we know that the new calDateTime has the appropriate date in it for the end Date?
<lilmatt>	If we don't have an endDate, we make one as of now
<lilmatt>	Hence the new CalDateTime
<ctalbert>	oh ok. The caller is will get back a null calDateTime and will realize that you have no enddate, therefore you're an all day event.
<lilmatt>	riht
<lilmatt>	\
<ctalbert>	I don't have a problem with the pseudoEndDate stuff at the end, in my opinion, it's always good to have a default case, but I don't think the code could ever be reached unless you're given a really wacked ICS file
<ctalbert>	Why do we remove the if((arguments in window.... piece from calendar.js?
<lilmatt>	jminta?
<ctalbert>	Oh, maybe to get rid of gCalendarWindow?
<jminta>	that's really really old commandline handling code
<jminta>	i've tried to remove it about 8 times, but never actually got a review done on it
<ctalbert>	We ought to handle command line stuff elsewhere. I guess that's the real change here, right?
<lilmatt>	OK guys. How about this then? I'll open a bug to remove that crap and get r1=clint, r2=joey?
<lilmatt>	Just that part
<ctalbert>	Which part?
<lilmatt>	th ancient cmd line
<ctalbert>	I don't have an issue with it being in here. It seems this patch has a lot of "cleanup code" in it.
<lilmatt>	handling
<lilmatt>	ok
<jminta>	yeah, cleanup patches don't often get reviewed :(
<jminta>	but rs=jminta for removing that one block
<ctalbert>	right. And this doesn't feel like that much of a "qa bug" it's more a development bug, one of those "we should handle this better" sort of things.
<ctalbert>	I'm ok with cleanup in those sorts of bugs
<ctalbert>	my last nit: createEventWithDialog. Down near the end of the patch, you call it with no parameters. But, looking at the function, I'm not clear on how it will behave in that case.
<ctalbert>	Oh wait, onNewEvent is a callback.
<ctalbert>	ok yeah, nevermind
<ctalbert>	In all, the only thing I find questionable is the thing about the duration not being in the property bag. But, that might entail much more refactoring than we want to do at the moment.

I think someone with more familiarity of the architecture should take a look at the property bag thing and make a more informed decision about whether or not that should be an issue.
Comment 10 Matthew (lilmatt) Willis 2006-09-24 11:01:28 PDT
Created attachment 239918 [details] [diff] [review]
rev2 - removes the unrelated cmd line handler change from patch

This removes the unrelated command line handler change from this patch.
It was landed elsewhere.
Comment 11 Michiel van Leeuwen (email: mvl+moz@) 2006-09-25 10:50:37 PDT
I see a whole lot of seemingly unrelated code in this patch, mainly related to the way events are created. why? It looks quite risky...
(example:
-  <command id="new_command" oncommand="newEvent()"/>
+  <command id="new_command" oncommand="createEventWithDialog()"/>
)
Comment 12 Dan Mosedale (:dmose) 2006-09-25 11:56:18 PDT
After discussion in IRC, we've decided that we'd do better to make the interfaces more closely mirror the ICS semantics: duration should be completely separate from the start date and end date, and if one hasn't been set, getting the duration should probably throw.  This will require touching a bunch of callers and is even riskier than the existing patch.  As such, we'll have to relnote this for 0.3.
Comment 13 Matthew (lilmatt) Willis 2006-10-27 08:09:26 PDT
Comment on attachment 239918 [details] [diff] [review]
rev2 - removes the unrelated cmd line handler change from patch

removing review per comment 12
Comment 14 Matthew (lilmatt) Willis 2007-02-28 17:57:19 PST
Not going to make the 0.5 train. Moving forward.
Comment 15 Simon Paquet [:sipaq] 2007-11-24 12:11:51 PST
Setting wanted0.8- as the main Calendar developers will not devote any time to
this in the 0.8 timeframe. Patches are, of course, always welcome.
Comment 16 Sebastian Schwieger 2007-11-24 15:08:51 PST
Can this bug be closed, as the behavior seems now to be different and is tracked in bug 390492?
Comment 17 Daniel Boelzle [:dbo] 2007-11-25 10:01:29 PST
(In reply to comment #16)
IMO this is a different bug. If someone would want to fix bug 390492, then this one could likely to be fixed, too.
Comment 18 Sebastian Schwieger 2007-12-21 09:18:26 PST
At the moment endDate is calculated from duration if the latter is existing, and duration is replaced by endDate on serialization (fixed in bug 390492). Is this still considered a bug? It might hurt interoperability but any client should support both DTEND and DURATION.
Comment 19 Daniel Boelzle [:dbo] 2007-12-21 09:33:40 PST
(In reply to comment #18)
AFAIR w.r.t. to ics calendars, DURATION won't get lost (and shouldn't). Once endDate gets written (e.g. via event dialog), then DTEND is enforced (and DURATION removed). However round tripping via local storage calendar, DURATION is never preserved.
Are your tests observing something different, sebo?
Comment 20 Sebastian Schwieger 2007-12-21 10:06:47 PST
(In reply to comment #19)
Yes, you are right. If the event is not changed, then DURATION gets roundtripped. Otherwise it gets replaced.
Roundtripping via storage will be an issue once we have offline support.
Is the replacement considered a bug?

Comment 21 Daniel Boelzle [:dbo] 2007-12-22 11:09:43 PST
(In reply to comment #20)
> Is the replacement considered a bug?
No, I don't think so.
Comment 22 Daniel Boelzle [:dbo] 2008-01-05 08:43:08 PST
IMO this bug has been fixed with changes for bug 390492:
- DURATION is preserved if event is untouched
- DURATION is removed if DTEND is set
Comment 23 Andreas Treumann 2008-01-31 04:40:46 PST
Checked in latest nightly build -> task is fixed and verified.

Note You need to log in before you can comment on or make changes to this bug.