Re: [evolution-patches] weather calendars

[Rodrigo Moya <rodrigo novell com>]

On Thu, 2004-12-30 at 00:46 -0700, David Trowbridge wrote:
> Here's my shot at this. Some things to cogitate:
> 
> * Events only show up in the list view. I'm trying to debug this, and
> Rodrigo has suggested that it might be a buglet left over from the
> recurrences branch merge. I've verified that the backend is sending
> events properly for all the SExp queries made to it.
> 
> * Changing parameters (refresh, units preferences, etc) on the source
> doesn't take immediate effect. It appears as though these changes aren't
> communicated back to e-d-s.
> 
not even when connecting the backends to the "changed" signal in the
source?

> I hope to solve these two soon.
> 
> * The bounty specifies that the e-d-s backend should supply the category
> icons, but this seems overly complicated. I've added them to gal's
> master category list for now, but in my ideal vision of this, a
> rewritten calendar view system could allow the EPlugin to take care of
> drawing the event, in which case the category system could be completely
> uninvolved. However, such a fantastic plugin hook isn't necessary for
> this to be useful now.
> 
this is now solved, you can use the new e-categories API in
libedataserver to add new categories and their icons.

So, some comments:
> (e-d-s.diff)
>
this is missing the change to e-source.c, and also ChangeLog entries for
the other changes.

There are also some formatting issues, like the { on their own line.
They should be on the same line as the associated if/else/whatever
statement.

> e-cal-backend-weather.c:
>
> * Set category and visibility */
> 	e_cal_component_set_categories (cal_comp, getCategory (report));
> 	e_cal_component_set_classification (cal_comp, E_CAL_COMPONENT_CLASS_PRIVATE);
>
why PRIVATE? I guess they should be marked as PUBLIC.

> static ECalBackendSyncStatus
> e_cal_backend_weather_get_static_capabilities (ECalBackendSync *backend, EDataCal *cal, char **capabilities)
> {
> 	*capabilities = NULL;
> 
> 	return GNOME_Evolution_Calendar_Success;
> }
I guess here you should really return some capabilities, which tell clients what the
backend does and does not support. See the list in libecal/e-cal-util.h


> static ECalBackendSyncStatus
> e_cal_backend_weather_remove (ECalBackendSync *backend, EDataCal *cal)
> {
> 	return GNOME_Evolution_Calendar_PermissionDenied;
> }
the _remove method is called when the calendar is removed from the configuration, so
you should at least remove the cache, like the HTTP backend does.

> static icaltimezone *
> e_cal_backend_weather_internal_get_timezone (ECalBackend *backend, const char *tzid)
> {
> 	ECalBackendWeather *cbw = E_CAL_BACKEND_WEATHER (backend);
> 
> 	return cbw->priv->default_zone;
> }
here you should try to find the timezone for the tzid passed as argument.

The GAL part, since you can now add new categories from the API, please just avoid it.

So, it looks mainly ok, so please, send a new patch with all the requested changes
and I'll commit it to CVS.

cheers
-- 
Rodrigo Moya <rodrigo novell com>