Proposal: change GLib timezone logic to libical

[mcclurgm]

I'd like to propose that we simply remove (or deprecate) Calendar.Util.icaltime_get_timezone. Here's my pitch: it is potentially fragile and can cause data loss, and is very much dependent on our ability to properly match timezones in any calendar event to some built-in timezone that GLib will recognize. As far as I can tell, this is done only by the TZID. (I have not seen any other solution in other programs' source code, although there could certainly be others.) This works fine for events that were created on Linux (and apparently Mac) systems, which tend to use the Olson TZID standard that's recognized by GLib. The problem comes when an event uses a non-standard timezone, or potentially if the definition of a timezone changes. (For example, if the US stops using daylight savings, we'll see that impact quite soon!) Then, matching a TZID will fail and cause time display errors (see #572). Even if we could get the fallbacks to work, we would still wind up losing timezone info, since we'd be creating it timezone from offset only (this would cause issues if we edited the time across a daylight savings boundary, for example). We could work around this by using ECal.Client.check_timezones_sync. This would solve those mapping issues. The issue then would be, how would we want to apply it? Do we want to run it on each event on startup/sync, and do we want to save the modified timezones to the events?

I would propose that we forgo this method altogether and use libical's built-in timezone conversion tools. The groundwork for this has mostly been laid out. The only case where we use a GLib timezone is in Calendar.Util.icaltime_to_datetime. The resulting times are only used for display (such as in the agenda view), as far as I can tell, so they're immediately converted to the local timezone. So I suggest that we do the conversions to local time in libical. We would then restrict these to be used for display, basically like temporary variables.

The only case where we would need to deal with GLib times that aren't purely for display is in the editor, which are by definition modified. But this should be a simple matter, since the editing sequence should be unaffected by converting in libical. If #653 is accepted (we have no visible timezone support), we display the local time for editing. Then, on save, we convert it back to its original timezone using the original libical timezone. None of this would be affected, since the GLib timezone is not used here. (This could potentially use some discussion, but I'll bring that up on the relevant PR. If we change it that would be to set the timezone to local, which would still be unaffected.)

This does have some disadvantages. We would lose all timezone information in the resulting GLib DateTimes. I think that in that case, we wouldn't be able to do calendrical calculations without causing possible errors. (For example, there could be an issue if there's a daylight savings change in either the system or event timezone.) I believe that user editing would be acceptable, since they know exactly what time they want the event to occur, but I don't think automatic calculations would be safe. So these DateTimes would truly have to be temporary variables, and we'd have to make explicit in our code so that it won't cause issues in the future.

Implementing this should be fairly simple, since we don't have any centralized store. All our GLib.DateTimes are pretty much implicitly temporary, so changing our logic to use libical shouldn't require much surgery. This is what brought me to suggest this change: in order to deal with things like Windows-formatted timezones and keep all timezone information in GLib, we may have to do some fairly deep surgery to our code. It looks possible that we'd have to pass ECal.Clients all the way down to Calendar.Util.icaltime_get_timezone, which wouldn't be much fun. So maybe, this change would be easier to implement than those sorts of shims, and would be simpler to boot.

The advantage of this change would be that issues like #572 should no longer be possible. We would use libical for all the time logic, and it's aware of the timezones in each event by design, and does not have to convert between systems. As far as I can tell, this would pretty much eliminate an entire category of possible bugs.

Let me know what you think, especially if you find any issues I haven't thought of! It's very possible that I haven't thought of a lot of things here.

[tintou]

I'm alright with this, we probably used GLib.Timezone at first because we had to deal with libical instead of libical-glib so it's not an issue anymore to deal with ICal objects. So whatever makes the codebase easier to follow and avoids information loss is great to me.

[mcclurgm]

Alright great, I'll give it a shot. I hope it won't be too difficult, but we'll see.