GNOME Bugzilla – Bug 651541
Add e_book_get_vcards() to avoid parsing
Last modified: 2013-09-14 16:54:12 UTC
Created attachment 188928 [details] [review] Proposed patch Hi, I'm currently working on the QtContacts backend for EDS part of MeeGo. The attached patch adds 2 functions (asynchronous and synchronous) to retrieve vCards in an EBook that match a given query. These are optimized versions of their get_contacts equivalents that allow the client to avoid (or delay) vCard parsing.
This patch is complementary to the one attached to https://bugzilla.gnome.org/show_bug.cgi?id=651446 although there is no inter-dependency.
I'm marking this as a duplicate of the other bug, it will be better to deal with it together. *** This bug has been marked as a duplicate of bug 651446 ***
This patch adds e_book_get_vcards() and e_book_get_vcards_async(). The patch in the duplicate added e_book_get_contact_uids() and e_book_get_contact_uids_async(). Are the *_vcard functions still necessary for you? If so, this bug shouldn't be closed.
Created attachment 189533 [details] [review] Proposed patch Well, yes, we need both. But they are not acceptable upstream for gnome-2-32 because they change the API. Anyway, we do have these patches applied to our evolution-data-server package in MeeGo. I'm attaching a more recent version of this patch that adds also the *_get_vcard*() functions (additionally to *get_vcards()).
So why not propose it for master?
Why not indeed :)
Reopening then.
(In reply to comment #5) > So why not propose it for master? Because EBook is deprecated in master, the patch should be in EBookClient, or, maybe, as a virtual method of EClient, something like e_client_get_object_string(), e_client_get_object_list_string(), though that would be too much probably. There is, right now, e_book_client_get_contacts_uids (..., const gchar *sexp, ...); added for similar reasons, as mentioned above (in the duplicate bug). Having similar functions available in both calendar and address book makes sense to me, thus the idea of pushing this into EClient itself, though that would make documentation pretty ugly, for the differences between calendar and address book. Then what about: gboolean e_book_client_get_contacts_uids_sync ( EBookClient *client, const gchar *sexp, GSList **contacts_uids, GCancellable *cancellable, GError **error); gboolean e_book_client_get_contacts_raw_sync ( EBookClient *client, const gchar *sexp, gchar ***vcards, GCancellable *cancellable, GError **error); gboolean e_book_client_get_contact_raw_sync ( EBookClient *client, const gchar *uid, gchar **vcard, GCancellable *cancellable, GError **error); gboolean e_book_client_contact_exists_sync ( EBookClient *client, const gchar *uid, gboolean *exists, GCancellable *cancellable, GError **error); and, of course, their calendar equivalents, which will be basically same, only uid will consist of two strings (parameters), uid + rid, and the e_cal_client_get_objects_uids_sync() 's uids GSList will not contain strings, but ECalComponentId structures.
What about the asynchronous equivalents of e_book_client_get_contacts_raw_sync() and e_book_client_get_contact_raw_sync()?
(In reply to comment #9) > What about the asynchronous equivalents of > e_book_client_get_contacts_raw_sync() and e_book_client_get_contact_raw_sync()? Of course, that's why these have _sync suffix. I chose showing only _sync versions because these get an overview of input and output parameters and because they do not eat so much space.
> Then what about: > > gboolean e_book_client_get_contacts_uids_sync ( > EBookClient *client, const gchar *sexp, GSList **contacts_uids, > GCancellable *cancellable, GError **error); > > gboolean e_book_client_get_contacts_raw_sync ( > EBookClient *client, const gchar *sexp, gchar ***vcards, > GCancellable *cancellable, GError **error); > > gboolean e_book_client_get_contact_raw_sync ( > EBookClient *client, const gchar *uid, gchar **vcard, > GCancellable *cancellable, GError **error); > > gboolean e_book_client_contact_exists_sync ( > EBookClient *client, const gchar *uid, gboolean *exists, > GCancellable *cancellable, GError **error); Before we continue to add special purpose API functions, can we take a step back and consider the use cases? I know we started all of this by looking for ways of making QtContacts-EDS more efficient, but this is now taking a whole life of its own. For example, where did this existence check come from and who needs it? Remember, EDS does not support locking of the database. So any check at time A for existence of contact FOO is not guarantee that at time B the contact still doesn't exist. IMHO adding new APIs which only work under the assumption that EDS only has one client should be avoided. Listing all current UIDs has the same problem. A much better approach is to do it via EBookView. Currently we focus on implementing the "required fields = UID" approach to retrieve a list of UIDs. It is not quite as efficient as a dedicated API because the UIDs have to be wrapped in a vCard. That drawback could be overcome by adding flags to EBookView which control not only what data is included in the vCard ("vcard with UID"), but also the data format ("UID string instead of vCard"). See bug #652172.
Is the currently proposed patch already applied in 'meego-eds' ? Is someone still working on a patch for master ? (In reply to comment #11) > > Then what about: > > > > gboolean e_book_client_get_contacts_uids_sync ( > > EBookClient *client, const gchar *sexp, GSList **contacts_uids, > > GCancellable *cancellable, GError **error); [...] > Listing all current UIDs has the same problem. A much better approach is to do > it via EBookView. Currently we focus on implementing the "required fields = > UID" approach to retrieve a list of UIDs. It is not quite as efficient as a > dedicated API because the UIDs have to be wrapped in a vCard. That drawback > could be overcome by adding flags to EBookView which control not only what data > is included in the vCard ("vcard with UID"), but also the data format ("UID > string instead of vCard"). See bug #652172. Currently the EBookView is going to call e_contact_new_from_vcard() for every vcard that it sends notifications about. We could: a.) Create new signal(s) on EBookView that notify with vcard lists instead of EContacts b.) Add a setting to the EBookView to notify with the new "raw" signals instead, the default would be to notify with EContacts and not break any api this way. Actually, we could hijack the same "contacts-added/modified" signals and just place vcards in the lists directly depending on the setting, even (I suppose using a separate/new signal is a stronger/safer api though). This way we can avoid calling e_contact_new_from_vcard() for every contact if the view (user/caller) prefers vcards to EContacts. Should this be done ?
(In reply to comment #12) > Is the currently proposed patch already applied in 'meego-eds' ? It was applied but then reverted, as "no longer needed now that we have lazy vCard parsing.", presumably via the patch in bug 652173 : http://git.gnome.org/browse/evolution-data-server/log/?h=meego-eds&qt=grep&q=Add+methods+to+fetch+vCards > Is someone still working on a patch for master ? I think you can assume not. But as far as I can tell, neither change is needed in master. Does anyone disagree?
This is no longer needed since we have lazy vCard parsing. Closing.