For this bug we should be able to take a similar approach as with
libebook on bug 652172 (and bug 652179).

libecal master allows gives us the api:

void
e_cal_client_view_set_fields_of_interest (ECalClientView *view, 
                                          const GSList *fields_of_interest,
                                          GError **error);

With this the client should be allowed to specify that only UID,
RECURRENCE-ID and LAST-MODIFIED fields are required.

However I'm not sure how the backend is supposed to handle the
fields-of-interest in libecal yet... in libebook there is
e-book-backend-sqlite.c, libecal has e-cal-backend-cache.c which
seems to be used by a couple backends and might be the right
place to implement the added search functionality that the
sqlitedb cache offers:

GSList *
e_book_backend_sqlitedb_search (EBookBackendSqliteDB *ebsdb,
                                const gchar *folderid,
                                const gchar *sexp,
                                GSList *fields_of_interest,
                                GError **error);

libical has at least some traces of a mysql storage backend, see
icalset_new_mysql(). I have no idea how complete it is - probably not
much.

Instead of trying to implement a sqlite cache on top of a
traditional .ics file, I would find it more interesting to move a
complete VCALENDAR into a sqlite file.

But this is outside the scope of the current work, unless you like a challenge.
What I'd like to see is just the API and a rudimentary implementation based on the
current in-memory copy of the calendar file.

Regarding e_cal_client_view_set_fields_of_interest(). Note that the UID / RID are *always* returned, according to the doc. Therefore, only other interesting fields need to be explicitly passed to e_cal_client_view_set_fields_of_interest() and NULL indicates that we want only UID / RID.

Also note that I could not find a way to indicate that LAST-MODIFIED is an interesting field. I cannot find any mapping from LAST-MODIFIED to an ECalComponentField.

Christophe asked me to write here few comments, thus here they are:
The ECalBackendCache is supposed to be replaced with ECalBackendStore, though it's not finished yet, same as the EDataCalView provides only API for fields-of-interest, but it's not used anywhere yet. It's all under to-be-done task.
Main issue with calendar part is cache, as Tristan and Patrick mentioned above, because it has no summary, it's only "all-components-in-memory", thus there is nothing to save, as was doable in book with the SQLite summary. The only thing to save would be traffic over DBus, for a price of higher CPU usage while "cleaning" components for fields-of-interest. All that is due to different approaches used in addressbook and calendar factories.

Thus, if you think the DBus bandwidth cut worths it for the higher CPU usage, then the EDataCalView can have function which will "sanitize" components used to notifications of 'added' and 'changed' signals, to strip fields which are not required, before calls of
   e_data_cal_view_notify_objects_added*
   e_data_cal_view_notify_objects_modified*
. I thought of using it internally only, but as the above mentioned functions have list of strings, then it would be too much for parsing back to component, then remove unneeded fields and then make it string again.

I'm thinking of a function like this:
   gchar *e_data_cal_view_strip_icalcomp (EDataCalView *view, /* const */ icalcomponent *icalcomp) {
       if (!view->priv->fields_of_interest)
          return icalcomponent_as_ical_string_r (icalcomp);

       create artificial component from icalcomp based
       on fields_of_interest and return it;
       The icalcomp should be kept as is.
   }

This change will result in a traversal API change, because all the e_cal_backend_ is using objects as strings, and parses components forts and back as needed, which is not very practical, thus also functions like e_cal_backend_notify_object_* would have the API changed, to provides objects where possible - it's subject to check whether it's possible to provide old objects as objects always.

(In reply to comment #3)
> Regarding e_cal_client_view_set_fields_of_interest(). Note that the UID / RID
> are *always* returned, according to the doc. Therefore, only other interesting
> fields need to be explicitly passed to
> e_cal_client_view_set_fields_of_interest() and NULL indicates that we want only
> UID / RID.

The NULL case isn't TRUE, from my point of view, you cannot pass NULL lists over the bus, it's forbidden by eds, thus if you would need only UID and RID, then use one of them as a field of interest. Note that these are default fields only because they are mandatory. Some other mandatory fields would be recurrence generation related fields.

> Also note that I could not find a way to indicate that LAST-MODIFIED is an
> interesting field. I cannot find any mapping from LAST-MODIFIED to an
> ECalComponentField.

As we spoke on IRC, this list is used for ETable only (as is written in the comment above the enum definition), and, from my point of view, should not be part of e-cal-component.c/.h at all.

>The NULL case isn't TRUE, from my point of view, you cannot pass NULL lists
> over the bus, it's forbidden by eds, thus if you would need only UID and RID,
> then use one of them as a field of interest. Note that these are default fields
> only because they are mandatory. Some other mandatory fields would be
> recurrence generation related fields.

I interpreted that from the doc (?):
"""The UID/RID fields are returned always. Initial views has no fields of interest and using NULL for fields_of_interest will unset any previous changes."

Also, tests/libecal/client/test-client-get-view.c passes NULL to e_cal_client_view_set_fields_of_interest().

(In reply to comment #4)
> Note that these are default fields
> only because they are mandatory. Some other mandatory fields would be
> recurrence generation related fields.

If a client asks for a certain set of fields, it should get exactly those. We should not add more just because the iCalendar or vCard standard happen to define them as mandatory.

These fields are mandatory so that the recipient has something that he can rely on. But when that recipient explicitly asks not to get them (as SyncEvolution would do when asking for contacts with only the UID set), then the sender should not needlessly send more.

(In reply to comment #5)
> I interpreted that from the doc (?):
> """The UID/RID fields are returned always. Initial views has no fields of
> interest and using NULL for fields_of_interest will unset any previous
> changes."
> 
> Also, tests/libecal/client/test-client-get-view.c passes NULL to
> e_cal_client_view_set_fields_of_interest().

Ah, right, e_cal_client_view is taking care of the NULL argument, and passes an empty array to the factory, which unsets fields-of-interest, if previously set, thus the view will return complete components again.

(In reply to comment #6)
> If a client asks for a certain set of fields, it should get exactly those. We
> should not add more just because the iCalendar or vCard standard happen to
> define them as mandatory.

OK, I agree, as long as caller counts with those limitations, then it might be all fine.

Created attachment 194067 [details] [review]
Patch to implement 'fields-of-interest' for libecal

This huge patch implements fields-of-interest for libecal.

Note this patch will probably not apply directly to master without
the patch for bug 652177 being applied first (as of course by now
there is a backlog of commits on the branch, e_cal_get_revision()
is the only other commit touching the calendar directory).

Some comments on the implementation:

   - Instead of changing all the apis mostly I added new
     api variants that take ECalComponent instead of the ical string.

     This should be easier to integrate for now and later the
     old variants can be more removed in further patches.

   - I did break the ECalBackendSync apis regarding creating,
     modifying and removing calendar components (so that they
     return a reference to an ECalComponent instead of an allocated
     ical string)... as well as the respective e_data_cal_respond_*()
     apis also to take ECalComponents instead of ical strings.

     The patch includes some updates to the other backends so that
     everything in eds repo still compiles (and presumably works too).

   - The function e_data_cal_view_get_component_string() which does
     the actual filtering (i.e. this function "gets a string for a
     component that is suitable for the listeners of @view")... might
     be wrong and it might not be fixable without modifications to
     libical itself.

     Here is the problem:

     The function icalcomponent_as_ical_string_r () does this little
     magic trick when serializing the component:

     if (kind != ICAL_X_COMPONENT) {
       kind_string  = icalcomponent_kind_to_string(kind);
     } else {
       kind_string = impl->x_name;
     }

     The 'kind_string' in this context is what ends up in the component
     header like so  "BEGIN:%s\r\n"

     However our code in libecal afaics has no access to 'impl->x_name',
     from reading the ical code the ->x_name is only ever assigned when
     icalcomponent_new_x() is called and is then only used internally.

     Without access to this ->x_name I can't reliably say that we're
     capable at all of generating filtered ical strings.

   - This patch still does not include a test case, I have a test case
     here and I've found that the text 'uid' indeed works for filtering
     the objects down to the UID, however I still don't know the property
     key names for the RID or LAST-MODIFICATION fields of the ical components.

     Any tips here will help me to finish up the test case more quickly...

The related commit is:

commit 457cb46dcd8b4213eca047939f2ff7017709569c
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Wed Aug 17 18:58:08 2011 +0200

    Really implement e_cal_client_view_set_fields_of_interest().
    
    As discussed on bug https://bugzilla.gnome.org/show_bug.cgi?id=652180,
    this patch adds many '_component' variants to functions that are named
    with '_object', all the '_component' variants take an ECalComponent object
    instead of an ical string.
    
    The magic filtering happens in e_data_cal_view_get_component_string()
    which creates an ical string representation while omitting properties
    that are not mentioned in the fields of interest.

Thanks for the patch. The description of changes (I didn't open/test the patch yet) sounds reasonable, and I was also thinking that there will be no other way of doing that, than changing the API. Note that some functions returning icalcomponent can return VCALENDAR, which ECalComponent refuses to hold inside (the setter function returns FALSE for these cases). VCALENDAR components can be returned for events with detached instances. The thing is that I'm not 100% sure whether it matters here or not.

(In reply to comment #8)
>    - This patch still does not include a test case, I have a test case
>      here and I've found that the text 'uid' indeed works for filtering
>      the objects down to the UID, however I still don't know the property
>      key names for the RID or LAST-MODIFICATION fields of the ical components.
> 
>      Any tips here will help me to finish up the test case more quickly...

Checkout libical sources (from SourceForge) and see src/libical/icalderivedproperty.c, where is an array of icalproperty_kind and their string representation, same as functions operating with the array, like icalproperty_kind_to_string(), icalproperty_string_to_kind().

Unfortunately, the master is under API freeze, thus this will come to 3.3.1 in the earlier. We missed the date. I'll try to not forget of this after the branch of eds for 3.2.0.

(In reply to comment #9)
[...]
> Checkout libical sources (from SourceForge) and see
> src/libical/icalderivedproperty.c, where is an array of icalproperty_kind and
> their string representation, same as functions operating with the array, like
> icalproperty_kind_to_string(), icalproperty_string_to_kind().

Thanks that was exactly what I needed to complete the test case.

Happily after modifying the test case, its passing as expected
(without any further iteration on the already attached patch).

I've rebased 'openismus-work-master' branch against master today
as I regularly do and added the test case with this commit:

commit b46dccbf4f87a3cf729c9db5696187e1ad8fdcf4
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Thu Aug 18 19:41:43 2011 +0200

    Added test to show that fields-of-interest is working in libecal
    
    Test is based on test-client-get-view.c, this version of the test
    sets fields-of-interest to UID, RECURRENCE-ID and LAST-MODIFIED
    fields, tests that all those fields are returned and asserts that
    the event summary (which is indeed set) is always NULL.

I'm sorry for a delay, there seemed to be more prioritized tasks after your update. I'm briefly reading through your patch and I'm wondering about things like this:
+	gchar *str = icalcomponent_as_ical_string_r (master);
+
+	*new_component = e_cal_component_new_from_string (str);
+
+	g_free (str);

I know it can have its advantages to encode and parse, but what about:

	icalclone = icalcomponent_clone (master);
	*new_component = e_cal_component_new ();
	if (!e_cal_component_set_icalcomponent (*new_component,	icalclone)) {
		icalcomponent_free (icalclone);
		g_object_unref (*new_component);
		*new_component = NULL;
	}

Seeing how many times it's used in the code, it should be a utility function for the ECalBackend for sure, with all the nice error checking. Say it'll take the icalcomonent as an argument and will return ECalComponent as a result, NULL if something will go wrong. That way the file size will be reduced too.

Oh, wait, I'm not sure with the ECalComponent being chosen as the replacement for strings. It works for most cases, no doubt, but what if you are notifying about change in an object which is not only a master, but also master with detached instances? In that case the icalcomponent is a VCALENDAR (not VEVENT) and contains both master object and its detached instances. ECalComponent cannot hold VCALENDAR.

Take care of empty lines too, please, because this is a little typo I spot:
@@ -718,12 +775,240 @@ e_data_cal_view_get_fields_of_interest (EDataCalView *view)
 	return view->priv->fields_of_interest;
 }
 
+
+
+static gboolean
+filter_component (icalcomponent *icomponent, GHashTable *fields_of_interest, GString *string)

I didn't compile the patch yet, neither read it till the end, I expect it being done against current master, with some changes. Of course, it would be good to provide necessary API changes for evolution-exchange, evolution-groupwise, evolution-mapi and evolution-couchdb too, once we decide on actual API changes (the configure.ac increment on API version for libedatacal would be good too, if not increased earlier for the respective version - it's already changed for 3.3.1). It will be good to have this in early for 3.3.x, thus others will have time to adapt to API changes being done here.

(In reply to comment #11)
[...]
> 
> Oh, wait, I'm not sure with the ECalComponent being chosen as the replacement
> for strings. It works for most cases, no doubt, but what if you are notifying
> about change in an object which is not only a master, but also master with
> detached instances? In that case the icalcomponent is a VCALENDAR (not VEVENT)
> and contains both master object and its detached instances. ECalComponent
> cannot hold VCALENDAR.

Honestly it took me a while to sink my head back into libecal and
get a grasp on this.

So, we changed the passing around of strings to ECalComponents because
it dramatically reduces the amount of parsing involved.

We can't use ECalComponents because they only represent a subset
of the data we were previously sending with the ical strings
(i.e. a VCALENDAR component with detached instances).

While it seems a little gross to let the ical types bleed through 
the backend apis, I can't think of a better solution than to just
redo the same patch but instead of passing around an 'ECalComponent *'
throughout the backend, we pass an 'icalcomponent *' instead.

This way backends can notify with the icalcomponent directly and
we still wait to the last possible moment to encode them into
strings for notifications.

The added function e_data_cal_view_get_component_string() which
does the magick filtering only needs an 'icalcomponent *' anyway.

Should have a patch soon...

Created attachment 199068 [details] [review]
Patch to implement 'fields-of-interest' for libecal [ take 2 ]

Ok, it seems that things work well with icalcomponent * pointers passed around
instead of ECalComponents or strings.

Things would honestly be better if an object could be returned instead
of an icalcomponent * (i.e., perhaps if ECalComponent were made to 
support the VCALENDAR for this case)... because then the backend has
the choice to clone or to only return a reference to a component who's
state will persist until the end of the operation (the 'old_component'
in ->modify_object() must be cloned or else it would be the same
object as the 'new_object' in notifications).

Well, the patch is pretty much the same as the last one except for
the main detail of using icalcomponent * everywhere.

The test (incase I haven't mentioned before) is
   tests/libecal/client/test-client-revision-view.c
(on openismus-work-master branch)

The new commit for this patch on the 'openismus-work-master' branch is:

commit 400c8e1e9d0d82ee32db58ceed8747e123a4564c
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Sat Oct 15 11:34:32 2011 -0400

    Really implement e_cal_client_view_set_fields_of_interest()....

Clone versus reference: I prefer the clone, as it is the component state when the function was called. Being it a reference for an internal component then it could be modified before the notification to client would be done - similar like your comment #13 notice about necessary clone in modify_object(); that is why to clone than take a reference.

I see that you use the ecal_comp_from_icalcomp() in each of the e_cal_backend_notify_component_*() functions. I tried to reproduce the issue I would expect (like the new_component being a VCALENDAR instead of VEVENT), but I was unable to from evolution's UI. There is not shown the runtime warning from ecal_comp_from_icalcomp() for me. It might mean that I'm either wrong with the detached instances notifications, or I didn't do the right changes in evolution. I'll be very sorry, if I would mistaken you with this.

----------------------------------------------------------------------------

I'm not sure why you changed it, but the first chunk in 
   calendar/backends/caldav/e-cal-backend-caldav.c
produces a compiler warning for me:
   e-cal-backend-caldav.c: In function 'convert_to_inline_attachment':
   e-cal-backend-caldav.c:3045:4: warning: pointer targets in passing argument
     1 of 'icalattach_new_from_data' differ in signedness [-Wpointer-sign]
   /usr/include/libical/ical.h:949:13: note: expected 'const char *' but
     argument is of type 'guchar *'

It will be good if you'll be consistent with code indentation, like in do_remove_object() in caldav you write one long line, but few lines above, in do_create_object() you break the statement in multiple lines quite quickly. Let's use the "one argument on one line" for function prototypes, like you did in do_modify_object().

In process_object() you are calling g_object_unref() on an icalcomponent, which is wrong, the right function is icalcomponent_free(). When you run e-calednar-factory from a console you should see many warnings about a pointer not being a GObject when running your test program.

There left a debug message:
   g_message ("Removing an instance '%s'", uid);

In e_cal_backend_file_receive_objects() I see that remove_instance() can set both old_component and component, but you reset the component variable always to the icalcomp from comp, the the previously set component can leak.

At the end of that chunk (@@ -3166,40 +3260,49 @@ e_cal_backend_file_receive_objects) you call e_cal_backend_notify_component_created() with clone & free on component. It's not needed, because e_cal_backend_notify_component_created() creates its own clone anyway. I would define the e_cal_backend_notify_component_created() with const 'component' to indicate it. Basically for each notify function, thus the memory management will be clear. It may save a bit of cloning too.

Would it make any sense to mimic what e_cal_component_get_id() does in notify_add_component(), instead of creating another clone of the icalcomp only for this uid+rid structure? I see you've there an XXX comment on one place there.

You do not check for non-null icalcomponent before callign icalcomponet_free in cal_backend_create_object(), but you do test it in the other two function below. Let's test it here too.

in filter_component(), does the icalproperty_as_ical_string_r() return property with the CRLF included? I guess it is, as you are not using the newline variable there.

This might be a typo:
- * Since: 2.24

The comment at e_cal_backend_notify_component_removed() has there
+ * Like e_cal_backend_notify_objects_added() ...

You've couple new lines before filter_component() function, which might not be there.

It looks pretty well, just fix the above things and I'll do a final review before the commit to master. Thanks for the work on this.

Created attachment 199274 [details] [review]
Patch to implement 'fields-of-interest' for libecal [ take 3 ]

Thanks for the prompt and very detailed review :)

Here's a new patch with these issues addressed.

(In reply to comment #14)
> Clone versus reference: I prefer the clone, as it is the component state when
> the function was called. Being it a reference for an internal component then it
> could be modified before the notification to client would be done - similar
> like your comment #13 notice about necessary clone in modify_object(); that is
> why to clone than take a reference.
> 
> I see that you use the ecal_comp_from_icalcomp() in each of the
> e_cal_backend_notify_component_*() functions. I tried to reproduce the issue I
> would expect (like the new_component being a VCALENDAR instead of VEVENT), but
> I was unable to from evolution's UI. There is not shown the runtime warning
> from ecal_comp_from_icalcomp() for me. It might mean that I'm either wrong with
> the detached instances notifications, or I didn't do the right changes in
> evolution. I'll be very sorry, if I would mistaken you with this.
> 
> ----------------------------------------------------------------------------
> 
> I'm not sure why you changed it, but the first chunk in 
>    calendar/backends/caldav/e-cal-backend-caldav.c
> produces a compiler warning for me:
>    e-cal-backend-caldav.c: In function 'convert_to_inline_attachment':
>    e-cal-backend-caldav.c:3045:4: warning: pointer targets in passing argument
>      1 of 'icalattach_new_from_data' differ in signedness [-Wpointer-sign]
>    /usr/include/libical/ical.h:949:13: note: expected 'const char *' but
>      argument is of type 'guchar *'

Interesting, I have libical 0.43 (I guess jhbuild gave me that some time
early this summer)... and changing that line shut up the compiler warning
for me... I'm assuming it's a change in libical... (anyway reverted that
bit out of the patch).

> 
> It will be good if you'll be consistent with code indentation, like in
> do_remove_object() in caldav you write one long line, but few lines above, in
> do_create_object() you break the statement in multiple lines quite quickly.
> Let's use the "one argument on one line" for function prototypes, like you did
> in do_modify_object().

Gotcha, I prefer this style too as I dont like using small fonts... 

> 
> In process_object() you are calling g_object_unref() on an icalcomponent, which
> is wrong, the right function is icalcomponent_free(). When you run
> e-calednar-factory from a console you should see many warnings about a pointer
> not being a GObject when running your test program.

Eeek, this stuff leaked in obviously from the last patch which was
using ECalComponent... I tried to catch as much of these as possible
but I must have left out the caldav backend (I did not get the luxury
of warnings... I'm not sure how to run the caldav backend ;-)).

> There left a debug message:
>    g_message ("Removing an instance '%s'", uid);
> 
> In e_cal_backend_file_receive_objects() I see that remove_instance() can set
> both old_component and component, but you reset the component variable always
> to the icalcomp from comp, the the previously set component can leak.

Fixed.

> At the end of that chunk (@@ -3166,40 +3260,49 @@
> e_cal_backend_file_receive_objects) you call
> e_cal_backend_notify_component_created() with clone & free on component. It's
> not needed, because e_cal_backend_notify_component_created() creates its own
> clone anyway. I would define the e_cal_backend_notify_component_created() with
> const 'component' to indicate it. Basically for each notify function, thus the
> memory management will be clear. It may save a bit of cloning too.

Gotcha, changed all the added notification apis in ECalBackend and
EDataCalView to this effect (and removed the unneeded clone).

> Would it make any sense to mimic what e_cal_component_get_id() does in
> notify_add_component(), instead of creating another clone of the icalcomp only
> for this uid+rid structure? I see you've there an XXX comment on one place
> there.

Yes I believe that would make sense... that might take you only 2 minutes
to fix... personally I haven't done so because I'm a little confused
as to how to do resolve the ECalComponent's 'priv->uid' (which is used
in e_cal_component_get_id())... previously the string had to be deserialized
to get the id... now this of course should not be needed.

Actually there were 2 XXX comments, the other one was regarding
a notification that was still being made by string (I changed that
in this patch to avoid that... in e_cal_backend_empty_cache()).

> 
> You do not check for non-null icalcomponent before callign icalcomponet_free in
> cal_backend_create_object(), but you do test it in the other two function
> below. Let's test it here too.
> 
> in filter_component(), does the icalproperty_as_ical_string_r() return property
> with the CRLF included? I guess it is, as you are not using the newline
> variable there.

The code in filter_component() is derived directly from
icalcomponent_as_ical_string_r(), plus the added filtering checks
and minus the code which I left in comments which special cases
the ICAL_X_COMPONENT (this special case is not possible from outside
libical... for reference, I brought this up in comment 8 above).


> This might be a typo:
> - * Since: 2.24

Indeed.

> 
> The comment at e_cal_backend_notify_component_removed() has there
> + * Like e_cal_backend_notify_objects_added() ...
> 
> You've couple new lines before filter_component() function, which might not be
> there.
> 
> It looks pretty well, just fix the above things and I'll do a final review
> before the commit to master. Thanks for the work on this.

New commit for this on 'openismus-work-master' branch is:

commit 09bf28a70128d0256f756491906b2d11beb7622d
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Sat Oct 15 11:34:32 2011 -0400

(In reply to comment #15)
> Yes I believe that would make sense... that might take you only 2 minutes
> to fix... personally I haven't done so because I'm a little confused
> as to how to do resolve the ECalComponent's 'priv->uid' (which is used
> in e_cal_component_get_id())... previously the string had to be deserialized
> to get the id... now this of course should not be needed.

It's icalcomponent_get_uid(), where this one can come from.

Jus the final call, as I see the patch is ready to hit master, please remove this change:

+ * @object: Object to match..

as it seems like a typo and add a "Since: 3.4" to each new public function comments, like it is with 3.2 or any previous version, thus it'll be clear when the function was added to sources.

You can change the uid+rid reading from the icalcomponent for bonus points too :)
but I'll keep it up to you. Just make the final changes and commit to master please. Thanks fr helping with this.

Ok I finally pushed this.

It seems somehow very recently in master the required version
of several dependencies was bumped... resulting in a long and
arduous 'jhbuild build evolution-data-server' on my side.

I didn't make it as far as creating the ECalComponentId from the
icalcomponent, while I think the recipe for the ECalComponentId
will be:
   'icalproperty_get_uid (icalcomponent_get_uid ())'
to get the ->uid part... replicating the inside of
e_cal_component_get_recurid_as_string() was going to be
another challenge...

I filled bug #664572 as a follow-up on my faulty decision on using icalcomponent instead of ECalComponent in new API functions.