After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 638281 - Add an EDS backend
Add an EDS backend
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: e-d-s backend
git master
Other All
: High enhancement
: Future
Assigned To: Raul Gutierrez Segales
Raul Gutierrez Segales
Depends on: 651458 651509 651722 651847 651946 652530 653548
Blocks: 633781
 
 
Reported: 2010-12-29 16:16 UTC by Marco Barisione
Modified: 2011-07-12 18:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
eds backend (120.45 KB, patch)
2011-01-05 01:53 UTC, Raul Gutierrez Segales
none Details | Review
updated tests to make them play nicely with the new backends (5.29 KB, patch)
2011-01-05 01:53 UTC, Raul Gutierrez Segales
none Details | Review
eds backend (120.72 KB, patch)
2011-01-05 15:35 UTC, Raul Gutierrez Segales
none Details | Review
eds backend (121.34 KB, patch)
2011-01-05 16:05 UTC, Raul Gutierrez Segales
none Details | Review
eds backend (121.82 KB, patch)
2011-01-05 18:29 UTC, Raul Gutierrez Segales
none Details | Review
add libebook and libedataserver VAPI files (49.35 KB, patch)
2011-01-06 13:40 UTC, Raul Gutierrez Segales
none Details | Review
eds backend (72.76 KB, patch)
2011-01-06 13:41 UTC, Raul Gutierrez Segales
none Details | Review
updated tests to make them play nicely with the new backends (5.29 KB, patch)
2011-01-06 13:43 UTC, Raul Gutierrez Segales
none Details | Review
eds backend (79.06 KB, patch)
2011-01-10 21:31 UTC, Raul Gutierrez Segales
none Details | Review
eds backend (77.54 KB, patch)
2011-01-11 19:13 UTC, Raul Gutierrez Segales
none Details | Review
e-d-s backend implementation (75.96 KB, patch)
2011-01-27 05:25 UTC, Raul Gutierrez Segales
none Details | Review
updated tests to make them play nicely with the new backends (5.03 KB, patch)
2011-01-27 05:27 UTC, Raul Gutierrez Segales
reviewed Details | Review
add (updated) libebook and libedataserver VAPI files (49.54 KB, patch)
2011-02-07 19:58 UTC, Raul Gutierrez Segales
none Details | Review
e-d-s backend implementation (80.17 KB, patch)
2011-02-07 20:08 UTC, Raul Gutierrez Segales
needs-work Details | Review
updated tests to make them play nicely with the new backend (5.04 KB, application/octet-stream)
2011-02-07 20:09 UTC, Raul Gutierrez Segales
  Details
updated tests to make them play nicely with the new backend (5.04 KB, patch)
2011-02-07 20:11 UTC, Raul Gutierrez Segales
none Details | Review
updated tests to make them play nicely with the new backend (5.04 KB, patch)
2011-02-07 20:11 UTC, Raul Gutierrez Segales
reviewed Details | Review
Readonly support for notes (2.28 KB, patch)
2011-06-08 20:35 UTC, Alexander Larsson
none Details | Review
Update to build with latest API (901 bytes, patch)
2011-06-10 12:46 UTC, Alexander Larsson
none Details | Review
Fix the use of FieldDetails parameters for emails (6.00 KB, patch)
2011-06-10 12:46 UTC, Alexander Larsson
none Details | Review
Initial implementation of an e-d-s backend (192.49 KB, patch)
2011-06-12 19:31 UTC, Raul Gutierrez Segales
needs-work Details | Review
Initial implementation of an e-d-s backend (192.88 KB, patch)
2011-06-13 07:30 UTC, Raul Gutierrez Segales
none Details | Review
Pass null to E.ContactPhoto.get_inlined() size out argument (940 bytes, patch)
2011-06-13 08:35 UTC, Alexander Larsson
none Details | Review
Append to the writable url set, not the readonly one (888 bytes, patch)
2011-06-13 08:35 UTC, Alexander Larsson
none Details | Review
Implenment UrlDetails in Edsf.Persona (1.16 KB, patch)
2011-06-13 08:35 UTC, Alexander Larsson
none Details | Review
Initial implementation of an e-d-s backend (193.86 KB, patch)
2011-06-13 10:03 UTC, Raul Gutierrez Segales
needs-work Details | Review
Initial implementation of an e-d-s backend (193.68 KB, patch)
2011-06-14 10:52 UTC, Raul Gutierrez Segales
none Details | Review

Description Marco Barisione 2010-12-29 16:16:30 UTC
To access and (in the future) store local contacts we need to implement an EDS backend. Raúl has an initial implementation of it at http://git.collabora.co.uk/?p=user/rgs/folks/.git;a=summary
Comment 1 Philip Withnall 2010-12-29 22:32:00 UTC
What changes to e-d-s does this depend on?
Comment 2 Raul Gutierrez Segales 2010-12-29 23:07:13 UTC
Basically a few annotations were needed to generate a complete GIR file that then could be fed to vapigen. No code changes were needed. 

Changes can be found here:

http://git.collabora.co.uk/?p=user/rgs/evolution-data-server/.git;a=summary

I haven't tried to upstream them yet since e-d-s is undergoing an re-write now so I wanted to wait until the whole things settles. But in the meanwhile, to avoid blocking on them, I bundled the generated VAPIs with the backend. 

Is that even close to being acceptable?
Comment 3 Marco Barisione 2010-12-30 16:23:10 UTC
I know this is a work in progress, but I have some quick comments on the branch.

Don't just concatenate path with +, see g_build_filename(). Testing for the existence of a file/directory and then creating it is not really nice. You could just use g_mkdir_with_parents.

The trust level of a local address book should be full. You put data in it and usually people mostly trust themselves.

Something like e_book_query_any_field_contains ("") should be the query you want to use to match every contact.

In _get_book_view, why do you need those casts of self and of the IDs?

I'm not sure if it's better to keep all the values coming from EDS on the Persona or if it's better to keep the EContact around and when you do something like persona.foo the getter of the "foo" property just call e_contact_get.

How can contact.get(E.Contact.field_id("family_name")) work? Shouldn't you just get the FN and N fields from the vcard and map them to the properties of the Names interface?

+          uint8[] v = {};
+          for (int i=0; i<p.photo_data_length; i++)
+            {
+              uint8 temp = (uint8) p.photo_data[i];
+              v += temp;
+            }

This is going to reallocate the array for every byte.
Comment 4 Philip Withnall 2010-12-30 18:45:51 UTC
In addition to Marco's comments:

From eds-persona-store.vala:

 141       // FIXME: There is no better error for this.
 142       throw new PersonaStoreError.UNSUPPORTED_ON_USER (
 143           "Personas cannot be added to this store.");

Add a new PersonaStoreError.READ_ONLY error code and use it instead.

 188               catch (GLib.Error e1)
 189                 {
 190                   stdout.printf("Couldn't open addressbook with URI: %s\n", e1.message);
 191                 }

Throw a suitable PersonaStoreError error instead.

Why are you using a queue instead of a list in contacts_added() and contacts_removed()?


From eds-persona.vala:

  33  * TBD interfaces:  Names,  ExtendedInfo

Looks like these have been implemented. :-)

  71       private set
  72         {
  73           this._postal_addresses = new GLib.List<PostalAddress> ();
  74           foreach (unowned PostalAddress pa in value)
  75             {
  76               this._postal_addresses.prepend (pa);
  77             }
  78           this._postal_addresses.reverse ();
  79         }

Couldn't you just do “this._postal_addresses = value.copy ()”? Same for phone numbers, e-mail addresses and URLs.

 190       string uid = this.build_uid ("folks", store.id, id);

The first parameter is supposed to be the backend name (“eds”).

Does e-d-s provide some way of determining which Eds.Persona is the user?

 241               emails.prepend (fd);

You never reverse the emails list before assigning it to this.emails.

 245       if (this.emails != emails)
 246         {
 247           this.emails = emails;
 248         }

I can't see how this check could ever fail.

 282       if (avatar_path != "")
 283         {
 284           var avatar_file = File.new_for_path (avatar_path);
 285           if (this.avatar != avatar_file)
 286             {
 287               this.avatar = avatar_file;
 288             }
 289         }

this.avatar doesn't get updated if avatar_path == "". I don't see how the check this.avatar != avatar_file could ever fail unless Vala magically converts it into this.avatar.equals(avatar_file).

 301           urls.prepend (new FieldDetails (u));

You never reverse the urls list before assigning it to this.urls.

 316       this._get_im_eds_map().foreach ((proto, proto_aliases) =>
 317           {

Is it possible to use foreach() here rather than a lambda function? It would be more efficient.

 332                       GLib.stdout.printf("Problem when trying to normalise address: %s\n", e.message);

This should be GLib.warning(…).

 356           if (FileUtils.test(photo_path, FileTest.EXISTS))
 357             {
 358               FileUtils.remove(photo_path);
 359             }

Is it necessary to check for existence before removing? Why not just try to remove the file and ignore any errors?

 403           GLib.List<string>  properties = new GLib.List<string> ();
 404           properties.append ("im_" + p + "_home_1");
 405           properties.append ("im_" + p + "_home_2");
 406           properties.append ("im_" + p + "_home_3");
 407           properties.append ("im_" + p + "_work_1");
 408           properties.append ("im_" + p + "_work_2");
 409           properties.append ("im_" + p + "_work_3");

It would be more efficient to prepend these in reverse order.

 429               urls.append(u);

It would be more efficient to prepend to urls and then reverse the list before returning.

 447               phones.prepend (fd);

You don't reverse the list before assigning it to this.phone_numbers.

 483               addresses.prepend (pa);

As above, the list isn't reversed before being assigned to this.postal_addresses.


From tests/eds/individual-retrieval.vala:

  29       /* Ignore the error caused by not running the logger */
  30       Test.log_set_fatal_handler ((d, l, m) =>
  31         {
  32           return !m.has_suffix ("couldn't get list of favourite individuals: " +
  33               "The name org.freedesktop.Telepathy.Logger was not provided by " +
  34               "any .service files");
  35         });

Since the telepathy backend is disabled, you shouldn't need this.


The backend loading changes look fine to me.

A couple of really picky points: the folks coding style specifies a space between function names and opening brackets (e.g. “strcmp (foo, bar)” rather than “strcmp(foo, bar)”), and I noticed a few places where there were two consecutive blank lines, which should be fixed.
Comment 5 Marco Barisione 2010-12-31 12:10:41 UTC
(In reply to comment #4)
> Add a new PersonaStoreError.READ_ONLY error code and use it instead.

That code is copied from the libsocialweb backend. The reason I didn't add it earlier is that I wanted to discuss writable things more. One of the options I saw was to have a different interface for writable backends. If not maybe juts modify the base class implementation to define this method returning an error.

> Why are you using a queue instead of a list in contacts_added() and
> contacts_removed()?

That's something that is done a lot in tp-glib and gabble too. GLists are evil and GLib.List are even more evil :)

>   71       private set
>   72         {
>   73           this._postal_addresses = new GLib.List<PostalAddress> ();
>   74           foreach (unowned PostalAddress pa in value)
>   75             {
>   76               this._postal_addresses.prepend (pa);
>   77             }
>   78           this._postal_addresses.reverse ();
>   79         }
> 
> Couldn't you just do “this._postal_addresses = value.copy ()”? Same for phone
> numbers, e-mail addresses and URLs.

No, it doesn't copy the values inside the list.

> Does e-d-s provide some way of determining which Eds.Persona is the user?

I think to remember it does, but you don't get the self user with a normal query.


Another problem is that photos can also be not inside the EContact, but just a URL pointing to the image file. Your code doesn't seem to handle it. It's probably fine for now just to add a FIXME and avoid crashing in that case.
Comment 6 Philip Withnall 2010-12-31 14:33:32 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > Add a new PersonaStoreError.READ_ONLY error code and use it instead.
> 
> That code is copied from the libsocialweb backend. The reason I didn't add it
> earlier is that I wanted to discuss writable things more. One of the options I
> saw was to have a different interface for writable backends. If not maybe juts
> modify the base class implementation to define this method returning an error.

I see. How about adding a PersonaStoreError.READ_ONLY error code for now, and if we end up adding a different interface for writeable backends, or majorly rewriting the current one, it can be removed again (along with a load of other API, presumably).

> > Why are you using a queue instead of a list in contacts_added() and
> > contacts_removed()?
> 
> That's something that is done a lot in tp-glib and gabble too. GLists are evil
> and GLib.List are even more evil :)

It might be better to use libgee's LinkedList implementation, since we're using libgee already.

> >   71       private set
> >   72         {
> >   73           this._postal_addresses = new GLib.List<PostalAddress> ();
> >   74           foreach (unowned PostalAddress pa in value)
> >   75             {
> >   76               this._postal_addresses.prepend (pa);
> >   77             }
> >   78           this._postal_addresses.reverse ();
> >   79         }
> > 
> > Couldn't you just do “this._postal_addresses = value.copy ()”? Same for phone
> > numbers, e-mail addresses and URLs.
> 
> No, it doesn't copy the values inside the list.

Oh, the prepend() call adds a reference to pa? That makes sense.

> > Does e-d-s provide some way of determining which Eds.Persona is the user?
> 
> I think to remember it does, but you don't get the self user with a normal
> query.

The backend will need to support exposing the user.
Comment 7 Travis Reitter 2011-01-03 18:35:43 UTC
(In reply to comment #6)
> The backend will need to support exposing the user.

Here's the relevant API:

http://library.gnome.org/devel/libebook/stable/EBook.html#e-book-get-self

e_book_get_self(), e_book_set_self(), e_book_is_self().

I'm not sure if it requires a special query, as Marco suggested, if you're using these functions. So it'd be good to double-check with the evolution developers.
Comment 8 Travis Reitter 2011-01-03 18:36:37 UTC
I'll help in the next round of review, to avoid a little duplicate work here.
Comment 9 Marco Barisione 2011-01-04 17:54:38 UTC
(In reply to comment #6)
> It might be better to use libgee's LinkedList implementation, since we're using
> libgee already.

But then I have to create a new GList from it. Or not?
Comment 10 Philip Withnall 2011-01-04 18:10:05 UTC
(In reply to comment #9)
> (In reply to comment #6)
> > It might be better to use libgee's LinkedList implementation, since we're using
> > libgee already.
> 
> But then I have to create a new GList from it. Or not?

Aargh. Good point. Ignore me.
Comment 11 Raul Gutierrez Segales 2011-01-05 01:51:44 UTC
(In reply to comment #3)
> I know this is a work in progress, but I have some quick comments on the
> branch.
> 
> Don't just concatenate path with +, see g_build_filename(). Testing for the
> existence of a file/directory and then creating it is not really nice. You
> could just use g_mkdir_with_parents.
Fixed.



> The trust level of a local address book should be full. You put data in it and
> usually people mostly trust themselves.
Maybe only for local and Gmails addressbooks? While not for ones that come from LDAP?

 
> Something like e_book_query_any_field_contains ("") should be the query you
> want to use to match every contact.
That isn't accepted by e-d-s. Trying with "(contains \"x-evolution-any-field\" \"\")". Also, according to the type of addressbooks (local, LDAP, etc) we might want to scope the query in the future.


> In _get_book_view, why do you need those casts of self and of the IDs?
Fixed, actually there is no need to get the book view async-cally (and the libebook VAPI file stills needs some work to get the signatures for callbacks correctly).


> I'm not sure if it's better to keep all the values coming from EDS on the
> Persona or if it's better to keep the EContact around and when you do something
> like persona.foo the getter of the "foo" property just call e_contact_get.

I also wonder whats more convenient.

 
> How can contact.get(E.Contact.field_id("family_name")) work? Shouldn't you just
> get the FN and N fields from the vcard and map them to the properties of the
> Names interface?
Oops. Fixed (mapping EContactName to the fields in StructuredName).

> 
> +          uint8[] v = {};
> +          for (int i=0; i<p.photo_data_length; i++)
> +            {
> +              uint8 temp = (uint8) p.photo_data[i];
> +              v += temp;
> +            }
> 
> This is going to reallocate the array for every byte.
Ooops. Fixed. 

Attaching two patches:

1) that approaches (hopefully) or your comments
2) a patch that updates the tests to make them place nicely with the new backends

I've also added a couple of simple tests for the e-d-s backend. 

Thanks for your patience and reviews guys!
Comment 12 Raul Gutierrez Segales 2011-01-05 01:53:16 UTC
Created attachment 177538 [details] [review]
eds backend 

I'll address the rest of the comments/needed-fixes in the next run.
Comment 13 Raul Gutierrez Segales 2011-01-05 01:53:53 UTC
Created attachment 177539 [details] [review]
updated tests to make them play nicely with the new backends

tests fail when adding new backends
Comment 14 Raul Gutierrez Segales 2011-01-05 15:32:23 UTC
(In reply to comment #4)
>  188               catch (GLib.Error e1)
>  189                 {
>  190                   stdout.printf("Couldn't open addressbook with URI:
> %s\n", e1.message);
>  191                 }
> 
> Throw a suitable PersonaStoreError error instead.
I am not sure which of the available PersonaStoreErrors (INVALID_ARGUMENT, CREATE_FAILED, UNSUPPORTED_ON_USER, STORE_OFFLINE) is the suitable one for this case. Going with INVALID_ARGUMENT but it might be misleading. Is PersonaStoreError the right error domain? In folks/persona-store.vala:

public abstract async void prepare () throws GLib.Error;


> From eds-persona.vala:
> 
>   33  * TBD interfaces:  Names,  ExtendedInfo
> 
> Looks like these have been implemented. :-)
Fixed. 


>  190       string uid = this.build_uid ("folks", store.id, id);
> 
> The first parameter is supposed to be the backend name (“eds”).
Fixed. 


> Does e-d-s provide some way of determining which Eds.Persona is the user?
It does as noticed by Travis but i have to check if our default query is  filtering this out. 

>  241               emails.prepend (fd);
> You never reverse the emails list before assigning it to this.emails.
> 
>  245       if (this.emails != emails)
>  246         {
>  247           this.emails = emails;
>  248         }
Fixed. 


> I can't see how this check could ever fail.
> 
>  282       if (avatar_path != "")
>  283         {
>  284           var avatar_file = File.new_for_path (avatar_path);
>  285           if (this.avatar != avatar_file)
>  286             {
>  287               this.avatar = avatar_file;
>  288             }
>  289         }
> 
> this.avatar doesn't get updated if avatar_path == "". I don't see how the check
> this.avatar != avatar_file could ever fail unless Vala magically converts it
> into this.avatar.equals(avatar_file).
Fixed. 


>  301           urls.prepend (new FieldDetails (u));
> 
> You never reverse the urls list before assigning it to this.urls.
Fixed. 

>  316       this._get_im_eds_map().foreach ((proto, proto_aliases) =>
>  317           {
> 
> Is it possible to use foreach() here rather than a lambda function? It would be
> more efficient.
Fixed.


>  332                       GLib.stdout.printf("Problem when trying to normalise
> address: %s\n", e.message);
> 
> This should be GLib.warning(…).
Fixed.

>  356           if (FileUtils.test(photo_path, FileTest.EXISTS))
>  357             {
>  358               FileUtils.remove(photo_path);
>  359             }
> 
> Is it necessary to check for existence before removing? Why not just try to
> remove the file and ignore any errors?
Changed. 

>  403           GLib.List<string>  properties = new GLib.List<string> ();
>  404           properties.append ("im_" + p + "_home_1");
>  405           properties.append ("im_" + p + "_home_2");
>  406           properties.append ("im_" + p + "_home_3");
>  407           properties.append ("im_" + p + "_work_1");
>  408           properties.append ("im_" + p + "_work_2");
>  409           properties.append ("im_" + p + "_work_3");
> 
> It would be more efficient to prepend these in reverse order.
Changed. 

>  429               urls.append(u);
> 
> It would be more efficient to prepend to urls and then reverse the list before
> returning.
Changed. 

>  447               phones.prepend (fd);
> 
> You don't reverse the list before assigning it to this.phone_numbers.
Fixed.

>  483               addresses.prepend (pa);
> 
> As above, the list isn't reversed before being assigned to
> this.postal_addresses.
Fixed.

> From tests/eds/individual-retrieval.vala:
> 
>   29       /* Ignore the error caused by not running the logger */
>   30       Test.log_set_fatal_handler ((d, l, m) =>
>   31         {
>   32           return !m.has_suffix ("couldn't get list of favourite
> individuals: " +
>   33               "The name org.freedesktop.Telepathy.Logger was not provided
> by " +
>   34               "any .service files");
>   35         });
> 
> Since the telepathy backend is disabled, you shouldn't need this.
Fixed. 

> A couple of really picky points: the folks coding style specifies a space
> between function names and opening brackets (e.g. “strcmp (foo, bar)” rather
> than “strcmp(foo, bar)”), and I noticed a few places where there were two
> consecutive blank lines, which should be fixed.
That should be mostly fixexd, let me know if i fixed one.
Comment 15 Raul Gutierrez Segales 2011-01-05 15:35:49 UTC
Created attachment 177573 [details] [review]
eds backend 

Address Phillip's observations.
Comment 16 Raul Gutierrez Segales 2011-01-05 16:04:43 UTC
(In reply to comment #5)
> Another problem is that photos can also be not inside the EContact, but just a
> URL pointing to the image file. Your code doesn't seem to handle it. It's
> probably fine for now just to add a FIXME and avoid crashing in that case.

Fixed: now we cover both cases (inlined or URI). This is still not so robust since i am not sure if e-d-s deals with other mime-types (i am assuming image/jpeg).
Comment 17 Raul Gutierrez Segales 2011-01-05 16:05:51 UTC
Created attachment 177580 [details] [review]
eds backend 

i've extended the patch to catch both the inlined and URI contact photos that come from e-d-s
Comment 18 Philip Withnall 2011-01-05 16:20:23 UTC
(In reply to comment #11)
> > The trust level of a local address book should be full. You put data in it and
> > usually people mostly trust themselves.
> Maybe only for local and Gmails addressbooks? While not for ones that come from
> LDAP?

That sounds reasonable. Add a whitelist of types of address books which are trusted by the backend.

> > I'm not sure if it's better to keep all the values coming from EDS on the
> > Persona or if it's better to keep the EContact around and when you do something
> > like persona.foo the getter of the "foo" property just call e_contact_get.
> 
> I also wonder whats more convenient.

I suppose it depends on the lifecycle of the pointers exposed by e-d-s. If we're notified every single time they're changed, we could just call e_contact_get() in the getter of the relevant property. If they're liable to change without us being notified, I guess we have to copy them.

Without having looked at the code, not copying them sounds better. We're already quite heavy on memory usage.

(In reply to comment #14)
> (In reply to comment #4)
> >  188               catch (GLib.Error e1)
> >  189                 {
> >  190                   stdout.printf("Couldn't open addressbook with URI:
> > %s\n", e1.message);
> >  191                 }
> > 
> > Throw a suitable PersonaStoreError error instead.
> I am not sure which of the available PersonaStoreErrors (INVALID_ARGUMENT,
> CREATE_FAILED, UNSUPPORTED_ON_USER, STORE_OFFLINE) is the suitable one for this
> case. Going with INVALID_ARGUMENT but it might be misleading. Is
> PersonaStoreError the right error domain? In folks/persona-store.vala:
> 
> public abstract async void prepare () throws GLib.Error;

PersonaStoreError is the correct domain; the signature for prepare() is just being a little vague (we can't tighten it up without breaking API).

I guess the error code which is used depends on the error returned by e-d-s. INVALID_ARGUMENT would be fine for errors caused by the book URI being invalid. STORE_OFFLINE might be applicable in the case that the view can't be created due to it being an LDAP address book and the machine being offline (or something). For any other cases, we might have to create new error codes in PersonaStoreError.

Thanks!
Comment 19 Raul Gutierrez Segales 2011-01-05 17:05:37 UTC
(In reply to comment #18)
> > > I'm not sure if it's better to keep all the values coming from EDS on the
> > > Persona or if it's better to keep the EContact around and when you do something
> > > like persona.foo the getter of the "foo" property just call e_contact_get.
> > 
> > I also wonder whats more convenient.
> 
> I suppose it depends on the lifecycle of the pointers exposed by e-d-s. If
> we're notified every single time they're changed, we could just call
> e_contact_get() in the getter of the relevant property. If they're liable to
> change without us being notified, I guess we have to copy them.
> 
> Without having looked at the code, not copying them sounds better. We're
> already quite heavy on memory usage.

If you take a look at the _update* methods in backend/eds/eds-persona.vala you'll see there is a decent amount of computation to transform the data coming from e-d-s to the format in which the persona interfaces/properties are defined. We should consider the overhead cost of doing that on every property request.

Regards the life cycle of contacts pointers given to us by e-d-s, we depend on the following signals from BookView:

    public virtual signal void contacts_added (GLib.List<weak void*> contacts);
    public virtual signal void contacts_changed (GLib.List<weak void*> contacts);
    public virtual signal void contacts_removed (GLib.List<weak void*> ids);
Comment 20 Raul Gutierrez Segales 2011-01-05 18:29:12 UTC
Created attachment 177602 [details] [review]
eds backend 

Refreshed the patch with 2 little enhancements:

1) if the address book is writable the trust level is FULL. If it isn't we set the trust level to PARTIAL.

2) we set the is-user property to true if the contact is the user of the address book
Comment 21 Raul Gutierrez Segales 2011-01-06 13:40:59 UTC
Created attachment 177653 [details] [review]
add libebook and libedataserver VAPI files

I am breaking up the e-d-s backend patch into 2 commits: 1) the needed e-d-s VAPI files which in the future might be shipped with e-d-s' libs 2) the actual e-d-s backend.

Sorry for the noise, hopefully this'll make reviews easier.
Comment 22 Raul Gutierrez Segales 2011-01-06 13:41:54 UTC
Created attachment 177654 [details] [review]
eds backend 

e-d-s backend without libebook and libedataserver VAPI files
Comment 23 Raul Gutierrez Segales 2011-01-06 13:43:40 UTC
Created attachment 177655 [details] [review]
updated tests to make them play nicely with the new backends

last patch of the updated patch set.
Comment 24 Travis Reitter 2011-01-07 00:13:16 UTC
(In reply to comment #16)
> (In reply to comment #5)
> > Another problem is that photos can also be not inside the EContact, but just a
> > URL pointing to the image file. Your code doesn't seem to handle it. It's
> > probably fine for now just to add a FIXME and avoid crashing in that case.
> 
> Fixed: now we cover both cases (inlined or URI). This is still not so robust
> since i am not sure if e-d-s deals with other mime-types (i am assuming
> image/jpeg).

Last I remember, it hard-coded support for image/jpeg and image/png. Please double-check this (in the code, if necessary).
Comment 25 Travis Reitter 2011-01-07 20:08:34 UTC
This review is just for the eds backend itself - I'll review the tests in a following review. These may keep you busy for a while anyhow :)

And we'll need to do a little more work after we finalize the pending interfaces that your branch is based upon, but I think we're making good progress.
===================
A rather major change I think you should make is to split the backend into two libraries, as we have for the Telepathy backend. One is just a thin GModule that contains the Backend and BackendFactory. This will be installed in the backend directory. The "feature" library contains the core functionality. The GModule library links to the feature library and any client that wants to take advantage of EDS-specific functionality will also link against this feature library.

This sort of separation lets us keep libfolks as simple as possible while still exposing backend-specific functionality through their feature libraries.

+       -DG_LOG_DOMAIN=\"LibEBook\" \

+  public override string name { get { return "eds"; } }

I recently factored out strings like this and made them the same for each backend/core. See these commits:

http://git.gnome.org/browse/folks/commit/?id=7b39b7188be6bf37925f76615c9004c5151e543c
http://git.gnome.org/browse/folks/commit/?id=cfdd138a035c6aa13889660852e13b2fb458dbd5
http://git.gnome.org/browse/folks/commit/?id=cf95d8dcce95840188c510d0f1cfe78af7a44ecc


eds-backend-factory.vala should rename backend_factory to _backend_factory, since it's private.


+              string use_address_book = Environment.get_variable ("FOLKS_BACKEND_EDS_ADDRESSBOOK");

+                      if (use_address_book != null && use_address_book != "")
+                        {
+                          if (s.peek_name () == use_address_book)
+                            {
+                              add_addressbook(s);
+                            }
+                        }

I think this variable should be named something like FOLKS_BACKEND_EDS_USE_ADDRESSBOOKS and accept a list of address.

There are a number of places where there's a missing space between a function name and its parameter list or between the closing ) of a cast and its operand. You should be able to find most of them with "egrep '[a-zA-Z]\(|\)[a-zA-Z]' *.vala"

Also make sure your lines are < 80 characters wide.


+  public override async Folks.Persona? add_persona_from_details (

+      // FIXME: There is no better error for this.
+      throw new PersonaStoreError.UNSUPPORTED_ON_USER (
+          "Personas cannot be added to this store.");

+  public override async void remove_persona (Folks.Persona persona)

+      // FIXME: There is no better error for this.
+      throw new PersonaStoreError.UNSUPPORTED_ON_USER (
+          "Personas cannot be removed from this store.");

As discussed previously on this bug, add READ_ONLY to PersonaStoreError. It would be best to move this commit before the initial eds backend commit and then modify the backend commit to use this new error here.


+  private const string[] _linkable_properties = {};

This should have at least "im-addresses" and "emails" (though this should be "email-addresses" for consistency, but I'll that's an issue for bug #638279; it has to match the property name, so your code should match whatever name it ends up having).


+      string uid = this.build_uid ("eds", store.id, id);

Another place to avoid a magic string.

+      this._update_avatar(contact);
+      this._update_avatar(contact);

We probably don't need to do this twice in a row :)


+   * FIXME: save contact as an instance var and then call all the others.

Agreed. I don't think it needs to be passed in as an argument, and we'll certainly want an accessor for it (for client programs, if they want to take advantage of EContact-specific functionality)

+  public void update (E.Contact contact)

This should be internal instead of public (and thus renamed "_update").


+      HashTable<string, GLib.List<string>> im_eds_map = this._get_im_eds_map ()

This can be simplified (without a loss of type safety) as:

var im_eds_map = this._get_im_eds_map ();


+  private HashTable<string, GLib.List<string>> _get_im_eds_map()

This should just use an internal "singleton" (ie, build the table the first time this function is called and return the existing table every time after that).


+  /*
+   * We save images in ~/.cache/folks/avatars/$ID_OF_THE_PERSONA
+   */

I think the code is simple enough that someone reading it could determine this path. But more importantly, if GLib.Environment.get_user_cache_dir() ends up returning a different value at some point, this comment will be wrong until someone notices the problem and fixes it (which tends to be a long time, regardless of project). And, beyond that, it's configurable at run-time in Unix (with XDG_CACHE_HOME) and this path is completely different in Windows (see the documentation for g_get_user_cache_dir()).

+  private string _get_photo_path(E.Contact contact)
+    {
+      string filename = this.uid + ".jpg";

We can't hard-code .jpg here, since the photo stored for a contact can be PNG (or, theoretically or actually, another format).


+      if (p.type == ContactPhotoType.INLINED)
+        {
+          try
+            {
+              GLib.FileUtils.set_data (photo_path, (uint8 []) p.photo_data);

Use p.mime_type to determine the proper filename extension (I believe there's a GLib utility function to do that).


diff --git a/configure.ac b/configure.ac
...
+                     libxml-2.0])

Are you sure this is actually a direct requirement? (ie, do we actually use libxml Vala bindings ourselves?)


+BACKEND_SW='$(top_builddir)/backends/libsocialweb/.libs/libfolks-backend-libsoc
+AC_SUBST([BACKEND_SW])
+BACKEND_EDS='$(top_builddir)/backends/eds/.libs/libfolks-backend-eds.so'
+AC_SUBST([BACKEND_EDS])
 
 # All of the backend libraries in our tree; to be used by the tests
-BACKEND_UNINST_PATH='$(BACKEND_KF):$(BACKEND_TP)'
+BACKEND_UNINST_PATH='$(BACKEND_KF):$(BACKEND_TP):$(BACKEND_SW):$(BACKEND_EDS)'

The addition of the libsocialweb components to these variables should be done in a separate commit since it's unrelated.

Post-rebase changes
===================

Here are some changes that you should make after rebasing your branch upon the latest version of master. You may want to defer these changes a bit, since you're already based upon a few outstanding branches that are based on the older version of master.


I just pushed some changes to the way debugging output works, so you'll need to call Debug._register_domain() to redirect your debugging output. See the recent commits.

Please add a bug for each of the outstanding TODOs and FIXMEs that remain when this gets merged.
Comment 26 Travis Reitter 2011-01-08 01:35:23 UTC
The final commit in your public repo (updating the existing tests) looked fine. Here are my comments for the new eds backend tests:

+      c1.set(Folks.Backends.Eds.Persona.email_fields[0], "bernie@somedomain.org");

It's probably best to use an example.{org,net,com} domain, since those are guaranteed to not exist (in case we add tests where we send an email to our contacts, we don't want them to accidentally spam someone).


+      this.eds_backend.set_up("addressbook" + (Random.next_int() % 1000).to_string());

Tests need to be as consistent as possible, so don't use pseudo-random numbers here. If there were a good reason to use a bunch of pseudo-random data (eg, for fuzzing), we'd want to at least provide a consistent seed to begin with.

In this case, it would be good to use the name of the test (factor it out to avoid having magic strings).


+      // BROKEN: haven't figure how to instantiate EContactAddress
+      //this.add_test ("postal addresses interface", this.test_postal_addresses);

If you don't have a fix for this yet, just cut it out of this commit and add it in a latter commit. I'd like to avoid merging commented-out code.


+  public void test_postal_addresses ()

I think this test should validate the results a little more carefully - eg, in the individuals-changed handler, iterate through every key/value in c1 and assert the equivalent detail is in the persona (otherwise remove the key/value pair from the hash). After the loop, assert that c1 is empty.

And the same for the other tests.

Otherwise, if we introduced some bug that dropped/corrupted a specific address field, we probably wouldn't notice.


+      c3.set(Folks.Backends.Eds.Persona.email_fields[0], "barack@whitehouse.gov");

I know this is sometimes used for examples, but moreso than the somedomain.org addresses, I'd like to avoid accidentally sending spam here :)


+  void test_removal ()

As long as the e-d-s backend is read-only, this test should confirm that removing a contact results in a PersonaStoreError.READ_ONLY error, not that it was successful (since it shouldn't be).


+  void test_updates ()

Same here - this should confirm PersonaStoreError.READ_ONLY is thrown when it tries to change the persona's full name. The first pass of this backend should be read-only for simplicity. We can still wait to merge the branch until it's read-write if you'd like, but I think we should save writability for later commits (since this is a pretty huge commit as it is).


+public class EdsTest.Backend
+{
+  private string addressbook_name;
+  private E.Book addressbook;
+  private GLib.List<E.Contact> e_contacts;

These should be prepended with a _, since they're private.


+  private void _prepare_addressbook () throws BackendSetupError
+    {

+          this.addressbook.open (false);

Make this function async and use open_async (). Even in tests, we should prefer async functions (since that's how we should be doing it behind the scenes as well).

Same for all the instances of commit, add_contact, remove, etc. (And it's even more important that we do so in the backend itself).


+  private void _set_contact_fields (E.Contact contact, Gee.HashMap<string, stri

Cut the commented code in this function


+          if (k.length > 12 && k.slice(0, 12) == "contact_name")

This seems a bit fragile - is there any way you can determine the number instead of hardcoding it? If you do need to hard-code it for some reason, please define it as a constant with a name that explains its meaning.
Comment 27 Travis Reitter 2011-01-08 01:38:01 UTC
As mentioned in the review for the tests, please ensure all the libebook and libedataserver functions we use are the async version (this includes at least Addressbook.open, .add_contact, .remove_contact, .commit, .remove, etc.).

Folks is designed to never block on I/O (which can certainly block a long time in the case of e-d-s (particularly with an LDAP backend)), so we need to make sure that, eg, Eds.PersonaStore.prepare() (which is an async function) doesn't block waiting on E.Addressbook.open().
Comment 28 Raul Gutierrez Segales 2011-01-10 21:29:44 UTC
(In reply to comment #25)
> ===================
> A rather major change I think you should make is to split the backend into two
> libraries, as we have for the Telepathy backend. One is just a thin GModule
> that contains the Backend and BackendFactory. This will be installed in the
> backend directory. The "feature" library contains the core functionality. The
> GModule library links to the feature library and any client that wants to take
> advantage of EDS-specific functionality will also link against this feature
> library.
> 
> This sort of separation lets us keep libfolks as simple as possible while still
> exposing backend-specific functionality through their feature libraries.

Done.

> +       -DG_LOG_DOMAIN=\"LibEBook\" \
> 
> +  public override string name { get { return "eds"; } }
> 
> I recently factored out strings like this and made them the same for each
> backend/core. See these commits:
> 
> http://git.gnome.org/browse/folks/commit/?id=7b39b7188be6bf37925f76615c9004c5151e543c
> http://git.gnome.org/browse/folks/commit/?id=cfdd138a035c6aa13889660852e13b2fb458dbd5
> http://git.gnome.org/browse/folks/commit/?id=cf95d8dcce95840188c510d0f1cfe78af7a44ecc

Done. 


> eds-backend-factory.vala should rename backend_factory to _backend_factory,
> since it's private.
Done.


> +              string use_address_book = Environment.get_variable
> ("FOLKS_BACKEND_EDS_ADDRESSBOOK");
> 
> +                      if (use_address_book != null && use_address_book != "")
> +                        {
> +                          if (s.peek_name () == use_address_book)
> +                            {
> +                              add_addressbook(s);
> +                            }
> +                        }
> 
> I think this variable should be named something like
> FOLKS_BACKEND_EDS_USE_ADDRESSBOOKS and accept a list of address.
Done. I've also made it a class constant to avoid accumulating magic strings.


> There are a number of places where there's a missing space between a function
> name and its parameter list or between the closing ) of a cast and its operand.
> You should be able to find most of them with "egrep '[a-zA-Z]\(|\)[a-zA-Z]'
> *.vala"
Done.


> Also make sure your lines are < 80 characters wide.
Done.


> +  public override async Folks.Persona? add_persona_from_details (
> 
> +      // FIXME: There is no better error for this.
> +      throw new PersonaStoreError.UNSUPPORTED_ON_USER (
> +          "Personas cannot be added to this store.");
> 
> +  public override async void remove_persona (Folks.Persona persona)
> 
> +      // FIXME: There is no better error for this.
> +      throw new PersonaStoreError.UNSUPPORTED_ON_USER (
> +          "Personas cannot be removed from this store.");
> 
> As discussed previously on this bug, add READ_ONLY to PersonaStoreError. It
> would be best to move this commit before the initial eds backend commit and
> then modify the backend commit to use this new error here.
Done. 



> +  private const string[] _linkable_properties = {};
> 
> This should have at least "im-addresses" and "emails" (though this should be
> "email-addresses" for consistency, but I'll that's an issue for bug #638279; it
> has to match the property name, so your code should match whatever name it ends
> up having).
Done. 


> +      string uid = this.build_uid ("eds", store.id, id);
> 
> Another place to avoid a magic string.
Done. 

> +      this._update_avatar(contact);
> +      this._update_avatar(contact);
> 
> We probably don't need to do this twice in a row :)
Oops. Fixed. 


> +   * FIXME: save contact as an instance var and then call all the others.
> 
> Agreed. I don't think it needs to be passed in as an argument, and we'll
> certainly want an accessor for it (for client programs, if they want to take
> advantage of EContact-specific functionality)
Done.


> +  public void update (E.Contact contact)
> 
> This should be internal instead of public (and thus renamed "_update").
Done.

> +      HashTable<string, GLib.List<string>> im_eds_map = this._get_im_eds_map
> ()
> 
> This can be simplified (without a loss of type safety) as:
> 
> var im_eds_map = this._get_im_eds_map ();
Done.


> +  private HashTable<string, GLib.List<string>> _get_im_eds_map()
> 
> This should just use an internal "singleton" (ie, build the table the first
> time this function is called and return the existing table every time after
> that).
Done. 

> +  /*
> +   * We save images in ~/.cache/folks/avatars/$ID_OF_THE_PERSONA
> +   */
> 
> I think the code is simple enough that someone reading it could determine this
> path. But more importantly, if GLib.Environment.get_user_cache_dir() ends up
> returning a different value at some point, this comment will be wrong until
> someone notices the problem and fixes it (which tends to be a long time,
> regardless of project). And, beyond that, it's configurable at run-time in Unix
> (with XDG_CACHE_HOME) and this path is completely different in Windows (see the
> documentation for g_get_user_cache_dir()).
Agreed, comment is gone (it accidentally made it pass my initial tests). 



> +  private string _get_photo_path(E.Contact contact)
> +    {
> +      string filename = this.uid + ".jpg";
> 
> We can't hard-code .jpg here, since the photo stored for a contact can be PNG
> (or, theoretically or actually, another format).
> +      if (p.type == ContactPhotoType.INLINED)
> +        {
> +          try
> +            {
> +              GLib.FileUtils.set_data (photo_path, (uint8 []) p.photo_data);
> 
> Use p.mime_type to determine the proper filename extension (I believe there's a
> GLib utility function to do that).
Hmmm, looking but couldn't find it. I'll keep digging. 



> diff --git a/configure.ac b/configure.ac
> ...
> +                     libxml-2.0])
> 
> Are you sure this is actually a direct requirement? (ie, do we actually use
> libxml Vala bindings ourselves?)
libebook and libedataserver have public methods that take libxml stuff as parameters... Good enough reason?


> +BACKEND_SW='$(top_builddir)/backends/libsocialweb/.libs/libfolks-backend-
libsoc
> +AC_SUBST([BACKEND_SW])
> +BACKEND_EDS='$(top_builddir)/backends/eds/.libs/libfolks-backend-eds.so'
> +AC_SUBST([BACKEND_EDS])
> 
>  # All of the backend libraries in our tree; to be used by the tests
> -BACKEND_UNINST_PATH='$(BACKEND_KF):$(BACKEND_TP)'
> +BACKEND_UNINST_PATH='$(BACKEND_KF):$(BACKEND_TP):$(BACKEND_SW):$(BACKEND_EDS)'
> 
> The addition of the libsocialweb components to these variables should be done
> in a separate commit since it's unrelated.
Done.
Comment 29 Raul Gutierrez Segales 2011-01-10 21:31:05 UTC
Created attachment 177962 [details] [review]
eds backend
Comment 30 Raul Gutierrez Segales 2011-01-11 19:10:33 UTC
(In reply to comment #26)
> +      c1.set(Folks.Backends.Eds.Persona.email_fields[0],
> "bernie@somedomain.org");
> 
> It's probably best to use an example.{org,net,com} domain, since those are
> guaranteed to not exist (in case we add tests where we send an email to our
> contacts, we don't want them to accidentally spam someone).
Yup, changed. 


> +      this.eds_backend.set_up("addressbook" + (Random.next_int() %
> 1000).to_string());
> 
> Tests need to be as consistent as possible, so don't use pseudo-random numbers
> here. If there were a good reason to use a bunch of pseudo-random data (eg, for
> fuzzing), we'd want to at least provide a consistent seed to begin with.
> 
> In this case, it would be good to use the name of the test (factor it out to
> avoid having magic strings).
True, changed. 


> +      // BROKEN: haven't figure how to instantiate EContactAddress
> +      //this.add_test ("postal addresses interface",
> this.test_postal_addresses);
> 
> If you don't have a fix for this yet, just cut it out of this commit and add it
> in a latter commit. I'd like to avoid merging commented-out code.
Done. 


> +  public void test_postal_addresses ()
> 
> I think this test should validate the results a little more carefully - eg, in
> the individuals-changed handler, iterate through every key/value in c1 and
> assert the equivalent detail is in the persona (otherwise remove the key/value
> pair from the hash). After the loop, assert that c1 is empty.
> 
> And the same for the other tests.
> 
> Otherwise, if we introduced some bug that dropped/corrupted a specific address
> field, we probably wouldn't notice.
Done. 

> +      c3.set(Folks.Backends.Eds.Persona.email_fields[0],
> "barack@whitehouse.gov");
> 
> I know this is sometimes used for examples, but moreso than the somedomain.org
> addresses, I'd like to avoid accidentally sending spam here :)
Changed. 


> +  void test_removal ()
> 
> As long as the e-d-s backend is read-only, this test should confirm that
> removing a contact results in a PersonaStoreError.READ_ONLY error, not that it
> was successful (since it shouldn't be).
> 
> +  void test_updates ()
> 
> Same here - this should confirm PersonaStoreError.READ_ONLY is thrown when it
> tries to change the persona's full name. The first pass of this backend should
> be read-only for simplicity. We can still wait to merge the branch until it's
> read-write if you'd like, but I think we should save writability for later
> commits (since this is a pretty huge commit as it is).

These tests are actually aimed to test how the e-d-s backend reacts to updates and removal from the e-d-s side of the world not from Folks itself. Unless I am misunderstanding you... 

> +public class EdsTest.Backend
> +{
> +  private string addressbook_name;
> +  private E.Book addressbook;
> +  private GLib.List<E.Contact> e_contacts;
> 
> These should be prepended with a _, since they're private.
Done.

> +  private void _prepare_addressbook () throws BackendSetupError
> +    {
> 
> +          this.addressbook.open (false);
> 
> Make this function async and use open_async (). Even in tests, we should prefer
> async functions (since that's how we should be doing it behind the scenes as
> well).
> 
> Same for all the instances of commit, add_contact, remove, etc. (And it's even
> more important that we do so in the backend itself).

I'll take a go at making sure everything is async in my next iteration. Whatever isn't at the time was probably because I was struggling with getting the libebook VAPI file in good shape. Speaking of the e-d-s Vala bindings, who should we depend on to get those from (Vala?, e-d-s?) ?

> +  private void _set_contact_fields (E.Contact contact, Gee.HashMap<string,
> stri
> 
> Cut the commented code in this function
Done.

> +          if (k.length > 12 && k.slice(0, 12) == "contact_name")
> 
> This seems a bit fragile - is there any way you can determine the number
> instead of hardcoding it? If you do need to hard-code it for some reason,
> please define it as a constant with a name that explains its meaning.

I can by using "contact_name".length (which is what i've done for now). 
It would be nice if Vala would let you slice up strings regardless of their length. I.e.: "foobar".slice(0, 10000) ---> "foobar" 
instead of giving you null as it does now).
Comment 31 Raul Gutierrez Segales 2011-01-11 19:13:53 UTC
Created attachment 178080 [details] [review]
eds backend 

Updated tests. I need to make one final run to make sure every e-d-s operation is done in async fashion.
Comment 32 Travis Reitter 2011-01-13 16:56:25 UTC
(In reply to comment #28)
> > diff --git a/configure.ac b/configure.ac
> > ...
> > +                     libxml-2.0])
> > 
> > Are you sure this is actually a direct requirement? (ie, do we actually use
> > libxml Vala bindings ourselves?)
> libebook and libedataserver have public methods that take libxml stuff as
> parameters... Good enough reason?

Sure, that's a good reason. I just couldn't recall any places where that happened.
Comment 33 Travis Reitter 2011-01-14 00:14:12 UTC
Sorry, this turned out to be bad for distcheck (and I've adjusted for it in master):

+	-DBACKEND_NAME=\"$(BACKEND_NAME)\" \
+	-DG_LOG_DOMAIN=\"$(BACKEND_NAME)\" \

+-include backend.mk

Could you just define BACKEND_NAME in backends/eds/Makefile.am and backends/eds/lib/Makefile.am?

+  public static const string use_addressbooks =
+      "FOLKS_BACKEND_EDS_USE_ADDRESSBOOKS";

This should be named starting with a _

Because of that, it's currently shadowed by this definition:

+              string [] use_addressbooks = this._get_addressbooks_from_env ();


These strings in the tests could similarly be factored out (just within their own files or the test library):

+          Environment.set_variable ("FOLKS_BACKEND_EDS_USE_ADDRESSBOOKS",
+                                    this._addressbook_name, true);

+          Environment.set_variable ("FOLKS_BACKEND_EDS_USE_ADDRESSBOOKS", "", true);
Comment 34 Travis Reitter 2011-01-14 00:22:52 UTC
(In reply to comment #30)
...
> > 
> > Same here - this should confirm PersonaStoreError.READ_ONLY is thrown when it
> > tries to change the persona's full name. The first pass of this backend should
> > be read-only for simplicity. We can still wait to merge the branch until it's
> > read-write if you'd like, but I think we should save writability for later
> > commits (since this is a pretty huge commit as it is).
> 
> These tests are actually aimed to test how the e-d-s backend reacts to updates
> and removal from the e-d-s side of the world not from Folks itself. Unless I am
> misunderstanding you... 

I think the tests should test both sides of it (ie, both that we react correctly to e-d-s itself and that we pass along the proper errors). Feel free to start new additional tests if you don't want to cram it all into one file.

Though, like you said, we don't want to test Folks itself here (that's for the general tests).

> > +  private void _prepare_addressbook () throws BackendSetupError
> > +    {
> > 
> > +          this.addressbook.open (false);
> > 
> > Make this function async and use open_async (). Even in tests, we should prefer
> > async functions (since that's how we should be doing it behind the scenes as
> > well).
> > 
> > Same for all the instances of commit, add_contact, remove, etc. (And it's even
> > more important that we do so in the backend itself).
> 
> I'll take a go at making sure everything is async in my next iteration.
> Whatever isn't at the time was probably because I was struggling with getting
> the libebook VAPI file in good shape. Speaking of the e-d-s Vala bindings, who
> should we depend on to get those from (Vala?, e-d-s?) ?

(Just mentioning this here so we've got a record of it)

As discussed on IRC, the e-d-s Vala bindings should go within e-d-s itself (ideally). Certainly the gobject-introspection work should go directly into e-d-s.

Though for testing and development purposes (short-term) we can maintain and ship our own vapi for e-d-s.
Comment 35 Travis Reitter 2011-01-20 01:46:53 UTC
Raul, please see the latest attachment at bug #638279 and rebase your branch upon it.

Note that the e-d-s tests (at least in my branch) don't pass. And we'll need to split the main eds backend library from its GModule library (the way we do for the Telepathy backend).
Comment 36 Travis Reitter 2011-01-20 19:21:30 UTC
(In reply to comment #35)
> Raul, please see the latest attachment at bug #638279 and rebase your branch
> upon it.
> 
> Note that the e-d-s tests (at least in my branch) don't pass. And we'll need to
> split the main eds backend library from its GModule library (the way we do for
> the Telepathy backend).

I've got a slightly newer branch on that bug, in case you already started (re-rebasing would be trivial if you have). It also fixes the references for the e-d-s backend commits about which prior commits to squash them on.
Comment 37 Raul Gutierrez Segales 2011-01-27 04:24:24 UTC
(In reply to comment #33)
> Sorry, this turned out to be bad for distcheck (and I've adjusted for it in
> master):
> 
> +    -DBACKEND_NAME=\"$(BACKEND_NAME)\" \
> +    -DG_LOG_DOMAIN=\"$(BACKEND_NAME)\" \
> 
> +-include backend.mk
> 
> Could you just define BACKEND_NAME in backends/eds/Makefile.am and
> backends/eds/lib/Makefile.am?
> 

Done. 

> +  public static const string use_addressbooks =
> +      "FOLKS_BACKEND_EDS_USE_ADDRESSBOOKS";
> 
> This should be named starting with a _
> 
> Because of that, it's currently shadowed by this definition:
> 
> +              string [] use_addressbooks = this._get_addressbooks_from_env ();

Done (and made it private too).


> These strings in the tests could similarly be factored out (just within their
> own files or the test library):
> 
> +          Environment.set_variable ("FOLKS_BACKEND_EDS_USE_ADDRESSBOOKS",
> +                                    this._addressbook_name, true);
> 
> +          Environment.set_variable ("FOLKS_BACKEND_EDS_USE_ADDRESSBOOKS", "",
> true);

Done.
Comment 38 Raul Gutierrez Segales 2011-01-27 05:25:27 UTC
Created attachment 179411 [details] [review]
e-d-s backend implementation

Almost everything should be adhering to the reviews on this bug report. There might be a place or two that still need be to switch to their async counterparts though.
Comment 39 Raul Gutierrez Segales 2011-01-27 05:27:07 UTC
Created attachment 179412 [details] [review]
updated tests to make them play nicely with the new backends

Removed a few old references to libsocialweb stuff.
Comment 40 Philip Withnall 2011-01-29 13:18:41 UTC
Review of attachment 179411 [details] [review]:

::: backends/eds/lib/edsf-persona.vala
@@ +319,3 @@
+    }
+
+  private string _get_photo_path ()

Since this calls _save_photo(), this should be async (see below).

@@ +347,3 @@
+   *       If so, don't overwrite.
+   */
+  private bool _save_photo (string photo_path, E.ContactPhoto p)

This should be async.

@@ +355,3 @@
+          try
+            {
+              GLib.FileUtils.set_data (photo_path, (uint8 []) p.photo_data);

Use GIO's async methods here (g_file_replace_contents_async() should work).

@@ +370,3 @@
+              File src_file = File.new_for_uri (p.photo_uri);
+              File dst_file = File.new_for_path (photo_path);
+              src_file.copy (dst_file, FileCopyFlags.OVERWRITE);

This should also be async.
Comment 41 Philip Withnall 2011-01-29 13:19:28 UTC
Review of attachment 179412 [details] [review]:

Just a few whitespace issues.

::: tests/folks/backend-loading.vala
@@ +56,3 @@
       store.prepare.begin ((o, r) =>
         {
+          store.disable_backend("eds");

Needs to be a space before "(".

@@ +76,2 @@
               assert (backends_expected.size == 0);
+              store.enable_backend("eds");

Needs to be a space before "(".

::: tests/key-file/individual-retrieval.vala
@@ +51,3 @@
+          store.disable_backend("eds");
+          store.disable_backend("telepathy");
+          store.disable_backend("libsocialweb");

Needs to be a space before "(".

@@ +102,3 @@
+          store.disable_backend("eds");
+          store.disable_backend("telepathy");
+          store.disable_backend("libsocialweb");

Needs to be a space before "(".
Comment 42 Travis Reitter 2011-02-01 23:26:46 UTC
Add some more goal bugs for the 0.3.5 release.
Comment 43 Travis Reitter 2011-02-02 00:23:18 UTC
(Actually setting the new milestone for these bugs)
Comment 44 Raul Gutierrez Segales 2011-02-07 19:58:14 UTC
Created attachment 180329 [details] [review]
add (updated) libebook and libedataserver VAPI files

I updated the libebook vapi file to treat ContactPhoto as a struct since we have no constructor for it and we need an instance of it for tests.
Comment 45 Raul Gutierrez Segales 2011-02-07 20:08:20 UTC
Created attachment 180330 [details] [review]
e-d-s backend implementation

New updates:

* cleaned tests a little (everything should pass now!)
* made avatar handling async
* updated tests to match ESourceGroup's new URI format
* fixed missing spaces in a few places
Comment 46 Raul Gutierrez Segales 2011-02-07 20:09:09 UTC
Created attachment 180331 [details]
updated tests to make them play nicely with the new backend

Fixed missing spaces in a few places
Comment 47 Raul Gutierrez Segales 2011-02-07 20:11:02 UTC
Created attachment 180332 [details] [review]
updated tests to make them play nicely with the new backend

Fixed missing spaces in a few places
Comment 48 Raul Gutierrez Segales 2011-02-07 20:11:36 UTC
Created attachment 180333 [details] [review]
updated tests to make them play nicely with the new backend
Comment 49 Philip Withnall 2011-02-07 23:28:53 UTC
Review of attachment 180330 [details] [review]:

Lots of very minor comments. Sorry to keep putting you through this.

::: backends/eds/Makefile.am
@@ +37,3 @@
+	libedataserver-1.2 \
+	libxml-2.0 \
+	posix \

Is this still necessary?

@@ +57,3 @@
+	$(EDATASERVER_LIBS) \
+	$(LIBXML_LIBS) \
+	lib/libfolks-eds.la \

I think the .la libraries should be listed first (L[IB|D]ADD should be ordered from least to most general; see http://cygwin.ru/ml/automake/2003-10/msg00082.html).

::: backends/eds/eds-backend-factory.vala
@@ +31,3 @@
+
+/**
+ * The eds backend module entry point.

All the Valadoc comments in the backend need @since 0.3.UNRELEASED added.

::: backends/eds/eds-backend.vala
@@ +23,3 @@
+using GLib;
+using E;
+using Posix;

Is this still used?

@@ +42,3 @@
+
+  /**
+   * {@inheritDoc}

Same here; all the comments need “@since 0.3.UNRELEASED” added.

@@ +75,3 @@
+  /**
+   * {@inheritDoc}
+   *

Stray empty comment line.

@@ +83,3 @@
+          if (!this._is_prepared)
+            {
+              string [] use_addressbooks = this._get_addressbooks_from_env ();

Can you put the array brackets (“[]”) next to the type name (i.e. “string[]” instead of “string []”)?

@@ +98,3 @@
+                          if (s.peek_name () in use_addressbooks)
+                            {
+                              add_addressbook (s);

Should be “this._add_addressbook (s)”.

@@ +140,3 @@
+  /**
+   * Add a new addressbook connected to a Persona Store.
+   *

Stray empty comment line (and the comment needs a “@since” taglet).

@@ +142,3 @@
+   *
+   */
+  private void add_addressbook (E.Source s)

This should be “_add_addressbook” (underscores for private methods).

@@ +164,3 @@
+  private string [] _get_addressbooks_from_env ()
+    {
+      string [] addressbooks = {};

s/string []/string[]/

::: backends/eds/lib/Makefile.am
@@ +11,3 @@
+VAPIGENFLAGS += \
+	--vapidir=. \
+	--vapidir=$(top_srcdir)/folks

Please add a $(NULL) to the end of this list.

@@ +48,3 @@
+	--pkg libedataserver-1.2 \
+	--pkg libxml-2.0 \
+	--pkg posix \

Could you not use $(addprefix --pkg ,$(folks_backend_eds_deps))?

@@ +61,3 @@
+	libedataserver-1.2 \
+	libxml-2.0 \
+	posix \

Is this still needed?

@@ +78,3 @@
+	$(GEE_LIBS) \
+	$(TP_GLIB_LIBS) \
+	$(top_builddir)/folks/libfolks.la \

.la files should be listed first in LIBADD.

::: backends/eds/lib/edsf-persona-store.vala
@@ +45,3 @@
+   * The type of persona store this is.
+   *
+   * See {@link Folks.PersonaStore.type_id}.

Again, @since should be added to all these Valadoc comments.

@@ +54,3 @@
+   * See {@link Folks.PersonaStore.can_add_personas}.
+   *
+   * @since 0.3.1

This @since should become “@since 0.3.UNRELEASED” — the “UNRELEASED” will be replaced just before the next release.

@@ +198,3 @@
+  private void _book_view_cb
+    (E.Book book, E.BookStatus status, E.BookView book_view)
+  {

It looks like something's wrong with the indentation in this method, though it could just be my web browser.

@@ +207,3 @@
+    this._ebookview.start ();
+
+    this.trust_level = PersonaStoreTrust.PARTIAL;

Can we not detect the user's local address book and give it full trust (and all other address books partial trust)?

@@ +233,3 @@
+        {
+          string persona_id = this.id + ":" + (string) c.get
+              (E.Contact.field_id ("id"));

It might be an idea to factor out construction of the persona_id into a private method.

::: backends/eds/lib/edsf-persona.vala
@@ +25,3 @@
+using E;
+using Xml;
+using Posix;

Is this still used?

@@ +36,3 @@
+    Phoneable,
+    Emailable,
+    URLable,

Due to bug #641780, the URLable interface will be renamed to Urlable.

@@ +39,3 @@
+    GenderOwner
+{
+  public static const string [] phone_fields = {

s/string []/string[]/

@@ +188,3 @@
+   * Update attribs of the persona.
+   *
+   * FIXME: save contact as an instance var and then call all the others.

Does this FIXME still apply?

::: tests/eds/individual-retrieval.vala
@@ +5,3 @@
+public class IndividualRetrievalTests : Folks.TestCase
+{
+  private static const string TEST_SINGLETON_INDIVIDUALS_ADDREBOOK =

s/ADDREBOOK/ADDRESSBOOK/

::: tests/eds/persona-store-tests.vala
@@ +8,3 @@
+  "test_persona_store";
+  private EdsTest.Backend eds_backend;
+  private HashSet<string> group_flags_received;

We should probably follow the prefix-private-members-with-underscores convention in the tests too.

::: tests/lib/eds/Makefile.am
@@ +34,3 @@
+	$(GEE_LIBS) \
+	$(top_builddir)/folks/libfolks.la \
+	$(top_builddir)/backends/eds/libfolks-backend-eds.la \

.la files should be listed first.
Comment 50 Philip Withnall 2011-02-07 23:29:45 UTC
Review of attachment 180333 [details] [review]:

Looks good to commit once the backend's been committed.
Comment 51 Philip Withnall 2011-02-14 23:14:01 UTC
Punting to 0.3.7.
Comment 52 Alexander Larsson 2011-05-16 07:37:40 UTC
e-d-s master now has the introspection and vapi files added.
Comment 53 Raul Gutierrez Segales 2011-05-16 08:05:34 UTC
(In reply to comment #52)
> e-d-s master now has the introspection and vapi files added.

Yeah - though annotations need some work. I've pushed a couple of new/fixed annotations to libebook this weekend, I need to push some more for libedataserver and then rebase the e-d-s backend on top of master.
Comment 54 Raul Gutierrez Segales 2011-05-25 23:12:23 UTC
I am rebasing this on top of master here:

http://cgit.collabora.com/git/user/rgs/folks/log/?h=eds-0.5

I still need to:
- update the tests
- get rid of deprecated calls in e-d-s
- fight a couple rough edges with introspection support / Vala bindings in e-d-s
- general cleanups
Comment 55 Raul Gutierrez Segales 2011-05-26 10:52:06 UTC
Ok, the tests now compile too.

Hopefully I'll get some time this afternoon to get rid of deprecated e-d-s stuff, work with the Vala devs to figure out one last bug (# 651132) thats preventing e-d-s from generating 100% usable Vala bindings and do general testing. Ah, and implement a couple of missing interfaces.
Comment 56 Raul Gutierrez Segales 2011-05-29 21:47:08 UTC
Pushed more changes:

- all tests now pass
- updated to new e-d-s API. Mysteriously, the async version of get_book_view () for BookClient is broken. Got debug that, might be the same thing alexl got bitten by

Still missing:
- add the other interfaces (Notes, etc)
- write support
Comment 57 Alexander Larsson 2011-05-30 06:41:56 UTC
Why does the backend seem to repeat the full name both as the nickname (even if the nickname in the eds contact is empty) and the alias in folks? I don't want to show the name three times in the ui.
Comment 58 Alexander Larsson 2011-05-30 06:56:41 UTC
In fact, even if nickname is set in evo its not set by the backend.

There is also some issues with the email subtypes, for instance i have a user with home and work email specified and they show up as email_1 and email_2 types.

Also, the phone subtypes are e.g. mobile_phone, or other_phone. Is this some standard? I can't show them like that in the UI so i need to have some mapping from a well known standard backend set then (need that nevertheless since i need to translate these). And, how are custom phone types shown?
Comment 59 Raul Gutierrez Segales 2011-05-30 09:23:28 UTC
(In reply to comment #57)
> Why does the backend seem to repeat the full name both as the nickname (even if
> the nickname in the eds contact is empty) and the alias in folks? I don't want
> to show the name three times in the ui.

Oops. Fixed the nickname part. The AliasDetails interface is not implemented yet since I have to add write support first so we can write the Alias back to e-d-s.
Comment 60 Raul Gutierrez Segales 2011-05-30 09:29:58 UTC
(In reply to comment #58)
> In fact, even if nickname is set in evo its not set by the backend.

Fixed as noted in previous comment.

> There is also some issues with the email subtypes, for instance i have a user
> with home and work email specified and they show up as email_1 and email_2
> types.
> 
> Also, the phone subtypes are e.g. mobile_phone, or other_phone. Is this some
> standard? I can't show them like that in the UI so i need to have some mapping
> from a well known standard backend set then (need that nevertheless since i
> need to translate these). And, how are custom phone types shown?

Hmm. I grabbed the emails from:

http://git.gnome.org/browse/evolution-data-server/tree/addressbook/libebook/e-contact.h#n48

I think we should maybe use:

http://git.gnome.org/browse/evolution-data-server/tree/addressbook/libebook/e-contact.h#n163

which might contain pretty-print values. I'll check with the Evo people.

Same for phone.

I'll try to get this fixed by this afternoon.
Comment 61 Raul Gutierrez Segales 2011-05-30 10:50:15 UTC
Proposed a patch for e-d-s to get inlined avatars (the only type e-d-s is using at the time?) working.
Comment 62 Raul Gutierrez Segales 2011-06-05 22:47:16 UTC
A couple of additions:

commit bd7ea84d01ae0fda707a35e7d895106ce843c6f9
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Sun Jun 5 23:42:02 2011 +0100

    e-d-s: support for removing personas

commit ebfe6a73348890b40bddc5b67e2ce96b46018ccf
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Sun Jun 5 23:21:15 2011 +0100

    e-d-s: support for structured names

commit 49c9864d2944a5cca1c7c7d3a272e37a45c7e9c4
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Sun Jun 5 21:04:47 2011 +0100

    e-d-s: support adding postal addresses

commit b650e5f3a27da68662e856e88efa6cf0343a3316
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Sun Jun 5 20:50:09 2011 +0100

    e-d-s: test for postal addresses details interface

commit 7e4b53ce327769fbe69328107baf9c31e308da4c
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Sun Jun 5 18:54:47 2011 +0100

    e-d-s: API change for test data backend

commit 156d9a6c5712469930af76f5dcdfd8eace46fc05
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Sun Jun 5 15:29:15 2011 +0100

    e-d-s: support adding phone numbers

commit 67150e6298c6870a8ef95e6b9779ac2a4b875c17
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Sun Jun 5 15:12:19 2011 +0100

    e-d-s: support adding im-addresss

commit 4c4cf53e99b89927055e8ccafe8dcf09c8d1c02b
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Sun Jun 5 14:57:38 2011 +0100

    e-d-s: test for im-details interface

commit 8a5bd1adf1153b4463157568009fb96ebf98a45c
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Sun Jun 5 14:28:34 2011 +0100

    e-d-s: support adding avatar and e-mails

commit 01c5879ba569c2bfa9d4231f34e6869a3a189ad1
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Sun Jun 5 13:42:07 2011 +0100

    e-d-s: test for adding personas

commit 542ab99208ba895804ab4dc05b39c531d3203307
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Sun Jun 5 13:41:25 2011 +0100

    e-d-s: basic support for adding Edsf.Personas

commit beca5a3be88e5630ffb0cdcd93d3d599c557554b
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Sun Jun 5 11:42:57 2011 +0100

    e-d-s: Add test for AvatarDetails interface

commit 4378075dc24ada998ad9c8813657f01d55b6f29d
Author: Raul Gutierrez Segales <raul.gutierrez.segales@collabora.co.uk>
Date:   Mon May 30 12:00:22 2011 +0100

    e-d-s: improve avatar handling


Next up:

- Attribute updates
- Pending interfaces
- Linking support
Comment 63 Alexander Larsson 2011-06-08 20:35:50 UTC
Created attachment 189502 [details] [review]
Readonly support for notes

This patch adds readonly support for notes (i needed it for gnome-contacts testing of the notes feature).
Comment 64 Raul Gutierrez Segales 2011-06-09 00:32:20 UTC
(In reply to comment #63)
> Created an attachment (id=189502) [details] [review]
> Readonly support for notes
> 
> This patch adds readonly support for notes (i needed it for gnome-contacts
> testing of the notes feature).

Merged into my branch, thanks.

Also pushed support to update a couple of attributes:

- full name
- emails
- im-addresses

Still to go:
- phones
- postal addresses
- names


And also support to perform linking inside e-d-s.
Comment 65 Alexander Larsson 2011-06-10 12:46:34 UTC
Created attachment 189622 [details] [review]
Update to build with latest API

get_inlined now takes an out size_t parameter
Comment 66 Alexander Larsson 2011-06-10 12:46:48 UTC
Created attachment 189623 [details] [review]
Fix the use of FieldDetails parameters for emails

We now use the EVCard apis so that we get the correct vcard-style
parameters for emails.
Comment 67 Raul Gutierrez Segales 2011-06-12 13:36:40 UTC
(In reply to comment #66)
> Created an attachment (id=189623) [details] [review]
> Fix the use of FieldDetails parameters for emails
> 
> We now use the EVCard apis so that we get the correct vcard-style
> parameters for emails.

Pushed, thanks.

Also pushed changes to update:

- phones
- postal addresses
- names

Would be nice to get some feedback once you've got adding/updates support in Gnome Contacts. 

Still to go:
- support for linking (should we just go for linking via im addresses for the time being?)
- a couple of interfaces (RoleDetails, BirthdayDetails, UrlDetails)
Comment 68 Raul Gutierrez Segales 2011-06-12 19:31:44 UTC
Created attachment 189771 [details] [review]
Initial implementation of an e-d-s backend

This is squashed to ease up review. It also includes support for linking personas.
Comment 69 Philip Withnall 2011-06-12 20:59:03 UTC
Review of attachment 189771 [details] [review]:

Review of everything except the test programs, which can be looked at later. Several of the points from my review in comment #49 haven't been fixed yet; they should all be fairly easy to fix. (Just me being picky, really.) I might've accidentally repeated a few of them in this review. (Sorry.)

Nice work. :-)

::: backends/eds/Makefile.am
@@ +18,3 @@
+
+backenddir = $(BACKEND_DIR)/eds
+backend_LTLIBRARIES = libfolks-backend-eds.la

By bug #649296, we might want to just call it eds.la.

@@ +34,3 @@
+	libedataserver-1.2 \
+	libxml-2.0 \
+	posix \

Is posix really used? It's referenced as a dependency in several places, but does any code actually use it?

::: backends/eds/eds-backend.vala
@@ +1,2 @@
+/*
+ * Copyright (C) 2010 Collabora Ltd.

2011!

@@ +100,3 @@
+                          if (s.peek_name () in use_addressbooks)
+                            {
+                              add_addressbook (s);

Missing a “this.”.

@@ +105,3 @@
+                      else
+                        {
+                          add_addressbook (s);

Missing a “this.”.

@@ +153,3 @@
+      var store = new Edsf.PersonaStore (s);
+      this._persona_stores.set (store.id, store);
+      store.removed.connect (this.store_removed_cb);

Shouldn't you disconnect from this signal in store_removed_cb()?

@@ +164,3 @@
+    }
+
+  private string [] _get_addressbooks_from_env ()

No space in “string[]”.

::: backends/eds/lib/Makefile.am
@@ +45,3 @@
+	--pkg libedataserver-1.2 \
+	--pkg libxml-2.0 \
+	--pkg posix \

This list of --pkg arguments could be produced using $(addprefix --pkg ,$(folks_backend_eds_deps)).

::: backends/eds/lib/edsf-persona-store.vala
@@ +124,3 @@
+   * Create a new PersonaStore.
+   *
+   * Create a new persona store to store the {@link Persona}s for the contacts

Missing documentation for the “s” parameter.

@@ +200,3 @@
+              Set<PostalAddress> postal_addresses =
+                (Set<PostalAddress>) v.get_object ();
+                yield this._set_contact_postal_addresss (contact,

s/sss/ss/. :-D

Also, the indentation's wrong on this line.

@@ +256,3 @@
+   * Remove a {@link Persona} from the PersonaStore.
+   *
+   * See {@link Folks.PersonaStore.remove_persona}.

Missing documentation for the “persona” parameter.

@@ +271,3 @@
+        {
+          /* FIXME: we need a specific error contact removal
+           *        failures. */

File a separate bug report about adding such an error code.

@@ +301,3 @@
+                  if (this._addressbook.is_opened ())
+                    {
+

Extraneous new line.

@@ +356,3 @@
+  private async void _set_contact_avatar (E.Contact contact,
+      File avatar)
+  {

Indentation in this method is broken.

@@ +360,3 @@
+      {
+        uint8 []photo_content;
+        avatar.load_contents (null, out photo_content);

Shouldn't there be a “yield” here?

@@ +391,3 @@
+      catch (GLib.Error error)
+        {
+          GLib.warning ("Can't update emails: %s\n",

s/emails/e-mail addresses/.

@@ +467,3 @@
+    {
+      GLib.List <E.VCardAttribute> attributes =
+        new GLib.List <E.VCardAttribute>();

You could use “var” in the declaration here.

@@ +475,3 @@
+          foreach (var param_name in e.parameters.get_keys ())
+            {
+              var param = new E.VCardAttributeParam (param_name.up());

Missing space after “.up”.

@@ +485,3 @@
+        }
+
+      attributes.reverse ();

Does it matter what order the attributes are in?

@@ +554,3 @@
+      catch (GLib.Error error)
+        {
+          GLib.warning ("Can't update emails: %s\n", error.message);

s/emails/IM addresses/.

@@ +594,3 @@
+      lock (this._personas)
+        {
+          foreach (E.Contact c in (GLib.List<E.Contact>) contacts)

Why is the cast necessary?

@@ +622,3 @@
+            {
+              persona._update (c);
+            }

If (persona == null), should we perhaps add the persona to the store?

@@ +641,3 @@
+        }
+
+       if (removed_personas.size> 0)

Missing space before the “>”.

::: backends/eds/lib/edsf-persona.vala
@@ +41,3 @@
+    PostalAddressDetails
+{
+  public static const string [] phone_fields = {

Why are these public?

@@ +56,3 @@
+    "blog_url", "fburl", "homepage_url", "video_url"
+  };
+  private const string[] _linkable_properties = {"im-addresses"};

Need spaces between the braces and quotation marks.

@@ +115,3 @@
+      private set
+        {
+        }

Perhaps a FIXME comment here would be better than nothing?

@@ +241,3 @@
+  internal static string build_iid (string store_id, string contact_id)
+    {
+      return "%s:%s".printf (store_id, contact_id);

Do either of these IDs need escaping for colons first? Might be helpful to put an example of the input/output strings' formats in the documentation.

@@ +244,3 @@
+    }
+
+

Extraneous new line.

@@ +279,3 @@
+              "full_name");
+
+      debug ("Creating new Edsf.Persona with id '%s'", iid);

s/id/IID/, so that the message isn't ambiguous.

@@ +349,3 @@
+      this._email_addresses = new HashSet<FieldDetails> ();
+
+      var attrs = _contact.get_attributes(E.ContactField.EMAIL);

Missing space before the opening bracket.

@@ +427,3 @@
+  private void _update_avatar ()
+    {
+      string filename = this.uid.delimit ("/", '-');

Might be more portable and clearer to use G_DIR_SEPARATOR instead of a “"/"”.

@@ +439,3 @@
+          var content_new = this._get_avatar_content_from_contact (p);
+
+          if (content_old != content_new)

It would be cheaper to do a comparison of just the files' sizes first; if they're different, it saves loading the old cached avatar for an expensive string comparison.

@@ +446,3 @@
+                      content_new.length,
+                      null, false, FileCreateFlags.REPLACE_DESTINATION,
+                      null);

This should really be async.

@@ +458,3 @@
+        {
+          this._avatar = null;
+          FileUtils.remove (cached_avatar_path);

It would be better to consistently use the GFile version of cached_avatar_path, since that means we can make this remove() call async.

@@ +497,3 @@
+                    {
+                      string address = (owned) normalise_im_address
+                          ((owned) field_value, proto);

Missing an “ImDetails.” here.

@@ +523,3 @@
+            {
+              uint8 []content_temp;
+              this._avatar.load_contents (null, out content_temp);

This should be async.

@@ +549,3 @@
+              uint8 []temp_content;
+              var file = File.new_for_uri (p.get_uri ());
+              file.load_contents (null, out temp_content);

This should be async.

@@ +566,3 @@
+   */
+  internal static HashTable<string, GLib.List<string>> _get_im_eds_map ()
+    {

Might it be worth making this initialisation thread-safe?

@@ +575,3 @@
+          foreach (string p in protos)
+            {
+              GLib.List<string>  properties = new GLib.List<string> ();

You can use “var” here.

@@ +624,3 @@
+  private void _update_phones ()
+    {
+      this._phone_numbers = new HashSet<FieldDetails> ();

It'd be slightly cleaner to call “this._phone_numbers.clear ()” instead of replacing it with a new instance; and it would also save having to re-initialise this._phone_numbers_ro (which entails another object instantiation).

Same with all the other _update_*() methods.

@@ +626,3 @@
+      this._phone_numbers = new HashSet<FieldDetails> ();
+
+      var attrs = _contact.get_attributes(E.ContactField.TEL);

Missing space before the opening bracket.

@@ +661,3 @@
+            {
+              /* FIXME: might be my broken setup, but it looks like
+               * e-d-s is ignoring the address_format param */

Has this been resolved?

::: backends/eds/lib/folks-eds.deps
@@ +3,3 @@
+folks
+libebook-1.2
+libedataserver-1.2

Missing libgee here?

::: tests/lib/eds/Makefile.am
@@ +35,3 @@
+	$(top_builddir)/folks/libfolks.la \
+	$(top_builddir)/backends/eds/libfolks-backend-eds.la \
+	$(NULL)

You're missing EBOOK_LIBS and EDATASERVER_LIBS here.

::: tests/tools/eds.sh
@@ +1,2 @@
+#
+# Helper functions to start your own Tracker instance. This depends

Tracker?
Comment 70 Raul Gutierrez Segales 2011-06-13 05:53:36 UTC
(In reply to comment #69)
> @@ +439,3 @@
> +          var content_new = this._get_avatar_content_from_contact (p);
> +
> +          if (content_old != content_new)
> 
> It would be cheaper to do a comparison of just the files' sizes first; if
> they're different, it saves loading the old cached avatar for an expensive
> string comparison.

Most of the times (always?) the image coming from e-d-s won't be inlined, so file size comparison might be bogus. How about calculating a hash of the saved image and using that instead of full content comparison for next time?
Comment 71 Raul Gutierrez Segales 2011-06-13 05:57:21 UTC
(In reply to comment #70)
> (In reply to comment #69)
> > @@ +439,3 @@
> > +          var content_new = this._get_avatar_content_from_contact (p);
> > +
> > +          if (content_old != content_new)
> > 
> > It would be cheaper to do a comparison of just the files' sizes first; if
> > they're different, it saves loading the old cached avatar for an expensive
> > string comparison.
> 
> Most of the times (always?) the image coming from e-d-s won't be inlined, so
> file size comparison might be bogus. How about calculating a hash of the saved
> image and using that instead of full content comparison for next time?

Meant to say: "will be inlined".
Comment 72 Raul Gutierrez Segales 2011-06-13 06:56:41 UTC
(In reply to comment #69)
> @@ +446,3 @@
> +                      content_new.length,
> +                      null, false, FileCreateFlags.REPLACE_DESTINATION,
> +                      null);
> 
> This should really be async.

If this was async then _update_avatar () should be async too and we wouldn't be able to call _update_avatar () from Edsf.Persona's constructor, right? Unless we figure out what to do about properties that should be set in construction time vs thought that should be lazily (or asynchronously) loaded. 

Note that at the the time the IndividualAggregator expects that the (recently) created Personas it receives are in a consistent state. So if we still are setting properties while the IndividualAggregator is working on the Persona we'll have problem (as I've just experienced by seeing tests fail when making the above async).
Comment 73 Raul Gutierrez Segales 2011-06-13 07:30:25 UTC
Created attachment 189804 [details] [review]
Initial implementation of an e-d-s backend

I've address most of the points noted on comment #69 and made some further question about some of them. 

I'll address what's pending on comment #49 at some point today (hopefully).

Thanks for the review!
Comment 74 Alexander Larsson 2011-06-13 08:35:36 UTC
Created attachment 189806 [details] [review]
Pass null to E.ContactPhoto.get_inlined() size out argument

It seems this function got a new argument at some point.
Comment 75 Alexander Larsson 2011-06-13 08:35:40 UTC
Created attachment 189807 [details] [review]
Append to the writable url set, not the readonly one
Comment 76 Alexander Larsson 2011-06-13 08:35:44 UTC
Created attachment 189808 [details] [review]
Implenment UrlDetails in Edsf.Persona

Without this the merging of urls doesn't happen. Note that url setting
is still not supported.
Comment 77 Raul Gutierrez Segales 2011-06-13 08:59:36 UTC
(In reply to comment #75)
> Created an attachment (id=189807) [details] [review]
> Append to the writable url set, not the readonly one

Applied, thanks!
Comment 78 Raul Gutierrez Segales 2011-06-13 08:59:58 UTC
(In reply to comment #76)
> Created an attachment (id=189808) [details] [review]
> Implenment UrlDetails in Edsf.Persona
> 
> Without this the merging of urls doesn't happen. Note that url setting
> is still not supported.

Applied, thanks!
Comment 79 Raul Gutierrez Segales 2011-06-13 09:02:19 UTC
(In reply to comment #74)
> Created an attachment (id=189806) [details] [review]
> Pass null to E.ContactPhoto.get_inlined() size out argument
> 
> It seems this function got a new argument at some point.

Fixed with latest Vala version.
Comment 80 Raul Gutierrez Segales 2011-06-13 10:03:18 UTC
Created attachment 189814 [details] [review]
Initial implementation of an e-d-s backend

Addressed issues of comment #49 and merged Alex's latest patches.
Comment 81 Travis Reitter 2011-06-13 17:24:48 UTC
Review of attachment 189814 [details] [review]:

Here's the first round of reviews. It's almost entirely building, running the tests, and edsf-persona-store.vala. I still need to review the other files.

=======================

edsf-persona.vala:466.30-466.45: error: 1 missing arguments for `uint8[] E.ContactPhoto.get_inlined (out size_t len)'
          content = (string) p.get_inlined ();
                             ^^^^^^^^^^^^^^^^

===================================

  VALAC  libfolks_eds_la_vala.stamp
edsf-persona-store.vala:486.44-486.47: warning: Argument 1: Cannot pass null to non-null parameter type
          var attr = new E.VCardAttribute (null, attrib_name);

The first argument to the constructor (group name) needs to be marked "allow-none=1" in the gobject annotation in EDS

======================

edsf-persona-store.vala
-----------------------

  /**
   * Create a new PersonaStore.
   *
   * Create a new persona store to store the {@link Persona}s for the contacts
   *

There's redundancy here.


      this._source = s;
      this._addressbook_uri =  uri;

Source should be a readable property. Possibly the addressbook URI as well.


  public override async Folks.Persona? add_persona_from_details (
...
          lock (this._personas)
            {
              string added_uid;
              var result = yield this._addressbook.add_contact (contact,
                  out added_uid);

This lock needs to happen after the async call to avoid its lifespan being multiple function calls.

We should add a test case that adds a ton of contacts in parallel and make sure that they all successfully add without duplicates.


  public override async void remove_persona (Folks.Persona persona)
      throws Folks.PersonaStoreError
    {
      try
        {
          var contact_id = ((Edsf.Persona) persona).contact_id;
          E.Contact contact;
          yield this._addressbook.get_contact (contact_id, out contact);
          yield this._addressbook.remove_contact (contact);

Why isn't the contact just a property on the Persona? Then we wouldn't need to fetch it here and the many other places we call get_contact() (which must end up burning a lot of time yielding instead of the constant-time lookup).


          /* FIXME: https://bugzilla.gnome.org/show_bug.cgi?id=652425 */
          throw new PersonaStoreError.INVALID_ARGUMENT (
              "Can't remove contact: %s\n", e.message);

Let's fix this bug so we can fix this code.


                  /* FIXME: The async version of open () is mysteriously
                   * broken. */
                  this._addressbook.open_sync (true, null);

The async version needs to get fixed upstream (whether it's a problem in libebook or its introspection annotations) and we need to use it here.


Edsf.PersonaStore.prepare() needs to throw GLib.Error

              catch (GLib.Error e1)
                {
                  throw new PersonaStoreError.INVALID_ARGUMENT (
                      "Couldn't open addressbook with URI: %s\n", e1.message);
                }

This is the result of bug #652425 again.


  private async void _set_contact_avatar (E.Contact contact,
      File avatar)
  {
    try
      {
        uint8[] photo_content;
        yield avatar.load_contents_async (null, out photo_content);

We should optimize to use URIs as much as possible, though we can defer that until we fix bug #650414.


      /* FIXME: do we care about the order? */
      attributes.reverse ();

Nope.


      /* Lets reset everything first */
Let's


In general, I think we should move the backend from using EContact to using EVCard (which EContact derives from). Then we can eliminate or dramatically simplify the equivalent of Edsf.Persona._get_im_eds_map(). We'll be able to use, eg, the VCard (attribute: X-AIM, parameter: TYPE=HOME) pair instead of im_aim_home_1, won't need to have the hard-coded _1, _2_, etc.

==========================

backend.vala:51.11-51.46: warning: local variable `pa' declared but never used
      var pa = (PostalAddress) v.get_object ();
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

====================

*** Warning: Linking the shared library libeds-test.la against the loadable module
*** libfolks-backend-eds.so is not portable!
make[4]: Leaving directory `/home/treitter/checkout/gnome/folks/tests/lib/eds'
make[4]: Entering directory `/home/treitter/checkout/gnome/folks/tests/lib'

====================

All the EDS backend tests fail like:

FAIL: set-postal-addresses
/LinkPersonasTests/test linking personas: 
libebook-WARNING **: e_book_client_new: Cannot get book from factory: The name org.gnome.evolution.dataserver.AddressBook1 was not provided by any .service files

I think this should be fixed by the port to GDBus, though.
Comment 82 Raul Gutierrez Segales 2011-06-13 21:36:24 UTC
(In reply to comment #81)
> Review of attachment 189814 [details] [review]:
> =======================
> 
> edsf-persona.vala:466.30-466.45: error: 1 missing arguments for `uint8[]
> E.ContactPhoto.get_inlined (out size_t len)'
>           content = (string) p.get_inlined ();
>                              ^^^^^^^^^^^^^^^^
> 

You've seem to have an old Vala version (get_inlined takes no argument, thats a transparent out param that holds the length of the returned array).

> ===================================
> 
>   VALAC  libfolks_eds_la_vala.stamp
> edsf-persona-store.vala:486.44-486.47: warning: Argument 1: Cannot pass null to
> non-null parameter type
>           var attr = new E.VCardAttribute (null, attrib_name);
> 
> The first argument to the constructor (group name) needs to be marked
> "allow-none=1" in the gobject annotation in EDS
> 
> ======================


I don't get this, again - old Vala?


>   public override async Folks.Persona? add_persona_from_details (
> ...
>           lock (this._personas)
>             {
>               string added_uid;
>               var result = yield this._addressbook.add_contact (contact,
>                   out added_uid);
> 
> This lock needs to happen after the async call to avoid its lifespan being
> multiple function calls.
> 

This has been addressed.

>   public override async void remove_persona (Folks.Persona persona)
>       throws Folks.PersonaStoreError
>     {
>       try
>         {
>           var contact_id = ((Edsf.Persona) persona).contact_id;
>           E.Contact contact;
>           yield this._addressbook.get_contact (contact_id, out contact);
>           yield this._addressbook.remove_contact (contact);
> 
> Why isn't the contact just a property on the Persona? Then we wouldn't need to
> fetch it here and the many other places we call get_contact() (which must end
> up burning a lot of time yielding instead of the constant-time lookup).

I wasn't sure about the lifecycle of the E.Contact object, will check.

>           /* FIXME: https://bugzilla.gnome.org/show_bug.cgi?id=652425 */
>           throw new PersonaStoreError.INVALID_ARGUMENT (
>               "Can't remove contact: %s\n", e.message);
> 
> Let's fix this bug so we can fix this code.

Lets please not block on this :( We can fix it afterwards. 

>                   /* FIXME: The async version of open () is mysteriously
>                    * broken. */
>                   this._addressbook.open_sync (true, null);
> 
> The async version needs to get fixed upstream (whether it's a problem in
> libebook or its introspection annotations) and we need to use it here.

I am hunting this. 

 
> In general, I think we should move the backend from using EContact to using
> EVCard (which EContact derives from). Then we can eliminate or dramatically
> simplify the equivalent of Edsf.Persona._get_im_eds_map(). We'll be able to
> use, eg, the VCard (attribute: X-AIM, parameter: TYPE=HOME) pair instead of
> im_aim_home_1, won't need to have the hard-coded _1, _2_, etc.
> 

Agreed - I am moving towards thanks to alexl's patches. 

> All the EDS backend tests fail like:
> 

This is odd - they work for me ... are you sure there ain't something old on your system?
Comment 83 Travis Reitter 2011-06-14 10:00:21 UTC
(In reply to comment #82)
> (In reply to comment #81)
> > Review of attachment 189814 [details] [review] [details]:
> > =======================
> > 
> > edsf-persona.vala:466.30-466.45: error: 1 missing arguments for `uint8[]
> > E.ContactPhoto.get_inlined (out size_t len)'
> >           content = (string) p.get_inlined ();
>
> You've seem to have an old Vala version (get_inlined takes no argument, thats a
> transparent out param that holds the length of the returned array).
> 

Sorry - I think that was leftover from my original review (which I started last week, when I was running Vala 0.12). You can just ignore this part for now.

> > ===================================
> > 
> >   VALAC  libfolks_eds_la_vala.stamp
> > edsf-persona-store.vala:486.44-486.47: warning: Argument 1: Cannot pass null to
> > non-null parameter type
> >           var attr = new E.VCardAttribute (null, attrib_name);
> > 
> > The first argument to the constructor (group name) needs to be marked
> > "allow-none=1" in the gobject annotation in EDS
> > 
> > ======================
> 
> 
> I don't get this, again - old Vala?

I'm sure I saw this yesterday (after maintainer-clean) and I'm using Vala 0.12.0.296-1e90d. But I just tried again now and I didn't hit this error.

> >           /* FIXME: https://bugzilla.gnome.org/show_bug.cgi?id=652425 */
> >           throw new PersonaStoreError.INVALID_ARGUMENT (
> >               "Can't remove contact: %s\n", e.message);
> > 
> > Let's fix this bug so we can fix this code.
> 
> Lets please not block on this :( We can fix it afterwards. 

But it should only take a minute. Just make an errordomain with the few values you need here and we'll expand as we need - we don't need to come up with all possible values now.

> > All the EDS backend tests fail like:
> > 
> 
> This is odd - they work for me ... are you sure there ain't something old on
> your system?

I'm checking this out - will follow-up.


I forgot to mention this build error as well (which happens after a 'make maintainer-clean'):

make[4]: Entering directory `/home/treitter/checkout/gnome/folks/backends/eds/lib'
make[4]: *** No rule to make target `folks-eds.vapi', needed by `all-am'.  Stop.
make[4]: *** Waiting for unfinished jobs....
  VALAC  libfolks_eds_la_vala.stamp
edsf-persona-store.vala:346.19-347.78: warning: unhandled error `Folks.PersonaStoreError'
Compilation succeeded - 1 warning(s)


You probably just need to adjust the stamp target - should it really just be empty as it is now?
Comment 84 Travis Reitter 2011-06-14 10:45:46 UTC
Please add this trivial patch as well (so we can run all the eds tests through gdb easily):

diff --git a/backends/eds/lib/Makefile.am b/backends/eds/lib/Makefile.am
index 90f7c1e..1c68ef1 100644
--- a/backends/eds/lib/Makefile.am
+++ b/backends/eds/lib/Makefile.am
@@ -115,3 +115,4 @@ EXTRA_DIST = \
 
 -include ../backend.mk
 -include $(top_srcdir)/git.mk
+-include $(top_srcdir)/check.mk
Comment 85 Travis Reitter 2011-06-14 10:47:45 UTC
(In reply to comment #84)
> Please add this trivial patch as well (so we can run all the eds tests through
> gdb easily):
> 
> diff --git a/backends/eds/lib/Makefile.am b/backends/eds/lib/Makefile.am
> index 90f7c1e..1c68ef1 100644
> --- a/backends/eds/lib/Makefile.am
> +++ b/backends/eds/lib/Makefile.am
> @@ -115,3 +115,4 @@ EXTRA_DIST = \
> 
>  -include ../backend.mk
>  -include $(top_srcdir)/git.mk
> +-include $(top_srcdir)/check.mk

Actually, only once we've merged bug #652434. So, disregard unless that's merged first.
Comment 86 Raul Gutierrez Segales 2011-06-14 10:52:49 UTC
Created attachment 189888 [details] [review]
Initial implementation of an e-d-s backend

A couple of minor updates (most notable: now we can use the async version of e_client_open, things should be way faster).
Comment 87 Travis Reitter 2011-06-14 10:56:11 UTC
(In reply to comment #84)
> Please add this trivial patch as well (so we can run all the eds tests through
> gdb easily):
> 
> diff --git a/backends/eds/lib/Makefile.am b/backends/eds/lib/Makefile.am
> index 90f7c1e..1c68ef1 100644
> --- a/backends/eds/lib/Makefile.am
> +++ b/backends/eds/lib/Makefile.am
> @@ -115,3 +115,4 @@ EXTRA_DIST = \
> 
>  -include ../backend.mk
>  -include $(top_srcdir)/git.mk
> +-include $(top_srcdir)/check.mk

Furthermore, I patched the wrong Makefile.am. It really should be:

diff --git a/tests/eds/Makefile.am b/tests/eds/Makefile.am
index 9b41868..ff3ce3c 100644
--- a/tests/eds/Makefile.am
+++ b/tests/eds/Makefile.am
@@ -196,3 +196,4 @@ EXTRA_DIST = \
        $(NULL)
 
 -include $(top_srcdir)/git.mk
+-include $(top_srcdir)/check.mk


Trying to react to this bug while being in a meeting is a bad idea. :)
Comment 88 Travis Reitter 2011-06-14 11:28:07 UTC
(In reply to comment #86)
> Created an attachment (id=189888) [details] [review]
> Initial implementation of an e-d-s backend
> 
> A couple of minor updates (most notable: now we can use the async version of
> e_client_open, things should be way faster).

+ Signal.connect (this._addressbook, "opened",
+ (GLib.Callback) this._addressbook_opened_cb, this);

We have to do this because

this._addressbook.opened.connect (this._addressbook_opened_cb);

fails due to EClient.opened being the property (and not the signal with the same name)? Couldn't we just connect to notify["opened"] in that case?

+ private void _addressbook_opened_cb (void *error, Edsf.PersonaStore self)
+ {
+ self.get_book_view ();
+ }
+
+ public async void get_book_view ()
+ {

I don't think get_book_view() should be public - anything to do with starting the view should be a consequence of prepare(). You might as well just move its body into _addressbook_opened_cb().

And if it were public, it should be named something like start_book_view().
Comment 89 Raul Gutierrez Segales 2011-06-14 11:50:08 UTC
(In reply to comment #88)
> (In reply to comment #86)
> > Created an attachment (id=189888) [details] [review] [details] [review]
> > Initial implementation of an e-d-s backend
> > 
> > A couple of minor updates (most notable: now we can use the async version of
> > e_client_open, things should be way faster).
> 
> + Signal.connect (this._addressbook, "opened",
> + (GLib.Callback) this._addressbook_opened_cb, this);
> 
> We have to do this because
> 
> this._addressbook.opened.connect (this._addressbook_opened_cb);
> 
> fails due to EClient.opened being the property (and not the signal with the
> same name)? Couldn't we just connect to notify["opened"] in that case?
> 
> + private void _addressbook_opened_cb (void *error, Edsf.PersonaStore self)
> + {
> + self.get_book_view ();
> + }
> +
> + public async void get_book_view ()
> + {
> 
> I don't think get_book_view() should be public - anything to do with starting
> the view should be a consequence of prepare(). You might as well just move its
> body into _addressbook_opened_cb().
> 
> And if it were public, it should be named something like start_book_view().

I'll revert all of this now that:

https://bugzilla.gnome.org/show_bug.cgi?id=652530

has been fixed.
Comment 90 Raul Gutierrez Segales 2011-06-14 12:24:28 UTC
I've reverted the previous commit and also reworked prepare():

http://cgit.collabora.com/git/user/rgs/folks/commit/?h=eds-0.5&id=403631eb7935032d3a44bf849780ee3a60896906
Comment 91 Travis Reitter 2011-06-14 14:35:01 UTC
(In reply to comment #81)
> All the EDS backend tests fail like:
> 
> FAIL: set-postal-addresses
> /LinkPersonasTests/test linking personas: 
> libebook-WARNING **: e_book_client_new: Cannot get book from factory: The name
> org.gnome.evolution.dataserver.AddressBook1 was not provided by any .service
> files
> 
> I think this should be fixed by the port to GDBus, though.

Good news, everyone! I rebuilt everything in my development PREFIX from Folks down and that seems to have eliminated this. All the tests pass as expected.
Comment 92 Travis Reitter 2011-06-14 14:40:56 UTC
  CC     individual-retrieval.o
link-personas.vala:270.17-270.63: warning: unhandled error `Folks.IndividualAggregatorError'
          yield this._aggregator.link_personas (this._personas);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Build warning.
Comment 93 Travis Reitter 2011-06-14 17:04:09 UTC
My last set of comments - once these and my prior comments are addressed, we can merge this backend.


edsf-persona.vala
-----------------------

public class Edsf.Persona : Folks.Persona,

This should also implement LocalIdAddressDetails and WebServiceAddressDetails.


  private const string[] _linkable_properties = { "im-addresses" };

Please support linking by web-service-addresses and local-ids.

  /* The following 4 definitions are used by the tests */
  public static const string[] phone_fields = {
    "assistant_phone", "business_phone", "business_phone_2", "callback_phone",
    "car_phone", "company_phone", "home_phone", "home_phone_2", "isdn_phone",
    "mobile_phone", "other_phone", "primary_phone"
  };
  public static const string[] address_fields = {
    "address_home", "address_other", "address_work"
  };
  public static const string[] email_fields = {
    "email_1", "email_2", "email_3", "email_4"
  };
  public static const string[] url_properties = {
    "blog_url", "fburl", "homepage_url", "video_url"
  };

These don't need to be changed immediately, but we will change these when we switch to EVCard (which should hopefully be shortly after mering).


  private weak E.Contact _contact;

This should be a public read-only property (to replace all the this._addressbook.get_contact() calls in Edsf.PersonaStore). I think you said something about the EBookView replacing instances of EContacts, but in that case, we'll just need the PersonaStore to compare the contents of contacts-changed and update the contact property of each Persona where the contact UIDs match (and we'll need to make the Edsf.Persona.contact internally-writable).


  public Set<Note> notes
    {
      get { return this._notes_ro; }
      private set
        {
          /* FIXME: to be implemented */

Please implement.


      /* FIXME: should we escape/replace colons in store_id and
       *        contact_id? We still have copies of both these
       *        parameters saved so I am not sure if its worth it. */
      return "%s:%s".printf (store_id, contact_id);

No need to escape them, since iid is officially opaque. Cut this FIXME.


      /* FIXME:
       *
       * For some unknown reason when we create an e-d-s contact from
       * add_persona_from_details (), the contact's id might not be
       * immediately retrievable. So we workaround by using contact_uid
       * which was returned by BookClient.add_contact () instead of
       * querying the contact for its id property.
       *
       * This should be tracked with the e-d-s devs.
       */

Is this still true?


  internal static HashTable<string, GLib.List<string>> _get_im_eds_map ()

Let's simplify this when we port completely to EVCard


  /*
   * TODO: we should check if addresses corresponding to different types
   *       are the same and if so instantiate only one PostalAddress
   *       (with the given types).
   */
  private void _update_addresses ()

We could do a simple comparison (ie, all fields case-insensitively-identical). Doing it properly (normalizing addresses) would be infeasible though.


edsf-persona-store.vala
-----------------------

This doesn't seem to stop the book view and close the ebookclient. Both should happen when disposing of the Edsf.PersonaStore and when unprepare()ing a store. And we should be unprepare()ing all the known stores in Edsf.Backend.unprepare()


add-persona.vala
-----------------------

We should add a variation on this (later in our ordered list of TESTS) that adds a ton of contacts (on the order of a few thousand) and makes sure they're successfully added and we don't get duplicates. The point of this is to make sure our locking issue has truly been resolved.
Comment 94 Philip Withnall 2011-06-15 07:37:21 UTC
(In reply to comment #93)
>       /* FIXME: should we escape/replace colons in store_id and
>        *        contact_id? We still have copies of both these
>        *        parameters saved so I am not sure if its worth it. */
>       return "%s:%s".printf (store_id, contact_id);
> 
> No need to escape them, since iid is officially opaque. Cut this FIXME.

I think there is, otherwise (e.g.) store_id and contact_id could be "file://foo:bar:baz" and "file://bish:bosh:bash" resulting in an ambiguous string being constructed.

It's not likely that the IDs could be in such pathogenic formats, but iirc they're both URIs, so could contain colons. Raul, is this correct?
Comment 95 Raul Gutierrez Segales 2011-06-15 08:29:14 UTC
(In reply to comment #94)
> (In reply to comment #93)
> >       /* FIXME: should we escape/replace colons in store_id and
> >        *        contact_id? We still have copies of both these
> >        *        parameters saved so I am not sure if its worth it. */
> >       return "%s:%s".printf (store_id, contact_id);
> > 
> > No need to escape them, since iid is officially opaque. Cut this FIXME.
> 
> I think there is, otherwise (e.g.) store_id and contact_id could be
> "file://foo:bar:baz" and "file://bish:bosh:bash" resulting in an ambiguous
> string being constructed.
> 
> It's not likely that the IDs could be in such pathogenic formats, but iirc
> they're both URIs, so could contain colons. Raul, is this correct?

Right - there will be colons. But as Travis mentioned, do we really care this stays parse-able? I.e.: I don't know of any need to de-construct the iid to recover the store_id and contact_id. The only real need is that it has to be unique (and that is still accomplished even with the extra colons).
Comment 96 Travis Reitter 2011-06-16 12:02:23 UTC
(In reply to comment #95)
> (In reply to comment #94)
> > (In reply to comment #93)
> > >       /* FIXME: should we escape/replace colons in store_id and
> > >        *        contact_id? We still have copies of both these
> > >        *        parameters saved so I am not sure if its worth it. */
> > >       return "%s:%s".printf (store_id, contact_id);
> > > 
> > > No need to escape them, since iid is officially opaque. Cut this FIXME.
> > 
> > I think there is, otherwise (e.g.) store_id and contact_id could be
> > "file://foo:bar:baz" and "file://bish:bosh:bash" resulting in an ambiguous
> > string being constructed.
> > 
> > It's not likely that the IDs could be in such pathogenic formats, but iirc
> > they're both URIs, so could contain colons. Raul, is this correct?
> 
> Right - there will be colons. But as Travis mentioned, do we really care this
> stays parse-able? I.e.: I don't know of any need to de-construct the iid to
> recover the store_id and contact_id. The only real need is that it has to be
> unique (and that is still accomplished even with the extra colons).

Well, by opaque, I mean, externally-opaque. We may want to keep it internally-parseable (like we do for parsing the Persona.uid).
Comment 97 Travis Reitter 2011-06-20 16:48:08 UTC
This also needs to depend upon Vala >= 0.13.0 (the first release to fulfill its requirements).
Comment 98 Travis Reitter 2011-06-22 16:38:16 UTC
Now that bug #652434 has been fixed in master, please update all the eds test Makefile.am file(s) accordingly.
Comment 99 Alexander Larsson 2011-06-29 07:54:31 UTC
One major issue that is still left for gnome-contacts is that the type field for postal addresses is still not using the vcard style TYPE field. This means we don't sanely display postal address types in contacts, nor can you edit it.
Comment 100 Raul Gutierrez Segales 2011-06-29 08:55:19 UTC
(In reply to comment #99)
> One major issue that is still left for gnome-contacts is that the type field
> for postal addresses is still not using the vcard style TYPE field. This means
> we don't sanely display postal address types in contacts, nor can you edit it.

I'll push an update for that today.
Comment 101 Travis Reitter 2011-06-29 16:24:27 UTC
(In reply to comment #99)
> One major issue that is still left for gnome-contacts is that the type field
> for postal addresses is still not using the vcard style TYPE field. This means
> we don't sanely display postal address types in contacts, nor can you edit it.

Note that it's stored slightly differently for PostalAddress fields. Instead of being wrapped by a FieldDetails object (which provides the type), PostalAddress contains the member Set<string> types.

The reason we did it this way is that FieldDetails is based upon simple string values. If we changed it to be completely generic, it would have been a pain to use from C.
Comment 102 Alexander Larsson 2011-06-29 18:47:13 UTC
Travis: That is unfortunate, because that means its impossible to do custom labels or other vcard extension stuff for postal addresses. Is it to late to at least make it a full MultiSet member?
Comment 103 Travis Reitter 2011-06-29 22:18:09 UTC
(In reply to comment #102)
> Travis: That is unfortunate, because that means its impossible to do custom
> labels or other vcard extension stuff for postal addresses. Is it to late to at
> least make it a full MultiSet member?

I've filed the following to deal with the remaining details which can't currently support arbitrary parameters and parameter values:

* bug #653679
* bug #653680
* bug #653682

(these are all blocking our API-breaking meta bug #653684). I want to make Folks 0.5.4 our last API-breaking release for the indefinite future, so we might as well get these in in our final push.
Comment 104 Travis Reitter 2011-06-29 22:51:24 UTC
(In reply to comment #103)
> (In reply to comment #102)
> > Travis: That is unfortunate, because that means its impossible to do custom
> > labels or other vcard extension stuff for postal addresses. Is it to late to at
> > least make it a full MultiSet member?
> 
> I've filed the following to deal with the remaining details which can't
> currently support arbitrary parameters and parameter values:
> 
> * bug #653679
> * bug #653680
> * bug #653682
> 
> (these are all blocking our API-breaking meta bug #653684). I want to make
> Folks 0.5.4 our last API-breaking release for the indefinite future, so we
> might as well get these in in our final push.

By the way, I intentionally skipped over supporting arbitrary parameters for Role (ie, ORG in vCards), since they don't seem particularly useful. If anyone can think of good use cases, I'll reconsider.
Comment 105 Alexander Larsson 2011-06-30 13:49:19 UTC
Hmm, i just found a weird bug. If i have an eds contact with two phone numbers,  say one WORK and one OTHER, but both happent to have the same value (i.e. the phone number). Now folks only shows this as one item, even though the fielddetails parameters should be different...
Comment 106 Travis Reitter 2011-07-11 19:18:39 UTC
Just a couple trivial comments on the latest version in your eds-0.5 branch. Please do the following:

* fix these trivial points
* ensure NEWS is updated
* ensure it builds with and without --enable-docs
* ensure it builds with and without --enable-eds-backend
* squash commits as appropriate
* merge to master

Thanks for all the work on this! Now things get a bit more interesting :)


@@ -233,6 +234,12 @@ public class Edsf.PersonaStore : Folks.PersonaStore
yield this._set_contact_web_service_addresses (contact,
web_service_addresses);
}
+ else if (k == Folks.PersonaStore.detail_key (PersonaDetail.NOTES))
+ {
+ var notes = (Gee.HashSet<Note>) v.get_object ();
+ yield this._set_contact_notes (contact, notes);
+ }
+
}

Excessive empty line at the end.


+ /* First lets remove everything */

lets -> let's
Comment 107 Raul Gutierrez Segales 2011-07-12 18:52:30 UTC
commit 31f40df6da4766401b576635a8a632db84ee0a01
Author: Raul Gutierrez Segales <raul.gutierrez.segales@collabora.co.uk>
Date:   Tue Jul 12 19:00:00 2011 +0100

    Add an EDS backend
    
    Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=638281