GNOME Bugzilla – Bug 671177
Port calendar-server to ECalClient
Last modified: 2012-03-10 16:34:56 UTC
EClient is deprecated, and in our quest to perfection we should use the new shiny ECalClient API. Well, it's not only that, actually. Using the old API, calendar backends are never opened (even though e_cal_open is called and succeeds), thus all operations on them fail with NotOpened (webcal) or return cached data only (caldav, weather). dbus-monitor tracking shows AuthenticationRequired signals that are never handled. OTOH, with the new API, and the nice e_client_utils_authenticate_helper that implements everything auth related, calendars are correctly opened and show updated data, without ever opening evolution.
Created attachment 208835 [details] [review] calendar-server: update to ECalClient ECal is deprecated and replaced by ECalClient. This has the advantage of using e_utils to handle authentication, and should fix NotOpened errors (that affect in particular webcal calendars prior to evolution running)
Review of attachment 208835 [details] [review]: ::: src/calendar-server/gnome-shell-calendar-server.c @@ +632,2 @@ { + g_warning ("Error opening calendar %s: %s\n", Warning: Error opening calendar... Don't do that. Is it an error or a warning? Even so, this isn't related to the ECalClient port, it doesn't belong here. Put it in a separate commit.
(In reply to comment #2) > Review of attachment 208835 [details] [review]: > > ::: src/calendar-server/gnome-shell-calendar-server.c > @@ +632,2 @@ > { > + g_warning ("Error opening calendar %s: %s\n", > > Warning: Error opening calendar... > > Don't do that. Is it an error or a warning? Even so, this isn't related to the > ECalClient port, it doesn't belong here. Put it in a separate commit. It's both. It's an error, in the sense that it's something that went wrong (a dbus error, most likely), and it's a warning, in the sense of something that starts with "(gnome-shell-calendar-server:<pid>)" (so it's picked up by abrt and the like) and it's not hidden by the default log handler. Ok for the separate commit, though.
Created attachment 209031 [details] [review] calendar-server: update to ECalClient ECal is deprecated and replaced by ECalClient. This has the advantage of using e_utils to handle authentication, and should fix NotOpened errors (that affect in particular webcal calendars prior to evolution running)
Created attachment 209032 [details] [review] calendar-server: use g_warning instead of g_printerr This way error messages include the process name and PID, so they're picked up by bug reporting tools that grep ~/.xsession-errors.
Review of attachment 209032 [details] [review]: If it's an error, we want g_critical, not g_warning, right?
(In reply to comment #6) > Review of attachment 209032 [details] [review]: > > If it's an error, we want g_critical, not g_warning, right? I wouldn't say so - g_critical() is fairly much only used for g_return_if_fail() - g_warning() is anything that shouldn't happen but isn't worth aborting for immediately.
I've asked the evolution guys to look this over, too.
ECalClientViewClass::objects_added/_modified/_removed uses GSList, not GList. Of course, using async functions, especially if this is run in the main thread, is preferred. Apart of that I do not see anything obvious - aka it looks good otherwise. About the g_critical, I agree with Owen, you definitely do not want to abort gnome-shell's calendar server due to issues like user not writing correct password for three times, or if the remote server is temporarily unavailable, which may result in an error on any of those three changed places (it can get temporarily unavailable after user connected to it already too). Of course, these issues can happen, even pretty often, when you connect to your calendar hidden in VPN where you are not connected currently.
Created attachment 209216 [details] [review] calendar-server: update to ECalClient ECal is deprecated and replaced by ECalClient. This has the advantage of using e_utils to handle authentication, and should fix NotOpened errors (that affect in particular webcal calendars prior to evolution running) Updated for GSList instead of GList
One last note: I kept the existing sync functions, since technically they're called in the main thread, but really the helper acts like a worker thread for gnome-shell, and thus blocking it is fine.
The patch looks good from my side, just check compiler warnings, which I suppose you did already. This is mainly about "simple" replacements. I do not know the gnome-shell-calendar code, but I guess, with this change, you also do not want to include e-passwords.h anymore, as this is covered by the e-client-utils' authenticate function.
Attachment 209032 [details] pushed as 0ea690a - calendar-server: use g_warning instead of g_printerr Attachment 209216 [details] pushed as d68ff69 - calendar-server: update to ECalClient Pushed including the e-passwords.h fix.