290722 - calICalendar.getItems: DateTime range should use exclusive end dateTime.

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[gekacheka]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3
Build Identifier: trunk

Interface calICalendar method getItems specifies:
238    * @param aRangeStart Items starting at this time or after should be 
239    *                    returned.  If invalid, assume "since the beginning
240    *                    of time".
241    * @param aRangeEnd   Items starting at this time or before should be
242    *                    returned.  If invalid, assume "until the end of time".
http://lxr.mozilla.org/mozilla/source/calendar/base/public/calICalendar.idl#241

To specify all events in one month, currently have to do:
  endDate = new Date(startDate)
  endDate.setMonth(startDate.getMonth() + 1);
  endDate.setMilliseconds(endDate.getMilliseconds() - 1);

To specify all events in one week, currently have to do: 
  endDate = new Date(startDate)
  endDate.setDate(startDate.getDate() + 7);
  endDate.setMilliseconds(endDate.getMilliseconds() - 1);

To specify all events in one day, currently have to do: 
  endDate = new Date(startDate)
  endDate.setDate(startDate.getDate() + 1);
  endDate.setMilliseconds(endDate.getMilliseconds() - 1);

To specify all events in one hour, currently have to do: 
  endDate = new Date(startDate)
  endDate.setHours(startDate.getHours() + 1);
  endDate.setMilliseconds(endDate.getMilliseconds() - 1);

1. The last step (set milliseconds) would not be necessary if the end
date were exclusive.  

2. It makes code dependent on the minimum time granularity, which may be
application dependent and provider implemention dependent. For
example, someone might write code that subtracts one microsecond, or
one second, or one minute, or one hour, or one day instead.

3. RFC2445sec4.6.1 specifies end times are exclusive.
     The "DTEND" property for a "VEVENT"
     calendar component specifies the non-inclusive end of the event.

4. If someone knows a particular implemenation uses millisecond
granularity, but the display code only uses second granularity, then
they can sneak in an event in the middle of that last second and it
won't be covered by any day queries.  It should never be possible to
"hide" events like this regardless of implemenation.  Queries for
continguous periods should return all events in the span of those
periods.

(The only case where an inclusive end time may help a little is if
there is some reason to retrieve zero-duration events that start at a
particular time, then you can use endDate = startDate.  But I don't
think this case occurs in the code (there is always a non-zero period
of interest), so it is probably rare in general.  If it becomes
needed, then a method that covers this case and just needs one
parameter time could be added, but it is not needed now.)

I recommend changing the calICalendar.getItems spec use an exclusive end date:
    * @param aRangeStart Items starting at this time or after should be 
    *                    returned.  If invalid, assume "since the beginning
    *                    of time".
    * @param aRangeEndEx  Items starting before but not including this time
    *                     should be returned.  If invalid, assume "until the
    *                     end of time".

(Suffix "-Ex" on the end parameter name can help remind that it is exclusive.)

Reproducible: Always

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Sold.

Will patch on Mondayish, barring any violent disagreement..

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: non-inclusive-end-0.patch

Change all getItems to be non-inclusive on end, except for caldav provider. 
dmose says that one already does slightly wrong things for the range there..

Comment 3

[Mike Shaver (:shaver emeritus)]

Comment on attachment 181333 [details] [diff] [review]
non-inclusive-end-0.patch

>-   * @param aRangeEnd   Items starting at this time or before should be
>+   * @param aRangeEnd   Items starting before (not including aRangeEnd) should be
>    *                    returned.  If invalid, assume "until the end of time".

Parens end one word too late, IMO, and I like geka's suggestion of an Ex
suffix.

Looks good other than that, thanks to all.

Comment 4

[gekacheka]

Q. What does 'invalid' mean?  Does it accept a null value as invalid?  Or just
calIDateTime where datetime.isValid is false?

Maybe change wording to

(If accepts null as invalid)
"If null or aRangeStart.isValid is false, assume "until the end of time"

(If does not accept null as invalid)
If aRangeStart.isValid is false, assume "until the end of time"

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in with both changes made.