GNOME Bugzilla – Bug 722416
Don't change the timezone name
Last modified: 2014-02-07 16:12:24 UTC
The timezone mapping done between libical (Olson format) and MSDN format is not 1:1. So, sometimes can happen that the user set "America/Sao_Paulo" as the timezone and when it's refreshed, it is shown as an equivalent timezone but with a different name.
What I had on mind when suggesting this was to create a new named property on the folder item, where will be stored the used libical zone name, and then on read, if the UTC offset will match the one from EWS properties, then use it. That means to involve PropertyName [1] and use our own name - it'll be good to have some prefix and value. [1] http://msdn.microsoft.com/en-us/library/aa564843%28v=exchg.140%29.aspx
Thinking a bit about it ... If I understand correctly I can create an extended property and we can store, in the server, the timezone prefix and name used by libical. Is it? Considering this, I can remove one of the hash tables and a few functions. We can have the code even simpler than it is. So we don't need to check if the UTC offset will match. Actually, we can just ignore this conversion MSDN -> libical, don't we?
(In reply to comment #2) > Considering this, I can remove one of the hash tables and a few functions. We > can have the code even simpler than it is. So we don't need to check if the UTC > offset will match. Actually, we can just ignore this conversion MSDN -> > libical, don't we? Nah, we don't. We still have the cases where the event was not created by evo. But yes, seems doable anyway.
(In reply to comment #2) > Thinking a bit about it ... > If I understand correctly I can create an extended property and we can store, > in the server, the timezone prefix and name used by libical. Is it? Correct. It should be doable. Your task is to figure out whether it is. I want the check for UTC offset to not garble event, like when you create it in evolution, then edit it in Outlook, changing timezone, and then open it in evolution. Without UTC offset check you might get wrong timezone used. (In reply to comment #3) > Nah, we don't. We still have the cases where the event was not created by evo. > But yes, seems doable anyway. Correct too, evolution is not the only client accessing exchange calendars :)
(In reply to comment #4) > Correct. It should be doable. Your task is to figure out whether it is. It's partially doable. Thinking about an event, we should take care of the timezones in two different actions: #1 - Create an event: This is the part where it's doable. CalendarItem[0] does have support to ExtendedProperties and we can, easily, go with your suggestion. #2 - Changing an event: This is the problematic part. I didn't find a way to set the Extended Properties in the SetItemField[1], used by UpdateItem[2], when an event is changed. [0]: http://msdn.microsoft.com/en-us/library/aa564765(v=exchg.140).aspx [1]: http://msdn.microsoft.com/en-us/library/aa581487(v=exchg.140).aspx [2]: http://msdn.microsoft.com/en-us/library/aa580254(v=exchg.140).aspx Tips about #2? The implementation is still doable, using only the #1, but not as optimal as expected previously :-(
(In reply to comment #5) > Tips about #2? Naah. Nevermind. After a bit of brute force it's working.
(In reply to comment #6) > Naah. Nevermind. > After a bit of brute force it's working. Nice :) It would be weird to have support for extended properties only in Create, but not in Update actions.
Created attachment 267463 [details] [review] Bug #722416 - Don't change the timezone name
Created attachment 267606 [details] [review] Bug #722416 - Don't change the timezone name
Review of attachment 267606 [details] [review]: OK, it basically looks fine, I've only few comments: a) "EvolutionEWSStartTime" name is misleading, you do not store there the time, but the time _zone_. So it should be named that way - similar for EndTime. b) the "EvolutionEWSEndTime" is not used on all places as the "EvolutionEWSStartTime", maybe it's not a problem, maybe not, please verify (for example at ews_cal_sync_get_items_sync() you add only the "EvolutionEWSStartTime", but not "EvolutionEWSEndTime" c) finally, if I read all the places correctly, then EEwsExtendedFieldURI/EEwsIndexedFieldURI/EEwsAdditionalProps all store statically allocated string, no need to duplicate them (there is one place, the MAPI properties identified by their ID, where you use g_strdup_printf ("%d", for them, but the server is capable or reading 0xFFFF too, isn't it? if it is, then this only place can be easily made with static strings as well). I would still keep the ..._free functions for these structures, in case we'll find a need to store dynamically allocated strings in the values or anything like that.
(In reply to comment #10) > b) the "EvolutionEWSEndTime" is not used on all places as the > "EvolutionEWSStartTime", maybe it's not a problem, maybe not, please verify > (for example at ews_cal_sync_get_items_sync() you add only the > "EvolutionEWSStartTime", but not "EvolutionEWSEndTime" Yeah. The old code was basically ignoring EndTimeZone. I'm still confused in how we can use it properly. I mean, when we get the item from the server ... should we look for StartTimeZone and EndTimeZone? And about creating an item? It has been done only using the start time zone for old servers. Any suggestion is welcome :-) > c) finally, if I read all the places correctly, then > EEwsExtendedFieldURI/EEwsIndexedFieldURI/EEwsAdditionalProps all store > statically allocated string, no need to duplicate them (there is one place, the > MAPI properties identified by their ID, where you use g_strdup_printf ("%d", > for them, but the server is capable or reading 0xFFFF too, isn't it? I'd say "sometimes". My first attempt was trying to use only statically allocated strings and then I had a few problems with MAPI properties (the last problem was with the property "0x8530", for instance). So, for consistency and because safer is better, I'm duping every string.
(In reply to comment #11) > (In reply to comment #10) > > > b) the "EvolutionEWSEndTime" is not used on all places as the > > "EvolutionEWSStartTime", maybe it's not a problem, maybe not, please verify > > (for example at ews_cal_sync_get_items_sync() you add only the > > "EvolutionEWSStartTime", but not "EvolutionEWSEndTime" > > Yeah. The old code was basically ignoring EndTimeZone. > I'm still confused in how we can use it properly. I mean, when we get the item > from the server ... should we look for StartTimeZone and EndTimeZone? > And about creating an item? It has been done only using the start time zone for > old servers. libical has also timezones for DTSTART and DTEND, thus the change is straightforward - use them both consistently. To create such event, you might probably edit the iCal file yourself, I'm not aware of a software which allows setting different zone for the start and end times. > I'd say "sometimes". > My first attempt was trying to use only statically allocated strings and then I > had a few problems with MAPI properties (the last problem was with the property > "0x8530", for instance). > So, for consistency and because safer is better, I'm duping every string. OK, then keep it as is for now, and we can revisit if it'll have any negative impact on performance (it definitely does make valgrind busy on something which is not that crucial, but I agree that runing under valgrind is not done that often, neither our target).
(In reply to comment #12) > libical has also timezones for DTSTART and DTEND, thus the change is > straightforward - use them both consistently. To create such event, you might > probably edit the iCal file yourself, I'm not aware of a software which allows > setting different zone for the start and end times. How evo-ews was doing this before my changes? After my changes, for supported timezones and when the server is newer than 2010 I'm using start and end timezone. However, when the event is created by evultion, start and end timezone are the same. When we are modifying the timezone, I'm also using start and end timezone. But here, what I'd like to see is evolution "cheating" a bit and changing the end timezone to the same one used as start timezone. How does it sound to you? (I'm not sure if it's not the current behavior). When we are updating an item we received from the server, I'd like to do the same. I'm asking those questions because using the 2007 servers evo-ews only sets the "MeetingTimeZone" using the start date. Sorry if what I'm saying does not make sense, but I'm still a bit confused with this.
Well, evo-ews did it wrong. Or, better said, that was the only option with properties which 2007 server protocol version offered. Since you upgraded the protocol version to 2010+, when talking to 2010+ servers, evo-ews got an option to do the same thing better, by setting extra timezone for start time, and extra for end time, the same way the libical allows it (remember that Tasks use Due date, not End time, with the same meaning - I might check it, as I do not know off-head, but probably Due date in Exchange servers is not tight to any certain timezone). That said, you can now support two things: a) behave like before your change, for 2007 servers b) use both Start and End time zones when reading and writing times from/to exchange 2010+ servers Cheating by changing user-set times by convert between timezones is unacceptable to me :) (not that it would solve some issues).
Created attachment 267906 [details] [review] Bug #722416 - Don't change the timezone name
The patch doesn't seem to work properly. I created an event in Europe/Vienna timezone, and when server responds back with the save, the timezone is changed to Europe/Prague. The two timezones are not exactly the same (diff shows some differences before 1981), but they are otherwise the same, at least these days, thus I expect to see Europe/Vienna when evoltuion-ews receives an update from the server. This was with an Exchange 2010. I recall I suggested you to use current time for the timezone offset checking, but thinking of it, the correct would be to check DTSTART (and/or DTEND/DUE) time for the two timezone candidates. I've also got runtime warnings on the calendar factory console: libecal-CRITICAL **: e_timezone_cache_get_timezone: assertion 'tzid != NULL' failed one with a backtrace: (gdb) bt
+ Trace 233116
Created attachment 268259 [details] [review] Bug #722416 - Don't change the timezone name
Review of attachment 268259 [details] [review]: Please see below, you added some memory leaks to the code, and a very bad habit on confusing memory management (which also causes memory leaks). Please fix this first. ::: src/calendar/e-cal-backend-ews.c @@ +2895,3 @@ + if (tzid == NULL) { + /* + * When we are working with Exchanfe server 2010 or newer, we have to handle a few typo 'Exchanfe' @@ +2899,3 @@ + * - MSDN timezone names: + * Used setting StartTimeZone and EndTimeZone. MSDN timezone names are not + * the same used in libical, so we need to have an table of equivalence to 'a table' @@ +2905,3 @@ + * Used to keep track if the timezone shown to the user is the same one set + * by him/her. As we have a table of equivalence, sometimes the user sets a + * timezone but. without EvoEWSStartTiemZone property, another timezone name, extra dot after 'but' @@ +2910,3 @@ + * As we have to work with DTEND setting an event when using EWS server 2010 or + * newer, we have to care about set it properly here, instead of use the same + * on used in DTSTART. s/on/as is/ @@ +2983,3 @@ + } + + zone = NULL; unneeded assignment (Covscan and similar tools may claim on this) @@ +3399,3 @@ + + g_slist_free_full (add_props->extended_furis, g_free); + add_props->extended_furis = NULL; a) wrong indent for the two commands above; b) why do you do the extra free here? I'd expect the e_ews_additional_props_free() takes care of it; I see there are more places like this, where you free internal members of the structure on your own, instead of using the free method. Such thing only adds to the bad memory management habit in the ews code. :) c) the 'add_props' variable can be local for this if only, not global for whole function. ::: src/camel/camel-ews-folder.c @@ +141,3 @@ +ews_folder_get_summary_message_mapi_flags (void) +{ + GSList *list = NULL; unused assignment ::: src/server/e-ews-connection.c @@ +405,3 @@ + if (add_props != NULL) { + g_free (add_props->field_uri); + g_slist_free_full (add_props->extended_furis, g_free); incorrect free function, it holds a structure, not a simple pointer, thus it should be e_ews_extended_field_uri_free(), instead of g_free. @@ +406,3 @@ + g_free (add_props->field_uri); + g_slist_free_full (add_props->extended_furis, g_free); + g_slist_free_full (add_props->indexed_furis, g_free); Similar here, the SList holds EEwsIndexedFieldURI, which has two non-const gchar* members. ::: src/server/e-ews-item.c @@ +1520,3 @@ +e_ews_item_get_iana_end_time_zone (EEwsItem *item) +{ + g_return_val_if_fail (E_IS_EWS_ITEM (item), "UTC"); Why do you return UTC here (and above too)? Both can be NULL, thus return NULL on error too.
I did some action-testing, and it basically works, except of one case: a) create an event in evolution in Europe/Prague time zone b) save it, wait for a refresh, open it - see it's Europe/Prague - good, so far c) open it and change timezone to Europe/Rome (it has same UTC offset, somehow) d) save it, wait for a refresh, open it - see there is still Europe/Prague, instead of Europe/Rome. I can repeat it the other way too, start with Europe/Rome and try to change to Europe/Prague.
Err, on calendar factory console: > CRITICAL **: e_cal_backend_ews_tz_util_get_msdn_equivalent: assertion > 'ical_tz_location != NULL' failed
Created attachment 268321 [details] [review] Bug #722416 - Don't change the timezone name
Review of attachment 268321 [details] [review]: Final things, please. ::: src/calendar/e-cal-backend-ews-utils.c @@ +2170,3 @@ + } + + if ((dt_end_changed || dt_start_changed_timezone_name) && ical_location_start != NULL) suspicious: '(end || start) && start' while few lines below is '(end || end) && end' ::: src/camel/camel-ews-folder.c @@ +639,3 @@ g_cond_broadcast (&priv->fetch_cond); + e_ews_additional_props_free (add_props); this is causing double free ::: src/server/e-ews-item.c @@ +575,3 @@ + else if (g_strcmp0 (name, "EvolutionEWSEndTimeZone") == 0) + priv->iana_end_time_zone = g_strdup (value); + } else { I did not notice this before, but does this 'if' mean that you cannot store string properties addressed by ID, but only those addressed by their name? It doesn't look correct.
Created attachment 268333 [details] [review] Bug #722416 - Don't change the timezone name
Created attachment 268334 [details] [review] Bug #722416 - Don't change the timezone name
Review of attachment 268334 [details] [review]: Looks good and works fine, please commit to master. Thanks.
Pushed to master as 34e0db4 (evo-ews 3.11.90+)