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 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
Re: [Evolution-hackers] improve PHOTO support in libebook/EDS
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 ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ...
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]). 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
Re: [Evolution-hackers] improve PHOTO support in libebook/EDS
On Fri, 2011-07-08 at 14:45 -0400, Tristan Van Berkom wrote: However the EContact passed to e_book_commit_contact() will not be modified by the call to the backend (the gdbus call simply queues a job in the addressbook thread and returns)... so for the caller to get the real uri one must further call e_book_get_contact() or have an EBookView in play to receive it. I don't think EDS ever made the promise that an items would be saved exactly as sent. Apps which need an exact copy of the stored item indeed should use a view to get the real content of the database after requesting changes. Perhaps the backend needs to claim that it supports a list of protocols for staging data (such as 'file','http','ftp' etc) ? The staging dir would be, by definition, local. I don't think we should support anything else. Ah I had imagined it the other way around (that the addressbook would take care of downloading any data from a client's directory). This is also what I had in mind. But it seems more practical to leave file uploading to the client, We can design the API to make that possible, but I don't think we should worry to much about it now. All in all then the added code I think would be: o An api for a client to get a staging directory (which can return NULL) const gchar *e_book_get_staging_uri (); The api would be implemented with a new GDBus method call or property which would in turn be handled by EDataBook. EDataBook would implement it in a backend specific way and return NULL if the backend does not implement the new -get_staging_uri() vfunc. o EBookBackendFile (the BDB addressbook backend) will then manage a staging directory and implement the new vfunc: EBookBackendClass-get_staging_uri() to report the file://path/to/staging/directory/ Managing the staging directory will only consist of creating it at book creation time, removing it at book remove time and emptying it out every time the book is freshly opened (just in case). Do we want one staging directory per address book, or one staging directory per client? My proposal was to create one per client, because then client's don't have to worry about file name collisions (they are the only ones writing into it) and files can be deleted once the client exits. If we have a long-running user of the address book (like Evolution) and a short-lived one (like SyncEvolution) and only one staging directory, then files created and not properly committed by SyncEvolution cannot be cleaned until also Evolution quits. It might also be advantageous to have one directory per client process once MeeGo adds separate privileges for processes running under the same user ID. This is currently a pie in the sky, though. Note that the private staging directory is a bit at odds with allowing arbitrary URIs. While such a guarantee is easy for local directories, it might make less sense for other URIs (like HTTP resources + PUT). This could be resolved by making the guarantee only for file:// URIs. o The local file backend will then move files from the staging directory to the 'photos' directory and update the uris in the stored vCards (whenever receiving vCard updates that refer to files in the staging uri). If the addressbook receives uris in the staging directory which have failed to upload or do not exist for any reason that will be considered an error (the client needs to finish uploading successfully before sending vcards holding staged uris of course). Clients can then try to get a staging uri, if they receive a staging uri then they should check if they support the protocol. If the backend returns an ssh:// staging directory and the client cannot deal with that, then the client should fall back to sending binary blobs (whether or not there is a staging directory available, clients can continue to send photo data as binary blobs reliably anyway). In theory clients can deal with the addressbook owned staging uri pretty conveniently using GIO to check if the staging uri is supported and to upload data. Perhaps this is not so very complex after all ? I think it is doable. -- Bye, Patrick Ohly -- patrick.o...@gmx.de http://www.estamos.de/ ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] improve PHOTO support in libebook/EDS
On 7 July 2011 09:48, Patrick Ohly patrick.o...@gmx.de wrote: At any rate, if it's judged that the performance gain of using a staging directory is worth the added complexity then let's do it. I'd like to hear some other opinions about this. Ross? It does sound like a lot of extra complexity and complication for the performance gain... Ross ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] improve PHOTO support in libebook/EDS
On Fri, 2011-07-08 at 10:42 +0100, Burton, Ross wrote: On 7 July 2011 09:48, Patrick Ohly patrick.o...@gmx.de wrote: At any rate, if it's judged that the performance gain of using a staging directory is worth the added complexity then let's do it. I'd like to hear some other opinions about this. Ross? It does sound like a lot of extra complexity and complication for the performance gain... So what do you suggest? Inline the data when storing a photo and strip it out in the backend? -- Bye, Patrick Ohly -- patrick.o...@gmx.de http://www.estamos.de/ ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] improve PHOTO support in libebook/EDS
On Thu, 2011-07-07 at 20:54 -0400, Tristan Van Berkom wrote: It's possible but will need to be conditional, probably depending on whether there is a staging directory available or not. I'll start thinking about how this staging directory could be implemented, I suppose the client needs to create one somehow while opening the EBook, and only on the condition that the backend can handle incoming data from a staging directory. The EDS daemon needs to create the directory. That's required for the cleanup rules to work. I would do it in the backend, possibly with some server-side utility code that can be used by multiple backends. Possibly at book creation time one needs to specify an actual directory for this (or has the option to specify a directory)... The backend should choose. That way it can pick something that is suitable (like on the same filesystem) without exposing implementation details to the client. How do remote backends handle uris from the local staging dir ? They'll simply refuse to create a staging directory and thus request that the client continues to inline data. perhaps the staged uris would be sent as ftp:// to the remote backend but locally stored in a file:// uri... does the backend need to participate in the formatting of the uris that will be sent to it ? That is one possibility. Don't forget that the local file backend also has to rewrite the URI (from staging dir to final location). Perhaps the backend needs to claim that it supports a list of protocols for staging data (such as 'file','http','ftp' etc) ? The staging dir would be, by definition, local. I don't think we should support anything else. -- Best Regards Patrick Ohly Senior Software Engineer Intel GmbH Open Source Technology Center Pützstr. 5 Phone: +49-228-2493652 53129 Bonn Germany ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] improve PHOTO support in libebook/EDS
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. 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. -- Bye, Patrick Ohly -- patrick.o...@gmx.de http://www.estamos.de/ ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] improve PHOTO support in libebook/EDS
On Fri, 2011-07-08 at 12:49 +0200, Patrick Ohly wrote: On Thu, 2011-07-07 at 20:54 -0400, Tristan Van Berkom wrote: It's possible but will need to be conditional, probably depending on whether there is a staging directory available or not. I'll start thinking about how this staging directory could be implemented, I suppose the client needs to create one somehow while opening the EBook, and only on the condition that the backend can handle incoming data from a staging directory. The EDS daemon needs to create the directory. That's required for the cleanup rules to work. I would do it in the backend, possibly with some server-side utility code that can be used by multiple backends. Possibly at book creation time one needs to specify an actual directory for this (or has the option to specify a directory)... The backend should choose. That way it can pick something that is suitable (like on the same filesystem) without exposing implementation details to the client. How do remote backends handle uris from the local staging dir ? They'll simply refuse to create a staging directory and thus request that the client continues to inline data. perhaps the staged uris would be sent as ftp:// to the remote backend but locally stored in a file:// uri... does the backend need to participate in the formatting of the uris that will be sent to it ? That is one possibility. Don't forget that the local file backend also has to rewrite the URI (from staging dir to final location). Right, that should work in the same way that it currently works with incoming binary blobs: The blob or staged uri gets transformed before being stored in the BDB... only after that do any notifications get sent (so the notifications already come with the properly transformed contact). However the EContact passed to e_book_commit_contact() will not be modified by the call to the backend (the gdbus call simply queues a job in the addressbook thread and returns)... so for the caller to get the real uri one must further call e_book_get_contact() or have an EBookView in play to receive it. Perhaps the backend needs to claim that it supports a list of protocols for staging data (such as 'file','http','ftp' etc) ? The staging dir would be, by definition, local. I don't think we should support anything else. Ah I had imagined it the other way around (that the addressbook would take care of downloading any data from a client's directory). But it seems more practical to leave file uploading to the client, then as you say managing the directory becomes more straight forward and also at initialization time I suppose the backend only needs to report a staging uri. All in all then the added code I think would be: o An api for a client to get a staging directory (which can return NULL) const gchar *e_book_get_staging_uri (); The api would be implemented with a new GDBus method call or property which would in turn be handled by EDataBook. EDataBook would implement it in a backend specific way and return NULL if the backend does not implement the new -get_staging_uri() vfunc. o EBookBackendFile (the BDB addressbook backend) will then manage a staging directory and implement the new vfunc: EBookBackendClass-get_staging_uri() to report the file://path/to/staging/directory/ Managing the staging directory will only consist of creating it at book creation time, removing it at book remove time and emptying it out every time the book is freshly opened (just in case). o The local file backend will then move files from the staging directory to the 'photos' directory and update the uris in the stored vCards (whenever receiving vCard updates that refer to files in the staging uri). If the addressbook receives uris in the staging directory which have failed to upload or do not exist for any reason that will be considered an error (the client needs to finish uploading successfully before sending vcards holding staged uris of course). Clients can then try to get a staging uri, if they receive a staging uri then they should check if they support the protocol. If the backend returns an ssh:// staging directory and the client cannot deal with that, then the client should fall back to sending binary blobs (whether or not there is a staging directory available, clients can continue to send photo data as binary blobs reliably anyway). In theory clients can deal with the addressbook owned staging uri pretty conveniently using GIO to check if the staging uri is supported and to upload data. Perhaps this is not so very complex after all ? Cheers, -Tristan ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ...
Re: [Evolution-hackers] improve PHOTO support in libebook/EDS
On Wed, 2011-07-06 at 16:16 -0400, Tristan Van Berkom wrote: [staging directory for out-of-band photo data transmission] This alternative proposal is strictly regarding the photo data for which the server must take ownership of I assume, correct ? Yes. I might not have been clear enough in my proposal but I only intend for new or modified photos to be sent to the addressbook. Understood. I personally don't mind the D-Bus overhead in these cases because I consider them rare. But there are people who run massive write benchmarks against EDS anyway and do flag low performance in them as a problem. So if we can find a solution that also optimizes the write performance, then it would be better. The staging directory you propose if I understand it correctly would also be used under only the same circumstances, I suppose the questions are: o What is faster: writing a file to a staging directory and then moving it to a permanent location or sending the blob once over D-Bus and saving it to the permanent location. If the photo data needs to be stored as file and moving the file into its permanent location can be done with a rename, then the staging directory approach is faster. I expect writing the file to be equally fast, regardless of who does it. Sending inline data adds D-Bus overhead (considerable for large chunks of data!) and encoding/decoding overhead. o What is more reliable ? (it seems that there are less points of failure when sending the blob but I could be wrong). Agreed. I tried to mitigate that with the rules around cleaning up the staging directory. Perhaps the staging directory is a little faster when batching lots of photo modifications during a 'sync' operation ? (the client gets to write to disk while waiting for the server to be ready for the next batch of contact additions perhaps ?) At any rate, if it's judged that the performance gain of using a staging directory is worth the added complexity then let's do it. I'd like to hear some other opinions about this. Ross? I think the more important issue at hand is that to offer a reliable api for EBookView then the backend has to track the views which have received notification of the contact modifications. In other words when I have an EBookView and I have been notified that a contact exists with a photo at a given uri path, then until I receive notification that that uri is invalid, removed or replaced with a new one... I should be able to access that file. I don't see this as a problem. When a process fails to load a photo because the contact was already removed on the server together with that photo, then shortly after the attempt to read the photo the process will get the notification that the contact is gone and thus the process no longer needs the photo. I also think that this is a very rare situation. Definitely doesn't justify adding complex code to the D-Bus interface between libebook and EDS. Another hard limitation is regarding storing uris for which you don't want the addressbook to assume ownership of. The clean way as far as I can see is whoever is responsible for adding an 'alien uri' to an addressbook is responsible for removing that entry from the addressbook at the time that the uri might be deleted. Agreed. But I don't think there is a clean and efficient way of solving this. It'll be much simpler to accept that there will be URIs that point to deleted files. The intention is to not use URIs pointing to arbitrary local files. Therefore we don't need to provide a solution that assists apps who want to do that. Well... perhaps we should just swallow the ugly race and do nothing about it and say that: a uri might be invalid from time to time directly before your view receives notification of the removal ? Would that be acceptable ? Yes. -- Bye, Patrick Ohly -- patrick.o...@gmx.de http://www.estamos.de/ ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] improve PHOTO support in libebook/EDS
On Thu, 2011-07-07 at 10:48 +0200, Patrick Ohly wrote: On Wed, 2011-07-06 at 16:16 -0400, Tristan Van Berkom wrote: [staging directory for out-of-band photo data transmission] This alternative proposal is strictly regarding the photo data for which the server must take ownership of I assume, correct ? Yes. I might not have been clear enough in my proposal but I only intend for new or modified photos to be sent to the addressbook. Understood. I personally don't mind the D-Bus overhead in these cases because I consider them rare. But there are people who run massive write benchmarks against EDS anyway and do flag low performance in them as a problem. So if we can find a solution that also optimizes the write performance, then it would be better. The staging directory you propose if I understand it correctly would also be used under only the same circumstances, I suppose the questions are: o What is faster: writing a file to a staging directory and then moving it to a permanent location or sending the blob once over D-Bus and saving it to the permanent location. If the photo data needs to be stored as file and moving the file into its permanent location can be done with a rename, then the staging directory approach is faster. I expect writing the file to be equally fast, regardless of who does it. Sending inline data adds D-Bus overhead (considerable for large chunks of data!) and encoding/decoding overhead. I like the staging directory approach. Would it be possible for implementing the logic to convert the inline data -file inside e_book_client_add_contact so that the clients need to worry about changing the code ? - Chenthill. o What is more reliable ? (it seems that there are less points of failure when sending the blob but I could be wrong). Agreed. I tried to mitigate that with the rules around cleaning up the staging directory. Perhaps the staging directory is a little faster when batching lots of photo modifications during a 'sync' operation ? (the client gets to write to disk while waiting for the server to be ready for the next batch of contact additions perhaps ?) At any rate, if it's judged that the performance gain of using a staging directory is worth the added complexity then let's do it. I'd like to hear some other opinions about this. Ross? I think the more important issue at hand is that to offer a reliable api for EBookView then the backend has to track the views which have received notification of the contact modifications. In other words when I have an EBookView and I have been notified that a contact exists with a photo at a given uri path, then until I receive notification that that uri is invalid, removed or replaced with a new one... I should be able to access that file. I don't see this as a problem. When a process fails to load a photo because the contact was already removed on the server together with that photo, then shortly after the attempt to read the photo the process will get the notification that the contact is gone and thus the process no longer needs the photo. I also think that this is a very rare situation. Definitely doesn't justify adding complex code to the D-Bus interface between libebook and EDS. Another hard limitation is regarding storing uris for which you don't want the addressbook to assume ownership of. The clean way as far as I can see is whoever is responsible for adding an 'alien uri' to an addressbook is responsible for removing that entry from the addressbook at the time that the uri might be deleted. Agreed. But I don't think there is a clean and efficient way of solving this. It'll be much simpler to accept that there will be URIs that point to deleted files. The intention is to not use URIs pointing to arbitrary local files. Therefore we don't need to provide a solution that assists apps who want to do that. Well... perhaps we should just swallow the ugly race and do nothing about it and say that: a uri might be invalid from time to time directly before your view receives notification of the removal ? Would that be acceptable ? Yes. ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] improve PHOTO support in libebook/EDS
On Thu, 2011-07-07 at 15:09 +0530, Chenthill wrote: I like the staging directory approach. Would it be possible for implementing the logic to convert the inline data -file inside e_book_client_add_contact so that the clients need to worry about changing the code ? need *not* worry, right? Yes, I can imagine that this would work. Should we allow to enable or disable this client side? In other words, should we force the separate storing of photo data in all cases or is there a legitimate situation where a client might want to force the data to be stored inline? I can't think of one myself. Note that once we introduce this mechanism (whether it is mandatory or not), all clients must be prepared to deal with PHOTO URIs. But this is not new, because there might already be contacts in current EDS with URIs. The only change would be that clients get an URI for their *own* contacts even if those had inline data. -- Bye, Patrick Ohly -- patrick.o...@gmx.de http://www.estamos.de/ ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] improve PHOTO support in libebook/EDS
On Thu, 2011-07-07 at 10:48 +0200, Patrick Ohly wrote:
On Wed, 2011-07-06 at 16:16 -0400, Tristan Van Berkom wrote:
[staging directory for out-of-band photo data transmission]
This alternative proposal is strictly regarding the photo data for
which the server must take ownership of I assume, correct ?
Yes.
I might not have been clear enough in my proposal but I only
intend for new or modified photos to be sent to the addressbook.
Understood. I personally don't mind the D-Bus overhead in these cases
because I consider them rare. But there are people who run massive write
benchmarks against EDS anyway and do flag low performance in them as a
problem. So if we can find a solution that also optimizes the write
performance, then it would be better.
The staging directory you propose if I understand it correctly
would also be used under only the same circumstances, I suppose
the questions are:
o What is faster: writing a file to a staging directory and
then moving it to a permanent location or sending the blob
once over D-Bus and saving it to the permanent location.
If the photo data needs to be stored as file and moving the file into
its permanent location can be done with a rename, then the staging
directory approach is faster. I expect writing the file to be equally
fast, regardless of who does it. Sending inline data adds D-Bus overhead
(considerable for large chunks of data!) and encoding/decoding overhead.
o What is more reliable ? (it seems that there are less
points of failure when sending the blob but I could be wrong).
Agreed. I tried to mitigate that with the rules around cleaning up the
staging directory.
Perhaps the staging directory is a little faster when batching lots
of photo modifications during a 'sync' operation ? (the client
gets to write to disk while waiting for the server to be ready
for the next batch of contact additions perhaps ?)
At any rate, if it's judged that the performance gain of using
a staging directory is worth the added complexity then let's do it.
I'd like to hear some other opinions about this. Ross?
I think the more important issue at hand is that to offer
a reliable api for EBookView then the backend has to track
the views which have received notification of the contact
modifications.
In other words when I have an EBookView and I have been
notified that a contact exists with a photo at a given uri path,
then until I receive notification that that uri is invalid,
removed or replaced with a new one... I should be able to
access that file.
I don't see this as a problem. When a process fails to load a photo
because the contact was already removed on the server together with that
photo, then shortly after the attempt to read the photo the process will
get the notification that the contact is gone and thus the process no
longer needs the photo.
I also think that this is a very rare situation. Definitely doesn't
justify adding complex code to the D-Bus interface between libebook and
EDS.
Another hard limitation is regarding storing uris for which
you don't want the addressbook to assume ownership of.
The clean way as far as I can see is whoever is responsible
for adding an 'alien uri' to an addressbook is responsible
for removing that entry from the addressbook at the time
that the uri might be deleted.
Agreed. But I don't think there is a clean and efficient way of solving
this. It'll be much simpler to accept that there will be URIs that point
to deleted files.
The intention is to not use URIs pointing to arbitrary local files.
Therefore we don't need to provide a solution that assists apps who want
to do that.
Well... perhaps we should just swallow the ugly race and do nothing
about it and say that:
a uri might be invalid from time to time directly before your view
receives notification of the removal ?
Would that be acceptable ?
Yes.
Ok actually that sounds reasonable, I wasn't sure and would have
been embarrassed to be told that
my uri is invalid before I receive the remove notification.
I now have a much simpler patch up on the openismus-work branch[0].
(it's also in patch form on the bug[1]).
It was easy enough to manage the photo files with a
simple algorithm like so:
in_add_modify_remove_contact_code () {
/* Find a new filename for the incoming binary blobs and put
* the new data on disk
*/
maybe_transform_incoming_contact ();
/* Check new uris vs old uris in the DB... if old unused uris
* belong to us (that is if they are in our photo cache directory)
* then delete the old uri.
*/
maybe_delete_unused_uris ();
}
The branch has a test case that verifies modifications and additions of
binary blobs get properly transformed.
Currently I don't have any unit tests in place that ensure the files are
properly deleted.
Also there is a strange use case worth
Re: [Evolution-hackers] improve PHOTO support in libebook/EDS
On Wed, 2011-07-06 at 18:15 +0200, Patrick Ohly wrote: Hello! It was pointed out before that support for PHOTO data in EDS is sub-optimal because the data has to be encoded as B64 data in the vCard and then gets transmitted like that via D-BUs and stored in Berkley DB. Hi, Unfortunately I started this off list so I'm just going to append my original off-list proposal at the end of this mail (it may help for people to understand the context better). A high-level description of the problem is here: http://wiki.meego.com/Architecture/planning/evolution-data-server/eds-improvements#Contacts:_Store_PHOTO_Data_as_Plain_Files Photos should be transferred over D-Bus via a filepath to a local file rather than transferring the actual photo data over D-Bus. The VCard format already allows this option (See the PHOTO property). We must be careful to keep these local files in sync and to remove them when contacts are removed, and to restrict access to the files from other processes. There should be utility code available to libebook users to convert between the different representations. Goals: 1. Apps should be able to create photo files without ever passing the data to libebook. 2. External PHOTO file and corresponding contact must be kept in sync: 1. If a contact is deleted, the photo also needs to be removed. 2. If a photo file is created and then storing the contact fails, who removes the file? 3. Access to photo data must be limited to processes which have the right permissions. --- There also was a proposal for how to solve this, but I am no longer entirely happy with it. Openismus is working on this at the moment, see https://bugzilla.gnome.org/show_bug.cgi?id=652178 Before diving further into prototyping or implementing a solution, I would like to come to an agreement about the client visible behavior and a rough outline how to implement it. First let me clarify that we need to distinguish two different use cases: 1. The photo app owns and manages a set of photos. A link is added to a contact. If the contact is deleted, the photo must remain because it is part of a slide show. If the photo is moved or renamed, the link remains but is obviously no longer useful (dangling link as in a symbolic link in a file system). 2. Photo data is attached to a contact. EDS becomes the owner for the data and must ensure that it is stored and deleted together with the contact. Apps get access to the photo via its URI, exactly as in the first case. The extension that we are discussing addresses the second point. It must not break the first one. Tristan's plan seems to be to pass the PHOTO data inline in the vCard to the file backend and to strip it out there. Note that this is not optimal in terms of performance: if the data is already available separately (as it is in syncing, for example), then it has to be inlined and transferred via D-Bus (slow). This is acceptable because contacts are read a lot more often than written. But that doesn't stop people from running write benchmarks ;-) I'd only go down that route if it really makes the implementation a lot simpler. From the description in 652178 and private email it doesn't sound like it necessarily does. How about a different approach: * Each EBookClient can request a staging directory. If the backend supports this mechanism, it is created for this client by EDS. If not, the client has to inline PHOTO data as before. * The content of the staging directory is ephemeral. When a client crashes or disconnects, EDS removes the directory and everything inside it. When EDS crashes, it cleans up all staging directories when it restarts. * If a client wants EDS to take ownership of the photo, it creates a file in the staging directory with an arbitrary name and sets the PHOTO URI such that it points to it with an absolute path. * The client submits the contact. At that point it stops being responsible for the file. * The EDS backend checks for such special PHOTO URIs. If it finds one, it copies or moves the file to its permanent location and updates the URI before storing the contact and notifying clients. Note that backends are free to do that however they want. For example, a CardDAV backend might upload the file data to a resource on the HTTP server and set the PHOTO URI to a http:// URL. The file backend chooses a staging directory in the same file system as the permanent photo directory and thus can simply move the file instead of having to copy it. libebook might offer some additional
