I find this quite confusing in Empathy, because my contacts all appears twice: once in my manually assigned groups and once in Personal. 

I never put anyone in Personal.

I now realize that if I modify a contact in Evolution, I need to go online and remove it from "Personal", everytime or it disappears on my phone. 

If I simply remove "Personal" in Evolution categories, I then need to go online add it to "My Contacts".  

Currently it appears there is no way to create/modify contacts in evolution and have them appear in the standard "My Contacts" folder and not also in the "Personal" folder without secondary manipulation.

I confirm this issue. All contact created in evolution are stored in other contact/personal in google and are not accessible on android device.

Thanks for a bug report. I tried this with libgdata-0.10.1 and I do not see an issue here. If I create a new contact in evolution, with Personal category, then the Google's UI shows only that being set, no other category. I do not have an androind-capable phone to test this fully, though.

I'm adding Philip into the loop.

I'd just like to re-iterate how I believe Google see's groups.  There is usually good online API docs but experience is pretty clear. I found this:

http://support.google.com/mail/bin/answer.py?hl=en&answer=97952 

My Contacts = Default Group for People. Typically the group for contacts you want to see/use everywhere.  This is where contacts from evolution should go.  Today, if I add a contact to Evolution and put them in *any* group, including personal, this group seems to be added automatically. If I do not specify a group, as executed by default when adding via Evolution right-click "Add to Contacts..." the contact does not end up here. Instead it lands in "Other Contacts".  IMHO this behavior needs to be corrected, and adding them to the Google "Personal" group is not a desired outcome. 

Personal = Private Group for People.  These are typically contacts that you do not want to appear when scrolling through an addressbook on your client devices, or using applications on phone/tablet that might correlate to your addressbook. They can be found by explicit search.

Other Contacts = When no groups are specified.  For instance when "My Contacts" is not specified, contacts land here.  This is where the default Evolution "Add to Contacts..." puts people.  Contacts here do not appear on many devices and applications that interact with "My Contacts". This includes email clients on Android platforms and Phone Dialing on Android phones.  Gtalk, Skype, etc. all ignore "Other Contacts" from what I can tell.  

In summary, the desired behavior for a default "Add Contact" without specifying a group should be to add the contact to only the "My Contacts" group and not Personal. This would be consistent with adding a contact on an email client running on an Google Android device, which I believe is a good use case for Evolution's use of Google Contacts. 

Sorry for the verbosity, but the subtle impact of Group assignment is the issue, not how the libgdata integration is operating.

That makes sense. This should be a relatively simple change to the grouping code in EDS’ Google Contacts backend, but I won't have time to do it for a while. Patches welcome.

Philip, I can do it, it really seems like pretty simple, like "make sure group X is provided for newly created contacts without any category set, keep as is on edits", but I do not know the right group name, from comment #0 I understood that the "Personal" is not the one. By the way, does the name depend on current locale? The names in categories do, which might not be that big problem, I'm concerned on the X group name here.

(In reply to comment #7)
> Philip, I can do it, it really seems like pretty simple, like "make sure group
> X is provided for newly created contacts without any category set, keep as is
> on edits", but I do not know the right group name, from comment #0 I understood
> that the "Personal" is not the one. By the way, does the name depend on current
> locale? The names in categories do, which might not be that big problem, I'm
> concerned on the X group name here.

This is where it gets a bit confusing. Google’s “My Contacts” group is a system group[1], which EDS’ Google backend maps to EDS’ “Personal” group. Therefore, the “Personal” group should be added to all contacts when they’re created, which should result in them appearing in “My Contacts” online.

[1]: http://code.google.com/apis/contacts/docs/3.0/developers_guide.html#Groups

(In reply to comment #0)
> It seems logical then that new contacts added in Evolution in the category
> "Personal" would then appear in "My Contacts".  This works, except that these
> new evolution-created contacts also go into a "My Contacts" group called
> "Personal" at which point they become invisible on devices like Android phones. 

The reporter, as quoted above, suggested the same, but claimed there is a side-effect of using Personal category in evolution.

From reading the code I see a mapping from GDATA_CONTACTS_GROUP_CONTACTS to _("Personal"), in sanitise_group_name(), but not the reverse mapping, thus maybe the priv->groups_by_id and priv->groups_by_name should always contain reverse mappings from sanitise_group_name()?

(In reply to comment #9)
> (In reply to comment #0)
> > It seems logical then that new contacts added in Evolution in the category
> > "Personal" would then appear in "My Contacts".  This works, except that these
> > new evolution-created contacts also go into a "My Contacts" group called
> > "Personal" at which point they become invisible on devices like Android phones. 
> 
> The reporter, as quoted above, suggested the same, but claimed there is a
> side-effect of using Personal category in evolution.
> 
> From reading the code I see a mapping from GDATA_CONTACTS_GROUP_CONTACTS to
> _("Personal"), in sanitise_group_name(), but not the reverse mapping, thus
> maybe the priv->groups_by_id and priv->groups_by_name should always contain
> reverse mappings from sanitise_group_name()?

Yes, that might fix it. We need to ensure that the branch on line 3006 isn't taken for system groups, since that would result in creating a new group with the same name as an existing system group.

It might also be worthwhile to change create_group() to automatically return the corresponding system group ID if category_name is a system group name as listed in sanitise_group_name().

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

for evolution-data-server;

This should make it. I decided to not populate groups_by_name, because it only means to have more places with the same strings, which usually leads to issues, thus your suggestion to fix create_group() made this even simpler.

The issue is that this patch doesn't work with libgdata-0.10.1, or I do something wrong, because if I try to create a new contact with no category set (actually without Personal category set), then I'm adding it and the backend claims an error received from the server:

> Cannot add contact: Invalid request URI or header, or unsupported
> nonstandard parameter: Reference to a foreign group 'http:Contacts'

Do you think this is an issue how I changed code, or with my version of libgdata, or something complete different?

Review of attachment 209776 [details] [review]:

> > Cannot add contact: Invalid request URI or header, or unsupported
> > nonstandard parameter: Reference to a foreign group 'http:Contacts'
> 
> Do you think this is an issue how I changed code, or with my version of
> libgdata, or something complete different?

That's odd. It could be an issue with your patch or with libgdata. The only real way to debug problems like this is to look at protocol traces. Could you use LIBGDATA_DEBUG=3 G_MESSAGES_DEBUG=all GOOGLE_DEBUG=1 to get a log of the HTTP requests made by the backend which cause this error please? Either attach it here or e-mail it to me (bugzilla@tecnocode.co.uk) if it's got personal data in.

::: addressbook/backends/google/e-book-backend-google.c
@@ +918,3 @@
+{
+	struct _GroupsMap {
+		const gchar *google_name;

It would be clearer to call this “google_id”, since it's an ID not a label.

@@ +1068,3 @@
 	GDataEntry *group, *new_group;
 	gchar *uid;
+	const gchar *google_system_name;

Similarly, this should probably be “system_group_id”.

@@ +3051,3 @@
 		gdata_contacts_contact_add_group (GDATA_CONTACTS_CONTACT (entry), category_id);
+		if (g_strcmp0 (category_id, GDATA_CONTACTS_GROUP_CONTACTS) == 0)
+			ensure_personal_group = FALSE;

This shouldn't be necessary; libgdata automatically filters out duplicate groups (they're stored in a hash table in the GDataContactsContact).

@@ +3055,3 @@
 	}
 
+	/* to have contacts shown in My Contacts by default */

Might be useful to reference bug #663324 here so the discussion doesn't get lost.

This is what I get (the related part when creating a new contact with no group set, thus a default Contacts group is added):

libgdata-DEBUG: > POST /m8/feeds/contacts/default/full HTTP/1.1
libgdata-DEBUG: > Soup-Debug-Timestamp: 1331802341
libgdata-DEBUG: > Soup-Debug: SoupSessionSync 1 (0xe6a6d0), SoupMessage 11 (0x7fb734013c40), SoupSocket 2 (0x7fb72c00ea10)
libgdata-DEBUG: > Host: www.google.com
libgdata-DEBUG: > Authorization: GoogleLogin auth=...
libgdata-DEBUG: > GData-Version: 3
libgdata-DEBUG: > Content-Type: application/atom+xml
libgdata-DEBUG: > Connection: Keep-Alive
libgdata-DEBUG: > 
libgdata-DEBUG: > <?xml version='1.0' encoding='UTF-8'?><entry xmlns='http://www.w3.org/2005/Atom' xmlns:gContact='http://schemas.google.com/contact/2008' xmlns:gd='http://schemas.google.com/g/2005' xmlns:app='http://www.w3.org/2007/app'><title type='text'>auto group</title><content type='text'></content><category term='http://schemas.google.com/contact/2008#contact' scheme='http://schemas.google.com/g/2005#kind'/><gd:name><gd:givenName>auto</gd:givenName><gd:familyName>group</gd:familyName><gd:fullName>auto group</gd:fullName></gd:name><gd:extendedProperty name='X-MOZILLA-HTML'>FALSE</gd:extendedProperty><gContact:groupMembershipInfo href='Contacts'/></entry>

libgdata-DEBUG: < HTTP/1.1 400 Bad Request
libgdata-DEBUG: < Soup-Debug-Timestamp: 1331802341
libgdata-DEBUG: < Soup-Debug: SoupMessage 11 (0x7fb734013c40)
libgdata-DEBUG: < Content-Type: text/plain; charset=UTF-8
libgdata-DEBUG: < Date: Thu, 15 Mar 2012 09:05:41 GMT
libgdata-DEBUG: < Expires: Thu, 15 Mar 2012 09:05:41 GMT
libgdata-DEBUG: < Cache-control: private, max-age=0
libgdata-DEBUG: < X-Content-Type-Options: nosniff
libgdata-DEBUG: < X-Frame-Options: SAMEORIGIN
libgdata-DEBUG: < X-XSS-Protection: 1; mode=block
libgdata-DEBUG: < Server: GSE
libgdata-DEBUG: < Transfer-Encoding: chunked
libgdata-DEBUG: < 
libgdata-DEBUG: < Reference to a foreign group &#39;http:Contacts&#39;

For the review:
- renaming variables, sure can be.
- the test for the name, I thought it'll be cleaner to do, as I have no
  knowledge of the internal processing on libgdata side
- you can get the bug ID and commit itself with 'git blame ...', but if you
  want it there, then I have no objection and will add it

I'll do the changes into the patch when we figure out what is wrong with setting system group id on a newly created contact.

(In reply to comment #13)
> This is what I get (the related part when creating a new contact with no group
> set, thus a default Contacts group is added):
> 
> libgdata-DEBUG: > POST /m8/feeds/contacts/default/full HTTP/1.1
> libgdata-DEBUG: > Soup-Debug-Timestamp: 1331802341
> libgdata-DEBUG: > Soup-Debug: SoupSessionSync 1 (0xe6a6d0), SoupMessage 11
> (0x7fb734013c40), SoupSocket 2 (0x7fb72c00ea10)
> libgdata-DEBUG: > Host: www.google.com
> libgdata-DEBUG: > Authorization: GoogleLogin auth=...
> libgdata-DEBUG: > GData-Version: 3
> libgdata-DEBUG: > Content-Type: application/atom+xml
> libgdata-DEBUG: > Connection: Keep-Alive
> libgdata-DEBUG: > 
> libgdata-DEBUG: > <?xml version='1.0' encoding='UTF-8'?><entry
> xmlns='http://www.w3.org/2005/Atom'
> xmlns:gContact='http://schemas.google.com/contact/2008'
> xmlns:gd='http://schemas.google.com/g/2005'
> xmlns:app='http://www.w3.org/2007/app'><title type='text'>auto
> group</title><content type='text'></content><category
> term='http://schemas.google.com/contact/2008#contact'
> scheme='http://schemas.google.com/g/2005#kind'/><gd:name><gd:givenName>auto</gd:givenName><gd:familyName>group</gd:familyName><gd:fullName>auto
> group</gd:fullName></gd:name><gd:extendedProperty
> name='X-MOZILLA-HTML'>FALSE</gd:extendedProperty><gContact:groupMembershipInfo
> href='Contacts'/></entry>
> 
> libgdata-DEBUG: < HTTP/1.1 400 Bad Request
> libgdata-DEBUG: < Soup-Debug-Timestamp: 1331802341
> libgdata-DEBUG: < Soup-Debug: SoupMessage 11 (0x7fb734013c40)
> libgdata-DEBUG: < Content-Type: text/plain; charset=UTF-8
> libgdata-DEBUG: < Date: Thu, 15 Mar 2012 09:05:41 GMT
> libgdata-DEBUG: < Expires: Thu, 15 Mar 2012 09:05:41 GMT
> libgdata-DEBUG: < Cache-control: private, max-age=0
> libgdata-DEBUG: < X-Content-Type-Options: nosniff
> libgdata-DEBUG: < X-Frame-Options: SAMEORIGIN
> libgdata-DEBUG: < X-XSS-Protection: 1; mode=block
> libgdata-DEBUG: < Server: GSE
> libgdata-DEBUG: < Transfer-Encoding: chunked
> libgdata-DEBUG: < 
> libgdata-DEBUG: < Reference to a foreign group &#39;http:Contacts&#39;

Having looked at the documentation closer (https://developers.google.com/google-apps/contacts/v3/#working_with_contact_groups, grep for “<gContact:systemGroup id='Contacts'/>”) reminded me that system group IDs aren't the same as the entry IDs for those groups.

i.e. Each group in Google Contacts has an Atom entry with an ID such as “https://www.google.com/m8/feeds/groups/userEmail/full/6”. System groups *also* have a system group ID such as “Contacts”.

We're getting the error because we're using the wrong group ID in the call to gdata_contacts_contact_add_group() — we need to use the entry ID, not the system group ID.

The way to fix this is to change map_google_with_evo_group() to return an entry ID rather than a system group ID. i.e. Look up the GDataContactsGroup corresponding to the system group ID, then return the value of gdata_entry_get_id() for that group.

Sorry for not realising this sooner; it's been a while since I last dealt with IDs in Google Contacts.

> For the review:
> - renaming variables, sure can be.
> - the test for the name, I thought it'll be cleaner to do, as I have no
>   knowledge of the internal processing on libgdata side
> - you can get the bug ID and commit itself with 'git blame ...', but if you
>   want it there, then I have no objection and will add it
> 
> I'll do the changes into the patch when we figure out what is wrong with
> setting system group id on a newly created contact.

OK.

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

for evolution-data-server;

This is getting more fun, but I believe I got it now (according to log outputs I did) :) So, the patch has applied all requested changes from the previous review, though there are too many ids anyway, thus will see how much this will be confusing. Anyway, new changes are:
a) distinguish between system and user groups, to get proper URL of the group
b) for system groups store group names as shown in evolution
c) fetch list of groups at the beginning, to have them available

Philip, do you think you can take a look at this patch before Monday?  Otherwise it will miss 3.4.0.

I should be around on IRC all weekend if you need an evo dev for something.

How easy is a modified patch for 3.2.3 to make it into Ubuntu 12.04 Beta? I had about four hunks fail, but have not had a closer look to see if I can manually insert them. 

I would then be able to do some more extensive testing this weekend.

Review of attachment 209943 [details] [review]:

A few small comments, but the patch looks good to me. If you're happy that it works (and have tested it with things like clearing your cache; adding, updating and deleting contacts) then please commit with the modifications below.

::: addressbook/backends/google/e-book-backend-google.c
@@ +999,3 @@
+		g_warn_if_fail (name != NULL);
+		if (!name)
+			name = g_strdup (system_group_id);

Wouldn't it be best to use the old value of name in this case (i.e. the result of sanitise_group_name())?

@@ +3061,2 @@
 	for (category_names = e_contact_get (contact, E_CONTACT_CATEGORY_LIST); category_names != NULL; category_names = category_names->next) {
 		gchar *category_id;

This needs to be initialised to NULL.

@@ +3101,3 @@
+		const gchar *group_entry_id = g_hash_table_lookup (priv->system_groups_by_id, GDATA_CONTACTS_GROUP_CONTACTS);
+
+		g_warn_if_fail (group_entry_id != NULL);

There are three separate places in the code where g_hash_table_lookup() is called on priv->system_groups_by_id, followed by a call to g_warn_if_fail().

It might be an idea to move these two lines into a get_entry_id_from_system_group_id() function.

@@ +3343,3 @@
 			g_warning ("Couldn't find name for category with ID '%s'.", category_id);
+
+		g_free (category_id);

This should be a separate commit.

Philip and I agreed to let this patch miss the 3.3.92 release so Milan can look over the suggested changes and maybe Ian can test it.  Then if everyone's happy we can either request a code freeze break for 3.4.0 or commit it after the code freeze for 3.4.1.

Review of attachment 209943 [details] [review]:

Another little thing I noticed. Changing patch status to needs-work as per comment #19.

::: addressbook/backends/google/e-book-backend-google.c
@@ +928,3 @@
+		{ GDATA_CONTACTS_GROUP_COWORKERS, N_("Coworkers") } /* System Group: Coworkers */
+	};
+	gint ii;

This should be a guint.

Created attachment 210002 [details] [review]
Version of patch 209943 for gnome-3-2

Ian, here's a version of the patch (attachment #209943 [details]) which I just rebased onto EDS’ gnome-3-2 branch. It compiles fine, but I haven't tested it at all. It would be great if you could find some time to test it this weekend. Thanks.

(Patch marked as rejected because it's for the gnome-3-2 branch.)

(In reply to comment #18)
> ::: addressbook/backends/google/e-book-backend-google.c
> @@ +999,3 @@
> +        g_warn_if_fail (name != NULL);
> +        if (!name)
> +            name = g_strdup (system_group_id);
> 
> Wouldn't it be best to use the old value of name in this case (i.e. the result
> of sanitise_group_name())?

I prefer system_group_id because it's not localized and it's consistent value. For me the system name is "Contacts", while the Name is "Personal", as returned by the google server.

> @@ +3061,2 @@
>      for (category_names = e_contact_get (contact, E_CONTACT_CATEGORY_LIST);
> category_names != NULL; category_names = category_names->next) {
>          gchar *category_id;
> 
> This needs to be initialised to NULL.

Ouch, you've right. I tested my changes with system groups only, thus it didn't trigger the issue.

> @@ +3101,3 @@
> +        const gchar *group_entry_id = g_hash_table_lookup
> (priv->system_groups_by_id, GDATA_CONTACTS_GROUP_CONTACTS);
> +
> +        g_warn_if_fail (group_entry_id != NULL);
> 
> There are three separate places in the code where g_hash_table_lookup() is
> called on priv->system_groups_by_id, followed by a call to g_warn_if_fail().
> 
> It might be an idea to move these two lines into a
> get_entry_id_from_system_group_id() function.

Hmm, I do not think it worth it, it's just error checking, which is not that mandatory, even it helped me to find out that group names are not fetched on addressbook open, if the cache is "fresh enough".

> @@ +3343,3 @@
>              g_warning ("Couldn't find name for category with ID '%s'.",
> category_id);
> +
> +        g_free (category_id);
> 
> This should be a separate commit.

OK. I'll keep it in the patch, but commit it separately.

Created attachment 210099 [details] [review]
proposed eds patch ]I[

for evolution-data-server;

Updated patch, with avoided possible use of uninitialized variable.
The last chunk should be committed separately.

Created attachment 210139 [details]
Test Case results from patch in Comment #21

I didn't get very far, my first three test cases were fail, crash, crash :) after applying the patch in comment #21 (3.2.x version of this bug)

It should be noted that I also have these additional two patches applied to my 3.2.3ubuntu4 source currently in Ubuntu 12.04B

- Comment#37 from https://bugzilla.gnome.org/show_bug.cgi?id=659756
- Comment#1 from https://bugzilla.gnome.org/show_bug.cgi?id=669003

I'll rebuild with *just* this bug patch just to be sure. No dice for now.

Patches from those two bugs are fine. I see Philip missed the catch he found within this chunk:
@@ -2970,11 +3033,22 @@ _gdata_entry_update_from_e_contact (EBookBackend *backend,
 	for (category_names = e_contact_get (contact, E_CONTACT_CATEGORY_LIST); category_names != NULL; category_names = category_names->next) {
 		gchar *category_id;
 		const gchar *category_name = category_names->data;
+		const gchar *system_group_id;

The "gchar *category_id;" should be "gchar *category_id = NULL;" which will fix the double free abort of the factory.

The first backtrace for the loading of a dialog with duplicate contacts merging question is different. I think it's part of bug #659890, which we didn't figure out yet.

(In reply to comment #24)
> Created an attachment (id=210139) [details]
> Test Case results from patch in Comment #21
> 
> I didn't get very far, my first three test cases were fail, crash, crash :)
> after applying the patch in comment #21 (3.2.x version of this bug)

Thanks for testing this. In order to debug the failure of the first test, could you please:
 - make sure you've emptied your EDS Google backend cache (~/.cache/evolution/addressbook/google*)
 - provide a debug log by running e-addressbook-factory with GOOGLE_DEBUG=1 LIBGDATA_DEBUG=3 G_MESSAGES_DEBUG=all environment variables set

Only the entries in the debug log concerning contacts A, W, B, etc. in the test cases are necessary (so please do redact any personal data from the logs if necessary).

Thanks!

(In reply to comment #25)
> Patches from those two bugs are fine. I see Philip missed the catch he found
> within this chunk:
> @@ -2970,11 +3033,22 @@ _gdata_entry_update_from_e_contact (EBookBackend
> *backend,
>      for (category_names = e_contact_get (contact, E_CONTACT_CATEGORY_LIST);
> category_names != NULL; category_names = category_names->next) {
>          gchar *category_id;
>          const gchar *category_name = category_names->data;
> +        const gchar *system_group_id;
> 
> The "gchar *category_id;" should be "gchar *category_id = NULL;" which will fix
> the double free abort of the factory.

Whoops, sorry. :-(

Created attachment 210219 [details]
strange EDS output

I applied the tweak in #25 to the codebase and repeated with a cleared cache.

I was actually able to complete some of my test cases successfully (1-3) but have not had success doing them all at once as I have been seeing inconsistent crashes at different places and having to restart.

The consistent behavior, that seem strange, is that each time I start my debug output I have thousands of strange lines while the addressbook-factory is populating my complete contact list for the first time.  It gets to about 1880 of my 1982 contacts, and then messages below/attached appear for about 6000 lines and then stop.  

The bizarre part is that addressbook-factory does not crash, and no more address-book processes spawn, but there is no more DEBUG output. 

The stranger thing is that I can actually still do things in evolution, and it completes the "load" of all my contacts (very very slowly - 5 min later) and eventually gets to the proper total number - but during all the time there are no additional Soup messages on my console/logfile. 

Anyways, I think I have another problem to solve here, possibly related to the patch. I've attached a larger snippet of these repeating messages; unfortunately my debug is huge and loaded  with private data, but looked normal up until this point. 

(e-addressbook-factory:8254): libgdata-DEBUG: < HTTP/1.1 200 OK
(e-addressbook-factory:8254): libgdata-DEBUG: < Soup-Debug-Timestamp: 1332299688
(e-addressbook-factory:8254): libgdata-DEBUG: < Soup-Debug: SoupMessage 5 (0x7f6008073190)
(e-addressbook-factory:8254): libgdata-DEBUG: < Content-Type: image/jpeg
(e-addressbook-factory:8254): libgdata-DEBUG: < Expires: Wed, 21 Mar 2012 03:14:26 GMT
(e-addressbook-factory:8254): libgdata-DEBUG: < Date: Wed, 21 Mar 2012 03:14:26 GMT
(e-addressbook-factory:8254): libgdata-DEBUG: < Cache-Control: private, max-age=0, must-revalidate, no-transform
(e-addressbook-factory:8254): libgdata-DEBUG: < Vary: Accept, X-GData-Authorization, GData-Version
(e-addressbook-factory:8254): libgdata-DEBUG: < GData-Version: 3.1
(e-addressbook-factory:8254): libgdata-DEBUG: < ETag: "ZQ9aOV4oSit7I2BcKmkvVAxWJV9DN0clcSo."
(e-addressbook-factory:8254): libgdata-DEBUG: < Transfer-Encoding: chunked
(e-addressbook-factory:8254): libgdata-DEBUG: < X-Content-Type-Options: nosniff
(e-addressbook-factory:8254): libgdata-DEBUG: < X-Frame-Options: SAMEORIGIN
(e-addressbook-factory:8254): libgdata-DEBUG: < X-XSS-Protection: 1; mode=block
(e-addressbook-factory:8254): libgdata-DEBUG: < Server: GSE
(e-addressbook-factory:8254): libgdata-DEBUG: < 
(e-addressbook-factory:8254): libgdata-DEBUG: < \xff\xd8\xff\xe0

And what is content-type image doing in there?? Is this CAPTCHA death?

(In reply to comment #27)
> Created an attachment (id=210219) [details]
> strange EDS output
> 
> I applied the tweak in #25 to the codebase and repeated with a cleared cache.
> 
> I was actually able to complete some of my test cases successfully (1-3) but
> have not had success doing them all at once as I have been seeing inconsistent
> crashes at different places and having to restart.

Are those crashes all related to bug #659890, or different?

> Anyways, I think I have another problem to solve here, possibly related to the
> patch. I've attached a larger snippet of these repeating messages;
> unfortunately my debug is huge and loaded  with private data, but looked normal
> up until this point. 

*snip*

Those log messages are perfectly normal: they're the result of EDS downloading the photos for all your contacts. The “\xff\xd8\xff\xe0” sequence is the first four bytes of a JPEG file, encoded in hex. The entire file isn't printed to the console (thankfully), but it should be downloaded correctly.

Created attachment 210304 [details]
Test Case results from patch

Phillip, yes all my crashes are related to duplication of existing contacts. I found a "human error" with my clean-up routine for test data .. I confirmed this for all my attempts by observing that I captured the data in my backups.

Anyways, I actually tripped over it accidently again in my test run, but just continued (I included the stacktrace, same as last for confirmation). 

In summary, the patch works great. Here are all the non-pass cases for review.

Cases 1 & 10) A contact in Google's Personal (private) group appears in Evolution categories as "Personal, Personal" - confusing, and can't be re-created from Evolution.

Cases 17 & 18) Categories created in Google don't appear in the Category drop-down menu. It seems that all categories created in the session in Evolution were missing too.  I've known about this, and there is a related bug somewhere.

Case 19) When a group is renamed in Google, the change is not propagated to Evolution

Other Observations that might be other bugs/features:
- Whenever I add a category in the contact editor, I have to click "OK" twice; Once to release the category field and a second time to actually "OK".
- In case 13) the Evo category ordering is non-intuitive (not alphabetical); I don't care, but noted.

(In reply to comment #30)
> Created an attachment (id=210304) [details]
> Test Case results from patch
> 
> Phillip, yes all my crashes are related to duplication of existing contacts. I
> found a "human error" with my clean-up routine for test data .. I confirmed
> this for all my attempts by observing that I captured the data in my backups.

Right.

> In summary, the patch works great. Here are all the non-pass cases for review.
>
> Cases 1 & 10) A contact in Google's Personal (private) group appears in
> Evolution categories as "Personal, Personal" - confusing, and can't be
> re-created from Evolution.

This looks like the only bug remaining in the patch, caused by not checking for duplicate categories in the category_names list in _e_contact_new_from_gdata_entry().

> Cases 17 & 18) Categories created in Google don't appear in the Category
> drop-down menu. It seems that all categories created in the session in
> Evolution were missing too.  I've known about this, and there is a related bug
> somewhere.
> 
> Case 19) When a group is renamed in Google, the change is not propagated to
> Evolution
> 
> Other Observations that might be other bugs/features:
> - Whenever I add a category in the contact editor, I have to click "OK" twice;
> Once to release the category field and a second time to actually "OK".
> - In case 13) the Evo category ordering is non-intuitive (not alphabetical); I
> don't care, but noted.

These can be dealt with in separate bugs later.

Created attachment 210642 [details] [review]
proposed eds patch IV

for evolution-data-server;

This is the same as patch ]I[, the only difference is 
		if (category_name != NULL) {
			if (g_list_find_custom (category_names, category_name, (GCompareFunc) g_strcmp0) == NULL)
				category_names = g_list_prepend (category_names, category_name);
		} else

I do not expect this being a problem from performance point of view, thus I'm not creating a hash table first, then GList from it.

Review of attachment 210642 [details] [review]:

Looks good to me. I guess this can be committed. :-)

   The possible use-after-free:
Created commit 0cc952d in eds master (3.5.1+)
Created commit 6f6f415 in eds gnome-3-4 (3.4.1+)

   The patch itself:
Created commit abb440b in eds master (3.5.1+)
Created commit d2d75b5 in eds gnome-3-4 (3.4.1+)

*** Bug 670866 has been marked as a duplicate of this bug. ***

(In reply to comment #30)
> Cases 17 & 18) Categories created in Google don't appear in the Category
> drop-down menu. It seems that all categories created in the session in
> Evolution were missing too.  I've known about this, and there is a related bug
> somewhere.

That's bug #660535.

> Case 19) When a group is renamed in Google, the change is not propagated to
> Evolution

I've filed bug #672899 and added you to the CC list.

> Other Observations that might be other bugs/features:
> - Whenever I add a category in the contact editor, I have to click "OK" twice;
> Once to release the category field and a second time to actually "OK".

I'm not sure what you mean here. Would you mind opening a new bug with more detailed reproduction instructions (specifically, which ‘category field’?). Thanks.

> - In case 13) the Evo category ordering is non-intuitive (not alphabetical); I
> don't care, but noted.

I've mentioned this in bug #514041, which is about re-vamping the display of categories across Evolution.

I think we're done here. Thanks for your help.

Thanks Philip; 

Is there going to be a 3.2 commit as well to simplify the downstream merge?

(In reply to comment #37)
> Thanks Philip; 
> 
> Is there going to be a 3.2 commit as well to simplify the downstream merge?

I would guess not, since I doubt there'll be another 3.2 release of EDS (since it's now a year old).

Philip, can you do a quick rebase of the final commit for EDS 3-2; something similar to your previous patch that I can apply downstream?

Created attachment 210907 [details] [review]
Version of patch 210642 for gnome-3-2

Here we go. This rolls attachment #210642 [details] in with the fix for prematurely freeing a category ID. i.e. It's commits 0cc952de5fb4ea3e8e491ee9f7d82656f9f395b7 and abb440b8b2c17482596ca080c30ce21fb54afcb8 from master, backported to gnome-3-2. Untested at runtime, but it compiles for me.

Created attachment 210973 [details]
Rejects on 3.2.3

Thanks Philip; This one still requires a bit of manual merging.  If I do it, it makes downstream merge less confident

patching file addressbook/backends/google/e-book-backend-google.c
Hunk #1 succeeded at 85 (offset -3 lines).
Hunk #2 succeeded at 116 (offset -1 lines).
Hunk #3 succeeded at 884 (offset -30 lines).
Hunk #4 FAILED at 970.
Hunk #5 succeeded at 948 (offset -32 lines).
Hunk #6 FAILED at 1088.
Hunk #7 succeeded at 1116 (offset -43 lines).
Hunk #8 succeeded at 1830 (offset -58 lines).
Hunk #9 FAILED at 2305.
Hunk #10 succeeded at 2568 (offset -40 lines).
Hunk #11 succeeded at 2743 (offset -11 lines).
Hunk #12 succeeded at 2778 with fuzz 2 (offset -11 lines).
Hunk #13 succeeded at 3020 (offset -28 lines).
Hunk #14 FAILED at 3078.
Hunk #15 succeeded at 3270 (offset -41 lines).
4 out of 15 hunks FAILED -- saving rejects to file addressbook/backends/google/e-book-backend-google.c.rej

All sorted out; confirmed with downstream dev, looks like we are merged and cherry picked appropriately downstream.  Super-excited about working Evolution+Google in 12.04. thanks