Created attachment 227464 [details] [review]
eds: expose Google system groups in the public API

This will allow gnome-contacts to change it directly (to switch a contact
from My Contacts to Other and back)

Review of attachment 227464 [details] [review]:

This looks good to me.  My one concern on reading the bug was that non google address books needed to be considered, but you handled that nicely.  As long as the below comments are ok/fixed it should be good to merge.

I assume you've tested this by the way?  It might also be nice to add a unit test for this new api in folks/tests/eds if you would.

::: backends/eds/lib/edsf-persona-store.vala
@@ +2055,3 @@
+        }
+
+      vcard.add_attribute (new_attr);

Does this make the change in EDS permanent?

::: backends/eds/lib/edsf-persona.vala
@@ +1765,3 @@
+
+              if (!added_sysgroups.is_empty || !removed_sysgroups.is_empty)
+                should_notify_sysgroups = true;

Please wrap if statement contents in brackets

Thanks for the review.
I'm not sure how to be implement tests, though, as the existing test cases rely on the local eds backend, while this functionality requires a Google address book.

Review of attachment 227464 [details] [review]:

> I'm not sure how to be implement tests, though, as the existing test cases rely
> on the local eds backend, while this functionality requires a Google address
> book

The way to do that would be to implement a mock EDS contact factory over D-Bus, which mock-up local and Google address books. That’s a lot of work for this bug though.

::: backends/eds/lib/edsf-persona-store.vala
@@ +1997,3 @@
+      Set<string> system_groups) throws PropertyError
+    {
+      if (!this._is_google_contacts_address_book())

Missing space before the ‘()’.

@@ +1998,3 @@
+    {
+      if (!this._is_google_contacts_address_book())
+		throw new PropertyError.NOT_WRITEABLE (_("My Contacts is only available for Google Contacts"));

Dodgy indentation. Please use braces and spaces like the rest of the code.

@@ +2044,3 @@
+        contact.remove_attribute(prev_attr);
+
+      E.VCardAttribute new_attr = new E.VCardAttribute("", "X-GOOGLE-SYSTEM-GROUP-IDS");

Missing spaces before the opening brackets of the function calls on this line and the previous line.

@@ +2047,3 @@
+      foreach (var group in system_groups)
+        {
+          if (group == "")

You might want to skip null groups as well.

::: backends/eds/lib/edsf-persona.vala
@@ +618,3 @@
     }
 
+  public async void change_system_groups (Set<string> system_groups) throws PropertyError

This needs a documentation comment, including a “@since UNRELEASED” line, a “@param system_groups …” line, and a “@throws PropertyError …” line.

@@ +623,3 @@
+    }
+
+  public async void change_in_google_personal_group (bool in_personal) throws PropertyError

This also needs a documentation comment.

@@ +632,3 @@
+        {
+          if (sg == "Contacts" && !in_personal)
+            continue;

This needs a comment explaining where the magic “"Contacts"” string comes from.

@@ +634,3 @@
+            continue;
+
+          new_system_groups.add(sg);

Missing space before the ‘(’ (and in other places in this file).

@@ +638,3 @@
+
+      if (in_personal)
+        new_system_groups.add("Contacts");

If this “"Contacts"” string is going to be used multiple times, it should probably be declared as a private class-level constant.

@@ +781,3 @@
 
+  [CCode (notify = false)]
+  public Set<string>? system_groups

This also needs a documentation comment.

@@ +808,3 @@
           return this._in_google_personal_group;
         }
+	  set { this.change_in_google_personal_group.begin (value); }

Dodgy indentation.

@@ +1733,3 @@
+
+                  if (this._system_groups == null ||
+					  !this._system_groups.contains (system_group_id))

More dodgy indentation. There should be no tabs at all in any folks code.

@@ +1740,3 @@
+
+              /* Work out which categories have been removed. */
+              var removed_sysgroups = new LinkedList<string> ();

Just use a standard Vala array for removed_sysgroups, rather than a LinkedList. It’ll be a lot faster. Same for added_sysgroups.

Created attachment 228179 [details] [review]
eds: expose Google system groups in the public API

This will allow gnome-contacts to change it directly (to switch a contact
from My Contacts to Other and back)

(Please note that this still depends on the associated e-d-s bug)

Review of attachment 228179 [details] [review]:

Looks good to me.

Attachment 228179 [details] pushed as 49760b6 - eds: expose Google system groups in the public API