299327 - Events created after 17:00 appear in the next day

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect, P1)

Description

[Stuart Parmenter]

In the week view, if I create an event between 17:00-23:59, it shows up at that
time +1 day.  Some date is in UTC I suspect...

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

*** Bug 304890 has been marked as a duplicate of this bug. ***

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

It seems that icaltime_compare_date_only() has the following implementation
quirk: the "date only" comparison happens in UTC: so even though the two times
may be on the same day in PST, they may fall on different sides of the UTC
dateline once they've been normalized so that they can be compared.  It seems to
me that the API may be broken here: it's not obvious to me how the function can
possibly do the right thing in all cases unless it's given a third parameter:
the timezone to do the comparison in.  Thoughts?

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

At the calIDateTime level, I'm inclined to make the compare() function
asymmetric by adding the following semantic: for date-only comparisons, the
compare is done in the timezone of the object that compare() is being called on.
 This could be implemented in one of several ways:

* add a third arg to icaltime_compare_date_only
* add a icaltime_compare_date_only_in_timezone to libical
* leave libical alone, and do a bunch of normalization/workaround math with the
dates before calling icaltime_compare_date_only

The first solution seems to me to be the most correct.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: workaround for multiday-view, p1

This is just a hack to make it possible to use lightning as dogfood until I get
the real fix done.  I'm going to send mail to the libical mailing list
proposing an API change there.	This patch also contains a minor code cleanup.

Comment 5

[Mike Shaver (:shaver emeritus)]

Comment on attachment 193292 [details] [diff] [review]
workaround for multiday-view, p1

r=shaver

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: the real fix

Comment 7

[Mike Shaver (:shaver emeritus)]

Comment on attachment 193770 [details] [diff] [review]
the real fix

>+   * An invalid or nonexistant timezone was encountered.

"nonexistent"

>+    if (mIsDate || otherIsDate) {
>+        icaltimezone *tz;
>+        nsresult rv = GetIcalTZ(mTimezone, &tz);
>+        if (NS_FAILED(rv))
>+            return calIErrors::INVALID_TIMEZONE;
>+
>+        *aResult = icaltime_compare_date_only(a, b, tz);
>+    }
>+    else {
>         *aResult = icaltime_compare(a, b);
>+    }

Our braces like to cuddle with elses:
} else {

(Too much time in mailnews for you!)

I like it -- can you send it upstream too?  (The libical parts.)

r=shaver with nits fixed.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: the real fix, v2

Nits fixed; carrying forward r=shaver.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Checked in; mail sent to the libical list to see if they want this.  Gonna leave
this open until the upstream bits get resolved.

Comment 10

[Mike Shaver (:shaver emeritus)]

If you don't mind, can you file a new bug with just the libical stuff for
upstream merge?  We don't have this (awful) problem any more, and it's not
well-summarized to describe the outstanding merge work. *smooch*

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Done.  Bug 306112 has been filed to track the upstream propagation.