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

bugzilla-daemon at freedesktop.org bugzilla-daemon at freedesktop.org
Mon Aug 24 16:38:54 CEST 2009


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





--- Comment #2 from Will Thompson <will.thompson at collabora.co.uk>  2009-08-24 07:38:53 PST ---

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

--


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



More information about the telepathy-bugs mailing list