[Bug 32053] Add a TpContactSearch proxy object

[bugzilla-daemon at freedesktop.org]

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

--- Comment #27 from Emilio Pozuelo Monfort <pochu27 at gmail.com> 2011-01-18 06:52:01 PST ---
Thanks for the review.

(In reply to comment #26)
> /**
>  * TpContactSearchResult:
>  *
>  * A proxy object for the results of a Telepathy contact
> 
> It's not a proxy (in the sense of a TpProxy).
> Same for TpContactSearch.

Fixed

> struct _TpContactSearchResultPrivate
>   GList *fields;
> Please add a comment explaining the content of the list and if it's owned or
> borrowed.
> 
> 
>      case PROP_IDENTIFIER:
>         g_free (self->priv->identifier);
> As it's construct only, we ususuall don't free and use:
> g_assert (self->priv->identifier == NULL);  /* construct-only */

Fixed

> tp_contact_search_result_get_fields
> Please document that the returned list should be free using g_list_free()

Done

> tests/contact-search-result.c
> is leaky, use check-valgrind to check for leaks.

Fixed

> * They also abstract the creation of contact search channels, but you need
>  * to call @tp_contact_search_reset_async between searches.
> This isn't very clear imho. I'd focus on explaining how the object should be
> used; channel creation is a TP implementation detail.

I've clarified this a bit.

> Use DEBUG() instead of g_print().

Done

>   request = tp_asv_new (
>       TP_PROP_CHANNEL_CHANNEL_TYPE,
>       G_TYPE_STRING,
>       TP_IFACE_CHANNEL_TYPE_CONTACT_SEARCH,
>       NULL);
>   if (self->priv->server != NULL)
>     tp_asv_set_string (request,
>         TP_PROP_CHANNEL_TYPE_CONTACT_SEARCH_SERVER,
>         self->priv->server);
>   if (self->priv->limit != 0)
>     tp_asv_set_uint32 (request,
>       TP_PROP_CHANNEL_TYPE_CONTACT_SEARCH_LIMIT,
>       self->priv->limit);
> Add some \n between those.

Done

>         self->priv->account = g_object_ref (g_value_get_object (value));
> Use g_value_dup_object()

Done

> account and server params are contruct only so please add an assertion as
> explained above.

Done

>    * use @tp_capabilities_supports_contact_search to check if it's
> syntax is "use tp_capabilities_supports_contact_search() to check.."
> Same for other doc comments reffering to functions.

All fixed

My gtk-doc foo is (slowly) getting better... :)

>       g_cancellable_cancel (self->priv->cancellable);
> 
>       while (self->priv->async_res != NULL)
>         g_main_context_iteration (NULL, TRUE);
> This is a bit weird; maybe we should request a g_cancellable_cancel_async() in
> glib?
> Do we really need to wait btw ?

As it is now, yes, since we use the same GCancellable, so we need to wait and
reset it. Maybe we could have a list of GCancellables and just cancel it
without waiting, or maybe we can cancel it and unref it (if gio cancells it
before disposing the GCancellable), so we don't need to keep a list. The
current code works though.

> tp_contact_search_start(): shouldn't we check if another search is already
> active?

We do:

  g_return_if_fail (self->priv->state ==
      TP_CHANNEL_CONTACT_SEARCH_STATE_NOT_STARTED);


> +  if (with_limit)
> if (with_limit != NULL)
> 
> +      if (!tp_strdiff (chan_type, TP_IFACE_CHANNEL_TYPE_CONTACT_SEARCH))
> use "continue" to limit nesting.

I cannot use "continue" there since I have to check both with_limit and
with_server, and if I continue in the first one, I won't get to check the
second one. I could do

if (with_limit != NULL && !tp_strdiff (...))

to avoid one level. Not sure if it's worth it...

> tp_capabilities_supports_contact_search: you can use tp_value_array_unpack()

Done

> Having test for TpContactSearch would make me an happier badger...

I tried to look at this, but apparently I need to create some classes for the
DBus tests and I couldn't really understand how that stuff works.

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