Branch based on the one for bug #32609: http://git.collabora.co.uk/?p=user/xclaesse/telepathy-glib.git;a=shortlog;h=refs/heads/contact-groups

I'm working on unit test ;)

done :)

And now with tp_contact_set_contact_groups_async(), including unit tests for it :)

Branch ready for review

+    /* ContactGroups */
+    GPtrArray *contact_groups;
Please document the content of this array and own the memory is managed.

+ * @n_groups: the length of @groups, or -1 if @groups is %NULL-terminated
+ * @groups: (array length=n_groups) (allow-none): the set of groups which the
+ *  contact should be in (may be %NULL if @n_groups is 0)

Is this binding friendly? Maybe it is, I'm just wondering.


+  g_return_if_fail (groups != NULL || n_groups == 0);
strictly speaking, groups == NULL && n_groups == -1 isn't valid as well? :)

+ * Finishes an async set of @self contact groups. If the operation was
+ * successful, the contact's groups can be accessed using
+ * tp_contact_get_contact_groups().

This is a bit miss leading. tp_contact_get_contact_groups() can  still be used
even if the call failed. It will just return the old groups.

You should use _tp_implement_finish_* macros to implement your finish
functions.

   * This is set to %NULL if %TP_CONTACT_FEATURE_CONTACT_GROUPS is not set
+   * on this contact,
"is not prepared" ?

Did you valgrind your tests?

+      for (iter = removed; *iter != NULL; iter++)
+        for (j = 0; j < contact->priv->contact_groups->len; j++)
I'd 'add {} for extra clarity.

contact_maybe_set_contact_groups() arithmetic on pointers scares me, I would
have used an iterator, but I guess that's fine.


Can't contact_maybe_set_contact_groups() be called twice and so hit the
g_assert()?

(In reply to comment #6)
> +    /* ContactGroups */
> +    GPtrArray *contact_groups;
> Please document the content of this array and own the memory is managed.

Fixed

> + * @n_groups: the length of @groups, or -1 if @groups is %NULL-terminated
> + * @groups: (array length=n_groups) (allow-none): the set of groups which the
> + *  contact should be in (may be %NULL if @n_groups is 0)
> 
> Is this binding friendly? Maybe it is, I'm just wondering.

Should be, yes. Now I changed a bit to do like in tp_base_contact_list_groups_created()

> 
> +  g_return_if_fail (groups != NULL || n_groups == 0);
> strictly speaking, groups == NULL && n_groups == -1 isn't valid as well? :)

For tp_base_contact_list_groups_created() NULL is accepted if -1, but for features in tp_connection_get_contacts_by_id() not. I changed to match the former since it is the only I know for string array.

> + * Finishes an async set of @self contact groups. If the operation was
> + * successful, the contact's groups can be accessed using
> + * tp_contact_get_contact_groups().
> 
> This is a bit miss leading. tp_contact_get_contact_groups() can  still be used
> even if the call failed. It will just return the old groups.

ok, removed that part

> You should use _tp_implement_finish_* macros to implement your finish
> functions.

indeed, fixed.

>    * This is set to %NULL if %TP_CONTACT_FEATURE_CONTACT_GROUPS is not set
> +   * on this contact,
> "is not prepared" ?

fixed

> Did you valgrind your tests?

How do I run it with valgrind?

> +      for (iter = removed; *iter != NULL; iter++)
> +        for (j = 0; j < contact->priv->contact_groups->len; j++)
> I'd 'add {} for extra clarity.

done

> contact_maybe_set_contact_groups() arithmetic on pointers scares me, I would
> have used an iterator, but I guess that's fine.

done

> Can't contact_maybe_set_contact_groups() be called twice and so hit the
> g_assert()?

I don't think it can because it's called only when feature gets prepared and that can happen only once. However other contact_maybe_set functions don't make sure assert and just free the value if already set. So I modified to do that as well.

(In reply to comment #7)
> > + * @n_groups: the length of @groups, or -1 if @groups is %NULL-terminated
> > + * @groups: (array length=n_groups) (allow-none): the set of groups which the
> > + *  contact should be in (may be %NULL if @n_groups is 0)
> > 
> > Is this binding friendly? Maybe it is, I'm just wondering.
> 
> Should be, yes. Now I changed a bit to do like in
> tp_base_contact_list_groups_created()

gir will understand the -1 if @groups is %NULL-terminated logic?
Maybe check with Tomeu just to be sure that this API is gir friendly?

> > Did you valgrind your tests?
> 
> How do I run it with valgrind?

make -C tests/dbus check-valgrind TESTS=test-badger

(In reply to comment #8)
> (In reply to comment #7)
> > > + * @n_groups: the length of @groups, or -1 if @groups is %NULL-terminated
> > > + * @groups: (array length=n_groups) (allow-none): the set of groups which the
> > > + *  contact should be in (may be %NULL if @n_groups is 0)
> > > 
> > > Is this binding friendly? Maybe it is, I'm just wondering.
> > 
> > Should be, yes. Now I changed a bit to do like in
> > tp_base_contact_list_groups_created()
> 
> gir will understand the -1 if @groups is %NULL-terminated logic?
> Maybe check with Tomeu just to be sure that this API is gir friendly?

It don't need to understand the -1, bindings will always give len value. The -1 is only extra convenience for C apps.

> > > Did you valgrind your tests?
> > 
> > How do I run it with valgrind?
> 
> make -C tests/dbus check-valgrind TESTS=test-badger

When passing only the contact-groups it works fine, but it seems avatar test case has leaks.

Ok; check with a real tp-glib person (smcv, wjt) for the API and it's fine with me.

Why is tp_contact_set_contact_groups_finish() written out in longhand rather than using a beautiful macro? but this looks fine API-wise to me.

(In reply to comment #11)
> Why is tp_contact_set_contact_groups_finish() written out in longhand rather
> than using a beautiful macro? but this looks fine API-wise to me.

This is already fixed in 691a7d7095cae86d75bca374203f660a295effb5

Branch merged, thanks for the reviews.