If Philip will not find time for this till tomorrow, then I'll fix this for 3.2.0 on Friday. The change is more or less trivial anyway.

By the way, how does the folks distinguish between address types? I suppose they have there some parameter in the X-URIS, do they? Maybe if you have an example what is mapped how, then we can mimic it for compatibility. It's fine to see how respective addresses are stored in these attributes of a vCard.

(In reply to comment #1)
> If Philip will not find time for this till tomorrow, then I'll fix this for
> 3.2.0 on Friday. The change is more or less trivial anyway.

That would be great, thanks. If you need patch review, let me know.

> By the way, how does the folks distinguish between address types? I suppose
> they have there some parameter in the X-URIS, do they? Maybe if you have an
> example what is mapped how, then we can mimic it for compatibility. It's fine
> to see how respective addresses are stored in these attributes of a vCard.

The code to parse X-URIS is here: http://git.gnome.org/browse/folks/tree/backends/eds/lib/edsf-persona.vala#n1169
The code to set X-URIS is here: http://git.gnome.org/browse/folks/tree/backends/eds/lib/edsf-persona-store.vala#n1092

So folks uses the multi-valued attribute parameter “TYPE” of X-URIS to distinguish address types; which iirc is fairly standard.

The values it uses for TYPE are here: http://git.gnome.org/browse/folks/tree/backends/eds/lib/edsf-persona.vala#n60

If any more values need adding, or if those values need changing, or if using TYPE is wrong, or if X-URIS needs renaming, please say so now since it's probably just about still possible to change in folks.

(In reply to comment #2)
> (In reply to comment #1)
> > If Philip will not find time for this till tomorrow, then I'll fix this for
> > 3.2.0 on Friday. The change is more or less trivial anyway.
> 
> That would be great, thanks. If you need patch review, let me know.

OK, I'll do it now, so we have a little more time for testing.

> > By the way, how does the folks distinguish between address types? I suppose
> > they have there some parameter in the X-URIS, do they? Maybe if you have an
> > example what is mapped how, then we can mimic it for compatibility. It's fine
> > to see how respective addresses are stored in these attributes of a vCard.
> 
> The code to parse X-URIS is here:
> http://git.gnome.org/browse/folks/tree/backends/eds/lib/edsf-persona.vala#n1169
> The code to set X-URIS is here:
> http://git.gnome.org/browse/folks/tree/backends/eds/lib/edsf-persona-store.vala#n1092

I do not know vala at all, but do I understand it right that if the url is homepage/blog/fburl/video then the newly allocated 'attr' is leaked? It seems so to me, but I do not know how vala handles memory management and garbage collections.

> So folks uses the multi-valued attribute parameter “TYPE” of X-URIS to
> distinguish address types; which iirc is fairly standard.

Yup, that's really a standard way of doing such thing. Furthermore, as this begins with "X-", then you can do pretty anything with it.

> If any more values need adding, or if those values need changing, or if using
> TYPE is wrong, or if X-URIS needs renaming, please say so now since it's
> probably just about still possible to change in folks.

Everything seems fine on the folks side, except of the memory question I wrote above. From the vCard point of view is everything right, as far as I can tell. And thanks a lot for pointers.

Created attachment 196600 [details] [review]
proposed eds patch

for evolution-data-server;

Could you try this, please? I tested it against Google itself, but I do not know whether it's 100% compatible with folks, thus a little bit of testing with it is much appreciated. Thanks in advance.

Review of attachment 196600 [details] [review]:

The whole remove_websites stuff needs to go, we now store all the websites so we should rewrite them all when we save.
Otherwise we will be unable to remove a X-URIS website.

Furthermore, Its definately allowed to save multple links in a contact with the same "rel" value, so this is currently broken for multiple home-page or blog links.

Created attachment 196645 [details] [review]
proposed eds patch ][

for evolution-data-server;

This fixes the above mentioned issue I didn't think of initially. Unfortunately, the local cache is vanished if it is not of the "expected" version. There seem to be issues with updates, which are not propagated to evo when done on the server, but I'm not going to fix everything here, only this X-URIS issue.

Patch looks good to me

Works in testing here, and mbarnes was ok with it, so i pushed this to master.

Actually, there seems to be some issue with adding a link field via folks. Looking into it now.

Review of attachment 196645 [details] [review]:

No need to change anything now, but just for future reference:

::: addressbook/backends/google/e-book-backend-google.c
@@ +2851,3 @@
+
+				website = gdata_gcontact_website_new (url, param->data, NULL, FALSE);
+				if (website) {

gdata_gcontact_website_new() never returns NULL, so this check's redundant.

Created attachment 196659 [details] [review]
Incremental patch

Ah, it failed because folks didn't supply a type and the "rel" property is mandatory. Lets make it "other" in case its not there, Tested it with contacts, and it now works.

Review of attachment 196659 [details] [review]:

::: addressbook/backends/google/e-book-backend-google.c
@@ +2848,2 @@
 			param = e_vcard_attribute_get_param (attr, EVC_TYPE);
+			type = param ? param->data : "other";

Might be better to map from the types which folks uses to the GDATA_GCONTACT_WEBSITE_* #defines in libgdata (http://git.gnome.org/browse/libgdata/tree/gdata/gcontact/gdata-gcontact-website.h#n40).

If the folks type is unknown, it should either map to GDATA_GCONTACT_WEBSITE_OTHER, or (preferably?) be set as the label for the website instead. Using labels would need some changes to _e_contact_new_from_gdata_entry() to do the conversion in the opposite direction, though.

True, google will probably throw an error if we put in some random type in the rel field. However, tha vcard TYPE attribute is not really meant to be a display label, so showing it as such is not ideal.

This is related to bug 653623 where we want to define what types means for urls in folks.

Created attachment 196690 [details] [review]
eds patch for no-type defined

for evolution-data-server;

What about this version then? And thinking of it, there is lost some information for those types in evolution's blog/home url addresses, like the label and primary website. Might not be an issue, though.

EVC_LABEL == LABEL == vcard field for physical address, don't use that.
the google backend uses GOOGLE_LABEL_PARAM == "X-GOOGLE-LABEL", see add_label_param().

Also, i'm not sure this is right. Because you save something with X-URIS http://foo type=bar and get back something X-URIS http://foo x-google-label=bar

Let me think on this some more.

Actually, in theory you can save any vcard style parameter on the url field and its assumed to be saved, but we lose any but type...

Created attachment 196700 [details] [review]
New patch

This patch uses X-GOOGLE-LABEL, and handles primary better.
It also looks more like the rest of the google backend, sharing the same
helper functions.

I would say yes, for both using X-GOOGLE-LABEL, then also for deeply integrated patch, but I would like to see Philip's idea too. If he approves, then please commit before Monday release (hard code freeze). Thanks.

Review of attachment 196700 [details] [review]:

A few small comments to fix, but it otherwise looks good to me. Please commit.

::: addressbook/backends/google/e-book-backend-google.c
@@ +2851,1 @@
+			website =gdata_gc_contact_website_from_attribute (attr, &have_uri_primary);

Missing space after the equals sign.

@@ +3346,3 @@
+	{ "home", { "HOME", NULL }},
+	{ "other", { "OTHER", NULL }},
+	{ "work", { "WORK", NULL }},

It would be nice to use the #defines which libgdata provides here (e.g. GDATA_GCONTACT_WEBSITE_HOME_PAGE instead of "home-page").

@@ +3735,3 @@
+		return;
+
+	attr = e_vcard_attribute_new (NULL, "X-URIS");

Would be nice to move the "X-URIS" out into a #define at the top of the file to prevent typos of it anywhere.

@@ +3744,3 @@
+
+	if (attr)
+		e_vcard_add_attribute (vcard, attr);

Could attr ever be NULL here?

(In reply to comment #16)
> Actually, in theory you can save any vcard style parameter on the url field and
> its assumed to be saved, but we lose any but type...

There's not much we can do about it, since (afaik, though I haven't actually tested it) Google don't support arbitrary attributes on XML elements.

(In reply to comment #18)
> I would say yes, for both using X-GOOGLE-LABEL, then also for deeply integrated
> patch, but I would like to see Philip's idea too. If he approves, then please
> commit before Monday release (hard code freeze). Thanks.

Falling back to using X-GOOGLE-LABEL if we don't understand a TYPE is a reasonable change which should be made to the entire backend. Probably a bit late in the cycle to spend time on it now. I've filed bug #659285 about it, and I'll try and look at that next cycle.

Comment on attachment 196700 [details] [review]
New patch

Committed with the suggested fixes from comment #19.

commit a7312a51508974847ed6c63837eb5e88ac6f6609
Author: Alexander Larsson <alexl@redhat.com>
Date:   Sun Sep 18 17:36:47 2011 +0100

    Bug 659079 — Read all gContact:website
    
    Convert the website code in EDS' Google Contacts backend to support TYPEs
    and generally use the existing framework code.
    
    Closes: bgo#659079

 .../backends/google/e-book-backend-google.c        |  127 ++++++++++++++++----
 1 files changed, 104 insertions(+), 23 deletions(-)

(I also renamed some of the non-standard types to use an ‘X-’ prefix; e.g. ‘X-HOME-PAGE’.)