322230 - need an xpcom service to format a datetime

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

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

There is code to format a datetime in dateUtils.js, but that can't be called from xpcom components (like the exporters). We should convert the code into a proper xpcom service.

Comment 1

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

Attachment: patch v1

Ported the code. Didn't convert any callers yet.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 207427 [details] [diff] [review]
patch v1

In general, this looks good.  Most of my review comments here are nits.

The boilerplate for this IDL claims that it's Oracle Corporation code.  I bet it's not.  :-)

>+[scriptable, uuid(17101743-f016-4fb2-90dd-0c043aa44579)]
>+interface calIDateTimeFormatter : nsISupports

There are a number of methods in this interface where the doxygen comments don't match the method name (mostly short vs long).

formatDateTime looks like it wants some doxygen love.

>+    // XXX using wstring instead of AString, because AString as outparam
>+    //     seems to fail in javascript.
>+    void formatInterval(in calIDateTime aStartDate,
>+                        in calIDateTime aEndDate,
>+                        out wstring aStartString,
>+                        out wstring aEndString);

If that commentary is true, that sounds like a pretty serious regression.  Shaver claims this shouldn't be happening.  Can you come up with an isolated test case?

>Index: base/src/calDateTimeFormatter.js
>===================================================================
>RCS file: base/src/calDateTimeFormatter.js
>diff -N base/src/calDateTimeFormatter.js
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ base/src/calDateTimeFormatter.js	3 Jan 2006 18:23:42 -0000
>
>+ * The Original Code is Oracle Corporation code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ *  Michiel van Leeuwen <[e-mail]>
>+ * Portions created by the Initial Developer are Copyright (C) 2006
>+ * the Initial Developer. All Rights Reserved.

I think the boilerplate for this file probably needs to be inherited from the original pre-ported code and then have your name appended to it.

>+    // If LONG FORMATTED DATE is same as short formatted date,
>+    // then OS has poor extended/long date config, so use workaround.
>+    this.mUseLongDateService = true;
>+    var probeDate = 
>+        Components.classes["@mozilla.org/calendar/datetime;1"]
>+                  .createInstance(Components.interfaces.calIDateTime);
>+    probeDate.jsDate = new Date(2002,3-1,4);

Why |3-1| here?

>+calDateTimeFormatter.prototype.formatDate =
>+function formatDate(aDate) {
>+    // Format the date using user's format preference (long or short)
>+    var format;
>+    var prefBranch = Components.classes["@mozilla.org/preferences-service;1"]
>+                                .getService(Components.interfaces.nsIPrefBranch);
>+    try {     
>+        format = prefBranch.getIntPref("calendar.date.format");
>+    } catch(e) {
>+        format = 0;
>+    }
>+
>+    if (format == 0)
>+        return this.formatDateLong(aDate);
>+    else
>+        return this.formatDateShort(aDate);
>+};

FWIW, i'd vote for this being "long" or "short" for pref file readability, if there aren't already clients out there that depend on the existing syntax.

>+
>+calDateTimeFormatter.prototype.formatDateWithoutYear =
>+function formatDateWithoutYear(aDate) {
>+    // Format the date using a hardcoded format for now, since everything
>+    // that displays the date uses this function we will be able to 
>+    // make a user settable date format and use it here.
>+    return this.shortMonthName(aDate.month) + " " + aDate.day;
>+};

I don't understand what the above comment means.

>+
>+calDateTimeFormatter.prototype.formatDateTime =
>+function formatDateTime(aDate) {
>+    var formattedDate = this.formatDate(aDate);
>+    var formattedTime = this.formatTime(aDate);
>+
>+    var timeBeforeDate;
>+    var prefBranch = Components.classes["@mozilla.org/preferences-service;1"]
>+                                .getService(Components.interfaces.nsIPrefBranch);
>+    try {     
>+        timeBeforeDate = prefBranch.getBoolPref("calendar.date.formatTimeBeforeDate");
>+    } catch(e) {
>+        timeBeforeDate = 0;
>+    }

Time before date isn't a piece of information that we can somehow extract from the locale?

>+calDateTimeFormatter.prototype.formatInterval =
>+function formatInterval(aStartDate, aEndDate, aStartString, aEndString) {

If you could give each clause in this method a bit of commentary, it would make for easier reading/reviewing.

Comment 3

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

Attachment: patch v2

Updated the patch to the review comments, except for changing the pref, because that pref is already in use, and i don't want to write a pref migrator just for this. Also didn't remove the formatTimeBeforeDate pref, because it isn't really a locale thing, but a user pref thing (and it was like that in the old code...)

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 207651 [details] [diff] [review]
patch v2

>+calDateTimeFormatter.prototype.formatInterval =
>+function formatInterval(aStartDate, aEndDate, aStartString, aEndString) {
>+    endDate = aEndDate.clone();
>+    if (aStartDate.isDate) { 
>+        endDate.day -= 1;
>+        endDate.normalize();
>+    }

I assume this is because end-date is exclusive rather than inclusive.  Can you add a comment to that effect?

Otherwise, looks great.  r=dmose.

Comment 5

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

patch checked in. Actually using this is bug 322768

Comment 6

[Stefan Sitter [:ssitter]]

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