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

RESOLVED FIXED in Firefox 54

Status

()

defect
P3
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

(Blocks 1 bug)

unspecified
mozilla54
Unspecified
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(3 attachments)

Now dateFormat.properties is used nsNavHistory only.  I think that we can replace it with ICU.  ICU has a lot of localized month string.
Blocks: 724529
Sure, it would be great to do that and remove dateFormat.properties

is ICU available also if ENABLE_INTL_API is 0?
(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.
Priority: -- → P3
Assignee: nobody → m_kato
:m_kato, my work in bug 1287677 (and its parent bug) may come of help.
(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 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.
Attachment #8838350 - Flags: review?(VYV03354)
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.
Attachment #8838351 - Flags: review?(mak77)
Attachment #8838350 - Flags: review?(VYV03354) → review+
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;
Attachment #8838351 - Flags: review?(mak77) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
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
https://hg.mozilla.org/mozilla-central/rev/87908aced111
https://hg.mozilla.org/mozilla-central/rev/b10f3f8d8d77
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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.
(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.
(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?
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 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.
Attachment #8840814 - Flags: review?(jorgk) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33bc0c088fba
Part 3. Revert the order of kDateFormat. r=jorgk
Depends on: 1342753
You need to log in before you can comment on or make changes to this bug.