(In reply to comment #0)
> Another approach is to add an interface for everything we can get: full names,
> URLs, phone numbers, emails, etc. Personas would only implement the interfaces
> corresponding to the information their backend can provide.
> The problem of this is that Individual has to implement all the possible
> interfaces. Do we really want this?

Sounds like a reasonable approach to me.

It also gives us the option in future of making the set of interfaces that each Individual instance implements dynamic, so that an Individual's capabilities are always defined exactly by the set of interfaces it implements. (This is a wild idea: I haven't thought about it much.)

> The naming is highly inconsistent due to all the naming changes in upstream
> folks. I will fix it when libfolks with a final naming will be decided,
> committed and released :)

I think the consensus was to go with “-able” for interfaces like these (https://bugzilla.gnome.org/show_bug.cgi?id=627397#c13), so:
 • Names → Nameable
 • Phone → Phoneable
 • EmailOwner → Emailable

The other interfaces' names (ExtendedInfo, PostalAddressOwner) sound OK to me.

> Another problem of the interfaces is that I tried to keep some of the useful
> things from vcard and make it possible at some level to map from vcard to
> libfolks. For instance in vcards a URL can be something like (and there can be
> multiple URLs):
>   url;type=work;type=home;x-foo=bar:http://www.example.com/
> Mapping this completely in libfolks means that a UrlOwner (or whatever you want
> to call it) has a list of urls, each url is a structure/object that contains a
> string (the actual values) and a hash table (the parameters) that maps from a
> string to a list of strings. Quite ackward to use in C...
> How would you like this API to be?

What you've got looks reasonable from a first look to me, though I haven't tried to use the API (from Vala or C) yet. I can't think of how this could reasonably be made simpler, other than perhaps by ensuring there's a convenience API to “just return me a list of URL strings and forget about all those confusing parameters”.

(In reply to comment #1)
> Sounds like a reasonable approach to me.

It means you cannot add things without modifying libfolks.

> It also gives us the option in future of making the set of interfaces that each
> Individual instance implements dynamic, so that an Individual's capabilities
> are always defined exactly by the set of interfaces it implements. (This is a
> wild idea: I haven't thought about it much.)

How would you do that?

> I think the consensus was to go with “-able” for interfaces like these
> (https://bugzilla.gnome.org/show_bug.cgi?id=627397#c13), so:
>  • Names → Nameable

This is terrible :)

>  • Phone → Phoneable
>  • EmailOwner → Emailable

Isn't this implying an action?

(In reply to comment #2)
> (In reply to comment #1)
> > Sounds like a reasonable approach to me.
> 
> It means you cannot add things without modifying libfolks.

How would you be able to do this with a different approach? I don't see that as a problem.

> > It also gives us the option in future of making the set of interfaces that each
> > Individual instance implements dynamic, so that an Individual's capabilities
> > are always defined exactly by the set of interfaces it implements. (This is a
> > wild idea: I haven't thought about it much.)
> 
> How would you do that?

Would it be possible to do it with g_type_add_interface_dynamic() or have I completely misinterpreted the purpose of that function?

> > I think the consensus was to go with “-able” for interfaces like these
> > (https://bugzilla.gnome.org/show_bug.cgi?id=627397#c13), so:
> >  • Names → Nameable
> 
> This is terrible :)
> 
> >  • Phone → Phoneable
> >  • EmailOwner → Emailable
> 
> Isn't this implying an action?

“This persona can be e-mailed: look, here's its e-mail address. Now go and do the grunt work of implementing SMTP yourself.”

I think it's better than “EmailOwner”, anyway.

Looking mostly at the "big picture" issues in the interfaces and Marco's original comment:

names.vala:
-------------------------
There's obviously overlap between Aliasable.alias and Names.nickname. And there isn't quite a clear r/w vs. r/o distinction, since the e-d-s backend will support writing Names.full_name and Names.structured_name. I suppose we could force Names.nickname to always be r/o even for writable backends, since the contact-set vs. user-set distinction is useful (though, at least in the case of e-d-s, the backing store for Names.nickname can be edited by third-party programs).

field-details.vala:
-------------------------
I think it's fine to let clients modify the parameters 'directly' - we can still control the access through the set function (to do validation, etc). Otherwise we'll need to add mutators to remove parameters (I don't see any other way to remove them as it is).

extended-info.vala:
-------------------------
I see the general appeal of lumping less-common fields together - but the problem is that every backend that implements ExtendedInfo will have to keep up with the addition of new fields each time they build.

But much worse than that, libfolks would break backwards compatability with backends every time ExtendedInfo adds a member or method. Following this procedure:

* make Tpf.Persona implement ExtendedInfo
* install the Tp backend
* add a field and function to ExtendedInfo
* install newer libfolks but /don't/ rebuild or install the Tp backend
* execute the following code (thrown into Individual._set_personas() for simplicity):

      foreach (var p in persona_list)
        {
          if (p is ExtendedInfo)
            {
              var ei = p as ExtendedInfo;
              GLib.message ("%p is also an ExtendedInfo; persona hash: %p; got int: %d",
                  ei,
                  ei.string_persona_hash,
                  ei.get_int_from_string_persona ("bar baz", null));
            }
        }

This leads to three serious issues:

* we get a critical warning when constructing a Tpf.Persona, since it doesn't implement the new property 'string-persona-hash'
* we segfault when trying to access the string-persona-hash property of the Tpf.Persona because the C code is:

      GHashTable* folks_extended_info_get_string_persona_hash (FolksExtendedInfo* self) {
        return FOLKS_EXTENDED_INFO_GET_INTERFACE (self)->get_string_persona_hash (self);
      }

but the TpfPersona get_string_persona_hash() pointer was never filled in, so we execute some random garbage.

* we similarly segfault when attemping to execute the unimplemented get_int_from_string_persona () method

The last two points should be fixed in Vala, so I've filed bug #638434. But we would still risk hitting the first point, which I still think is an issue (especially for debugging).

On the other hand, if we created an interface per (group of) detail(s), older backends simply wouldn't claim to implement them and we shouldn't hit this issue.

We certainly don't want Individual implementing a ton of interfaces but I think we'll hit a natural limit after we add the obvious vCard-inspired fields. As I said in my talk a few weeks ago, I think the best goal for Folks is to be a powerful address book and limit ourselves to that. Some of the fancier bits (like correlating peoples of people to their Individual) belong in external stores (in that case, something like Tracker) to keep Folks relatively lean.

One other detail to think about is arbitrary fields. It seems like the best way to support that is to have some interface that acts a bit like EVCard, in that we can store and retrieve data corresponding to an arbitrary string. We'll want to make it clear that it should only be used for trivial amounts of data. Maybe we could split Gender and URLOwner into their own interfaces and use ExtendedInfo for the arbitrary fields.

I think part of the reason EVCard is painful to use is because accessing the common fields (email, name, etc.) requires using a 'catch-all' API. If clients only had to use the catch-all for a couple of fields they use, it shouldn't be so bad. We still have to worry about managing the FieldDetail for each value, but even in C, that doesn't seem too bad (especially if you leave the parameters empty, which I think would be common - we'll need to always handle an empty hash table when examining the parameters).

postal-address-owner.vala
-------------------------
I guess for the sake of vCard compatibility we'll need to keep postal addresses structured like this, but I am strongly tempted to lean toward our conclusion from the Maemo 5 postal fields: postal addresses are human-readable flat strings that should not be machine-formatted. The only time they should be machine-interpretted is when they're copied, verbatim (as a simple string), into a maps program/website.

Displaying structured addresses properly seems like a huge mess. I tried to get an idea of how Evolution does it, but at quick glance, it seems like it just has a static ordering. To do things properly in a UI, it would need to localize the display each postal address, potentially adjust it upon edit.

If we just stored a simple flat string, maybe we could export it to vCard like this?:

instead of:

ADR;TYPE=WORK:;;100 Waters Edge;Baytown;LA;30314;United States of America
LABEL;TYPE=WORK:100 Waters Edge\nBaytown, LA 30314\nUnited States of America

->

ADR;TYPE=WORK:;;;;;;
LABEL;TYPE=WORK:100 Waters Edge\nBaytown, LA 30314\nUnited States of America

or

ADR;TYPE=WORK:;;100 Waters Edge\nBaytown, LA 30314\nUnited States of America;;;;
LABEL;TYPE=WORK:100 Waters Edge\nBaytown, LA 30314\nUnited States of America

The latter would probably be the best option. I'm not sure how well it'd be interpretted by most programs that would import it. But I seriously question the benefits of keeping this a structured field. And maybe we could even export to structured vCard through a utility library that interprets the address.

+      /* FIXME: Detect duplicates somehow? */

Similarly, I don't think we should bother trying to find duplicates (aside from basic case-insensitive matches). Finding duplicates is workable for phone numbers, but I think it's beyond the scope of Folks to make sense of postal addresses at that level.

> Another problem of the interfaces is that I tried to keep some of the useful
> things from vcard and make it possible at some level to map from vcard to
> libfolks. For instance in vcards a URL can be something like (and there can be
> multiple URLs):
>   url;type=work;type=home;x-foo=bar:http://www.example.com/
> Mapping this completely in libfolks means that a UrlOwner (or whatever you want
> to call it) has a list of urls, each url is a structure/object that contains a
> string (the actual values) and a hash table (the parameters) that maps from a
> string to a list of strings. Quite ackward to use in C...
> How would you like this API to be?

Generally, I think this API is OK as long as we make a point to handle TYPE-less fields as well as we handle TYPE-ful fields. In my own experience, by far the most common type I have is <blank>, followed by Home or Work.

Commit 79c1a0b3c5f08948c2b956568ccc215de13d7a38 (which implements ExtendedInfo in Individual) also removes the Avatar and Favouritable interfaces from Individual without replacing their functionality (and without stripping the fields and functions to support them).

(In reply to comment #3)
> > This is terrible :)
> > 
> > >  • Phone → Phoneable
> > >  • EmailOwner → Emailable
> > 
> > Isn't this implying an action?
> 
> “This persona can be e-mailed: look, here's its e-mail address. Now go and do
> the grunt work of implementing SMTP yourself.”
> 
> I think it's better than “EmailOwner”, anyway.

I agree. Most of these fields are actionable (otherwise they wouldn't be terribly interesting), but that's left as an exercise for external programs.

"*Owner" is for the un-actionable fields (avatar, presence, etc.).

(In reply to comment #4)
> names.vala:
> -------------------------
> There's obviously overlap between Aliasable.alias and Names.nickname. And there
> isn't quite a clear r/w vs. r/o distinction, since the e-d-s backend will
> support writing Names.full_name and Names.structured_name. I suppose we could
> force Names.nickname to always be r/o even for writable backends, since the
> contact-set vs. user-set distinction is useful (though, at least in the case of
> e-d-s, the backing store for Names.nickname can be edited by third-party
> programs).

Not sure, but the whole writable / non-writable thing is a bit confused atm, so I'm not sure about the best approach.
Are you also suggesting that an application should use EDS directly to modify contacts?

> field-details.vala:
> -------------------------
> I think it's fine to let clients modify the parameters 'directly' - we can
> still control the access through the set function (to do validation, etc).
> Otherwise we'll need to add mutators to remove parameters (I don't see any
> other way to remove them as it is).

What do you mean? The hash table is public, but some utility methods to modify the HashTable<string, List<string>> of parameters could be convenient and avoid bugs. We should also add methods to remove parameters, I'm sure that a large number of people that will have to write code in C to delete an element from a GList that is a value of a GHashTable will get it somewhat wrong.

> extended-info.vala:
> -------------------------
> I see the general appeal of lumping less-common fields together - but the
> problem is that every backend that implements ExtendedInfo will have to keep up
> with the addition of new fields each time they build.

Yeah, I reach the same conclusion. I blame Sjoerd for having the “misc stuff” interface :P

> We certainly don't want Individual implementing a ton of interfaces but I think
> we'll hit a natural limit after we add the obvious vCard-inspired fields. As I
> said in my talk a few weeks ago, I think the best goal for Folks is to be a
> powerful address book and limit ourselves to that. Some of the fancier bits
> (like correlating peoples of people to their Individual) belong in external
> stores (in that case, something like Tracker) to keep Folks relatively lean.

QtContacts has 20 different details, so I guess we will add a similar number of interfaces. Isn't it a bit too much?

> One other detail to think about is arbitrary fields. It seems like the best way
> to support that is to have some interface that acts a bit like EVCard, in that
> we can store and retrieve data corresponding to an arbitrary string. We'll want
> to make it clear that it should only be used for trivial amounts of data. Maybe
> we could split Gender and URLOwner into their own interfaces and use
> ExtendedInfo for the arbitrary fields.

I thought about that too, but it will lead to new problems. For instance if you initially have a “url” field in ExtendedInfo and later you add a URLOwner interface. Will you have to implement both?

> postal-address-owner.vala
> -------------------------
> I guess for the sake of vCard compatibility we'll need to keep postal addresses
> structured like this, but I am strongly tempted to lean toward our conclusion
> from the Maemo 5 postal fields: postal addresses are human-readable flat
> strings that should not be machine-formatted. The only time they should be
> machine-interpretted is when they're copied, verbatim (as a simple string),
> into a maps program/website.

Something that I thought was to have a structured address and free text address (like we have FN and N), but EDS doesn't support that (LABEL doesn't mean what you think it means) so it would be completely unused.

> Displaying structured addresses properly seems like a huge mess.

I know. I think that EDS has an extra field that allows to tell you how to format addresses, but it doesn't seem to be used.

> ADR;TYPE=WORK:;;;;;;

This means almost all programs will ignore the address.

> LABEL;TYPE=WORK:100 Waters Edge\nBaytown, LA 30314\nUnited States of America

LABEL is what you would write on a label to send a letter or something else. It can include also the name of the person receiving it and details about the delivery.

> or
> 
> ADR;TYPE=WORK:;;100 Waters Edge\nBaytown, LA 30314\nUnited States of
> America;;;;

This will make several apps really unhappy.

> LABEL;TYPE=WORK:100 Waters Edge\nBaytown, LA 30314\nUnited States of America

Most apps seem to ignore LABEL or show it as an unrelated field. Note that programs have no way to know that the pevious LABEL is the same address as the ADDR. (There would be groups, but I never saw them used.)

> The latter would probably be the best option. I'm not sure how well it'd be
> interpretted by most programs that would import it. But I seriously question
> the benefits of keeping this a structured field. And maybe we could even export

I question that too, but if the only backend that has addresses uses them I don't see other ways of dealing with this.
What if you use libfolks to access the contacts and modify them? Will you change a contact (that maybe is synced with another device) losing some information?

> +      /* FIXME: Detect duplicates somehow? */
> 
> Similarly, I don't think we should bother trying to find duplicates (aside from
> basic case-insensitive matches). Finding duplicates is workable for phone
> numbers, but I think it's beyond the scope of Folks to make sense of postal
> addresses at that level.

It could be useful if you have different backends supporting PostalAddress to avoid duplicates. But yes, I don't really plan to implement it soon.


(In reply to comment #5)
> Commit 79c1a0b3c5f08948c2b956568ccc215de13d7a38 (which implements ExtendedInfo
> in Individual) also removes the Avatar and Favouritable interfaces from
> Individual without replacing their functionality (and without stripping the
> fields and functions to support them).

I guess that happened by mistake while rebasing my branched. The change of the interface names meant a lot of conflicts.


(In reply to comment #6)
> I agree. Most of these fields are actionable (otherwise they wouldn't be
> terribly interesting), but that's left as an exercise for external programs.

URLable?

(In reply to comment #7)
> (In reply to comment #4)

> Not sure, but the whole writable / non-writable thing is a bit confused atm, so
> I'm not sure about the best approach.
> Are you also suggesting that an application should use EDS directly to modify
> contacts?

No, I'd hope we'd have some editor that works on Folks and makes the e-d-s editor unnecessary (eventually), but just warning that other applications can modify the contacts in e-d-s (eg, changing their N/FN entries) behind our back.

In keeping with our general scheme of "1 writable backend (highest priority), n readable backends", writes to e-d-s through the e-d-s backend and writes directly to e-d-s should end up doing similar work most of the time, so hopefully edits to e-d-s directly won't cause us trouble. We'll definitely need some solid test cases around this.

> What do you mean? The hash table is public, but some utility methods to modify
> the HashTable<string, List<string>> of parameters could be convenient and avoid
> bugs. We should also add methods to remove parameters, I'm sure that a large
> number of people that will have to write code in C to delete an element from a
> GList that is a value of a GHashTable will get it somewhat wrong.

Here's the hash table (from your eds-backend branch):

public HashTable<string, List<string>> parameters { get; private set; }

so we can't do an absolute set. I'm suggesting we replace the "private set" with set function that does any necessary validation, so external programs can set the parameters by passing a HashTable if it's more convenient than clearing them and iterating calls to {set,add,extend}_parameter ().

> > We certainly don't want Individual implementing a ton of interfaces but I think
> > we'll hit a natural limit after we add the obvious vCard-inspired fields. As I
> > said in my talk a few weeks ago, I think the best goal for Folks is to be a
> > powerful address book and limit ourselves to that. Some of the fancier bits
> > (like correlating peoples of people to their Individual) belong in external
> > stores (in that case, something like Tracker) to keep Folks relatively lean.
> 
> QtContacts has 20 different details, so I guess we will add a similar number of
> interfaces. Isn't it a bit too much?

Looking at the set from this doc page:
http://doc.qt.nokia.com/qtmobility-1.1-tp/qcontactdetail.html

Already supported
-----------------
QContactAvatar
QContactDisplayLabel
QContactGuid
QContactGlobalPresence
QContactOnlineAccount
QContactPresence
QContactThumbnail
QContactType

Will be supported by this branch
--------------------------------
QContactAddress
QContactEmailAddress
QContactGender
QContactName
QContactNickname
QContactPhoneNumber
QContactUrl

Should be supported in the future
---------------------------------
QContactBirthday
QContactGeoLocation
QContactNote
QContactTimestamp

Questionable utility and/or could be
handled by arbitrary-field support
------------------------------------
QContactAnniversary
QContactFamily
QContactOrganization
QContactRingtone
QContactSyncTarget
QContactTag

So, I count 11 new interfaces (10 of these fields + 1 arbitrary field interface) beyond what we've got in origin/master. That would bring us up to 17 (+ 1 for Object) interfaces/superclasses for Individual. Most backends would implement most of the new and existing interfaces in their Persona implementation.

I'll try to implement an example program around the time we merge the e-d-s backend to make sure it's feasible to reliably track trivial arbitrary data (eg, the last group of fields above) without needing to explicitly create an interface for each of them.

In fact, we could probably push the gender, note, and url fields into the last group, since they're trivial bits of data.

> > One other detail to think about is arbitrary fields. It seems like the best way
> > to support that is to have some interface that acts a bit like EVCard, in that
> > we can store and retrieve data corresponding to an arbitrary string. We'll want
> > to make it clear that it should only be used for trivial amounts of data. Maybe
> > we could split Gender and URLOwner into their own interfaces and use
> > ExtendedInfo for the arbitrary fields.
> 
> I thought about that too, but it will lead to new problems. For instance if you
> initially have a “url” field in ExtendedInfo and later you add a URLOwner
> interface. Will you have to implement both?

I think we could have a policy that once we "support" a field in the arbitrary field interface (eg, define a string for its name, like EVC_URL) we won't implement an interface for it. And I'm thinking in terms of vCard, where we'd have API like:

  HashTable<string, List<FieldDetail>> get_values (string field_id);

not with pre-defined functions for each type of field, as we have in ExtendedInfo now.

I see 2 main use cases for arbitrary fields here:

"common" fields
===============
(like Anniversary or Organization) that don't seem to warrant their own interface. What qualifies a field to have its own interface or not is really a judgement call. But I'll be happy to give my opinions and discuss the merits either way for each field :)

I think it'd make sense for us to refer to these by their well-known vCard name (and define those strings like EVCard does) as an ID.

app-specific fields
===================
These are simple key/value pairs that external applications may associate with people, since it would be a large hassle for applications to maintain a database if they only need to associate a couple of details per person.

The rule of thumb would be something like "if you need more than a 3 fields per person or structured data for any field, manage your own database with Individual IDs as foreign keys". Eg, an app that stores musical interests or photos for people would definitely need its own database. 

These would use application-chosen vCard-style IDs (eg, "X-FOOAPP-FAMILY-ID").

> > postal-address-owner.vala
> > -------------------------
> > I guess for the sake of vCard compatibility we'll need to keep postal addresses
> > structured like this, but I am strongly tempted to lean toward our conclusion
> > from the Maemo 5 postal fields: postal addresses are human-readable flat
> > strings that should not be machine-formatted. The only time they should be
> > machine-interpretted is when they're copied, verbatim (as a simple string),
> > into a maps program/website.
> 
> Something that I thought was to have a structured address and free text address
> (like we have FN and N), but EDS doesn't support that (LABEL doesn't mean what
> you think it means) so it would be completely unused.

I'm not sure it would really be used if it were in a structured format. I know all I've ever done with the addresses in Evolution is write them down or re-type them into Google Maps.

I looked at the Google Maps API a bit, and the only place I could find anything that was reasonably structured was in the Geocoding API. That API converts a human-readable flat string or lat/long pair into the following return type (which is a superset of the components in ADR):

http://code.google.com/apis/maps/documentation/geocoding/#Types

So, in short, it looks like the only addresses Google Maps even takes as input are a flat address string or lat/long pair.

As far as I can tell, OpenStreeMap doesn't accept structured address input in its main API. There's OpenStreetMap Nominatim, which is a map and service API. You can pass it structured details (country, city, street, building number), but it doesn't seem to work at this point (its API for searching by flat string does work though).

Yahoo Maps seems to allow (street, city, state, zip code) or a simple string containing any or all of those. And it seems to be US-specific (which makes the whole issue for them a lot easier, and we obviously don't have that luxury).

Bing Maps allows both structured and unstructured lookups.

Ovi Maps on Maemo 5 allows unstructured queries and possibly structured queries (I'm not sure, but I think Contacts converts the structured fields into a flat string when it passes it to Maps).

So the TL;DR is: all the major maps APIs allow unstructured queries and a couple of them allow structured queries. If our main use case for postal addresses is opening them up on a map, I think a flat string works just fine. And of course it works perfectly for the major concern of localization.

> > Displaying structured addresses properly seems like a huge mess.
> 
> I know. I think that EDS has an extra field that allows to tell you how to
> format addresses, but it doesn't seem to be used.

Yeah - I'm not sure that's ever been used.

> > ADR;TYPE=WORK:;;;;;;
> 
> This means almost all programs will ignore the address.

Fair point.

> > LABEL;TYPE=WORK:100 Waters Edge\nBaytown, LA 30314\nUnited States of America
> 
> LABEL is what you would write on a label to send a letter or something else. It
> can include also the name of the person receiving it and details about the
> delivery.

I wonder how often it includes "deep addressing" like that ("Legal Dept., ATTN: John Smith"). My guess would be /very rarely/, to the point that it might be OK to conflate the two.

> > or
> > 
> > ADR;TYPE=WORK:;;100 Waters Edge\nBaytown, LA 30314\nUnited States of
> > America;;;;
> 
> This will make several apps really unhappy.

Yeah, best to avoid this.

> > LABEL;TYPE=WORK:100 Waters Edge\nBaytown, LA 30314\nUnited States of America
> 
> Most apps seem to ignore LABEL or show it as an unrelated field. Note that
> programs have no way to know that the pevious LABEL is the same address as the
> ADDR. (There would be groups, but I never saw them used.)

Right. I just looked at RFC 2426 for the LABEL definition:
 
http://tools.ietf.org/html/rfc2426#page-13

The example indicates that you can add "deep addressing" ("Mr.John Q. Public\, Esq.\nMail Drop: TNE QB"), though it doesn't actually say how LABELs are even supposed to be correlated with ADRs (if at all). I guess the assumption is that it's based on matching TYPE lists (not necessarily the ordering of the entries).

I also verified what happens if you include those extra details in Google Maps - it treats it like "Mr. John Q Public near 123 Main Street, ...", so it does a proximity search (which is probably what most people mean when they prepend details to a US address).

> > The latter would probably be the best option. I'm not sure how well it'd be
> > interpretted by most programs that would import it. But I seriously question
> > the benefits of keeping this a structured field. And maybe we could even export
> 
> I question that too, but if the only backend that has addresses uses them I
> don't see other ways of dealing with this.
> What if you use libfolks to access the contacts and modify them? Will you
> change a contact (that maybe is synced with another device) losing some
> information?

The is the major downside to not supporting structured addresses. But there are two sides to it:

Importing structured addresses:

Facebook has: street, city, zip code, neighborhood, residence, room, mailbox, so I don't think we could have a universal transform. But I couldn't find which API actually contains this. Is it in the Graph 'location' person result? The doc says it contains 'name' and 'id', but it's not clear if there are other parameters.

At any rate, do you know if Facebook has API for sending the location as a flat string (or if it's contained in that 'location' result)?

Flickr has: hometown, current (city, country). I imagine for almost all locales it'd be acceptable to import that as "%s, %s" % (city, country)

Exporting structured addresses:

I think our short-term answer should just be "we strip out ADR out when we export but include a LABEL" to minimize the data loss. For certain applications, they could use an external geocoding API (like Google's) to generate the structured ADR. I think for a dedicated exporter tool, it'd be reasonable to have a preference to turn that on or off (with the option to turn it off for a specific export if the device doesn't have connectivity at the time).

> (In reply to comment #5)
> > Commit 79c1a0b3c5f08948c2b956568ccc215de13d7a38 (which implements ExtendedInfo
> > in Individual) also removes the Avatar and Favouritable interfaces from
> > Individual without replacing their functionality (and without stripping the
> > fields and functions to support them).
> 
> I guess that happened by mistake while rebasing my branched. The change of the
> interface names meant a lot of conflicts.
> 
> 
> (In reply to comment #6)
> > I agree. Most of these fields are actionable (otherwise they wouldn't be
> > terribly interesting), but that's left as an exercise for external programs.
> 
> URLable?

Sure. It's kind of ugly, but at least consistent. URLs are certainly actionable.

(In reply to comment #8)
> In fact, we could probably push the gender, note, and url fields into the last
> group, since they're trivial bits of data.

URIs are quite common pieces of information exposed by various potential backends — I could imagine that they might be exposed on personas of various backends in the future, backends which don't expose much else. I think it might be prudent to keep URIs in a separate “URLable” (or whatever) interface.

> "common" fields
> ===============
> (like Anniversary or Organization) that don't seem to warrant their own
> interface. What qualifies a field to have its own interface or not is really a
> judgement call. But I'll be happy to give my opinions and discuss the merits
> either way for each field :)

There is a case for having a “personal information” interface which exposes things like anniversary, spouse, birthday and family. In the one interface, since they're all related and will a backend will (probably?) expose all of them if it exposes one.

I'd like to keep the number of properties exposed as arbitrary fields to a minimum.

> app-specific fields
> ===================
> These are simple key/value pairs that external applications may associate with
> people, since it would be a large hassle for applications to maintain a
> database if they only need to associate a couple of details per person.
> 
> The rule of thumb would be something like "if you need more than a 3 fields per
> person or structured data for any field, manage your own database with
> Individual IDs as foreign keys". Eg, an app that stores musical interests or
> photos for people would definitely need its own database. 
> 
> These would use application-chosen vCard-style IDs (eg, "X-FOOAPP-FAMILY-ID").

I like!

Another bit:

* EmailOwner.emails -> EmailOwner.email_addresses

for accuracy and consistency with IMable.im_addresses and PostalAddressOwner.postal_addresses. This matters since we have to use the names of these properties as IDs when determining links in IndividualAggregator.

The gtkdoc for the PostalAddressOwner constructor is missing @params for country and types.

I've started implementing the collective suggestions here in a new branch - will push for review as soon as I'm done.

Created attachment 178768 [details] [review]
Add most of the new interfaces required by advanced backends and add eds backend

Note that this still isn't the final version of anything, but it's a major step toward the goal.

I took Raul's latest (at the time) e-d-s branch and did the following:

* removed the libsocialweb backend, since it needs a bit more work before it can be merged
* removed the PostalAddressOwner interface, since we haven't finalized the best way to handle structured vs. unstructured addresses (though I plan to shortly after this gets merged)
* split ExtendedInfo into URLable and GenderOwner (an even better example of our naming scheme getting ugly)
* made the other renames suggested by pwith
* set up GConf in a sandbox for the e-d-s tests (this will be merge-deferred till we get to that backend)
* etc.

There are two version of my new branch. The first leaves nearly all my changes as new commits (to make it easier to sort out exactly what I've changed):

http://git.collabora.co.uk/?p=user/treitter/folks.git;a=shortlog;h=refs/heads/bgo638279--interfaces--pre-squash

The second applies the squashing suggested in their commit messages:

http://git.collabora.co.uk/?p=user/treitter/folks.git;a=shortlog;h=refs/heads/bgo638279--interfaces--squashed

In case of doubt, the second branch wins. It leaves the e-d-s backend commits separate, so Raul should be able to rebase his latest work against it directly, then squash the commits as suggested (to make rebasing a little easier, hopefully).

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

Before final merging, I plan to rename Nameable (once again) to NameOwner, since it's non-actionable. I didn't realize that until I'd done most of this work, and these changes were fairly massive as it is.

Also before final merging, I'll want to add plenty of generic tests for these interfaces (and preferably some for how they relate to the Tp backend; the e-d-s backend already includes some such tests).

Review of attachment 178768 [details] [review]:

Here's a review of the interfacey bits. I haven't looked at this version of the e-d-s backend yet.

::: folks/emailable.vala
@@ +1,2 @@
+/*
+ * Copyright (C) 2010 Collabora Ltd.

s/2010/2011/ (in several files)

@@ +22,3 @@
+
+/**
+ * Interface for classes that have email addressed, such as {@link Persona}

s/addressed/addresses/

::: folks/field-details.vala
@@ +23,3 @@
+/**
+ * Object representing a string value that can have some parameters
+ * associated to it.

s/associated to/associated with/ (and again below)

@@ +38,3 @@
+  /* The value of the field.
+   *
+   * The value of the field, the exact content depends on what  the

Double space between “what” and “the”.

@@ +50,3 @@
+   * A hash table of the parameters associated to
+   * {@link Folks.FieldDetails.value}. The keys are the names of the
+   * parameters, while the values as list of strings.

“the values are a list of strings”?

@@ +79,3 @@
+   * @param parameter_name the parameter name
+   * @return a list of values for `parameter_name` or `null` (i.e. an empty
+   * list) if the are no such parameters.

“there are”

@@ +118,3 @@
+                  break;
+                }
+            }

Could this not be done using values.find_custom(parameter_value, strcmp)?

@@ +153,3 @@
+  public void extend_parameters (HashTable<string, List<string>> additional)
+    {
+      additional.for_each ((k, v) =>

We should try to avoid lambda functions and use foreach{} loops instead. Lambda functions have a runtime overhead (a function call for each loop iteration), and make the generated C code harder to understand and debug.

@@ +158,3 @@
+            unowned List<string> values = (List<string>) v;
+            foreach (unowned string val in values)
+              add_parameter (name, val);

this.add_parameter

::: folks/individual.vala
@@ +233,3 @@
+   * {@inheritDoc}
+   */
+  public string nickname { get { return this._nickname; } }

Why doesn't this have a private setter?

@@ +886,3 @@
+                    {
+                      this.structured_name = new_value;
+                      this.notify_property ("structured-name");

Shouldn't setting the structured name using this.structured_name automatically send a notification?

@@ +907,3 @@
+                    {
+                      this.full_name = new_value;
+                      this.notify_property ("full-name");

See above comment about structured names.

@@ +928,3 @@
+                    {
+                      this._nickname = new_value;
+                      this.notify_property ("nickname");

See above comment about structured names.

@@ +1006,3 @@
+      this._urls = urls_set.get_values ();
+
+      this.notify_property ("urls");

It might be an idea to add a comment here explaining that we're not just assigning to this.urls so that we avoid iterating through the list again unnecessarily.

::: folks/nameable.vala
@@ +42,3 @@
+    {
+      get { return _family_name; }
+      construct set { _family_name = value != null ? value : ""; }

All these member variable accesses should be using “this.[foo]”.

::: folks/persona-store.vala
@@ +96,3 @@
+   * The {@link PersonaStore} doesn't support write operations.
+   *
+   * @since 0.3.4

0.3.UNRELEASED?

(In reply to comment #14)
> Review of attachment 178768 [details] [review]:
> 
> Here's a review of the interfacey bits. I haven't looked at this version of the
> e-d-s backend yet.

Yeah, no need on the e-d-s backend - it needs a bit more work before the next review (eg, its tests fail). See bug #638281

> ::: folks/emailable.vala
> @@ +1,2 @@
> +/*
> + * Copyright (C) 2010 Collabora Ltd.
> 
> s/2010/2011/ (in several files)

Fixed

> @@ +22,3 @@
> +
> +/**
> + * Interface for classes that have email addressed, such as {@link Persona}
> 
> s/addressed/addresses/

Thanks - I didn't review all the comments carefully.

> ::: folks/field-details.vala
> @@ +23,3 @@
> +/**
> + * Object representing a string value that can have some parameters
> + * associated to it.
> 
> s/associated to/associated with/ (and again below)

Fixed

> @@ +38,3 @@
> +  /* The value of the field.
> +   *
> +   * The value of the field, the exact content depends on what  the
> 
> Double space between “what” and “the”.

Fixed

> @@ +50,3 @@
> +   * A hash table of the parameters associated to
> +   * {@link Folks.FieldDetails.value}. The keys are the names of the
> +   * parameters, while the values as list of strings.
> 
> “the values are a list of strings”?

Yup, fixed.

> @@ +79,3 @@
> +   * @param parameter_name the parameter name
> +   * @return a list of values for `parameter_name` or `null` (i.e. an empty
> +   * list) if the are no such parameters.
> 
> “there are”
> 
> @@ +118,3 @@
> +                  break;
> +                }
> +            }
> 
> Could this not be done using values.find_custom(parameter_value, strcmp)?

I think so - fixed.

> @@ +153,3 @@
> +  public void extend_parameters (HashTable<string, List<string>> additional)
> +    {
> +      additional.for_each ((k, v) =>
> 
> We should try to avoid lambda functions and use foreach{} loops instead. Lambda
> functions have a runtime overhead (a function call for each loop iteration),
> and make the generated C code harder to understand and debug.

Agreed, though I don't think we can use a foreach here - the parameter is a HashTable, which doesn't implement Iterator (and we can't use an iterator type as the parameter unless we want to make libgee a hard requirement for clients.

Or did you mean copying the HashTable contents into a HashMap, then using foreach?

> @@ +158,3 @@
> +            unowned List<string> values = (List<string>) v;
> +            foreach (unowned string val in values)
> +              add_parameter (name, val);
> 
> this.add_parameter

Fixed.

> ::: folks/individual.vala
> @@ +233,3 @@
> +   * {@inheritDoc}
> +   */
> +  public string nickname { get { return this._nickname; } }
> 
> Why doesn't this have a private setter?

The problem is that if we add a private setter, we have to add a protected or internal Nameable.nickname.set(), which means a public folks_nameable_set_nickname() in the C API.

Vala doesn't allow us to make a private Nameable.nickname.set() with private implementations.

> @@ +886,3 @@
> +                    {
> +                      this.structured_name = new_value;
> +                      this.notify_property ("structured-name");
> 
> Shouldn't setting the structured name using this.structured_name automatically
> send a notification?

Yup - I fixed a few of these but apparently didn't get them all.

> @@ +907,3 @@
> +                    {
> +                      this.full_name = new_value;
> +                      this.notify_property ("full-name");
> 
> See above comment about structured names.

Fixed.

> @@ +928,3 @@
> +                    {
> +                      this._nickname = new_value;
> +                      this.notify_property ("nickname");
> 
> See above comment about structured names.

Interestingly, we can't do this here, since it only works for properties with public setters (!).

> @@ +1006,3 @@
> +      this._urls = urls_set.get_values ();
> +
> +      this.notify_property ("urls");
> 
> It might be an idea to add a comment here explaining that we're not just
> assigning to this.urls so that we avoid iterating through the list again
> unnecessarily.

Fixed.

> ::: folks/nameable.vala
> @@ +42,3 @@
> +    {
> +      get { return _family_name; }
> +      construct set { _family_name = value != null ? value : ""; }
> 
> All these member variable accesses should be using “this.[foo]”.

Fixed.

> ::: folks/persona-store.vala
> @@ +96,3 @@
> +   * The {@link PersonaStore} doesn't support write operations.
> +   *
> +   * @since 0.3.4
> 
> 0.3.UNRELEASED?

Fixed

Created attachment 178804 [details] [review]
Add most of the new interfaces required by advanced backends and add eds backend

Squashed diff of newer version of the branch addressing pwith's latest comments:

http://git.collabora.co.uk/?p=user/treitter/folks.git;a=shortlog;h=refs/heads/bgo638279--interfaces--squashed-2-rebase

(In reply to comment #15)
> > @@ +153,3 @@
> > +  public void extend_parameters (HashTable<string, List<string>> additional)
> > +    {
> > +      additional.for_each ((k, v) =>
> > 
> > We should try to avoid lambda functions and use foreach{} loops instead. Lambda
> > functions have a runtime overhead (a function call for each loop iteration),
> > and make the generated C code harder to understand and debug.
> 
> Agreed, though I don't think we can use a foreach here - the parameter is a
> HashTable, which doesn't implement Iterator (and we can't use an iterator type
> as the parameter unless we want to make libgee a hard requirement for clients.
> 
> Or did you mean copying the HashTable contents into a HashMap, then using
> foreach?

For GLib HashTables, we can use HashTableIter. e.g.:

var iter = new HashTableIter (hash_table);
while (iter.next (out unowned key, out unowned value))
  {
    // Do stuff
  }

> > ::: folks/individual.vala
> > @@ +233,3 @@
> > +   * {@inheritDoc}
> > +   */
> > +  public string nickname { get { return this._nickname; } }
> > 
> > Why doesn't this have a private setter?
> 
> The problem is that if we add a private setter, we have to add a protected or
> internal Nameable.nickname.set(), which means a public
> folks_nameable_set_nickname() in the C API.
> 
> Vala doesn't allow us to make a private Nameable.nickname.set() with private
> implementations.

Ah, right. That'll have to do, then.

Created attachment 178876 [details] [review]
Add most of the new interfaces required by advanced backends and add eds backend

Squashed diff of branch:

http://git.collabora.co.uk/?p=user/treitter/folks.git;a=shortlog;h=refs/heads/bgo638279--interfaces--squashed-3

This replaces a use of HashTable.for_each() with use of a HashTableIter, as suggested by Philip and fixes the e-d-s backend commit message references to earlier commits.

(In reply to comment #17)
> (In reply to comment #15)
> > > @@ +153,3 @@
> > > +  public void extend_parameters (HashTable<string, List<string>> additional)
> > > +    {
> > > +      additional.for_each ((k, v) =>
> > > 
> > > We should try to avoid lambda functions and use foreach{} loops instead. Lambda
> > > functions have a runtime overhead (a function call for each loop iteration),
> > > and make the generated C code harder to understand and debug.
> > 
> > Agreed, though I don't think we can use a foreach here - the parameter is a
> > HashTable, which doesn't implement Iterator (and we can't use an iterator type
> > as the parameter unless we want to make libgee a hard requirement for clients.
> > 
> > Or did you mean copying the HashTable contents into a HashMap, then using
> > foreach?
> 
> For GLib HashTables, we can use HashTableIter. e.g.:
> 
> var iter = new HashTableIter (hash_table);
> while (iter.next (out unowned key, out unowned value))
>   {
>     // Do stuff
>   }

Fixed in the newer branch - I thought you had meant foreach() literally.

Created attachment 179011 [details] [review]
Add tests for FieldDetails

This is the last patch from this branch:

http://git.collabora.co.uk/?p=user/treitter/folks.git;a=shortlog;h=refs/heads/bgo638279--interfaces--squashed-4

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

This branch is ready to be merged as soon as this last patch gets approval. We'll follow it up with the e-d-s backend branch as soon as it's ready.

Review of attachment 179011 [details] [review]:

::: tests/folks/field-details.vala
@@ +39,3 @@
+          assert (v == values_1[i]);
+          i++;
+        });

Use a for{} instead of a lambda function and manually incremented counter?

@@ +50,3 @@
+          assert (v == values_2[i]);
+          i++;
+        });

See above.

@@ +82,3 @@
+          assert (values.nth_data (i) == val);
+          i++;
+        }

Again, a for{} loop might be better (I think its intention might be clearer).

Add some more goal bugs for the 0.3.5 release.

(Actually setting the new milestone for these bugs)

(In reply to comment #21)
> Review of attachment 179011 [details] [review]:
...
> Use a for{} instead of a lambda function and manually incremented counter?

Fixed.

Merged as:

commit bc92884e49adf7d424467b6b21a9689172c02831
Author: Travis Reitter <travis.reitter@collabora.co.uk>
Date:   Fri Jan 21 13:49:10 2011 -0800

    Add FieldDetails tests.
    
    Helps bgo#638279 - Add interfaces for phone, URLs, emails, etc.

 configure.ac                   |    1 +
 tests/folks/Makefile.am        |    6 +++
 tests/folks/field-details.vala |   86 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 0 deletions(-)

commit 236ea55666acbb8c0b21af935d95155c9f25c140
Author: Raul Gutierrez Segales <raul.gutierrez.segales@collabora.co.uk>
Date:   Mon Jan 10 19:30:33 2011 +0000

    Added the READ_ONLY PersonaStore error

 folks/persona-store.vala |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

commit 73e7a6342455daedae51628761a843117c6cc772
Author: Travis Reitter <travis.reitter@collabora.co.uk>
Date:   Mon Jan 17 15:47:46 2011 -0800

    Don't print out a stacktrace when an error message is empty.
    
    This was printing a lot of stacktraces for non-errors from GConf.

 tests/lib/test-case.vala |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

commit 89df5dc2da998801cebe0c4bfa79fc1b5671ce3d
Author: Marco Barisione <marco@barisione.org>
Date:   Tue Dec 28 17:42:18 2010 +0100

    Add Emailable for contacts with emails

 folks/Makefile.am     |    1 +
 folks/emailable.vala  |   37 ++++++++++++++++++++++++++++++++++
 folks/individual.vala |   53 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+), 0 deletions(-)

commit 9ea499303e6dceb462b34b930e2065c7668ac753
Author: Marco Barisione <marco@barisione.org>
Date:   Mon Dec 20 12:37:50 2010 +0000

    Add Phoneable interface for contacts with phone numbers

 folks/Makefile.am     |    1 +
 folks/individual.vala |   55 ++++++++++++++++++++++++++++++++++--------------
 folks/phoneable.vala  |   39 ++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 16 deletions(-)

commit 281a329984c46c07ac40cc2d74da1aa9305faf30
Author: Travis Reitter <travis.reitter@collabora.co.uk>
Date:   Tue Jan 18 13:57:44 2011 -0800

    Add GenderOwner interface for specifying contact gender

 folks/Makefile.am       |    1 +
 folks/gender-owner.vala |   58 ++++++++++++++++++++++++++++++
 folks/individual.vala   |   90 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 149 insertions(+), 0 deletions(-)

commit 4b82bef6acfeedac468ce08733e222cd0ddaa078
Author: Travis Reitter <travis.reitter@collabora.co.uk>
Date:   Tue Jan 18 13:30:33 2011 -0800

    Add URLable interface for contacts with associated websites

 folks/Makefile.am     |    1 +
 folks/individual.vala |   38 +++++++++++++++++++++++++++++++++++++-
 folks/urlable.vala    |   39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 1 deletions(-)

commit 2a000bdc4b9138be9eac5059f2980542ed00452e
Author: Marco Barisione <marco@barisione.org>
Date:   Wed Nov 10 11:54:01 2010 +0000

    Add a FieldDetails object for details with extra parameters
    
    The class is used for values that can have additional parameters
    attached, for instance phone numbers or URLs.

 folks/Makefile.am        |    1 +
 folks/field-details.vala |  164 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 165 insertions(+), 0 deletions(-)

commit 6c3f7632ed52c471fccea71d56b3592a9b7b4bf5
Author: Marco Barisione <marco@barisione.org>
Date:   Mon Nov 8 13:56:31 2010 +0000

    Add a NameOwner interface for contacts with full names and nicknames

 folks/Makefile.am     |    1 +
 folks/individual.vala |  105 ++++++++++++++++++++++++-
 folks/name-owner.vala |  212 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 317 insertions(+), 1 deletions(-)