Re: [Evolution-hackers] improve PHOTO support in libebook/EDS
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 no
Re: [Evolution-hackers] [evolution-kolab] towards Upstream-Integration
Hi, On Friday, 15 July 2011, Matthew Barnes wrote: > Sorry for the delayed response... NP at all, thanks for your input! > [...] > The process really isn't as formal as the wiki makes it out to be. > Because you're an extension module for an application that has already > met these requirements, and because you're using a compatible license, I > think for the most part you're grandfathered in. Sounds fine. Just wanted to make sure we don't miss something important. > > From there, I took the hop to http://live.gnome.org/CopyrightAssignment > > and found myself puzzled again. :) Of how much relevance are the details > > listed there for an Evolution plugin? Our source is LGPL v2.1+, the user > > docs and stuff are CC BY-SA ... so, are any details of that copyright > > stuff applicable to us? > > Obviously IANAL, but I believe as long as you're not engaged in such > nefarious activities as requiring outside evolution-kolab contributors > to hand over copyrights of their own code to you or your organization, > or are holding or seeking patents on parts of the evolution-kolab code, > there shouldn't be anything to worry about. Honest LGPL v2.1+, no tricks, no pitfalls. We're not evil(tm). ;-) > [...] > Translations will come when the project is added to Damned Lies > (http://l10n.gnome.org/). Long as the software is set up to receive > translations using gettext and intltool, which you said below it is, > you're fine. Yep. The gettext/intltool infrastructure needs some "love" still, but that should cause no harm. > > From there, the hop was to > > http://live.gnome.org/ReleasePlanning/ModuleProposing and the following > > questions arose: > > * does evolution-kolab as an Evo plugin need to go through the module > > proposal cycle? > [...] > > * "Judgement Criteria" ... can I deduce from what is listed there, that > > evolution-kolab > [...] > Again, because you're an extension module for an application that has > already passed this process, I think you're pretty much grandfathered > in. Maybe check with the release team about jhbuild, but otherwise I > see no need for evolution-kolab to undergo this process. Just start > uploading release tarballs when you're ready. Very well. jhbuild will be waiting for us somewhere farther down the road then. Thanks for the insights. We should be fine to continue from here. Kind regards, Christian PS.:I'll be on leave for a month, so I'll be able to continue with this topic only after returning from vacation and and being back inside my cubicle. Most likely, there will be some bugs and issues waiting for me to fix them, so I can just as well fix them first and push upstream next. -- kernel concepts GbRTel: +49-271-771091-14 Sieghuetter Hauptweg 48 D-57072 Siegen http://www.kernelconcepts.de/ signature.asc Description: This is a digitally signed message part. ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... http://mail.gnome.org/mailman/listinfo/evolution-hackers
