Follow-up to bug 1229684 to restore the pervious behaviour of considering the OS regional settings for datetime formatting for Calendar once the last patch for that bug landed. Meanwhile mozIntl is available do do this, unfortunately this is only available for chrome code and probably at least for email invitation preview dates/times we need some logic to mock this for consistency. Nevertheless, for most of the use cases using Services.intl.createDateTimeFormat(undefined, { dateStyle: "xxx", timeStyle: "xxx" }); in the datetimeformatter instead with xxx = long|medium|short (iirc) should do the trick.

. Adding me to the CC list

In bug 1229684 we were able to show dates and times in the format it was expected, but only in views. At least, when changing timezone in operating system settings, date and time formatting was adjusted acordingly. When changing the locale, date format was also changed, as far as I remember. Then, what this bug is about is asking for the same to be done on code that doesn't format strings for chrome. Could you provide some steps so developer could visually test changes?

Oops, pressed "enter" too quickly. I'm using an en-US Daily but with an en-GB format locale configured on Windows. If Calendar used MozIntl, an en-US version would follow the en-GB locale settings and display 05/06/2017 for the 5th of June 2017. I also see 6 PM in the UI in some (not all) spots where it should be 18:00. So Javi, to test this, use and en-US Daily and set your locale to en-GB and configure DD/MM/YYYY with 24h time.

Is this related to Bug 1350679?

I wasn't aware of bug 1350679. Not really related, I'd say, but part of the big date/time format rework. Bug 1350679 has a very limited scope, only few callers of toLocaleFormat() left: https://searchfox.org/comm-central/search?q=toLocaleFormat&case=true&regexp=false&path=calendar

Thanks, Javi, for taking the bug. Looks like it's not a large patch this time: https://hg.mozilla.org/try-comm-central/rev/612e36a37f41ca15b29e8ef6f87d29237e48d563 It would be great to have this completed before the next branch date, 2017-06-12, to go into TB 55 without uplifts. Also, en-US date/time display which is not taking any OS settings into account is really quite annoying to many Daily users like myself. So the sooner we can get this fixed, the better.

Hi Jorg. Yes. And that try-run was just a test, I didn't remove most of the code which wasn't run, in fact. Final code will be even smaller, as most of the code for the past patch for bug 1229684 will be provided by mozIntl instead :) (Removing the needinfo for :makemyday as that was answered by a previous comment)

Hello, I use Windows 10 x64, French-Canadian version (fr-CA). As a fallout of bug 1344594, the date format has changed in Thunderbird. TB v. 55 (nightly from 2017-05-19) has fixed the issue for Thunderbird itself but not for Lightning. So in Thunderbird's list of messages, the date appears as "2017-05-17 à 23:00" (litteraly aaaa-mm-dd at HH:mm), which is much better than what happened with TB 53 and 54. However, in the calendar (integrated Lightning), the date appears as "dd/mm/yyyy HH:MM" (ex.: 17/05/2017 18:30), whereas it previously appeared as "2017-05-17 23:00". It makes it a bit confusing, especially as I live in an environment where the U.S., European French and ISO notations are used. BTW, the system configuration is "aaaa-mm-dd HH:mm".

(In reply to Michel from comment #9) > > As a fallout of bug 1344594, the date format has changed in Thunderbird. TB > v. 55 (nightly from 2017-05-19) has fixed the issue for Thunderbird itself > but not for Lightning. > Calendar application is about time -both dates and time itself- so needs more granular control over dates and times, while Thunderbird doesn't need it. > So in Thunderbird's list of messages, the date appears as "2017-05-17 à > 23:00" (litteraly aaaa-mm-dd at HH:mm), which is much better than what > happened with TB 53 and 54. > > However, in the calendar (integrated Lightning), the date appears as > "dd/mm/yyyy HH:MM" (ex.: 17/05/2017 18:30), whereas it previously appeared > as "2017-05-17 23:00". It makes it a bit confusing, especially as I live in > an environment where the U.S., European French and ISO notations are used. > There are lots of places where Calendar is showing a date. Could you be more especific on where that date was showing, please? That way I could chack when testing the patch, although in a non-French build. Bug 1229684 introduced that change and here we are going to ammend it. Nowadays there are lots of changes in the comm-central repo related to date and time formatting. Previous bug had to land because there was a problem that need to be fixed when building. That was fixed by 1229684.

Javi, sorry for not responding earlier, I have been off for some days. It seems you already got the instructions to prepare the environment to fix this. Looking at your recent try push, you should stick with the use of _inTimezone to avaoid regressions and introduce the use of mozIntl therein, passing the the appropriate dtOptions in from the format* functions accordingly. While this would be straight forward, have you already checked whether we need to work around the mozIntl restrictions for email invitation preview?

(In reply to [:MakeMyDay] from comment #11) > Javi, sorry for not responding earlier, I have been off for some days. It > seems you already got the instructions to prepare the environment to fix > this. > Hi :) No problem. > Looking at your recent try push, you should stick with the use of > _inTimezone to avaoid regressions and introduce the use of mozIntl therein, > passing the the appropriate dtOptions in from the format* functions > accordingly. > Yeah. That try-push was a test. Very very very dirty one. It showed that it wasn't working as expected in a different timezone and locale configuration than mine, which was what I was testing for. My latest local patch has modified a little bit the code introduced in bug 1229684. I am using _inTimezone, but passing dateStyle instead in order to avoid timezone conversions. The fields which should be shown are provided by mozIntl, so that makes new code more compact. Testing in my local system after changing system timezone to LA shows that it is working almost as expected. What I am facing problems with is that the date formatting: although requesting the formatter to use day="2-digit", it is always using day:"number". That is making tests to fail. However, I think using 2-digit is mandatory, so I will keep working on it. > While this would be straight forward, have you already checked whether we > need to work around the mozIntl restrictions for email invitation preview? No :/ :\ :/ :\ Steps to reproduce, please?

You just need a received email invitation in your inbox and look at the dates/times in the invitation preview with different regional settings. I would expect mozIntl wouldn't apply the custom settings in that case.

Any update here? The bug is completing its second month. I find calender hard to use in an en-US Daily since it shows en-US date/time formats all over the place, whereas the rest of TB is respecting OS settings for the most part (and I've picked up bug 1346549 to make it even better).

This is what MMD suggested in comment #11. My calender looks a whole lot more "civilised" in my en-US Daily since it now respects the en-GB OS settings. Note: I don't really want to "hijack" this bug, but more than a month inactivity make it look like some gentle push were necessary.

(In reply to [:MakeMyDay] from comment #11) > While this would be straight forward, have you already checked whether we > need to work around the mozIntl restrictions for email invitation preview? I'm not sure what that means. I checked an invitation that had this: DTSTART:20170604T151500Z DTEND:20170604T162500Z and in Berlin at GMT+2 I see this in my e-mail: 4 June 2017 17:15 – 18:25 So that looks pretty good to me. The only thing I don't understand in the code is the: let timezone = aDate.timezone; // we set the tz only if we have a valid tz - otherwise localtime will be used on formatting. if (timezone && (timezone.isUTC || timezone.icalComponent)) { aOptions.timeZone = timezone.tzid; } When using mozIntl, this is obviously ignored. So I'm not sure what to do about that.

(In reply to Jorg K (GMT+2) from comment #16) > (In reply to [:MakeMyDay] from comment #11) > > While this would be straight forward, have you already checked whether we > > need to work around the mozIntl restrictions for email invitation preview? > I'm not sure what that means. I checked an invitation that had this: > DTSTART:20170604T151500Z > DTEND:20170604T162500Z > > and in Berlin at GMT+2 I see this in my e-mail: > 4 June 2017 17:15 – 18:25 > So that looks pretty good to me. > > The only thing I don't understand in the code is the: > let timezone = aDate.timezone; > // we set the tz only if we have a valid tz - otherwise localtime > will be used on formatting. > if (timezone && (timezone.isUTC || timezone.icalComponent)) { > aOptions.timeZone = timezone.tzid; > } > When using mozIntl, this is obviously ignored. So I'm not sure what to do > about that. Timezone field is used when on the aOptions parameter in mozIntl. It is required to keep the timezone unchanged -so no timezone conversion should be done- for the UI, not for the tasks dates. If someone in New Zealand creates a meeting for certain hour, the meeting assistants woudl receive the date in their own timezone, like you said. However, the system also needs no time conversion for showing the hour in the left of the calendar view or in the settings popup menu for configuring the work hours of the day. There were tests added for that and was the reason previous bug was delayed for so long.

That is for reference and possible comparison. It is not passing short date-time formats as it keeps using the shorter form always. Likely Jorg's is a better solution

Hmm, I think Javi's solution is far more elegant, in comparison my |if (xxx in aOptions)| is just very clumsy. Can we go ahead with Javi's solution? What more is needed? I didn't know about the timezone support in mozIntl, but if Javi thinks his patch (almost) works, let's use it ;-) I'm not sure what the |if (aStyle.dateStyle) aStyle.day = "2-digit";| is meant to be for. Also, in a previous try push there was a |weekday: 'long'|. I understand the mozIntl will now deliver the right format, so it it doesn't return two digits, so be it, we just need to adapt the test expectations.

Javi's patch without the |if (aStyle.dateStyle) aStyle.day = "2-digit";|: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=409ff694f68d6bcb13e9a6fbfe29afdb4513118c

OK, right now only one platform has finished, Windows, and we get one test failure only: TEST-UNEXPECTED-FAIL | xpcshell-icaljs.ini:calendar/test/unit/test_datetimeformatter.js | formatDate_test - [formatDate_test : 47] (test #2) - "4/1/2017" == "04/01/2017" I'd adjust the expected test result and be done with it, no?

Comment on attachment 8881817 [details] [diff] [review] WIP: 1360915-mozIntl-calendar.patch I think Javi's patch is nicer.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=409ff694f68d6bcb13e9a6fbfe29afdb4513118c OK, the various platforms have various failures, mostly complaining about the missing leading zero in a one digit date. However, on Mac and Linux we also have: TEST-UNEXPECTED-FAIL | xpcshell-libical.ini:calendar/test/unit/test_ltninvitationutils.js | compareInvitationOverlay_test - [compareInvitationOverlay_test : 511] (test #3): imipHtml-when-content - false == true So Javi and MMD, how can we proceed here? Fix the test expectations?

The 2-digit format was a requirement in previous bug -the one which begin using the Intl JavaScript standard- but I have been unable to use it. mozIntl was supossed to allow the customization of the format of fields via the dtOptions passed to the method which actually creates the format and that should be passed to the actual formatter. However it is not working as expected. I tried to force the usage of that 2-digit format in _inTimezone() function. The usage of the timezone in aStyle is working as expected. However createDateTimeFormat is always using the date/timeStyle format without customizations, bypassing the fields formatting customizations. In a few minutes I am going to test that my patch is actually using the expected format depending on the OS locale. Right now I don't know why the imitHtml test is falling in 2 of the 3 platforms.

(In reply to Javi Rueda from comment #24) > In a few minutes I am going to test that my patch is actually using the > expected format depending on the OS locale. I'm using your version of the patch and it works fine respecting OS settings for en-GB while using an en-US build. I already stated that in comment #15: "My calender looks a whole lot more "civilised" in my en-US Daily since it now respects the en-GB OS settings".

Oh, sorry. But I had to test it as I was intrigued about that 2-digit formatting. It somehow seems mozIntl respects the formatting and will force to use it. en-GB show the month in a 2-digit format while en-US is not showing it that way. Order is showing in the right order now. For testing, I changed both the language and the region in System Preferences. In that dialog box, at the bottom it is show a preview of the date and it shows exactly the same as I have just said. So, for dates mozIntl is using the same format as the operating system, which maybe is the right way to go. For times, it was required to select 24-hour format in the operating system preferences. Then it shows hours with the 2-digit format. That seems to demonstrate mozIntl is using the same format that the operating system is using. So maybe the tests should change that and test for no-leading zeros for dates. Also it seems that the code removed on the latest try-server run should also be removed, as it is not working. That code was added to try to force the format and then the same would be done for the month and hour fields. Oh, I can see that the imipHtml test is failing because the day format: it was also expecting 2-digit day and it is getting only one digit.

As discussed, this works fine for me and passes most tests. Javi, can you please handle the remaining test failures.

I have just pushed a new try-server run with the patch. It removes the lines that forced the usage of 2-digit -the ones you also removed in your last f+ patch-, and also replaces the comments for the new parameters in _inTimezone. Also clarifies the reason for the timezonechange if-block, as it doesn't explained the reason of it been there. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7c80a301c3a27822c93645853c29cb3d02e228ff Furthermore, a new patch was added to the push to try to pass the formatting tests. I am however uncertain that :MakeMyDay will approve it. Anyway, it seems that it is the best way as explained previously as mozIntl is bypassing any single field formatting.

Thanks, I'm glad the bug is moving now. Since the system is now getting the time format from the OS, some tests will get different results on different platforms. For example the original test expecting two digit dates did *not* fail on Windows: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=409ff694f68d6bcb13e9a6fbfe29afdb4513118c&selectedJob=110477006 So I suspect your test run will not succeed on Mac and Linux but fail on Windows. Take a look at https://dxr.mozilla.org/mozilla-central/source/intl/locale/tests/gtest/TestDateTimeFormat.cpp to see how the C++ interface is tested in M-C. There I "normalise" the result. If I remember correctly, those tests did have some options as to the expected result, see: https://hg.mozilla.org/comm-central/rev/a07da5ede621fc0ec9b6d6d71e1c3b821b4de8ac#l14.12 Also, leading zeroes are not a must since we didn't have them before, see: https://hg.mozilla.org/comm-central/rev/a07da5ede621fc0ec9b6d6d71e1c3b821b4de8ac#l14.73 In summary: When using mozIntl we're going back to some platform dependence and the tests need to be more flexible.

I think the tests need to be adjusted like this. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c6accc254899fd6bf2b89aa2673731d0d0345bdd Both tests pass locally on Windows, so let's see what the other platforms do.

Yes, we need to re-introduce the previous per OS split up of test patterns for automation and maybe some custom formats for local test runs on common different combinations of app locale default and OS regional settings - we had this beforehand, too. However, when applying the patch from comment 27, it has no effect for me (as well as the recently landed patches for editor/mail/chat, btw). It works neither on Win7 nor on Win10 - using an en-us app locale and German OS locale and regional settings always results in displaying in en-US datetime format. But this seems to be a bug/limitation in the mozIntl implementation and not your code, so you can proceed with the approach of your patch. But please move the the MozIntl formatting operation itself to a separate method in calUtils.jsm, that way we can make use of it with JS only formatting operations like in bug 1350679.

(In reply to [:MakeMyDay] from comment #31) > However, when applying the patch from comment 27, it has no effect for me > (as well as the recently landed patches for editor/mail/chat, btw). It works > neither on Win7 nor on Win10 - using an en-us app locale and German OS > locale and regional settings always results in displaying in en-US datetime > format. But this seems to be a bug/limitation in the mozIntl implementation > and not your code, so you can proceed with the approach of your patch. Indeed, as has been discussed widely in other bugs. mozIntl only honours the OS settings if they are made for a language that matches the app locale. So your en-US app will honour en-GB settings, as it does for me, but not "de" settings. > But please move the the MozIntl formatting operation itself to a separate > method in calUtils.jsm, that way we can make use of it with JS only > formatting operations like in bug 1350679. I don't understand. Using mozIntl won't be useful in bug 1350679 as per bug 1350679 comment #6, and I agree. There are still test failures in the try push, so I have to try again ;-)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6b69d7cf7419acbfd3cfce926f8039e24bd1dd6d Instead of looking at the OS, now I simply provide a few test result options.

Passed on Windows now, but short dates on Linux/Mac are 4/1/17. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5753d8054294c16744d5df6b4fb8268f1e671b16 Hopefully the last try run :-(

(v3) passed on Windows and Mac, but not on Linux. So here yet another try on Linux only: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b3e06e50f8e1ed7d5fc8c4efb71cf42e85d527e5

This needs a lot of patience :-( - Still not right, here another go: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=197c423629e2374dadabf6e251ef31fbdea094b9

Comment on attachment 8883001 [details] [diff] [review] 1360915-test-expectations.patch (v5). Finally the tests all pass on all platforms. Whatever the code will look like, we need to adjust the tests like this. MakeMyDay, can you please describe in more detail how you would like to refactor Javi's patch, quote: "move the the MozIntl formatting operation itself to a separate method in calUtils.jsm".

(In reply to Jorg K (GMT+2) from comment #37) > > MakeMyDay, can you please describe in more detail how you would like to > refactor Javi's patch, quote: "move the the MozIntl formatting operation > itself to a separate method in calUtils.jsm". I think he wants the 2 latest lines in _inTimezone() -lines 79 and 80 in attachment 8882303 [details] [diff] [review]- to be placed in calUtils module. Am I right, :MakeMyDay?

Comment on attachment 8883001 [details] [diff] [review] 1360915-test-expectations.patch (v5). Review of attachment 8883001 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for checking the test. r+ with the comments below considered. ::: calendar/test/unit/test_datetimeformatter.js @@ +16,5 @@ > datetime: "20170401T180000", > timezone: "Pacific/Fakaofo", > dateformat: 0 // long > }, > + expected: [ "Saturday, April 01, 2017", "Saturday, April 1, 2017" ] Please remove the spaces after opeing and before closing for this type of brackets here and at other places in the patch (n.b. curley brackets are treated differently and require the spaces) expected: ["Saturday, April 01, 2017", "Saturday, April 1, 2017"] @@ +23,5 @@ > datetime: "20170401T180000", > timezone: "Pacific/Fakaofo", > dateformat: 1 // short > }, > + expected: [ "04/01/2017", "4/1/2017", "4/1/17" ] Can you please add a comment here (and in a similar way also to the other places), that the first the patterns are required for automated tests and shouldn't be changed unless there are test failures on automation. It would be also helpful to mention what pattern is for which OS as debugging OS specific formatting is a pita like you already learned (maybe some of that information can be moved to a general comment at the top of the tests). Custom patterns to satisfy local test runs should be appended thereafter.

(In reply to Jorg K (GMT+2) from comment #32) > Indeed, as has been discussed widely in other bugs. mozIntl only honours the > OS settings if they are made for a language that matches the app locale. So > your en-US app will honour en-GB settings, as it does for me, but not "de" > settings. This is a non-starter. Is there a bug on file requesting to allow this also cross language?

Comment on attachment 8882303 [details] [diff] [review] Javi's patch minus the two lines that don't work Review of attachment 8882303 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, r+ with the comment considered. We can spare the previously requested move to calUtils for now. We will need to move away from the xpcom interface anyway and can take care of consolidating formatting operations in an appropriate bug. Apart from that, this bug isn't fixed with the currentset of patches, as it doesn't fully restore the previous behaviour. But to go ahead, please file a new bug to restore the remaining bits and investigate the options to achive the original purpose of this bug, if you want to land the existing patches. ::: calendar/base/src/calDateTimeFormatter.js @@ +71,2 @@ > */ > + _inTimezone: function(aDate, aStyle) { Please leave the var name with aOptions and the comment as it was before. style is just another attribute along with others.

Thanks for the reviews. Javi, I'll take care of the issues and get this landed. (In reply to [:MakeMyDay] from comment #40) > This is a non-starter. Is there a bug on file requesting to allow this also > cross language? What you call "non-starter" has been Mozilla's core strategy since Christmas 2016 (with the advent of ICU) and we've been "suffering" from it ever since, starting with TB 53, if I remember correctly (or was it TB 54?). Landing this bug here will finally align us with M-C again so we can enjoy the goodness that might come from bug 1366134. If you want to follow one bug in this context, this is the bug to follow, I won't mention all the other bugs where the "non-starter" was hotly discussed.

Thanks for the reference. Even if it was the strategy, this is a non-starter for me from a user's pov. But we don't need to discuss this in this bug.

(In reply to Jorg K (GMT+2) from comment #42) > Thanks for the reviews. Javi, I'll take care of the issues and get this > landed. > Ok. Thank you, Jorg :)

I added comments as requested and changed [ x, y ] to [x, y]. Looking over the try runs again, I thing I don't been some of the patterns, so I removed them again. Here's another run before landing this (also includes bug 1350679). https://hg.mozilla.org/try-comm-central/pushloghtml?changeset=d512d5f13575f76d19538a927baf7bcc500e2335

Gotta love this grammar: I thing I don't been some of the patterns ... I meant: I think I don't need some of the patterns.

https://hg.mozilla.org/comm-central/rev/f0f85441cb740327b5dac5ac74a63330a90d3234 https://hg.mozilla.org/comm-central/rev/9aad32d14d945e0e89c43cff9fef9d67b11f9dd0

Comment on attachment 8884531 [details] [diff] [review] bug-1360915.patch - Javi's patch with review comments addressed We need to uplift this since TB 55 will be a messy mixture of date/time formatting otherwise.

Beta (TB 55, Calendar 5.7): https://hg.mozilla.org/releases/comm-beta/rev/1106971fef9c https://hg.mozilla.org/releases/comm-beta/rev/ae6be7b8e398