[Bug 32702] TpContact should have properties for ContactGroups

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=32702

--- Comment #7 from Xavier Claessens <xclaesse at gmail.com> 2011-02-20 04:12:37 PST ---
(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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.