Re: [Evolution-hackers] cleaning up the timezone handling mess

["P Chenthill" <pchenthill novell com>]

On Thu, 2008-04-03 at 19:45 +0200, Patrick Ohly wrote:
> Hello,
> 
> did the slightly inflammatory subject catch your attention? Good, please
> keep reading... ;-)
> 
> Details can be found in multiple Bugzilla entries, the most important
> one apparently being this one:
> http://bugzilla.gnome.org/show_bug.cgi?id=301363
> 
> Let me summarize the current situation: since 2.12, Evolution uses the
> host's timezone information. This is only a partial solution. For events
> created by other programs the incomplete and sometimes obsolete
> VTIMEZONE information is still used.
> 
> As an example, take the two attached meeting invitations. The "2006" one
> is a real stripped down invitation from Outlook, created in 2006 using
> the old DST rules. The newer "2008" one uses the current rules.
> 
> When importing "2006" into a local calendar file, the "GMT -0600
> (Standard) / GMT -0500 (Daylight)" timezone definition is added to the
> calendar. When importing "2008", the timezone definition is not updated.
> This has the effect that the new meeting is not displayed correctly in
> the calendar. But replacing the timezone definition would not be correct
> either, because it would break the mapping of past events of the old
> event before the DST change in 2007.
> 
> This is a conceptual problem of how timezone definitions are handled:
> they should be attached to events, not to calendars. The Itip formatter
> plugin demonstrates that: it uses the timezone definition included in
> the meeting invitation and correctly calculates the local times for both
> events.
> 
> One could argue that the sending program is at fault: it could have used
> a different TZID after changing the timezone definition. Alternatively
> it could have sent a more complex VTIMEZONE which also includes the
> historic rules. I believe both causes other problems in practice.
> Speculating about it is futile anyway and won't change the meeting
> invitations that Evolution has to deal with.
> 
> Besides, don't blame Outlook too much: Evolution >= 1.12 now does
> exactly the same thing. For example, a meeting invitation now uses
> "/softwarestudio.org/Tzfile/America/Los_Angeles" and only includes the
> current rules for daylight saving transitions.
It just includes the current rules for outlook compatibility.

> 
> The "2006" event demonstrates another problem: the recurrences during
> those weeks in 2007 and 2008 where old and new rules differ are not
> calculated correctly.
> 
> A user of SyncEvolution and ScheduleWorld reported timezone issues here:
> http://www.scheduleworld.com/jforum/posts/list/1543.page
> 
> In the ensuing discussion Mark Swanson from ScheduleWorld suggested that
> all clients should use the same TZIDs to reference a complete local
> Olsen database. I don't think that this is doable in all cases, but I
> think Evolution should at least try it. Without further ado, here's my
> proposal how importing events into Evolution should work. If you agree,
> then I can try to create a patch.
> 
> For each TZID used in an event, Evolution should try to find the
> corresponding system timezone via a fuzzy search. If the TZID already
> follows the pseudo-standard set by the Olsen database, then the
> comparison could be based on the local part, e.g. "America/Los_Angeles".
> This solves the problem of importing events generated by ScheduleWorld
> (which currently uses a /scheduleworld.com/<revision date>/ prefix and
> thus is not mapped to system time zones).
> 
> If the TZID is in some other format (like the ones used by Outlook),
> then a hard-coded table could do the mapping. This would solve the
> problem with the "2006" example. This step is kind of ugly and optional:
> I myself would argue that for such recurring meetings the sender is
> responsible for sending an update with the current VTIMEZONE. In my
> experience this really happened in many cases beginning of 2007.
> 
> If a match is found, then the event is stored with the original TZIDs
> replaced by the matching system ones. The VTIMEZONE is discarded.
> 
> If no match is found, then the custom VTIMEZONE definition must be
> stored in the calendar. I don't think that the EDS infrastructure should
> be changed. It would break APIs and be a lot of work to implement, plus
> it would store multiple redundant copies of identical VTIMEZONE
> definitions, leading to worse performance.
> 
> What I suggest instead is the following scheme: check whether the
> calendar already has a VTIMEZONE with a given TZID. If one is found,
> check whether the timezone definition is really identical. If it is, all
> is well. If not, then rename the new timezone by attaching " <number>".
> Repeat the check and renaming until a 100% match is found or the
> timezone can be stored without colliding with an existing timezone. Then
> proceed with importing the event with renamed TZIDs.
> 
> I believe that this could be implemented without changing anything in
> libecal. I could implement it as part of SyncEvolution, but that won't
> solve the problem when Evolution itself imports events. A new API call
> in libecal would be a better solution.

> Before I proceed, let me ask: are there other plans to tackle the
> problem? Is someone working on it? Similar questions in #301363 were not
> answered.
> 
> Do you agree with the outlined plan in principle? If no-one else is
> working on it and you agree, then I would describe the new API call in
> more detail and see whether I can get it implemented. I would reuse the
> same code in SyncEvolution in order to improve the situation with older
> Evolution releases and to avoid depending on an extended libecal in the
> binary releases.
I am just back after a small vacation.

I dont think anyone is work on this issue currently. I made a patch for
exchange backend for solving this issue long time back. But
unfortunately its not committed in trunk. It uses a timestamp in the
VTIMEZONE file to store the latest one always. By this way we would also
know the last time period when the timezone has been changed for that
location. Have attached the patch for a reference.

I am ok with handling using the sequence numbers. I am not sure whether
a new libecal API would be needed to solve this, since the timezones
would be handled at the backend. Overall I can understand the approach
and it seems good.

It would be better to handle all the timezones in a common place inside
EDS. The reason for that is, same timezones can exist in multiple
calendars which could not be matched to either system timezone or
through the fuzzy logic. Currently it is handled separately for every
calendar and the same information is stored redundantly. Another issue
which should be taken care of is, the system timezones should not be
stored in the cache. They should always be taken from the libical
builtin timezone. Please consider these too while implementing the
solution.

> 
> What is the timeline for this? 2.22 is released; if this version is
> picked up by distributions unpatched, then we are bound to run into the
> same problems again end of 2008 and probably also 2009.
If there are no interface changes, we can definitely take the patch in
for stable branch. Please come up with the initial design and once the
patch is ready, we can also look at requesting for a freeze break to get
the patch in for the stable branch also.

- Chenthill.

Index: calendar/e-cal-backend-exchange.c
===================================================================
--- calendar/e-cal-backend-exchange.c	(revision 1330)
+++ calendar/e-cal-backend-exchange.c	(working copy)
@@ -1094,6 +1094,27 @@
 	return priv->default_timezone;
 }
 
+static icaltimetype 
+get_timestamp_from_tzcomp (icalcomponent *icalcomp)
+{
+	icalproperty *icalprop;
+	icaltimetype dtstamp = icaltime_null_time ();		
+
+	for (icalprop = icalcomponent_get_first_property (icalcomp, ICAL_X_PROPERTY); icalprop != NULL;
+			icalprop = icalcomponent_get_next_property (icalcomp, ICAL_X_PROPERTY)) {
+		const char *name = icalproperty_get_x_name (icalprop);
+
+		if (name && g_str_equal (name, "X-EVOLUTION-TZ-TIMESTAMP")) {
+			const char *value = icalproperty_get_x (icalprop);
+			dtstamp = icaltime_from_string (value);
+			break;
+		}
+	}
+
+	return dtstamp;
+}
+
+
 ECalBackendSyncStatus
 e_cal_backend_exchange_add_timezone (ECalBackendExchange *cbex,
 				     icalcomponent *vtzcomp)
@@ -1109,9 +1130,19 @@
 		return GNOME_Evolution_Calendar_InvalidObject;
 	tzid = icalproperty_get_tzid (prop);
 	d(printf("ecbe_add_timezone: tzid = %s\n", tzid));
-	if (g_hash_table_lookup (cbex->priv->timezones, tzid))
-		return GNOME_Evolution_Calendar_ObjectIdAlreadyExists;
+	if ((zone = g_hash_table_lookup (cbex->priv->timezones, tzid))) {
+		icalcomponent *tzcomp_old;
+		icaltimetype dtstamp_old, dtstamp_new;
 
+		tzcomp_old = icaltimezone_get_component (zone);
+			
+		dtstamp_old = get_timestamp_from_tzcomp (tzcomp_old);
+		dtstamp_new = get_timestamp_from_tzcomp (vtzcomp);
+
+		if (icaltime_is_null_time (dtstamp_new) || (!icaltime_is_null_time (dtstamp_old) && icaltime_compare (dtstamp_new, dtstamp_old) <= 0))
+			return GNOME_Evolution_Calendar_ObjectIdAlreadyExists;
+	}
+
 	zone = icaltimezone_new ();
 	if (!icaltimezone_set_component (zone, icalcomponent_new_clone (vtzcomp))) {
 		icaltimezone_free (zone, TRUE);
Index: calendar/e-cal-backend-exchange-calendar.c
===================================================================
--- calendar/e-cal-backend-exchange-calendar.c	(revision 1330)
+++ calendar/e-cal-backend-exchange-calendar.c	(working copy)
@@ -65,12 +65,25 @@
 static void update_x_properties (ECalBackendExchange *cbex, ECalComponent *comp);
 
 static void
-add_timezones_from_comp (ECalBackendExchange *cbex, icalcomponent *icalcomp)
+add_timestamp_to_tzcomp (icalcomponent *tzcomp, icaltimetype dtstamp)
 {
+	icalproperty *icalprop;
+	const char *timestamp;
+
+	timestamp = icaltime_as_ical_string (dtstamp);
+	icalprop = icalproperty_new_x (timestamp);
+	icalproperty_set_x_name (icalprop, "X-EVOLUTION-TZ-TIMESTAMP");
+	icalcomponent_add_property (tzcomp, icalprop);
+}
+
+static void
+add_timezones_from_comp (ECalBackendExchange *cbex, icalcomponent *icalcomp, icaltimetype dtstamp)
+{
 	icalcomponent *subcomp;
 
 	switch (icalcomponent_isa (icalcomp)) {
 	case ICAL_VTIMEZONE_COMPONENT:
+		add_timestamp_to_tzcomp (icalcomp, dtstamp);
 		e_cal_backend_exchange_add_timezone (cbex, icalcomp);
 		break;
 
@@ -78,6 +91,7 @@
 		subcomp = icalcomponent_get_first_component (
 			icalcomp, ICAL_VTIMEZONE_COMPONENT);
 		while (subcomp) {
+			add_timestamp_to_tzcomp (subcomp, dtstamp);
 			e_cal_backend_exchange_add_timezone (cbex, subcomp);
 			subcomp = icalcomponent_get_next_component (
 				icalcomp, ICAL_VTIMEZONE_COMPONENT);
@@ -196,6 +210,7 @@
 	GSList *attachment_list = NULL;
 	gboolean status;
 	ECalBackend *backend = E_CAL_BACKEND (cbex);
+	icaltimetype dtstamp;
 
 	/* Check for attachments */
 	if (uid)
@@ -250,11 +265,15 @@
 		}
 		return FALSE;
 	}
+	
 
-	add_timezones_from_comp (cbex, icalcomp);
-
 	subcomp = icalcomponent_get_first_component (
 		icalcomp, ICAL_VEVENT_COMPONENT);
+	
+	/* Add the timezones with the dtstamp */
+	dtstamp = icalcomponent_get_dtstamp (subcomp);
+	add_timezones_from_comp (cbex, icalcomp, dtstamp);
+	
 	while (subcomp) {
 		if (uid && !strcmp (uid, icalcomponent_get_uid (subcomp)) && attachment_list) {
 			ecomp = e_cal_component_new ();
@@ -1538,7 +1557,7 @@
 	ECalBackendExchangeComponent *ecomp;
 	ECalComponent *comp = NULL;
 	GList *comps, *l;
-	struct icaltimetype current;
+	struct icaltimetype current, dtstamp;
 	icalproperty_method method;
 	icalcomponent *subcomp, *icalcomp;
 	ECalBackendSyncStatus status = GNOME_Evolution_Calendar_Success;	
@@ -1558,9 +1577,13 @@
 	status = e_cal_backend_exchange_extract_components (calobj, &method, &comps);
 	if (status != GNOME_Evolution_Calendar_Success)
 		return GNOME_Evolution_Calendar_InvalidObject;
-		
+
+	dtstamp = icaltime_null_time ();
+	if (comps && comps->data)
+		dtstamp = icalcomponent_get_dtstamp (comps->data);	
+			
 	icalcomp = icalparser_parse_string (calobj);
-	add_timezones_from_comp (E_CAL_BACKEND_EXCHANGE (backend), icalcomp);
+	add_timezones_from_comp (E_CAL_BACKEND_EXCHANGE (backend), icalcomp, dtstamp);
 	icalcomponent_free (icalcomp);
 
 	for (l = comps; l; l= l->next) {