[Bug 30296] Add Addressing support to Gabble

[bugzilla-daemon at freedesktop.org]

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

--- Comment #7 from Eitan Isaacson <eitan.isaacson at collabora.co.uk> 2010-09-28 14:56:15 PDT ---
(In reply to comment #3)
> 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.

d9c0f21

> 
> +#define DEBUG_FLAG GABBLE_DEBUG_PROPERTIES
> 
> Make a new debug flag?

75ed12f

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

busted! 77e76e9

> +        "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?
> 

busted again! 77e76e9

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

Seems like most tp-glib mixins have this in the constructor. Uh oh, busted!
again! 77e76e9

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

Good catch 267a11b

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

0199ce5

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

Yeah, this will need to be redone.

> +  /* 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;

fd2eb00

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

Yeah, the stuff in gdummyfile.c would actually be useful.

> 
> +  for (i=0;i<len;i++)
> +  for (field=addressable_vcard_fields;*field!=NULL;field++)
> 
> Nitpicking: there should be more spaces in these.

0316d17

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

Yeah, I could probably add some comments there.

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

You would think. The function that this functions serves is *_requestotron,
which differentiates between returning FALSE and an error. It's confusing, and
I would like to see this function go away, that is why tp-glib should translate
addressing requests like TargetID.

> +++ b/src/conn-addressing.c
> @@ -0,0 +1,216 @@
> +/*
> + * conn-addressing.h - Header for Gabble connection code handling addressing.
> 
> That is no header!

c8ff335

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

This is for bug 30310.

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

I agree. Alternatives? The issue is that one consumer of the error is
Create/Ensure where NotImplemented is an appropriate error. And the other
consumer is Conn.I.Addressing.GetContactsByVCardField, where NotImplemented
would be very confusing. The UI might assume that the CM has a partial
Addressing implementation.

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

77df07f

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