Update internal timezone definitions

VERIFIED FIXED in 0.8

Status

Calendar
Internal Components
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Stefan Sitter, Assigned: gkw)

Tracking

unspecified
Dependency tree / graph
Bug Flags:
blocking-calendar0.8 +

Details

Attachments

(7 attachments, 6 obsolete attachments)

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
(Reporter)

Description

10 years ago
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?

Comment 1

10 years ago
See also http://groups.google.com/group/mozilla.support.calendar/msg/43983804689c1b58
Flags: wanted-calendar0.8? → blocking-calendar0.8+
(Assignee)

Comment 2

10 years ago
Created attachment 296569 [details] [diff] [review]
Change Caracas' timezone from -0400 to -0430

My first patch for Calendar!

Changed from -0400 to -0430. I have no idea what else to change.
Attachment #296569 - Flags: review?(ctalbert)
(Assignee)

Comment 3

10 years ago
Created attachment 296590 [details] [diff] [review]
better, more complete patch 

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)

Comment 4

10 years ago
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.
(Assignee)

Comment 5

10 years ago
Created attachment 297384 [details] [diff] [review]
version 2 incorporating Jan Mayen changes

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 6

10 years ago
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.
(Assignee)

Comment 7

10 years ago
Created attachment 299301 [details] [diff] [review]
patch version 3

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)

Comment 8

10 years ago
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).

Comment 9

10 years ago
Created attachment 299482 [details] [diff] [review]
storage TZID update piece

More testing welcome!
Attachment #299482 - Flags: review?(ctalbert)
(Assignee)

Comment 10

10 years ago
Created attachment 299822 [details] [diff] [review]
patch v3.1

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 11

10 years ago
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 12

10 years ago
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+

Comment 13

10 years ago
Created attachment 302089 [details] [diff] [review]
New tzdata.c file with proper specification on Jerusalem

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)

Comment 14

10 years ago
Created attachment 302090 [details] [diff] [review]
Updated ICS file for Jerusalem
Attachment #302090 - Flags: review?(daniel.boelzle)

Comment 15

10 years ago
Created attachment 302094 [details]
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+
Created attachment 302622 [details] [diff] [review]
storage TZID update piece
Attachment #299482 - Attachment is obsolete: true
Attachment #302622 - Flags: review?(ctalbert)

Comment 19

10 years ago
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.

Comment 20

10 years ago
(In reply to comment #19)

Filled the separate bug 417020 for this.

(Assignee)

Updated

10 years ago
Blocks: 417541

Comment 21

10 years ago
Created attachment 303732 [details] [diff] [review]
Patch v5 for tzdata.c with Australian DST exceptions

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?

Updated

10 years ago
Attachment #303732 - Flags: review? → review?(ctalbert.moz)

Updated

10 years ago
Duplicate of this bug: 417808

Comment 23

10 years ago
(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 24

10 years ago
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 25

10 years ago
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+
(Assignee)

Comment 26

10 years ago
> 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?

Comment 27

10 years ago
(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
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 28

10 years ago
Follow on bug mentioned is Bug 418336

Comment 29

10 years ago
> > 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
(Reporter)

Comment 30

10 years ago
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 → ---
(Assignee)

Comment 31

10 years ago
Created attachment 304235 [details] [diff] [review]
Fix v1

Fix for broken timezone and event dialogs.
Attachment #304235 - Flags: review?(ctalbert)
(Assignee)

Comment 32

10 years ago
Created attachment 304241 [details] [diff] [review]
Fix v2

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)
(Assignee)

Updated

10 years ago
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
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Comment 35

10 years ago
Thanks Philipp!  Good catch Stefan!

Updated

10 years ago
Keywords: relnote

Updated

10 years ago
Duplicate of this bug: 419793
(Assignee)

Updated

10 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Updated

10 years ago
Duplicate of this bug: 423304
(Reporter)

Updated

9 years ago
Depends on: 429521

Updated

9 years ago
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.