[Bug 20829] Implement ContactSearch (telepathy-spec 0.17.22 version)

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=20829





--- Comment #4 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk>  2009-08-25 06:37:06 PST ---
contact search review

(In reply to comment #2)
> +static void
> +free_info (GPtrArray *info)
> +{
> +  GValue v = { 0, };
> +
> +  g_value_init (&v, GABBLE_ARRAY_TYPE_CONTACT_INFO_FIELD_LIST);
> +  g_value_take_boxed (&v, info);
> +  g_value_unset (&v);
> +}
> +
> 
> Can't this be replaced with
> 
>     g_boxed_free (GABBLE_ARRAY_TYPE_CONTACT_INFO_FIELD_LIST, info);

Indeed. Fixed.


> I've decided that:
> 
>   /* Use a temporary variable because we don't want search_channel_closed_cb to
>    * remove the channel from the hash table a second time
>    */:
>   if (self->priv->channels != NULL)
>     {
>       GHashTable *tmp = self->priv->channels;
> 
>       DEBUG ("closing channels");
>       self->priv->channels = NULL;
>       g_hash_table_destroy (tmp);
>     }
> 
> is an anti-pattern. close_all() should just call a close() method on all the
> channels, and then you don't have to do the stupid "move the hash table aside"
> dance. Plus, if you explicitly tell the channel to close you're not relying on
> owning the only reference to it, and it closing as a side-effect of being
> disposed.


Fixed.


> It'd be nice if we could delay the channel creation if we know that service
> discovery hasn't finished yet. I notice in disco.c:
> 
>     /* FIXME - service discovery done - signal that somehow */
> 
> Maybe it's worth making that happen (a "done" signal would presumably suffice)?
> Then the server == NULL case in gabble_search_manager_create_channel could
> check whether service discovery's finished, and if it's not hook up to the
> "done" signal and return from CreateChannel when it's fired? This could be kind
> of similar to how we wait on making streamed media channels until we're sure of
> the peer's caps, except less fragile and ad-hoc.


Done.

>   self->priv->status_changed_id = g_signal_connect (self->priv->conn,
>       "status-changed", (GCallback) connection_status_changed_cb, obj);
> 
> Can't this use gabble_signal_connect_object?

Done. (the signal wasn't ever disconnected btw...)


> 
> static void
> gabble_search_manager_foreach_channel (TpChannelManager *manager,
>                                    TpExportableChannelFunc func,
>                                    gpointer user_data)
> {
>   /* GabbleSearchManager *self = GABBLE_SEARCH_MANAGER (manager); */
> }
> 
> Bzzt. Sorry, guess I left out the FIXME there.

Oups, I missed that. Fixed and tested.

> Interestingly, this can't just iterate priv->channels, because we put
> not-ready-yet channels in there too. Maybe priv->channels should be split up;
> or, we could have gabble_search_channel_is_ready ().

Indeed. I added a _is_ready ()


> What do you think about:
> 
>           /* - Maybe CreateChannel should be specced to raise PermissionDenied?
>            *   Then we could map XMPP_ERROR_FORBIDDEN to that.
>            * - Should XMPP_ERROR_JID_MALFORMED be mapped to InvalidArgument?
>            */

I think that make sense. I added it in the spec and fixed implementation.


> I think the answer to:
> 
>           /* Do we want to prefix the error string with something? */
> is no, given that Daf fixed the XMPP error-to-GError code to actually use the
> error message from the XMPP stream if there is one.

I agree.


> If there's an outstanding request for a search channel when we disconnect, the
> CreateChannel method will never terminate, because the search channel will
> never emit ready-or-not.
> 
> I *think* the correct fix here is to make gabble_search_channel_close() look
> something like:
> 
>   if (!closed)
>       emit closed;
>   else if (hasn't emitted ready-or-not)
>       emit it with the appropriate error.
>   else
>       DEBUG ("nothing to do");

Actually no, tp-glib cancel the request for us. You even tested that case in 
eci-nest-pas-un-serveur.py : disconnected_before_reply  :)

> 
> /* FIXME: it's unclear how "first" and "last" should be mapped.
>  * http://xmpp.org/registrar/formtypes.html#jabber:iq:se
arch maps
>  * "first" with "First Name" and "last" with "Family Name".
>  * But Example 7 of XEP-0055 describes "first" as the "Given Name".
>  * Maybe we need to add x-first and x-last?
>  */
> 
> Given that "last" is defined to mean "family name", I think the intention was
> for "first" to mean "given name". I think the mapping is correct. The naming's
> wrong, but at least ejabberd gets it right with its data forms
> implementation.

Cool. Do you think we should remove this FIXME or let it while the XEP has been
clarified?


> --
> 
> Nitpicks:
> 
> What's the point of
> <http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=commitdiff;h=1b1bceceda>?
> The first thing that happens to 'h' is that tp_handle_ensure's return value is
> assigned to it, so I don't see the point of initializing it to 0. (And in more
> complicated situations, initializing variables suppresses the compiler's
> warnings about using variables uninitialized, which can save you if you forget
> to assign to a variable in one branch of an 'if' or something.)

I was needed in an old version of the branch but not any more. I removed it.

> 
> +    # Openfire only supports one text field and a bunch on checkbox
> 
> a bunch *of* checkboxes

fixed.

> 
> btw. we have assertEquals, assertLength, assertContains, etc. so
> that you don't have to say "assert foo == bar, (foo, bar)". Would be nice to
> use them, but I'm not sure whether it's worth going through all these tests
> now.

Tbh, I'm not really motivated to change that at this point :)


> I vaguely wonder whether channel managers should cache their channel classes so
> that we don't repeatedly build and free tiny hash tables whenever anyone looks
> at RCC.

Maybe. Could be done later to all the channels managers.


(In reply to comment #3)
> parse_data_form:
> 
>   gboolean found_form_type_search = FALSE;
>           found_form_type_search = TRUE;
> 
> but then it's never actually used again.

removed.

>       tp_name = g_hash_table_lookup (xmpp_to_tp, var);
>       if (tp_name != NULL)
>         {
>           g_ptr_array_add (search_keys, tp_name);
>           g_hash_table_insert (self->priv->tp_to_xmpp, g_strdup (tp_name),
>               g_strdup (var));
> 
> So what happens if a form contains two fields that map to the same Telepathy
> name? I think that the key will appear twice in AvailableSearchKeys, and when
> the client calls Search() the last field name seen will be used. The former is
> a slight bug but it's harmless; the latter is also valid. Maybe there should be
> a test to make sure Gabble doesn't implode when this happens, and if you're
> feeling really keen, code to avoid duplicates?

Tested and fixed


> add_search_result() builds the 'n' field from the separate given and family
> components. It should also use the middle name if present, maybe? Also, should
> we be constructing an 'org' field out of x-org-unit and x-org-name? Ditto
> 'adr'? "No" is a reasonable answer. :)

Tbh I have no idea. I don't know much  about vCard and just reused your
existing code. I can add that if you think we should.


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