GNOME Bugzilla – Bug 652173
libebook: Delay client-side vCard parsing
Last modified: 2018-12-12 11:52:49 UTC
The evolution-data-server client-side APIs currently parse and (temporarily) store the parsed details of VCards. However, this is often unnecessary because APIs such as e_book_get_book_view() cause it to return the whole VCard anyway. The VCard parsing is superfluous (and a performance problem) for the QtContact API when it uses an evolution-data-server backend, because QtContact does its own VCard parsing. We propose delaying VCard parsing until it is really needed by calls to functions such as e_contact_new_from_vcard(). The following calls would then transfer data without any additional parsing in libebook: * e_book_get_contact() (or asynchronous read/view) followed by e_vcard_to_string(). * e_contact_new_from_vcard() followed by e_book_add_contact() then e_book_commit_contact() (or asynchronous storing). Apparently libecal can already do this, though libebook can't yet. We plan to provide a patch for this, but please let us know if it sounds acceptable.
Created attachment 190385 [details] [review] Lazy vCard parsing patch proposal Here is my implementation proposal for this. It does not require any API change since everything happens internally in the e-vcard. The vCard is lazily parsed, meaning that it only gets parsed the first time that a field is accessed.
Created attachment 190408 [details] [review] Second patch to stop generating FILE_AS in e_contact_new_from_vcard() Unfortunately, this seems to be required. Does anyone have an idea how to fix it more nicely so that it becomes acceptable for upstream?
(In reply to comment #1) > Created an attachment (id=190385) [details] [review] > Lazy vCard parsing patch proposal > > Here is my implementation proposal for this. It does not require any API change > since everything happens internally in the e-vcard. The vCard is lazily parsed, > meaning that it only gets parsed the first time that a field is accessed. Seems that patch is inspired by Fremantle's EDS. Very nice to see this code landing upstream. I think we didn't have that e_vcard_to_string() optimisation. Good catch. + /* XXX: The format is ignored but it does not really matter + since only 3.0 is supported at the moment */ I'd still check the format argument. Sooner or later you want to properly support vCard 2.0. This (horribly outdated) format seems to be the only working interchangeable vCard version. You'll also find quite some vcard 2.0 fixes in F-EDS (hint, hint, ;-))
(In reply to comment #2) > Created an attachment (id=190408) [details] [review] > Second patch to stop generating FILE_AS in e_contact_new_from_vcard() > > Unfortunately, this seems to be required. Does anyone have an idea how to fix > it more nicely so that it becomes acceptable for upstream? AFAIK for Fremantle we just ignored FILE_AS. If you want to support it, I'd hook a dynamic getter for E_CONTACT_FILE_AS? Like it is done for all the other computed properties.
Hi, Yes, I chose the same approach as in eds-fremantle. I think it was nicely solved. For the FILE_AS, I'm currently working on generating it in e-vcard.c instead of in e-contact.c, in e_vcard_ensure_attributes() function. I think this will work and at least we would not loose the functionality. Please stop me if I missed something and this will not work :)
Created attachment 190412 [details] [review] Improved patch Here is an improved patch which supports generating the FILE_AS attribute as soon as the vCard gets parsed.
I should mention that this patch is for gnome 2.32 at the moment. It is from the meego-eds branch but it applies just fine on gnome-2-32 branch. If it is acceptable for upstream, I can certainly rebase on master later on.
Thanks for a patch. The EVCard way of later parsing is nice, I like the idea, but please do not move FILE_AS filling from EContact to EVCard, rather redefine FILE_AS EContact attribute to have a setter and a getter and in the getter populate it, if not filled.
Created attachment 190431 [details] [review] Revised patch with FILE_AS generation in contact getter Based on mcrha's feedback, I have revised my patch and I would appreciate if someone could review the result.
Created attachment 190437 [details] [review] Revised patch with FILE_AS generation in contact getter Use g_strdup() instead of strdup(), thanks mcrha.
Thanks, looks good. I see you didn't copy&paste the file_as getter code for creation on the fly, (some spaces are missing and it's differently structured), but that's no issue. I'll commit this to master, with one more little change which isn't related to your changes at all. Few lines below your change in e_contact_get() is a call to g_strstrip(). I made it conditional based on the rv being NULL or not. Just in case.
Created commit b4e498e in eds master (3.1.3+)
Milan> I don't think the NULL check for rv is necessary since there is already a check 3 lines before: if (info->t & E_CONTACT_FIELD_TYPE_STRUCT) return (gpointer) info->boxed_type_getter (); else if (!rv) return NULL; else - return g_strstrip (g_strdup (rv)); + return rv ? g_strstrip (g_strdup (rv)) : NULL;
Ouch, you are right, my bad, I'm sorry for that. Reverted as commit 1255268 in eds. Thanks for letting me know, I do not know where I was looking that I overlooked such obvious thing :(
Do we need an equivalent change in the master branch? If so, this bug should probably stay open.
Ah, I see that you did commit something to master, mentioned in comment #11. But do we need something equivalent for the new EClient API, rather than just the deprecated EBook API?
(In reply to comment #16) > But do we need something equivalent for the new EClient API, rather than just > the deprecated EBook API? Nope, fortunately not. This is not touching EBook at all, at least not directly, this is touching EVCard and EContact. Both of these are used in both EBook and EBookClient. I wouldn't commit this if I might think that this requires a change in the new EClient API ;)
Created attachment 203335 [details] [review] eds patch for evolution-data-server; The part for file_as getter has a regression. The code before this change replaced also empty file-as with a sane string, while the new code lefts this value empty (non-null, just empty string). Patrick Ohly helped me to discover this issue when looking around a downstream bug https://bugzilla.redhat.com/show_bug.cgi?id=760491
Created commit 6f0fe6f in eds master (3.3.3+) Created commit d072dd3 in eds gnome-3-2 (3.2.3+)
A link to the fix: http://git.gnome.org/browse/evolution-data-server/commit/?id=6f0fe6f4efc6dec69da734f96ac55f0102fb3df3 Thanks for fixing this, Milan.
*** Bug 605454 has been marked as a duplicate of this bug. ***