Last Comment Bug 314339 - need to handle non-native timezone bits
: need to handle non-native timezone bits
Status: VERIFIED FIXED
: dataloss
Product: Calendar
Classification: Client Software
Component: Internal Components (show other bugs)
: unspecified
: All All
: -- normal with 13 votes (vote)
: 0.8
Assigned To: cmtalbert
:
Mentors:
: 336640 355449 356905 357704 361046 362495 365812 366567 376011 378019 387658 387660 395885 396540 407336 413925 (view as bug list)
Depends on: 400950 402518
Blocks: 321653 363191
  Show dependency treegraph
 
Reported: 2005-10-29 11:17 PDT by Dan Mosedale (:dmose)
Modified: 2008-02-15 05:26 PST (History)
42 users (show)
bugzilla: blocking‑calendar0.8+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
This shows the event before and after adding to lightning from the iTIP based STR (2.52 KB, text/plain)
2007-01-26 09:11 PST, cmtalbert
no flags Details
Showing the odd ics where agenda lists event as 3PM, views as 9AM (1.77 KB, text/plain)
2007-01-26 09:12 PST, cmtalbert
no flags Details
A patch showing the outline of the database and the approach (22.77 KB, patch)
2007-03-26 21:26 PDT, cmtalbert
no flags Details | Diff | Splinter Review
Work in progress patch (30.47 KB, patch)
2007-03-28 11:29 PDT, cmtalbert
no flags Details | Diff | Splinter Review
Bare Bones Patch (34.77 KB, patch)
2007-04-01 22:14 PDT, cmtalbert
no flags Details | Diff | Splinter Review
changes based on applied "Bare Bones patch" 260317 (16.47 KB, patch)
2007-04-06 16:42 PDT, Daniel Boelzle [:dbo]
no flags Details | Diff | Splinter Review
First pass at incorporating a timezone resolver interface into the existing code (16.02 KB, patch)
2007-08-19 17:47 PDT, cmtalbert
no flags Details | Diff | Splinter Review
Work in progress patch (93.34 KB, patch)
2007-09-13 03:56 PDT, cmtalbert
no flags Details | Diff | Splinter Review
A database file with timezones pre-loaded for testing (260.00 KB, application/octet-stream)
2007-09-13 04:14 PDT, cmtalbert
no flags Details
Event on Feb 6th is showed one hour too late (59.62 KB, text/plain)
2008-02-04 05:07 PST, Thomas E. Deutsch
no flags Details

Description Dan Mosedale (:dmose) 2005-10-29 11:17:42 PDT
We need to make sure that TZIDs and VTIMEZONEs that do not originate from our internal database are properly preserved in all calendars.
Comment 1 Dan Mosedale (:dmose) 2005-12-13 13:05:13 PST
Taking, as wrapping one's head around the flow of a timezone through the code takes a while, and I'm mostly already there.
Comment 2 Dan Mosedale (:dmose) 2006-01-27 13:06:08 PST
0.1 is now targetted at not having any dataloss bugs on self-generated data, so this can wait.
Comment 3 Sebastian Schwieger 2006-09-08 01:28:18 PDT
*** Bug 336640 has been marked as a duplicate of this bug. ***
Comment 4 Sebastian Schwieger 2006-09-08 01:36:01 PDT
Copying the description of bug 336640:

>When importing an event with non-mozilla timezone, the storage provider doesn't
>store the timezone data. On next startup, the timezone is forgotten.
Comment 5 Dan Mosedale (:dmose) 2006-09-13 11:22:45 PDT
I think we need to at least understand our failure modes better here in order to decide whether we need any patches for 0.3, so I'm marking this as blocking.  In particular, it would be interesting to know what the ICS looks like, before and after, when we import an event with a non-local timezone to a storage calendar, restart the app, and then export that event.  Note that the restart is an important piece of the test because of the way the timezone service code works. 
Comment 6 Stefan Sitter 2006-09-15 13:54:03 PDT
(In reply to comment #5)

Original ics content (shortened to one event) from [http://ical.mac.com/corgimom28/Lost.ics]:

--------------------------------------------------------------------------
BEGIN:VCALENDAR
VERSION:2.0
X-WR-CALNAME:Lost
PRODID:-//Apple Computer\, Inc//iCal 2.0//EN
X-WR-RELCALID:16413259-E4F6-44F8-B79E-6B1BE8BD3764
X-WR-TIMEZONE:US/Eastern
CALSCALE:GREGORIAN
METHOD:PUBLISH
BEGIN:VTIMEZONE
TZID:US/Eastern
LAST-MODIFIED:20060914T030741Z
BEGIN:DAYLIGHT
DTSTART:20060402T070000
TZOFFSETTO:-0400
TZOFFSETFROM:+0000
TZNAME:EDT
END:DAYLIGHT
BEGIN:STANDARD
DTSTART:20061029T020000
TZOFFSETTO:-0500
TZOFFSETFROM:-0400
TZNAME:EST
END:STANDARD
BEGIN:DAYLIGHT
DTSTART:20070311T010000
TZOFFSETTO:-0400
TZOFFSETFROM:-0500
TZNAME:EDT
END:DAYLIGHT
END:VTIMEZONE
BEGIN:VEVENT
DURATION:PT1H
LOCATION:ABC
DTSTAMP:20060914T023435Z
UID:7DC6E752-9591-4485-9375-A8C95D3CDA5F-8316DFB9-C72E-4A56-AD21-37A21B7
 CB6E6
SEQUENCE:7
DTSTART;TZID=US/Eastern:20061011T210000
SUMMARY:Lost (3.02): \"Further Instructions\"
DESCRIPTION:Clair-centric episode
END:VEVENT
END:VCALENDAR
--------------------------------------------------------------------------

After import event is displayed from 03:00 to 04:00 on 12-Oct-2006 in my TZ (UTC+2). Ok. After restart the event is displayed from 01:00 to 02:00.
After export ics content looks like:

--------------------------------------------------------------------------
BEGIN:VCALENDAR
VERSION:2.0
PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN
BEGIN:VEVENT
CREATED:20060915T203630Z
LAST-MODIFIED:20060915T203630Z
DTSTAMP:20060915T203630Z
UID:
 7DC6E752-9591-4485-9375-A8C95D3CDA5F-8316DFB9-C72E-4A56-AD21-37A21B7\rCB6E
 6
SUMMARY:Lost (3.02): \"Further Instructions\"
DTSTART:20061012T010000
DTEND:20061012T020000
SEQUENCE:7
LOCATION:ABC
DESCRIPTION:Clair-centric episode
DURATION:PT1H
END:VEVENT
END:VCALENDAR
--------------------------------------------------------------------------

Notable changes: time changed, tz changed to floating, '\r' in UID, event has DTEND and DURATION.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060915 Calendar/0.3a2+
Comment 7 Dan Mosedale (:dmose) 2006-09-18 11:32:20 PDT
This will probably need a schema change, so the landing of this bug should be coordinated with that of bug 333688.
Comment 8 Matthew (lilmatt) Willis 2006-09-23 22:58:42 PDT
After talking with dmose about this, and thinking more about it, my thought is that we should add a column to both the cal_events table and cal_todos table, and serialize _ALL_ the VTIMEZONEs of a VEVENT or VTODO into it. 
Comment 9 Dan Mosedale (:dmose) 2006-09-26 17:25:30 PDT
After spending a number of hours investigating various implementation strategies, all of them seem non-trivial enough that we wouldn't want to take them at this late date.  We should definitely relnote this; the failure mode is that 
1) non-UTC items that come from any calendar other than current Lightning or Sunbird and end up in a local storage calendar (the default) will lose their timezone after a restart and appear to be floating.  This could happen via import or (in Lightning) iMIP, and possibly other venues (event paste in Sunbird perhaps?). 
Comment 10 Stefan Sitter 2006-10-17 02:00:25 PDT
*** Bug 356905 has been marked as a duplicate of this bug. ***
Comment 11 Stefan Sitter 2006-12-01 10:04:45 PST
*** Bug 362495 has been marked as a duplicate of this bug. ***
Comment 12 Stefan Sitter 2007-01-10 09:18:14 PST
*** Bug 366567 has been marked as a duplicate of this bug. ***
Comment 13 Matthew (lilmatt) Willis 2007-01-11 05:35:22 PST
Bumping the priority on this.

This is on my "must fix" hit list for 0.5.
Comment 14 cmtalbert 2007-01-25 09:48:38 PST
*** Bug 361046 has been marked as a duplicate of this bug. ***
Comment 15 cmtalbert 2007-01-26 09:10:04 PST
I think that you miss something in Comment 9.  After restart, the event is converted to UTC and is branded as floating time. This can have very unintended consequences. 
Here are the steps to reproduce such a scenario.
1. Create an event in America/Los_Angeles at 7AM using Apple iCAL.
2. Invite yourself to the event and mail yourself an invitation
3. Pick up the invitation using your Thunderbird + Lightning which is configured for the America/Chicago timezone.
4. Click the "Add to Calendar" button to add the event to the calendar --> Event shows up at 9AM in both Agenda and Calendar views.
5. Restart Thunderbird. View event again: Event is now at 15:00! (0700 + 8= 15:00 UTC).

However, I have seen an instance of 0.4 (on a profile with mixed America/Central time events and the America/Los_Angeles event in question) where the America/Los_Angeles event *was* displayed properly.  However, in the Agenda tab of Lightning the event was displayed as 3PM. I cannot reproduce this behavior with a clean profile, though.
Comment 16 cmtalbert 2007-01-26 09:11:26 PST
Created attachment 252922 [details]
This shows the event before and after adding to lightning from the iTIP based STR
Comment 17 cmtalbert 2007-01-26 09:12:52 PST
Created attachment 252923 [details]
Showing the odd ics where agenda lists event as 3PM, views as 9AM

There were other America/Chicago zone events in this ICS, but they were removed for readability.
Comment 18 Bas van den Bosch 2007-03-05 13:23:55 PST
see also bug 365812 and bug 357704.
Comment 19 Matthew (lilmatt) Willis 2007-03-06 06:17:39 PST
Reassigning to ctalbert
Comment 20 Bas van den Bosch 2007-03-12 12:50:15 PDT
Perhaps the simplest, and the most correct imho, solution would be to add every foreign timezone which is stumbled upon in an external calendar and in imported calendars to the local database of timezone's. This means for every (re-)load of a calendar there is at least one extra check with the timezone's database but saves you the trouble of adding a column to both the cal_events table and cal_todos table like lilmatt states in comment 8. Advantage of this is that the system can easily be extended to events having different timezones for startdate and enddate (for travellers, though I suspect it won't be used much...). To be sure sunbird can be upgraded correctly, you should have two databases. If the timezone doesn't start with mozilla/ it will be foreign and should be looked up, if it starts with mozilla/ we can safely assume it's a valid mozilla defined timezone. When setting timezones in the options-dialog one should have to be able to choose from both lists so ensure interoperability with foreign providers. Hope this is not too late for 0.5...
Comment 21 Stefan Sitter 2007-03-14 15:07:30 PDT
Removing qawanted keyword. Using Sunbird 0.5pre (2007031403) the failure mode is still the same as during last qawanted cycle. See Comment #6 for details.
Comment 22 cmtalbert 2007-03-26 21:26:25 PDT
Created attachment 259763 [details] [diff] [review]
A patch showing the outline of the database and the approach

This patch shows the general approach I am thinking of taking.  I am of course hoping for and accepting ideas and feedback on this.  I have looked at several storage implementations sketched out several and then finally created this one.

Here are some high points of the design:

1. There is only one table.  This seems stupid at first blush.  But, it enables us to cache the statement objects in memory (as member variables on the service, which is done in several other places in the codebase).  It also allows us a much easier system than having two identically constructed tables (one for mozilla zones and one for foreign zones). Using the ismozzone column (an indexed integer that will be either 1 (is a mozila zone) or 0 (not a mozilla zone) we can slice this table either way we want it.  That will make upgrading very easy by the use of a compound statemnt: insert into newtable where (select * from moz_timezones where (ismozzone=1) AND (refcount<>0))

2. I've pulled out many high level attributes of the timezone into the table.  THe entire timezone ICAL definition (including the VCALENDAR part) will live in the "icaldata" column.  The reason so many of the columns like latitude, longitude, location (i.e. X-LIC-LOCATION) were pulled into specific columns is because they may prove useful for searching in future versions.  

3. Both the stdoffset and dayoffset refer to the "current" standard and daylight UTC offset for the zone.  Those might be useful for searching. I could see that the standard offset will be very useful for searching with our graphical timezone picker.  I'm still debating whether or not a specific daylight column would prove useful or not.  

4. An integer primary key...SQLITE takes care of us here. There is an autoincrement ROWID column that is always created in a SQLITE table.  Therefore, I saw no need to include an artificial one.

5. Tzid is NOT a "PRIMARY KEY" because this generates an unnamed index that cannot be changed or dropped in the future.  Instead, I created a UNIQUE INDEX on it, which still enforces uniqueness, but allows us the flexibility to change that later via simple ALTER TABLE statements should we decide to.

6. While I did put the extra columns in the table, I decided to remove the statement handlers for searching on them. I thought that it would be best to only have the searching handlers for the queries we will have, and later we can add the others if we choose to.

7. The interface is essentially the same as the "timezone" interfaces on calIICSService.  This is intentional.  Once this is ready, the timezone interfaces on calIICSService will be changed to be a wrapper for the calITimezoneStorageService interface.  This is necessary so that code above calIICSService will not have to change.

8. The chicken and the egg.  In the future I imagine that we will ship either a zipped up file of timezones or the actual sqlite timezone database file itself.  However, in the short term, neither of those things are happening, and we will still have the built-in timezone ID's (tzdata.c), so this code will craft a timezone database out of the information contained in tzdata.c to seed itself with the current suite of mozilla timezones.  If we'd rather just ship the database file now, I can change it to do that, but having it seed itself now, and then deal with the upgrades later seemed the easier option, given the schedule.
Comment 23 Bas van den Bosch 2007-03-26 22:36:55 PDT
Just some remarks which come to mind, not knowing the inner workings of calendar :

1) what happens when a timezone is updated, do you want to keep the old definition  in the database? On updating a timezone all event-data will have to be updated too so no need for keeping the old one there but later on there's always a chance events with old data get in the calendar. How is this handled now? Is old data always updated, so when stumbling on an old definition is the event rewritten with the new definition? Always keeping old data means the table could in theory grow very large but otoh there won't be hundreds of changes in the next decades presumably. It's better to have one table if possible, I agree. Also, what's refcount used for, the number of events which use this tz?

2. Agreed, it's a good thing to have the entire definition in the table. Some timezones are really complicated (outlook isn't able to handle these) so the has to be quite some data in it.

3. having the current offset's in the database will save parsing of timezone-data but I think it might be difficult to keep this data correct. If one leaves SB open for a couple of days, this data will have to be updated. If you put current DST in the database this is gets much better, the only time this data will have to be updated is on dates when DST-definitions- change (which happens about twice a year world-wide). I think a restart of SB twice a year (assuming update on restart) is no problem, you could also add a timer which refreshes these fields once every 24 hours for example. It you ommit the DST-definition, these fields will have to be updated every couple of minutes as the change of current time by DST-changes happens twice a year in every zone. 

4. agreed

5. I think it's a bad idea to have doublures in the table, when altering or adding timezone's there should always be a check for unique timezone-definitions. You're not going to put the integer in the VTIMEZONE-data so looking data up will be on TZID, not on ID.

8. I guess you won't whip tzdata.c as foreign timezones are added to the database on the users side. If you ship tzdata.c these changes will be overwritten which might lead to errors. Seeding the database sounds correct (see also my comments in 1)

6/7. beyond my understandig so no comments :-)
Comment 24 cmtalbert 2007-03-27 04:18:53 PDT
(In reply to comment #23)
> 1) what happens when a timezone is updated, do you want to keep the old
> definition  in the database? On updating a timezone all event-data will have to
> be updated too so no need for keeping the old one there but later on there's
> always a chance events with old data get in the calendar. How is this handled
> now? Is old data always updated, so when stumbling on an old definition is the
> event rewritten with the new definition? Always keeping old data means the
> table could in theory grow very large but otoh there won't be hundreds of
> changes in the next decades presumably. It's better to have one table if
> possible, I agree. Also, what's refcount used for, the number of events which
> use this tz?
> 
When a timezone is updated, the old information should stay inside the full (iCal data) timezone definition, so the old timezone can be removed or updated in place on the database table. Currently, the old timezone information is being dropped due to the fact we want to maintain Outlook interoperability. 

With the database, here is how updates work:
If it is a mozilla timezone, and the TZID *does not* change then the columns are updated with the new information.
If it is a mozilla timezone and the tzid *does* change then the nextTzid field is updated to point to the new tzid. As events reference the old tzid, they are updated to point to the new tzid and the refcount is decremented.  Once the refcount hits zero on the old timezone, it is removed from the database.
At this time, we cannot detect updates on non-mozilla timezones. (that will be handled later).


Refcount is used to help manage the lifetime of the foreign zones. But, note that it can't go totally to zero in some cases (remote calendars) until we have caching in place. I'm not sure how big of a problem that will be.  We may also want to have a database datestamp of "last accessed" time, and slowly decay the refcount if the last accessed time is older than some x number of time.  Or we could nuke foreign zones that have not been accessed in x amount of time when we upgrade the mozilla zones.  This may be mitigated by having a true database server and a dependable agreed upon set of timezones. Then we can merge foreign zones into our standard list rather than simply adding them.

> 3. having the current offset's in the database will save parsing of
> timezone-data but I think it might be difficult to keep this data correct. If
> one leaves SB open for a couple of days, this data will have to be updated. If
> you put current DST in the database this is gets much better, the only time
> this data will have to be updated is on dates when DST-definitions- change
> (which happens about twice a year world-wide). I think a restart of SB twice a
> year (assuming update on restart) is no problem, you could also add a timer
> which refreshes these fields once every 24 hours for example. It you ommit the
> DST-definition, these fields will have to be updated every couple of minutes as
> the change of current time by DST-changes happens twice a year in every zone. 
Maybe I wasn't clear here. The "current standard and daylight offset" means the most recent defined UTC offset for the zone, not the zone's current offset based on today's date.  These only need to be updated when the timezone definition changes.  For example, event though America/Chicago is currently in DST, it's defined standard offset is still -0600 and its defined Daylight offset is still -0500.  These will not change unless the United States decides to change it to say -0615 and -0515 or some such.

> 5. I think it's a bad idea to have doublures in the table, when altering or
> adding timezone's there should always be a check for unique
> timezone-definitions. You're not going to put the integer in the VTIMEZONE-data
> so looking data up will be on TZID, not on ID.
Agreed, hence the reason I didn't want to include an extra integer ID in the table.

> 
> 8. I guess you won't whip tzdata.c as foreign timezones are added to the
> database on the users side. If you ship tzdata.c these changes will be
> overwritten which might lead to errors. Seeding the database sounds correct
> (see also my comments in 1)
> 
Right. When we generate a new set of Mozilla timezones, the mozilla zones will be updated as I specified above.  The foreign timezones that have a non-zero refcount will be moved forward to the new database using an "insert into new select * from old where ismozzone=0 and refcount <>0" style query.
  

Comment 25 cmtalbert 2007-03-27 08:00:43 PDT
Forget everything I ever said about seeding the database using tzdata.c.  I'm going to do lazy adds like calIICSService currently does.  Otherwise, it's a huge performance sink.  Since we don't have the timezone picker (which is about the only reason to have a fully seeded Db on startup that I can think of), I think this will be ok. 
Comment 26 Joey Minta 2007-03-27 08:04:22 PDT
(In reply to comment #25)
Since we don't have the timezone picker (which is about
> the only reason to have a fully seeded Db on startup that I can think of), I
> think this will be ok. 
> 
The default tz-guessing code iterates through all the available timezones on startup already (note that it may return early in some cases).  If you could combine this loop, it might give you the same win without as much cost.
Comment 27 cmtalbert 2007-03-28 06:02:46 PDT
*** Bug 357704 has been marked as a duplicate of this bug. ***
Comment 28 cmtalbert 2007-03-28 06:32:32 PDT
*** Bug 365812 has been marked as a duplicate of this bug. ***
Comment 29 cmtalbert 2007-03-28 11:29:45 PDT
Created attachment 259923 [details] [diff] [review]
Work in progress patch

This patch removes some of the "calITimezone" interfaces off the calITimeonzeStorageService and it shows the code for adding a timezone to the database.  It also contains the code for handling the timezone refcounting.

I spent quite a bit of time tinkering with libical and its timezone handling in order to properly parse out the offsets for the database table. Since the first pass of this code will only be used for foreign timezones and since we are not going to wire foreign timezones into our timezone picker, I am probably not going to put the foreign timezone offsets into the "offset" database columns.  

From what I see, I think that I would need to add a function(s) to libical in order to properly retrieve the current offset values for standard and daylight time, so I think that should be done as part of the post 0.5 work.
Comment 30 Jids 2007-03-28 14:56:03 PDT
I just installed Sunbird v0.3.1 over v0.3, and it was fine the first time it started up. The second time it started, all my events were offset by one hour (8:00 --> 9:00). Is there any way I can reset this?
Comment 31 cmtalbert 2007-04-01 22:14:01 PDT
Created attachment 260317 [details] [diff] [review]
Bare Bones Patch

The Ref-Counting of foreign timezones would not work without the addition of the event UID to the database and therefore the get/add timezone interfaces.  I'm not yet totally convinced this is the only way to do this, or the best way to do this, so I took out all the ref-counting parts of this patch.

So, what we have here is just a patch that stores foreign timezone definitions into a database and gets them out.

However, in doing this, I encountered a nasty little memory gotcha.  The TimezoneEntry hash in calICSService keeps raw calIIcalComponents around, and therefore when one of these is obtained, it *already* has a refcount of 1.  That is then ADDREF'd by calICSService::GetTimezone to a value of 2 and returned to callers.

Since the foreign timezone database stores its timezone components in a serialized format, the timezone components it returns are only ADDREF'd to a value of 1 when they are returned.  The illustrated an issue in calIcalComponent::GetDateTimeAttribute.  Here, the tzcal member (http://lxr.mozilla.org/mozilla1.8/source/calendar/base/src/calICSService.cpp#712) is declared inside an if statement scope.  At the end of this scope (assuming the timezone definition is found by GetTimezone) you can see:

// correct itt which would else appear floating:
itt.zone = icalcomponent_get_timezone(tzcal->GetIcalComponent(), tzid);

This sets itt.zone to the exact pointer value of internal timezone component inside of the tzcal object without ADDREF'ing the tzcal object.

Next, we leave the scope (dereferencing tzcal) and use the itt.zone pointer when we create the new calDateTime object.

In the case of timezone components retrieved from the built-in hash, the tzcal pointer value was not freed due to the fact that the refcount on tzcal did not go to zero.  However, when using a non-native zone from the database, the zone pointer would be freed, causing a crash in the calDateTime constructor.  My temporary solution (pending further research) was to move the tzcal variable outside of the if statement scope, so that we avoid the RELEASE in question, and so that the zone pointer value does not refer to a variable that has gone out of scope.  However, I'm worried this will make the leak of tzcal worse than it was previously in the case where we get the timezone from the internal calICSService timezone hash.
Comment 32 Daniel Boelzle [:dbo] 2007-04-06 16:39:59 PDT
(In reply to comment #31)
> The Ref-Counting of foreign timezones would not work without the addition of
> the event UID to the database and therefore the get/add timezone interfaces. 
> I'm not yet totally convinced this is the only way to do this, or the best way
> to do this, so I took out all the ref-counting parts of this patch.

I am not really convinced by this solution either. Maintaining this kind of ref-count is a very nasty and error-prone task.

> So, what we have here is just a patch that stores foreign timezone definitions
> into a database and gets them out.

> However, in doing this, I encountered a nasty little memory gotcha.  The
> TimezoneEntry hash in calICSService keeps raw calIIcalComponents around, and
> therefore when one of these is obtained, it *already* has a refcount of 1. 
> That is then ADDREF'd by calICSService::GetTimezone to a value of 2 and
> returned to callers.

Which is by design:
1) ics-service::addTimezone() has been designed that providers could pump in foreign timezones at startup, so they could let parse ical strings (only referring those foreign timezones by id) into events.
2) All calDatetime objects rely on the fact that they could get their ical_component_ timezone information by tzid. If you want to change that, calDatetime needs to refer to its timezone via hard nsCOMPtr<calIIcalComponent>, not via string (mTimezone). Side-Note: there is also an implication with calDatetime::GetIcalTZ(): it gets the calIIcalComponent from ics-service, retrieves the inner ical_component_ pointer being returned to the caller, and leaves its function scope(!).
THe latter 2) could be reworked of course, but overall this lead to the runtime caching as currently in the ics-service (timezone entries stuff).

Further side-note: A feasible alternative I could imagine is
- The ics-service only refers *weakly* to added (foreign) timezones
- A provider instance that has added its foreign timezones holds hard references to all those added ones, effectively keeping them live/addressable until it gets unloaded itself.
=> Thus events from that calendar keep a hard reference to their calendar instance, assuring that those events' datetime objects keep resolvable tzids.

Finally, please excuse my ignorance and enlighten my understanding, but IMO the core problem is still the storage provider which forgets its timezones, so why shouldn't it store them (+ref-counting its used ones)? Being complete, every other provider does so: ics stores them in the file, WCAP asks its server, ... So why not fix the storage provider code / do that code in the storage provider, but create a further database to be shared?
Comment 33 Daniel Boelzle [:dbo] 2007-04-06 16:42:34 PDT
Created attachment 260864 [details] [diff] [review]
changes based on applied "Bare Bones patch" 260317

Going on with the review (assuming that Clint will come up with good answeres pro central timezone database), I have played around with the code, so we don't lose time. Applying the patch, WCAP crashes, thus I reworked the code regarding comment #32 (the datetime problem). The patch is based on Clint's code to make my changes clearer.

further comments/changes (independent of comment #32):
- Holding the storage statements as members of the service object disables any concurrent use.
- Do the statements need to be reset after usage? I have seen this in storage provider.
- Makes sense to hold a reference to ics service, not calling do_GetService() everytime; I have added a late-init function for that.
- Although there is no code regarding the ref-counting, the schema still has it.
- minor style stuff
Comment 34 Michiel van Leeuwen (email: mvl+moz@) 2007-04-07 07:01:05 PDT
(In reply to comment #33)
> The patch is based on Clint's code to make
> my changes clearer.

Just a (kind of offtopic) note: If you both use cvs diff to create the patches, and have the files in the same order, you can make bugzilla do the magic of showing the differences between the two patches. That way it's easier to apply the patch.

On reason I can think of for the timezone database are items that are not in a calendar, like incoming imip items. I'm not sure if they always will have a timezone attached to them. (they should, but clients might be broken).
Also, how does the memory provider store the timezones?
Comment 35 Daniel Boelzle [:dbo] 2007-04-07 12:16:04 PDT
(In reply to comment #34)
> On reason I can think of for the timezone database are items that are not in a
> calendar, like incoming imip items. I'm not sure if they always will have a
> timezone attached to them. (they should, but clients might be broken).

Regarding the latter case case (where no timezone is attached to the imip message): Even with a timezone db, we can run into that situation and IMO need to try to match to one of our internal Olson ones, e.g. incoming Europe/Paris => /mozilla.org/2007.../Europe/Paris.

> Also, how does the memory provider store the timezones?
IMO it need not store its timezones, because the memory calendar data is not made persistent.
Comment 36 Michiel van Leeuwen (email: mvl+moz@) 2007-04-08 07:55:12 PDT
(In reply to comment #35)
> IMO it need not store its timezones, because the memory calendar data is not
> made persistent.

It for sure needs to remember the timezone details for the current session... (so the timezones would be 'stored' in memory)

As far as I can see now, it only stored the timezone id (actually, the memory provider only stores calIDateTime's, but those do carry a timezone id). This is then matched to the timezone db inside the ics service. But iirc, the ics service remembers timezones. So the memory provider, and thus the ics provider, also doesn't really need a seperate timezone db.
Comment 37 Bill Gianopoulos [:WG9s] 2007-04-08 08:58:33 PDT
Well, timezones are one fo the trickiest things in a calendaring application.  I have not found one yet that displays times in a useful manner.  storing the meeting times is the easy part becuase that should all be stored in UTC (GMT) period.

The tricky bit comes with what timezone to display.  If I am at home in Boston on a Monday but will be in California Wednesday through Friday, I would like to be able to see Wednesday through Friday in California time.  Otherwise I am unable to plan plane flights, lunch diner etc.  without doing time conversions which the programs should be able to do form me.

I suppose this could be accomplished by just temporarily altering the timezone when looking at that part of the calendar and changing it back, so dynamic changes of timezone need to be supported.  It would be nice if this could be done form the calendar view rather than having to go to a preference window.

I just point all this out here so that it can be taken into account when designing a solution.  This does not need to be solved in this bug, but I wanted to make sure whatever is implemented does not somehow preclude doing this in the future.
Comment 38 Mike 2007-04-08 21:04:02 PDT
Guys,

Help me out here...

It seems like all that is needed is store all calendar events in GMT and then adjust for the machine's local time zone, when displaying the events on the calendar.  But, I must be missing something...

For example, if I receive a meeting invite for meeting that takes place on April 09, 2007 at 11am PDT, but I am located in HST, wouldn't we just store the meeting as April 09, 2007 at 6pm GMT and then adjust for my local time zone (HST = GMT-10) when displaying the meeting on my calendar, which puts the meeting at 8am HST?  In this case, the meeting would be displayed correctly for me and also for the user in PDT, who saved the event to their local calendar.

I would also like to voice a small note of disagreement with Bill's previous comment about altering the timezones of specific dates to accommodate future travel.

My thought here is that the calendar become too confusing when you start assigning time zones to dates. In a case where you are looking at a month's worth of dates, how would you know what time zone each date was in?  Some kind of visual indicator would be needed to indicate which time zone applied each date.  However, I think that even if the calendar could visually indicate which time zone applied to which date, this would only be useful to small population of the user base.

The larger issue I think we are dealing with is to simply display meetings at the correct local time, per the user's local machine time zone, offset based on the time zone of the meeting requester.
Comment 39 Michiel van Leeuwen (email: mvl+moz@) 2007-04-09 01:14:38 PDT
I have described a few times before, so i'm not going to repeat all of it. But the core is: storing datetimes in GMT is wrong. very wrong. It will hopelessly fail for repeating events.
And to reply the other comment: Yes, we already convert the datetimes to the timezone of the client. Everything you raised, we already thought of.
Comment 40 Bill Gianopoulos [:WG9s] 2007-04-09 02:57:07 PDT
 (In reply to comment #38)
> Guys,
> 
> Help me out here...
> 
> It seems like all that is needed is store all calendar events in GMT and then
> adjust for the machine's local time zone, when displaying the events on the
> calendar.  But, I must be missing something...
> 
> For example, if I receive a meeting invite for meeting that takes place on
> April 09, 2007 at 11am PDT, but I am located in HST, wouldn't we just store the
> meeting as April 09, 2007 at 6pm GMT and then adjust for my local time zone
> (HST = GMT-10) when displaying the meeting on my calendar, which puts the
> meeting at 8am HST?  In this case, the meeting would be displayed correctly for
> me and also for the user in PDT, who saved the event to their local calendar.
> 
> I would also like to voice a small note of disagreement with Bill's previous
> comment about altering the timezones of specific dates to accommodate future
> travel.
> 
> My thought here is that the calendar become too confusing when you start
> assigning time zones to dates. In a case where you are looking at a month's
> worth of dates, how would you know what time zone each date was in?  Some kind
> of visual indicator would be needed to indicate which time zone applied each
> date.  However, I think that even if the calendar could visually indicate which
> time zone applied to which date, this would only be useful to small population
> of the user base.
> 
> The larger issue I think we are dealing with is to simply display meetings at
> the correct local time, per the user's local machine time zone, offset based on
> the time zone of the meeting requester.
> 
I kind of thought different timezones for different days was too complex, but i would like a feature to be able to temporarily change the display timezone for the entire calendar.  My thought was that it would  be better to NOT change the client timezone to do this and if the display timezone differed from the client timezone, have some kind of warning on the display page so that you would not forget it in this state and get confused later.  This would satisfy my wish for being able to look at future dates shedules in a different timezone without over complicating things.

Comment 41 Michiel van Leeuwen (email: mvl+moz@) 2007-04-09 03:08:49 PDT
PLEASE, PLEASE don't comment about front-end features here! This bug is only about not forgetting the timezone details. Nothing else, nothing more. All other comments makes this bug impossible to read, and thus impossible to fix.
(including this one, sorry for that. But the offtopic comments need to stop somewhere)
Comment 42 cmtalbert 2007-04-17 17:19:02 PDT
** Development Issues With Storage provider only fix **
I looked into the possibility of fixing this for the short term by simply patching the storage provider.  The fix would change the storage provider by adding a database table to it to persist the timezone information.  This would involve adding logic to the storage provider to call into calIICSService to get/set the timezone information in the and in the internal in-memory hash that calIICSService keeps.  Since we are modifying the database schema, we would also have to provide a mechanism to update the current database schema version to a new database schema version.  Much of this code would be duplicated once the true timezone database service lands, and so, it would only be a stop gap fix.

** QA Implications of the short term fix **
Because this would incur a database schema update, QA is compelled to then perform more extensive upgrade testing between 0.3.1 and 0.5 and between 0.3 and 0.5 to ensure that all necessary database pieces are properly upgraded and nothing was harmed or left out of the database schema upgrade.  Of course, QA will also be required to test the actual persistence mechanism of the timezone test and ensure that timezone calculations were not disturbed or broken by adding this feature.
Lastly, general regressions would need to be run against storage calendars to ensure that the new feature did not break provider functionality (which can be tested in parallel with database upgrade testing).

After considering this, I chatted in IRC with dmose and I came to the conclusion that we probably want to continue to relnote this bug as we have done in the past and fix it soon after the 0.5 release with a more fully baked timezone database service patch.

So, the net of this is that I think we want to remove the blocking status from this bug, and ensure that it gets relnoted for 0.5 and fixed soon afterward.

Removing request for review.
Comment 43 Simon Paquet [:sipaq] 2007-04-18 09:19:41 PDT
Does not block the release, as per the weekly conference call.
Comment 44 Mike 2007-04-18 22:29:54 PDT
Is there anything higher priority than this?  A calendaring application that can not pass appointments back and forth doesn't seem like it meets the basic requirements necessary to be called a calendaring application, that is - being able to share calendar information.

Without a way to store ics files in the correct time zone, the only use for this application is as a personal appointment calendar, which seems to contrary to the whole purpose of this application.

It seems to me that accurately representing time zones should be the highest priority. 
Comment 45 Sebastian Schwieger 2007-04-19 06:45:11 PDT
*** Bug 378019 has been marked as a duplicate of this bug. ***
Comment 46 Bas van den Bosch 2007-04-19 07:08:06 PDT
*** Bug 376011 has been marked as a duplicate of this bug. ***
Comment 47 cmtalbert 2007-05-09 22:31:01 PDT
*** Bug 355449 has been marked as a duplicate of this bug. ***
Comment 48 Daniel Boelzle [:dbo] 2007-06-25 01:25:52 PDT
Chatting with Clint about this bug and his excellent summary on <http://wiki.mozilla.org/Calendar:Hamburg_F2F_Meeting#Timezone_Discussion>, , we came to the following solution:

- We will establish a "reference" set of timezones and a timezone service. We may outsource that code into a separate extension, so we can update the reference set using mozilla's extension update mechanism. The latter step is secondary, but it would avoid the need for updating the whole app (think of 0.3.1) when only updating the timezones is needed.

- Timezones won't be tagged/prefixed; we will stick to the "Olson" names.

- When updating the reference timezone set, new definitions overwrite old ones (of the same TZID) while existing (Olson purged ones) stay in the set. Avoiding any upgrade problems, we possibly want to always ship the whole database set on updates to users.

- Data sources/sinks (i.e. the providers) are required to distinct between reference and foreign timezones. They are required to store/provide foreign timezone definitions; the provider is the only instance that can keep track of properly cleaning up unused foreign timezones.

- We stay with the (transient) calICSService::addTimezone API, so providers can publish definitions that way.

- We (may) offer a tz resolver interface that can be implemented by providers, so they need not care about what foreign tz definitions they have already published (nor publish all at startup).

- All "/mozilla.org/..." timezones should be matched to their non-prefixed form.

- Two timezones of the same TZID issued by different Outlook versions: We can't decide which one is newer, so need to store both side-by-side, under separate TZIDs.
Comment 49 David Pearce 2007-07-04 03:32:10 PDT
I have some problems with an .ics invitation that uses the following timezone info
BEGIN:VTIMEZONE
TZID:Kuala Lumpur\, Singapore
X-MICROSOFT-CDO-TZID:21
BEGIN:STANDARD
Which works if I open the event in lightning 0.5 and save it

BEGIN:VTIMEZONE
TZID:GMT +0800 (Standard) / GMT +0800 (Daylight)
BEGIN:STANDARD
does not even import into the calendar
my calendar is set to asia/kuala lumpur
What we have here are many different ways of expressing the same timezone
Comment 50 Stefan Sitter 2007-07-11 07:03:28 PDT
*** Bug 387658 has been marked as a duplicate of this bug. ***
Comment 51 Thomas E. Deutsch 2007-07-16 02:51:54 PDT
*** Bug 387660 has been marked as a duplicate of this bug. ***
Comment 52 Thomas E. Deutsch 2007-07-16 02:56:02 PDT
I've Problems importing .ics from Apple iCal:

BEGIN:VEVENT
DESCRIPTION:9 Uhr bis 12 Uhr
DTEND;TZID=Europe/Zurich:20070718T120000
DTSTAMP:20070716T080158Z
DTSTART;TZID=Europe/Zurich:20070718T090000
SEQUENCE:23
SUMMARY:Process Kick-off Meeting
UID:63067FC4-6332-40E8-9F47-4A1002EB0E88-5E015DD5-B261-46C1-BE31-3FE43F1A6
 7CC
END:VEVENT

This is a event, which is from 9am to 12am. In some sunbirds (Version 0.2), the
event will be displayed right, but in newer sunbirds/lightening, it will be
displayed from 11am to 2pm. PHPiCalendar (http://phpicalendar.net/) displays it correctly.

I've made a event at the same time using sunbird, which has been displayed at
the right time. I've noticed, that DTSTART and DTEND of the sunbird-event
contains this string, which iCal-events does not have: /mozilla.org/20070129_1/

If I do add this string manually to the iCal-Event, sunbird displays it
correctly.
Comment 53 Marcel Berteler 2007-07-31 04:02:06 PDT
One more that gives in-correct times after a restart:

TB 2.0.0.5pre 20070719
Calendar 0.7pre 2007072604

BEGIN:VCALENDAR
METHOD:REQUEST
PRODID:Microsoft CDO for Microsoft Exchange
VERSION:2.0
BEGIN:VTIMEZONE
TZID:(GMT+02.00) Harare/Pretoria
X-MICROSOFT-CDO-TZID:50
BEGIN:STANDARD
DTSTART:16010101T000000
TZOFFSETFROM:+0200
TZOFFSETTO:+0200
END:STANDARD
BEGIN:DAYLIGHT
DTSTART:16010101T000000
TZOFFSETFROM:+0200
TZOFFSETTO:+0200
END:DAYLIGHT
END:VTIMEZONE
BEGIN:VEVENT
DTSTAMP:20070730T091339Z
DTSTART;TZID="(GMT+02.00) Harare/Pretoria":20070801T090000
SUMMARY:FOSS software development (Open Source Competency centre) & Citizen
  Portal Discussion 
UID:040000008200E00074C5B7101A82E00800000000E05620AE9AD2C701000000000000000
 010000000980FC6DD307FC848B571DFEC5234B846
ATTENDEE;ROLE=REQ-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN="xxxx":MAILTO:x@xxx.xx
ATTENDEE;ROLE=REQ-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN="xxxx":MAILTO:x@xxx.xx
ORGANIZER;CN="xxx":MAILTO:x@xxx.xx
LOCATION:RSC-Clifton
DTEND;TZID="(GMT+02.00) Harare/Pretoria":20070801T100000
DESCRIPTION:\N
SEQUENCE:0
PRIORITY:5
CLASS:
CREATED:20070730T091428Z
LAST-MODIFIED:20070730T091428Z
STATUS:CONFIRMED
TRANSP:OPAQUE
X-MICROSOFT-CDO-BUSYSTATUS:BUSY
X-MICROSOFT-CDO-INSTTYPE:0
X-MICROSOFT-CDO-INTENDEDSTATUS:BUSY
X-MICROSOFT-CDO-ALLDAYEVENT:FALSE
X-MICROSOFT-CDO-IMPORTANCE:1
X-MICROSOFT-CDO-OWNERAPPTID:-63014953
X-MICROSOFT-CDO-APPT-SEQUENCE:0
X-MICROSOFT-CDO-ATTENDEE-CRITICAL-CHANGE:20070730T091339Z
X-MICROSOFT-CDO-OWNER-CRITICAL-CHANGE:20070730T091339Z
BEGIN:VALARM
ACTION:DISPLAY
DESCRIPTION:REMINDER
TRIGGER;RELATED=START:-PT00H15M00S
END:VALARM
END:VEVENT
END:VCALENDAR
Comment 54 Marcel Berteler 2007-07-31 04:11:34 PDT
Found one more strange thing. After accepting the above invitation, the correct times are shown in the calendar as well as the today pane.

Restarting results in a move of the event 2 hours back. Both in the Calendar as well as in the today pane.

After selecting the original email with the invitation (I have message pane switch on) without accepting or declining it, the event in the calendar suddenly changes to the correct time, while the event is still 2 hours too early in the today pane.

Restarting results in a re-set and the event is once again 2 hours too early in the today pane as well as the calendar.

Is viewing the invitation in any way effecting timezone settings?
Comment 55 Daniel Boelzle [:dbo] 2007-08-01 07:15:42 PDT
@Thomas, Marcel: Please stop flooding this bug! It makes it more and more unreadable! File new ones for your ics problems which may be DUPLICATE to this after investigation.
Comment 56 Daniel Boelzle [:dbo] 2007-08-01 07:21:47 PDT
Addition to comment #48: Reading the revised RFC 2445 TZID section (http://tools.ietf.org/rfcmarkup?doc=2445#section-4.2.19), I think we need to change our current naming scheme, i.e. remove the leading '/' (us-ascii 47). That one is reserved for unique ids of the future timezone registry.
Comment 57 Bill Gianopoulos [:WG9s] 2007-08-01 11:56:20 PDT
(In reply to comment #55)
> @Thomas, Marcel: Please stop flooding this bug! It makes it more and more
> unreadable! File new ones for your ics problems which may be DUPLICATE to this
> after investigation.
> 

Hold on a second here.  THis is the same issue raised in Bug 336640 that ws duped to this one over a year ago.  Now you claim that issue is unrelated to this bug?  SO no action whatsoever has been taken on this issue in over a year???

Hey don't yell at users because they are getting ticked off at the crappy level of support they are getting on this product.  It is obviously NOT the users fault.
Comment 58 Bill Gianopoulos [:WG9s] 2007-08-01 12:01:41 PDT
(In reply to comment #57)
> (In reply to comment #55)
> > @Thomas, Marcel: Please stop flooding this bug! It makes it more and more
> > unreadable! File new ones for your ics problems which may be DUPLICATE to this
> > after investigation.
> > 
> 
> Hold on a second here.  THis is the same issue raised in Bug 336640 that ws
> duped to this one over a year ago.  Now you claim that issue is unrelated to
> this bug?  SO no action whatsoever has been taken on this issue in over a
> year???
> 
> Hey don't yell at users because they are getting ticked off at the crappy level
> of support they are getting on this product.  It is obviously NOT the users
> fault.
> And if that isn't avclose enough match, bug 362495 also duped to this bug describes exactly the same situation as Thomas and Marcel are describing.  SO which is it.  Are they duplicates or not.  You can't have it both ways.

Comment 59 Thomas E. Deutsch 2007-08-02 00:32:04 PDT
(In reply to comment #55)
> @Thomas, Marcel: Please stop flooding this bug! It makes it more and more
> unreadable! File new ones for your ics problems which may be DUPLICATE to this
> after investigation.

I see, I see... It looks like helping users are not welcome here. I do understand. Wishing you guys good luck with this issue, but don't count on me.
Comment 60 Daniel Boelzle [:dbo] 2007-08-02 01:32:45 PDT
Guys, please understand that it's sufficiently known what needs to be done. Any further comments that may (or may not) describe the same problem are not helpful, but only make it harder to read this bug and have technical discussions. Please file new bugs for those problems. If it turns out to be a DUPLICATE, this is no problem. We could cross-verify those bugs after this (main) bug has been fixed.

(In reply to comment #58)
> > Hold on a second here.  THis is the same issue raised in Bug 336640 that ws
> > duped to this one over a year ago.  Now you claim that issue is unrelated to
> > this bug?  SO no action whatsoever has been taken on this issue in over a
> > year???
You misunderstand me: I never claimed that those bugs are unrelated. They obviously have been set to DUPLICATE, because they describe this same bug.

> > Hey don't yell at users because they are getting ticked off at the crappy level
> > of support they are getting on this product.  It is obviously NOT the users
> > fault.
Bill, you are free to help constructively; I'd appreciate if you could spend some time on fixing bugs.

(In reply to comment #59)
> I see, I see... It looks like helping users are not welcome here. I do
> understand. Wishing you guys good luck with this issue, but don't count on me.
I regret that; you would help us if you file a new bug for your problem.
Comment 61 Thomas E. Deutsch 2007-08-02 01:54:38 PDT
Daniel, thank you for this clarification. I didn't know that you guys are watching the "duplicate" bugs (I've created one in Bug 387660). Due to this, my target was to provide the same infos here. Sorry for that ;-)
Comment 62 Daniel Boelzle [:dbo] 2007-08-02 04:58:04 PDT
(In reply to comment #61)
Thomas, if you are not sure that this bug fixes your bug 387660, then leave it unverified (but DUPLICATE). That way it could be verified once this one is fixed.
Comment 63 cmtalbert 2007-08-19 17:47:25 PDT
Created attachment 277332 [details] [diff] [review]
First pass at incorporating a timezone resolver interface into the existing code

This is the first part of the patch.  This incorporates the idea of the timezone resolver interface into the existing calICSService code.  You can read more about the initial thoughts of the interface here: http://wiki.mozilla.org/Calendar:Feature_Implementations:Timezones#Timezone_Backend_Design_.28DRAFT.29_v3
Comment 64 Michiel van Leeuwen (email: mvl+moz@) 2007-09-10 12:56:42 PDT
Comment on attachment 277332 [details] [diff] [review]
First pass at incorporating a timezone resolver interface into the existing code

(I just took a quick look at the interface)

>+interface calITimezoneResolver : nsISupports
>+{
>+    calIIcalComponent addTimezone(in AUTF8String tzid,

From what I understand, there can be multiple implementations of this interface. But if an unknown timezone was found, to which implementation should it be added?
I think this interface should be split. There should be readonly resolvers for known sources and a separate interface for storing unknown timezones. Or would having only one implementation of the resolver be enough? Are multiple really needed? Which sources can there be?

>+    nsIUTF8StringEnumerator getTimezoneList();
>+    PRUint32 getCount();
I'd make those properties instead of methods.
Comment 65 cmtalbert 2007-09-10 14:05:48 PDT
(In reply to comment #64)
> (From update of attachment 277332 [details] [diff] [review])
> (I just took a quick look at the interface)
Thanks!
> 
> >+interface calITimezoneResolver : nsISupports
> >+{
> >+    calIIcalComponent addTimezone(in AUTF8String tzid,
> 
> From what I understand, there can be multiple implementations of this
> interface. But if an unknown timezone was found, to which implementation should
> it be added?
Yes, this is the core issue with the idea. This code is changing faster than can be kept up with here in the bug.  My current thinking is that the calITimezoneResolver interface will first only be implemented by the Olson Timezone Resolver.  And this will use a specific parameterized interface like: @mozilla.org/calendar/timezone-resolver;1?type=olson.  This will help when making a determination on where to add a timezone.

Secondly, in further thinking, Daniel and I decided that since all providers (except storage) store their own timezone definitions for foreign timezones, we don't really need the ability to "add" a foreign timezone into the resolver.  We can instead have the provider store the foreign timezone information, and make the calITimezoneResolver interface a "readonly" store for timezone information.  

By forcing the providers to store their own foreign timezones, we also remove the problem of foreign timezone refcounting.  Basically, this means that we don't have to worry about when to delete these timezones, and we can allow the provider to make that determination, which is easier since the provider has better knowledge of how the events are connected to the foreign timezone.

> I think this interface should be split. There should be readonly resolvers for
> known sources and a separate interface for storing unknown timezones. Or would
> having only one implementation of the resolver be enough? Are multiple really
> needed? Which sources can there be?
This is exactly the solution that Daniel and I have also come to.  The current patch now has one timezoneResolver, the Olson resolver, which is readonly.  The patch also fixes the storage provider so that it stores *only* foreign timezones.
> 
> >+    nsIUTF8StringEnumerator getTimezoneList();
> >+    PRUint32 getCount();
> I'd make those properties instead of methods.
> 
Done.

Thanks for your comments and ideas.  Keep them coming.


Comment 66 cmtalbert 2007-09-12 08:59:59 PDT
*** Bug 395885 has been marked as a duplicate of this bug. ***
Comment 67 Michiel van Leeuwen (email: mvl+moz@) 2007-09-12 12:20:33 PDT
(In reply to comment #65)
> By forcing the providers to store their own foreign timezones,

You also force items to always belong to a calendar. But is that always the case? I'm thinking about incoming items via itip.
Comment 68 Daniel Boelzle [:dbo] 2007-09-12 13:52:28 PDT
(In reply to comment #67)
We want to encounter that problem by reworking calDatetime which currently relies on its TZID being resolvable (via calICSService). The idea is that calDatetime keeps an nsCOMPtr on the timezone component (definition), thus there is no need for lookup via calICSService. That way providers as well as the iTIP code that parses the incoming VCALENDAR build up calendar items which hold calDatetimes (which hold their (foreign) timezone definitions).
However, I don't know whether this goal is reachable short term (w.r.t. regressions and time we've left), we could go with the current calIICSService::addTimezone API first, which you can use to pump in a foreign timezone definition being resolvable by TZID. That way, the iTIP code would pump in the foreign timezone definitions into calICSService on parsing.
Both would IMO solve the iTIP scenario, although the latter doesn't solve TZID clashes (meaning multiple definitions sharing the same TZID).
Comment 69 cmtalbert 2007-09-13 03:56:09 PDT
Created attachment 280722 [details] [diff] [review]
Work in progress patch 

This work in progress patch contains the C++ interface implementations for the calITimezoneResolver, the calITimezoneStorageService, the changes to the calICSService, the changes to the storage provider, and the python script that will take a Vzic database push it into a sqlite database for our extension.
Comment 70 cmtalbert 2007-09-13 04:14:29 PDT
Created attachment 280723 [details]
A database file with timezones pre-loaded for testing

This is a timezone database. You can drop this into your profile for timezone testing, but you need to manually go in and edit calCalendarManager (or some other good place) to register the calOlsonTimezoneResolver with the calICSService in order to get full functionality (without that, you will merely get storage provider lookups happening, which might also be interesting too.
Comment 71 Michiel van Leeuwen (email: mvl+moz@) 2007-09-15 11:51:28 PDT
Do you want to ship the pre-loaded database by default? If so, i think you should kill tzdata.c. Having the same data twice is a bit overkill...
Comment 72 Stefan Sitter 2007-09-18 05:23:22 PDT
*** Bug 396540 has been marked as a duplicate of this bug. ***
Comment 73 Simon Paquet [:sipaq] 2007-09-19 09:53:58 PDT
We've decided to not take this for 0.7.

We thought we were nearly there, but then some issues popped up, which are proving hard to debug and sorting those out will take some time. In addition even if we fix these issues, there are a lot of places in our codebase which have certain expectations regarding the way timezones are handled, so the risk of regressions is very high.

Therefore we do not feel confident enough to fix this in a short time. We will probably try to fix this early in the 0.9 timeframe, as this will leave us with enough time to test this thoroughly and to sort out all remaining issues.
Comment 74 Daniel Boelzle [:dbo] 2007-10-24 05:50:02 PDT
Comment #68 will be handled in separate bug 400950.
Comment 75 Omar Bajraszewski 2007-10-26 01:27:33 PDT
I think it's a very important problem and should be fixed as early as possible.
Comment 76 klint 2007-10-26 02:02:08 PDT
I agree with Omar, especially if Lightning interacts whith invitation come from Outlook/Exchange... at least in my company, where Exchange seems to be using non-native timezones.
Comment 77 Simon Paquet [:sipaq] 2007-10-26 02:12:50 PDT
This is definitely planned for 0.8
Comment 78 Jerry Yang 2007-10-26 10:07:44 PDT
If I accept any .ics file sourced from Outlook, the time will show 4 hours later than the correct time. But if the .ics file is sourced from Lightning, I don't have this problem. The two files are attached below.

The "bad" .ics file from Outlook:

BEGIN:VCALENDAR
METHOD:REQUEST
PRODID:Microsoft CDO for Microsoft Exchange
VERSION:2.0
BEGIN:VTIMEZONE
TZID:(GMT-05.00) Eastern Time (US & Canada)
X-MICROSOFT-CDO-TZID:10
BEGIN:STANDARD
DTSTART:16010101T020000
TZOFFSETFROM:-0400
TZOFFSETTO:-0500
RRULE:FREQ=YEARLY;WKST=MO;INTERVAL=1;BYMONTH=11;BYDAY=1SU
END:STANDARD
BEGIN:DAYLIGHT
DTSTART:16010101T020000
TZOFFSETFROM:-0500
TZOFFSETTO:-0400
RRULE:FREQ=YEARLY;WKST=MO;INTERVAL=1;BYMONTH=3;BYDAY=2SU
END:DAYLIGHT
END:VTIMEZONE
BEGIN:VEVENT
DTSTAMP:20071026T155751Z
DTSTART;TZID="(GMT-05.00) Eastern Time (US & Canada)":20071026T123000
SUMMARY:test
UID:{C1691EA2-C669-45AE-9822-D41B2052C104}
ATTENDEE;ROLE=REQ-PARTICIPANT;PARTSTAT=NEEDS-ACTION;RSVP=TRUE;CN="Yang, Jer
 ry (CAR:RF11)":MAILTO:YANGJ@nortel.com
ORGANIZER;CN="Yang, Jerry (CAR:RF11)":MAILTO:YANGJ@nortel.com
LOCATION:
DTEND;TZID="(GMT-05.00) Eastern Time (US & Canada)":20071026T130000
DESCRIPTION:test\N
SEQUENCE:2
PRIORITY:5
CLASS:
CREATED:20071026T155721Z
LAST-MODIFIED:20071026T155751Z
STATUS:CONFIRMED
TRANSP:OPAQUE
X-MICROSOFT-CDO-BUSYSTATUS:BUSY
X-MICROSOFT-CDO-INSTTYPE:0
X-MICROSOFT-CDO-INTENDEDSTATUS:BUSY
X-MICROSOFT-CDO-ALLDAYEVENT:FALSE
X-MICROSOFT-CDO-IMPORTANCE:1
X-MICROSOFT-CDO-OWNERAPPTID:-1
X-MICROSOFT-CDO-ATTENDEE-CRITICAL-CHANGE:20071026T155751Z
X-MICROSOFT-CDO-OWNER-CRITICAL-CHANGE:20071026T155751Z
BEGIN:VALARM
ACTION:DISPLAY
DESCRIPTION:REMINDER
TRIGGER;RELATED=START:-PT00H15M00S
END:VALARM
END:VEVENT
END:VCALENDAR

The "good" .ics file sourced from Lightning:

BEGIN:VCALENDAR
PRODID:-//Mozilla Calendar//NONSGML Sunbird//EN
VERSION:2.0
METHOD:REQUEST
BEGIN:VTIMEZONE
TZID:/mozilla.org/20070129_1/America/Toronto
X-LIC-LOCATION:America/Toronto
BEGIN:DAYLIGHT
TZOFFSETFROM:-0500
TZOFFSETTO:-0400
TZNAME:EDT
DTSTART:19700308T020000
RRULE:FREQ=YEARLY;INTERVAL=1;BYDAY=2SU;BYMONTH=3
END:DAYLIGHT
BEGIN:STANDARD
TZOFFSETFROM:-0400
TZOFFSETTO:-0500
TZNAME:EST
DTSTART:19701101T020000
RRULE:FREQ=YEARLY;INTERVAL=1;BYDAY=1SU;BYMONTH=11
END:STANDARD
END:VTIMEZONE
BEGIN:VEVENT
CREATED:20071026T165225Z
LAST-MODIFIED:20071026T165255Z
DTSTAMP:20071026T165255Z
UID:6fb53780-e1d7-451c-9694-40d17668edcb
SUMMARY:test3
ORGANIZER;PARTSTAT=ACCEPTED;ROLE=REQ-PARTICIPANT:MAILTO:yangj@nortel.com
ATTENDEE;RSVP=TRUE;CN=Jerry Yang;PARTSTAT=NEEDS-ACTION;
 ROLE=REQ-PARTICIPANT:MAILTO:yangj@nortel.com
DTSTART;TZID=/mozilla.org/20070129_1/America/Toronto:20071026T131000
DTEND;TZID=/mozilla.org/20070129_1/America/Toronto:20071026T141000
X-MOZ-SEND-INVITATIONS:TRUE
SEQUENCE:1
BEGIN:VALARM
TRIGGER;VALUE=DURATION:-PT15M
DESCRIPTION:Mozilla Alarm: test3
ACTION:DISPLAY
END:VALARM
END:VEVENT
END:VCALENDAR
Comment 79 Sebastian Schwieger 2007-11-03 03:56:04 PDT
This is one of the main three goals for version 0.8 [1]. A release shouldn't happen without this bug fixed. Requesting blocking stauts.

[1] http://wiki.mozilla.org/Calendar:Status_Meetings:2007-10-31
Comment 80 Sebastian Schwieger 2007-11-11 02:03:35 PST
It looks like there is a broad disagreement with my point of view. Removing blocking request.
Comment 81 Simon Paquet [:sipaq] 2007-11-11 06:27:05 PST
We will not release 0.8 without this.

Clint, what is the status on this bug? As we need a fair amount of testing before pushing this out in a release, this should land weeks or even months before the scheduled release date.
Comment 82 Stefan Sitter 2007-12-07 05:55:01 PST
*** Bug 407336 has been marked as a duplicate of this bug. ***
Comment 83 Daniel Boelzle [:dbo] 2008-01-21 01:52:17 PST
Clint, since bug 400950 and bug 402518 are done, could this one be closed finally or is something still missing?
Bug 363191 and bug 406579 handle the timezone registry (our Olson reference set), which is IMO separate from this.
Comment 84 cmtalbert 2008-01-22 14:15:54 PST
Daniel, I agree.  Let's close this one.

I for one am happy to see it go.

Going to mark it as WFM since the issues were fixed in other bugs.
Comment 85 Stefan Sitter 2008-01-24 23:23:24 PST
In my opinion this should be resolved as fixed by bug 400950 and bug 402518 allowing retest and validation.
Comment 86 Stefan Sitter 2008-01-24 23:24:00 PST
*** Bug 413925 has been marked as a duplicate of this bug. ***
Comment 87 Thomas E. Deutsch 2008-02-04 02:21:52 PST
Daniel, Clint and Stefan: Can someone verify that this is really fixed? Because I can't. Using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.12pre) Gecko/20080203 Calendar/0.8pre, I've tried to check this, but it does not work for me. A event, created on Apple iCal (on Tiger), is shown exactly as wrong as before. Event is scheduled for 3pm (Europe/Zurich), but is showed as 4pm. Is this my mistake or does this bug still exist?
Comment 88 Matp75 2008-02-04 02:55:09 PST
Using Lightning 0.8pre 2008020218, old events received via email and created by outlook, which used to be shown one or two hours in the future are now showing correctly. This doesn't prove all cases are fixed but the situation is much better than before !
Comment 89 Bas van den Bosch 2008-02-04 04:59:20 PST
Thomas, could you attatch the ics which gives a erronuous time?
Comment 90 Thomas E. Deutsch 2008-02-04 05:07:15 PST
Created attachment 301262 [details]
Event on Feb 6th is showed one hour too late

This is the ics which still makes problems. Its the event on Feb 6th, which is at 15:00, but showed as 16:00.
Comment 91 Stefan Sitter 2008-02-04 06:02:57 PST
Comment on attachment 301262 [details]
Event on Feb 6th is showed one hour too late

The referenced timezone in the attached calendar file is specified as:

   BEGIN:VTIMEZONE
   TZID:Europe/Zurich
   END:VTIMEZONE

As you can see no UTC offset is specified, hence the event is treat as 15:00 UTC. This corresponds to 16:00 in your display timezone - assuming you have configured Zurich (UTC+1) in Sunbird. Therefore Sunbird correctly displays the event at 16:00.

Also note that the fix only applies to foreign events that will be added in the future. Foreign events that were already stored in the database using Sunbird 0.7 still will be displayed wrong.

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