GNOME Bugzilla – Bug 651446
Implement e_book_client_get_contacts_uids()
Last modified: 2011-06-01 17:42:21 UTC
Created attachment 188868 [details] [review] Proposed patch Hi, I'm currently working on the QtContacts backend for EDS, part of MeeGo. For performance reasons, I adapted a patch from eds-fremantle GIT repository that never got merged upstream: """ The client can now tell the book_view to send unparsed vCards in its signals. The unparsed vCards are emitted as NULL terminated array of strings. The receiver is responsible for parsing them. This optimization avoids uneeded vCard parsing in the case where the client is not interested in it. This patch is based on commit 62aad491561fd70ca254f2eafda30fd8d2985434 from eds-fremantle GIT repository. """
Review of attachment 188868 [details] [review]: Looks good. Please commit the patch to master.
Any chance to get this in gnome-3-0 branch too?
Chen's call, but we generally don't add new APIs on stable branches.
Rather not to the stable branches, as it's under API freeze, and this one is changing API, even rather transparently (it adds new function, which influences signal handler prototype). I consider this a problem, because signals are defined as > void (* contacts_changed) (EBookView *book_view, const GList *contacts); > void (* contacts_added) (EBookView *book_view, const GList *contacts); but you are passing in "const gchar * const *". It's no problem for the marshallers, but it's really quite hidden. I wouldn't use this change as is, I would rather see much cleaner solution. Most significantly this is surprising: > This optimization avoids uneeded vCard parsing in the case where the > client is not interested in it. You told me on IRC that you are looking for UIDs only, thus why not having there some API to fetch only that value, even not encoded as a vCard? The current git master has e_book_client_view_set_fields_of_interest() which is supposed to work in 3.2 and which will instruct backend to return only partial vCards (but it can also return complete vCards). There is no such equivalent for e_book_client_get_contacts(). What about adding a new function e_book_client_get_contact_uids(...const gchar *sexp, GSList **contact_uids, ...) which will do exactly what you want? And it can benefit from the future sqlite DB summary too.
*** Bug 651541 has been marked as a duplicate of this bug. ***
Created commit 6e1a5b1 in eds master (3.1.2+) This commit implements e_book_client_get_contacts_uids() and all around. It's still waiting for the sqlite DB backend summary for much efficient implementation, but otherwise this function is the right way to go. I'm not sure if you will need to backport it, because it's not about backporting in the real mean, it's rather about rewriting this, as this is highly dependant on the new API. But feel free to ask for more details, if you wish.
Created attachment 188972 [details] [review] Proposed patch for v2.32 I know this is not suitable for upstream but I would still appreciate if you could review my patch for EDS 2.32 in MeeGo. Note that I only patches 2 backends: file and vcf. I don't even think vcf is required in my case. Could you please confirm that?
I had a look at your patch also, it is way more exhaustive than mine obviously (all backends are patched) and you are using GSList rather than GList (probably more efficient?). Besides that I did not notice any major difference except: in e_book_backend_file_get_contact_list_uids() you might want to use free() instead of g_free() to free vcard_dbt.data since the memory is allocated by the database, not glib. It probably does not matter a lot but I thought I would mention it.
Created attachment 188982 [details] [review] Proposed patch for v2.32 (Tested) I tested the previous patch and I found I missed a few definitions at a few places. I fixed them and this new patch seems to work for me. Do I really need to update the xml file or is it generated? I see that you did not update any xml file in your commit so I was wondering.
(In reply to comment #8) > I had a look at your patch also, it is way more exhaustive than mine obviously > (all backends are patched) and you are using GSList rather than GList (probably > more efficient?). Besides that I did not notice any major difference except: It's just my preference in the new API, to use one of GList and GSList, as the old API (both in book and calendar) used both types, and I wanted to use only one of them. And I chose GSList. > in e_book_backend_file_get_contact_list_uids() you might want to use free() > instead of g_free() to free vcard_dbt.data since the memory is allocated by the > database, not glib. It probably does not matter a lot but I thought I would > mention it. Technically can be true, in both ways. I use g_free() only for its NULL/not-NULL checking. It might be the same as free() in all other aspects. (In reply to comment #9) > Do I really need to update the xml file or is it generated? I see that you did > not update any xml file in your commit so I was wondering. XML files are there only for cases when you generate DBus stubs from them, which we do not do anymore, thus I didn't change anything in them. I was thinking of dropping them too. With respect of the patch itself, I would not use g_assert() here. For a case when you call the function "unintentionally" on any other backend, then the e-addressbook-factory will immediately crash. it would be better to return an "unsupported" error instead (I think it is available in your version too). Otherwise looks good, especially if it works for you. And when you move to 3.2+, then you'll be moving to EBookClient anyway, and will rewrite relevant parts of your application.
Created attachment 188993 [details] [review] Proposed patch for v2.32 (Improved) Here is an updated version of my patch based on your feedback. I'm merely overwriting the patch attached to the bug report in case anyone is interested in it.
I suppose it's same as the previous patch, but with the fallback code in the e-book-backend-sync.c. I appreciate for your review and a notice of my fault calling incorrect function in git master, which I fixed after you told me. Thanks for that, it was my pretty silly mistake.