Re: [Evolution-hackers] improve PHOTO support in libebook/EDS

[Tristan Van Berkom <tristanvb openismus com>]

On Fri, 2011-07-08 at 13:02 +0200, Patrick Ohly wrote:
> On Thu, 2011-07-07 at 20:39 -0400, Tristan Van Berkom wrote:
> > I now have a much simpler patch up on the openismus-work branch[0].
> > (it's also in patch form on the bug[1]).
> 
> That looks a lot saner than the previous solution. Removing all the
> attempts to keep clients perfectly informed about file removal has paid
> off.
> 
> I haven't done a detailed code review. I'd prefer to have the EDS
> experts do that. But perhaps one change first: this code seems useful
> for a variety of local backends. I think the bulk of it should be in the
> shared libedata-book library.
> 
> > Also there is a strange use case worth pointing out:
> > 
> > You are not allowed to use an addressbook generated uri as the uri
> > for another field of the same contact or another contact.
> > 
> > So you are allowed to set multiple contacts photo fields
> > to point to "http://hostyouravatar.com/buster.png";, but you
> > are not allowed to do:
> > 
> >    /* Get that contact named "Elvis Presley" */
> >    PhotoContact *photo = e_contact_get (contact_a, E_CONTACT_PHOTO);
> > 
> >    /* Use Elvis's face on this new contact I'm creating */
> >    PhotoContact *new_photo = create_photo (photo->data.uri);
> > 
> >    e_book_commit_contact (new_photo);
> > 
> > Not sure if that's an acceptable rule or not, if not then we
> > either need to add some complex code to implement internal
> > refcounting on the uris in the backend... or perhaps we
> > could trap the incoming shared uris and create copies
> > on disk instead...
> 
> I would trap the incoming URI and create a hardlink under a different
> name. That'll share the data without requiring us to implement
> refcounting in the backend.

Alright, 
   I think I've dealt with most of the issues for this patch,
currently the patch handles shared uris which are owned by 
the addressbook as you suggested, simply by using a hard link.

I've updated the test case to assert that if you:
   a.) Use the uri from one contact photo as the uri 
       for another contact photo
   b.) Delete the original contact
   c.) The remaining contact holding the "shared uri"
       will still be accessible on disk (the test
       case runs a simple g_file_test() to assert this).

One minor concern I have is regarding the process of
transforming a binary blob into a uri, I think I mentioned
this before, currently I store all these uris as "filename.data".

Depending on the mime type and the tools in use to load the
file from disk, there is a loss.

It would be nice if we used the EContactPhoto->data.inlined.mime_type
to generate a suitable filename extension at least for the
dumped uri.

However there seems to be no easy way of doing this that I know of,
it seems that we would have to have some kind of hard coded table
of known mime types and the file extensions we want to give it.

So if that's the case and we have to add messy hard code of the like,
maybe we would rather just live with "binary blobs are saved as .data".

Another issue is the addressbook possibly takes a slight performance
hit for all of this, because now when intercepting vCards from the
client (via e_book_commit/add/remove_contact()), the current version
of the uri already stored in the BDB needs to be pulled out and
checked against the incoming data.

The fact we need to exercise a db->get() and do some manipulation before
doing the actual db->set/put() in any and every transaction can probably
be optimized (i.e. right now I do it for every field, I could at least
limit the amount of db->get()/e_contact_new_from_vcard() to exactly one
per transaction... but the fact is we absolutely need to do this extra
db->get() for every transaction at least once and there's no way I can
see to avoid that (short of adding some uri/contact caching hash table
to the backend and micro-managing each contact).

Finally, I'm going to re-review my own patch a final time and it would
be nice if someone else did, it's possible I'm leaking memory in the
places that vcards get transformed to EContacts and EContactPhotos are
set on contacts and such (the internal EDS apis seem to enjoy throwing
data from one function to the next which is probably good for
performance while on the other hand it's hard for me to tell what should
be freed or what should be "given" to e_contact_set()).

> > In any case, I think we could try to close this first and
> > then look at the staging directory feature separately, seeing
> > that this code will very easily scale from:
> >   a.) "If the incoming photo is a binary blob" to...
> >   b.) "If the incoming photo is a binary blob or 
> >        a uri inside the staging directory..." 
> 
> I agree. We can always do another iteration if the D-Bus overhead for
> storing photos becomes important.
> 

Great,
   Then if the current patch is satisfactory we can move on
right away...

Cheers,
         -Tristan