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

2011-07-15 Thread Tristan Van Berkom
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

2011-07-14 Thread Tristan Van Berkom
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

2011-07-14 Thread Tristan Van Berkom
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

2011-07-09 Thread Patrick Ohly
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

2011-07-08 Thread Burton, Ross
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

2011-07-08 Thread Patrick Ohly
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

2011-07-08 Thread Patrick Ohly
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

2011-07-08 Thread Patrick Ohly
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

2011-07-08 Thread Tristan Van Berkom
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

2011-07-07 Thread Patrick Ohly
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

2011-07-07 Thread Chenthill
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

2011-07-07 Thread Patrick Ohly
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

2011-07-07 Thread Tristan Van Berkom
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

2011-07-06 Thread Tristan Van Berkom
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