1512123 - Calendar's event time keeps resetting to AM with Korean locale

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[lilis]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.110 Safari/537.36

Steps to reproduce:

1. Switch to Calendar tab
2. Chose a date and opened the 'Add event' dialog
3. Entered details and saved


Actual results:

- When I tried to change the event time (e.g. 14:00 - 16:00), it keeps reverting to AM (02:00 - 04:00)

- Ranged date events are saved as single date event


Expected results:

- It should be the time I've set myself
- Event date range should be applied when necessary

Comment 1

[[:MakeMyDay]]

Date and time format follows the locale's definition for datetime formatting locale that is derived from your setting of Options > Advanved > General > Date and Time Formatting. You cannot overrule this formatting by tying in a different format.

What is the locale this setting resolves to? What is your OS locale? Do you use a localized TB (if so, which locale)?

Comment 2

[Jeff Cho]

(In reply to [:MakeMyDay] from comment #1)
> Date and time format follows the locale's definition for datetime formatting
> locale that is derived from your setting of Options > Advanved > General >
> Date and Time Formatting. You cannot overrule this formatting by tying in a
> different format.
> 
> What is the locale this setting resolves to? What is your OS locale? Do you
> use a localized TB (if so, which locale)?

Comment 3

[Jeff Cho]

i have the same issue. keeps reverting to AM 
i don't know if it's related but found someone else reporting the same 
https://bugzilla.mozilla.org/show_bug.cgi?id=1514077

Comment 4

[Jeff Cho]

    update channel: release
    user agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0
    OS: Windows_NT 10.0
    build id: 20181217101353

    I have 'Provider for Google Calendar 4.4.2' and Lightning 6.2.4 installed
    
    language : ko
    locale : ko-KR
    OS locale : ko-KR

Comment 5

[[:MakeMyDay]]

Please check with a Daily build if the issue still persits once bug 1503731 landed. Make sure to use a separate or new profile when doing so. Using beta or esr profiles might corrupt them.

Comment 6

[[:MakeMyDay]]

Please check with the upcoming TB66 beta [1] (available in the next couple of days) whether the issue is still present. If doing so, please use either a new profile or make sure to backup your TB profile before using it with the beta, since profile downgrading (from beta to esr after completing the test) is not supported and may cause problems.

[1] https://www.thunderbird.net/de/channel/

Comment 7

[Jeff Cho]

i can confirm that the issue is still present in TB66 beta.
My timezone is GMT +9.

Comment 8

[Jeff Cho]

(In reply to [:MakeMyDay] from comment #6)

Please check with the upcoming TB66 beta [1] (available in the next couple of days) whether the issue is still present. If doing so, please use either a new profile or make sure to backup your TB profile before using it with the beta, since profile downgrading (from beta to esr after completing the test) is not supported and may cause problems.

[1] https://www.thunderbird.net/de/channel/

Application Basics
Name: Thunderbird
Version: 66.0b1
Build ID: 20190209112700

Update Channel: beta
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:66.0) Gecko/20100101 Thunderbird/66.0
OS: Windows_NT 10.0

Internationalization & Localization

  Application Settings
  Requested Locales: ["en-US"]
  Available Locales: ["en-US"]
  App Locales: ["en-US"]
  Regional Preferences: ["ko-KR"]
  Default Locale: "en-US"

  Operating System
  System Locales: ["ko-KR"]
  Regional Preferences: ["ko-KR"]

i hope this helps

Comment 9

[[:MakeMyDay]]

Thanks, what timezone setting did you set in TB and in Windows? Do you have automatic timezone update enabled? If so, does disabling it change anything?

Comment 10

[Jeff Cho]

(In reply to [:MakeMyDay] from comment #9)

Thanks, what timezone setting did you set in TB and in Windows? Do you have automatic timezone update enabled? If so, does disabling it change anything?

timezone setting in TB : Asia/Seoul
Windows : UTC +09:00 Seoul
and i have automatic timezone update disabled.
enabling it and then disabling it did not help either

Comment 11

[Jeff Cho]

(In reply to [:MakeMyDay] from comment #5)

Please check with a Daily build if the issue still persits once bug 1503731 landed. Make sure to use a separate or new profile when doing so. Using beta or esr profiles might corrupt them.

I think this bug is unrelated to the bug you mentioned.
oh and i forgot to mention that i've tested TB66 beta[1] with no addons enabled but lightning
i installed it on windows 10 fresh install. new install, new profile, new os.

please let me know if you need anyting else
i'll be more than happy to help you out to fix this bug

Comment 12

[Jeff Cho]

(In reply to [:MakeMyDay] from comment #5)

Please check with a Daily build if the issue still persits once bug 1503731 landed. Make sure to use a separate or new profile when doing so. Using beta or esr profiles might corrupt them.

Hello. I can see that TB66b3 has been released and I'm afraid to tell you that this time reverting to AM issue did not get resolved.

Comment 13

[[:MakeMyDay]]

Attachment: {e2fda1a4-762b-4020-b5ad-a41df1933103}.xpi

Thanks for checking, Jeff. We need your support to dig further into the issue.

Can you please start your TB66b3 with a new profile (you can just close the the wizard poping up after startup), enable "calendar.debug.log" and "calendar.debug.log.verbose" preferences (go to menu->options->advanced->tb config editor and type in the forementioned preference names), install the attached Lightning package with the addons manager and restart TB.

When TB comes up again, please open the error console (CTRL+SHIFT+j) and clear all existing entries. Then switch back to the main window, open calendar tab, create a new event and reproduce the issue. Once completed, please copy all the log entries you got to a text file and attach that to this bug.

The Lightning package is the same version bundled with Korean TB66b3 with additional debug logging.

Comment 14

[Jeff Cho]

Attachment: thunderbird_log.txt

Please see the attached for the further info.
"오전" means AM, "오후" means PM
I'm not sure if you have CJK font on your computer

Comment 15

[Jeff Cho]

Lightning: 오후 6:00 datetimepickers.xml:1592
Lightning:
Array(9) [ " 6:00", undefined, "6", ":", "00", undefined, undefined, undefined, undefined ]
datetimepickers.xml:1607
Lightning:
Date 2019-03-11T21:00:00.000Z
datetimepickers.xml:1669
Lightning: datetimepicker-base:parseTime (end) datetimepickers.xml:1670
Lightning: datetimepicker-base:formatTime (start) datetimepickers.xml:1888
Lightning:
Date 2019-03-11T21:00:00.000Z
datetimepickers.xml:1889
Lightning: 오전 6:00 datetimepickers.xml:1892

I think this issue has got nothing to do with the timezone.

thunderbird doesn't seem to recognize what "오후" means.

/^(?:[AaАа][. ]?[MmМм][. ]?|ص|上午|午前)$/
/^(?:[PpРр][. ]?[MmМм][. ]?|م|下午|午後)$/
if this regex is being used to detect if the time is PM, i don't think it will work
it should be like this:
/^(?:[AaАа][. ]?[MmМм][. ]?|ص|上午|午前|오전)$/
/^(?:[PpРр][. ]?[MmМм][. ]?|م|下午|午後|오후)$/

Comment 16

[[:MakeMyDay]]

Attachment: {e2fda1a4-762b-4020-b5ad-a41df1933103}.xpi (with a fix for Korean AM/PM characters)

Yes, it looks like that. Does the attached xpi resolve the issue for you?

Comment 17

[Jeff Cho]

Attachment: thunderbird_log_with_fix.txt

Yes it does!!! =)
thank you very much!
will this fix be applied to the next beta?

Comment 18

[[:MakeMyDay]]

Attachment: RecognizeKoreanAmPm-V1.diff

This fixes the issue. The beta uplift request is obsolete if we land this before tomorrow merges.

Comment 19

[[:MakeMyDay]]

Attachment: RegognizeKoreanAmPm-ESR60-V1.diff

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 9051563 [details] [diff] [review]
RecognizeKoreanAmPm-V1.diff

What about, for example, Greek, Hebrew and Thai, just to mention some other writing systems?

Since our source code is UTF-8, why do we need to use the \uNNNN notation that no one can read? We have plenty of non-ASCII stuff all over the code base.

Comment 21

[[:MakeMyDay]]

This is only relevant for languages which use 12 hour clocks and have their own characters for am/pm, which is not true for most languages.

We recently had broken functionality for not having \u encoded non ascii characters, so this is the preferred and more safe way to go for calendar until there is an automatated encoding check available on CI to guard that no non utf-8 files can be checked in (and having arabic, chinese, korean or japanese chracters in the main code does not make it more readable to me then a \u encoding).

Comment 22

[Jorg K (CEST = GMT+2)]

Yes, I remember the windows-1252 stuff slipping in :-(

Hmm, we're not doing Assamese, so you're not missing:
new Services.intl.DateTimeFormat("as", {timeStyle: "long"}).format(Date.now()); - "অপৰাহ্ণ ১১:৫১:৩৬"

I think you're missing Vietnamese:
new Services.intl.DateTimeFormat("vi", {timeStyle: "long"}).format(Date.now()); - "11:52:03 CH"
... Malay (which we had once, but has disappeared since):
new Services.intl.DateTimeFormat("ms", {timeStyle: "long"}).format(Date.now()); - "11:56:09 PTG"
... Greek (as I said)
new Services.intl.DateTimeFormat("el", {timeStyle: "long"}).format(Date.now()); - "12:01:29 π.μ."

Comment 23

[Jorg K (CEST = GMT+2)]

Actually, replacing the \uNNNN encoding with the UTF-8 character has the advantage that you can actually see the character like a the bottom of comment #15. With a little knowledge you can actually see Arabic, Chinese and Korean characters there, and Greek to come soon.

There is no risk of mis-encoding since there isn't a single character set that can encode all those languages, so the mistake that happened in the other bug, windows-1252 instead of UTF-8, cannot happen here.

Just for fun:
https://searchfox.org/comm-central/search?q=%CF%80.%CE%BC.&case=true&regexp=false&path=

Comment 24

[Jorg K (CEST = GMT+2)]

And another thought: Hard-coding all those AM/PM suffixes seems quite illegible and error-prone. Wouldn't it be better to get the suffixes for the given locale from the system?

Comment 25

[Jeff Cho]

i've just noticed that there's been an update for thunderbird on the release channel.
I was wondering if the fix has been applied to this version. there's no mention of this fix on the release note
so i'm assuming the fix did not make it to the release yet?

(In reply to [:MakeMyDay] from comment #18)

Created attachment 9051563 [details] [diff] [review]
RecognizeKoreanAmPm-V1.diff

This fixes the issue. The beta uplift request is obsolete if we land this before tomorrow merges.

Comment 26

[Jorg K (CEST = GMT+2)]

As long as the bug isn't marked FIXED, nothing has been shipped. You will notice it when it happens, perhaps for TB 60.7. I think users in Greece and Vietnam are also affected, see comment #22.

Comment 27

[Jeff Cho]

oh ok. thanks for the clarification =)
your comment was very educational btw. i never knew Greece, Vietnam also have their own writing for am/pm

just for my curiosity, what went wrong if you don't mind me asking?
it was working alright till last december if my memory serves me right
i don't know how it works but..didn't you already have am/pm suffixes in place?

Comment 28

[Jorg K (CEST = GMT+2)]

Between TB 52 and TB 60 the Mozilla platform shook up datetime processing and formatting 500%. This will be a fallout. As I said in comment #24, I don't think hard-coding all the possible combinations is a good thing. But if Korean worked before, maybe I'm wrong on that one.

Comment 29

[Jeff Cho]

I agree. you are not wrong on that one.
hard-coding all the possible combination is not a good thing.
it would be better if you can get the suffixes from the system.

please let me know if you need me for testing =)
i'll do whatever i can do to help you out

Comment 30

[Jorg K (CEST = GMT+2)]

Thanks. Just to clarify a bit more: I'm TB's "code manager", I put code together for releases, and I'm generally snooping around to see what's happening in the project. I also fix problems in Thunderbird core code. This issue here is handled by the Calendar team, so in this case, MakeMyDay (MMD) as the assignee and patch author and Philipp as the reviewer. Both are volunteers, so we can't expect 24 hours turnaround. So far review hasn't happened, and as far as I can see, the patch isn't ready since, as observer, at least Greek and Vietnamese are missing. Or they will decide to take a different approach. In the meantime you can use the special version MMD provided.

Comment 31

[[:MakeMyDay]]

Attachment: Lightning for TB 66b3 - alternate fix approach

Jeff, can you please check whether the attached xpi for TB66b3 works for you as well and report back?

Comment 32

[Jeff Cho]

Attachment: thunderbird_log_with_alternate_fix.txt

Hi
it works too =)

Comment 33

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9051563 [details] [diff] [review]
RecognizeKoreanAmPm-V1.diff

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

Can you add tests for this? Maybe also on the other bug I just approved.

Comment 34

[Jorg K (CEST = GMT+2)]

Umm, OK, the bug says "Korean", but unless I'm mistaken, Vietnamese and Greek will also not work, see comment #22.

And I'd really prefer not to have UTF-8 stuff encoded as \uNNNN since you can actually search the code base for π.μ. or 오전:
https://searchfox.org/mozilla-central/search?q=%CF%80.%CE%BC.&case=true&regexp=false&path=
https://searchfox.org/mozilla-central/search?q=%EC%98%A4%EC%A0%84&case=true&path=

Comment 35

[Philipp Kewisch [:Fallen][📱📆]]

Sorry, i didn't read the remaining comments. Jörg is right, it would be good to fix it for other locales as well. MakeMyDay, can you look through other locales and see if there are some symbols we could add?

I don't really mind if it us \uNNNN or the symbol, I'll leave that up to you.

Comment 36

[Jorg K (CEST = GMT+2)]

Don't you think comment #15 is a whole more readable: /^(?:[PpРр][. ]?[MmМм][. ]?|م|下午|午後|오후)$/
You can actually check the correctness without having to search the codes on Google.

Comment 37

[Stefan Sitter [:ssitter]]

Is there no possibility to get those information from system or toolkit? When looking at the common abbreviations for AM/PM e.g. [dayperiod-short] (https://unicode.org/cldr/charts/latest/by_type/date_&_time.fields.html#64532e2c24e52150) there are many, many more languages not handled.

Comment 38

[Jorg K (CEST = GMT+2)]

According to MMD it's only an issue where the the locale uses 12h time, which is a minority. Try
new Services.intl.DateTimeFormat("vi", {timeStyle: "long"}).format(Date.now());
in the console and replace the "vi" with the locale of your choice. I suggested getting the AM/PM pattern from the system, see comment #24. MMD also had an "alternate fix approach", but I haven't looked in detail what that does.

Comment 39

[[:MakeMyDay]]

Attachment: RecognizeKoreanAmPm-V2.diff

This is the patch for the alternate approach (also confirmed to be working in comment 32) which avoids hardcoding additional detection patterns and instead makes use of the day period indicator from the time probes based on the current locale.

The patch applies on top of bug 1517569.

For the additional detection of additional time formats I have no intention to change the existing \u encoding in this file.

Comment 40

[[:MakeMyDay]]

Attachment: RecognizeKoreanAmPm-ESR60-V2.diff

Corresponding ESR patch for the alternate approach.

Comment 41

[Jorg K (CEST = GMT+2)]

This is not the easiest to understand code I've ever seen. Looks like it was imported into HG mostly like it is today in 2008:
https://hg.mozilla.org/releases/comm=esr60/rev/7c0bfdcda673#l1198.1749 (EDIT: damaged URL by changing - to =)

So if the alternate approach doesn't need to hard-code any Korean AM/PM codes (오전/오후) it will also work for other locales, like Greek and Vietnamese? And could this block be removed?
https://searchfox.org/comm-central/rev/2a6de7ec2104e7efa2a3f29fb0d4162c49ad2b67/calendar/resources/content/datetimepickers/datetimepickers.js#1457-1463

Comment 42

[[:MakeMyDay]]

It will work for eevery locale. And the other block cannot be removed since that is to support further international date formats (and we stay with the /u encoding).

Comment 43

[Jeff Cho]

Lightning: this.amRegExp: /^(?:[오][전][ ]|[AaАа][. ]?[MmМм][. ]?|ص|上午|午前)$/ datetimepickers.xml:1849
Lightning: this.pmRegExp: /^(?:[오][후][ ]|[PpРр][. ]?[MmМм][. ]?|م|下午|午後)$/ datetimepickers.xml:1850

i thought it was odd that you were separating words letter by letter
thinking "오" by itself doesn't mean anything
i didn't know what the alternate approach was.

after reading through the comments, i realized it was to match any localized am pm string
i think the hard coded stuff is left there for when the pattern matching fails
it will most likely work for every locale. it worked for Korean =)

thank you MMD =)
I will happily wait for the next beta and regular release

Comment 44

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9054606 [details] [diff] [review]
RecognizeKoreanAmPm-V2.diff

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

Codewise this looks ok to me. Jörg, aside from the code being somewhat less readable, are you satisfied with this?

Comment 45

[Jorg K (CEST = GMT+2)]

Comment on attachment 9054606 [details] [diff] [review]
RecognizeKoreanAmPm-V2.diff

Thanks for asking. I find the entire function totally unreadable.

From a sheriff's point of view this doesn't help since it's based on bug 1517569 (see comment #39) and that one got backed out.

Comment 46

[Jorg K (CEST = GMT+2)]

That said, we would still add a comment to explain the \uNNNN business like I saw here coincidentally:
https://searchfox.org/comm-central/rev/7c6567ce75e77c61bab4faebf67404872af3e994/mailnews/base/test/TestMsgStripRE.cpp#56

Comment 47

[Jorg K (CEST = GMT+2)]

Can we move this forward? This is blocked by bug 1517569 which couldn't land due to test failures. Or should I try rebasing the patch without bug 1517569 to see what happens. Maybe the same test failures?

Comment 48

[Jorg K (CEST = GMT+2)]

Attachment: RecognizeKoreanAmPm-V2.diff - Rebased

Rebased after bug 1541026 and without bug 1517569. Let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4157e93efa47473f5fc335cd008313aad1624673

Bug 1517569 made 8 tests fail on all platforms, so Linux and Mac are enough here.

Comment 49

[Jorg K (CEST = GMT+2)]

Same test failures as in bug 1517569 comment #33. Not a surprise.

Comment 50

[Geoff Lankow (:darktrojan)]

Attachment: 1512123-am-pm-regexp-1.diff

I've re-rebased this, on top of my new patch for bug 1517569, and tidied up.

(\D*) will have the same effect as ([\D]+)? except that the resulting value will be "" instead of null.

Comment 51

[Geoff Lankow (:darktrojan)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=56e07a22870a91317af0e27e44f1980cbd9f59ab

Comment 52

[Jorg K (CEST = GMT+2)]

Good news for both bugs!

Comment 53

[Geoff Lankow (:darktrojan)]

Let's give this bug a spell on nightly/beta before we push it to ESR.

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/75267aa625a2
Make timepicker recognize local characters representing the day period. r=philipp DONTBUILD

Comment 55

[Jorg K (CEST = GMT+2)]

(In reply to Geoff Lankow (:darktrojan) from comment #53)

Let's give this bug a spell on nightly/beta before we push it to ESR.

Good, going to beta 68 soon, then to ESR 60.8 in July, OK?

Comment 56

[Jorg K (CEST = GMT+2)]

TB 68 beta / Cal 7.0:
https://hg.mozilla.org/releases/comm-beta/rev/af338bc05fce66b2cee74880a4b2d05d61f5d964

Comment 57

[Jorg K (CEST = GMT+2)]

TB 60.8 ESR / Cal 6.2.8:
https://hg.mozilla.org/releases/comm-esr60/rev/549d1a48f63db04f9c5778071d50b7ee14ecfccb

Need 6.2.8.

Comment 58

[Jorg K (CEST = GMT+2)]

Jeff, could you please try our TB 60.8.0 release candidate from here:
http://ftp.mozilla.org/pub/thunderbird/candidates/60.8.0-candidates/build1/
Click on the platform, like win32, then on the localisation.

After installing and starting TB, please make sure that Lightning was updated to version 6.2.8.

Comment 59

[Jeff Cho]

Hi!
I confirm that TB 60.8.0 RC does not have this problem! =)