1225696 - Use ICU's formatter instead of dateFormat.properties resource bundle

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P3)

Description

[Makoto Kato [:m_kato]]

Now dateFormat.properties is used nsNavHistory only.  I think that we can replace it with ICU.  ICU has a lot of localized month string.

Comment 1

[Marco Bonardo [:mak]]

Sure, it would be great to do that and remove dateFormat.properties

is ICU available also if ENABLE_INTL_API is 0?

Comment 2

[Makoto Kato [:m_kato]]

(In reply to Marco Bonardo [::mak] from comment #1)
> Sure, it would be great to do that and remove dateFormat.properties
> 
> is ICU available also if ENABLE_INTL_API is 0?

ICU available when ENABLE_INTL_API is defined.  If not, it isn't defined.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

:m_kato, my work in bug 1287677 (and its parent bug) may come of help.

Comment 4

[Makoto Kato [:m_kato]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #3)
> :m_kato, my work in bug 1287677 (and its parent bug) may come of help.

Thanks, but places uses C++ code unfortunately.

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Part 1. Add kDateFormatMonthLong and kDateFormatYearMonthLong to DateTimeFormat

Comment 6

[Makoto Kato [:m_kato]]

Attachment: Part 2. Use DateTimeFormat instead of dateFormat.properties

Comment 7

[Makoto Kato [:m_kato]]

Comment on attachment 8838350 [details] [diff] [review]
Part 1. Add kDateFormatMonthLong and kDateFormatYearMonthLong to DateTimeFormat

Places requires long format of month name (January) and year mongth (January 2010).  So I want this feature to DateTimeFormat to remove localization string.

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8838351 [details] [diff] [review]
Part 2. Use DateTimeFormat instead of dateFormat.properties

When DateTimeFormat supports Long format of month and year-month by part 1's fix, we should use it instead of L10N properties.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8838351 [details] [diff] [review]
Part 2. Use DateTimeFormat instead of dateFormat.properties

Review of attachment 8838351 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you, this is much better!

::: toolkit/components/places/nsNavHistory.cpp
@@ +4217,5 @@
> +                                                     kTimeFormatNone,
> +                                                     &aTime,
> +                                                     month);
> +  if (NS_FAILED(rv)) {
> +    aResult = nsPrintfCString("[%d]", aTime.tm_month);

Should likely be aTime.tm_month + 1 since tm_month is 0-based. (the previous code was adding 1 in the caller)

@@ +4233,5 @@
> +                                                     kTimeFormatNone,
> +                                                     &aTime,
> +                                                     monthYear);
> +  if (NS_FAILED(rv)) {
> +    aResult.AppendLiteral("finduri-MonthYear");

This was already horrible, maybe we should instead:
aResult = nsPrintfCString("[%d-%d]", aTime.tm_month + 1, tm.tm_year);

::: toolkit/components/places/nsNavHistory.h
@@ -207,5 @@
>     * These functions return non-owning references to the locale-specific
>     * objects for places components.
>     */
>    nsIStringBundle* GetBundle();
> -  nsIStringBundle* GetDateFormatBundle();

You can also remove mDateFormatBundle;

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87908aced111
Part 1. Add kDateFormatMonthLong and kDateFormatYearMonthLong to DateTimeFormat. r=emk
https://hg.mozilla.org/integration/mozilla-inbound/rev/b10f3f8d8d77
Part 2. Use DateTimeFormat instead of dateFormat.properties. r=mak

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87908aced111
https://hg.mozilla.org/mozilla-central/rev/b10f3f8d8d77

Comment 12

[Jorg K (CEST = GMT+2)]

Why did you insert the new options in the middle?
https://hg.mozilla.org/mozilla-central/rev/87908aced111#l2.8
enum
 {
     kDateFormatNone = 0,            // do not include the date  in the format string
     kDateFormatLong,                // provides the long date format for the given locale
     kDateFormatShort,               // provides the short date format for the given locale
     kDateFormatYearMonth,           // formats using only the year and month 
+    kDateFormatYearMonthLong,       // long version of kDateFormatYearMonth
+    kDateFormatMonthLong,           // long format of month name only
     kDateFormatWeekday              // week day (e.g. Mon, Tue)
 };

Thunderbird users used kDateFormatWeekday==4 and now they need to switch to 6.

Comment 13

[Makoto Kato [:m_kato]]

(In reply to Jorg K (GMT+1) from comment #12)
> Why did you insert the new options in the middle?
> https://hg.mozilla.org/mozilla-central/rev/87908aced111#l2.8
> enum
>  {
>      kDateFormatNone = 0,            // do not include the date  in the
> format string
>      kDateFormatLong,                // provides the long date format for
> the given locale
>      kDateFormatShort,               // provides the short date format for
> the given locale
>      kDateFormatYearMonth,           // formats using only the year and
> month 
> +    kDateFormatYearMonthLong,       // long version of kDateFormatYearMonth
> +    kDateFormatMonthLong,           // long format of month name only
>      kDateFormatWeekday              // week day (e.g. Mon, Tue)
>  };
> 
> Thunderbird users used kDateFormatWeekday==4 and now they need to switch to
> 6.

I will change to tail if you want to keep number on c-c.

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to Makoto Kato [:m_kato] from comment #13)
> I will change to tail if you want to keep number on c-c.
Thanks.

These formats are little used, however, we had some complaints in bug 1325745 about these formats not working after bug 1301640. Users where happy that they started working again after bug 1329841, and then there's been some confusion after this bug here. 

The numbers are unofficially documented here: http://kb.mozillazine.org/Date_display_format

So the argument for keeping the numbers is that users don't have to change anything. But there are also arguments for changing the numbers and keeping what you have now:

Firstly, the order appears more logical. Secondly, we have
if ( ( res >= kDateFormatNone ) && ( res <= kDateFormatWeekday ) )
https://dxr.mozilla.org/comm-central/rev/c207ee1de2c7aaf9f375328c49e53b95785a8991/mailnews/base/src/nsMsgDBView.cpp#7893
so unless we change this, the new formats wouldn't be available to TB users. That said, kDateFormatYearMonth, kDateFormatYearMonthLong and kDateFormatMonthLong aren't all that useful for date display in TB's thread pane. 

So I see three options:
1) Leave it as it is now.
2) Reinstate the previous numbers
 
enum
 {
     kDateFormatNone = 0,            // do not include the date  in the format string
     kDateFormatLong,                // provides the long date format for the given locale
     kDateFormatShort,               // provides the short date format for the given locale
     kDateFormatYearMonth,           // formats using only the year and month 
     kDateFormatYearMonthLong = 5,   // long version of kDateFormatYearMonth
     kDateFormatMonthLong = 6,       // long format of month name only
     kDateFormatWeekday = 4          // week day (e.g. Mon, Tue)
 };

3) Bite the bullet and do it right:

// Options 0 to 3 are available in Mailnews.
enum
 {
     kDateFormatNone = 0,            // do not include the date  in the format string
     kDateFormatLong,                // provides the long date format for the given locale
     kDateFormatShort,               // provides the short date format for the given locale
     kDateFormatWeekday,             // week day (e.g. Mon, Tue)
     kDateFormatYearMonth,           // formats using only the year and month 
     kDateFormatYearMonthLong,       // long version of kDateFormatYearMonth
     kDateFormatMonthLong            // long format of month name only
 };

What do you think?

Comment 15

[Makoto Kato [:m_kato]]

If we shouldn't change order, we should add the comment of it and if value has specific number, we should set number to value.
But about this case, we should add the comment.

Comment 16

[Makoto Kato [:m_kato]]

Attachment: Part 3. Revert the order of kDateFormat.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8840814 [details] [diff] [review]
Part 3. Revert the order of kDateFormat.

Review of attachment 8840814 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. I don't think I have review rights here, but you can have my approval. Sorry about the nit-picking.

::: intl/locale/nsIScriptableDateFormat.idl
@@ +8,5 @@
>  typedef long nsDateFormatSelector;
>  %{ C++
>  enum
>  {
> +    // XXX DON'T CHANGE THE ORDER OF THIS VALUE (See bug 1225696)

Maybe better not to shout ;-)
Typically XXX marks locations in the code which require further work, which is not the case here. I suggest:

// Do not change the order of the values below (see bug 1225696).

@@ +13,4 @@
>      kDateFormatNone = 0,            // do not include the date  in the format string
>      kDateFormatLong,                // provides the long date format for the given locale
>      kDateFormatShort,               // provides the short date format for the given locale
>      kDateFormatYearMonth,           // formats using only the year and month 

Let's remove the trailing space while we're here.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33bc0c088fba
Part 3. Revert the order of kDateFormat. r=jorgk

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/33bc0c088fba