Closed Bug 1512123 Opened 6 years ago Closed 5 years ago

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

Categories

(Calendar :: Dialogs, defect)

Lightning 6.2.3.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: lilis, Assigned: darktrojan)

References

Details

(Whiteboard: [datetime-issue-6.2])

Attachments

(7 files, 5 obsolete files)

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
Component: Untriaged → General
Product: Thunderbird → Calendar
Version: 60 → Lightning 6.2.3.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)?
Flags: needinfo?(lilis)
(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)?
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
    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
Flags: needinfo?(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.

Flags: needinfo?(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/

Flags: needinfo?(jcho)
Whiteboard: [datetime-issue-6.2]
Component: General → Dialogs

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

Flags: needinfo?(jcho)

(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

Flags: needinfo?(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?

Flags: needinfo?(makemyday)

(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

(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

(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.

Attached file {e2fda1a4-762b-4020-b5ad-a41df1933103}.xpi (obsolete) (deleted) —

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.

Flags: needinfo?(jcho)
Attached file thunderbird_log.txt (obsolete) (deleted) —

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

Flags: needinfo?(jcho)

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Мм][. ]?|م|下午|午後|오후)$/

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

Attachment #9049804 - Attachment is obsolete: true
Flags: needinfo?(jcho)
Attachment #9051501 - Attachment description: {e2fda1a4-762b-4020-b5ad-a41df1933103}.xpi → {e2fda1a4-762b-4020-b5ad-a41df1933103}.xpi (with a fix for Korean AM/PM characters)
Attached file thunderbird_log_with_fix.txt (obsolete) (deleted) —

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

Attachment #9050218 - Attachment is obsolete: true
Flags: needinfo?(jcho)
Summary: Calendar's event time keeps resetting to AM → Calendar's event time keeps resetting to AM with Korean locale
Attached patch RecognizeKoreanAmPm-V1.diff (deleted) — Splinter Review

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

Assignee: nobody → makemyday
Attachment #9051501 - Attachment is obsolete: true
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #9051563 - Flags: review?(philipp)
Attachment #9051563 - Flags: approval-calendar-esr?(philipp)
Attachment #9051563 - Flags: approval-calendar-beta?(philipp)
Flags: needinfo?(lilis)
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.

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).

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 π.μ."

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=

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?

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.

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.

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?

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.

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

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.

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

Flags: needinfo?(jcho)

Hi
it works too =)

Attachment #9051543 - Attachment is obsolete: true
Flags: needinfo?(jcho)
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.
Attachment #9051563 - Flags: review?(philipp)
Attachment #9051563 - Flags: review+
Attachment #9051563 - Flags: approval-calendar-esr?(philipp)
Attachment #9051563 - Flags: approval-calendar-esr+
Attachment #9051563 - Flags: approval-calendar-beta?(philipp)
Attachment #9051563 - Flags: approval-calendar-beta+

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=

Flags: needinfo?(philipp)

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.

Flags: needinfo?(philipp)

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.

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.

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.

Attached patch RecognizeKoreanAmPm-V2.diff (deleted) — Splinter Review

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.

Attachment #9054606 - Flags: review?(philipp)
Attachment #9054606 - Flags: approval-calendar-esr?(philipp)
Attachment #9054606 - Flags: approval-calendar-beta?(philipp)

Corresponding ESR patch for the alternate approach.

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

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).

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

Depends on: 1517569
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?
Attachment #9054606 - Flags: review?(philipp)
Attachment #9054606 - Flags: review+
Attachment #9054606 - Flags: feedback?(jorgk)
Attachment #9054606 - Flags: approval-calendar-esr?(philipp)
Attachment #9054606 - Flags: approval-calendar-esr+
Attachment #9054606 - Flags: approval-calendar-beta?(philipp)
Attachment #9054606 - Flags: approval-calendar-beta+
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.
Attachment #9054606 - Flags: feedback?(jorgk)

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

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?

Flags: needinfo?(makemyday)
Attached patch RecognizeKoreanAmPm-V2.diff - Rebased (obsolete) (deleted) — Splinter Review

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.

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

Attachment #9054606 - Flags: approval-calendar-esr+
Attachment #9054606 - Flags: approval-calendar-beta+
Attachment #9051563 - Flags: approval-calendar-esr+
Attachment #9051563 - Flags: approval-calendar-beta+
Attached patch 1512123-am-pm-regexp-1.diff (deleted) — Splinter Review

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.

Attachment #9064118 - Attachment is obsolete: true
Flags: needinfo?(makemyday)
Attachment #9070912 - Flags: review+
Attachment #9070912 - Flags: feedback?(philipp)

Good news for both bugs!

Attachment #9070912 - Flags: approval-calendar-beta?(philipp)
Attachment #9070912 - Flags: feedback?(philipp)
Attachment #9070912 - Flags: feedback+
Attachment #9070912 - Flags: approval-calendar-beta?(philipp)
Attachment #9070912 - Flags: approval-calendar-beta+
Keywords: checkin-needed
Assignee: makemyday → geoff
Attachment #9070912 - Flags: approval-calendar-esr?(philipp)
Attachment #9070912 - Flags: approval-calendar-esr?(philipp) → approval-calendar-esr+

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

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

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 7.1

(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?

Flags: needinfo?(philipp)
Target Milestone: 7.0 → 6.2.7

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.

Flags: needinfo?(jcho)

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

Flags: needinfo?(jcho)
Target Milestone: 6.2.7 → 6.2.8
Flags: needinfo?(philipp)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: