Last Comment Bug 1167939 - Long date uses OS date so it appears in OS language instead of Sorbian because there is no Sorbian OS
: Long date uses OS date so it appears in OS language instead of Sorbian becaus...
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: Internal Components (show other bugs)
: Lightning 4.0.0.1
: Unspecified Windows
-- normal (vote)
: 4.4
Assigned To: [:MakeMyDay]
:
:
Mentors:
Depends on: 1234223
Blocks:
  Show dependency treegraph
 
Reported: 2015-05-24 04:48 PDT by Michael Wolf
Modified: 2015-12-21 07:55 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
LongDateFormatReliesOnOSLocalization-V1.diff (5.21 KB, patch)
2015-05-24 12:30 PDT, [:MakeMyDay]
no flags Details | Diff | Splinter Review
LongDateFormatReliesOnOSLocalization-V2.diff (3.57 KB, patch)
2015-06-05 04:00 PDT, [:MakeMyDay]
philipp: review+
Details | Diff | Splinter Review
LongDateFormatReliesOnOSLocalization-V3.diff (3.61 KB, patch)
2015-07-04 10:51 PDT, [:MakeMyDay]
makemyday: review+
philipp: approval‑calendar‑beta-
philipp: approval‑calendar‑esr-
Details | Diff | Splinter Review

Description User image Michael Wolf 2015-05-24 04:48:28 PDT
Current situation:

Upper Sorbian and Lower Sorbian will be new Lightning locales from TB 38.0. Both Sorbian languages are minority languages and there is no OS in Sorbian. Lightning uses the long date form the OS and thus the OS language (here German) is used for the long date. For me as translator of a minority language this is a form of hardcoding.

Expected situation:

Long date (and perhaps other hardcode dates I do not know about yet) should be fully localizable that the long date appears in the locale language of Lightning.
Comment 1 User image [:MakeMyDay] 2015-05-24 12:30:53 PDT
Created attachment 8609896 [details] [diff] [review]
LongDateFormatReliesOnOSLocalization-V1.diff

Confirming this at least on Windows. This get's visible in the calendar pref UI, today pane, the day view and maybe elsewhere.

We have a fallback for Linux already, where we compose the long date string on our own. This patch removes the code where we're trying to get this from the OS and use the existing fallback instead for all OS.

NB: on Windows, this changes displaying day names from long to short names (Monday to Mon) for me, but I think that is okay and maybe related to my OS settings.
Comment 2 User image Philipp Kewisch [:Fallen] 2015-05-25 13:59:32 PDT
Check out the discussion in bug 399605. It seems there are some reasons why we use the OS date in some places. Maybe ssitter can comment on this?
Comment 3 User image Stefan Sitter 2015-05-26 11:25:50 PDT
If I recall correctly Sunbird/Lightning used/mixed some hard coded localization specific date and time formats in the past. This was changed with e.g. Bug 460988 to use the date and time format specified by the user in the OS. Probably main reason was to show dates and times that are known to the user and matches what is shown in Thunderbird. Currently we are using nsIScriptableDateFormat, see e.g. Bug 441167 for the request to change it from OS language to application language.
Comment 4 User image [:MakeMyDay] 2015-06-01 23:15:45 PDT
Based on comment #3 I would call this one a duplicate of bug 441167, unless we want to work around thisin Calendar by the proposed solution, which may have downsides on different platforms. So, what do you think how to deal with that issue now?
Comment 5 User image Michael Wolf 2015-06-02 06:56:40 PDT
My focus is on the language. I think in bug 441167 the focus is more on the date format. There is no OS in Sorbian so the longe date is displayed in German. The date format is approximately the same:
%d. %B %Y. There is a small diference only: The Sorbian month name is in genitive singular. In fact, both things must be correct: date format and language. And as language should be used the Lightning language.

Example:

German: 2. Juni 2015
Sorbian: 2. junija 2015

The month names is "junij" - "junija" is the genitive singular.
Comment 6 User image [:MakeMyDay] 2015-06-05 04:00:25 PDT
Created attachment 8615931 [details] [diff] [review]
LongDateFormatReliesOnOSLocalization-V2.diff

Ok, this one leaves it in general to use OS long date, but checks additionally, whether localized weekdays appear therein. If not, the fall back approach is applied like for Linux already.
Comment 7 User image Michael Wolf 2015-06-14 13:00:22 PDT
A question: Is there already a TB milestone for that? TB 38.0.1 uses system long date yet.
Comment 8 User image [:MakeMyDay] 2015-06-15 00:05:34 PDT
That depends whether we go with the proposed approach within Lightning or a fix in toolkit is required instead. In case of the latter, there will be no ETA at the moment. If we proceed with this patch, the fix will be at least in next ESR, but eventually we can backport it to 4.0.x as well.

Please note, that the current patch will result not in "Juni" resp. "junija" but in the three letter abbreviation "Jun" as this is today aleady the case for Linux.
Comment 9 User image Philipp Kewisch [:Fallen] 2015-06-30 03:03:25 PDT
Comment on attachment 8615931 [details] [diff] [review]
LongDateFormatReliesOnOSLocalization-V2.diff

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

r=philipp with comments considered. I'd suggest testing this patch on trunk for a while before we backport it to 4.0.x

::: calendar/base/src/calDateTimeFormatter.js
@@ +102,5 @@
> +            // check whether weekday name appears as in Lightning localization. if not, this is
> +            // probably a minority language without OS support, so we should fall back to compose
> +            // longDate on our own. May be not needed anymore once bug 441167 is fixed.
> +            if (longDate.indexOf(this.dayName(aDate.weekday) == -1) &&
> +                longDate.indexOf(this.longDayName(aDate.weekday) == -1)) {

You can use longDate.includes() here. Also, since this is really a one time thing, can you set this.mUseLongDateService = false here to cache the value? Maybe it makes sense to check a dummy value once when initializing mUseLongDateService, which would simplify this method a bit.
Comment 10 User image [:MakeMyDay] 2015-07-04 10:51:28 PDT
Created attachment 8629593 [details] [diff] [review]
LongDateFormatReliesOnOSLocalization-V3.diff

I have changed to includes() and cached this.mUseLongDateService. To move the check to the initializing code wouldn't bring any benefit from my pow.
Comment 11 User image Philipp Kewisch [:Fallen] 2015-07-16 05:16:56 PDT
Pushed to comm-central changeset 40897c80a0e6
Comment 12 User image [:MakeMyDay] 2015-08-15 07:19:36 PDT
Comment on attachment 8629593 [details] [diff] [review]
LongDateFormatReliesOnOSLocalization-V3.diff

Let's track this one for 4.0.3.
Comment 13 User image Philipp Kewisch [:Fallen] 2015-09-08 06:23:39 PDT
Comment on attachment 8629593 [details] [diff] [review]
LongDateFormatReliesOnOSLocalization-V3.diff

Given the recent failures in 4.0.x releases and due to the fact that changing the date format isn't a light thing to do, I am not yet convinced we should take this for 4.0.3.

I'm happy to reconsider if you can convince me. Maybe it would help to add a unit test for this change.
Comment 14 User image [:MakeMyDay] 2015-09-08 09:46:56 PDT
This patch provides only an additional fallback solution which is already applied today for Linux. That said, if that fails with the patch, it would have done without as well. A test on a not localized environment (in a minority language) would make no real sense.
Comment 15 User image Stefan Sitter 2015-12-21 07:55:31 PST
This change seems to have regressed Bug 1234223.

Note You need to log in before you can comment on or make changes to this bug.