Closed Bug 392853 Opened 17 years ago Closed 17 years ago

libical/calIDateTime's subtractDate doesn't honor timezones

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Fallen, Assigned: dbo)

Details

Attachments

(3 files, 2 obsolete files)

Attached file Testcase #1 (obsolete) —
Consider the attached testcase. The times are the same, the timezone is different, but subtractDate says the times are PT0S apart. Also, DST is not properly handled, see bug 372839.
Attached file Testcase #2 (obsolete) —
should lead to zero length duration.
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
mNativeTime needs to be updated when timezone is set.
Flags: blocking-calendar0.7+
More flaws are occurring for
- subtractDate(): mixing DATE and DATETIMES. I strongly suggest to change subtractDate() using the same mimic as documented for compare().
- addDuration(): if object is DATE, what happens if duration has time parts? I suggest to document that those will be chopped.
I think it would be sensible to have 2007-08-15 + PT1H = 2007-08-16 01:00:00 and 2007-08-15 - PT1H = 2007-08-14 23:00:00. This way you don't have to set isDate false, add a day, then add an hour for positive durations or set isDate to false, and remove an hour for negative durations.

Something similar might be possible for subtractDate, although that might be much harder to implement. Intuitively, I would assume the following calculations valid:

2007-08-15 15:00:00 UTC - 2007-08-15 = PT15H
2007-08-15 - 2007-08-14 15:00:00 UTC = PT9H
2007-08-16 15:00:00 UTC - 2007-08-15 = PT15H

These are just random thoughts, I haven't looked into how hard it would be to implement or how much sense it makes w.r.t the code.
(In reply to comment #4)
Due to the fact that comparison is already reasonably defined well, i.e. coercion to DATE, I would want to stick to it for subtractDate's mimic. IMO we need to educate that DATE an DATETIME are two separate classes.
BTW: looking at calDateTime from a code quality perspective, we should IMO strongly consider to revise the code holding/working on an icaltime; all those mapping FromIcalTime/ToIcalTime gets confusing.
Attachment #277690 - Flags: review?(philipp)
Attached file test case
unified test case
Attachment #277368 - Attachment is obsolete: true
Attachment #277394 - Attachment is obsolete: true
Comment on attachment 277691 [details]
test case

Could I get you to convert this testcase into a unit test, and pout it into calendar/test/unit ?
(In reply to comment #8)
Good point; Martin volunteered to keep care of that as he wants to refurbish the unit tests.
Comment on attachment 277690 [details] [diff] [review]
fixing mNativeTime when timezone changes

Looks fine, does what it is supposed to.
Attachment #277690 - Flags: review?(philipp) → review+
(In reply to comment #3)
filed follow-up bug 393391 for that
(In reply to comment #6)
filed follow-up bug 393392 for that
Checked in on HEAD and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.7
(In reply to comment #9)
> Good point; Martin volunteered to keep care of that as he wants to refurbish
> the unit tests.

More than two month later... my first test.
Attachment #287248 - Flags: review?(daniel.boelzle)
Comment on attachment 287248 [details] [diff] [review]
Unit test v1 [checked in]

thanks! r=dbo

just a side-note: putting more and more tests into a single function, we should either start to write up more sub functions or block scope the individual tests:

{ // test 1
...
}
{ // test 2
...
}
...

so we could better manage  the used variables.
Attachment #287248 - Flags: review?(daniel.boelzle) → review+
Comment on attachment 287248 [details] [diff] [review]
Unit test v1 [checked in]

Checked in on HEAD and MOZILLA_1_8_BRANCH

Checking in calendar/test/unit/test_datetime.js;
/cvsroot/mozilla/calendar/test/unit/test_datetime.js,v  <--  test_datetime.js
new revision: 1.4; previous revision: 1.3
done

Checking in calendar/test/unit/test_datetime.js;
/cvsroot/mozilla/calendar/test/unit/test_datetime.js,v  <--  test_datetime.js
new revision: 1.1.2.4; previous revision: 1.1.2.3
done
Attachment #287248 - Attachment description: Unit test v1 → Unit test v1 [checked in]
Flags: in-testsuite+
verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: