Bug 32702

Summary: TpContact should have properties for ContactGroups
Product: Telepathy Reporter: Xavier Claessens <xclaesse>
Component: tp-glibAssignee: Telepathy bugs list <telepathy-bugs>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/xclaesse/telepathy-glib.git;a=shortlog;h=refs/heads/contact-groups
Whiteboard:
i915 platform: i915 features:
Bug Depends on: 32609    
Bug Blocks:    

Description Xavier Claessens 2010-12-28 08:03:48 UTC
High-level API for ContactAttribute

org.freedesktop.Telepathy.Connection.Interface.ContactGroups/groups
Comment 2 Xavier Claessens 2010-12-28 08:07:27 UTC
I'm working on unit test ;)
Comment 3 Xavier Claessens 2010-12-29 03:19:57 UTC
done :)
Comment 4 Xavier Claessens 2010-12-31 09:02:56 UTC
And now with tp_contact_set_contact_groups_async(), including unit tests for it :)
Comment 5 Xavier Claessens 2011-01-29 14:17:47 UTC
Branch ready for review
Comment 6 Guillaume Desmottes 2011-01-31 02:44:11 UTC
+    /* 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()?
Comment 7 Xavier Claessens 2011-02-20 04:12:37 UTC
(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.
Comment 8 Guillaume Desmottes 2011-02-21 00:57:34 UTC
(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
Comment 9 Xavier Claessens 2011-02-21 01:47:10 UTC
(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.
Comment 10 Guillaume Desmottes 2011-02-21 01:56:20 UTC
Ok; check with a real tp-glib person (smcv, wjt) for the API and it's fine with me.
Comment 11 Will Thompson 2011-02-22 03:15:58 UTC
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.
Comment 12 Xavier Claessens 2011-02-22 03:27:31 UTC
(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.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.