Bug 20829

Implemented in http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=shortlog;h=refs/heads/contact-search  (based on Will's old branch).
Needs a review.

+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);

?

--

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.

--

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.

--

  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?

--

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.

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 ().

--

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 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.

--

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");

--

/* FIXME: it's unclear how "first" and "last" should be mapped.
 * http://xmpp.org/registrar/formtypes.html#jabber:iq:search 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.
--

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.)

--

+    # Openfire only supports one text field and a bunch on checkbox

a bunch *of* checkboxes

--

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.

--

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.

--

parse_data_form:

  gboolean found_form_type_search = FALSE;
          found_form_type_search = TRUE;

but then it's never actually used again.

      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?

--

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. :)

======

Here endeth the review!

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.

FYI, psa has confirmed that "first" and "last" names in the XEP are really meant to mean "given" and "family", and will be adding some Chinese examples to make this clearer.

(In reply to comment #5)
> FYI, psa has confirmed that "first" and "last" names in the XEP are really
> meant to mean "given" and "family", and will be adding some Chinese examples to
> make this clearer.
> 

Thanks; I updated the comment in the code to reflect that.

(In reply to comment #4)
> > 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.

Having done this, _close_all () shouldn't destroy the HT. finalize can assert that it's empty, and then destroy it. Then nothing has to deal with it possibly being NULL.

That's a nitpick though; otherwise ++

Fixed in http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=shortlog;h=refs/heads/contact-search

Merged; will be in 0.9.0.