392853 - libical/calIDateTime's subtractDate doesn't honor timezones

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Testcase #1

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.

Comment 1

[Daniel Boelzle [:dbo]]

Attachment: Testcase #2

should lead to zero length duration.

Comment 2

[Daniel Boelzle [:dbo]]

mNativeTime needs to be updated when timezone is set.

Comment 3

[Daniel Boelzle [:dbo]]

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.

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 5

[Daniel Boelzle [:dbo]]

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

Comment 6

[Daniel Boelzle [:dbo]]

Attachment: fixing mNativeTime when timezone changes

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.

Comment 7

[Daniel Boelzle [:dbo]]

Attachment: test case

unified test case

Comment 8

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

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 ?

Comment 9

[Daniel Boelzle [:dbo]]

(In reply to comment #8)
Good point; Martin volunteered to keep care of that as he wants to refurbish the unit tests.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 277690 [details] [diff] [review]
fixing mNativeTime when timezone changes

Looks fine, does what it is supposed to.

Comment 11

[Daniel Boelzle [:dbo]]

(In reply to comment #3)
filed follow-up bug 393391 for that

Comment 12

[Daniel Boelzle [:dbo]]

(In reply to comment #6)
filed follow-up bug 393392 for that

Comment 13

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH.

Comment 14

[Martin Schröder [:mschroeder]]

Attachment: Unit test v1 [checked in]

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

Comment 15

[Daniel Boelzle [:dbo]]

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.

Comment 16

[Martin Schröder [:mschroeder]]

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

Comment 17

[Daniel Boelzle [:dbo]]

verified