Closed Bug 322768 Opened 14 years ago Closed 14 years ago

use calIDateTimeFormatter in the UI

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mvl, Assigned: jminta)

References

Details

Attachments

(1 file)

The UI should use calIDateTimeFormatter, so that we can remove the code from dateUtils.js, and only have one version of the formatting code.
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. :-)
Assignee: base → jminta
Status: NEW → ASSIGNED
Attachment #213402 - Flags: first-review?(mvl)
Blocks: 329582
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.
(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.
(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? 
(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.
Blocks: 306157
Comment on attachment 213402 [details] [diff] [review]
remove dateUtils.js

ok, r=mvl
Attachment #213402 - Flags: first-review?(mvl) → first-review+
Patch checked in.  I'm guessing we're going to need some follow-up bugs for substantive differences in presentation.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.