Closed Bug 410931 Opened 16 years ago Closed 16 years ago

Update internal timezone definitions

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: ssitter, Assigned: gkw)

References

Details

Attachments

(7 files, 6 obsolete files)

708.06 KB, patch
cmtalbert
: review+
Details | Diff | Splinter Review
363.47 KB, patch
dbo
: review+
Details | Diff | Splinter Review
2.12 KB, patch
dbo
: review+
Details | Diff | Splinter Review
3.86 KB, application/octet-stream
Details
11.58 KB, patch
cmtalbert
: review+
Details | Diff | Splinter Review
369.18 KB, patch
cmtalbert
: review+
Details | Diff | Splinter Review
161.87 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
Update internal timezone definitions

There were some recent timezone changes e.g. in South America. For the 0.8 release we should include updated timezone information. As of today the current Olson release if 2007k. 

This could be done together with the already planed timezone backend changes.
Flags: wanted-calendar0.8?
See also http://groups.google.com/group/mozilla.support.calendar/msg/43983804689c1b58
Flags: wanted-calendar0.8? → blocking-calendar0.8+
My first patch for Calendar!

Changed from -0400 to -0430. I have no idea what else to change.
Attachment #296569 - Flags: review?(ctalbert)
Attached patch better, more complete patch (obsolete) — Splinter Review
A much better, more comprehensive patch.

Incorporating all changes from Olson database 2007k dated 31 Dec 2007.

Specifically, new timezone info for America/Marigot, America/Resolute, America/St_Barthelemy and America/Indiana/Tell_City over the previous data dated Jan 2007.
Assignee: nobody → nth10sd
Attachment #296569 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #296590 - Flags: review?(ctalbert)
Attachment #296569 - Flags: review?(ctalbert)
Having a quick look I think "Atlantic/Jan_Mayen" has been purged out. we shouldn't do that since we don't know if someone has referenced it in his storage calendar.
many thanks to ctalbert and ssitter in IRC for this one, and dbo for pointing out Jan Mayen.
Attachment #296590 - Attachment is obsolete: true
Attachment #297384 - Flags: review?(ctalbert)
Attachment #296590 - Flags: review?(ctalbert)
Comment on attachment 297384 [details] [diff] [review]
version 2 incorporating Jan Mayen changes

>Index: base/src/calTimezoneService.cpp
>         if (tzid.EqualsLiteral("/mozilla.org/20050126_1/Africa/Asmera")) {
>             _retval.AssignLiteral("/mozilla.org/20070129_1/Africa/Asmara");
>         } else if (tzid.EqualsLiteral("/mozilla.org/20050126_1/Africa/Timbuktu")) {
>             _retval.AssignLiteral("/mozilla.org/20070129_1/Africa/Bamako");
>         }
>     } else if (continent.EqualsLiteral("Atlantic")) {
>         if (tzid.EqualsLiteral("/mozilla.org/20050126_1/Atlantic/Faeroe")) {
>             _retval.AssignLiteral("/mozilla.org/20070129_1/Atlantic/Faroe");
>+        } else if (tzid.EqualsLiteral("/mozilla.org/20070129_1/Atlantic/Jan_Mayen")) {
>+            _retval.AssignLiteral("/mozilla.org/20071231_1/Europe/Oslo");
>         }
>     } else if (continent.EqualsLiteral("America")) {
>         if (tzid.EqualsLiteral("/mozilla.org/20050126_1/America/Argentina/ComodRivadavia")) {
>             _retval.AssignLiteral("/mozilla.org/20070129_1/America/Argentina/Catamarca");
>         } else if (tzid.EqualsLiteral("/mozilla.org/20050126_1/America/Louisville")) {
>             _retval.AssignLiteral("/mozilla.org/20070129_1/America/Kentucky/Louisville");
>         }
>     } else if (continent.EqualsLiteral("Europe")) {

Gary, w.r.t. the above code: IMO you need to adopt all aliases to 20071231_1, not only Europe/Oslo.
Moreover, please keep in mind that storage calendar won't upgrade its TZIDs to 20071231_1, see <http://mxr.mozilla.org/mozilla1.8/source/calendar/providers/storage/calStorageCalendar.js#1169>. However, this will still work since the timezone service can handle those TZIDs.
Attached patch patch version 3 (obsolete) — Splinter Review
a 3rd version changing all 20070129_1 entries in calTimezoneService.cpp to 20071231_1.

Also changes the entries in calStorageCalendar.js (a comment) and a unit test case in test_datetime.js since these are the only 2 other files with 20070129_1 that were not touched on previously.
Attachment #297384 - Attachment is obsolete: true
Attachment #299301 - Flags: review?(ctalbert)
Attachment #297384 - Flags: review?(ctalbert)
Gary, think of the case that people may already have upgraded their storage.sdb to the latest schema version. That way calStorageCalendar won't run into the migration code again (and won't update the TZIDs).
Attached patch storage TZID update piece (obsolete) — Splinter Review
More testing welcome!
Attachment #299482 - Flags: review?(ctalbert)
Attached patch patch v3.1Splinter Review
removed my previous comment changes in calStorageCalendar.js in this patch as Daniel has provided a better one.
Attachment #299301 - Attachment is obsolete: true
Attachment #299822 - Flags: review?(ctalbert)
Attachment #299301 - Flags: review?(ctalbert)
Comment on attachment 299482 [details] [diff] [review]
storage TZID update piece

>Index: calendar/providers/storage/calStorageCalendar.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/providers/storage/calStorageCalendar.js,v
> 
>-        if (oldVersion == 6 && this.DB_SCHEMA_VERSION >= 7) {
>-            dump ("**** Upgrading schema from 6 -> 7\n");
>+        // run TZID updates both on db of version 6 and 7:
>+        if ((oldVersion == 6 || oldVersion == 7) && this.DB_SCHEMA_VERSION >= 8) {
>+            dump ("**** Upgrading schema from 6/7 -> 8\n");
Daniel as far as I can tell, this if statement is not evaluating to true.  I can't say why that would happen, it makes no sense.  It's got to be something obvious, I'm just too tired to see it.  But because of that the database doesn't get upgraded.

>                 // Update the version stamp, and commit.
>-                this.mDB.executeSimpleSQL("UPDATE cal_calendar_schema_version SET version = 7;");
>+                this.mDB.executeSimpleSQL("UPDATE cal_calendar_schema_version SET version = 8;");
>                 this.mDB.commitTransaction();
>-                oldVersion = 7;
>+                oldVersion = 8;
>             } catch (e) {
>                 dump ("+++++++++++++++++ DB Error: " + this.mDB.lastErrorString + "\n");
>                 Components.utils.reportError("Upgrade failed! DB Error: " +
>                                              this.mDB.lastErrorString);
>                 this.mDB.rollbackTransaction();
>                 throw e;
>             }
>         }
When the database upgrade fails, I DON'T see this catch ^^^ which makes me think we never go into the if statement at all.
> 
>-        if (oldVersion != 7) {
>+        if (oldVersion != 8) {
>             dump ("#######!!!!! calStorageCalendar Schema Update failed -- db version: " + oldVersion + " this version: " + this.DB_SCHEMA_VERSION + "\n");
>             throw Components.results.NS_ERROR_FAILURE;
Instead, I see this dump statement and the resulting throw.  I put in a bunch of dump statements to try to debug this, but none of them are showing up at all.  I'm not sure if I have a bad build or a bad head at this point.

So, I'm going to r- on this until we can get it working.
Attachment #299482 - Flags: review?(ctalbert) → review-
Comment on attachment 299822 [details] [diff] [review]
patch v3.1

I tested the handling of the timezone upgrade on a new profile (no upgrade performed) and it worked very well.  I'm going to r=ctalbert on this part, with one caveat: Jerusalem is specified incorrectly.  I'll add a follow-on patch that properly details Jerusalem.
Attachment #299822 - Flags: review?(ctalbert) → review+
This is the new tzdata.c file containing a proper Jerusalem timezone.  My recommendation is to land the giant patch first, erase tzdata.c, update to the old tzdata.c from CVS, and then apply this patch.  The same will go for the next patch - the update to calendar/libical/zoneinfo/asia/jerusalem.ics, although the ICS file is small enough to simply copy by hand.
Attachment #302089 - Flags: review?(daniel.boelzle)
Attachment #302090 - Flags: review?(daniel.boelzle)
Attached file Tests for the upgrade
These are the ICS files you need to adequately test the upgrade of the timezones to the current definition.  You have to be sure that the Daylight savings updates are applied to Adeliade, Auckland, that Jerusalem is correct, that JanMayen becomes Oslo, and that Caracas gets it's half hour offset.  You should also just vet the other zones here too.

 == Steps ==
1. Start up Calendar using a nightly
2. Import the "import me" calendar into a storage calendar
3. Open the "open me" calendar as an ICS file.
4. Close Calendar
5. Apply the patches to your build, rebuild
6. Open that same profile with the patched build.
7. Use http://www.timeanddate.com/worldclock/advmeeting.html to help check that all timezones are upgraded properly.
Comment on attachment 302089 [details] [diff] [review]
New tzdata.c file with proper specification on Jerusalem

r=dbo
Attachment #302089 - Flags: review?(daniel.boelzle) → review+
Comment on attachment 302090 [details] [diff] [review]
Updated ICS file for Jerusalem

r=dbo
Attachment #302090 - Flags: review?(daniel.boelzle) → review+
Attachment #299482 - Attachment is obsolete: true
Attachment #302622 - Flags: review?(ctalbert)
UTC is needed as a separate time zone. Europe/London is not a replacement because it has a normal time / daylight saving time border in it.
(In reply to comment #19)

Filled the separate bug 417020 for this.

The original file and/or patch #302089 (2008-02-08 00:16 PST) has for the timezones Cairo, Currie, Hobart and Perth identical TZOFFSETFROM and TZOFFSETTO values in standard time. 

Additionally RDATEs are set to keep the Australian DT/DST changes valid since  year 2000 with the new recurrence rules. I assume that the the referendum after the great "Western Australia Daylight Saving Trial 2006-2009" which affects the Perth time zone will give a GO for daylight saving time and that the DT/DST switch days will be changed to the nationwide dates.

Should the single ICS files under mozilla/calendar/libical/zoneinfo/ be maintained? This is done for Jerusalem but for Buenos Aires not for example.
Attachment #303732 - Flags: review?
Attachment #303732 - Flags: review? → review?(ctalbert.moz)
(In reply to comment #21)
> Created an attachment (id=303732) [details]
> Patch v5 for tzdata.c with Australian DST exceptions
Hb, thanks for the patch, it looks really good.  I do have a couple of general questions before I r+ though.
> 
> The original file and/or patch #302089 (2008-02-08 00:16 PST) has for the
> timezones Cairo, Currie, Hobart and Perth identical TZOFFSETFROM and TZOFFSETTO
> values in standard time. 
I'll look at these.  Thanks for mentioning it.  

> 
> Additionally RDATEs are set to keep the Australian DT/DST changes valid since 
> year 2000 with the new recurrence rules. I assume that the the referendum after
> the great "Western Australia Daylight Saving Trial 2006-2009" which affects the
> Perth time zone will give a GO for daylight saving time and that the DT/DST
> switch days will be changed to the nationwide dates.
Where did you pull the rDate information from?

> 
> Should the single ICS files under mozilla/calendar/libical/zoneinfo/ be
> maintained? This is done for Jerusalem but for Buenos Aires not for example.
> 
It's probably not necessary so much.  We basically only have them in there for reference, and the most up-to-date files are only checked in on trunk (which I just discovered).  So, we'll be fixing that with these checkin.


Comment on attachment 303732 [details] [diff] [review]
Patch v5 for tzdata.c with Australian DST exceptions

r=ctalbert

I verified these, and I verified the zones that you updated (Cairo, Curries, Hobart, and Perth). Thanks for catching those.  This looks good.  In the future, the bugzilla user I watch is ctalbert at mozilla dot com.  So, us that one next time you need a review.  Nice work.
Attachment #303732 - Flags: review?(ctalbert.moz) → review+
Comment on attachment 302622 [details] [diff] [review]
storage TZID update piece

>Index: calendar/providers/storage/calStorageCalendar.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/providers/storage/calStorageCalendar.js,v
>+                // Schema changes between v7 and v8:
>+                //
>+                // - Migrate all stored mozilla.org timezones from 20070129_1
>+                //   to 20071231_1.
>+
>                 // Get a list of the /mozilla.org/* timezones used in the db
>                 var tzId;
>                 getTzIds = createStatement(this.mDB,
>                     "SELECT DISTINCT(zone) FROM ("+
>                         "SELECT recurrence_id_tz AS zone FROM cal_attendees  WHERE recurrence_id_tz LIKE '/mozilla.org%' UNION " +
>                         "SELECT recurrence_id_tz AS zone FROM cal_events     WHERE recurrence_id_tz LIKE '/mozilla.org%' UNION " +
>                         "SELECT event_start_tz   AS zone FROM cal_events     WHERE event_start_tz   LIKE '/mozilla.org%' UNION " +
>                         "SELECT event_end_tz     AS zone FROM cal_events     WHERE event_end_tz     LIKE '/mozilla.org%' UNION " +
>@@ -1215,29 +1221,29 @@ calStorageCalendar.prototype = {
>                             "UPDATE cal_todos      SET recurrence_id_tz = '" + update.newTzId + "' WHERE recurrence_id_tz = '" + update.oldTzId + "'; " +
>                             "UPDATE cal_todos      SET todo_entry_tz    = '" + update.newTzId + "' WHERE todo_entry_tz    = '" + update.oldTzId + "'; " +
>                             "UPDATE cal_todos      SET todo_due_tz      = '" + update.newTzId + "' WHERE todo_due_tz      = '" + update.oldTzId + "'; " +
>                             "UPDATE cal_todos      SET alarm_time_tz    = '" + update.newTzId + "' WHERE recurrence_id_tz = '" + update.oldTzId + "';");

I have one concern, but I don't think it is that bad as long as we communicate this very clearly.  During my testing, I worked with a 0.7 build converting it to a 0.8 build, which is what most people will likely be doing.  That worked ok.  However, when I first began testing, I used an old 0.8 nightly and attempted to use that.  In the old 0.8 nightly, this code above failed to upgrade any timezones because the TZID was not stored in the database, instead the full timezone definition was being stored for "mozilla" zones in the old 0.8 build.  This is incorrect per the implementation as far as I can tell, so I'm not sure how it got into that state.  Working with a current 0.8 nightly, only the TZID is stored in the database for mozilla zones, which is proper.  

So, the point here is that for some users on some builds of our nightlies, the timezone upgrade will fail.  It will cause the application to throw and it will no longer startup properly.  I think users running old releases will be ok.  What concerns me is that we do have a fair contingent of users running nightlies.

I'm going to wait a day or two to check this in to see if anyone has comment on it.  I'll also post to the blog about it to warn folks.

r=ctalbert
Attachment #302622 - Flags: review?(ctalbert) → review+
> So, the point here is that for some users on some builds of our nightlies, the
> timezone upgrade will fail.  It will cause the application to throw and it will
> no longer startup properly.  I think users running old releases will be ok. 
> What concerns me is that we do have a fair contingent of users running
> nightlies.

Re: nightlies issue, is that worthy enough to go into the release notes for 0.8?
(In reply to comment #26)
> 
> Re: nightlies issue, is that worthy enough to go into the release notes for
> 0.8?
> 
No, I don't think so because the release notes are for the release, not the nightlies.

Ok.  I have checked in this monster.  Thanks to everyone that stepped up to help out with patches: nth10sd, dbo, and Hb.  

While attempting the checkin I realized that back in the 0.3.1 days the new ICS files only got checked in on TRUNK, so these diff's only apply to those trunk files.  I checked in the new ICS files only on trunk, and cross-committed the rest of the code.  

I think that since we only check these in for a loose reference anyway, this is ok.  It'd be difficult and pointless to update those files on BRANCH too, so we should probably just make a follow on bug to CVS REMOVE them from branch and continue with this tradition of updating them on trunk only.

With great pleasure, marking this bug as FIXED.  

 

Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Follow on bug mentioned is Bug 418336
> > Re: nightlies issue, is that worthy enough to go into the release notes for
> > 0.8?
> > 
> No, I don't think so because the release notes are for the release, not the
> nightlies.

I think this should be relnoted. We've been referring to nigtly builds on the forums, the weblog and the ng's for much desired features a lot. So probably a lot of users are running 0.8 nightlies already and may run into trouble.
Target Milestone: --- → 0.8
The timezone dialog in the preference pane and the event dialog are not updated, therefore they will not work anymore:
http://lxr.mozilla.org/seamonkey/search?string=20070129_
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Fix v1 (obsolete) — Splinter Review
Fix for broken timezone and event dialogs.
Attachment #304235 - Flags: review?(ctalbert)
Attached patch Fix v2Splinter Review
Apologies, forgot to remove menu item for Jan Mayen now that it's no longer in the 2007k Olson database.
Attachment #304235 - Attachment is obsolete: true
Attachment #304241 - Flags: review?
Attachment #304235 - Flags: review?(ctalbert)
Attachment #304241 - Flags: review? → review?(ctalbert)
Comment on attachment 304241 [details] [diff] [review]
Fix v2

Taking over review since I have a moment of time :)

Changes look good, r=philipp
Attachment #304241 - Flags: review?(ctalbert) → review+
Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Thanks Philipp!  Good catch Stefan!
Keywords: relnote
Status: RESOLVED → VERIFIED
Depends on: 429521
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.