The API already supports this, you can set a filename in the EContactPhoto struct.

The enhancements that Patrick desires are about adding a policy on top, i.e. copying the files to .local/, removing them when contacts are deleted and so on.

I'm working on this now and I'm starting by just modifying the file backend
and a test case to verify that uris are transferred.

In the mean time here are some questions about the details:

  o For the local BDB backend, should the photos be stored 
    on the filesystem *instead* of the BDB ?

    I guess ideally yes, if so does there need to be a migration of
    existing data from the DBD to the filesystem ?

  o Currently modifying the local addressbook code to send uris 
    instead of inlined data will break any client side code which
    does not handle the uris, is this an issue ?

    Perhaps we need to also introduce an api which returns an
    image for the E_CONTACT_PHOTO from inlined data or from uri
    in an abstract way ?

  o While we can optionally receive photo data as uris, can we
    send photo data as a uri through e_book_client_modify_contact[_sync] () ?

    My assumption right now (correct me if I'm wrong please) is that 
    only the local 'file backend' addressbook is expected to run on 
    the same system as the client and as such is the only backend
    which can possibly handle receiving the photos as uris.

    However another consideration is file access permissions; can
    the client assume that they have read access to the uris sent
    over from the addressbook ? yes that should at worst be a
    detail of installing EDS on the system correctly.

    Can the addressbook daemon assume that they have permission to
    read a uri from a $HOME subdirectory where the client has write
    permissions ? I don't think so.

    Should the convention be that a /tmp file be used for such transfers ?

    I think right now it would be best to just insure any contacts sent
    through e_book_client_modify_contact[_sync]() either use inline photo
    data or send the same uri which they previously received for the said
    contact.

    Either by asserting and failing or by chaining the whole dbus
    operation through g_file_read[_async]() first (i.e. implicitly
    load the data and inline it before sending it).

(In reply to comment #2)
>   o For the local BDB backend, should the photos be stored 
>     on the filesystem *instead* of the BDB ?

I guess yes, because we want to give the filepath to the client.

>     I guess ideally yes, if so does there need to be a migration of
>     existing data from the DBD to the filesystem ?

This doesn't sound terribly important. For the systems (such as Meego) where the performance is critical, people would likely start with an empty address book. If it's needed, this can be an additional patch later.
 
>   o Currently modifying the local addressbook code to send uris 
>     instead of inlined data will break any client side code which
>     does not handle the uris, is this an issue ?

I don't think so. Ross says that the API already supports the use of URIs, so it's the job of applications to handle that possibility. Meego's apps can expect the special behaviour in their patched e-d-s, and upstream can apply this in a new version, giving applications time to adapt if necessary.

>     Perhaps we need to also introduce an api which returns an
>     image for the E_CONTACT_PHOTO from inlined data or from uri
>     in an abstract way ?

Doesn't GdkPixbuf already offer this well enough?
http://developer.gnome.org/gdk-pixbuf/stable/gdk-pixbuf-file-loading.html#gdk-pixbuf-new-from-file

I guess that Qt offers something similar. It seems like a general issue that's orthogonal.

However, I think Patrick Ohly suggested this originally, so he might have a reason.

>   o While we can optionally receive photo data as uris, can we
>     send photo data as a uri through e_book_client_modify_contact[_sync] () ?

I guess we need to support that, yes, though I guess that we would change the URI after copying the file.

>     My assumption right now (correct me if I'm wrong please) is that 
>     only the local 'file backend' addressbook is expected to run on 
>     the same system as the client and as such is the only backend
>     which can possibly handle receiving the photos as uris.

Yes.
 
>     However another consideration is file access permissions; can
>     the client assume that they have read access to the uris sent
>     over from the addressbook ? yes that should at worst be a
>     detail of installing EDS on the system correctly.

Yes, let's assume that it's not an issue for now.

>     Can the addressbook daemon assume that they have permission to
>     read a uri from a $HOME subdirectory where the client has write
>     permissions ? I don't think so.
> 
>     Should the convention be that a /tmp file be used for such transfers ?

I think it would be enough to suggest that in the API documentation.

>     I think right now it would be best to just insure any contacts sent
>     through e_book_client_modify_contact[_sync]() either use inline photo
>     data or send the same uri which they previously received for the said
>     contact.

See my comment above about copying the file. I think we need to support this properly. Nothing in our task description seems to suggest otherwise:
http://wiki.meego.com/Architecture/planning/evolution-data-server/eds-improvements#Contacts:_Store_PHOTO_Data_as_Plain_Files

Ok I've pushed an initial try into 'openismus-work' branch, it's a work in
progress and I plan to squash the commits in openismus-work with more
coherent patches before proposing them.

I'll modify my code to try copying incoming photo modifications as
uris as well, if the file copies fails while modifying contacts then
we can return an error and abort modifying the contact (so the convention
would be, you can send uris to the addressbook and if the addressbook
can read them then it's not an error, defining such a path for incomming
uris we can leave up to users of the apis and sysadmins).

So let me briefly just go over what is done in the patch so far
and what needs to be done and make sure we're on the same page.

What the patch does so far:

  o Currently this patch adds the 'confirm-contacts' method to
    the EBookView's egdbus mechanics, this method is called in
    response to every 'contacts-added/removed/changed' signal
    (This is the *wrong* way to do it as I'll explain below but
    gives me something to work with for now).

  o EDataBookView now manages a stack of current transactions
    and manages the "state" of a contact as perceived by it
    view proxy (states can be 'exists' 'absent' 'adding' 'removing'
    and the like).

  o EDataBookView reports a new 'contact-status-changed' signal
    if notifications have been enabled for a said contact in
    the book view (this again could be done with a better api
    which I need to improve which I'll describe below)

  o EBookBackendFile now watches the currently active notifications
    when a contact is modified of removed, this allows us to reliably
    and safely modify and remove photo blob type contact fields.

    To accomplish this, when requested to remove a contact we:
      o enable notifications for the said contact id
      o add the currently present views to a list of expected
        confirmations on that contact
      o when the notifications arrive that all views have indeed
        been notified that the contact is removed, we can then
        unlink the photo files from the disk.

Problems, what still needs doing:

  o The main problem I'm facing right now is the GDBus apis dont
    exactly enjoy what I'm demanding of them, currently we have
    the exported EDataBookView emitting a 'contacts-changed'
    signal in the dark which has no async reply or anything
    to let us know if or when the proxy actually gets the signal

    Instead of boiling down to g_dbus_connection_emit_signal(),
    messages sent to the proxy about contact changes should
    boil down to g_dbus_message_send_with_reply().

    What is the "right" way to do this for an exported object
    on the bus like the addressbook I'm not sure (and could
    use a hand with that), this is why I currently added the
    "confirm-contacts" method in order to create a virtual
    reply to the 'contacts-changed' (and related) messages.

    So, this is obviously error prone and plain wrong and
    needs to be fixed.

  o Also currently I have the 'contact-status-changed' signal
    fired by EDataBookView along with 
    e_data_book_view_enable_notifications_for_contact() which
    is IMO an error prone api which I only started off with to
    avoid some extra api churn which I think is needed in the end,
    what should be done instead:

    e_data_book_view_notify_something() should have a variant which
    takes a callback and user data to be called to notify when the
    client has actually received the notification for a said contact.
    (this would make things much more straight forward and reliable
    for the EBookBackendFile).

    So my plan is after fixing the first problem and getting a real
    reply or error from contact notification signals, to go and
    change the above api for something more reliable (one notification
    per call to e_data_book_view_notify_contact_with_reply()).

  o Finally there is also a detail related to transforming a contact
    which has inline data to a contact stored by URI: currently
    the EBookBackendFile does not know about image formats and just
    dumps the image data to 'complex-long-file-name.data', probably
    we need to be constructing a proper filename extension based
    on the provided mime type string.


Currently the code works more or less and there is an added test case:
  test-ebook-photo-photo-is-uri.c (which asserts that inline photo data
gets transformed to a uri).

For some reason the backend currently tries to delete a contact
twice for every call to e_book_contact_remove(), which is probably
a problem in my code... I hope that once I iron out the above
GDBus issue I can get all the proper bookkeeping in place and
come up with a much more thorough test case (make sure files
still exist on disk after removing contacts and before notifications
complete, with multiple views open etc).

Created attachment 191502 [details] [review]
Patch to store incomming binary photo blobs as local file uris

This patch makes the local addressbook backend store incomming
binary photo blobs as uris.

General implications for the local addressbook:

   o If the addressbook is given a photo blob, the addressbook
     assumes ownership of that data and manages a copy on disk

   o If the addressbook is given any uri, then the addressbook
     just keeps a copy of the uri

   o If the addressbook is given a local uri which it previously
     generated, the addressbook treats that uri like any other uri

   o A uri on disk is owned specifically by a single contact, that
     is to say contacts cannot share a uri.

Implications for the caller (libebook user):

   o A libebook user may give the addressbook a uri or a binary blob
     as photo data

   o If you give libebook an external uri, you are responsible for
     unsetting the the photo field when the uri becomes invalid.

   o Photo field uris received from the book should be resolved and
     read before trusted. A photo uri might be invalid before your
     EBookView receives notification of a pending change.

   o You are not allowed to link contact photo fields together by uri
     as a user (i.e. you cannot assign the photo uri of contact A to
     the same uri as contact B).

     Doing so is harmless for uris that are not managed by the addressbook,
     but cross-referencing uris that belong to the local addressbook like
     this will eventually result in premature deletion of a photo resource.

(In reply to comment #5)
> Created an attachment (id=191502) [details] [review]
> Patch to store incomming binary photo blobs as local file uris
> 
> This patch makes the local addressbook backend store incomming
> binary photo blobs as uris.

This has been obsoleted by a more recent patch in the EDS git repo, right?

Please ensure that Bugzilla is always up-to-date.

From the EDS hacker list:


From: 	Tristan Van Berkom <tristanvb@openismus.com>
To: 	Patrick Ohly <patrick.ohly@gmx.de>
Cc: 	Evolution Hackers <evolution-hackers@gnome.org>, Ross Burton <ross.burton@intel.com>, Murray Cumming <murrayc@openismus.com>
Subject: 	Re: [Evolution-hackers] improve PHOTO support in libebook/EDS
Date: 	Fri, 15 Jul 2011 19:14:56 -0400 (16.07.2011 01:14:56)


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 reducing the inline BDB api interactions and error handling
as much as possible.

Finally I squashed the whole 'openismus-work' branch down to two
comprehensible commits, this should both make it easier to review
and allow me to port the patch to master with less trouble.

Cheers,
       -Tristan

The commits are (still on openismus-work branch):

commit cb9c59ba5812a07086b87c1638ac7a456f5e1b74
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Fri Jul 15 19:00:51 2011 -0400

    Make local addressbook backend store image data as URIs.
    
    Whenever the local address book receives a uri as a binary
    blob it proceeds to transform it to a uri accessbile on
    the system somewhere under $XDG_DATA_HOME. The local backend
    cleans up old photo uris when contacts are removed or modified
    to use new photos.
    
    Additionally, whenever it is detected that the user is
    cross-referencing a uri which belongs to another contact,
    a hard-link will be created to the addressbook owned file
    on disk and a new uri will be assigned to any additional
    contacts which try to share a uri owned by the addressbook.


commit 781fd655b9722e44633947cd283abe6b8ba89e08
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Fri Jul 15 19:01:25 2011 -0400

    Added test case showing photo data stored as uris.
    
    The test asserts:
      o binary inlined photos added to the EBook come out
        as uris in the EBookView signals
      o that it is still possible to use an external uri that the
        addressbook does not recognize at which point the addressbook
        is simply expected to store the provided URI string without
        any extra management (the test does this, however it only
        asserts that a uri is indeed returned).
      o When sharing an addressbook owned uri fetched from one contact
        and assigning it to the next contact, the second contact's uri
        is still accessible on disk after deleting the first contact.

Created attachment 192344 [details] [review]
Patch to store incomming binary photo blobs as local file uris [take 2]

Indeed sorry I forgot to post the patch or comment here
.
Here is the new patch attached.

Created attachment 192513 [details] [review]
Patch to store incomming binary photo blobs as local file uris [targetting master branch]

This is the patch ported to 'openismus-work-master', (however its worth
noting that the code is very much the same for the file backend in 
gnome-2-32 and master).

Here are the 2 related commits for this on 'openismus-work-master' branch:

commit e1144f8e964192a093a693892c3d564a42bbd1cf
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Fri Jul 22 20:05:21 2011 -0400

    Make local addressbook backend store image data as URIs.
    
    Whenever the local address book receives a uri as a binary
    blob it proceeds to transform it to a uri accessbile on
    the system somewhere under $XDG_DATA_HOME. The local backend
    cleans up old photo uris when contacts are removed or modified
    to use new photos.
    
    Additionally, whenever it is detected that the user is
    cross-referencing a uri which belongs to another contact,
    a hard-link will be created to the addressbook owned file
    on disk and a new uri will be assigned to any additional
    contacts which try to share a uri owned by the addressbook.

commit 83fe7132d85a099582b5a379b52907df2b6a5680
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Fri Jul 22 20:06:38 2011 -0400

    Added test case showing photo data stored as uris.
    
    The test asserts:
      o binary inlined photos added to the EBook come out
        as uris in the EBookView signals
      o that it is still possible to use an external uri that the
        addressbook does not recognize at which point the addressbook
        is simply expected to store the provided URI string without
        any extra management (the test does this, however it only
        asserts that a uri is indeed returned).
      o When sharing an addressbook owned uri fetched from one contact
        and assigning it to the next contact, the second contact's uri
        is still accessible on disk after deleting the first contact.

Started reviewing this.  To reduce the diff I may commit refactoring patches first.

if (perror && !perror)
  db_error_to_gerror (db_error, perror);

Surely that's not right.

Created attachment 192708 [details] [review]
Minor refactor to local addressbook backend

I've split up the commits on openismus-work-master to split up some
of the refactoring so that it should be easier for you to review/commit
separately.

This patch is the following commit on openismus-work-master:

commit 1fd9e37085f41bb5f024c9ce1df971593504a2c8
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Tue Jul 26 19:06:16 2011 -0400

    Minor refactor of EBookBackendFile.
    
    This minor refactor adds the functions:
      remove_file(), create_directory() and load_vcard().
    
    Mostly this just improves readability, for the case of load_vcard()
    a considerable amount of redundant lines of code are removed.

Created attachment 192709 [details] [review]
Patch to store incomming binary photo blobs as local file uris [targetting master, take 2]

This patch targets master and depends on the previous refactoring patch.

Indeed there was a typo in e-book-backend-file.c:do_create(), instead of:

   if (perror && !perror)

it should read:

   if (db_error && !perror)

Also I corrected the return status so that code now reads:

   if (db_error && !perror)
      db_error_to_gerror (db_error, perror);

   return db_error == 0 && status != STATUS_ERROR;

The commit on openismus-work-master for this patch is now:

commit d0180c59847351cef67f8f75882e09a4a6e9ae87
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Fri Jul 22 20:05:21 2011 -0400

    Make local addressbook backend store image data as URIs.
    
    Whenever the local address book receives a uri as a binary
    blob it proceeds to transform it to a uri accessbile on
    the system somewhere under $XDG_DATA_HOME. The local backend
    cleans up old photo uris when contacts are removed or modified
    to use new photos.
    
    Additionally, whenever it is detected that the user is
    cross-referencing a uri which belongs to another contact,
    a hard-link will be created to the addressbook owned file
    on disk and a new uri will be assigned to any additional
    contacts which try to share a uri owned by the addressbook.

Created attachment 192711 [details] [review]
Patch to store incomming binary photo blobs as local file uris [targetting meego-eds, take 3]

This patch is a remake of the whole patch on 'meego-eds' branch, it's just
an update to cover the bugfix in the last patch for master which Ross pointed
out.

Updated commit on 'openismus-work' branch is:

commit 5952ee44c1a9bac61bc91ae4084bc29191d725f1
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Fri Jul 15 19:00:51 2011 -0400

At this point I dont see a reason to split up the patch additionally
on the branch targetting meego-eds (should be good enough to backport
any applicable fixes which might result from the review in master).

... Unless there is a specific request for me to do the same on the 
other branch I'll leave it this way ...

Sorry for all the noise and the bookkeeping 2 patches on one bug report...

There is another detail I wanted to bring up for this patch,
you'll note that in the added function "maybe_delete_unused_uris()"
and some places we manually check the E_CONTACT_PHOTO and E_CONTACT_LOGO
fields.

However these code fragments should be iterative instead, but I did
not yet find the proper apis to do this:

for (field in contact.fields) {
   if (is_photo_type_field (field))
     do_something_with_this_contact_field (field);
}

I think something iterative like that can be done at the EVCard
level but it seems to work with attribute lists directly and not 'fields'
(and every contact field... *is* an attribute list if I understand right),
well... I'd be happy to change that part to something proper... what
would be the right api to do that ? (should it be an added iteration and
field type checking api to EContact ?)

Thanks for taking time to review this.

Created attachment 192713 [details] [review]
Patch to store incomming binary photo blobs as local file uris [targetting master, take 3]

Oops, I was sure I had compiled, but I somehow ended up with a bad merge
in that last commit.

This is the revised patch for master which depends on the minor refactor.

The revised commit id is:

commit 56521e482ad570c9722b07efad223afa7ff75ae0
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Fri Jul 22 20:05:21 2011 -0400

(In reply to comment #10)
> Created an attachment (id=192708) [details] [review]
> Minor refactor to local addressbook backend

Committed as part of fix for bug #656058

Created attachment 198959 [details] [review]
Patch to store incomming binary photo blobs as local file uris [targetting master, take 4]

Refreshing patches for huge rebase of 'openismus-work-master' off of
current master branch.

This one took a little bit more attention than the others because
of the new loops and transactions (create and modify are now done
in batches), but the test still runs fine and the backend seems to
do the correct cleanups (however this is only perceivable by
enabling the debugging in the file backend code and tracing the
file deletions).

In the end thanks to the refactoring already done in master,
this patch is slightly simplified in do_create() (but not by
very much).

The new related commits on 'openismus-work-master' are:

commit 2a20e869e5c401bc74fbe70198b2c5f1255c8539
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Fri Jul 22 20:06:38 2011 -0400

    Added test case showing photo data stored as uris.....

commit 690049eef13aa84487d43a397e226f09e611fb7e
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Fri Jul 22 20:05:21 2011 -0400

    Make local addressbook backend store image data as URIs....

Thanks for the update. My comments follow:
a) please mark string used in g_propagate_error() for translation, they can be shown in UI to a user

b) in safe_name_for_photo() and any functions which are calling it, what about encapsulate e_filename_mkdir_encoded() there? It seems to me like the right function for these things, especially when it does what you are doing there and a little bit more.

c) in hard_link_photo(), it's using 'link', I'm afraid of portability of this, especially on Windows - can there be used other approach?

d) use e_contact_photo_new() instead of g_slice_new0 (EContactPhoto); I know you free it immediately, but it still seems suspicious on the first look. Other option is to use a stack-allocated structure - I  know it's used in calendar code quite often for certain cases.

e) I noticed you are using strcmp all around, what about using case insensitive compares? The strcmp is fine for file names, at least on Linux, though the uris aren't case sensitive in general.

f) in do_create() the "if (db_error && !perror)" isn't doing what you want, I suppose, because 'perror' is 'GError **', not 'GError *'. You can define a GError *local_error = NULL; inside the function and use it instead of perror and at the end (or before each function's return) propagate the local_error into the perror.

Otherwise looks good, even the chunks around e_book_backend_file_modify_contact() are obsolete now, since the change to modify_contacts. I'm sorry for that.

Is the 'take 3' obsolete now?

take 3 is to target the meego-eds branch.

I'm not personally aware about the status of that branch, I suspect
these meego-eds patches, if not applied, will be obsoleted after a
switch to more recent EDS.

(In reply to comment #17)
> Thanks for the update. My comments follow:
> a) please mark string used in g_propagate_error() for translation, they can be
> shown in UI to a user
> 
> b) in safe_name_for_photo() and any functions which are calling it, what about
> encapsulate e_filename_mkdir_encoded() there? It seems to me like the right
> function for these things, especially when it does what you are doing there and
> a little bit more.
> 
> c) in hard_link_photo(), it's using 'link', I'm afraid of portability of this,
> especially on Windows - can there be used other approach?

We discussed the details of this on the ML, I originally wrote code
that micromanages the ownership of uris allowing multiple contacts
to share the same uri, in the end I believe it's much more desirable
to offload this to the operating system (basically another approach
pulls in a lot of code that can be avoided)... see the conversation
in this thread:
    http://mail.gnome.org/archives/evolution-hackers/2011-July/msg00032.html

On windows it depends how you compile, you can definitely create hardlinks
on a windows system but you might get an emulated link() from unistd.h
or you might use something like CreateHardLink():
http://msdn.microsoft.com/en-us/library/windows/desktop/aa363860%28v=vs.85%29.aspx

> 
> d) use e_contact_photo_new() instead of g_slice_new0 (EContactPhoto); I know
> you free it immediately, but it still seems suspicious on the first look. Other
> option is to use a stack-allocated structure - I  know it's used in calendar
> code quite often for certain cases.
> 
> e) I noticed you are using strcmp all around, what about using case insensitive
> compares? The strcmp is fine for file names, at least on Linux, though the uris
> aren't case sensitive in general.
> 
> f) in do_create() the "if (db_error && !perror)" isn't doing what you want, I
> suppose, because 'perror' is 'GError **', not 'GError *'. You can define a
> GError *local_error = NULL; inside the function and use it instead of perror
> and at the end (or before each function's return) propagate the local_error
> into the perror.
> 
> Otherwise looks good, even the chunks around
> e_book_backend_file_modify_contact() are obsolete now, since the change to
> modify_contacts. I'm sorry for that.

Created attachment 199074 [details] [review]
Patch to store incomming binary photo blobs as local file uris [targetting master, take 5]

Here's an updated patch which addresses all the previous comments:
  - insensitive string comparisons on uris
  - use e_filename_mkdir_encoded() where applicable
  - _("translatable messages in g_propagate_error")
  - use e_contact_photo_new()/e_contact_photo_free()
  - The "if (db_error && !perror)" code is fixed with
    "if (db_error == 0 && status != STATUS_ERROR)"

Except for using a different approach for link()... it's a lot
of complexity to micromanage the uris owned by the file backend
and share them among contacts (and I'm not sure you really prefer
adding that code the backend).

(In reply to comment #19)
> take 3 is to target the meego-eds branch.
> 
> I'm not personally aware about the status of that branch, I suspect
> these meego-eds patches, if not applied, will be obsoleted after a
> switch to more recent EDS.

Please focus on master and mark older versions of the patch for other branches as obsolete.

(In reply to comment #21)
> Except for using a different approach for link()... it's a lot
> of complexity to micromanage the uris owned by the file backend
> and share them among contacts (and I'm not sure you really prefer
> adding that code the backend).

I see. Thanks for the reminder.

Hmm, the patch doesn't apply cleanly to git master of eds as of today, I get:
> 4 out of 25 hunks FAILED -- saving rejects to
> file addressbook/backends/file/e-book-backend-file.c.rej

The last change on the file is from 2011-10-11. I'll test this probably tomorrow.

I see I overlooked/didn't-care-of few things, namely:
a) in collect_uri_for_field() the e_contact_get() returns newly allocated memory, thus it should be freed with e_contact_photo_free(); Smae in maybe_transform_vcard_field_for_photo() for the 'old_photo' when not NULL.

b) in the e_book_backend_file_remove(), in the newly added g_warning() should not be used bf->priv->dirname, but bf->priv->photo_dirname.

c) I got double-free when I'm trying to copy a contact with a photo form one local book into another. This is an old contact, and it has the photo included inline.

==16748== Thread 4:
==16748== Invalid free() / delete / delete[]
==16748==    at 0x4A0662E: free (vg_replace_malloc.c:366)
==16748==    by 0x356F84B7F2: g_free (gmem.c:263)
==16748==    by 0x1B29B518: maybe_transform_vcard_field_for_photo (e-book-backend-file.c:512)
==16748==    by 0x1B29B7BC: maybe_transform_vcard_for_photo (e-book-backend-file.c:612)
==16748==    by 0x1B29BE6A: do_create (e-book-backend-file.c:781)
==16748==    by 0x1B29C19B: e_book_backend_file_create_contacts (e-book-backend-file.c:862)
==16748==    by 0x4C2A5DD: e_book_backend_sync_create_contacts (e-book-backend-sync.c:91)
==16748==    by 0x4C2CD83: book_backend_create_contacts (e-book-backend-sync.c:498)
==16748==    by 0x4C2E79E: e_book_backend_create_contacts (e-book-backend.c:462)
==16748==    by 0x4C30E47: operation_thread (e-data-book.c:157)
==16748==    by 0x356F86C757: g_thread_pool_thread_proxy (gthreadpool.c:319)
==16748==    by 0x356F86A235: g_thread_create_proxy (gthread.c:1962)
==16748==    by 0x356DC07D30: start_thread (in /lib64/libpthread-2.14.90.so)
==16748==    by 0x356D8EFDFC: clone (in /lib64/libc-2.14.90.so)
==16748==  Address 0x1d8e5a30 is 0 bytes inside a block of size 128 free'd
==16748==    at 0x4A0662E: free (vg_replace_malloc.c:366)
==16748==    by 0x356F84B7F2: g_free (gmem.c:263)
==16748==    by 0x4E7CB9C: e_contact_photo_free (e-contact.c:2089)
==16748==    by 0x1B29B50C: maybe_transform_vcard_field_for_photo (e-book-backend-file.c:509)
==16748==    by 0x1B29B7BC: maybe_transform_vcard_for_photo (e-book-backend-file.c:612)
==16748==    by 0x1B29BE6A: do_create (e-book-backend-file.c:781)
==16748==    by 0x1B29C19B: e_book_backend_file_create_contacts (e-book-backend-file.c:862)
==16748==    by 0x4C2A5DD: e_book_backend_sync_create_contacts (e-book-backend-sync.c:91)
==16748==    by 0x4C2CD83: book_backend_create_contacts (e-book-backend-sync.c:498)
==16748==    by 0x4C2E79E: e_book_backend_create_contacts (e-book-backend.c:462)
==16748==    by 0x4C30E47: operation_thread (e-data-book.c:157)
==16748==    by 0x356F86C757: g_thread_pool_thread_proxy (gthreadpool.c:319)
==16748==    by 0x356F86A235: g_thread_create_proxy (gthread.c:1962)
==16748==    by 0x356DC07D30: start_thread (in /lib64/libpthread-2.14.90.so)
==16748==    by 0x356D8EFDFC: clone (in /lib64/libc-2.14.90.so)

Out of curiosity, these photos are stored in a private evolution's directory. What if I want to send such contact to someone else? I do not see any special functionality to provide the contact's photo inlined in these cases, also because there is no way to distinguish on the backend side. Would it make any sense to provide such contacts with URI photos for views only, and the rest functions will transform these photos to inlined? (I mean the contacts for which the URI will be pointing into private backend's directory.)

(In reply to comment #24)
> Out of curiosity, these photos are stored in a private evolution's directory.
> What if I want to send such contact to someone else?

This is orthogonal to adding these patches. Such a situation could occur already now, because non-inlined photos can be created also without the patch by using some other (hypothetical?) client.

Admittedly it doesn't happen when using only Evolution and I don't know whether there really is some app which does it.

> I do not see any special
> functionality to provide the contact's photo inlined in these cases, also
> because there is no way to distinguish on the backend side. Would it make any
> sense to provide such contacts with URI photos for views only, and the rest
> functions will transform these photos to inlined? (I mean the contacts for
> which the URI will be pointing into private backend's directory.)

Such an utility function would make sense. It could be done in libebook entirely on the client side.

Evolution should call it by default whenever it exports a contact as vCard. I cannot think of realistic use cases where one would prefer to export a URI to a local file instead of the data itself.

Comment on attachment 192711 [details] [review]
Patch to store incomming binary photo blobs as local file uris [targetting meego-eds, take 3]

Marking patches for meego-eds obsolete as requested in comment 22

(In reply to comment #25)
> Such an utility function would make sense. It could be done in libebook
> entirely on the client side.
> 
> Evolution should call it by default whenever it exports a contact as vCard. I
> cannot think of realistic use cases where one would prefer to export a URI to a
> local file instead of the data itself.

It might mean to change API for e_book_client_get_contact(), e_book_client_get_contacts() and e_book_client_get_view(), add a 'flags' parameter to these (and their _sync equivalents), with the current meaning of "prefer_inline_photos" or "prefer_uri_photos". Or add completely new API for this, but it's not usually done this way.

I checked evolution's code and when you save the whole address book, then it calls e_book_client_get_contacts_sync(), thus one can add there the flag to ensure the locally stored photos will be included inline, but when the selection from the view is saved then it is used directly, which means evolution would not benefit from this change, only to make sure the saved contacts will be saved directly. It also adds overhead on the factory side, to always encode photos when used from evolution. Thus my question is, whether DBus bandwidth worth these complications. (I know I should bring this earlier, but I realized only tomorrow.)

(In reply to comment #27)
> I checked evolution's code and when you save the whole address book, then it
> calls e_book_client_get_contacts_sync(), thus one can add there the flag to
> ensure the locally stored photos will be included inline, but when the
> selection from the view is saved then it is used directly, which means
> evolution would not benefit from this change, only to make sure the saved
> contacts will be saved directly. It also adds overhead on the factory side, to
> always encode photos when used from evolution. Thus my question is, whether
> DBus bandwidth worth these complications. (I know I should bring this earlier,
> but I realized only tomorrow.)

Sorry, I'm not sure I follow. When you say "evolution would not benefit from this change", do you mean inlining data inside the server with "this change"?

I don't think the ebook API should be made more complex by adding flags for inlining data inside the server or backends. I prefer a client-side EContact operation which replaces URIs with inlined data. Perhaps doing that server-side would save some CPU cycles (client needs to decode vCard, add data, encode again), but even that is not sure (if the server can return stored vCard data directly, then moving the re-encoding doesn't save anything).

(In reply to comment #28)
> Sorry, I'm not sure I follow. When you say "evolution would not benefit from
> this change", do you mean inlining data inside the server with "this change"?

With "this change" I meant "the transform from inline photos into uri photos on the backend side to make DBus replies with smaller bandwidth needed".

> I don't think the ebook API should be made more complex by adding flags for
> inlining data inside the server or backends.

I'm still not sure for the fix for the export case, thus worse case would be that the evolution will require photos inlined always, which is not the best thing. I agree with API changes being too complex, it was just the first idea.

> I prefer a client-side EContact
> operation which replaces URIs with inlined data. Perhaps doing that server-side
> would save some CPU cycles (client needs to decode vCard, add data, encode
> again), but even that is not sure (if the server can return stored vCard data
> directly, then moving the re-encoding doesn't save anything).

Yup, and that's the point. Having this client side only means that the postponed parsing on EContact will not work, hmm, which isn't a problem, because evolution is accessing contact attributes anyway. The question is how evolution knows that the uri the photo/logo points too is backend's private directory, not any other. Only if we'll have a rule that any file:// should be converted to inline photo/logo, in that case it's easily distinguishable and the only thing is to not miss any place where an export or attaching is done with the vCard itself (it's an export, drag&drop, copy/move to book, ...).

(In reply to comment #29)
> (In reply to comment #28)
> > I prefer a client-side EContact
> > operation which replaces URIs with inlined data. Perhaps doing that server-side
> > would save some CPU cycles (client needs to decode vCard, add data, encode
> > again), but even that is not sure (if the server can return stored vCard data
> > directly, then moving the re-encoding doesn't save anything).
> 
> Yup, and that's the point. Having this client side only means that the
> postponed parsing on EContact will not work, hmm, which isn't a problem,
> because evolution is accessing contact attributes anyway. The question is how
> evolution knows that the uri the photo/logo points too is backend's private
> directory, not any other. Only if we'll have a rule that any file:// should be
> converted to inline photo/logo, in that case it's easily distinguishable

That's indeed the rule that I would use. Even if the actual location is not in the backend directory, inlining the data is still needed when moving to a different device which doesn't have the file.

Attempts to determine whether the file can be accessed outside of the current host IMHO are bound to fail.

> and
> the only thing is to not miss any place where an export or attaching is done
> with the vCard itself (it's an export, drag&drop, copy/move to book, ...).

I see how this can be tricky.

OK, good, so if Tristan would add some
   void e_contact_convert_local_photos_to_inline (EContact *contact);
(I'm looking for a better name too), which will check PHOTO and LOGO attributes and if their URI will be file://, then it'll convert it to an inline photo, thus such function will be usable on any required place that easily, then I think we have this done.

A patch on evolution's side will be needed too, and if Tristan will not need, then I can create it myself (evo is quite huge, no need to waste Tristan's time on it - even I'm sure I will miss some places too ;))

What about third-party software? It might be covered by an announcement message on evolution-hackers list.

Created attachment 199697 [details] [review]
Patch to store incomming binary photo blobs as local file uris [targetting master, take 6]

Alright heres another attempt at this patch.

Fixed these issues from comment 24:

(In reply to comment #24)
> I see I overlooked/didn't-care-of few things, namely:
> a) in collect_uri_for_field() the e_contact_get() returns newly allocated
> memory, thus it should be freed with e_contact_photo_free(); Smae in
> maybe_transform_vcard_field_for_photo() for the 'old_photo' when not NULL.
>
> b) in the e_book_backend_file_remove(), in the newly added g_warning() should
> not be used bf->priv->dirname, but bf->priv->photo_dirname.
> 
> c) I got double-free when I'm trying to copy a contact with a photo form one
> local book into another. This is an old contact, and it has the photo included
> inline.

This last one was pretty obvious as well, the uri was freed as
a part of the contact and then again later on its own... a little
embarrassing all of these memory management related glitches.

(In reply to comment #31)
> OK, good, so if Tristan would add some
>    void e_contact_convert_local_photos_to_inline (EContact *contact);
> (I'm looking for a better name too), which will check PHOTO and LOGO attributes
> and if their URI will be file://, then it'll convert it to an inline photo,
> thus such function will be usable on any required place that easily, then I
> think we have this done.

The new patch adds this api:

/**
 * e_contact_inline_local_photos:
 * @contact: an #EContact
 * @error: the location to store any #GError which might occur
 *
 * Tries to modify any #EContactPhoto fields which are
 * stored on the local file system as type %E_CONTACT_PHOTO_TYPE_URI
 * to be inlined and stored as %E_CONTACT_PHOTO_TYPE_INLINED instead.
 *
 * Returns: %TRUE if there were no errors, upon error %FALSE is returned
 *    and @error is set.
 *
 * Since: 3.4
 */
gboolean
e_contact_inline_local_photos (EContact      *contact,
                               GError       **error);



The updated commit on openismus-work-master branch is:

commit 6066d46b95f2f04506eb7e6db33102fed58cc0a9
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Fri Jul 22 20:05:21 2011 -0400

    Make local addressbook backend store image data as URIs.

Why the patch doesn't apply cleanly against git master of eds?

patching file addressbook/backends/file/e-book-backend-file.c
Hunk #1 FAILED at 78.
Hunk #2 succeeded at 88 with fuzz 1 (offset 2 lines).
Hunk #3 succeeded at 123 (offset 2 lines).
Hunk #4 succeeded at 148 (offset 2 lines).
Hunk #5 succeeded at 171 (offset 2 lines).
Hunk #6 succeeded at 213 (offset 2 lines).
Hunk #7 succeeded at 751 (offset -68 lines).
Hunk #8 succeeded at 770 (offset -68 lines).
Hunk #9 succeeded at 813 (offset -68 lines).
Hunk #10 succeeded at 837 (offset -68 lines).
Hunk #11 succeeded at 914 (offset -70 lines).
Hunk #12 succeeded at 981 (offset -72 lines).
Hunk #13 succeeded at 1000 (offset -72 lines).
Hunk #14 succeeded at 1053 (offset -72 lines).
Hunk #15 succeeded at 1062 (offset -72 lines).
Hunk #16 succeeded at 1073 (offset -72 lines).
Hunk #17 succeeded at 1090 (offset -72 lines).
Hunk #18 FAILED at 1577.
Hunk #19 succeeded at 1565 (offset -79 lines).
Hunk #20 succeeded at 1753 (offset -81 lines).
Hunk #21 FAILED at 2057.
Hunk #22 succeeded at 2005 (offset -83 lines).
Hunk #23 succeeded at 2062 (offset -83 lines).
Hunk #24 FAILED at 2324.
Hunk #25 succeeded at 2333 (offset -93 lines).
4 out of 25 hunks FAILED -- saving rejects to file addressbook/backends/file/e-book-backend-file.c.rej
patching file addressbook/libebook/e-contact.c
patching file addressbook/libebook/e-contact.h

Please populate also EContactPhoto::data.inlined.mime_type, because otherwise e_contact_set makes it X-EVOLUTION-UNKNOWN, which is something we do not want. You can find a way how to check for the mime_type in evolution's source code, in e-util/e-util.c:e_util_guess_mime_type, where here you can also test with file contents.

(In reply to comment #33)
> Why the patch doesn't apply cleanly against git master of eds?
> 
> patching file addressbook/backends/file/e-book-backend-file.c
> Hunk #1 FAILED at 78.
> Hunk #2 succeeded at 88 with fuzz 1 (offset 2 lines).
> Hunk #3 succeeded at 123 (offset 2 lines).

That will be because there is a queue of patches sitting between
this patch and master.

Namely e_book_client_get_revision() and e_book_client_set_flags().

I'll take a look at the function e_util_guess_mime_type(), 
hopefully I can also use that to make a funner filename extension
when saving uris locally (funner than a .data file...).

And well, I can re-hash this patch so that it sits at the beginning
of the queue in 'openismus-work-master'... (of course then the other
patches wont apply until this one gets applied....)

(In reply to comment #35)
> (In reply to comment #33)
> > Why the patch doesn't apply cleanly against git master of eds?
>
> That will be because there is a queue of patches sitting between
> this patch and master.
> 
> Namely e_book_client_get_revision() and e_book_client_set_flags().

OK, if we have a certain order of patches, then they can be reviewed in that order, thus it'll be less pain for all of us. If you can give me list of bug numbers in that order, then maybe I can help on my side too. ;)

> I'll take a look at the function e_util_guess_mime_type(), 
> hopefully I can also use that to make a funner filename extension
> when saving uris locally (funner than a .data file...).

Thinking of it, when you are storing inline images into a uri, then they should have this mime_type filled already, thus what about saving them somewhere, rather than guess always (guess only when this information is not available when storing), and on the opposite conversion from local uri to inline photo the mime_type can be just read. What about encoding it into a file name, thus you'll have:
   UID_filedName_X.mimeType
where mimeType will be result of g_uri_escape_string(), which converts all bad symbols like dots, slashes and any filename letters into %XX string, thus when creating the inline photo you just take the string after the last dot (that why escape dots too) and pass it to g_uri_unescape_string(), thus you'll have the exactly same mime type like at the beginning. The only "issue" might be that the '%' signs will be encoded twice, the second time in the URI itself. But maybe that's not an issue at all.

The other option would be to add a parameter into the URI, but I'm not sure how well the g_filename_from_uri() copes with uris with parameters.

Created attachment 200271 [details] [review]
Patch to store incomming binary photo blobs as local file uris [targetting master, take 7]

Ok, here's an updated patch which rewinds the patch to the front of the list
so it should apply to master, as well as guessing the mime type as much as
possible both when storing the file on disk and when inlining them with
e_contact_inline_photos().

FWIW, the order of patches in 'openismus-work-master' right now are:
  o This patch
  o e_book_client_view_set_flags() (bug 652171, you reviewed it and
    I need to give it another shot)
  o e_book_client_get_revision() (bug 652175)
  o e_cal_client_get_revision() (bug 652177)

Rewinding this patch to the tip was not a problem and I think it's the
most complex of all the remaining patches (so I guess better deal with 
it sooner).

Thanks for the update. it applies cleanly now. When I compile it I receive one warning from compiler:
   e-book-backend-file.c: In function 'safe_name_for_photo':
   e-book-backend-file.c:375:9: warning: variable 'suffix' set but not used
Easy to fix. I would check for a non-empty value of photo->data.inlined.mime_type too, not only for != NULL, just in case.

You may copy extension from the src_filename in hard_link_photo() too, instead of loosing the information with hard-coded "data" extension.

Please increase the version number of LIBEBOOK_CURRENT in eds' configure.ac, then fix the above and commit it to master. Thanks.

Created attachment 200325 [details] [review]
evo patch

for evolution;

To learn evolution to understand URI Photo/Logo EContact elements. Please commit it after the commit on the eds part. Thanks in advance.

(In reply to comment #38)
> Thanks for the update. it applies cleanly now. When I compile it I receive one
> warning from compiler:
>    e-book-backend-file.c: In function 'safe_name_for_photo':
>    e-book-backend-file.c:375:9: warning: variable 'suffix' set but not used
> Easy to fix. I would check for a non-empty value of
> photo->data.inlined.mime_type too, not only for != NULL, just in case.

Ok, caught the problem with 'suffix' and checked that 'mime_type' is
not a zero length string here...

> You may copy extension from the src_filename in hard_link_photo() too, instead
> of loosing the information with hard-coded "data" extension.

Gotcha, caught the filename extension with strrchr() and using it
directly (or falling back to .data if no '.' found).

> Please increase the version number of LIBEBOOK_CURRENT in eds' configure.ac,
> then fix the above and commit it to master. Thanks.

Done.

Committed patch to master as:

commit e144f7b5b0aba6e7903c3c98c4db792fddbf9c50
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Fri Jul 22 20:05:21 2011 -0400

    Make local addressbook backend store image data as URIs.
    
And test case as:

commit d6f0b79e0dc54777a03cd530661e3141724ea0fb
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Fri Jul 22 20:06:38 2011 -0400

    Added test case showing photo data stored as uris.

(In reply to comment #39)
> Created an attachment (id=200325) [details] [review]
> evo patch
> 
> for evolution;
> 
> To learn evolution to understand URI Photo/Logo EContact elements. Please
> commit it after the commit on the eds part. Thanks in advance.

Done, committed this on evolution master as:

commit 02cbe55123aa9ee218ee6a33ffb19f7644110123
Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
Date:   Tue Nov 1 18:27:05 2011 -0400

    Teach Evolution about Photo/Logo EContact fields stored as URIs
    
    Committing Milan Crha's patch here at his request (bug 652178).

(In reply to comment #41)
> commit 02cbe55123aa9ee218ee6a33ffb19f7644110123
> Author: Tristan Van Berkom <tristan.van.berkom@gmail.com>
> Date:   Tue Nov 1 18:27:05 2011 -0400
> 
>     Teach Evolution about Photo/Logo EContact fields stored as URIs
> 
>     Committing Milan Crha's patch here at his request (bug 652178).

I use to:
   git commit --author "user <email>"
and preferred commit messages tight to bug reports are:
   Bug #652178 - Teach Evolution about Photo/Logo EContact fields stored as URIs
or without the pound sign, up to you (I use the pound, some evo devs not). Also the line may not be too long, say 72-80 letters max, though it's possible to provide it longer if needed. I think we do not have this written anywhere, though, thus no problem, I'm only mentioning it here.