296659 - calIDateTime needs methods to convert to/from jsDate by fields [else wrong timezone offset]

Status: RESOLVED WORKSFORME

(Calendar :: Internal Components, defect)

Description

[gekacheka]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: trunk

(bug to fork discussion following bug 286070 comment 14 and bug 286070 comment 30) 

Currently calDateTime converts to/from jsDates using absolute millisecond times.
 For example, in London new Date(0) produces the date with fields 1970-01-01
00:00, and in New York it produces the date with the fields 1969-12-31 19:00,
but both correspond to the same time, just in different timezones.  This can be
used by applications that compare jsDates that originated in calDateTimes with
different timezones, and applications that display calDateTimes with different
timezones in the the current OS timezone.

calDateTime also needs to be able to convert to/from jsDates by fields.  For
example, to produce the yyyy-MM-dd HH:MM fields 1970-01-01 00:00 regardless of
what the current OS timezone is.  This is needed for the floating timezone, and
for any timezone other than the current OS timezone.  Applications can use these
dates to call libraries for jsDates that depend on the field values.  Existing
libraries include those used for formatting and parsing localized dates, and the
date time pickers.

A starting proposal is to add methods with the following signature and semantics
(specified below in javascript) to calIDateTime.idl and calDateTime.cpp.

  /**
     Create a jsDate with same y/M/d/h/m/s field values as this calDateTime.
     For displaying dates not in current timezone, including floating timezone.
     In the result jsDate:
       Timezone may be incorrect (jsDates may only have OS timezone).
       Therefore, the milliseconds-since-1970-utc values returned by 
         result.valueOf() and result.getTime() may be offset by
         the timezone difference between the calDateTime and the OS timezone.
       So do not compare result with jsDates from other timezones.
  **/
  getJSDateByFields: function getJSDateByFields() {
    if (this.isDate) {
      return new Date(this.year,
                      this.month,
                      this.day);
    } else {
      return new Date(this.year,
                      this.month,
                      this.day,
                      this.hour,
                      this.minute,
                      this.second);
    }
  }                         

  /**
      Set this calDateTime from the y/M/d/h/m/s field values of the jsDate.
      (jsDate timezone is fixed at the os timezone,
       so just use its field values, not the millisecond time,
       esp. for floating dates, which are read from msec like utc dates.)
      @param isDate is optional.  If true, sets this.isDate and
      sets hour, minute, and second to zero.
      @param timezone is optional.  Default is "floating".
      (The current OS timezone might be an alternative default, but
      there currently is no easy way to get its timezone id.)
  **/
  setByJSDateFields: function setByJSDateFields(jsDate, isDate, timezone) {
    this.timezone = timezone || "floating";
    this.isDate   = isDate;
    this.year     = jsDate.getFullYear();
    this.month    = jsDate.getMonth();
    this.day      = jsDate.getDate(); // date of month, not day of week
    this.hour     = (isDate? 0 : jsDate.getHours());
    this.minute   = (isDate? 0 : jsDate.getMinutes());
    this.second   = (isDate? 0 : jsDate.getSeconds());
    this.normalize();
  }

Issue: why not use a property like calDateTime.jsDateByFields = new
Date(1970,0,1)?  
  A method is an obvious way to document multiple parameters.  jsDate does not
have existing properties for isDate or timezone.  
  An alternative would be to add the properties to the jsDate object such as
    var d = new Date(1970,0,1);
    d.timezone = "Asia/Tokyo";
    d.isDate = true;
    calDateTime.jsDateByFields = d;
  This has a couple drawbacks:
    It takes 3 statements to make the call, not just one call statement:
      calDateTime.setJSDateByFields(new Date(1970,0,1), true, "Asia/Tokyo");
    d.timezone and d.isDate look like date properties, but they aren't.
      Doing var d2 = new Date(d) does not copy these properties, 
      so d2.timezone is undefined.

Issue: why provide a method with three parameters, why not just use 3 properties?
  To make easier to avoid mistakes.  The order of setting the properties and
calling normalize matters.  For example, if you set the timezone first, then the
h/m/s, and then set isDate to true, it should set the h/m/s to zero in that
timezone.  But if you set the h/m/s first, then set isDate to true, it will set
the h/m/s to zero in the current timezone, and then changing the timezone will
result in a date where the h/m/s fields are not all zero.  (This isn't just
about isDate, the order matters whenever you set the time and the timezone.)

Issue: why not just assign property values, and then normalize when a field is
read?  Won't this guarantee the normalization and zero-setting occur after the
caller has set all the properties?
  If properties are set in separate statements, it appears the properties are
just fields, so some callers may read some fields before setting the remainder.
 For example, they might set the hour/minute/second on the start date, then
initialize the end date by reading the start date, and later set the timezone on
both dates.  In this case, a read occurred before the properties were all set,
so inserting a normalize then could produce mysterious behavior.  
  By providing a method, it is clearer what is the 'safe' way to set the date
from a jsDate and timezone: use the method.
  

Reproducible: Always

Steps to Reproduce:

Comment 1

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

I think we should make .jsDate use the fields, not the msec value, and that just
be it.
To select a different timezone, use getInTimezone().
for isDate, you can just use .isDate. It's the same result as a parameter call.

You can't do normal comparisation on the .jsDate's anymore (if they can be in
different timezones), but for those places we can use calDateTime directly. We
should try to minimize the amount of places that use .jsDate anyway, since it's
just a mess when trying to deal with timezones.

Comment 2

[Mike Shaver (:shaver emeritus)]

Can someone build a dependency graph on this bug for the things that such a
change would fix?  I'm leery of changing how we convert from JS Dates right now,
but it probably makes sense to do so before too long if we're going to do it.

I generally agree with mvl's assertion that we should move away from .jsDate
usage as much as possible, because it really isn't well-suited for the sorts of
complex timezone manipulations we want to perform in our calendar code.

Comment 3

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

The change would fix .jsDate on a floating time. Currently, .jsDate on a
floating datetime pretents the datetime is in UTC, and converts from that, but
then also converts to the local timezone. This causes an offset of a few hours,
and messes up the unifinder and the weekview, at least.

Comment 4

[gekacheka]

added 'timezone offset' to summary to make this bug easier to find.

Comment 5

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

Is this solved by the jsDateToFloatingDateTime(date) function
http://lxr.mozilla.org/seamonkey/source/calendar/resources/content/calendarUtils.js#136
and the change to calDateTime
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=calDateTime.cpp&branch=&root=/cvsroot&subdir=mozilla/calendar/base/src&command=DIFF_FRAMESET&rev1=1.46&rev2=1.47
which makes converting from a floating datetime work correctly?

Comment 6

[gekacheka]

If an app has a date picker (with a jsDate as value) and a timezone picker, or
if an app has parsed some jsDates and timezone, how can it simply create a
calDateTime with those date fields, in that timezone?  It is not obvious from
the interface or class, and it should be.

My concern is to make a single step technique for converting by fields to make
it harder to make mistakes, particularly for people who aren't experienced with
the code and don't know how it works internally.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Bug 326868 has details that will be useful in release-noting this bug.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Cleaning up incorrectly assigned bugs; search for dmosecleanup to get rid of this bugmail.

Comment 10

[Matthew (lilmatt) Willis]

mvl/gekacheka:
Since we can use calDateTime.jsDate(valueFromDatetimepicker) can this be marked FIXED?

Comment 11

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

No, it can't be closed. Nothing has changed since the bug was filed. jsDate is still totally buggy, especially in combination with the timepicker.
(Note the 'by fields' parts in the bug summary. We don't do that)

Comment 12

[Daniel Boelzle [:dbo]]

I think the current approach we are running is ok: feed the (jsDate based) controls floating, and (on the way back) take the raw jsDate's values, omitting the timezone. I've added support for this purpose calUtils's jsDateToDateTime in bug 328442.
As far as I understand this bug, IMO we could not safely fix it, because calDatetime and jsDate operate on different timezone definitions. Due to this, a transition from calDatetime into jsDate and back will potentially falsify the original calDatetime.

Comment 13

[Daniel Boelzle [:dbo]]

gekacheka, what's your opinion on my last comment?

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

I think we can close this bug for now. jsDate might still be broken, but we do the best we can to convert with the correct timezone offset, see calUtils' jsDateToDateTime and calIDateTime's jsDate.