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

[Tristan Van Berkom <tristanvb openismus com>]

On Thu, 2011-07-14 at 17:19 -0400, Tristan Van Berkom wrote:
> 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()).

Ok I reviewed my patch and covered the memory leaks which I've suspected
(I was confused mostly because e_contact_set() apis dont take ownership
of the passed data while other functions like
e_data_book_view_notify_vcard() eat up the passed data).

Anyway, it's always nice if someone could pass a keen eye over it but
I'm at this point quite confident myself that the patch does not
introduce any leaks.

I also took the liberty of creating 'load_contact()' and 'load_vcard()' 
static functions in the file backend as the code seems more readable
while reducing the inline BDB api interactions and error handling
as much as possible.

Finally I squashed the whole 'openismus-work' branch down to two
comprehensible commits, this should both make it easier to review
and allow me to port the patch to master with less trouble.

Cheers,
       -Tristan

The commits are (still on openismus-work branch):

commit cb9c59ba5812a07086b87c1638ac7a456f5e1b74
Author: Tristan Van Berkom <tristan van berkom gmail com>
Date:   Fri Jul 15 19:00:51 2011 -0400

    Make local addressbook backend store image data as URIs.
    
    Whenever the local address book receives a uri as a binary
    blob it proceeds to transform it to a uri accessbile on
    the system somewhere under $XDG_DATA_HOME. The local backend
    cleans up old photo uris when contacts are removed or modified
    to use new photos.
    
    Additionally, whenever it is detected that the user is
    cross-referencing a uri which belongs to another contact,
    a hard-link will be created to the addressbook owned file
    on disk and a new uri will be assigned to any additional
    contacts which try to share a uri owned by the addressbook.


commit 781fd655b9722e44633947cd283abe6b8ba89e08
Author: Tristan Van Berkom <tristan van berkom gmail com>
Date:   Fri Jul 15 19:01:25 2011 -0400

    Added test case showing photo data stored as uris.
    
    The test asserts:
      o binary inlined photos added to the EBook come out
        as uris in the EBookView signals
      o that it is still possible to use an external uri that the
        addressbook does not recognize at which point the addressbook
        is simply expected to store the provided URI string without
        any extra management (the test does this, however it only
        asserts that a uri is indeed returned).
      o When sharing an addressbook owned uri fetched from one contact
        and assigning it to the next contact, the second contact's uri
        is still accessible on disk after deleting the first contact.


> 
> > > 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
> 
> 
> _______________________________________________
> evolution-hackers mailing list
> evolution-hackers gnome org
> To change your list options or unsubscribe, visit ...
> http://mail.gnome.org/mailman/listinfo/evolution-hackers