[Bug 30296] Add Addressing support to Gabble

[bugzilla-daemon at freedesktop.org]

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

--- Comment #3 from Will Thompson <will.thompson at collabora.co.uk> 2010-09-23 12:59:15 PDT ---
This is going to be a pretty immense comment. :) First up, general comments on
the design and your points above:

I agree that most of this should live in telepathy-glib, as you say. I like the
design here a lot: it makes me happy that the changes to channel (manager)
implementations were pretty small.

For channel requests: one other tactic we could use would be to make
telepathy-glib's Requests infrastructure transform target addresses/URIs to
TargetHandle internally, just like it already transforms TargetID to
TargetHandle internally. This would do the job for Gabble at least;]

Regarding static arrays of interfaces: one possible other way to deal with this
I noticed in telepathy-ring is to have #defines that expand to a bunch of
standard allowed properties/interfaces. Then we could easily define three
different arrays of properties rather than having to do these error-prone
offset tricks.

The RCC explosion is a bit surprising, but I don't think it's actively bad. The
test situation is a pain, though. I saw you changed some assertLength(1, ...)s
to assertLength(4, ..)... I'm not sure how best we should deal with this.

‘...using the addressing bits for
requests requires handle type to be omitted.’: i think that's fine. you might
not know up-front what handle type the URI corresponds to. You could have an
XMPP URI for a MUC. (I don't know if such a beast exists, the internet
connection here isn't working.)

===

Here are some nitpicks on the implementation itself:

+ * addressing-mixin.c - Source for GabbleAddressingMixin
+ * Copyright (C) 2006-2008 Collabora Ltd.
+ * Copyright (C) 2006-2008 Nokia Corporation

These copyright dates are not right.

+#define DEBUG_FLAG GABBLE_DEBUG_PROPERTIES

Make a new debug flag?

+#define GABBLE_ADDRESSING_MIXIN_OFFSET(o) \
+  (GPOINTER_TO_UINT (g_type_get_qdata (G_OBJECT_TYPE (o), \
+                                       GABBLE_ADDRESSING_MIXIN_OFFSET_QUARK)))

I think this should probably be GPOINTER_TO_SIZE(), but I assume this is
copypasta from somewhere.

+        "gabble_addressing_mixin_get_offset_quark at 0.7.7");

I guess the number here is the version of tp-glib in which some other mixin was
added there?

+  g_type_set_qdata (G_OBJECT_TYPE (obj),
+                    GABBLE_ADDRESSING_MIXIN_OFFSET_QUARK,
+                    GINT_TO_POINTER (offset));

Shouldn't this go in a mixin_class_init() rather than in an instance init? We
don't need to re-annotate the type using this mixin every time an instance is
constructed: the mixin's always at the same offset into the struct. Maybe
_init_dbus_properties() could become _class_init() and take

+  TpHandleRepoIface *contact_handles =
+    tp_base_connection_get_handles (base_conn, TP_HANDLE_TYPE_CONTACT);
+
+  DEBUG ("%p", obj);
+
+  g_object_unref (mixin->priv->connection);
+
+  if (mixin->priv->target != 0)
+    tp_handle_unref (contact_handles, mixin->priv->target);

I think it's probably not kosher to use the handle repo borrowed from the
connection after unreffing the connection.

gabble_addressing_mixin_get_dbus_property should have an else clause that does
something useful like g_return_if_reached().

+  if (name == q_target_vcard_field)
+    {
+      g_value_set_static_string (value, vcard_fields[0]);

I guess the idea is that, in a CM that supported more than one vCard
field­—like telepathy-sofiasip—you'd pass which field is actually being used to
addressing_mixin_init(), rather than just always picking the first one?

+  /* excuse the poor man's URI parsing, couldn't find a GLib helper */
+  gchar **tokenized_uri = g_strsplit(uri, ":", 2);

Yeah, glib has g_uri_parse_scheme(), and I guess you could do something like:

  gchar *scheme = g_uri_parse_scheme (uri);
  const gchar *rest = uri + strlen (scheme) + 1;

which is a bit ghetto too. And then gio/gnetworkaddress.h has some
almost-useful functions. And actually glib does secretly have a URI parser in
gio/gdummyfile.c which handles queries and fragments and everything, but it's
not public. (I found this yesterday because it's actually buggy and breaks
clicking on certain perfectly-valid links in Empathy. Sigh.)

+  for (i=0;i<len;i++)
+  for (field=addressable_vcard_fields;*field!=NULL;field++)

Nitpicking: there should be more spaces in these.

gabble_make_addressing_requestable_properties_foreach_channel_class

Wow. This function is pretty magical. I see what it's there for—and it's good
that it reduces duplication—but I find it hard to follow. I'm not entirely sure
how to make it clearer; I'll have a think.

gabble_addressing_resolve_channel_target() probably wants to return FALSE if
neither condition is true?

+++ b/src/conn-addressing.c
@@ -0,0 +1,216 @@
+/*
+ * conn-addressing.h - Header for Gabble connection code handling addressing.

That is no header!

tp_contacts_mixin_get_contact_attributes(): I don't think you should have to
pass TP_IFACE_CONNECTION into it. tp-glib should make that assumption for us,
surely?

+          if (error->code == TP_ERROR_NOT_IMPLEMENTED)
+            {
+              error->code = TP_ERROR_INVALID_ARGUMENT;

A little distressing that the same condition leads to two different errors in
different places.

+    /* If more interfaces are added, either keep Group as the first, or change
+     * the implementations of gabble_tube_stream_get_interfaces () and
+     * gabble_tube_stream_get_property () too */

It looks like you've made the change the comment was asking for, so the comment
could be binned.

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