Closed Bug 328996 Opened 18 years ago Closed 16 years ago

guessSystemTimezone doesn't take northern/southern hemisphere into account

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gekacheka, Assigned: gekacheka)

References

Details

Attachments

(1 file, 10 obsolete files)

44.15 KB, patch
cmtalbert
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: lightning 20060228

guessSystemTimezone doesn't take northern/southern hemisphere into account

Reproducible: Always

Steps to Reproduce:
1. set O.S. timezone to melbourne australia, auto DST enabled.
2. start tb/lightning with new profile
   (so doesn't use any old lightning timezone setting)
3. in javascript-shell (e.g., using extensionDev extension)
   load("chrome://calendar/content/calendarUtils.js")
   guessSystemTimezone()

Actual Results:  
/mozilla.org/20050126_1/Asia/Sakhalin

Expected Results:  
One of
/mozilla.org/20050126_1/Australia/Melbourne
/mozilla.org/20050126_1/Australia/Sydney

Workaround: 
 lightning: set timezone in preference options.  
 sunbird: none yet (bug 304541)
(patch -l -p 2 -i file.patch)

Fixes checkTZ: 

a. adds calls to systemTZMatchesTimeShiftDates to check if the current
system timezone shifts offset on the dates of the candidate timezone,
and in the same direction.  This fixes the northern/southern
hemisphere errors.

b. uses findCurrentTimePeriod to get the time period.  In general a
VTIMEZONE may have several STANDARD and DAYTIME declarations if the
timezone offset has changed or will change, and we want the one which
is currently applicable, not necessarily the first one.

Extends locale likelyTimezone:  

c. now accepts a list of timezone names for locales that have multiple
timezones.  This should help guess America/Los_Angeles rather than
America/Dawson for Pacific time in the en-US locale, and lets another
locale, say en-CA or es-MX, specify its timezones.

d. reports errors in likelyTimezone to javascript console

Clarifies:

e. Probe dates are now in June and December, and variables are named
   xxxJun and xxxDec to make them easier to understand.

f. Simplified some parsing code with regexes.
Whiteboard: [cal relnote]
(patch -l -p 2 - i file.patch)

Improvements extend previous patch (comment 1):

* To the en-US likelyTimezone data, adds world timezones that have
  many English speakers that would otherwise default to an alphabetically 
  earlier but less likely timezone:
   Europe/London   likelier than Atlantic/Canary
   Europe/Paris    likelier than Africa/Ceuta (Paris: EU-WestEuropeanTime)
   America/Halifax likelier than America/Glace_Bay (Halifax CA-Atlantic Time)
   Asia/Singapore  likelier than Antarctica/Casey
   Asia/Tokyo      likelier than Asia/Dili
   Australia/Brisbane likelier than Antarctica/DumontDUrville
   Pacific/Auckland likelier than Antarctica/McMurdo
  Rationale: English speakers around the world may use the en-US
  localization, especially if there is no version for their country.
  (Other timezones of English speakers, such as India or the the other
   Australian timezones, are not currently added because they are
   already guessed correctly.)

* Checks NEXT occurrence dates of each timezone offset transition (standard to
  daylight and daylight to standard) to see if OS timezone transition matches.
  (Previous version checked start date (FIRST occurrence) of currently 
   applicable rule of timezone.  This produced problems in zones such as
   Asia/Baghdad due to inconsistent data:  the rules in the data says
   sundays, but the DTSTART dates are not Sundays.  
   [This was a little confusing to debug because the rule in the cal data
    http://lxr.mozilla.org/mozilla/source/calendar/base/src/tzdata.c#1201
    says the transitions are on sundays, but the rule in the Olsen data
http://lxr.mozilla.org/mozilla/source/calendar/libical/zoneinfo/Asia/Baghdad.ics
    says it occurs on the 1st of the month, not sunday (sunday is often
    a work day in Islamic countries).  This may be because the vzic
    program used to convert the data was run without the --pure option,
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/calendar/base/src/tzdata.c&rev=HEAD&mark=1.6
    meaning compatibility changes for some version of Microsoft 
    parsing (and timezones?).
http://dspace.dial.pipex.com/prod/dialspace/town/pipexdsl/s/asbm26/vzic/
    This might mean it is inconsistent with some OS that uses the
    'pure' Olsen data for its timezones (Linux?).])

* Adds another weak level of match, for dates that are off by up to a
  week.  A timezone with transition dates that match within hours is
  preferred, but if none is found, a timezone with transition dates
  that are a week or less off can be matched.
  (This is useful for guessing timezones such as Africa/Cairo, because
   the rule can be off by a week.  
   [The simplified rule in the cal data
http://lxr.mozilla.org/mozilla/source/calendar/base/src/tzdata.c#91
    specifies the last week of September, while the rule in the Olsen data
http://lxr.mozilla.org/mozilla/source/calendar/libical/zoneinfo/Africa/Cairo.ics#14  
    specifies the friday that is in either the last 6 days of september
    or the first day of october.  So the cal rule can be off by a week.]
   It can also find a usable match for timezones such as Europe/Bucharest.
   [The w2k OS timezone disagrees with the cal data rule for 
    Europe/Bucharest by a month (September vs. October), 
http://lxr.mozilla.org/mozilla/source/calendar/base/src/tzdata.c#1789
    but the w2k OS timezone closely matches the Asia/Amman rule 
http://lxr.mozilla.org/mozilla/source/calendar/base/src/tzdata.c#1171
    except for day of the week (changes on thursday, not Sunday).])

* Removes hidden dependence on gCalendarBundle so gCalendarBundle
  doesn't have to be defined in every window that uses it (and
  easier to debug).

Tested on w2k in all US timezones, Halifax, GMT, WET, Baghdad, Afghanistan, India, Singapore/Hongkong, all Australian timezones, Japan/Korea, New Zealand, plus some nearby timezones including those mentioned above.  To test I used time control panel (right click clock in task bar, click adjust date/time, click timezone tab) and left it open, using the 'apply' button after selecting a timezone.  I did have to restart TB after each change.  With the current patch, guessed timezones produce a warning on Javascript console, so it just means starting it up after changing the timezone and looking in the javascript console to see if it guessed ok.

Linux/MacOS: not tested.  It would be interesting to know if it also works well in other operating systems, since these operating systems may use different timezone rules than w2k, so they may or may not match as well.
Attachment #213612 - Attachment is obsolete: true
Comment on attachment 214180 [details] [diff] [review]
v2 check next occurrence dates; allow week fuzz; remove gCalendarBundle dependency

Requesting review, so this patch is not forgotten
Attachment #214180 - Flags: first-review?(jminta)
Comment on attachment 214180 [details] [diff] [review]
v2 check next occurrence dates; allow week fuzz; remove gCalendarBundle dependency

What kind of impact on startup performance is this going to have?  Doing something like findCurrentTimePeriod 2*343 times sounds quite painful.
I got the (possibly incorrect) impression from ctalbert that he had knowledge of system APIs that might actually allow us to get the right timezone from the OS instead of having to guess.  CCing him in case that's true and there happens to be an easier way to go about this...
(patch -l -p 2 -i file.patch)

Refresh patch; v2 patch failed to apply.
Attachment #214180 - Attachment is obsolete: true
Attachment #218559 - Flags: first-review?(jminta)
Attachment #214180 - Flags: first-review?(jminta)
(In reply to comment #4)
> (From update of attachment 214180 [details] [diff] [review] [edit])
> What kind of impact on startup performance is this going to have?  Doing
> something like findCurrentTimePeriod 2*343 times sounds quite painful.
> 

The guess is skipped if the user has set a timezone preference.

The increase in startup time is pretty negligible compared to the
total startup time, which is dominated by disk performance.

Below times are on a 700MHz machine.  
The compute times will probably be less on faster processors.

  With new/empty profile: 

    Cold start (worst case: nothing in disk-caches):
      With patch:    22 seconds
      Without patch: 22 seconds

    Hot restart (best case: everything in disk-caches):
      With patch:     3.7 seconds
      without patch:  3.3 seconds 
                      ---
                      0.4 seconds

  With test profile with about 30 items in 3 calendars:

    Cold start (worst case: nothing in disk-caches): 
      With patch:    27 seconds
      Without patch: 27 seconds

    Hot restart (best case: everything in disk-caches):
      With patch:     7.8 seconds
      Without patch:  7.4 seconds
		      ---
		      0.4 seconds compute time
(In reply to comment #5)
> ..system APIs that might actually allow us to get the right timezone from the
> OS instead of having to guess. 

That sounds like bug 300605.

For windows: bug 300605 comment 6 indicates that Windows does not seem to
provide locale-independent timezone ids, only localized timezone
names.  If so, that would mean keeping a set of files to map windows
timezone names in each locale to the Olsen timezone name.  And that
still might not work if the application only searched the current
locale timezone names and the OS locale didn't match the application
locale.

There seems to be no cross-platform OS timezone API, so this guess
function may be a useful backup even if some platforms do have a timezone
API.
When confronted with the same issue at Simdesk, we hunted for some kind of appreciable timezone cross-OS API, but found nothing that was really usable across various OS's. We were predominately interested in Linux and Windows, so we fashioned a best guess mapping from Windows to Olsen (based off your guess code, actually), and used the actual timezone name on Linux since it uses Olsen names. Of course, there is no guarantee that the Olsen names on Linux will match the Olsen names in your app either, (depending on versions of the Olsen database used), so it's always a game of chance.

(In reply to comment #7)
>     Hot restart (best case: everything in disk-caches):
>       With patch:     3.7 seconds
>       without patch:  3.3 seconds 
>                       ---
>                       0.4 seconds
> 
>     Hot restart (best case: everything in disk-caches):
>       With patch:     7.8 seconds
>       Without patch:  7.4 seconds
>                       ---
>                       0.4 seconds compute time
This, to me, looks pretty bad.  I also have a couple questions about what exactly was tested. 1.) What TZ was being returned here?  ie. did we have to iterate through all the TZs or just some for this test?  2.)  Won't this similarly impact event-creation, since window-context requires the guess to be made again?

Either way, a 5%-15% increase in startup time seems like a pretty big price to pay, in my mind.
1. Yes, it iterated through all the timezones
   (it reported the guess which appears in the js console; 
    it doesn't report if there was a name match and so it was able to quit loop early.)
2. I don't see why it would affect event creation.
   (Maybe you mean event dialog popup.  The guess should run at most once per 
    application startup, and that timezone should be shared with the
    event dialog.  Fixing that is beyond the scope of this bug.)
 
The Empty Profile Hot Start case will be rare for users.  If the user has enough memory that everything is always in disk cache, then most users that frequently access the calendar will just leave the application open.  (Many will also leave the application open so they can receive alarms.)  So in this case startup is rare, usually maybe once or twice a day.  If the application and files are not in disk cache, then startup time is much longer, so in that case the added computation time is a smaller percentage of startup time, maybe lost in the noise (variation due to disk scheduling).

The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Assignee: base → nobody
These are large places, that are getting guessed wrong:

London, Paris, Tokyo, Halifax, Singapore, Brisbane, Melbourne, Sydney, Auckland.

Most of those timezone names translate straight into some fairly well-populated geographical areas (rather than the incorrect ones which are practically unpopulated):

United Kingdom     60,209,500
France             63,587,700
Japan             128,085,000
Nova Scotia         1,000,000
Singapore           4,326,000
80% of Australia   16,000,000
New Zealand         4,147,972

So that is a total of 275 million people who will be getting the wrong timezone automatically guessed the first time they try and use the calendar.

It's kinda strange too, because the timezone must be somewhere on the system.  I don't know where it is stored on other OS's, but on my computer /etc/timezone clearly says "Pacific/Auckland".  Even after I have selected a preferred timezone, it seems that events are created in Antarctica/McMurdo.
Blocks: 363121
Attachment #218559 - Flags: first-review?(jminta) → review?(jminta)
Whiteboard: [cal relnote]
Attachment #218559 - Flags: review?(jminta) → review?(ctalbert)
Assignee: nobody → gekacheka
Status: NEW → ASSIGNED
Clint, any estimate when you might get to the review in this bug?
(In reply to comment #14)
> Clint, any estimate when you might get to the review in this bug?
> 

Will get to it this week.  CalendarUtils.js went away quite some time ago, so I have to port this to the code in base/src/calUtils.js, but I think it should be pretty straightforward.  I don't anticipate in making a material change to the code in the patch as a result of that. 
I'm starting to think this may have to get in line behind the calITimezoneResolver patch (bug 314339).  I think that this functionality may have to be tweaked once the calITimeozneResolver lands.
Flags: blocking-calendar0.7?
Comment on attachment 218559 [details] [diff] [review]
v3 check next occurrence dates; allow week fuzz; remove gCalendarBundle dependency (refresh patch)

I'm unfortunately going to have to r- this.  I like the code, I understand it pretty well.  But, when unbitrotting and playing with it, I can't get it to work.  Can you submit a new patch?  Sorry about the delayed review on this.
Attachment #218559 - Flags: review?(ctalbert) → review-
(patch -l -p 1 -i file.patch)

Refreshing patch.  See comment 1 and comment 2 for a description.
This seems to work for me.  


Tested on w2k by 
* removing preference for calendar.timezone.default in userprofile/prefs.js
* repeat
  - set OS timezone to a city (click right on clock in taskbar)
  - restart: sunbird -jsconsole 
  - observe guess in error console and verify whether guessed timezone city 
    makes sense for OS city.
OS city timezones to try include all the special case cities listed in the English locale properties section of this patch, plus other city timezones around the world with significant internet-using English speakers who might use Sunbird or Lightning, such as Calcutta, Brisbane, etc.


Changes to English locale data: Indianapolis has joined EST/EDT, so it is no longer a recognizably different from New York, and was removed.  Added Lagos (WAT), Johannesburg (SAST), Nairobi (EAT), Buenos Aires, and Mexico City as better guesses for their time zones.  (Brazil/Sao Paulo note: according to wikipedia, existence of summer-time, and if it exists the dates of change, are set yearly by local decrees, and they vary widely year to year.  Therefore Sunbird/Lightning will have trouble guessing Sao Paulo as a timezone unless OS timezone data and Sunbird timezone data agree [source data was updated at about the same date].)
Attachment #218559 - Attachment is obsolete: true
Attachment #285686 - Flags: review?(ctalbert)
Comment on attachment 285686 [details] [diff] [review]
v4 fix hemisphere errors; accept list of locale tz's; check next occurrence dates; allow week fuzz (refresh patch)

I have tested this patch on Mac OS X, and I must say that I'm very impressed.  I think this one patch alone will do more to solve 70% of the timezone issues that people randomly face (except the non-mozilla timezone issue) than anything else.  Very nice job!

I have some nits below...

>diff -r 3385c481dee8 mozilla/calendar/base/src/calUtils.js
>--- a/mozilla/calendar/base/src/calUtils.js	Sat Oct 20 11:30:15 2007 -0400
>+++ b/mozilla/calendar/base/src/calUtils.js	Sun Oct 21 18:21:06 2007 -0400
>@@ -116,154 +116,343 @@ function calendarDefaultTimezone() {
>  * winter JSdates.  However, when available, we also use the name of the
>  * timezone in the JSdate, or a string-bundle term from the locale.
>  *
>- * @returns  a ICS timezone string.
>+ * @return a mozilla ICS timezone string.
> */
> function guessSystemTimezone() {

>+
>+    // Check if Olsen timezone matches OS/JSDate timezone properties:
His name is spelled "Olson", please correct this.  Also, the first time we mention the Olson timezone database, we should use its formal name: "Olson ZoneInfo timezone".  After this comment we can just call it the "Olson timezone" or what have you.

>+    // returns 2=match-within-hours, 1=match-within-week, 0=no-match
>+    function systemTZMatchesTimeShiftDates(tzId, subComp) {
>+        // Verify autumn and spring shifts also occur in system timezone
>+        // (jsDate) on correct date in correct direction.
>+        const autumnShiftJSDate =
>+            findCurrentTimePeriod(tzId, subComp, "STANDARD", true);
>+        const afterAutumnShiftJSDate = new Date(autumnShiftJSDate);
>+        const beforeAutumnShiftJSDate = new Date(autumnShiftJSDate);
>+        const springShiftJSDate =
>+            findCurrentTimePeriod(tzId, subComp, "DAYLIGHT", true);
>+        const beforeSpringShiftJSDate = new Date(springShiftJSDate);
>+        const afterSpringShiftJSDate = new Date(springShiftJSDate);
>+        // Try with 6 HOURS fuzz in either direction, since OS and libical
>+        // may disagree on the exact time of shift (midnight, 2am, 4am, etc).
>+        beforeAutumnShiftJSDate.setHours(autumnShiftJSDate.getHours()-6);
>+        afterAutumnShiftJSDate.setHours(autumnShiftJSDate.getHours()+6);
>+        afterSpringShiftJSDate.setHours(afterSpringShiftJSDate.getHours()+6);
>+        beforeSpringShiftJSDate.setHours(beforeSpringShiftJSDate.getHours()-6);
>+        if ((beforeAutumnShiftJSDate.getTimezoneOffset() <
>+             afterAutumnShiftJSDate.getTimezoneOffset()) &&
>+            (beforeSpringShiftJSDate.getTimezoneOffset() >
>+             afterSpringShiftJSDate.getTimezoneOffset())) {
>+            return 2;
>+        }          
This function's variable names are very confusing.  I realize that when you state "beforeAutumnshift" you mean "beforeShiftIntoDST", since this occurs in the autumnal season for both hemispheres.  However, I think the confusion comes from those of us being in the northern hemisphere not really realizing that the southern hemisphere is in autumn in April.  

While this is an excellent mind-broadening exercise (which I approve), I think you might consider renaming the *Autumn* variables to "ShiftIntoDST" and the "Spring" variables to "ShiftOutOfDST".  I think this would make it more apparent how hemisphere neutral this code actually is.  Do you think it is better for future developers to read the code like this (since those without bunches of exposure to timezones will still tend to think of Autumn and Spring shifts relative to where they live)?

>+        // Try with 7 DAYS fuzz in either direction, so if no other tz found,
>+        // will have a nearby tz that disagrees only on the weekday of shift
>+        // (sunday vs. friday vs. calendar day), or off by exactly one week,
>+        // (e.g., needed to guess Africa/Cairo on w2k in 2006).
>+        beforeAutumnShiftJSDate.setDate(autumnShiftJSDate.getDate()-7);
>+        afterAutumnShiftJSDate.setDate(autumnShiftJSDate.getDate()+7);
>+        afterSpringShiftJSDate.setDate(afterSpringShiftJSDate.getDate()+7);
>+        beforeSpringShiftJSDate.setDate(beforeSpringShiftJSDate.getDate()-7);
>+        if ((beforeAutumnShiftJSDate.getTimezoneOffset() <
>+             afterAutumnShiftJSDate.getTimezoneOffset()) &&
>+            (beforeSpringShiftJSDate.getTimezoneOffset() >
>+             afterSpringShiftJSDate.getTimezoneOffset())) {
>             return 1;
>         }
Obviously if you change the variable names in the above comment, you'll need to change them here too.

>-    // Everything failed, so this is our only option.
>-    return "floating";
>+                case 3:
>+                    return tzId;
>+                }
>+            } catch (ex) {
>+                Components.utils.reportError
>+                    ("skipping locale timezone '"+bareTZId+"': "+ ex);
We need to grab these strings from the properties file so that they can be localized.

>+    // If reach here, there were no score=3 matches, so Warn in console.
>+    try { 
>+        const jsConsole = Components.classes["@mozilla.org/consoleservice;1"]
>+                           .getService(Components.interfaces.nsIConsoleService);
>+        switch(probableTZScore) {
>+        case 0:
>+            jsConsole.logStringMessage
>+                ("Warning:  Using \"floating\" timezone.\n"+
>+                 "No timezone data matched the operating system timezone.");
Let's get this string from the properties file so that we can localize it ^^

>+        case 1: case 2:
..snip..
>+            if (probableTZScore == 1) {
>+                // score 1 means has daylight time,
>+                // but transitions start on different weekday from os timezone.
>+                function weekday(icsDate) {
>+                    var calDate = createDateTime();
>+                    calDate.timezone = tzId;
>+                    calDate.icalString = icsDate;
>+                    return calDate.jsDate.toLocaleFormat("%a");
>+                }
>+                var standardStart = getIcalString(standard, "DTSTART");
>+                var standardStartWeekday = weekday(standardStart);
>+                var standardRule  = getIcalString(standard, "RRULE");
>+                var standardText = 
>+                    ("  Standard: "+standardStart+" "+standardStartWeekday+"\n"+
>+                     "            "+standardRule+"\n");
>+                var daylightStart = getIcalString(daylight, "DTSTART");
>+                var daylightStartWeekday = weekday(daylightStart);
>+                var daylightRule  = getIcalString(daylight, "RRULE");
>+                var daylightText =
>+                    ("  Daylight: "+daylightStart+" "+daylightStartWeekday+"\n"+
>+                     "            "+daylightRule+"\n");
>+                warningDetail =
>+                    ((standardStart < daylightStart
>+                      ? standardText + daylightText
>+                      : daylightText + standardText)+
>+                     "This timezone almost matches the operating system "+
>+                     "timezone.\n"+
>+                     "For this rule, the next transitions between "+
>+                     "daylight and standard time\n"+
>+                     "occur at most a week from the "+
>+                     "operating system timezone transitions.\n"+
>+                     "There may be discrepancies in the data, such as "+
>+                     "differing start date,\n"+
>+                     "or differing rule, or "+
>+                     "approximation for non-Gregorian rule.");
This string needs to be added to the properties file

>+            } else {
>+                warningDetail =
>+                    "This Olsen timezone seems to match the operating system "+
>+                    "timezone.";
This one too, with "Olson" corrected too.

>+            }
>+            jsConsole.logStringMessage
>+                ("Warning:  Using guessed timezone\n"+
>+                 "  "+tzId+" (UTC"+standardTZOffset+
>+                 (!daylightTZOffset? "": "/"+daylightTZOffset)+
>+                 ").\n"+warningDetail);
Yep, this too.

>diff -r 3385c481dee8 mozilla/calendar/locales/en-US/chrome/calendar/calendar.properties
>+#    Translators, please put the most likely timezone(s) that the people using
>+#    your locale will be in.  Use the Olsen-timezone name *in English*,
>+#    ie "Europe/Paris", (continent or ocean)/(largest city in timezone). 
>+#    (Particularly needed to guess the most relevant timezones if there are
>+#    similar timezones at the same June/December GMT offsets with alphabetically
>+#    earlier Olsen timezone names.)
This is a very good description, but I think you should also mention in here that the order of the timezones in the likelyTimezone string is not important (unless I read the code wrong and it is important).  Either way, please let the localizers know, because I imagine they would ask that question.

As I said, excellent work.  Please get those nits addressed, and we can carry my review forward.
r=ctalbert
Attachment #285686 - Flags: review?(ctalbert) → review+
This is nearing checkin, so it might not be needed, but I went ahead and transitioned the flags on this one.  It's definitely wanted-0.8, I guess I have authority to unilaterally make that decision :-).  But it's probably a blocker for 0.8, so I've requested that too.
Flags: wanted-calendar0.8+
Flags: blocking-calendar0.8?
Flags: blocking-calendar0.7?
This should block 0.8 as this bug is an integral part of our timezone improvement story.
Flags: blocking-calendar0.8? → blocking-calendar0.8+
> >+    var standardStart = getIcalString(standard, "DTSTART");
> >+    var standardStartWeekday = weekday(standardStart);
> >+    var standardRule  = getIcalString(standard, "RRULE");
> >+    var standardText = 
> >+        ("  Standard: "+standardStart+" "+standardStartWeekday+"\n"+
> >+         "            "+standardRule+"\n");
> >+    var daylightStart = getIcalString(daylight, "DTSTART");
> >+    var daylightStartWeekday = weekday(daylightStart);
> >+    var daylightRule  = getIcalString(daylight, "RRULE");
> >+    var daylightText =
> >+        ("  Daylight: "+daylightStart+" "+daylightStartWeekday+"\n"+
> >+         "            "+daylightRule+"\n");
> >+    warningDetail =
> >+        ((standardStart < daylightStart
> >+          ? standardText + daylightText
> >+          : daylightText + standardText)+
> >+         "This timezone almost matches the operating system "+
> >+         "timezone.\n"+
> >+         "For this rule, the next transitions between "+
> >+         "daylight and standard time\n"+
> >+         "occur at most a week from the "+
> >+         "operating system timezone transitions.\n"+
> >+         "There may be discrepancies in the data, such as "+
> >+         "differing start date,\n"+
> >+         "or differing rule, or "+
> >+         "approximation for non-Gregorian rule.");
> This string needs to be added to the properties file

The string concatenation will cause localization nightmare. There should be one full string per message with dynamic parameters that will be replaced e.g. by the daylight start date.
(patch -l -p 1 -i file.patch)

Localized warning messages as requested.  For localizers, added explanation of significance of order of tz ids.  Added note on how to test, and hints on how to trigger the warning messages.
Fixed spelling of Olson's name, and used the term "ZoneInfo".
In code, added explanation that autumn and spring shifts are local and differ in northern and southern hemisphere.  (I found the suggested names harder to distinguish, so I kept these and added explanation.)

Thanks for the review!
Attachment #285686 - Attachment is obsolete: true
Attachment #288162 - Flags: review?(ctalbert)
(In reply to comment #8)
> For windows: bug 300605 comment 6 indicates that Windows does not seem to
> provide locale-independent timezone ids, only localized timezone
> names.
See bug 300605 comment 11 for a way to get non-localized timezone names on Windows.
Comment on attachment 288162 [details] [diff] [review]
v5 fix hemisphere errors; accept list of locale tz's; check next occurrence dates; allow week fuzz (localized warnings)

Awesome, looks good. Thanks for the patch!
Attachment #288162 - Flags: review?(ctalbert) → review+
Attachment #288162 - Attachment description: v4 fix hemisphere errors; accept list of locale tz's; check next occurrence dates; allow week fuzz (localized warnings) → v5 fix hemisphere errors; accept list of locale tz's; check next occurrence dates; allow week fuzz (localized warnings)
(patch -l -p 1 -i file.patch)

Taking hints from bug300605 and 
"Why Discovering the Local Time Zone is a Hard Problem"
http://www.chronos-st.org/Discovering%20the%20Local%20Time%20Zone--Why%20It's%20a%20Hard%20Problem.html
added a stage to try to find the local timezone from the operating system.
  For windows, this means getting the localized timezone name, and
scanning the windows registry for the timezone with the same localized name,
and using its registry key to lookup the ZoneInfo timezone id for known timezones.  For other operating systems (mostly unix derived) this means looking in either the TZ environment variable or the first line of /etc/timezone or /etc/TIMEZONE for a ZoneInfo timezone id or a path that ends with a ZoneInfo timezone id.  (It does not scan the OS ZoneInfo database, since scanning the calendar ZoneInfo database is what we're trying to avoid and what it does if no OS timezone is found.)  If not found, continues as before.
  The operating system timezone data may disagree with the ZoneInfo timezone data if either or both are out of date, or if daylight saving time is disabled in a timezone that observes daylight saving time (via a simple checkbox on windows). Therefore, the algorithm checks whether the data agrees, and if there is a disagreement produces a warning message and searches for a timezone that matches better.  For example, on operating systems set to the old Indiana timezone (which did not observe DST, while the current ZoneInfo timezone for Indiana does observe DST), this will find another timezone at the same offset that does not observe DST, so alarm times will always agree with the operating system clock.
  Another improvement is that this version produces a warning message with more detail about the evidence used to guess the guessed timezone, such as the operating system timezone setting.  This will help users as well as developers better diagnose cases when the guess is wrong.
  (The conversion table for Windows 98 registry keys is separate so that it can be easily removed when Windows 98 is no longer supported after TB and SB move to Gecko 1.9.)
Attachment #288162 - Attachment is obsolete: true
Attachment #291094 - Flags: review?(ctalbert)
(In reply to comment #26)
> [...] looking in either the TZ environment variable or the first line of
> /etc/timezone or /etc/TIMEZONE for a ZoneInfo timezone id or a path that ends
> with a ZoneInfo timezone id.
On Red Hat and Fedora the timezone setting is saved in /etc/sysconfig/clock. The files /etc/timezone or /etc/TIMEZONE do not exist, and the TZ environment variable isn't set.
FOr what it's worth, I have no TZ env var on my mac. All I have is a /etc/localtime, which is a softlink to a real timezone file (in my case, /usr/share/zoneinfo/Europe/Amsterdam). That timezone file is a binary thing, so not parsable. You can only look at the filename
But i wouldn't object to getting the current patch in, before fixing the mac stuff. As long as it doesn't regress anything, it's fine.
(patch -l -p 1 -i file.patch)

Additions:
- if TZ has a value, see if it matches tzRegex before returning it, else "".
- if /etc/localtime is a symlink, see if its target path matches tzRegex.
  (MacOSX testers please test)
- if /etc/sysconfig/clock exists, looks for a line that matches tzRegex.
  (RedHat testers please test)

Also source filepath now appears in guess warning message,
to help diagnose the source of the OS timezone that it found.
Attachment #291094 - Attachment is obsolete: true
Attachment #291127 - Flags: review?(ctalbert)
Attachment #291094 - Flags: review?(ctalbert)
Works fine on my Fedora box.

+            var tzRegex   = new RegExp(".*((?:"+continent+"|"+ocean+")"+
+                                       "(?:[/][A-Z_a-z]+)+)");
Hyphen is also allowed in timezone names, e.g. America/Port-au-Prince.
(patch -l -p 1 -i file.patch)

Thanks for catching the hyphen issue!  Patch updated to allow hyphens.

(builders/testers: comment #18 has test steps.  Would be nice to report both OS and which source of timezone it uses [a guess warning msg] so we know which sources are tested.)
Attachment #291127 - Attachment is obsolete: true
Attachment #291550 - Flags: review?(ctalbert)
Attachment #291127 - Flags: review?(ctalbert)
Clint, this bug is an 0.8 blocker. Any chance that you review this soon?
Flags: wanted-calendar0.8+
Comment on attachment 291550 [details] [diff] [review]
v8 fix hemisphere errors; accept list of locale tz's; check next occurrence dates; allow week fuzz (try registry;TZ,/etc/localtime,/etc/timezone,/etc/sysconfig/clock;allow hyphen)

Looks good.  Sorry for the delay.
Attachment #291550 - Flags: review?(ctalbert) → review+
Keywords: checkin-needed
Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Why all the strings begin with \ char and are not in same line together with string identifier? This is messing up local compare-locales script and I'm afraid it will mess up l10n tinderboxen too.
I agree. This should fix.
Attachment #297873 - Flags: review?(ctalbert)
Seems this regressed Bug 413029.
Depends on: 413029
Attachment #297873 - Flags: review?(ctalbert)
I backed out this patch for now. I hope everything went well, its my first backout. 
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This isn't entirely complete, but these changes enable it to be a working calendar anyway.  There is still a little bit more to do because GetTimezone now returns the component already stripped out, and I think that's the source of our issues.
Attachment #297887 - Flags: review?(gekacheka)
Comment on attachment 297887 [details] [diff] [review]
Patch to fix the issues in 413029

This patch doesn't apply.

>-                tzId = icsSvc.tzIdPrefix + tzId;
>+                tzId = tzSvc.tzIdPrefix + tzId;
case error: it's calITimezoneService::tzidPrefix
(patch -l -p 1 -i file.patch)
Updated patch.  (Thanks for hints in the partial patches.)

CHANGES

* update for bug 400950 changes
  calIICSService.getTimezone(id) -> calITimezoneService.getTimezone(id)
  calIDateTime.timezone: string -> calITimezone
* added code to handle special case for UTC timezone
  since calITimezoneService.UTC.component is null
  (would be simpler if UTC.component was set)
* one line properties (comment 35)
  (continuation lines are legal .properties file syntax, but
  currently exceed limits of compare-locales.pl and compare-locales.py)

TESTED AS FOLLOWS:

1. USERPROFILE/prefs.js: remove saved preference for calendar.timezone.local
   (important, otherwise does not call guessSystemTimezone).

  //user_pref("calendar.timezone.local", "floating"); 

2. calUtils.js:161 calendarDefaultTimezone:
   comment out code that saves guessed preference 

  //setPref("calendar.timezone.local", "CHAR", calendarDefaultTimezone.mTz.tzid);

3. build, then test various OS timezone settings, watching Error console.
   (on w2k can set timezone via right-click on taskbar clock,
    click apply after each setting, restart "sunbird -jsconsole")

   * if exact match found (to tz name in new Date()), then no message.
   * otherwise, for windows should say something like:

 Warning:  Using guessed timezone
   /mozilla.org/20070129_1/America/New_York (UTC-0500/-0400).
 This ZoneInfo timezone seems to match the operating system timezone this year.
 This ZoneInfo timezone was chosen based on the operating system timezone
 identifier "Eastern Standard Time".  

   * otherwise, for Unix(MacOS,Linux,Solaris,...), might say something
     like the following (though source may differ from "/etc/TIMEZONE"):

 Warning:  Using guessed timezone
   /mozilla.org/20070129_1/America/New_York (UTC-0500/-0400).
 This ZoneInfo timezone seems to match the operating system timezone this year.
 This ZoneInfo timezone was chosen based on the operating system timezone
 identifier "/etc/TIMEZONE: /EXAMPLE/America/New_York".

4. calUtils.js:403: guessSystemTimezone
   Temporarily add code to skip the OS timezone
   (to test for other operating systems or unmatched OS timezones):

    // First, try to detect operating system timezone.
    try { 
+       throw "SKIP-OS-TZ"; // TESTING: throw to skip OS timezone
 
5. build, then test timezone settings in 'likelyTimezones' property in
  mozilla/calendar/locales/en-US/chrome/calendar/calendar.properties
  (preferred timezone at hour offset).
  For each timezone, should get message in Error console:

 Warning:  Using guessed timezone
   /mozilla.org/20070129_1/America/New_York (UTC-0500/-0400).
 This ZoneInfo timezone seems to match the operating system timezone this year.
 This ZoneInfo timezone was chosen based on matching the operating system
 timezone with likely timezones for internet users using US English.

6. test with various OS timezone settings not in 'likelyTimezones'
   property particularly those in southern hemisphere (e.g.,
   Santiago).  (For some it will guess a different timezone with same
   offsets but earlier in alphabetical list of timezone ids, e.g., if
   the OS timezone is set to Istanbul but Asia/Istanbul is not in the
   likely timezones of the locale, then it might guess the Asia/Beirut
   timezone, which is earlier alphabetically.)

 Warning:  Using guessed timezone
   /mozilla.org/20070129_1/Asia/Beirut (UTC+0200/+0300).
 This ZoneInfo timezone seems to match the operating system timezone this year.
 This ZoneInfo timezone was chosen based on matching the operating system
 timezone with known timezones in alphabetical order of timezone id.

7. cleanup
undo step 2
undo step 4
Attachment #291550 - Attachment is obsolete: true
Attachment #297873 - Attachment is obsolete: true
Attachment #297887 - Attachment is obsolete: true
Attachment #298029 - Flags: review?(ctalbert)
Attachment #297887 - Flags: review?(gekacheka)
Comment on attachment 298029 [details] [diff] [review]
v9 fix hemisphere errors; accept list of locale tz's; check next occurrence dates; allow week fuzz (try registry;TZ,/etc/localtime,/etc/timezone,/etc/sysconfig/clock;allow hyphen)

r=ctalbert.  Thanks for the quick turnaround on this Gekachecka!  I have done some more testing with it and found that the only issue I have is that a couple of Australian timezones are being misguessed, and Asia/Jerusalem is not being guessed properly.  I will open a follow on bug for this, because I want to get this patch checked in as soon as possible (because of the string freeze next week).
Attachment #298029 - Flags: review?(ctalbert) → review+
Follow on bug is bug 413265
Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
From WindowsNTToZoneInfoTZId.properties:
Yakutsk Standard Time:	Asia/Yakutskd
should be
Yakutsk Standard Time:	Asia/Yakutsk

Is this correct?
Philipp, thanks for checking this in.  I should have checked it in sooner but was waiting for the tree to open back up.

Martin, Yes, your assertion is correct.  I'll add this to my tracking bug for follow-on issues from this patch, bug 413265.
Blocks: 413265
You need to log in before you can comment on or make changes to this bug.