GNOME Bugzilla – Bug 745497
'org.gnome.evolution.calendar' has to be installed
Last modified: 2015-12-06 18:03:59 UTC
The `almanah` command crashes on my computer (Fedora) as Almanah needs the schema 'org.gnome.evolution.calendar' to be installed. Either Almanah should explicitely depends on Evolution (?), or it should handle that case without crashing.
Which Fedora version are you using? Which Almanah Diary version? If it's 0.11, how do you get the application? Compiling by yourself? One of the feature of Almanah Diary is the integration with your calendar appointments and tasks, allowing to show your events or tasks for a given date, but something like that is an optional at compile time. So I'm guessing that in your installation something is missing or something is wrong with the packages.
Almanah checks for EDS (specifically, libecal-1.2 and libedataserver-1.2) at compile time, but the GSettings schema which it uses at runtime for the timezone support (org.gnome.evolution.calendar) comes from Evolution, not EDS. Arnaud, do you have EDS installed, but not Evolution? Álvaro, this raises the question of whether we should still be getting timezone data from Evolution — this code was originally taken from gnome-panel back in the GNOME 2.0 days. Timezone handling in gnome-shell and the desktop as a whole has changed a bit since then. Perhaps we should query the timezone using g_time_zone_new_local() now?
Fedora 22, Almanah 0.11.1-2.fc22. Yes, I have EDS installed (I’m using GNOME), but not Evolution (not needed here).
(In reply to Arnaud Bonatti from comment #3) > Fedora 22, Almanah 0.11.1-2.fc22. Yes, I have EDS installed (I’m using > GNOME), but not Evolution (not needed here). Great, so Almanah either needs to query the timezone from elsewhere (probably the better option) or add a runtime dependency on Evolution. That might be best done by packagers.
(In reply to Philip Withnall from comment #4) > (In reply to Arnaud Bonatti from comment #3) > > Fedora 22, Almanah 0.11.1-2.fc22. Yes, I have EDS installed (I’m using > > GNOME), but not Evolution (not needed here). > > Great, so Almanah either needs to query the timezone from elsewhere > (probably the better option) or add a runtime dependency on Evolution. That > might be best done by packagers. You know the events code better than me so let's go to change the timezone stuff as you say because looks more reasonable. I'll work on this ASAP.
Fedora crash reports: https://retrace.fedoraproject.org/faf/reports/709558/
I have been reading the GTimeZone documentation and I have not found an easy way to retrieve the timezone name. Perhaps with "tzset();" and "tzname[daylight];" would be enough...
Created attachment 315110 [details] [review] Getting timezone allways from system
Review of attachment 315110 [details] [review]: This patch drops automatic updates of the timezone if the system timezone changes, which is rather important. I’ve just found the code for this in libgnome-desktop/gnome-wall-clock.c (https://git.gnome.org/browse/gnome-desktop/tree/libgnome-desktop/gnome-wall-clock.c#n81): it monitors /etc/localtime and re-runs g_time_zone_new_local() when the file changes. We could put similar code in Almanah. It’s too simple to need a dependency on libgnome-desktop to be added to Almanah.
(In reply to Philip Withnall from comment #9) > Review of attachment 315110 [details] [review] [review]: > > This patch drops automatic updates of the timezone if the system timezone > changes, which is rather important. > > I’ve just found the code for this in libgnome-desktop/gnome-wall-clock.c > (https://git.gnome.org/browse/gnome-desktop/tree/libgnome-desktop/gnome-wall- > clock.c#n81): it monitors /etc/localtime and re-runs g_time_zone_new_local() > when the file changes. > > We could put similar code in Almanah. It’s too simple to need a dependency > on libgnome-desktop to be added to Almanah. Yes, good point. Thanks! Looks a a really simple way to control timezone changes. In other way, I think that the e_cal_util_get_system_timezone_location is really helpful and we don't need GTimeZone (and I don't know how to get the timezone string from a GTimeZone...)
(In reply to Álvaro Peña from comment #10) > In other way, I think that the e_cal_util_get_system_timezone_location is > really helpful and we don't need GTimeZone (and I don't know how to get the > timezone string from a GTimeZone...) Mmm, it does look like it’s impossible to construct an icaltimezone from a GTimeZone. Maybe you should work out a nice way to do this (probably by adding API to GTimeZone) and submit it as a GLib bug?
(In reply to Philip Withnall from comment #11) > (In reply to Álvaro Peña from comment #10) > > In other way, I think that the e_cal_util_get_system_timezone_location is > > really helpful and we don't need GTimeZone (and I don't know how to get the > > timezone string from a GTimeZone...) > > Mmm, it does look like it’s impossible to construct an icaltimezone from a > GTimeZone. Maybe you should work out a nice way to do this (probably by > adding API to GTimeZone) and submit it as a GLib bug? ... http://i.imgur.com/fvRXV.gif Just kidding, I'll open a bug for that.
Created attachment 316111 [details] [review] Getting timezone allways from system Now watching /etc/timezone for changes
Review of attachment 316111 [details] [review]: ::: src/event-factories/calendar-client.c @@ +233,3 @@ you should assume UTC. */ static gchar * +calendar_client_config_get_timezone () ‘(void)’ But you could just remove this method entirely and call e_cal_util_get_system_timezone_location() from its call sites instead. @@ +244,2 @@ static icaltimezone * +calendar_client_config_get_icaltimezone () ‘(void)’ @@ +260,2 @@ static void +calendar_client_set_timezone (CalendarClient *client) This line seems to have a spurious change in it. @@ +278,3 @@ + __attribute__ ((unused)) GFile *file, + __attribute__ ((unused)) GFile *other_file, + __attribute__ ((unused)) GFileMonitorEvent *event, For portability (and brevity) you can use G_GNUC_UNUSED instead of __attribute__((unused)). @@ +395,3 @@ + tz = g_file_new_for_path ("/etc/localtime"); + client->priv->tz_monitor = g_file_monitor_file (tz, G_FILE_MONITOR_NONE, NULL, NULL); You need to handle errors here. On error, (client->priv->tz_monitor == NULL), so the g_signal_connect() call below would fail.
(In reply to Philip Withnall from comment #14) > Review of attachment 316111 [details] [review] [review]: > > ::: src/event-factories/calendar-client.c > @@ +233,3 @@ > you should assume UTC. */ > static gchar * > +calendar_client_config_get_timezone () > > ‘(void)’ > > But you could just remove this method entirely and call > e_cal_util_get_system_timezone_location() from its call sites instead. > > @@ +244,2 @@ > static icaltimezone * > +calendar_client_config_get_icaltimezone () > > ‘(void)’ > > @@ +260,2 @@ > static void > +calendar_client_set_timezone (CalendarClient *client) > > This line seems to have a spurious change in it. > > @@ +278,3 @@ > + __attribute__ ((unused)) GFile > *file, > + __attribute__ ((unused)) GFile > *other_file, > + __attribute__ ((unused)) > GFileMonitorEvent *event, > > For portability (and brevity) you can use G_GNUC_UNUSED instead of > __attribute__((unused)). > > @@ +395,3 @@ > > + tz = g_file_new_for_path ("/etc/localtime"); > + client->priv->tz_monitor = g_file_monitor_file (tz, G_FILE_MONITOR_NONE, > NULL, NULL); > > You need to handle errors here. On error, (client->priv->tz_monitor == > NULL), so the g_signal_connect() call below would fail. Ok to all your comments. Thanks for taking the time of review my patches, I'll really appreciate it.
(In reply to Philip Withnall from comment #14) > Review of attachment 316111 [details] [review] [review]: > > @@ +260,2 @@ > static void > +calendar_client_set_timezone (CalendarClient *client) > > This line seems to have a spurious change in it. Just removing a trailing whitespace
Created attachment 316475 [details] [review] Getting timezone always from system
Review of attachment 316475 [details] [review]: ::: src/event-factories/calendar-client.c @@ +232,2 @@ static icaltimezone * +calendar_client_config_get_icaltimezone () This should be `calendar_client_config_get_icaltimezone (void)`.
Created attachment 316852 [details] [review] Getting timezone always from system Fixed Philip suggestions. Commited.
All commited