Closed Bug 317786 Opened 19 years ago Closed 16 years ago

when an event has duration and no dtend, the duration is replaced by dtend on serializing (upload/export)

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mvl, Assigned: dbo)

Details

(Keywords: dataloss)

Attachments

(1 file, 2 obsolete files)

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)
No longer blocks: lightning-0.1
Attached patch add getter/setter (obsolete) — Splinter Review
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.
Assignee: base → jminta
Status: NEW → ASSIGNED
Attachment #225594 - Flags: first-review?(dmose)
Comment on attachment 225594 [details] [diff] [review]
add getter/setter

hmm, found a regression caused by having this patch in my tree, removing request.
Attachment #225594 - Flags: first-review?(dmose)
We need to kill some js-stuff to do this cleanly.  The patch in bug 329582 ought to be sufficient.
Depends on: 329582
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.
Flags: blocking0.3+
(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.

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

Whiteboard: [needs patch]
Assignee: jminta → lilmatt
Attachment #225594 - Attachment is obsolete: true
Attachment #239805 - Flags: first-review?(cmtalbert)
Whiteboard: [needs patch] → [patch in hand][needs review ctalbert]
Comment on attachment 239805 [details] [diff] [review]
rev1 - Updates jminta's patch with necessary stuff

Looks good.
Attachment #239805 - Flags: first-review?(cmtalbert) → first-review+
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.
Attachment #239805 - Flags: second-review?(dmose)
No longer depends on: 329582
Whiteboard: [patch in hand][needs review ctalbert] → [patch in hand][needs review dmose]
This removes the unrelated command line handler change from this patch.
It was landed elsewhere.
Attachment #239805 - Attachment is obsolete: true
Attachment #239918 - Flags: second-review?(dmose)
Attachment #239918 - Flags: first-review+
Attachment #239805 - Flags: second-review?(dmose)
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()"/>
)
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.
Assignee: lilmatt → nobody
Status: ASSIGNED → NEW
Flags: blocking0.3+ → blocking0.3-
Keywords: relnote
Whiteboard: [patch in hand][needs review dmose] → [cal relnote]
Target Milestone: --- → Sunbird 0.5
Comment on attachment 239918 [details] [diff] [review]
rev2 - removes the unrelated cmd line handler change from patch

removing review per comment 12
Attachment #239918 - Flags: second-review?(dmose)
Not going to make the 0.5 train. Moving forward.
Target Milestone: Sunbird 0.5 → Sunbird 0.7
Whiteboard: [cal relnote]
Flags: wanted-calendar0.8?
Target Milestone: 0.7 → ---
Version: Trunk → unspecified
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.
Flags: wanted-calendar0.8? → wanted-calendar0.8-
Can this bug be closed, as the behavior seems now to be different and is tracked in bug 390492?
(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.
Flags: wanted-calendar0.8- → wanted-calendar0.8+
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.
(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?
(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?

(In reply to comment #20)
> Is the replacement considered a bug?
No, I don't think so.
Assignee: nobody → daniel.boelzle
Flags: wanted-calendar0.8+ → blocking-calendar0.8+
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
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Flags: in-testsuite?
Checked in latest nightly build -> task is fixed and verified.
Status: RESOLVED → VERIFIED
Keywords: relnote
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.