322768 - use calIDateTimeFormatter in the UI

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

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

The UI should use calIDateTimeFormatter, so that we can remove the code from dateUtils.js, and only have one version of the formatting code.

Comment 1

[Joey Minta]

Attachment: remove dateUtils.js

This patch removes all the instances in current/updated code of DateFormater() and replaces them with the calIDateTimeFormatter service.  It also fixes 2 bugs in the mouseoverPreviews, namely the typo in todo.dueDate and the fact that times could be off with respect to timezone due to the use of jsDate.  (We get to remove a few more instances of jsDate use, although there are more in mouseoverPreviews that can now be removed.  I left them in to keep the patch smaller.)

Low priority patch.  No impact on Lightning.  DateUtils.js is 30k, so there may be some minor download size improvement.

Also brings us one step closer to finally removing gCalendarWindow. :-)

Comment 2

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

Comment on attachment 213402 [details] [diff] [review]
remove dateUtils.js

>-      boxAppendLabeledDateTimeInterval(vbox, "tooltipDate",
>-                                       startDate, endDate, event.startDate.isDate,
>-                                       relativeToDate);
>+      boxAppendLabeledDateTimeInterval(vbox, "tooltipDate", startDate, endDate);

Why is the relativeToDate parameter not needed anymore? (I'm not even sure what it used to do, but it looked like it had a reason to be there)

>-function boxAppendLabeledDateTime(box, labelProperty, date, isAllDay )
>+function boxAppendLabeledDateTime(box, labelProperty, date)

Those dates are now calIDateTime. Good.

>-function boxAppendLabeledDateTimeInterval(box, labelProperty, start, end, isAllDay, relativeToDate)
>+function boxAppendLabeledDateTimeInterval(box, labelProperty, start, end)
> {
>-  var formattedInterval = gCalendarWindow.dateFormater.formatInterval( start, end, isAllDay, relativeToDate);
>-  boxAppendLabeledText(box, labelProperty, formattedInterval);
>+  var dateFormatter = Components.classes["@mozilla.org/calendar/datetime-formatter;1"]
>+                                .getService(Components.interfaces.calIDateTimeFormatter);
>+  var startString = new Object();
>+  var endString = new Object();
>+  start = jsDateToDateTime(start).getInTimezone(calendarDefaultTimezone());
>+  end = jsDateToDateTime(end).getInTimezone(calendarDefaultTimezone());

But those are still jsDates. Can you change that? If not, please use a different var name for the converted dates. Using the same is confusing to read.

Comment 3

[Joey Minta]

(In reply to comment #2)
> (From update of attachment 213402 [details] [diff] [review] [edit])
> >-      boxAppendLabeledDateTimeInterval(vbox, "tooltipDate",
> >-                                       startDate, endDate, event.startDate.isDate,
> >-                                       relativeToDate);
> >+      boxAppendLabeledDateTimeInterval(vbox, "tooltipDate", startDate, endDate);
> 
> Why is the relativeToDate parameter not needed anymore? (I'm not even sure what
> it used to do, but it looked like it had a reason to be there)
> 
According to the new calIDateFormatter.idl, there is no longer a 3rd parameter accepted for formatInterval.  So, I dropped it because I didn't have a choice.  From what I understand relativeToDate would drop the date part of a datetime if the dates in question fell on the same date as the relitiveToDate, and only return the times.  I could be wrong though.

Comment 4

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

(In reply to comment #3)
> According to the new calIDateFormatter.idl, there is no longer a 3rd parameter
> accepted for formatInterval.  So, I dropped it because I didn't have a choice. 

The interface isn't frozen, so we can still add that parameter, if it makes sense. What is final visible difference that's caused by dropping it? 

Comment 5

[Joey Minta]

(In reply to comment #4)
> (In reply to comment #3)
> > According to the new calIDateFormatter.idl, there is no longer a 3rd parameter
> > accepted for formatInterval.  So, I dropped it because I didn't have a choice. 
> 
> The interface isn't frozen, so we can still add that parameter, if it makes
> sense. What is final visible difference that's caused by dropping it? 
> 
Looking more closely, I think I'm ok with the new formatting code as it is.  The effective result of the old code was to append just the times of an event if it  only spanned one day.  This means that for most events, the old code would create a tooltip that says "Date: 10:00am - 11:00am" while the new code's result is "Date: 20 March 2006 10:00am - 11:00am".  I think that the additional information in the new code's result is helpful, so I vote for keeping things as is in this patch.

Comment 6

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

Comment on attachment 213402 [details] [diff] [review]
remove dateUtils.js

ok, r=mvl

Comment 7

[Joey Minta]

Patch checked in.  I'm guessing we're going to need some follow-up bugs for substantive differences in presentation.