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]).

Oops.

It's been said before but I probably should have mentioned again,
the code is available on 'openismus-work' branch which is regularly
rebased off of 'meego-eds' branch. 

To see the code in action, read and run:
    addressbook/tests/ebook/test-ebook-photo-is-uri.c

As soon as the patch is finalized I'll cook another one
up which targets master (on openismus-work-master branch).

Cheers,
        -Tristan

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