294104 - remove isUtc from calDateTime

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

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

Get rid of isUTC, and have timezone specify "UTC", a tzid, or null for floating
time.  Also get rid of passing null to getInTimezone and friends to return UTC
time, and instead require "UTC" to be passed in.

Comment 1

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

Attachment: remove isUtc patch

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 183552 [details] [diff] [review]
remove isUtc patch

>+  // null
>+  //   - the object is in floating time
>   attribute AUTF8String timezone;

You should probably say something like "empty string (or null from JS)" here,
because C++ callers won't be able to pass null, and they don't need to do the
SetIsVoid dance because you just check for empty.  (Should we just accept
"floating" and "float" here, and avoid having to deal with null/empty terms
everywhere?  It would also make grepping source for floating cases easier,
perhaps usefully.  And since floating is such a strange case, I'd rather have
it be more explicit, and less easily confused with a "default timezone" case,
etc.)

Also, you should probably say that both "UTC" and "utc" are legal, since they
apparently are.

What error is thrown if an unrecognized TZID is passed in?  Should say here.

If I set a time to floating, it floats as the "local" time from its previous
timezone?  And if I set it from floating to a TZ, I just "carry it" into that
TZ by changing its TZ identifier, and not computing any change to
day/hour/minute?  If that's correct, please document it.  (And if it's not
correct, please document it.)

>+  // Sets this instance to aTime seconds from the epoch,
>+  // in the given timezone.
>+  // If aTimezone is null, the time is interpreted as UTC,
>+  // but the object is set to be a floating instance.
>+  // otherwise, aTime is interpreted as being relative to
>+  // the epoch in the given timezone.
>+  void setTimeInTimezone (in PRTime aTime, in AUTF8String aTimezone);

Indicate that the timezone is interpreted as in the |timezone| setter above. 
I'm tempted to make it illegal to pass in null/empty here.  I don't think
"floating with PRTime as UTC" is an interesting use enough use case to let them
skip |.setTimeInTimezone(time, "UTC"); .timezone = null;| and it complicates
the logic.  (Also, if it's floating, it's not _in_ a timezone, so calling
"setTimeInTimezone" seems like a bad bit anyway.)

>   // gets a new calIDateTime object representing this
>   // item in the given timezone
>-  calIDateTime getInTimezone (in string aTimezone);
>+  calIDateTime getInTimezone (in AUTF8String aTimezone);

What does it mean to get it in a "floating" timezone?  I presume that ymdhms
stays the same, and we just strip the TZ tag, but it would be nice to be
explicit about it.

>   // add the given calIDateTime, treating it as a duration, to
>   // this item
>   void addDuration (in calIDateTime aDuration);

(Some note about how timezones are ignored here would be good!)

>   // returns -1, 0, 1 to indicate this < aOtherDate,
>   // this == aOtherDate, or this > aOtherDate.
>   long compare (in calIDateTime aOtherDate);

How do comparisons work if one of the times is floating?  Are they both treated
as floating?  Do we throw if only one is floating?  (That would be my
temptation; the caller should probably explicitly ask for that comparison.)

>     if (mIsDate) {
>         // if it's a date, we really just want to make a copy of this
>         // and set the timezone.
>         cdt = new calDateTime(*this);
>-        if (aTimezone) {
>-            cdt->mTimezone.Assign(aTimezone);
>-            cdt->mIsUtc = PR_FALSE;
>-        } else {
>-            cdt->mTimezone.Truncate();
>-            cdt->mIsUtc = PR_TRUE;
>-        }
>+        cdt->mTimezone.Assign(aTimezone);

Is that true?  I guess the decision is whether getInTimezone preserves "isDate"
or "concurrent range of time".	We should document this behaviour, which seems
reasonable.

>+    icaltimezone* tz = nsnull;
>+    nsresult rv = GetIcalTZ(mTimezone, &tz);
>+    if (NS_FAILED(rv)) {
>+        NS_WARNING("Specified timezone not found; generating floating time!");
>         icalt->zone = nsnull;
>     } else {

This should be an error, IMO, and not a mostly-silent descent into the madness
of floating times.  NS_ERROR_INVALID_ARG is probably
the right thing.

>+    nsresult rv = ics->GetTimezone(tzid, getter_AddRefs(tz));
>+    if (NS_FAILED(rv) || !tz)
>+        return NS_ERROR_FAILURE;

NS_ERROR_INVALID_ARG?  ics->GetTimezone should be returning that for a bogus
TZ, if it's not already, and we should be propagating
it.

>           <![CDATA[
>+dump ("++++ minimonth refreshDisplay\n");
>             if (!this.mInitialized) {
>               this.mInitialized = true;

?

>       <method name="dayClicked">
>         <parameter name="aDay"/>
>         <body>
>           <![CDATA[
>             if (this.mSelected) {
>-              this.mSelected.setAttribute("selected", "");
>+              this.mSelected.removeAttribute("selected");
>             }

It'd be nice to tie all these into an internal _selectItem(null)
sort of call, to reduce the duplication of logic.  (And let me
call it from an external selectItem() for the agenda!)

>-      <field name="mTimezone">null</field>
>+      <field name="mTimezone">"UTC"</field>

WTB: a decent global source for timezone info.

r=shaver if you really do fix everything _and_ not want additional
review, but I bet you'll want to put another patch up and argue
a bit.

Comment 3

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

Attachment: new patch, just calDateTime changes

New patch, with comments addressed -- just calIDateTime/calDateTime changes,
I'll make a separate patch for the fixups for the rest of the code.

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 183949 [details] [diff] [review]
new patch, just calDateTime changes

>-  // returns true if this thing is able to be modified;
>-  // if the item is not mutable, attempts to modify
>-  // any data will throw CAL_ERROR_ITEM_IS_IMMUTABLE
>+  /**
>+   * isMutable is true if this instance is modifiable.
>+   * If isMutable is false, any attempts to modify
>+   * the object will return CAL_ERROR_ITEM_IS_MUTABLE.
>+   */
>   readonly attribute boolean isMutable;

This was better as "will throw CAL_ERROR_ITEM_IS_IMMUTABLE".

>-  // Month, 0-11
>+  /**
>+   * Month, 0-11, 0 = January
>+   */
>   attribute short month;

I think that we'll suffer for zero-basing, but I also think
we'd suffer for one-basing here, since we'd be out of step
with both NSPR and JS date structures.	Ah, life.

>+  /*
>+   * Methods
>+   */

(We have a method or two above, like makeImmutable() and
clone().)

>+  /**
>+   * Return a new calIDateTime instance that's the result of
>+   * converting this one into the given timezone.  Valid values
>+   * for aTimezone are the same as the timezone field; if
>+   * the given timezone is null or an empty string, then
>+   * this method is equivalent to cloning the object and
>+   * setting the timezone to null (that is, no conversion is
>+   * performed, only the timezone is set to null).
>+   */
>+  calIDateTime getInTimezone (in AUTF8String aTimezone);

But cloning the object and setting timezone to null will throw
NS_ERROR_INVALID_ARG.  So maybe just say that null/empty are
illegal, instead of making them work that out for themselves. =)

>-calDateTime::SetTimeInTimezone(PRTime aTime, const char *aTimezone)
>+calDateTime::SetTimeInTimezone(PRTime aTime, const nsACString& aTimezone)

BTW, can TZ identifiers be non-ASCII?

>+    icaltimezone* tz = nsnull;

These little locals are variously named "tz", "zone", or "z".  Consistency
may be for small minds, but it would make my small mind happier.

r=shaver with the "throws" and "null/empty" comment fixes above, at least.

Comment 5

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

Attachment: part 2 of patch, fix up all timezone users

This is the second part of the patch, just fixes up all current callers of
getInTimezone/timezone/etc.

Comment 6

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

checked in