Closed
Bug 1225696
Opened 8 years ago
Closed 7 years ago
Use ICU's formatter instead of dateFormat.properties resource bundle
Categories
(Toolkit :: Places, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: m_kato, Assigned: m_kato)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
4.86 KB,
patch
|
emk
:
review+
|
Details | Diff | Splinter Review |
10.44 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
1.36 KB,
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
Sure, it would be great to do that and remove dateFormat.properties is ICU available also if ENABLE_INTL_API is 0?
Assignee | ||
Comment 2•8 years ago
|
||
(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.
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → m_kato
Comment 3•8 years ago
|
||
:m_kato, my work in bug 1287677 (and its parent bug) may come of help.
Assignee | ||
Comment 4•8 years ago
|
||
(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.
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Comment 6•7 years ago
|
||
Assignee | ||
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8838350 -
Flags: review?(VYV03354) → review+
Comment 9•7 years ago
|
||
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+
Comment 10•7 years ago
|
||
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
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/87908aced111 https://hg.mozilla.org/mozilla-central/rev/b10f3f8d8d77
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 12•7 years ago
|
||
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.
Assignee | ||
Comment 13•7 years ago
|
||
(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•7 years ago
|
||
(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?
Assignee | ||
Comment 15•7 years ago
|
||
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.
Assignee | ||
Comment 16•7 years ago
|
||
Attachment #8840814 -
Flags: review?(jorgk)
Comment 17•7 years ago
|
||
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+
Comment 18•7 years ago
|
||
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
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/33bc0c088fba
You need to log in
before you can comment on or make changes to this bug.
Description
•