Bug 79014

Created attachment 99503 [details] [review]
fixes regressions from original patch

Created attachment 102164 [details] [review]
Fixed timezone cache and remove hardcoded strings

Created attachment 102165 [details]
Optimize-calendar-events-update-when-possible.

(In reply to comment #3)
> Created attachment 102165 [details]
> Optimize-calendar-events-update-when-possible.

Can you explain why this is using a time zone's display name as key for the cache?

I ran a full test without problems after applying the following change. See "ecal" branch.

commit b44b45b74207c2710415fd9d040427bdcefb67d4
Author: Patrick Ohly <patrick.ohly@intel.com>
Date:   Mon Jul 21 01:07:42 2014 -0700

    EDS calendar: cache by TZID
    
    The display name is not necessarily a unique identifier for the content of a
    time zone. For example, the 1990 version of CET/CEST might be different from
    the 2014 version. Therefore the display name should not be used as identifier.
    
    The TZID is not necessarily better, but at least the code in
    e-cal-check-timezones.c tries to verify content equality before reusing an
    existing timezone. It also has code which matches time zones based on their
    Olson name embedded in the TZID. If that code does not work well enough
    (for example, see FDO #81590), then it should be enhanced.

The problem with icaltimezone_get_display_name() also was that it's not currently covered by src/syncevo/eds_abi_wrapper.h, which causes compile errors when building binaries for syncevolution.org. This could be fixed, of course.

Hi Patryck,

Sorry for take so long to reply, I have been very busy these days.

Ok. I am complete lost here I do not understand how this tz names works. 
Just say want you want to me to do to make it acceptable to merge :D. (in last case lets remove the timezone cache patch).


Do you want to me to use tzid as the key for the cache? I am ok with that.

I do not see a big problem if the cache fail and for some reason stores 2 timezones with the same value.

The main reason to have the cache here is to avoid query eds for new timezone every time that you want to insert an event.
The only problem of storing two timezones with the same value is that EDS will do one extra query, and that still better that a query for every insert.

Let's look at this again...

What I'd like to understand better is which timezones this patch caches. Some examples would be nice.

I was not able to reproduce the timezone problem. We can ignore the timezone optimizations and use the others patches.

(In reply to renato filho from comment #8)
> I was not able to reproduce the timezone problem. We can ignore the timezone
> optimizations and use the others patches.

In the future, remember to include such information in the commit message or the source code itself. I still think that the patch has some merit, but without specific examples it is hard to determine how often it is useful and thus whether the added complexity is worth it.

My guess is that it is relevant for events created by Exchange, but I haven't tried it. I might have a look myself.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/SyncEvolution/syncevolution/issues/16.