307416 - providers use nativetime for queries, but that isn't correctly defined for floating items

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

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

The storage provider stores the .nativeTime together with the timezone
identifier string for dates. The problem is that a floating datetime doesn't
have a well-defined .nativeTime. When quering, events with a floating datetime
are handled like they are in utc.

For me (in gmt+2) this means that events near the end of the day are not showed
in the dayview.

A possible solution could be to add the gmt-offset to the datetime of the events
while quering (just for floating events)

Comment 1

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

Attachment: patch memory and storage providers

This patch adds the timezoneoffset of the start/end datetime to the stored
start/end datetimes when querying. This is the same as converting the stored
datetimes to the timezone, but without going through calIDateTime etc, which
would be slow (and nearly impossible for storage)
Then i changed the (old) views to actually query in the right timezone, not in
UTC. I think that's the right thing to do anyway.

Comment 2

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

this blocks 0.3a1, we use floating datetime now, so we better make sure it works.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 195391 [details] [diff] [review]
patch memory and storage providers

Looks good; just a few nits:

>   /**
>+   * The offset of the timezone this datetime is in, relative to gmt, in
>+   * seconds. A positive number means that the timezone is ahead of gmt.
>+   */
>+  readonly attribute long timezoneOffset;
>+

s/gmt/UTC/ if you would, please.

>Index: base/src/calDateTime.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/base/src/calDateTime.cpp,v
>retrieving revision 1.47
>diff -u -9 -p -d -r1.47 calDateTime.cpp
>--- base/src/calDateTime.cpp	29 Aug 2005 18:48:47 -0000	1.47
>+++ base/src/calDateTime.cpp	8 Sep 2005 21:17:56 -0000
>@@ -201,18 +201,29 @@ calDateTime::SetTimezone(const nsACStrin
>             return rv;
> 
>         mTimezone.Assign(aTimezone);
>     }
> 
>     return NS_OK;
> }
> 
> NS_IMETHODIMP
>+calDateTime::GetTimezoneOffset(PRInt32 *aResult)
>+{
>+    struct icaltimetype icalt;
>+    ToIcalTime(&icalt);
>+
>+    int dst;
>+    *aResult = icaltimezone_get_utc_offset((icaltimezone*)icalt.zone, &icalt, &dst);
>+    return NS_OK;

In general, C++-style casts are preferred to C-style ones, because they express
intent more precisely to the compiler, allowing it to catch more bugs at build
time.  So NS_CONST_CAST would be the thing here...

>Index: providers/storage/calStorageCalendar.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/providers/storage/calStorageCalendar.js,v
>retrieving revision 1.64
>diff -u -9 -p -d -r1.64 calStorageCalendar.js
>--- providers/storage/calStorageCalendar.js	25 Aug 2005 02:06:32 -0000	1.64
>+++ providers/storage/calStorageCalendar.js	8 Sep 2005 21:17:57 -0000
>@@ -630,18 +630,20 @@ calStorageCalendar.prototype = {
> 
>             var resultItems = [];
> 
>             // first get non-recurring events and recurring events that happen
>             // to fall within the range
>             sp = this.mSelectEventsByRange.params;
>             sp.cal_id = this.mCalId;
>             sp.event_start = startTime;
>             sp.event_end = endTime;
>+            sp.start_offset = aRangeStart ? aRangeStart.timezoneOffset * 1000000 : 0;
>+            sp.end_offset = aRangeEnd ? aRangeEnd.timezoneOffset * 1000000 : 0;

A short comment about what this 1000000 means and why it's necessary would be
helpful.  Alternately, using |const USECS_PER_SECOND| be a reasonable way to
document.

>-        this.mSelectEventsByRange = createStatement(
>+/*        this.mSelectEventsByRange = createStatement(
>             this.mDB,
>             "SELECT * FROM cal_events " +
>             "WHERE event_end >= :event_start AND event_start < :event_end " +
>             "  AND cal_id = :cal_id AND recurrence_id IS NULL"
>+            );*/

No need to leave this here.  We've got cvs history if we should need to revert
for some reason.  However...

>+
>+        this.mSelectEventsByRange = createStatement(
>+            this.mDB,
>+            "SELECT * FROM cal_events " +
>+            "WHERE " +
>+            " ((event_end_tz = 'floating' AND event_end - :end_offset >= :event_start) OR " +
>+            "  (event_end_tz != 'floating' AND event_end >= :event_start)) AND " +
>+            " ((event_start_tz = 'floating' AND event_start - :start_offset < :event_end) OR " +
>+            "  (event_start_tz != 'floating' AND event_start < :event_end)) " +
>+            "  AND cal_id = :cal_id AND recurrence_id IS NULL"
>             );
> 

This code is really difficult to read, but a big part of that is that the
original code uses :event_{start,end} which are really confusing parameter
names.	Seems to me that'd be a bunch less confusing if they were named
:range_{start,end}.  If you felt like making that change, that'd rock.

If you'd prefer to spin the above change off to a separate bug so that all
instances of that problem could be taken care of at once to keep the code in
sync, another possibility here would be to break the subclauses into individual
variables like so:

var CONVERTED_FLOATING_ENDTIME_IN_RANGE = "(event_end_tz = 'floating' AND
event_end - :end_offset >= :end_start";

etc.

>Index: providers/memory/calMemoryCalendar.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/providers/memory/calMemoryCalendar.js,v
>retrieving revision 1.35
>diff -u -9 -p -d -r1.35 calMemoryCalendar.js
>--- providers/memory/calMemoryCalendar.js	10 Aug 2005 23:05:35 -0000	1.35
>+++ providers/memory/calMemoryCalendar.js	8 Sep 2005 21:17:57 -0000
>@@ -381,38 +381,44 @@ calMemoryCalendar.prototype = {
>             var item = this.mItems[itemIndex];
>             var itemtoadd = null;
> 
>             var itemStartTime = 0;
>             var itemEndTime = 0;
> 
>             var tmpitem = item;
>             if (wantEvents && (item instanceof calIEvent)) {
>                 tmpitem = item.QueryInterface(calIEvent);
>-                itemStartTime = (item.startDate.isValid
>+                itemStartTime = (item.startDate
>                                  ? item.startDate.nativeTime
>                                  : START_OF_TIME);
>-                itemEndTime = (item.endDate.isValid
>+                itemEndTime = (item.endDate
>                                ? item.endDate.nativeTime
>                                : END_OF_TIME);

What's the reasoning behind this change?

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Also, can you change calIDateTime.nativeTime to throw an exception when called
on a floating object, and document this in the IDL with @exception?

Comment 5

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

(In reply to comment #3)
> >-                itemEndTime = (item.endDate.isValid
> >+                itemEndTime = (item.endDate
> What's the reasoning behind this change?

The event interface changed. Now they return a null .endDate when no end date
was set. (They used to return a valid object, but with .isValid set to false)

Comment 6

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

Attachment: patch v2

Updated the patch. I didn't make .nativeTime throw, i just specified it. (It is
still used in the memory provider, but now correct)

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 196676 [details] [diff] [review]
patch v2

Looks good.  Change the "warning:" to "@warning" so doxygen will pick it up,
and you've got r=dmose.

Comment 8

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

patch checked in.