We need to make sure that TZIDs and VTIMEZONEs that do not originate from our internal database are properly preserved in all calendars.
Taking, as wrapping one's head around the flow of a timezone through the code takes a while, and I'm mostly already there.
0.1 is now targetted at not having any dataloss bugs on self-generated data, so this can wait.
*** Bug 336640 has been marked as a duplicate of this bug. ***
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.
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.
(In reply to comment #5)
Original ics content (shortened to one event) from [http://ical.mac.com/corgimom28/Lost.ics]:
PRODID:-//Apple Computer\, Inc//iCal 2.0//EN
SUMMARY:Lost (3.02): \"Further Instructions\"
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:
PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN
SUMMARY:Lost (3.02): \"Further Instructions\"
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+
This will probably need a schema change, so the landing of this bug should be coordinated with that of bug 333688.
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.
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?).
*** Bug 356905 has been marked as a duplicate of this bug. ***
*** Bug 362495 has been marked as a duplicate of this bug. ***
*** Bug 366567 has been marked as a duplicate of this bug. ***
Bumping the priority on this.
This is on my "must fix" hit list for 0.5.
*** Bug 361046 has been marked as a duplicate of this bug. ***
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.
Created attachment 252922 [details]
This shows the event before and after adding to lightning from the iTIP based STR
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.
see also bug 365812 and bug 357704.
Reassigning to ctalbert
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...
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.
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.
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.
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 :-)
(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.
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.
(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.
*** Bug 357704 has been marked as a duplicate of this bug. ***
*** Bug 365812 has been marked as a duplicate of this bug. ***
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.
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?
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.
(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?
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
(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?
(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.
(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.
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.
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 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.
(In reply to comment #38)
> 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
> 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.
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)
** 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.
Does not block the release, as per the weekly conference call.
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.
*** Bug 378019 has been marked as a duplicate of this bug. ***
*** Bug 376011 has been marked as a duplicate of this bug. ***
*** Bug 355449 has been marked as a duplicate of this bug. ***
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.
I have some problems with an .ics invitation that uses the following timezone info
TZID:Kuala Lumpur\, Singapore
Which works if I open the event in lightning 0.5 and save it
TZID:GMT +0800 (Standard) / GMT +0800 (Daylight)
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
*** Bug 387658 has been marked as a duplicate of this bug. ***
*** Bug 387660 has been marked as a duplicate of this bug. ***
I've Problems importing .ics from Apple iCal:
DESCRIPTION:9 Uhr bis 12 Uhr
SUMMARY:Process Kick-off Meeting
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
One more that gives in-correct times after a restart:
TB 184.108.40.206pre 20070719
Calendar 0.7pre 2007072604
PRODID:Microsoft CDO for Microsoft Exchange
SUMMARY:FOSS software development (Open Source Competency centre) & Citizen
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?
@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.
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.
(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.
(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
> 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
> 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.
(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.
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.
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 ;-)
(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.
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 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.
(In reply to comment #64)
> (From update of attachment 277332 [details] [diff] [review])
> (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?
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.
Thanks for your comments and ideas. Keep them coming.
*** Bug 395885 has been marked as a duplicate of this bug. ***
(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.
(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).
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.
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.
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...
*** Bug 396540 has been marked as a duplicate of this bug. ***
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 #68 will be handled in separate bug 400950.
I think it's a very important problem and should be fixed as early as possible.
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.
This is definitely planned for 0.8
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:
PRODID:Microsoft CDO for Microsoft Exchange
TZID:(GMT-05.00) Eastern Time (US & Canada)
DTSTART;TZID="(GMT-05.00) Eastern Time (US & Canada)":20071026T123000
ORGANIZER;CN="Yang, Jerry (CAR:RF11)":MAILTO:YANGJ@nortel.com
DTEND;TZID="(GMT-05.00) Eastern Time (US & Canada)":20071026T130000
The "good" .ics file sourced from Lightning:
PRODID:-//Mozilla Calendar//NONSGML Sunbird//EN
DESCRIPTION:Mozilla Alarm: test3
This is one of the main three goals for version 0.8 . A release shouldn't happen without this bug fixed. Requesting blocking stauts.
It looks like there is a broad disagreement with my point of view. Removing blocking request.
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.
*** Bug 407336 has been marked as a duplicate of this bug. ***
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.
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.
In my opinion this should be resolved as fixed by bug 400950 and bug 402518 allowing retest and validation.
*** Bug 413925 has been marked as a duplicate of this bug. ***
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:220.127.116.11pre) 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?
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 !
Thomas, could you attatch the ics which gives a erronuous time?
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 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:
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.