use calIDateTimeFormatter in the UI

RESOLVED FIXED

Status

Calendar
Internal Components
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Michiel van Leeuwen (email: mvl+moz@), Assigned: Joey Minta)

Tracking

Dependency tree / graph

Details

Attachments

(1 attachment)

23.39 KB, patch
Michiel van Leeuwen (email: mvl+moz@)
: first-review+
Details | Diff | Splinter Review
The UI should use calIDateTimeFormatter, so that we can remove the code from dateUtils.js, and only have one version of the formatting code.
(Assignee)

Comment 1

12 years ago
Created attachment 213402 [details] [diff] [review]
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. :-)
Assignee: base → jminta
Status: NEW → ASSIGNED
Attachment #213402 - Flags: first-review?(mvl)
(Assignee)

Updated

12 years ago
Blocks: 329582
(Reporter)

Comment 2

12 years ago
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.
(Assignee)

Comment 3

12 years ago
(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.
(Reporter)

Comment 4

12 years ago
(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? 
(Assignee)

Comment 5

12 years ago
(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.
(Assignee)

Updated

12 years ago
Blocks: 306157
(Reporter)

Comment 6

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

ok, r=mvl
Attachment #213402 - Flags: first-review?(mvl) → first-review+
(Assignee)

Comment 7

12 years ago
Patch checked in.  I'm guessing we're going to need some follow-up bugs for substantive differences in presentation.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.