1329841 - Re-implement kDateFormatYearMonth and kDateFormatWeekday

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

[Masatoshi Kimura [:emk]]

+++ This bug was initially created as a clone of Bug #1301640 +++

kDateFormatYearMonth and kDateFormatWeekday are removed because they are not used in Firefox tree, but they are still used in Thunderbird. We should consider re-implementing them.

Note: although Mozilla-employees are not supposed to care about Thunderbird, I'm not a Mozilla-employee :)

Comment 1

[Masatoshi Kimura [:emk]]

(In reply to Jorg K (GMT+1) from bug 1301640 comment #161)
> Can you put support back? Does ICU support this? Here
> https://dxr.mozilla.org/comm-central/rev/0d823cf54df53e0cea75a74adebace956bd333d8/mozilla/intl/locale/DateTimeFormatICU.cpp#82
> I can only see three types: long, short, none.

We can use UDAT_PATTERN to implement custom format.

Comment 2

[Masatoshi Kimura [:emk]]

Gregory, are you interested in working on this?

Comment 3

[Gregory Moore [:gmoore]]

Yeah, definitely. By the way, in the cases where we use UDAT_PATTERN we will need to have separate UDateFormat objects for the date and the time. Therefore when the user chooses kDateFormatYearMonth or kDateFormatWeekday as the date format, and something other than UDAT_NONE for the time format, we won't have the "at" connecting the date and time. Not sure if that matters or not.

Comment 4

[Gregory Moore [:gmoore]]

Attachment: Added support into DateTimeFormat*.cpp for kDateFormatYearMonth and kDateFormatWeekday date format selectors.

Here's the patch; let me know if you see any problems.

Also, should we remove the kTimeFormatSecondsForce24Hour and kTimeFormatNoSecondsForce24Hour options from nsIScriptableDateFormat.idl?:
https://dxr.mozilla.org/mozilla-central/source/intl/locale/nsIScriptableDateFormat.idl#27
We no longer support those options. As far as I know Thunderbird isn't using those options either.

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 8825982 [details] [diff] [review]
Added support into DateTimeFormat*.cpp for kDateFormatYearMonth and kDateFormatWeekday date format selectors.

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

::: intl/locale/DateTimeFormatICU.cpp
@@ +128,5 @@
> +    dateTimeFormat = udat_open(timeStyle, dateStyle, mLocale->get(), nullptr, -1, nullptr, -1, &status);
> +  }
> +  else {
> +    if (aDateFormatSelector == kDateFormatYearMonth) {
> +      dateFormat = udat_open(UDAT_PATTERN, dateStyle, mLocale->get(), nullptr, -1, L"yyyy/MM", -1, &status);

Using a literal pattern like this doesn't account for the fact that the locale might call for a different separator (or even a different ordering). A better solution, I think, would be to use udatpg_getBestPattern to convert a skeleton such as "yyyyMM" into the appropriate pattern for the locale.

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to Gregory Moore from comment #3)
> Yeah, definitely. By the way, in the cases where we use UDAT_PATTERN we will
> need to have separate UDateFormat objects for the date and the time.
> Therefore when the user chooses kDateFormatYearMonth or kDateFormatWeekday
> as the date format, and something other than UDAT_NONE for the time format,
> we won't have the "at" connecting the date and time. Not sure if that
> matters or not.

It should be possible to improve this by getting the pattern from the chosen time style (udat_toPattern), extract the pattern skeleton from that (udatpg_getSkeleton), and combine that with the appropriate skeleton for the date part; then use udatpg_getBestPattern to turn the combined date+time skeleton back into a locale-specific pattern.

Comment 7

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

Would it be hard for Thunderbird to move to use Intl API from JS? I feel like we should aim to move away from doing such front-end formatting in the C++ code.

Comment 8

[Masatoshi Kimura [:emk]]

Jorg, see comment #7. If you move to use JS Intl API, I'll close this bug as WONTFIX.

Comment 9

[Jorg K (CEST = GMT+2)]

Frankly, I don't know. We do a whole lot of date/time formatting in C++ code. That can't be changed to JS in a hurry. What is that JS Intl API? Can that be called from C++ without a performance hit? We have this date column and if the e-mail folder/view has a bazillion messages, the date formatting is called an awful lot.

Counter question: How hard would it be to move FF certificate details to that interface. Now multiply by eight call sites we have and the staff ratio "FF vs. TB". And, as I said, keep in mind that FF would display *one* certificate date whereas we display dates for many messages.

As I said before, those two additional date formats aren't essential, but as there was will to re-implement them, they would be nice to have to keep supporting the (few?) users who currently use them. There don't seem to be a lot of changes in the patch and exercising UDAT_PATTERN might be a good thing to have in the box.

Comment 10

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

> What is that JS Intl API?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl#Locale_identification_and_negotiation

spec:

https://www.ecma-international.org/ecma-402/3.0/index.html

> As I said before, those two additional date formats aren't essential, but as there was will to re-implement them, they would be nice to have to keep supporting the (few?) users who currently use them.

Yeah, makes sense.

I definitely will want to move the certificate call eventually, but until then, we can keep the C++ APIs and since the patch is here, we can as well fix the regression.

But I would recommend TB to start moving Intl formatting toward JS as I do believe that we will want to move Gecko Intl to be in JS.

Comment 11

[Gregory Moore [:gmoore]]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> Using a literal pattern like this doesn't account for the fact that the
> locale might call for a different separator (or even a different ordering).
> A better solution, I think, would be to use udatpg_getBestPattern to convert
> a skeleton such as "yyyyMM" into the appropriate pattern for the locale.

Okay, thanks for the information. That's a good idea. 

However, if you look at the old nsDateTimeFormat*.cpp files, they all use literal patterns (such as "yyyy/MM") for kDateFormatYearMonth. So it doesn't seem like we would be really be regressing in that respect (and it would be simpler). However, yeah, it would be more correct if we did it the way you mentioned.

It seems like it's okay to go ahead with this patch for now. Should I work on implementing the improvements mentioned by :jfkthame in comment 5 and comment 6, or should we just go with the patch how it is?

Comment 12

[Masatoshi Kimura [:emk]]

(In reply to Gregory Moore from comment #4)
> Also, should we remove the kTimeFormatSecondsForce24Hour and
> kTimeFormatNoSecondsForce24Hour options from nsIScriptableDateFormat.idl?:
> https://dxr.mozilla.org/mozilla-central/source/intl/locale/
> nsIScriptableDateFormat.idl#27
> We no longer support those options. As far as I know Thunderbird isn't using
> those options either.

Let's remove unused constants.

(In reply to Gregory Moore from comment #11)
> It seems like it's okay to go ahead with this patch for now. Should I work
> on implementing the improvements mentioned by :jfkthame in comment 5 and
> comment 6, or should we just go with the patch how it is?

I think you should implement the improvement now rather than making a double effort.

Comment 13

[Gregory Moore [:gmoore]]

Attachment: Changed code to get the best format pattern from a skeleton, instead of using a literal pattern.

Alright, here is a patch with the improvements discussed above implemented. It's kind of messy; there's a lot of checking for buffer overflows and stuff, so there might be some improvements to be made there. It seems to work though. Let me know if you have any suggestions. Thanks.

Comment 14

[Masatoshi Kimura [:emk]]

Comment on attachment 8835132 [details] [diff] [review]
Changed code to get the best format pattern from a skeleton, instead of using a literal pattern.

Thank you for the updated patch.

You can just remove DateTimeFormatUnix.cpp (and optionally rename DateTimeFormatICU.cpp) because bug 1215247 has been (finally!) fixed.

Comment 15

[Masatoshi Kimura [:emk]]

Also, please add some basic unit tests for kDateFormatYearMonth and kDateFormatWeekday.

Comment 16

[Gregory Moore [:gmoore]]

(In reply to Masatoshi Kimura [:emk] from comment #14)
> You can just remove DateTimeFormatUnix.cpp (and optionally rename
> DateTimeFormatICU.cpp) because bug 1215247 has been (finally!) fixed.

Okay cool, I will do that.

> Also, please add some basic unit tests for kDateFormatYearMonth and
> kDateFormatWeekday.

Okay. What kind of tests should I include? So far I'm thinking:
a) Test Format*Time() (one of them) with aDateFormatSelector = kDateFormatYearMonth, and set aTimeFormatSelector to each of its possible values.
b) Test Format*Time() with aDateFormatSelector = kDateFormatWeekday, and set aTimeFormatSelector to each of its possible values.
We would set mLocale to a fixed value ("en-US") before the tests as well.

Besides that I'm not really sure what else to test though. I could try different time values but I'm not sure how much that would matter. Let me know if there are other cases I should test or if some of mine are unnecessary. Thanks.

Comment 17

[Masatoshi Kimura [:emk]]

(In reply to Gregory Moore from comment #16)
> Okay. What kind of tests should I include? So far I'm thinking:
> a) Test Format*Time() (one of them) with aDateFormatSelector =
> kDateFormatYearMonth, and set aTimeFormatSelector to each of its possible
> values.
> b) Test Format*Time() with aDateFormatSelector = kDateFormatWeekday, and set
> aTimeFormatSelector to each of its possible values.
> We would set mLocale to a fixed value ("en-US") before the tests as well.

I think this is sufficient.

Comment 18

[Gregory Moore [:gmoore]]

Attachment: Added unit test and removed DateTimeFormatUnix.cpp.

Okay, here is the updated patch. By the way, in the calls to udat_toPattern(), we are passing in TRUE for the localized argument, which tells it to localize the pattern. Since we are just turning the pattern into a skeleton after this, it doesn't really seems like it matters what this is set to (and if I change it to FALSE, I still seem to get the same results). I just wanted to check if it would possibly make more sense (or be more efficient) to pass in FALSE. 

Also, should I just get rid of the PATTERN_SKELETON_INITIAL_LEN #define, and replace it's uses with DATETIME_FORMAT_INITIAL_LEN? I think when I originally wrote that I thought it might have a smaller value than 127, but since DATETIME_FORMAT_INITIAL_LEN is also 127, we could probably just have a single define. Let me know, thanks.

Comment 19

[Masatoshi Kimura [:emk]]

Comment on attachment 8836247 [details] [diff] [review]
Added unit test and removed DateTimeFormatUnix.cpp.

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

(In reply to Gregory Moore from comment #18)
> Created attachment 8836247 [details] [diff] [review]
> Added unit test and removed DateTimeFormatUnix.cpp.
> 
> Okay, here is the updated patch. By the way, in the calls to
> udat_toPattern(), we are passing in TRUE for the localized argument, which
> tells it to localize the pattern. Since we are just turning the pattern into
> a skeleton after this, it doesn't really seems like it matters what this is
> set to (and if I change it to FALSE, I still seem to get the same results).
> I just wanted to check if it would possibly make more sense (or be more
> efficient) to pass in FALSE. 

udat_toPattern uses toLocalizedPattern if localized is TRUE. Otherwise it uses toPattern. But toLocalizedPattern and toPattern have no difference unless you set localized pattern characters:
http://www.icu-project.org/apiref/icu4c/classicu_1_1SimpleDateFormat.html#a48fd316bf28541f20449050bde094855
So you can use FALSE to make the function a bit faster.

> Also, should I just get rid of the PATTERN_SKELETON_INITIAL_LEN #define, and
> replace it's uses with DATETIME_FORMAT_INITIAL_LEN? I think when I
> originally wrote that I thought it might have a smaller value than 127, but
> since DATETIME_FORMAT_INITIAL_LEN is also 127, we could probably just have a
> single define. Let me know, thanks.

I think you can unify defines. Also, you should use const instead of #define.

Comment 20

[Masatoshi Kimura [:emk]]

You don't have to request ni every time you ask.

Comment 21

[Gregory Moore [:gmoore]]

Attachment: Replaced PATTERN_SKELETON_INITIAL_LEN with DATETIME_FORMAT_INITIAL_LEN and changed localized argument to FALSE.

(In reply to Masatoshi Kimura [:emk] from comment #20)
> You don't have to request ni every time you ask.

Oh okay, sorry about that. Here is the patch with the changes. Thanks.

Comment 22

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25be360f850f3047affa3dbde06a17f4c1d42ece

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89dd7740d001
Added support into DateTimeFormat*.cpp for kDateFormatYearMonth and kDateFormatWeekday date format selectors. r=emk

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89dd7740d001

Comment 25

[Jorg K (CEST = GMT+2)]

Gregory and Masatoshi-san: Many thanks for implementing this.

With the latest build of Thunderbird I set mail.ui.display.dateformat.thisweek to 4. So for messages received this week I now see for example "Sun 17:41" since Sunday was yesterday.