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